Repository: jupyterhub/oauthenticator

Issue:
New group config in OAuthenticator overlaps with `allowed_google_groups` for example and may not work - what do we do?
#735 moved group related config in Generic to OAuthenticator, and did a breaking change related to that group config previosly found only in GenericOAuthenticator.

<details><summary>Documentation about breaking change from #735</summary>

> ## Backwards compatibility
> * The existing `claim_groups_key` behavior is preserved, by being passed on to `auth_state_groups_key` in the base. It has been marked as deprecated. This is not a backwards compatibility break.
> * All group related functionality now **requires** `manage_groups` to be `True`, which was not the case earlier. Before this, if `manage_groups` is false but any of the group related authorization functionality (`allowed_groups` and `admin_groups`) is used, they control group related behavior **but don't show up as JupyterHub groups**. This causes confusion, as the 'groups' field in the admin panel will be empty, and possible other group related behavior (such as future profile list filtering, for example) would _not_ respect these groups. We basically would end up with _two_ group concepts - First class JupyterHub groups (which will show up in admin panel, API, can be _edited_ by admins, etc) as well as second class 'Authenticator' groups (which are only used for authorization and 'disappear' after that). I think this is unnecessary complication, and this is a good time to remove this distinction. Now, any kind of group related authorization functionality _requires_ `manage_groups` to be `True`, and we are back to having only one notion of 'group'. We also remove the confusing part where you may have `allowed_groups` set to something, manually modify the groups the user is a part of in JupyterHub admin, and it silently has no effect. This is a _breaking change_ for people who used groups functionality but set `manage_groups` to be `False`. However, I think that usage is fairly minor, because of the confusing behavior it causes. I have added the 'breaking' label here regardless.
> 
> ## Breaking change
> * All group related functionality (`allowed_groups`, `admin_groups`, `claims_group_key` and `auth_state_groups_key`) now also requires `manage_groups` to be set to `True`

</details>

We have two situations to address in my mind:

- Some classes have two sets of group config without guidance on whats supported and recommended to use.

  For example, these configs are available side by side with `allowed_groups` now.

  `GitHubOAuthenticator.allowed_organizations`
  `GitLabOAuthenticator.allowed_gitlab_groups`
  `GlobusOAuthenticator.allowed_globus_groups`
  `GoogleOAuthenticator.allowed_google_groups`
- New set of group config may not work consistently across authenticator classes

  Searching for `def update_auth_model` in the code base, it seems we typically don't call the base class implementation, making logic required for the new set of group config.
  
  Tests were updated in Generic and OpenShift, but not in other classes such as GitHub, GitLab, Globus, Google.

Discussion:

Patch:
diff --git a/docs/source/tutorials/general-setup.md b/docs/source/tutorials/general-setup.md
index 772b4e8..920a312 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:
diff --git a/docs/source/tutorials/provider-specific-setup/providers/azuread.md b/docs/source/tutorials/provider-specific-setup/providers/azuread.md
index 09359d6..7df9217 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 is the default
 ```
 
 This requires Azure AD to be configured to include the group-membership in the access token.
diff --git a/docs/source/tutorials/provider-specific-setup/providers/generic.md b/docs/source/tutorials/provider-specific-setup/providers/generic.md
index 386116a..fc8fe39 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 = "oauth_user.groups"
 
 # Authorization
 # -------------
diff --git a/oauthenticator/azuread.py b/oauthenticator/azuread.py
index e4e6682..dec2c2b 100644
--- a/oauthenticator/azuread.py
+++ b/oauthenticator/azuread.py
@@ -23,15 +23,26 @@ class AzureAdOAuthenticator(OAuthenticator):
         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 = "user.groups"
+        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 @@ class AzureAdOAuthenticator(OAuthenticator):
     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 266789e..30a4948 100644
--- a/oauthenticator/globus.py
+++ b/oauthenticator/globus.py
@@ -342,7 +342,7 @@ class GlobusOAuthenticator(OAuthenticator):
         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/google.py b/oauthenticator/google.py
index 5d901cd..5ed613d 100644
--- a/oauthenticator/google.py
+++ b/oauthenticator/google.py
@@ -7,7 +7,7 @@ import os
 from jupyterhub.auth import LocalAuthenticator
 from tornado.auth import GoogleOAuth2Mixin
 from tornado.web import HTTPError
-from traitlets import Bool, Dict, List, Set, Unicode, default, validate
+from traitlets import Dict, List, Set, Unicode, default, validate
 
 from .oauth2 import OAuthenticator
 
@@ -105,32 +105,6 @@ class GoogleOAuthenticator(OAuthenticator, GoogleOAuth2Mixin):
         """,
     )
 
-    strip_domain = Bool(
-        config=True,
-        help="""
-        Strip the username to exclude the `@domain` part. 
-        This happens by default when there is only one hosted domain specified
-
-        .. warning::
-
-            If domains are stripped from usernames and multiple `hosted_domains` are specified, 
-            there is a chance of clashing usernames.
-        """,
-    )
-
-    @default('strip_domain')
-    def _strip_if_single_domain(self):
-        return len(self.hosted_domain) <= 1
-
-    @validate('strip_domain')
-    def _check_multiple_hosted_domain(self, strip_domain):
-        if len(self.hosted_domain) > 1 and strip_domain:
-            self.log.warning(
-                "User names are stripped of `@domain`, but multiple domains are specified."
-                " This can lead to clashing usernames"
-            )
-        return strip_domain.value
-
     hosted_domain = List(
         Unicode(),
         config=True,
@@ -205,13 +179,21 @@ class GoogleOAuthenticator(OAuthenticator, GoogleOAuth2Mixin):
         """
         username = super().user_info_to_username(user_info)
         user_email = user_info["email"]
-        user_info["domain"] = user_email.split("@")[1].lower()
+        user_domain = user_info["domain"] = user_email.split("@")[1].lower()
 
         # NOTE: This is not an authorization check, it just about username
         #       derivation. Decoupling hosted_domain from this is considered in
         #       https://github.com/jupyterhub/oauthenticator/issues/733.
-
-        if self.strip_domain and user_info["domain"] in self.hosted_domain:
+        #
+        # NOTE: This code is written with without knowing for sure if the user
+        #       email's domain could be different from the domain in hd, so we
+        #       assume it could be even though it seems like it can't be. If a
+        #       Google organization/workspace manages users in a "primary
+        #       domain" and a "secondary domain", users with respective email
+        #       domain have their hd field set respectively.
+        #
+        if len(self.hosted_domain) == 1 and user_domain == self.hosted_domain[0]:
+            # strip the domain in this situation
             username = username.split("@")[0]
 
         return username
diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index 5daa357..8c62a67 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -1103,7 +1103,7 @@ class OAuthenticator(Authenticator):
         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 @@ class OAuthenticator(Authenticator):
         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 @@ class OAuthenticator(Authenticator):
             - `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 @@ class OAuthenticator(Authenticator):
 
         # 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 @@ class OAuthenticator(Authenticator):
             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/openshift.py b/oauthenticator/openshift.py
index 6b016b0..3f0c464 100644
--- a/oauthenticator/openshift.py
+++ b/oauthenticator/openshift.py
@@ -8,7 +8,7 @@ import os
 
 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 @@ from oauthenticator.oauth2 import OAuthenticator
 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 @@ class OpenShiftOAuthenticator(OAuthenticator):
         """,
     )
 
-    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 @@ class OpenShiftOAuthenticator(OAuthenticator):
         """
         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"""


Test patch:
diff --git a/oauthenticator/tests/test_auth0.py b/oauthenticator/tests/test_auth0.py
index e4ae3be..78a9b42 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,47 @@ def user_model():
             True,
             True,
         ),
+        # common tests with allowed_groups and manage_groups
+        (
+            "20",
+            {
+                "allowed_groups": {"group1"},
+                "auth_state_groups_key": "auth0_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            None,
+        ),
+        (
+            "21",
+            {
+                "allowed_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "auth0_user.groups",
+                "manage_groups": True,
+            },
+            False,
+            None,
+        ),
+        (
+            "22",
+            {
+                "admin_groups": {"group1"},
+                "auth_state_groups_key": "auth0_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            True,
+        ),
+        (
+            "23",
+            {
+                "admin_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "auth0_user.groups",
+                "manage_groups": True,
+            },
+            False,
+            False,
+        ),
     ],
 )
 async def test_auth0(
@@ -84,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 2bf7606..77a7c21 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 @@ from unittest import mock
 
 import jwt
 import pytest
-from pytest import fixture, mark
+from pytest import fixture, mark, raises
 from traitlets.config import Config
 
 from ..azuread import AzureAdOAuthenticator
@@ -49,6 +50,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": [
@@ -115,10 +117,14 @@ def user_model(tenant_id, client_id, name):
             True,
             None,
         ),
-        # test user_groups_claim
+        # test user_groups_claim (deprecated)
         (
             "30",
-            {"allow_all": True, "manage_groups": True},
+            {
+                "allow_all": True,
+                "user_groups_claim": "groups",
+                "manage_groups": True,
+            },
             True,
             None,
         ),
@@ -132,6 +138,43 @@ 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(
@@ -178,9 +221,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():
@@ -188,3 +233,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
diff --git a/oauthenticator/tests/test_bitbucket.py b/oauthenticator/tests/test_bitbucket.py
index 512e803..37d776c 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"],
     }
 
 
@@ -80,6 +81,47 @@ def user_model(username):
             True,
             True,
         ),
+        # common tests with allowed_groups and manage_groups
+        (
+            "20",
+            {
+                "allowed_groups": {"group1"},
+                "auth_state_groups_key": "bitbucket_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            None,
+        ),
+        (
+            "21",
+            {
+                "allowed_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "bitbucket_user.groups",
+                "manage_groups": True,
+            },
+            False,
+            None,
+        ),
+        (
+            "22",
+            {
+                "admin_groups": {"group1"},
+                "auth_state_groups_key": "bitbucket_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            True,
+        ),
+        (
+            "23",
+            {
+                "admin_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "bitbucket_user.groups",
+                "manage_groups": True,
+            },
+            False,
+            False,
+        ),
     ],
 )
 async def test_bitbucket(
@@ -101,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 4dac492..2b830a4 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,47 @@ def user_model(username, username_claim, **kwargs):
             True,
             True,
         ),
+        # common tests with allowed_groups and manage_groups
+        (
+            "20",
+            {
+                "allowed_groups": {"group1"},
+                "auth_state_groups_key": "cilogon_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            None,
+        ),
+        (
+            "21",
+            {
+                "allowed_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "cilogon_user.groups",
+                "manage_groups": True,
+            },
+            False,
+            None,
+        ),
+        (
+            "22",
+            {
+                "admin_groups": {"group1"},
+                "auth_state_groups_key": "cilogon_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            True,
+        ),
+        (
+            "23",
+            {
+                "admin_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "cilogon_user.groups",
+                "manage_groups": True,
+            },
+            False,
+            False,
+        ),
     ],
 )
 async def test_cilogon(
@@ -88,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)
@@ -416,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_generic.py b/oauthenticator/tests/test_generic.py
index 11da102..e322503 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
 
@@ -104,20 +105,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 +215,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(
@@ -501,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
diff --git a/oauthenticator/tests/test_github.py b/oauthenticator/tests/test_github.py
index 35b64a4..e49fe06 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,47 @@ def github_client(client):
             True,
             True,
         ),
+        # common tests with allowed_groups and manage_groups
+        (
+            "20",
+            {
+                "allowed_groups": {"group1"},
+                "auth_state_groups_key": "github_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            None,
+        ),
+        (
+            "21",
+            {
+                "allowed_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "github_user.groups",
+                "manage_groups": True,
+            },
+            False,
+            None,
+        ),
+        (
+            "22",
+            {
+                "admin_groups": {"group1"},
+                "auth_state_groups_key": "github_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            True,
+        ),
+        (
+            "23",
+            {
+                "admin_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "github_user.groups",
+                "manage_groups": True,
+            },
+            False,
+            False,
+        ),
     ],
 )
 async def test_github(
@@ -87,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 f8d7ad3..86d0c89 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,47 @@ def mock_api_version(client, version):
             True,
             True,
         ),
+        # common tests with allowed_groups and manage_groups
+        (
+            "20",
+            {
+                "allowed_groups": {"group1"},
+                "auth_state_groups_key": "gitlab_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            None,
+        ),
+        (
+            "21",
+            {
+                "allowed_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "gitlab_user.groups",
+                "manage_groups": True,
+            },
+            False,
+            None,
+        ),
+        (
+            "22",
+            {
+                "admin_groups": {"group1"},
+                "auth_state_groups_key": "gitlab_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            True,
+        ),
+        (
+            "23",
+            {
+                "admin_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "gitlab_user.groups",
+                "manage_groups": True,
+            },
+            False,
+            False,
+        ),
     ],
 )
 async def test_gitlab(
@@ -107,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 95ade11..906de7c 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,47 @@ def mock_globus_user(globus_tokens_by_resource_server):
             False,
             False,
         ),
+        # common tests with allowed_groups and manage_groups
+        (
+            "20",
+            {
+                "allowed_groups": {"group1"},
+                "auth_state_groups_key": "globus_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            None,
+        ),
+        (
+            "21",
+            {
+                "allowed_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "globus_user.groups",
+                "manage_groups": True,
+            },
+            False,
+            None,
+        ),
+        (
+            "22",
+            {
+                "admin_groups": {"group1"},
+                "auth_state_groups_key": "globus_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            True,
+        ),
+        (
+            "23",
+            {
+                "admin_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "globus_user.groups",
+                "manage_groups": True,
+            },
+            False,
+            False,
+        ),
     ],
 )
 async def test_globus(
@@ -300,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 3aeb652..7dc7c35 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,47 @@ def google_client(client):
             False,
             False,
         ),
+        # common tests with allowed_groups and manage_groups
+        (
+            "20",
+            {
+                "allowed_groups": {"group1"},
+                "auth_state_groups_key": "google_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            None,
+        ),
+        (
+            "21",
+            {
+                "allowed_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "google_user.groups",
+                "manage_groups": True,
+            },
+            False,
+            None,
+        ),
+        (
+            "22",
+            {
+                "admin_groups": {"group1"},
+                "auth_state_groups_key": "google_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            True,
+        ),
+        (
+            "23",
+            {
+                "admin_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "google_user.groups",
+                "manage_groups": True,
+            },
+            False,
+            False,
+        ),
     ],
 )
 async def test_google(
@@ -175,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 3510b74..794ba37 100644
--- a/oauthenticator/tests/test_mediawiki.py
+++ b/oauthenticator/tests/test_mediawiki.py
@@ -25,6 +25,7 @@ def mediawiki():
                 'iss': 'https://meta.wikimedia.org',
                 'iat': time.time(),
                 'nonce': request_nonce,
+                'groups': ['group1'],
             },
             'client_secret',
         ).encode()
@@ -74,6 +75,47 @@ def mediawiki():
             True,
             True,
         ),
+        # common tests with allowed_groups and manage_groups
+        (
+            "20",
+            {
+                "allowed_groups": {"group1"},
+                "auth_state_groups_key": "MEDIAWIKI_USER_IDENTITY.groups",
+                "manage_groups": True,
+            },
+            True,
+            None,
+        ),
+        (
+            "21",
+            {
+                "allowed_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "MEDIAWIKI_USER_IDENTITY.groups",
+                "manage_groups": True,
+            },
+            False,
+            None,
+        ),
+        (
+            "22",
+            {
+                "admin_groups": {"group1"},
+                "auth_state_groups_key": "MEDIAWIKI_USER_IDENTITY.groups",
+                "manage_groups": True,
+            },
+            True,
+            True,
+        ),
+        (
+            "23",
+            {
+                "admin_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "MEDIAWIKI_USER_IDENTITY.groups",
+                "manage_groups": True,
+            },
+            False,
+            False,
+        ),
     ],
 )
 async def test_mediawiki(
@@ -101,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)
@@ -110,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):
diff --git a/oauthenticator/tests/test_openshift.py b/oauthenticator/tests/test_openshift.py
index 8421cf0..8b87f0e 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,43 @@ 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(
@@ -173,7 +196,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"]