From e7c2e09ef913dc801fc69f09b9fcce21ddb35b81 Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Mon, 2 Sep 2024 10:38:53 +0200
Subject: [PATCH 01/12] Systematically test allowed_groups, admin_groups, and
 manage_groups

---
 oauthenticator/tests/test_auth0.py     | 16 ++++++++++++++
 oauthenticator/tests/test_azuread.py   | 16 ++++++++++++++
 oauthenticator/tests/test_bitbucket.py | 15 +++++++++++++
 oauthenticator/tests/test_cilogon.py   | 16 ++++++++++++++
 oauthenticator/tests/test_generic.py   | 29 +++++++++++++-------------
 oauthenticator/tests/test_github.py    | 16 ++++++++++++++
 oauthenticator/tests/test_gitlab.py    | 16 ++++++++++++++
 oauthenticator/tests/test_globus.py    | 16 ++++++++++++++
 oauthenticator/tests/test_google.py    | 16 ++++++++++++++
 oauthenticator/tests/test_mediawiki.py | 16 ++++++++++++++
 oauthenticator/tests/test_openshift.py | 29 +++++++++++++-------------
 11 files changed, 173 insertions(+), 28 deletions(-)

diff --git a/oauthenticator/tests/test_auth0.py b/oauthenticator/tests/test_auth0.py
index e4ae3be9..4e6f5efe 100644
--- a/oauthenticator/tests/test_auth0.py
+++ b/oauthenticator/tests/test_auth0.py
@@ -29,6 +29,7 @@ def user_model():
     return {
         "email": "user1@example.com",
         "name": "user1",
+        "groups": ["group1"],
     }
 
 
@@ -62,6 +63,21 @@ def user_model():
             True,
             True,
         ),
+        # common tests with allowed_groups and manage_groups
+        ("20", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
+        (
+            "21",
+            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            None,
+        ),
+        ("22", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
+        (
+            "23",
+            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            False,
+        ),
     ],
 )
 async def test_auth0(
diff --git a/oauthenticator/tests/test_azuread.py b/oauthenticator/tests/test_azuread.py
index 2bf7606c..e2e6071c 100644
--- a/oauthenticator/tests/test_azuread.py
+++ b/oauthenticator/tests/test_azuread.py
@@ -49,6 +49,7 @@ def user_model(tenant_id, client_id, name):
                 "96000b2c-7333-4f6e-a2c3-e7608fa2d131",
                 "a992b3d5-1966-4af4-abed-6ef021417be4",
                 "ceb90a42-030f-44f1-a0c7-825b572a3b07",
+                "group1",
             ],
             # different from 'groups' for tests
             "grp": [
@@ -132,6 +133,21 @@ def user_model(tenant_id, client_id, name):
             True,
             None,
         ),
+        # common tests with allowed_groups and manage_groups
+        ("40", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
+        (
+            "41",
+            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            None,
+        ),
+        ("42", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
+        (
+            "43",
+            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            False,
+        ),
     ],
 )
 async def test_azuread(
diff --git a/oauthenticator/tests/test_bitbucket.py b/oauthenticator/tests/test_bitbucket.py
index 512e8030..f213b7a7 100644
--- a/oauthenticator/tests/test_bitbucket.py
+++ b/oauthenticator/tests/test_bitbucket.py
@@ -80,6 +80,21 @@ def user_model(username):
             True,
             True,
         ),
+        # common tests with allowed_groups and manage_groups
+        ("20", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
+        (
+            "21",
+            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            None,
+        ),
+        ("22", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
+        (
+            "23",
+            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            False,
+        ),
     ],
 )
 async def test_bitbucket(
diff --git a/oauthenticator/tests/test_cilogon.py b/oauthenticator/tests/test_cilogon.py
index 4dac4927..cc8a87eb 100644
--- a/oauthenticator/tests/test_cilogon.py
+++ b/oauthenticator/tests/test_cilogon.py
@@ -27,6 +27,7 @@ def user_model(username, username_claim, **kwargs):
     return {
         username_claim: username,
         "idp": "https://some-idp.com/login/oauth/authorize",
+        "groups": ["group1"],
         **kwargs,
     }
 
@@ -61,6 +62,21 @@ def user_model(username, username_claim, **kwargs):
             True,
             True,
         ),
+        # common tests with allowed_groups and manage_groups
+        ("20", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
+        (
+            "21",
+            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            None,
+        ),
+        ("22", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
+        (
+            "23",
+            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            False,
+        ),
     ],
 )
 async def test_cilogon(
diff --git a/oauthenticator/tests/test_generic.py b/oauthenticator/tests/test_generic.py
index 11da1026..222f3225 100644
--- a/oauthenticator/tests/test_generic.py
+++ b/oauthenticator/tests/test_generic.py
@@ -104,20 +104,6 @@ def get_authenticator_variant(generic_client, userdata_from_id_token):
         ("03", {"allowed_users": {"not-test-user"}}, False, None),
         ("04", {"admin_users": {"user1"}}, True, True),
         ("05", {"admin_users": {"not-test-user"}}, False, None),
-        ("06", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
-        (
-            "07",
-            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
-            False,
-            None,
-        ),
-        ("08", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
-        (
-            "09",
-            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
-            False,
-            False,
-        ),
         # allow config, some combinations of two tested
         (
             "10",
@@ -228,6 +214,21 @@ def get_authenticator_variant(generic_client, userdata_from_id_token):
             True,
             None,
         ),
+        # common tests with allowed_groups and manage_groups
+        ("20", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
+        (
+            "21",
+            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            None,
+        ),
+        ("22", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
+        (
+            "23",
+            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            False,
+        ),
     ],
 )
 async def test_generic(
diff --git a/oauthenticator/tests/test_github.py b/oauthenticator/tests/test_github.py
index 35b64a46..ce152148 100644
--- a/oauthenticator/tests/test_github.py
+++ b/oauthenticator/tests/test_github.py
@@ -21,6 +21,7 @@ def user_model(username):
         'id': 5,
         'login': username,
         'name': 'Hoban Washburn',
+        "groups": ["group1"],
     }
 
 
@@ -66,6 +67,21 @@ def github_client(client):
             True,
             True,
         ),
+        # common tests with allowed_groups and manage_groups
+        ("20", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
+        (
+            "21",
+            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            None,
+        ),
+        ("22", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
+        (
+            "23",
+            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            False,
+        ),
     ],
 )
 async def test_github(
diff --git a/oauthenticator/tests/test_gitlab.py b/oauthenticator/tests/test_gitlab.py
index f8d7ad3e..72cbc838 100644
--- a/oauthenticator/tests/test_gitlab.py
+++ b/oauthenticator/tests/test_gitlab.py
@@ -28,6 +28,7 @@ def user_model(username):
     return {
         "username": username,
         "id": id,
+        "groups": ["group1"],
     }
 
 
@@ -86,6 +87,21 @@ def mock_version_response(request):
             True,
             True,
         ),
+        # common tests with allowed_groups and manage_groups
+        ("20", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
+        (
+            "21",
+            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            None,
+        ),
+        ("22", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
+        (
+            "23",
+            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            False,
+        ),
     ],
 )
 async def test_gitlab(
diff --git a/oauthenticator/tests/test_globus.py b/oauthenticator/tests/test_globus.py
index 95ade113..044eafec 100644
--- a/oauthenticator/tests/test_globus.py
+++ b/oauthenticator/tests/test_globus.py
@@ -17,6 +17,7 @@ def user_model(username, **kwargs):
     """Return a user model"""
     return {
         "preferred_username": username,
+        "groups": ["group1"],
         **kwargs,
     }
 
@@ -279,6 +280,21 @@ async def save_auth_state(self, state):
             False,
             False,
         ),
+        # common tests with allowed_groups and manage_groups
+        ("20", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
+        (
+            "21",
+            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            None,
+        ),
+        ("22", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
+        (
+            "23",
+            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            False,
+        ),
     ],
 )
 async def test_globus(
diff --git a/oauthenticator/tests/test_google.py b/oauthenticator/tests/test_google.py
index 3aeb652d..505a4920 100644
--- a/oauthenticator/tests/test_google.py
+++ b/oauthenticator/tests/test_google.py
@@ -18,6 +18,7 @@ def user_model(email, username="user1", hd=None):
         'email': email,
         'custom': username,
         'verified_email': True,
+        'groups': ['group1'],
     }
     if hd:
         model['hd'] = hd
@@ -151,6 +152,21 @@ def google_client(client):
             False,
             False,
         ),
+        # common tests with allowed_groups and manage_groups
+        ("20", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
+        (
+            "21",
+            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            None,
+        ),
+        ("22", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
+        (
+            "23",
+            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            False,
+        ),
     ],
 )
 async def test_google(
diff --git a/oauthenticator/tests/test_mediawiki.py b/oauthenticator/tests/test_mediawiki.py
index 3510b74d..85f294f6 100644
--- a/oauthenticator/tests/test_mediawiki.py
+++ b/oauthenticator/tests/test_mediawiki.py
@@ -25,6 +25,7 @@ def post_token(request, context):
                 'iss': 'https://meta.wikimedia.org',
                 'iat': time.time(),
                 'nonce': request_nonce,
+                'groups': ['group1'],
             },
             'client_secret',
         ).encode()
@@ -74,6 +75,21 @@ def post_token(request, context):
             True,
             True,
         ),
+        # common tests with allowed_groups and manage_groups
+        ("20", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
+        (
+            "21",
+            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            None,
+        ),
+        ("22", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
+        (
+            "23",
+            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            False,
+        ),
     ],
 )
 async def test_mediawiki(
diff --git a/oauthenticator/tests/test_openshift.py b/oauthenticator/tests/test_openshift.py
index 8421cf06..a39316e3 100644
--- a/oauthenticator/tests/test_openshift.py
+++ b/oauthenticator/tests/test_openshift.py
@@ -37,20 +37,6 @@ def user_model():
         ("03", {"allowed_users": {"not-test-user"}}, False, None),
         ("04", {"admin_users": {"user1"}}, True, True),
         ("05", {"admin_users": {"not-test-user"}}, False, None),
-        ("06", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
-        (
-            "07",
-            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
-            False,
-            None,
-        ),
-        ("08", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
-        (
-            "09",
-            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
-            False,
-            False,
-        ),
         # allow config, some combinations of two tested
         (
             "10",
@@ -150,6 +136,21 @@ def user_model():
             False,
             False,
         ),
+        # common tests with allowed_groups and manage_groups
+        ("20", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
+        (
+            "21",
+            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            None,
+        ),
+        ("22", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
+        (
+            "23",
+            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            False,
+            False,
+        ),
     ],
 )
 async def test_openshift(

From 3d7961509ea9244efaccccf449da5e41be46be86 Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Tue, 3 Sep 2024 11:22:24 +0200
Subject: [PATCH 02/12] docs: update example for Generic to use non-deprecated
 config

---
 .../tutorials/provider-specific-setup/providers/generic.md      | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/source/tutorials/provider-specific-setup/providers/generic.md b/docs/source/tutorials/provider-specific-setup/providers/generic.md
index 386116af..965c0951 100644
--- a/docs/source/tutorials/provider-specific-setup/providers/generic.md
+++ b/docs/source/tutorials/provider-specific-setup/providers/generic.md
@@ -35,7 +35,7 @@ c.GenericOAuthenticator.userdata_url = "https://accounts.example.com/auth/realms
 #
 c.GenericOAuthenticator.scope = ["openid", "email", "groups"]
 c.GenericOAuthenticator.username_claim = "email"
-c.GenericOAuthenticator.claim_groups_key = "groups"
+c.GenericOAuthenticator.auth_state_groups_key = "groups"
 
 # Authorization
 # -------------

From 79ef40367bafdd3299d3a53410e1fba488fa4cda Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Tue, 3 Sep 2024 11:23:17 +0200
Subject: [PATCH 03/12] tests: verify deprecation message for
 GenericOAuthenticator.claim_groups_key

---
 oauthenticator/tests/test_generic.py | 45 ++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/oauthenticator/tests/test_generic.py b/oauthenticator/tests/test_generic.py
index 222f3225..e3225039 100644
--- a/oauthenticator/tests/test_generic.py
+++ b/oauthenticator/tests/test_generic.py
@@ -1,4 +1,5 @@
 import json
+import logging
 import re
 from functools import partial
 
@@ -502,3 +503,47 @@ async def test_check_allowed_no_auth_state(get_authenticator, name, allowed):
     # these are previously-allowed users who should pass until subsequent
     # this check is removed in JupyterHub 5
     assert await authenticator.check_allowed(name, None)
+
+
+@mark.parametrize(
+    "test_variation_id,class_config,expect_config,expect_loglevel,expect_message",
+    [
+        (
+            "claim_groups_key",
+            {"claim_groups_key": "groups", "manage_groups": True},
+            {"auth_state_groups_key": "oauth_user.groups"},
+            logging.WARNING,
+            "GenericOAuthenticator.claim_groups_key is deprecated since OAuthenticator 17.0, use GenericOAuthenticator.auth_state_groups_key instead",
+        ),
+    ],
+)
+async def test_deprecated_config(
+    caplog,
+    test_variation_id,
+    class_config,
+    expect_config,
+    expect_loglevel,
+    expect_message,
+):
+    """
+    Tests that a warning is emitted when using a deprecated config and that
+    configuring the old config ends up configuring the new config.
+    """
+    print(f"Running test variation id {test_variation_id}")
+    c = Config()
+    c.GenericOAuthenticator = Config(class_config)
+
+    test_logger = logging.getLogger('testlog')
+    if expect_loglevel == logging.ERROR:
+        with raises(ValueError, match=expect_message):
+            GenericOAuthenticator(config=c, log=test_logger)
+    else:
+        authenticator = GenericOAuthenticator(config=c, log=test_logger)
+        for key, value in expect_config.items():
+            assert getattr(authenticator, key) == value
+
+    captured_log_tuples = caplog.record_tuples
+    print(captured_log_tuples)
+
+    expected_log_tuple = (test_logger.name, expect_loglevel, expect_message)
+    assert expected_log_tuple in captured_log_tuples

From da158fe65741cdc6c21cdafe0dd7aebabc15723d Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Tue, 3 Sep 2024 11:48:24 +0200
Subject: [PATCH 04/12] tests: iterate on allowed_groups and admin_groups tests

---
 oauthenticator/tests/test_auth0.py     | 39 ++++++++++++++++++++---
 oauthenticator/tests/test_azuread.py   | 34 +++++++++++++++++---
 oauthenticator/tests/test_bitbucket.py | 40 ++++++++++++++++++++---
 oauthenticator/tests/test_cilogon.py   | 44 ++++++++++++++++++++++----
 oauthenticator/tests/test_github.py    | 39 ++++++++++++++++++++---
 oauthenticator/tests/test_gitlab.py    | 39 ++++++++++++++++++++---
 oauthenticator/tests/test_globus.py    | 39 ++++++++++++++++++++---
 oauthenticator/tests/test_google.py    | 39 ++++++++++++++++++++---
 oauthenticator/tests/test_mediawiki.py | 39 ++++++++++++++++++++---
 oauthenticator/tests/test_openshift.py | 39 ++++++++++++++++++++---
 10 files changed, 341 insertions(+), 50 deletions(-)

diff --git a/oauthenticator/tests/test_auth0.py b/oauthenticator/tests/test_auth0.py
index 4e6f5efe..78a9b42f 100644
--- a/oauthenticator/tests/test_auth0.py
+++ b/oauthenticator/tests/test_auth0.py
@@ -64,17 +64,43 @@ def user_model():
             True,
         ),
         # common tests with allowed_groups and manage_groups
-        ("20", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
+        (
+            "20",
+            {
+                "allowed_groups": {"group1"},
+                "auth_state_groups_key": "auth0_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            None,
+        ),
         (
             "21",
-            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            {
+                "allowed_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "auth0_user.groups",
+                "manage_groups": True,
+            },
             False,
             None,
         ),
-        ("22", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
+        (
+            "22",
+            {
+                "admin_groups": {"group1"},
+                "auth_state_groups_key": "auth0_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            True,
+        ),
         (
             "23",
-            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            {
+                "admin_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "auth0_user.groups",
+                "manage_groups": True,
+            },
             False,
             False,
         ),
@@ -100,7 +126,10 @@ async def test_auth0(
 
     if expect_allowed:
         assert auth_model
-        assert set(auth_model) == {"name", "admin", "auth_state"}
+        if authenticator.manage_groups:
+            assert set(auth_model) == {"name", "admin", "auth_state", "groups"}
+        else:
+            assert set(auth_model) == {"name", "admin", "auth_state"}
         assert auth_model["admin"] == expect_admin
         auth_state = auth_model["auth_state"]
         assert json.dumps(auth_state)
diff --git a/oauthenticator/tests/test_azuread.py b/oauthenticator/tests/test_azuread.py
index e2e6071c..3a83dc8e 100644
--- a/oauthenticator/tests/test_azuread.py
+++ b/oauthenticator/tests/test_azuread.py
@@ -134,17 +134,43 @@ def user_model(tenant_id, client_id, name):
             None,
         ),
         # common tests with allowed_groups and manage_groups
-        ("40", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
+        (
+            "40",
+            {
+                "allowed_groups": {"group1"},
+                "auth_state_groups_key": "user.groups",
+                "manage_groups": True,
+            },
+            True,
+            None,
+        ),
         (
             "41",
-            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            {
+                "allowed_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "user.groups",
+                "manage_groups": True,
+            },
             False,
             None,
         ),
-        ("42", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
+        (
+            "42",
+            {
+                "admin_groups": {"group1"},
+                "auth_state_groups_key": "user.groups",
+                "manage_groups": True,
+            },
+            True,
+            True,
+        ),
         (
             "43",
-            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            {
+                "admin_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "user.groups",
+                "manage_groups": True,
+            },
             False,
             False,
         ),
diff --git a/oauthenticator/tests/test_bitbucket.py b/oauthenticator/tests/test_bitbucket.py
index f213b7a7..37d776ce 100644
--- a/oauthenticator/tests/test_bitbucket.py
+++ b/oauthenticator/tests/test_bitbucket.py
@@ -45,6 +45,7 @@ def user_model(username):
     """
     return {
         "username": username,
+        "groups": ["group1"],
     }
 
 
@@ -81,17 +82,43 @@ def user_model(username):
             True,
         ),
         # common tests with allowed_groups and manage_groups
-        ("20", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
+        (
+            "20",
+            {
+                "allowed_groups": {"group1"},
+                "auth_state_groups_key": "bitbucket_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            None,
+        ),
         (
             "21",
-            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            {
+                "allowed_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "bitbucket_user.groups",
+                "manage_groups": True,
+            },
             False,
             None,
         ),
-        ("22", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
+        (
+            "22",
+            {
+                "admin_groups": {"group1"},
+                "auth_state_groups_key": "bitbucket_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            True,
+        ),
         (
             "23",
-            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            {
+                "admin_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "bitbucket_user.groups",
+                "manage_groups": True,
+            },
             False,
             False,
         ),
@@ -116,7 +143,10 @@ async def test_bitbucket(
 
     if expect_allowed:
         assert auth_model
-        assert set(auth_model) == {"name", "admin", "auth_state"}
+        if authenticator.manage_groups:
+            assert set(auth_model) == {"name", "admin", "auth_state", "groups"}
+        else:
+            assert set(auth_model) == {"name", "admin", "auth_state"}
         assert auth_model["admin"] == expect_admin
         auth_state = auth_model["auth_state"]
         assert json.dumps(auth_state)
diff --git a/oauthenticator/tests/test_cilogon.py b/oauthenticator/tests/test_cilogon.py
index cc8a87eb..2b830a43 100644
--- a/oauthenticator/tests/test_cilogon.py
+++ b/oauthenticator/tests/test_cilogon.py
@@ -63,17 +63,43 @@ def user_model(username, username_claim, **kwargs):
             True,
         ),
         # common tests with allowed_groups and manage_groups
-        ("20", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
+        (
+            "20",
+            {
+                "allowed_groups": {"group1"},
+                "auth_state_groups_key": "cilogon_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            None,
+        ),
         (
             "21",
-            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            {
+                "allowed_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "cilogon_user.groups",
+                "manage_groups": True,
+            },
             False,
             None,
         ),
-        ("22", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
+        (
+            "22",
+            {
+                "admin_groups": {"group1"},
+                "auth_state_groups_key": "cilogon_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            True,
+        ),
         (
             "23",
-            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            {
+                "admin_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "cilogon_user.groups",
+                "manage_groups": True,
+            },
             False,
             False,
         ),
@@ -104,7 +130,10 @@ async def test_cilogon(
 
     if expect_allowed:
         assert auth_model
-        assert set(auth_model) == {"name", "admin", "auth_state"}
+        if authenticator.manage_groups:
+            assert set(auth_model) == {"name", "admin", "auth_state", "groups"}
+        else:
+            assert set(auth_model) == {"name", "admin", "auth_state"}
         assert auth_model["admin"] == expect_admin
         auth_state = auth_model["auth_state"]
         assert json.dumps(auth_state)
@@ -432,7 +461,10 @@ async def test_cilogon_allowed_idps(
 
     if expect_allowed:
         assert auth_model
-        assert set(auth_model) == {"name", "admin", "auth_state"}
+        if authenticator.manage_groups:
+            assert set(auth_model) == {"name", "admin", "auth_state", "groups"}
+        else:
+            assert set(auth_model) == {"name", "admin", "auth_state"}
         assert auth_model["admin"] == expect_admin
         auth_state = auth_model["auth_state"]
         assert json.dumps(auth_state)
diff --git a/oauthenticator/tests/test_github.py b/oauthenticator/tests/test_github.py
index ce152148..e49fe064 100644
--- a/oauthenticator/tests/test_github.py
+++ b/oauthenticator/tests/test_github.py
@@ -68,17 +68,43 @@ def github_client(client):
             True,
         ),
         # common tests with allowed_groups and manage_groups
-        ("20", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
+        (
+            "20",
+            {
+                "allowed_groups": {"group1"},
+                "auth_state_groups_key": "github_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            None,
+        ),
         (
             "21",
-            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            {
+                "allowed_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "github_user.groups",
+                "manage_groups": True,
+            },
             False,
             None,
         ),
-        ("22", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
+        (
+            "22",
+            {
+                "admin_groups": {"group1"},
+                "auth_state_groups_key": "github_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            True,
+        ),
         (
             "23",
-            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            {
+                "admin_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "github_user.groups",
+                "manage_groups": True,
+            },
             False,
             False,
         ),
@@ -103,7 +129,10 @@ async def test_github(
 
     if expect_allowed:
         assert auth_model
-        assert set(auth_model) == {"name", "admin", "auth_state"}
+        if authenticator.manage_groups:
+            assert set(auth_model) == {"name", "admin", "auth_state", "groups"}
+        else:
+            assert set(auth_model) == {"name", "admin", "auth_state"}
         assert auth_model["admin"] == expect_admin
         auth_state = auth_model["auth_state"]
         assert json.dumps(auth_state)
diff --git a/oauthenticator/tests/test_gitlab.py b/oauthenticator/tests/test_gitlab.py
index 72cbc838..86d0c891 100644
--- a/oauthenticator/tests/test_gitlab.py
+++ b/oauthenticator/tests/test_gitlab.py
@@ -88,17 +88,43 @@ def mock_version_response(request):
             True,
         ),
         # common tests with allowed_groups and manage_groups
-        ("20", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
+        (
+            "20",
+            {
+                "allowed_groups": {"group1"},
+                "auth_state_groups_key": "gitlab_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            None,
+        ),
         (
             "21",
-            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            {
+                "allowed_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "gitlab_user.groups",
+                "manage_groups": True,
+            },
             False,
             None,
         ),
-        ("22", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
+        (
+            "22",
+            {
+                "admin_groups": {"group1"},
+                "auth_state_groups_key": "gitlab_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            True,
+        ),
         (
             "23",
-            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            {
+                "admin_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "gitlab_user.groups",
+                "manage_groups": True,
+            },
             False,
             False,
         ),
@@ -123,7 +149,10 @@ async def test_gitlab(
 
     if expect_allowed:
         assert auth_model
-        assert set(auth_model) == {"name", "admin", "auth_state"}
+        if authenticator.manage_groups:
+            assert set(auth_model) == {"name", "admin", "auth_state", "groups"}
+        else:
+            assert set(auth_model) == {"name", "admin", "auth_state"}
         assert auth_model["admin"] == expect_admin
         auth_state = auth_model["auth_state"]
         assert json.dumps(auth_state)
diff --git a/oauthenticator/tests/test_globus.py b/oauthenticator/tests/test_globus.py
index 044eafec..906de7c2 100644
--- a/oauthenticator/tests/test_globus.py
+++ b/oauthenticator/tests/test_globus.py
@@ -281,17 +281,43 @@ async def save_auth_state(self, state):
             False,
         ),
         # common tests with allowed_groups and manage_groups
-        ("20", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
+        (
+            "20",
+            {
+                "allowed_groups": {"group1"},
+                "auth_state_groups_key": "globus_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            None,
+        ),
         (
             "21",
-            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            {
+                "allowed_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "globus_user.groups",
+                "manage_groups": True,
+            },
             False,
             None,
         ),
-        ("22", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
+        (
+            "22",
+            {
+                "admin_groups": {"group1"},
+                "auth_state_groups_key": "globus_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            True,
+        ),
         (
             "23",
-            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            {
+                "admin_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "globus_user.groups",
+                "manage_groups": True,
+            },
             False,
             False,
         ),
@@ -316,7 +342,10 @@ async def test_globus(
 
     if expect_allowed:
         assert auth_model
-        assert set(auth_model) == {"name", "admin", "auth_state"}
+        if authenticator.manage_groups:
+            assert set(auth_model) == {"name", "admin", "auth_state", "groups"}
+        else:
+            assert set(auth_model) == {"name", "admin", "auth_state"}
         assert auth_model["admin"] == expect_admin
         auth_state = auth_model["auth_state"]
         assert json.dumps(auth_state)
diff --git a/oauthenticator/tests/test_google.py b/oauthenticator/tests/test_google.py
index 505a4920..7dc7c35f 100644
--- a/oauthenticator/tests/test_google.py
+++ b/oauthenticator/tests/test_google.py
@@ -153,17 +153,43 @@ def google_client(client):
             False,
         ),
         # common tests with allowed_groups and manage_groups
-        ("20", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
+        (
+            "20",
+            {
+                "allowed_groups": {"group1"},
+                "auth_state_groups_key": "google_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            None,
+        ),
         (
             "21",
-            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            {
+                "allowed_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "google_user.groups",
+                "manage_groups": True,
+            },
             False,
             None,
         ),
-        ("22", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
+        (
+            "22",
+            {
+                "admin_groups": {"group1"},
+                "auth_state_groups_key": "google_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            True,
+        ),
         (
             "23",
-            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            {
+                "admin_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "google_user.groups",
+                "manage_groups": True,
+            },
             False,
             False,
         ),
@@ -191,7 +217,10 @@ async def test_google(
 
     if expect_allowed:
         assert auth_model
-        assert set(auth_model) == {"name", "admin", "auth_state"}
+        if authenticator.manage_groups:
+            assert set(auth_model) == {"name", "admin", "auth_state", "groups"}
+        else:
+            assert set(auth_model) == {"name", "admin", "auth_state"}
         assert auth_model["admin"] == expect_admin
         auth_state = auth_model["auth_state"]
         assert json.dumps(auth_state)
diff --git a/oauthenticator/tests/test_mediawiki.py b/oauthenticator/tests/test_mediawiki.py
index 85f294f6..417ec7e5 100644
--- a/oauthenticator/tests/test_mediawiki.py
+++ b/oauthenticator/tests/test_mediawiki.py
@@ -76,17 +76,43 @@ def post_token(request, context):
             True,
         ),
         # common tests with allowed_groups and manage_groups
-        ("20", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
+        (
+            "20",
+            {
+                "allowed_groups": {"group1"},
+                "auth_state_groups_key": "mediawiki_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            None,
+        ),
         (
             "21",
-            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            {
+                "allowed_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "mediawiki_user.groups",
+                "manage_groups": True,
+            },
             False,
             None,
         ),
-        ("22", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
+        (
+            "22",
+            {
+                "admin_groups": {"group1"},
+                "auth_state_groups_key": "mediawiki_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            True,
+        ),
         (
             "23",
-            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            {
+                "admin_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "mediawiki_user.groups",
+                "manage_groups": True,
+            },
             False,
             False,
         ),
@@ -117,7 +143,10 @@ async def test_mediawiki(
 
     if expect_allowed:
         assert auth_model
-        assert set(auth_model) == {"name", "admin", "auth_state"}
+        if authenticator.manage_groups:
+            assert set(auth_model) == {"name", "admin", "auth_state", "groups"}
+        else:
+            assert set(auth_model) == {"name", "admin", "auth_state"}
         assert auth_model["admin"] == expect_admin
         auth_state = auth_model["auth_state"]
         assert json.dumps(auth_state)
diff --git a/oauthenticator/tests/test_openshift.py b/oauthenticator/tests/test_openshift.py
index a39316e3..64504cc9 100644
--- a/oauthenticator/tests/test_openshift.py
+++ b/oauthenticator/tests/test_openshift.py
@@ -137,17 +137,43 @@ def user_model():
             False,
         ),
         # common tests with allowed_groups and manage_groups
-        ("20", {"allowed_groups": {"group1"}, "manage_groups": True}, True, None),
+        (
+            "20",
+            {
+                "allowed_groups": {"group1"},
+                "auth_state_groups_key": "openshift_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            None,
+        ),
         (
             "21",
-            {"allowed_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            {
+                "allowed_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "openshift_user.groups",
+                "manage_groups": True,
+            },
             False,
             None,
         ),
-        ("22", {"admin_groups": {"group1"}, "manage_groups": True}, True, True),
+        (
+            "22",
+            {
+                "admin_groups": {"group1"},
+                "auth_state_groups_key": "openshift_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            True,
+        ),
         (
             "23",
-            {"admin_groups": {"test-user-not-in-group"}, "manage_groups": True},
+            {
+                "admin_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "openshift_user.groups",
+                "manage_groups": True,
+            },
             False,
             False,
         ),
@@ -174,7 +200,10 @@ async def test_openshift(
 
     if expect_allowed:
         assert auth_model
-        assert set(auth_model) == {"name", "admin", "auth_state"}
+        if authenticator.manage_groups:
+            assert set(auth_model) == {"name", "admin", "auth_state", "groups"}
+        else:
+            assert set(auth_model) == {"name", "admin", "auth_state"}
         assert auth_model["name"] == handled_user_model["metadata"]["name"]
         assert auth_model["admin"] == expect_admin
         auth_state = auth_model["auth_state"]

From d9ac90e7ee3543301f2167c9bd3f501833375a70 Mon Sep 17 00:00:00 2001
From: Min RK <benjaminrk@gmail.com>
Date: Tue, 3 Sep 2024 12:27:49 +0200
Subject: [PATCH 05/12] apply manage_groups behavior outside
 `update_auth_model`

always applied last, not overrideable by subclasses.

Subclasses govern this behavior via `get_user_groups`
---
 oauthenticator/azuread.py              | 26 ++++++++++----------
 oauthenticator/globus.py               |  2 +-
 oauthenticator/oauth2.py               | 33 ++++++++++++++++++--------
 oauthenticator/tests/test_azuread.py   | 14 +++++++----
 oauthenticator/tests/test_mediawiki.py | 10 ++++----
 5 files changed, 53 insertions(+), 32 deletions(-)

diff --git a/oauthenticator/azuread.py b/oauthenticator/azuread.py
index e4e6682b..f6db57db 100644
--- a/oauthenticator/azuread.py
+++ b/oauthenticator/azuread.py
@@ -23,15 +23,26 @@ def _username_claim_default(self):
         return "name"
 
     user_groups_claim = Unicode(
-        "groups",
+        "",
         config=True,
         help="""
-        Name of claim containing user group memberships.
+        .. deprecated:: 17.0
 
-        Will populate JupyterHub groups if Authenticator.manage_groups is True.
+            Use :attr:`auth_state_groups_key` instead.
         """,
     )
 
+    @default('auth_state_groups_key')
+    def _auth_state_groups_key_default(self):
+        key = ""
+        if self.user_groups_claim:
+            key = f"{self.user_auth_state_key}.{self.user_groups_claim}"
+            cls = self.__class__.__name__
+            self.log.warning(
+                f"{cls}.user_groups_claim is deprecated in OAuthenticator 17. Use {cls}.auth_state_groups_key={key!r}"
+            )
+        return key
+
     tenant_id = Unicode(
         config=True,
         help="""
@@ -55,15 +66,6 @@ def _authorize_url_default(self):
     def _token_url_default(self):
         return f"https://login.microsoftonline.com/{self.tenant_id}/oauth2/token"
 
-    async def update_auth_model(self, auth_model, **kwargs):
-        auth_model = await super().update_auth_model(auth_model, **kwargs)
-
-        if getattr(self, "manage_groups", False):
-            user_info = auth_model["auth_state"][self.user_auth_state_key]
-            auth_model["groups"] = user_info[self.user_groups_claim]
-
-        return auth_model
-
     async def token_to_user(self, token_info):
         id_token = token_info['id_token']
         decoded = jwt.decode(
diff --git a/oauthenticator/globus.py b/oauthenticator/globus.py
index 266789e9..30a49484 100644
--- a/oauthenticator/globus.py
+++ b/oauthenticator/globus.py
@@ -342,7 +342,7 @@ async def update_auth_model(self, auth_model):
         to False makes it be revoked.
         """
         user_groups = set()
-        if self.allowed_globus_groups or self.admin_globus_groups:
+        if self.allowed_globus_groups or self.admin_globus_groups or self.manage_groups:
             tokens = self.get_globus_tokens(auth_model["auth_state"]["token_response"])
             user_groups = await self._fetch_users_groups(tokens)
         # sets are not JSONable, cast to list for auth_state
diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index 5daa3574..8c62a67c 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -1103,7 +1103,7 @@ def build_auth_state_dict(self, token_info, user_info):
         Called by the :meth:`oauthenticator.OAuthenticator.authenticate`
 
         .. versionchanged:: 17.0
-            This method be async.
+            This method may be async.
         """
 
         # We know for sure the `access_token` key exists, oterwise we would have errored out already
@@ -1133,6 +1133,8 @@ async def get_user_groups(self, auth_state: dict):
         Returns a set of groups the user belongs to based on auth_state_groups_key
         and provided auth_state.
 
+        Only called when :attr:`manage_groups` is True.
+
         - If auth_state_groups_key is a callable, it returns the list of groups directly.
           Callable may be async.
         - If auth_state_groups_key is a nested dictionary key like
@@ -1168,11 +1170,23 @@ async def update_auth_model(self, auth_model):
             - `name`: the normalized username
             - `admin`: the admin status (True/False/None), where None means it
                 should be unchanged.
-            - `auth_state`: the dictionary of of auth state
-                returned by :meth:`oauthenticator.OAuthenticator.build_auth_state_dict`
+            - `auth_state`: the auth state dictionary,
+              returned by :meth:`oauthenticator.OAuthenticator.build_auth_state_dict`
 
         Called by the :meth:`oauthenticator.OAuthenticator.authenticate`
         """
+        # NOTE: this base implementation should _not_ be updated to do anything
+        # subclasses should have full control without calling super()
+        return auth_model
+
+    async def _apply_managed_groups(self, auth_model):
+        """Applies managed_groups logic
+
+        Called after `update_auth_model` to populate the `groups` field.
+        Only called if `manage_groups` is True.
+
+        The public method for subclasses to override is `.get_user_groups`.
+        """
         if self.manage_groups:
             auth_state = auth_model["auth_state"]
             user_groups = self.get_user_groups(auth_state)
@@ -1244,7 +1258,10 @@ async def authenticate(self, handler, data=None, **kwargs):
 
         # update the auth_model with info to later authorize the user in
         # check_allowed, such as admin status and group memberships
-        return await self.update_auth_model(auth_model)
+        auth_model = await self.update_auth_model(auth_model)
+        if self.manage_groups:
+            auth_model = await self._apply_managed_groups(auth_model)
+        return auth_model
 
     async def check_allowed(self, username, auth_model):
         """
@@ -1289,12 +1306,8 @@ async def check_allowed(self, username, auth_model):
             return True
 
         # allow users who are members of allowed_groups
-        if self.manage_groups and self.allowed_groups:
-            auth_state = auth_model["auth_state"]
-            user_groups = self.get_user_groups(auth_state)
-            if isawaitable(user_groups):
-                user_groups = await user_groups
-            if any(user_groups & self.allowed_groups):
+        if self.manage_groups and self.allowed_groups and auth_model.get("groups"):
+            if set(auth_model["groups"]) & self.allowed_groups:
                 return True
 
         # users should be explicitly allowed via config, otherwise they aren't
diff --git a/oauthenticator/tests/test_azuread.py b/oauthenticator/tests/test_azuread.py
index 3a83dc8e..31230feb 100644
--- a/oauthenticator/tests/test_azuread.py
+++ b/oauthenticator/tests/test_azuread.py
@@ -119,7 +119,11 @@ def user_model(tenant_id, client_id, name):
         # test user_groups_claim
         (
             "30",
-            {"allow_all": True, "manage_groups": True},
+            {
+                "allow_all": True,
+                "auth_state_groups_key": "user.groups",
+                "manage_groups": True,
+            },
             True,
             None,
         ),
@@ -128,7 +132,7 @@ def user_model(tenant_id, client_id, name):
             {
                 "allow_all": True,
                 "manage_groups": True,
-                "user_groups_claim": "grp",
+                "auth_state_groups_key": "user.grp",
             },
             True,
             None,
@@ -220,9 +224,11 @@ async def test_azuread(
         assert auth_model["name"] == user_info[authenticator.username_claim]
         if manage_groups:
             groups = auth_model['groups']
-            assert groups == user_info[authenticator.user_groups_claim]
+            assert (
+                groups == user_info[authenticator.auth_state_groups_key.rsplit(".")[-1]]
+            )
     else:
-        assert auth_model == None
+        assert auth_model is None
 
 
 async def test_tenant_id_from_env():
diff --git a/oauthenticator/tests/test_mediawiki.py b/oauthenticator/tests/test_mediawiki.py
index 417ec7e5..794ba377 100644
--- a/oauthenticator/tests/test_mediawiki.py
+++ b/oauthenticator/tests/test_mediawiki.py
@@ -80,7 +80,7 @@ def post_token(request, context):
             "20",
             {
                 "allowed_groups": {"group1"},
-                "auth_state_groups_key": "mediawiki_user.groups",
+                "auth_state_groups_key": "MEDIAWIKI_USER_IDENTITY.groups",
                 "manage_groups": True,
             },
             True,
@@ -90,7 +90,7 @@ def post_token(request, context):
             "21",
             {
                 "allowed_groups": {"test-user-not-in-group"},
-                "auth_state_groups_key": "mediawiki_user.groups",
+                "auth_state_groups_key": "MEDIAWIKI_USER_IDENTITY.groups",
                 "manage_groups": True,
             },
             False,
@@ -100,7 +100,7 @@ def post_token(request, context):
             "22",
             {
                 "admin_groups": {"group1"},
-                "auth_state_groups_key": "mediawiki_user.groups",
+                "auth_state_groups_key": "MEDIAWIKI_USER_IDENTITY.groups",
                 "manage_groups": True,
             },
             True,
@@ -110,7 +110,7 @@ def post_token(request, context):
             "23",
             {
                 "admin_groups": {"test-user-not-in-group"},
-                "auth_state_groups_key": "mediawiki_user.groups",
+                "auth_state_groups_key": "MEDIAWIKI_USER_IDENTITY.groups",
                 "manage_groups": True,
             },
             False,
@@ -155,7 +155,7 @@ async def test_mediawiki(
         user_info = auth_state[authenticator.user_auth_state_key]
         assert auth_model["name"] == user_info[authenticator.username_claim]
     else:
-        assert auth_model == None
+        assert auth_model is None
 
 
 async def test_login_redirect(mediawiki):

From baca8a42df952d51b6037a797702d61c517e2e9a Mon Sep 17 00:00:00 2001
From: Min RK <benjaminrk@gmail.com>
Date: Tue, 3 Sep 2024 12:39:18 +0200
Subject: [PATCH 06/12] fix auth_state_groups_key example in docs

add oauth_user
---
 .../tutorials/provider-specific-setup/providers/generic.md      | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/source/tutorials/provider-specific-setup/providers/generic.md b/docs/source/tutorials/provider-specific-setup/providers/generic.md
index 965c0951..fc8fe390 100644
--- a/docs/source/tutorials/provider-specific-setup/providers/generic.md
+++ b/docs/source/tutorials/provider-specific-setup/providers/generic.md
@@ -35,7 +35,7 @@ c.GenericOAuthenticator.userdata_url = "https://accounts.example.com/auth/realms
 #
 c.GenericOAuthenticator.scope = ["openid", "email", "groups"]
 c.GenericOAuthenticator.username_claim = "email"
-c.GenericOAuthenticator.auth_state_groups_key = "groups"
+c.GenericOAuthenticator.auth_state_groups_key = "oauth_user.groups"
 
 # Authorization
 # -------------

From 115ceeeb2bf8ecb2f064a878b4a618d59d652987 Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Tue, 3 Sep 2024 15:15:17 +0200
Subject: [PATCH 07/12] openshift: rely on base class implementation for
 allowed_groups and admin_groups

---
 oauthenticator/openshift.py | 60 ++++---------------------------------
 1 file changed, 5 insertions(+), 55 deletions(-)

diff --git a/oauthenticator/openshift.py b/oauthenticator/openshift.py
index 6b016b0d..3f0c4646 100644
--- a/oauthenticator/openshift.py
+++ b/oauthenticator/openshift.py
@@ -8,7 +8,7 @@
 
 from jupyterhub.auth import LocalAuthenticator
 from tornado.httpclient import HTTPClient, HTTPRequest
-from traitlets import Bool, Set, Unicode, default
+from traitlets import Bool, Unicode, default
 
 from oauthenticator.oauth2 import OAuthenticator
 
@@ -16,6 +16,10 @@
 class OpenShiftOAuthenticator(OAuthenticator):
     user_auth_state_key = "openshift_user"
 
+    @default("auth_state_groups_key")
+    def _auth_state_groups_key_default(self):
+        return "openshift_user.groups"
+
     @default("scope")
     def _scope_default(self):
         return ["user:info"]
@@ -45,24 +49,6 @@ def _http_request_kwargs_default(self):
         """,
     )
 
-    allowed_groups = Set(
-        config=True,
-        help="""
-        Allow members of selected OpenShift groups to sign in.
-        """,
-    )
-
-    admin_groups = Set(
-        config=True,
-        help="""
-        Allow members of selected OpenShift groups to sign in and consider them
-        as JupyterHub admins.
-
-        If this is set and a user isn't part of one of these groups or listed in
-        `admin_users`, a user signing in will have their admin status revoked.
-        """,
-    )
-
     openshift_auth_api_url = Unicode(
         config=True,
         help="""
@@ -158,42 +144,6 @@ def user_info_to_username(self, user_info):
         """
         return user_info['metadata']['name']
 
-    async def update_auth_model(self, auth_model):
-        """
-        Sets admin status to True or False if `admin_groups` is configured and
-        the user isn't part of `admin_users`. Note that leaving it at None makes
-        users able to retain an admin status while setting it to False makes it
-        be revoked.
-        """
-        if auth_model["admin"]:
-            # auth_model["admin"] being True means the user was in admin_users
-            return auth_model
-
-        if self.admin_groups:
-            # admin status should in this case be True or False, not None
-            user_info = auth_model["auth_state"][self.user_auth_state_key]
-            user_groups = set(user_info["groups"])
-            auth_model["admin"] = bool(user_groups & self.admin_groups)
-
-        return auth_model
-
-    async def check_allowed(self, username, auth_model):
-        """
-        Overrides OAuthenticator.check_allowed to also allow users part of
-        `allowed_groups`.
-        """
-        if await super().check_allowed(username, auth_model):
-            return True
-
-        if self.allowed_groups:
-            user_info = auth_model["auth_state"][self.user_auth_state_key]
-            user_groups = set(user_info["groups"])
-            if user_groups & self.allowed_groups:
-                return True
-
-        # users should be explicitly allowed via config, otherwise they aren't
-        return False
-
 
 class LocalOpenShiftOAuthenticator(LocalAuthenticator, OpenShiftOAuthenticator):
     """A version that mixes in local system user creation"""

From 6618811b3e8efe4b1589512a26b14dd1d21b2140 Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Tue, 3 Sep 2024 15:19:19 +0200
Subject: [PATCH 08/12] openshift: test equivalent of default
 auth_state_groups_key provided as before

---
 oauthenticator/tests/test_openshift.py | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/oauthenticator/tests/test_openshift.py b/oauthenticator/tests/test_openshift.py
index 64504cc9..8b87f0e5 100644
--- a/oauthenticator/tests/test_openshift.py
+++ b/oauthenticator/tests/test_openshift.py
@@ -141,7 +141,6 @@ def user_model():
             "20",
             {
                 "allowed_groups": {"group1"},
-                "auth_state_groups_key": "openshift_user.groups",
                 "manage_groups": True,
             },
             True,
@@ -151,7 +150,6 @@ def user_model():
             "21",
             {
                 "allowed_groups": {"test-user-not-in-group"},
-                "auth_state_groups_key": "openshift_user.groups",
                 "manage_groups": True,
             },
             False,
@@ -161,7 +159,6 @@ def user_model():
             "22",
             {
                 "admin_groups": {"group1"},
-                "auth_state_groups_key": "openshift_user.groups",
                 "manage_groups": True,
             },
             True,
@@ -171,7 +168,6 @@ def user_model():
             "23",
             {
                 "admin_groups": {"test-user-not-in-group"},
-                "auth_state_groups_key": "openshift_user.groups",
                 "manage_groups": True,
             },
             False,

From c16bb8caae1d5dd472d961fd8d6f75588f7925f5 Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Tue, 3 Sep 2024 15:38:11 +0200
Subject: [PATCH 09/12] tests: verify deprecation message for
 AzureAdOAuthenticator.user_groups_claim

---
 oauthenticator/azuread.py            |  2 +-
 oauthenticator/tests/test_azuread.py | 47 +++++++++++++++++++++++++++-
 2 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/oauthenticator/azuread.py b/oauthenticator/azuread.py
index f6db57db..ffdb14bc 100644
--- a/oauthenticator/azuread.py
+++ b/oauthenticator/azuread.py
@@ -39,7 +39,7 @@ def _auth_state_groups_key_default(self):
             key = f"{self.user_auth_state_key}.{self.user_groups_claim}"
             cls = self.__class__.__name__
             self.log.warning(
-                f"{cls}.user_groups_claim is deprecated in OAuthenticator 17. Use {cls}.auth_state_groups_key={key!r}"
+                f"{cls}.user_groups_claim is deprecated in OAuthenticator 17. Use {cls}.auth_state_groups_key = {key!r}"
             )
         return key
 
diff --git a/oauthenticator/tests/test_azuread.py b/oauthenticator/tests/test_azuread.py
index 31230feb..eaae2c39 100644
--- a/oauthenticator/tests/test_azuread.py
+++ b/oauthenticator/tests/test_azuread.py
@@ -1,6 +1,7 @@
 """test azure ad"""
 
 import json
+import logging
 import os
 import re
 import time
@@ -9,7 +10,7 @@
 
 import jwt
 import pytest
-from pytest import fixture, mark
+from pytest import fixture, mark, raises
 from traitlets.config import Config
 
 from ..azuread import AzureAdOAuthenticator
@@ -236,3 +237,47 @@ async def test_tenant_id_from_env():
     with mock.patch.dict(os.environ, {"AAD_TENANT_ID": tenant_id}):
         aad = AzureAdOAuthenticator()
         assert aad.tenant_id == tenant_id
+
+
+@mark.parametrize(
+    "test_variation_id,class_config,expect_config,expect_loglevel,expect_message",
+    [
+        (
+            "user_groups_claim",
+            {"user_groups_claim": "groups", "manage_groups": True},
+            {"auth_state_groups_key": "user.groups"},
+            logging.WARNING,
+            "AzureAdOAuthenticator.user_groups_claim is deprecated in OAuthenticator 17. Use AzureAdOAuthenticator.auth_state_groups_key = 'user.groups'",
+        ),
+    ],
+)
+async def test_deprecated_config(
+    caplog,
+    test_variation_id,
+    class_config,
+    expect_config,
+    expect_loglevel,
+    expect_message,
+):
+    """
+    Tests that a warning is emitted when using a deprecated config and that
+    configuring the old config ends up configuring the new config.
+    """
+    print(f"Running test variation id {test_variation_id}")
+    c = Config()
+    c.AzureAdOAuthenticator = Config(class_config)
+
+    test_logger = logging.getLogger('testlog')
+    if expect_loglevel == logging.ERROR:
+        with raises(ValueError, match=expect_message):
+            AzureAdOAuthenticator(config=c, log=test_logger)
+    else:
+        authenticator = AzureAdOAuthenticator(config=c, log=test_logger)
+        for key, value in expect_config.items():
+            assert getattr(authenticator, key) == value
+
+    captured_log_tuples = caplog.record_tuples
+    print(captured_log_tuples)
+
+    expected_log_tuple = (test_logger.name, expect_loglevel, expect_message)
+    assert expected_log_tuple in captured_log_tuples

From f92b5118fefc3d465f9ff5a640d909a903fb1f24 Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Tue, 3 Sep 2024 15:40:12 +0200
Subject: [PATCH 10/12] docs: update azuread docs about auth_state_groups_key

---
 .../tutorials/provider-specific-setup/providers/azuread.md    | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/docs/source/tutorials/provider-specific-setup/providers/azuread.md b/docs/source/tutorials/provider-specific-setup/providers/azuread.md
index 09359d69..f3520bfe 100644
--- a/docs/source/tutorials/provider-specific-setup/providers/azuread.md
+++ b/docs/source/tutorials/provider-specific-setup/providers/azuread.md
@@ -35,8 +35,6 @@ be relevant to read more about in the configuration reference:
 ## Loading user groups
 
 The `AzureAdOAuthenticator` can load the group-membership of users from the access token.
-This is done by setting the `AzureAdOAuthenticator.groups_claim` to the name of the claim that contains the
-group-membership.
 
 ```python
 c.JupyterHub.authenticator_class = "azuread"
@@ -44,7 +42,7 @@ c.JupyterHub.authenticator_class = "azuread"
 # {...} other settings (see above)
 
 c.AzureAdOAuthenticator.manage_groups = True
-c.AzureAdOAuthenticator.user_groups_claim = 'groups'  # this is the default
+c.AzureAdOAuthenticator.auth_state_groups_key = 'user.groups'
 ```
 
 This requires Azure AD to be configured to include the group-membership in the access token.

From 05778cd2800e3731e34ef86881554d6dde7f0a89 Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Tue, 3 Sep 2024 15:47:03 +0200
Subject: [PATCH 11/12] azuread: retain default of deprecated user_groups_claim
 within auth_state_groups_key

---
 .../provider-specific-setup/providers/azuread.md       |  2 +-
 oauthenticator/azuread.py                              |  2 +-
 oauthenticator/tests/test_azuread.py                   | 10 +++-------
 3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/docs/source/tutorials/provider-specific-setup/providers/azuread.md b/docs/source/tutorials/provider-specific-setup/providers/azuread.md
index f3520bfe..7df9217e 100644
--- a/docs/source/tutorials/provider-specific-setup/providers/azuread.md
+++ b/docs/source/tutorials/provider-specific-setup/providers/azuread.md
@@ -42,7 +42,7 @@ c.JupyterHub.authenticator_class = "azuread"
 # {...} other settings (see above)
 
 c.AzureAdOAuthenticator.manage_groups = True
-c.AzureAdOAuthenticator.auth_state_groups_key = 'user.groups'
+c.AzureAdOAuthenticator.auth_state_groups_key = "user.groups"  # this is the default
 ```
 
 This requires Azure AD to be configured to include the group-membership in the access token.
diff --git a/oauthenticator/azuread.py b/oauthenticator/azuread.py
index ffdb14bc..dec2c2b9 100644
--- a/oauthenticator/azuread.py
+++ b/oauthenticator/azuread.py
@@ -34,7 +34,7 @@ def _username_claim_default(self):
 
     @default('auth_state_groups_key')
     def _auth_state_groups_key_default(self):
-        key = ""
+        key = "user.groups"
         if self.user_groups_claim:
             key = f"{self.user_auth_state_key}.{self.user_groups_claim}"
             cls = self.__class__.__name__
diff --git a/oauthenticator/tests/test_azuread.py b/oauthenticator/tests/test_azuread.py
index eaae2c39..77a7c217 100644
--- a/oauthenticator/tests/test_azuread.py
+++ b/oauthenticator/tests/test_azuread.py
@@ -117,12 +117,12 @@ def user_model(tenant_id, client_id, name):
             True,
             None,
         ),
-        # test user_groups_claim
+        # test user_groups_claim (deprecated)
         (
             "30",
             {
                 "allow_all": True,
-                "auth_state_groups_key": "user.groups",
+                "user_groups_claim": "groups",
                 "manage_groups": True,
             },
             True,
@@ -133,7 +133,7 @@ def user_model(tenant_id, client_id, name):
             {
                 "allow_all": True,
                 "manage_groups": True,
-                "auth_state_groups_key": "user.grp",
+                "user_groups_claim": "grp",
             },
             True,
             None,
@@ -143,7 +143,6 @@ def user_model(tenant_id, client_id, name):
             "40",
             {
                 "allowed_groups": {"group1"},
-                "auth_state_groups_key": "user.groups",
                 "manage_groups": True,
             },
             True,
@@ -153,7 +152,6 @@ def user_model(tenant_id, client_id, name):
             "41",
             {
                 "allowed_groups": {"test-user-not-in-group"},
-                "auth_state_groups_key": "user.groups",
                 "manage_groups": True,
             },
             False,
@@ -163,7 +161,6 @@ def user_model(tenant_id, client_id, name):
             "42",
             {
                 "admin_groups": {"group1"},
-                "auth_state_groups_key": "user.groups",
                 "manage_groups": True,
             },
             True,
@@ -173,7 +170,6 @@ def user_model(tenant_id, client_id, name):
             "43",
             {
                 "admin_groups": {"test-user-not-in-group"},
-                "auth_state_groups_key": "user.groups",
                 "manage_groups": True,
             },
             False,

From 9e5520d16fda2d0c497f57be86e95857bfcc11eb Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Tue, 3 Sep 2024 16:02:16 +0200
Subject: [PATCH 12/12] docs: mention allowed_groups and admin_groups in
 general setup

---
 docs/source/tutorials/general-setup.md | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/docs/source/tutorials/general-setup.md b/docs/source/tutorials/general-setup.md
index 772b4e8f..920a3128 100644
--- a/docs/source/tutorials/general-setup.md
+++ b/docs/source/tutorials/general-setup.md
@@ -60,7 +60,9 @@ projects' authenticator classes.
    - {attr}`.OAuthenticator.allow_all`
    - {attr}`.OAuthenticator.allow_existing_users`
    - {attr}`.OAuthenticator.allowed_users`
+   - {attr}`.OAuthenticator.allowed_groups`
    - {attr}`.OAuthenticator.admin_users`
+   - {attr}`.OAuthenticator.admin_groups`
 
    Your authenticator class may have unique config, so in the end it can look
    something like this: