Repository: jupyterhub/oauthenticator
Pull request #735 (2024-03-29 05:27:30)

Move group management from generic to base oauthenticator
## Motivating use cases

External identity providers providing JupyterHub memberships is an extremely useful feature that should be present not just for `GenericOAuthenticator` but for all authenticators. But to do so in helpful ways, this PR considers two motivating use cases:

1. A hub using `Auth0OAuthenticator`. In Auth0, `scope`s granted are what is used to provide the notion of 'can the user perform this task?', which can be used as group membership. This is what auth0 recommends, there is currently no other way to do 'groups' in Auth0. `scope` is inside `auth_model`, but *not* `auth_state`, since `scope` is granted each time the user is logged in.
2. In `GitHubOAuthenticator`, we put the list of teams the user is in inside `auth_state`. This is the perfect piece of information to use for group membership.
3. `oauth_user` gets put inside `auth_state`, and in general `auth_state` is a good place for this kinda group information to be in. Authenticators can put arbitrary stuff inside `auth_state` and use them as they wish.


## Approaches considered and rejected

1. Each authenticator figures out how groups work within it. This was tried out in https://github.com/jupyterhub/oauthenticator/pull/739, and rejected by @minrk in https://github.com/jupyterhub/oauthenticator/pull/735#issuecomment-2041996689. I agree with this rejection.
2. Allow group information to be picked up from the `auth_model` with a `auth_model_groups_key`. This would be same as the current `claim_groups_key`, but pick from the entire `auth_model` instead of just from the returned user object. This was the tack this PR was taking, primarily because I thought we needed it to handle use case 1 mentioned earlier. But turns out I was wrong - I had thought that  `scope` was part of `auth_model` but not `auth_state`, but we do! And regardless, I also realized we don't expose `auth_model` *anywhere*, but we *do* expose `auth_state`. And I had a TODO for 'document what is in `auth_model`', and while trying to do that, decided we shouldn't expose that to configurable tweaks like this *for now*. So that was reverted in b337015306f51a8a9ea8e95924a7562bfa1e56ba and a different approach was taken.

## Approach this PR takes

The general approach to group management is:

1. Authenticator puts something that may be relevant to group membership inside `auth_state`.
2. There's an `auth_state_groups_key` that can be either a callable or dotted specification that generates a list of groups from something in `auth_state`.

This handles case (2) because list of teams is already in `auth_state`. And can handle (1) by us putting `scope` in some form inside `auth_state`. This also provides a clear extensible mechanism in the future for all group management - get it into `auth_state` (where it can be used for anything), and pick that out with `auth_state_groups_key`.

## 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`

## TODO

- [x] Update docs to refer to new property
- [x] Reword the docstring for `auth_state_groups_key`

Fixes https://github.com/jupyterhub/oauthenticator/issues/709

yuvipanda: All this functionality would be extremely useful to *all* the authenticators, so should be in base.

I think an important question now becomes - what is the point of GenericOAuthenticator? Is that just here for backwards compatibility?

I think the answer should be 'yes'. And in the future, anything that relies on OAuth2 functionality should just go to the base OAuth2 provider, and anything specific to any of the providers can go in their own files. And we suggest people who currently use `Generic` migrate to just using the base OAuth2 provider.
minrk: > what is the point of GenericOAuthenticator?

I believe GenericOAuthenticator should eventually go away and be merged into the base class, as long as we can do it smoothly and it doesn't complicate things unnecessarily (I don't _think_ it will). I think the big refactor in #526 was a big step toward making that feasible.

Not having done that yet, going forward in general I think we should avoid new features in Generic in favor of putting them in the base class, at least in most cases.
yuvipanda: I was thinking about how we can do a very common thing - sync GitHub team memberships and org memberships to JupyterHub groups. With this PR as is, that's actually not possible - team memberships are not part of userdata! But they are part of the auth_model (because there's a `populate_teams_in_auth_model` property already).

I think there are two paths forward here for groups to work everywhere:

1. Lift that into the base Authenticator class, but the key is for things from `auth_model`, not userdata. This also allows for using things like `scope` (which can control group membership with Auth0, from another community we are working with). We can name this appropriately, and the keep a deprecated `claims_group_key` in Generic that notifies it's deprecated but works the way it already does. 
2. https://github.com/jupyterhub/oauthenticator/pull/739 or similar for each authenticator we want to support good groups.

My preference is to do (1)! There's a clean backwards compat story, and the model of 'put stuff into auth_state, that is available via auth_model, and you can pull stuff out of that for groups' seems clean enough to explain.
minrk: Option 1 sounds like a great choice, and also a great opportunity to not inherit the name while preserving compatibility.
yuvipanda: This went through a few iterations, but is ready for review again! I've updated the PR body with more detailed information as well. Please take another look when you got time, @minrk :)
yuvipanda: After trying to write clear docs for the traitlet, I came to the conclusion that we shouldn't allow usage of these without `manage_groups` also being True. I've updated a paragraph under `Backwards compatibiltiy` with my rationale.

With that, this is ready to go!
yuvipanda: I found https://github.com/jupyterhub/oauthenticator/issues/709, which I think is closed by this PR. And my change with respect to `manage_groups` reflects:

> do we need to deal with additional groups, not specified upstream somehow? I'd say no, at least for now.

Which I totally agree with. Groups should have a 'single source of truth'.
yuvipanda: Can I get someone to hit merge? Thanks!
consideRatio: @yuvipanda thank you for working this!!!

I'm not able to invest time to review this fully atm :/ I saw some minor fixes I'd like to see updated:

- pr is labelled as breaking, but also references "deprecated in 16.4" - i think we should update one thing or the other (would be 17.0 if it is breaking).
- If its breaking, I request a highlight written about what is breaking in the PR description or under the changelog entry to offload subsequent release work.
- I think its great if the PR title (or as a fallback early in the description) highlights new config names explicitly
yuvipanda: Thanks @consideRatio.

> I'm not able to invest time to review this fully atm :/ 

I don't think you need to, as long as you don't block it. I think it's seen enough eyes, and it's been sitting here for close to 3 months. In general I'd like us to move faster and be less perfectionist. For example, I see tests are failing now - probably unrelated, as they're in globus. This is super demotivating, and I don't want to lose myself as a contributor. I'm also going to proactively merge other PRs and change my attitude a little bit more, being the change I want to see. 

With that said, I appreciate the other 3 points you have raised and the way you have communicated them. I've moved the references to 16.4 to 17.0, and added a section called `Breaking Changes` to call this out explicitly (rather than inside the `Backwards compatibilty` section as before. I prefer the PR title as is, but you're welcome to change it if you wish.
yuvipanda: These tests are failing on `main` as well, so I opened https://github.com/jupyterhub/oauthenticator/issues/743.

Back to now thinking this PR is ready to merge.

Patch:
diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index 2fd07be..f699c16 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -3,12 +3,11 @@ A JupyterHub authenticator class for use with any OAuth2 based identity provider
 """
 
 import os
-from functools import reduce
 
 from jupyterhub.auth import LocalAuthenticator
 from jupyterhub.traitlets import Callable
 from tornado.httpclient import AsyncHTTPClient
-from traitlets import Bool, Dict, Set, Unicode, Union, default
+from traitlets import Bool, Dict, Unicode, Union, default, observe
 
 from .oauth2 import OAuthenticator
 
@@ -22,44 +21,54 @@ class GenericOAuthenticator(OAuthenticator):
         [Unicode(os.environ.get('OAUTH2_GROUPS_KEY', 'groups')), Callable()],
         config=True,
         help="""
-        Userdata groups claim key from returned json for USERDATA_URL.
+        .. deprecated:: 17.0
 
-        Can be a string key name (use periods for nested keys), or a callable
-        that accepts the returned json (as a dict) and returns the groups list.
+        Use :attr:`auth_state_groups_key` instead.
 
-        This configures how group membership in the upstream provider is determined
-        for use by `allowed_groups`, `admin_groups`, etc. If `manage_groups` is True,
-        this will also determine users' _JupyterHub_ group membership.
-        """,
-    )
 
-    allowed_groups = Set(
-        Unicode(),
-        config=True,
-        help="""
-        Allow members of selected groups to sign in.
+        .. versionchanged:: 17.0
 
-        When configuring this you may need to configure `claim_groups_key` as
-        well as it determines the key in the `userdata_url` response that is
-        assumed to list the groups a user is a member of.
+        :attr:`manage_groups` is now required to be `True` to use this functionality
         """,
     )
 
-    admin_groups = Set(
-        Unicode(),
-        config=True,
-        help="""
-        Allow members of selected groups to sign in and consider them as
-        JupyterHub admins.
+    # Initialize value of auth_state_groups_key based on what is in claim_groups_key
+    @default('auth_state_groups_key')
+    def _auth_state_groups_key_default(self):
+        if callable(self.claim_groups_key):
+            # Automatically wrap the claim_groups_key call so it gets what it thinks it should get
+            return lambda auth_state: self.claim_groups_key(
+                auth_state[self.user_auth_state_key]
+            )
+        else:
+            return f"{self.user_auth_state_key}.{self.claim_groups_key}"
+
+    # propagate any changes to claim_groups_key to auth_state_groups_key
+    @observe("claim_groups_key")
+    def _claim_groups_key_changed(self, change):
+        # Emit a deprecation warning directly, without using _deprecated_oauth_aliases,
+        # as it is not a direct replacement for this functionality
+        self.log.warning(
+            "{cls}.claim_groups_key is deprecated since OAuthenticator 17.0, use {cls}.auth_state_groups_key instead".format(
+                cls=self.__class__.__name__,
+            )
+        )
 
-        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.
+        if change.new:
+            if not self.manage_groups:
+                raise ValueError(
+                    f'{change.owner.__class__.__name__}.{change.name} requires {change.owner.__class__.__name__}.manage_groups to also be set'
+                )
 
-        When configuring this you may need to configure `claim_groups_key` as
-        well as it determines the key in the `userdata_url` response that is
-        assumed to list the groups a user is a member of.
-        """,
-    )
+        if callable(change.new):
+            # Automatically wrap the claim_gorups_key call so it gets what it thinks it should get
+            self.auth_state_groups_key = lambda auth_state: self.claim_groups_key(
+                auth_state[self.user_auth_state_key]
+            )
+        else:
+            self.auth_state_groups_key = (
+                f"{self.user_auth_state_key}.{self.claim_groups_key}"
+            )
 
     @default("http_client")
     def _default_http_client(self):
@@ -100,73 +109,6 @@ class GenericOAuthenticator(OAuthenticator):
         """,
     )
 
-    def get_user_groups(self, user_info):
-        """
-        Returns a set of groups the user belongs to based on claim_groups_key
-        and provided user_info.
-
-        - If claim_groups_key is a callable, it is meant to return the groups
-          directly.
-        - If claim_groups_key is a nested dictionary key like
-          "permissions.groups", this function returns
-          user_info["permissions"]["groups"].
-
-        Note that this method is introduced by GenericOAuthenticator and not
-        present in the base class.
-        """
-        if callable(self.claim_groups_key):
-            return set(self.claim_groups_key(user_info))
-        try:
-            return set(reduce(dict.get, self.claim_groups_key.split("."), user_info))
-        except TypeError:
-            self.log.error(
-                f"The claim_groups_key {self.claim_groups_key} does not exist in the user token"
-            )
-            return set()
-
-    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` or `admin_groups`. Note that
-        leaving it at None makes users able to retain an admin status while
-        setting it to False makes it be revoked.
-
-        Also populates groups if `manage_groups` is set.
-        """
-        if self.manage_groups or self.admin_groups:
-            user_info = auth_model["auth_state"][self.user_auth_state_key]
-            user_groups = self.get_user_groups(user_info)
-
-        if self.manage_groups:
-            auth_model["groups"] = sorted(user_groups)
-
-        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
-            auth_model["admin"] = bool(user_groups & self.admin_groups)
-
-        return auth_model
-
-    async def check_allowed(self, username, auth_model):
-        """
-        Overrides the 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 = self.get_user_groups(user_info)
-            if any(user_groups & self.allowed_groups):
-                return True
-
-        # users should be explicitly allowed via config, otherwise they aren't
-        return False
-
 
 class LocalGenericOAuthenticator(LocalAuthenticator, GenericOAuthenticator):
     """A version that mixes in local system user creation"""
diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index 718092d..c36fb84 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -8,6 +8,7 @@ import base64
 import json
 import os
 import uuid
+from functools import reduce
 from urllib.parse import quote, urlencode, urlparse, urlunparse
 
 import jwt
@@ -20,7 +21,19 @@ from tornado.auth import OAuth2Mixin
 from tornado.httpclient import AsyncHTTPClient, HTTPClientError, HTTPRequest
 from tornado.httputil import url_concat
 from tornado.log import app_log
-from traitlets import Any, Bool, Callable, Dict, List, Unicode, Union, default, validate
+from traitlets import (
+    Any,
+    Bool,
+    Callable,
+    Dict,
+    List,
+    Set,
+    Unicode,
+    Union,
+    default,
+    observe,
+    validate,
+)
 
 
 def guess_callback_uri(protocol, host, hub_server_url):
@@ -316,6 +329,54 @@ class OAuthenticator(Authenticator):
         """,
     )
 
+    allowed_groups = Set(
+        Unicode(),
+        config=True,
+        help="""
+        Allow members of selected groups to log in.
+
+        Requires `manage_groups` to also be `True`.
+        """,
+    )
+
+    admin_groups = Set(
+        Unicode(),
+        config=True,
+        help="""
+        Allow members of selected 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.
+
+        Requires `manage_groups` to also be `True`.
+        """,
+    )
+
+    auth_state_groups_key = Union(
+        [Unicode(), Callable()],
+        config=True,
+        help="""
+        Determine groups this user belongs based on contents of auth_state.
+
+        Can be a string key name (use periods for nested keys), or a callable
+        that accepts the auth state (as a dict) and returns the groups list.
+
+        Requires `manage_groups` to also be `True`.
+        """,
+    )
+
+    @observe("allowed_groups", "admin_groups", "auth_state_groups_key")
+    def _requires_manage_groups(self, change):
+        """
+        Validate that group management keys are only set when manage_groups is also True
+        """
+        if change.new:
+            if not self.manage_groups:
+                raise ValueError(
+                    f'{change.owner.__class__.__name__}.{change.name} requires {change.owner.__class__.__name__}.manage_groups to also be set'
+                )
+
     authorize_url = Unicode(
         config=True,
         help="""
@@ -1025,6 +1086,28 @@ class OAuthenticator(Authenticator):
             self.user_auth_state_key: user_info,
         }
 
+    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.
+
+        - If auth_state_groups_key is a callable, it returns the list of groups directly.
+        - If auth_state_groups_key is a nested dictionary key like
+          "permissions.groups", this function returns
+          auth_state["permissions"]["groups"].
+        """
+        if callable(self.auth_state_groups_key):
+            return set(self.auth_state_groups_key(auth_state))
+        try:
+            return set(
+                reduce(dict.get, self.auth_state_groups_key.split("."), auth_state)
+            )
+        except TypeError:
+            self.log.error(
+                f"The auth_state_groups_key {self.auth_state_groups_key} does not exist in the auth_model. Available keys are: {auth_state.keys()}"
+            )
+            return set()
+
     async def update_auth_model(self, auth_model):
         """
         Updates and returns the `auth_model` dict.
@@ -1040,6 +1123,17 @@ class OAuthenticator(Authenticator):
 
         Called by the :meth:`oauthenticator.OAuthenticator.authenticate`
         """
+        if self.manage_groups:
+            auth_state = auth_model["auth_state"]
+            user_groups = self.get_user_groups(auth_state)
+
+            auth_model["groups"] = sorted(user_groups)
+
+            if self.admin_groups:
+                if not auth_model["admin"]:
+                    # auth_model["admin"] being True means the user was in admin_users
+                    # so their group membership should not affect their admin status
+                    auth_model["admin"] = bool(user_groups & self.admin_groups)
         return auth_model
 
     async def authenticate(self, handler, data=None, **kwargs):
@@ -1125,6 +1219,13 @@ class OAuthenticator(Authenticator):
         if username in self.allowed_users:
             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 any(user_groups & self.allowed_groups):
+                return True
+
         # users should be explicitly allowed via config, otherwise they aren't
         return False
 
@@ -1177,6 +1278,7 @@ class OAuthenticator(Authenticator):
             self.observe(
                 self._deprecated_oauth_trait, names=list(self._deprecated_oauth_aliases)
             )
+
         super().__init__(**kwargs)
 
 


Test patch:
diff --git a/oauthenticator/tests/test_generic.py b/oauthenticator/tests/test_generic.py
index 07e6752..1f656a4 100644
--- a/oauthenticator/tests/test_generic.py
+++ b/oauthenticator/tests/test_generic.py
@@ -104,16 +104,27 @@ 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"}}, True, None),
-        ("07", {"allowed_groups": {"test-user-not-in-group"}}, False, None),
-        ("08", {"admin_groups": {"group1"}}, True, True),
-        ("09", {"admin_groups": {"test-user-not-in-group"}}, False, False),
+        ("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",
             {
                 "allow_all": False,
                 "allowed_users": {"not-test-user"},
+                "manage_groups": True,
             },
             False,
             None,
@@ -123,6 +134,7 @@ def get_authenticator_variant(generic_client, userdata_from_id_token):
             {
                 "allowed_users": {"not-test-user"},
                 "admin_users": {"user1"},
+                "manage_groups": True,
             },
             True,
             True,
@@ -132,6 +144,7 @@ def get_authenticator_variant(generic_client, userdata_from_id_token):
             {
                 "allowed_groups": {"group1"},
                 "admin_groups": {"group1"},
+                "manage_groups": True,
             },
             True,
             True,
@@ -141,6 +154,7 @@ def get_authenticator_variant(generic_client, userdata_from_id_token):
             {
                 "allowed_groups": {"group1"},
                 "admin_groups": {"test-user-not-in-group"},
+                "manage_groups": True,
             },
             True,
             False,
@@ -150,6 +164,7 @@ def get_authenticator_variant(generic_client, userdata_from_id_token):
             {
                 "allowed_groups": {"test-user-not-in-group"},
                 "admin_groups": {"group1"},
+                "manage_groups": True,
             },
             True,
             True,
@@ -159,6 +174,7 @@ def get_authenticator_variant(generic_client, userdata_from_id_token):
             {
                 "allowed_groups": {"test-user-not-in-group"},
                 "admin_groups": {"test-user-not-in-group"},
+                "manage_groups": True,
             },
             False,
             False,
@@ -168,6 +184,7 @@ def get_authenticator_variant(generic_client, userdata_from_id_token):
             {
                 "admin_users": {"user1"},
                 "admin_groups": {"group1"},
+                "manage_groups": True,
             },
             True,
             True,
@@ -177,6 +194,7 @@ def get_authenticator_variant(generic_client, userdata_from_id_token):
             {
                 "admin_users": {"user1"},
                 "admin_groups": {"test-user-not-in-group"},
+                "manage_groups": True,
             },
             True,
             True,
@@ -186,6 +204,7 @@ def get_authenticator_variant(generic_client, userdata_from_id_token):
             {
                 "admin_users": {"not-test-user"},
                 "admin_groups": {"group1"},
+                "manage_groups": True,
             },
             True,
             True,
@@ -195,6 +214,7 @@ def get_authenticator_variant(generic_client, userdata_from_id_token):
             {
                 "admin_users": {"not-test-user"},
                 "admin_groups": {"test-user-not-in-group"},
+                "manage_groups": True,
             },
             False,
             False,
@@ -343,6 +363,7 @@ async def test_generic_claim_groups_key_callable(get_authenticator, generic_clie
     c = Config()
     c.GenericOAuthenticator.claim_groups_key = lambda r: r["policies"]["roles"]
     c.GenericOAuthenticator.allowed_groups = ["super_user"]
+    c.GenericOAuthenticator.manage_groups = True
     authenticator = get_authenticator(config=c)
 
     handled_user_model = user_model("user1", policies={"roles": ["super_user"]})
@@ -358,6 +379,42 @@ async def test_generic_claim_groups_key_nested_strings(
     c = Config()
     c.GenericOAuthenticator.claim_groups_key = "permissions.groups"
     c.GenericOAuthenticator.admin_groups = ["super_user"]
+    c.GenericOAuthenticator.manage_groups = True
+    authenticator = get_authenticator(config=c)
+
+    handled_user_model = user_model("user1", permissions={"groups": ["super_user"]})
+    handler = generic_client.handler_for_user(handled_user_model)
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+
+    assert auth_model
+    assert auth_model["admin"]
+
+
+async def test_generic_auth_state_groups_key_callable(
+    get_authenticator, generic_client
+):
+    c = Config()
+    c.GenericOAuthenticator.auth_state_groups_key = lambda auth_state: auth_state[
+        "oauth_user"
+    ]["policies"]["roles"]
+    c.GenericOAuthenticator.allowed_groups = ["super_user"]
+    c.GenericOAuthenticator.manage_groups = True
+    authenticator = get_authenticator(config=c)
+
+    handled_user_model = user_model("user1", policies={"roles": ["super_user"]})
+    handler = generic_client.handler_for_user(handled_user_model)
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+
+    assert auth_model
+
+
+async def test_generic_auth_state_groups_key_nested_strings(
+    get_authenticator, generic_client
+):
+    c = Config()
+    c.GenericOAuthenticator.auth_state_groups_key = "oauth_user.permissions.groups"
+    c.GenericOAuthenticator.admin_groups = ["super_user"]
+    c.GenericOAuthenticator.manage_groups = True
     authenticator = get_authenticator(config=c)
 
     handled_user_model = user_model("user1", permissions={"groups": ["super_user"]})
@@ -368,6 +425,26 @@ async def test_generic_claim_groups_key_nested_strings(
     assert auth_model["admin"]
 
 
+@mark.parametrize(
+    ("trait_name", "value"),
+    [
+        ("auth_state_groups_key", "oauth_user.permissions.groups"),
+        ("admin_groups", ["super_users"]),
+        ("allowed_groups", ["all_users"]),
+    ],
+)
+async def test_generic_manage_groups_required(get_authenticator, trait_name, value):
+    c = Config()
+    setattr(c.GenericOAuthenticator, trait_name, value)
+    with raises(
+        ValueError,
+        match=re.escape(
+            rf'GenericOAuthenticator.{trait_name} requires GenericOAuthenticator.manage_groups to also be set'
+        ),
+    ):
+        get_authenticator(config=c)
+
+
 @mark.parametrize(
     "name, allowed",
     [
diff --git a/oauthenticator/tests/test_openshift.py b/oauthenticator/tests/test_openshift.py
index 07a6142..8421cf0 100644
--- a/oauthenticator/tests/test_openshift.py
+++ b/oauthenticator/tests/test_openshift.py
@@ -37,10 +37,20 @@ 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"}}, True, None),
-        ("07", {"allowed_groups": {"test-user-not-in-group"}}, False, None),
-        ("08", {"admin_groups": {"group1"}}, True, True),
-        ("09", {"admin_groups": {"test-user-not-in-group"}}, False, False),
+        ("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",
@@ -65,6 +75,7 @@ def user_model():
             {
                 "allowed_groups": {"group1"},
                 "admin_groups": {"group1"},
+                "manage_groups": True,
             },
             True,
             True,
@@ -74,6 +85,7 @@ def user_model():
             {
                 "allowed_groups": {"group1"},
                 "admin_groups": {"test-user-not-in-group"},
+                "manage_groups": True,
             },
             True,
             False,
@@ -83,6 +95,7 @@ def user_model():
             {
                 "allowed_groups": {"test-user-not-in-group"},
                 "admin_groups": {"group1"},
+                "manage_groups": True,
             },
             True,
             True,
@@ -92,6 +105,7 @@ def user_model():
             {
                 "allowed_groups": {"test-user-not-in-group"},
                 "admin_groups": {"test-user-not-in-group"},
+                "manage_groups": True,
             },
             False,
             False,
@@ -101,6 +115,7 @@ def user_model():
             {
                 "admin_users": {"user1"},
                 "admin_groups": {"group1"},
+                "manage_groups": True,
             },
             True,
             True,
@@ -110,6 +125,7 @@ def user_model():
             {
                 "admin_users": {"user1"},
                 "admin_groups": {"test-user-not-in-group"},
+                "manage_groups": True,
             },
             True,
             True,
@@ -119,6 +135,7 @@ def user_model():
             {
                 "admin_users": {"not-test-user"},
                 "admin_groups": {"group1"},
+                "manage_groups": True,
             },
             True,
             True,
@@ -128,6 +145,7 @@ def user_model():
             {
                 "admin_users": {"not-test-user"},
                 "admin_groups": {"test-user-not-in-group"},
+                "manage_groups": True,
             },
             False,
             False,