From f800551351e6e3c6d0ddfa8d9a905e52c7494dd1 Mon Sep 17 00:00:00 2001
From: YuviPanda <yuvipanda@gmail.com>
Date: Thu, 25 Apr 2024 12:39:21 -0700
Subject: [PATCH 01/20] Move allowed_groups and admin_groups to base
 authenticator

A simplification of https://github.com/jupyterhub/oauthenticator/pull/735,
moving 2 of the 3 traitlets. This is a straight up move, without any functional
breaking changes.

- `admin_groups` allows setting members of some groups as admins.
- `allowed_groups` allows setting what groups should be allowed to login.

Both of these are more useful with claim_groups_key, as that allows
an *external* party to drive group memberships. Without that, I guess
primarily this depends on membership within the JupyterHub admin UI.

Splitting this up helps us get this moving faster, as figuring out how
to move `claim_groups_key` is going to be slightly more involved.
---
 oauthenticator/generic.py | 71 ---------------------------------------
 oauthenticator/oauth2.py  | 42 ++++++++++++++++++++++-
 2 files changed, 41 insertions(+), 72 deletions(-)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index 2fd07be7..4a6f983f 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -33,34 +33,6 @@ def _login_service_default(self):
         """,
     )
 
-    allowed_groups = Set(
-        Unicode(),
-        config=True,
-        help="""
-        Allow members of selected groups to sign in.
-
-        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.
-        """,
-    )
-
-    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.
-
-        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.
-        """,
-    )
-
     @default("http_client")
     def _default_http_client(self):
         return AsyncHTTPClient(
@@ -124,49 +96,6 @@ def get_user_groups(self, user_info):
             )
             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 718092d4..780c6650 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -20,7 +20,7 @@
 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, Unicode, Union, default, Set, validate
 
 
 def guess_callback_uri(protocol, host, hub_server_url):
@@ -316,6 +316,27 @@ class OAuthenticator(Authenticator):
         """,
     )
 
+    allowed_groups = Set(
+        Unicode(),
+        config=True,
+        help="""
+        Allow members of selected groups to log in.
+        """,
+    )
+
+    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.
+        """,
+    )
+
+
     authorize_url = Unicode(
         config=True,
         help="""
@@ -1040,6 +1061,18 @@ async def update_auth_model(self, auth_model):
 
         Called by the :meth:`oauthenticator.OAuthenticator.authenticate`
         """
+        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 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 +1158,13 @@ async def check_allowed(self, username, auth_model):
         if username in self.allowed_users:
             return True
 
+        # allow users who are members of allowed_groups
+        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
 

From 5d18e62aee0a437e4c24729bafc6632a382b127e Mon Sep 17 00:00:00 2001
From: YuviPanda <yuvipanda@gmail.com>
Date: Thu, 25 Apr 2024 13:03:04 -0700
Subject: [PATCH 02/20] Convert claim_groups_key to auth_model_groups_key

---
 oauthenticator/generic.py | 34 +++++++++------------------------
 oauthenticator/oauth2.py  | 40 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 26 deletions(-)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index 4a6f983f..c11b5eb7 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -8,7 +8,7 @@
 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, Set, Unicode, Union, default, observe
 
 from .oauth2 import OAuthenticator
 
@@ -33,6 +33,14 @@ def _login_service_default(self):
         """,
     )
 
+    @observe("claim_groups_key")
+    def _claim_groups_key_changed(self, change):
+        if callable(change.new):
+            # Automatically wrap the claim_gorups_key call so it gets what it thinks it should get
+            self.auth_model_groups_key = lambda auth_model: self.claim_groups_key(auth_model["auth_state"][self.user_auth_state_key])
+        else:
+            self.auth_model_groups_key = f"auth_state.{self.user_auth_state_key}.{self.claim_groups_key}"
+
     @default("http_client")
     def _default_http_client(self):
         return AsyncHTTPClient(
@@ -72,30 +80,6 @@ def _default_http_client(self):
         """,
     )
 
-    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()
-
 
 class LocalGenericOAuthenticator(LocalAuthenticator, GenericOAuthenticator):
     """A version that mixes in local system user creation"""
diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index 780c6650..690fc137 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -7,6 +7,7 @@
 import base64
 import json
 import os
+from functools import reduce
 import uuid
 from urllib.parse import quote, urlencode, urlparse, urlunparse
 
@@ -336,6 +337,22 @@ class OAuthenticator(Authenticator):
         """,
     )
 
+    auth_model_groups_key = Union(
+        [Unicode(), Callable()],
+        config=True,
+        help="""
+        Determine groups this user belongs based on contents of the auth model.
+
+        Can be a string key name (use periods for nested keys), or a callable
+        that accepts the auth model (as a dict) and returns the groups list.
+
+        TODO: Document what auth_model actually looks like.
+
+        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.
+        """,
+    )
 
     authorize_url = Unicode(
         config=True,
@@ -1046,6 +1063,27 @@ def build_auth_state_dict(self, token_info, user_info):
             self.user_auth_state_key: user_info,
         }
 
+    def get_user_groups(self, auth_model: dict):
+        """
+        Returns a set of groups the user belongs to based on claim_groups_key
+        and provided auth_model.
+
+        - 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
+          auth_model["permissions"]["groups"].
+        """
+        if callable(self.claim_groups_key):
+            return set(self.auth_model_groups_key(auth_model))
+        try:
+            return set(reduce(dict.get, self.auth_model_groups_key.split("."), auth_model))
+        except TypeError:
+            self.log.error(
+                f"The auth_model_groups_key {self.auth_model_groups_key} does not exist in the auth_model. Available keys are: {auth_model.keys()}"
+            )
+            return set()
+
     async def update_auth_model(self, auth_model):
         """
         Updates and returns the `auth_model` dict.
@@ -1063,7 +1101,7 @@ async def update_auth_model(self, auth_model):
         """
         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)
+            user_groups = self.get_user_groups(auth_model)
 
         if self.manage_groups:
             auth_model["groups"] = sorted(user_groups)

From 667238ee3ceb79fa391fcc6acbbfae75e4765b40 Mon Sep 17 00:00:00 2001
From: "pre-commit-ci[bot]"
 <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date: Thu, 25 Apr 2024 20:03:59 +0000
Subject: [PATCH 03/20] [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
---
 oauthenticator/generic.py | 11 +++++++----
 oauthenticator/oauth2.py  | 19 ++++++++++++++++---
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index c11b5eb7..c2e4e36c 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -3,12 +3,11 @@
 """
 
 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, observe
+from traitlets import Bool, Dict, Unicode, Union, default, observe
 
 from .oauth2 import OAuthenticator
 
@@ -37,9 +36,13 @@ def _login_service_default(self):
     def _claim_groups_key_changed(self, change):
         if callable(change.new):
             # Automatically wrap the claim_gorups_key call so it gets what it thinks it should get
-            self.auth_model_groups_key = lambda auth_model: self.claim_groups_key(auth_model["auth_state"][self.user_auth_state_key])
+            self.auth_model_groups_key = lambda auth_model: self.claim_groups_key(
+                auth_model["auth_state"][self.user_auth_state_key]
+            )
         else:
-            self.auth_model_groups_key = f"auth_state.{self.user_auth_state_key}.{self.claim_groups_key}"
+            self.auth_model_groups_key = (
+                f"auth_state.{self.user_auth_state_key}.{self.claim_groups_key}"
+            )
 
     @default("http_client")
     def _default_http_client(self):
diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index 690fc137..7c3667de 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -7,8 +7,8 @@
 import base64
 import json
 import os
-from functools import reduce
 import uuid
+from functools import reduce
 from urllib.parse import quote, urlencode, urlparse, urlunparse
 
 import jwt
@@ -21,7 +21,18 @@
 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, Set, validate
+from traitlets import (
+    Any,
+    Bool,
+    Callable,
+    Dict,
+    List,
+    Set,
+    Unicode,
+    Union,
+    default,
+    validate,
+)
 
 
 def guess_callback_uri(protocol, host, hub_server_url):
@@ -1077,7 +1088,9 @@ def get_user_groups(self, auth_model: dict):
         if callable(self.claim_groups_key):
             return set(self.auth_model_groups_key(auth_model))
         try:
-            return set(reduce(dict.get, self.auth_model_groups_key.split("."), auth_model))
+            return set(
+                reduce(dict.get, self.auth_model_groups_key.split("."), auth_model)
+            )
         except TypeError:
             self.log.error(
                 f"The auth_model_groups_key {self.auth_model_groups_key} does not exist in the auth_model. Available keys are: {auth_model.keys()}"

From d39ae745028376cae7a32471c7872b5145075a4b Mon Sep 17 00:00:00 2001
From: YuviPanda <yuvipanda@gmail.com>
Date: Thu, 25 Apr 2024 13:04:53 -0700
Subject: [PATCH 04/20] Remove unused user_info variable

---
 oauthenticator/oauth2.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index 7c3667de..d4b38069 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -1113,7 +1113,6 @@ async def update_auth_model(self, auth_model):
         Called by the :meth:`oauthenticator.OAuthenticator.authenticate`
         """
         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(auth_model)
 
         if self.manage_groups:

From e6eb8a13a7e89799ede59d3e2e1b848fd216807c Mon Sep 17 00:00:00 2001
From: YuviPanda <yuvipanda@gmail.com>
Date: Thu, 25 Apr 2024 13:13:16 -0700
Subject: [PATCH 05/20] Fix some missed renames

---
 oauthenticator/oauth2.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index d4b38069..ee88681a 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -1076,16 +1076,16 @@ def build_auth_state_dict(self, token_info, user_info):
 
     def get_user_groups(self, auth_model: dict):
         """
-        Returns a set of groups the user belongs to based on claim_groups_key
+        Returns a set of groups the user belongs to based on auth_model_groups_key
         and provided auth_model.
 
-        - If claim_groups_key is a callable, it is meant to return the groups
+        - If auth_model_groups_key is a callable, it is meant to return the groups
           directly.
-        - If claim_groups_key is a nested dictionary key like
+        - If auth_model_groups_key is a nested dictionary key like
           "permissions.groups", this function returns
           auth_model["permissions"]["groups"].
         """
-        if callable(self.claim_groups_key):
+        if callable(self.auth_model_groups_key):
             return set(self.auth_model_groups_key(auth_model))
         try:
             return set(

From 1116adf195591b88476295e487c29a924365d8a3 Mon Sep 17 00:00:00 2001
From: YuviPanda <yuvipanda@gmail.com>
Date: Tue, 30 Apr 2024 20:55:01 -0700
Subject: [PATCH 06/20] Pass in get_user_groups wherever it is called

---
 oauthenticator/oauth2.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index ee88681a..fc159aca 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -1210,8 +1210,7 @@ async def check_allowed(self, username, auth_model):
 
         # allow users who are members of allowed_groups
         if self.allowed_groups:
-            user_info = auth_model["auth_state"][self.user_auth_state_key]
-            user_groups = self.get_user_groups(user_info)
+            user_groups = self.get_user_groups(auth_model)
             if any(user_groups & self.allowed_groups):
                 return True
 

From c3389f0ea102b87a75337c4621d5c302b260d8d8 Mon Sep 17 00:00:00 2001
From: YuviPanda <yuvipanda@gmail.com>
Date: Tue, 30 Apr 2024 21:19:04 -0700
Subject: [PATCH 07/20] Set defaults correctly for auth_model_groups_key

---
 oauthenticator/generic.py | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index c2e4e36c..39325ae6 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -32,6 +32,19 @@ def _login_service_default(self):
         """,
     )
 
+    # Initialize value of auth_model_groups_key based on what is in claim_groups_key
+    @default('auth_model_groups_key')
+    def _auth_model_groups_key_default(self):
+        if callable(self.claim_groups_key):
+            # Automatically wrap the claim_gorups_key call so it gets what it thinks it should get
+            return lambda auth_model: self.claim_groups_key(
+                auth_model["auth_state"][self.user_auth_state_key]
+            )
+        else:
+            return f"auth_state.{self.user_auth_state_key}.{self.claim_groups_key}"
+
+
+    # propagate any changes to claim_groups_key to auth_model_groups_key
     @observe("claim_groups_key")
     def _claim_groups_key_changed(self, change):
         if callable(change.new):

From 5fa43e1b38a8feda3000e72164e3241ba5b9ac0a Mon Sep 17 00:00:00 2001
From: "pre-commit-ci[bot]"
 <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date: Wed, 1 May 2024 04:19:30 +0000
Subject: [PATCH 08/20] [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
---
 oauthenticator/generic.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index 39325ae6..72ab8267 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -43,7 +43,6 @@ def _auth_model_groups_key_default(self):
         else:
             return f"auth_state.{self.user_auth_state_key}.{self.claim_groups_key}"
 
-
     # propagate any changes to claim_groups_key to auth_model_groups_key
     @observe("claim_groups_key")
     def _claim_groups_key_changed(self, change):

From 75f8bfdf79c70b25f6cf78c877767d0c71b40859 Mon Sep 17 00:00:00 2001
From: YuviPanda <yuvipanda@gmail.com>
Date: Wed, 1 May 2024 20:02:44 -0700
Subject: [PATCH 09/20] Add a couple of tests

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

diff --git a/oauthenticator/tests/test_generic.py b/oauthenticator/tests/test_generic.py
index 07e6752a..8934cad3 100644
--- a/oauthenticator/tests/test_generic.py
+++ b/oauthenticator/tests/test_generic.py
@@ -368,6 +368,35 @@ async def test_generic_claim_groups_key_nested_strings(
     assert auth_model["admin"]
 
 
+async def test_generic_auth_model_groups_key_callable(get_authenticator, generic_client):
+    c = Config()
+    c.GenericOAuthenticator.auth_model_groups_key = lambda r: r["auth_state"]["oauth_user"]["policies"]["roles"]
+    c.GenericOAuthenticator.allowed_groups = ["super_user"]
+    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_model_groups_key_nested_strings(
+    get_authenticator, generic_client
+):
+    c = Config()
+    c.GenericOAuthenticator.auth_model_groups_key = "auth_state.oauth_user.permissions.groups"
+    c.GenericOAuthenticator.admin_groups = ["super_user"]
+    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"]
+
+
 @mark.parametrize(
     "name, allowed",
     [

From b50dbac66483483ba6d3c439224812b1a505688d Mon Sep 17 00:00:00 2001
From: YuviPanda <yuvipanda@gmail.com>
Date: Thu, 2 May 2024 16:48:11 -0700
Subject: [PATCH 10/20] Use auth_state, not auth_model

auth_model is currently not documented nor exposed to customizable
code (without inheriting from a class and modifying it). So instead
of documenting auth_model and trying to keep that stable, we rely instead
on auth_model being populated.
---
 oauthenticator/generic.py            | 22 +++++++++---------
 oauthenticator/oauth2.py             | 34 ++++++++++++++--------------
 oauthenticator/tests/test_generic.py |  8 +++----
 3 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index 72ab8267..1fb5a301 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -32,28 +32,28 @@ def _login_service_default(self):
         """,
     )
 
-    # Initialize value of auth_model_groups_key based on what is in claim_groups_key
-    @default('auth_model_groups_key')
-    def _auth_model_groups_key_default(self):
+    # 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_gorups_key call so it gets what it thinks it should get
-            return lambda auth_model: self.claim_groups_key(
-                auth_model["auth_state"][self.user_auth_state_key]
+            return lambda auth_state: self.claim_groups_key(
+                auth_state[self.user_auth_state_key]
             )
         else:
-            return f"auth_state.{self.user_auth_state_key}.{self.claim_groups_key}"
+            return f"{self.user_auth_state_key}.{self.claim_groups_key}"
 
-    # propagate any changes to claim_groups_key to auth_model_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):
         if callable(change.new):
             # Automatically wrap the claim_gorups_key call so it gets what it thinks it should get
-            self.auth_model_groups_key = lambda auth_model: self.claim_groups_key(
-                auth_model["auth_state"][self.user_auth_state_key]
+            self.auth_state_groups_key = lambda auth_state: self.claim_groups_key(
+                auth_state[self.user_auth_state_key]
             )
         else:
-            self.auth_model_groups_key = (
-                f"auth_state.{self.user_auth_state_key}.{self.claim_groups_key}"
+            self.auth_state_groups_key = (
+                f"{self.user_auth_state_key}.{self.claim_groups_key}"
             )
 
     @default("http_client")
diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index fc159aca..1956a917 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -348,16 +348,14 @@ class OAuthenticator(Authenticator):
         """,
     )
 
-    auth_model_groups_key = Union(
+    auth_state_groups_key = Union(
         [Unicode(), Callable()],
         config=True,
         help="""
-        Determine groups this user belongs based on contents of the auth model.
+        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 model (as a dict) and returns the groups list.
-
-        TODO: Document what auth_model actually looks like.
+        that accepts the auth state (as a dict) and returns the groups list.
 
         This configures how group membership in the upstream provider is determined
         for use by `allowed_groups`, `admin_groups`, etc. If `manage_groups` is True,
@@ -1074,26 +1072,26 @@ def build_auth_state_dict(self, token_info, user_info):
             self.user_auth_state_key: user_info,
         }
 
-    def get_user_groups(self, auth_model: dict):
+    def get_user_groups(self, auth_state: dict):
         """
-        Returns a set of groups the user belongs to based on auth_model_groups_key
-        and provided auth_model.
+        Returns a set of groups the user belongs to based on auth_state_groups_key
+        and provided auth_state.
 
-        - If auth_model_groups_key is a callable, it is meant to return the groups
+        - If auth_state_groups_key is a callable, it is meant to return the groups
           directly.
-        - If auth_model_groups_key is a nested dictionary key like
+        - If auth_state_groups_key is a nested dictionary key like
           "permissions.groups", this function returns
-          auth_model["permissions"]["groups"].
+          auth_state["permissions"]["groups"].
         """
-        if callable(self.auth_model_groups_key):
-            return set(self.auth_model_groups_key(auth_model))
+        if callable(self.auth_state_groups_key):
+            return set(self.auth_state_groups_key(auth_state))
         try:
             return set(
-                reduce(dict.get, self.auth_model_groups_key.split("."), auth_model)
+                reduce(dict.get, self.auth_state_groups_key.split("."), auth_state)
             )
         except TypeError:
             self.log.error(
-                f"The auth_model_groups_key {self.auth_model_groups_key} does not exist in the auth_model. Available keys are: {auth_model.keys()}"
+                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()
 
@@ -1113,7 +1111,8 @@ async def update_auth_model(self, auth_model):
         Called by the :meth:`oauthenticator.OAuthenticator.authenticate`
         """
         if self.manage_groups or self.admin_groups:
-            user_groups = self.get_user_groups(auth_model)
+            auth_state = auth_model["auth_state"]
+            user_groups = self.get_user_groups(auth_state)
 
         if self.manage_groups:
             auth_model["groups"] = sorted(user_groups)
@@ -1210,7 +1209,8 @@ async def check_allowed(self, username, auth_model):
 
         # allow users who are members of allowed_groups
         if self.allowed_groups:
-            user_groups = self.get_user_groups(auth_model)
+            auth_state = auth_model["auth_state"]
+            user_groups = self.get_user_groups(auth_state)
             if any(user_groups & self.allowed_groups):
                 return True
 
diff --git a/oauthenticator/tests/test_generic.py b/oauthenticator/tests/test_generic.py
index 8934cad3..c23e7ef3 100644
--- a/oauthenticator/tests/test_generic.py
+++ b/oauthenticator/tests/test_generic.py
@@ -368,9 +368,9 @@ async def test_generic_claim_groups_key_nested_strings(
     assert auth_model["admin"]
 
 
-async def test_generic_auth_model_groups_key_callable(get_authenticator, generic_client):
+async def test_generic_auth_state_groups_key_callable(get_authenticator, generic_client):
     c = Config()
-    c.GenericOAuthenticator.auth_model_groups_key = lambda r: r["auth_state"]["oauth_user"]["policies"]["roles"]
+    c.GenericOAuthenticator.auth_state_groups_key = lambda auth_state: auth_state["oauth_user"]["policies"]["roles"]
     c.GenericOAuthenticator.allowed_groups = ["super_user"]
     authenticator = get_authenticator(config=c)
 
@@ -381,11 +381,11 @@ async def test_generic_auth_model_groups_key_callable(get_authenticator, generic
     assert auth_model
 
 
-async def test_generic_auth_model_groups_key_nested_strings(
+async def test_generic_auth_state_groups_key_nested_strings(
     get_authenticator, generic_client
 ):
     c = Config()
-    c.GenericOAuthenticator.auth_model_groups_key = "auth_state.oauth_user.permissions.groups"
+    c.GenericOAuthenticator.auth_state_groups_key = "oauth_user.permissions.groups"
     c.GenericOAuthenticator.admin_groups = ["super_user"]
     authenticator = get_authenticator(config=c)
 

From 4985d0dee9e8802474538229522ea4ab2c39da5d Mon Sep 17 00:00:00 2001
From: "pre-commit-ci[bot]"
 <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date: Thu, 2 May 2024 23:50:36 +0000
Subject: [PATCH 11/20] [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
---
 oauthenticator/tests/test_generic.py | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/oauthenticator/tests/test_generic.py b/oauthenticator/tests/test_generic.py
index c23e7ef3..20e30368 100644
--- a/oauthenticator/tests/test_generic.py
+++ b/oauthenticator/tests/test_generic.py
@@ -368,9 +368,13 @@ async def test_generic_claim_groups_key_nested_strings(
     assert auth_model["admin"]
 
 
-async def test_generic_auth_state_groups_key_callable(get_authenticator, generic_client):
+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.auth_state_groups_key = lambda auth_state: auth_state[
+        "oauth_user"
+    ]["policies"]["roles"]
     c.GenericOAuthenticator.allowed_groups = ["super_user"]
     authenticator = get_authenticator(config=c)
 

From cc40e4e9f6b2e58b41ff76ce2274229490df781c Mon Sep 17 00:00:00 2001
From: YuviPanda <yuvipanda@gmail.com>
Date: Thu, 2 May 2024 17:03:43 -0700
Subject: [PATCH 12/20] Mark claim_groups_key as deprecated

---
 oauthenticator/generic.py | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index 1fb5a301..b2fc46a8 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -21,14 +21,9 @@ def _login_service_default(self):
         [Unicode(os.environ.get('OAUTH2_GROUPS_KEY', 'groups')), Callable()],
         config=True,
         help="""
-        Userdata groups claim key from returned json for USERDATA_URL.
+        .. deprecated:: 16.4
 
-        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.
-
-        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.
+        Use :attr:`auth_state_groups_key` instead.
         """,
     )
 
@@ -46,6 +41,12 @@ def _auth_state_groups_key_default(self):
     # 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 OAuthenticatort 16.4, use {cls}.auth_state_groups_key instead",
+            cls=self.__class__.__name__
+        )
         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(

From 3f39887b264e072b2b4a56f48e5866d196807a86 Mon Sep 17 00:00:00 2001
From: "pre-commit-ci[bot]"
 <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date: Fri, 3 May 2024 00:04:05 +0000
Subject: [PATCH 13/20] [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
---
 oauthenticator/generic.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index b2fc46a8..e9d1843d 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -45,7 +45,7 @@ def _claim_groups_key_changed(self, change):
         # as it is not a direct replacement for this functionality
         self.log.warning(
             "{cls}.claim_groups_key is deprecated since OAuthenticatort 16.4, use {cls}.auth_state_groups_key instead",
-            cls=self.__class__.__name__
+            cls=self.__class__.__name__,
         )
         if callable(change.new):
             # Automatically wrap the claim_gorups_key call so it gets what it thinks it should get

From 9408c372e2a38435ef9ef0c19bd6daee12eed4da Mon Sep 17 00:00:00 2001
From: YuviPanda <yuvipanda@gmail.com>
Date: Thu, 2 May 2024 17:05:39 -0700
Subject: [PATCH 14/20] Call string format explicilty

---
 oauthenticator/generic.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index e9d1843d..8bcde877 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -44,8 +44,9 @@ 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 OAuthenticatort 16.4, use {cls}.auth_state_groups_key instead",
-            cls=self.__class__.__name__,
+            "{cls}.claim_groups_key is deprecated since OAuthenticatort 16.4, use {cls}.auth_state_groups_key instead".format(
+                cls=self.__class__.__name__,
+            )
         )
         if callable(change.new):
             # Automatically wrap the claim_gorups_key call so it gets what it thinks it should get

From e7cdf2598ae2a6c05a090073a7954e2747c41aa4 Mon Sep 17 00:00:00 2001
From: Yuvi Panda <yuvipanda@gmail.com>
Date: Tue, 7 May 2024 10:41:40 -0700
Subject: [PATCH 15/20] Fix typo

Co-authored-by: Simon Li <orpheus+devel@gmail.com>
---
 oauthenticator/generic.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index 8bcde877..ca329284 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -44,7 +44,7 @@ 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 OAuthenticatort 16.4, use {cls}.auth_state_groups_key instead".format(
+            "{cls}.claim_groups_key is deprecated since OAuthenticator 16.4, use {cls}.auth_state_groups_key instead".format(
                 cls=self.__class__.__name__,
             )
         )

From 49f71fab7542644227cda5b771c2c1fb00455c9b Mon Sep 17 00:00:00 2001
From: Yuvi Panda <yuvipanda@gmail.com>
Date: Tue, 7 May 2024 10:45:54 -0700
Subject: [PATCH 16/20] Fix typo

Co-authored-by: Simon Li <orpheus+devel@gmail.com>
---
 oauthenticator/generic.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index ca329284..b7177b76 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -31,7 +31,7 @@ def _login_service_default(self):
     @default('auth_state_groups_key')
     def _auth_state_groups_key_default(self):
         if callable(self.claim_groups_key):
-            # Automatically wrap the claim_gorups_key call so it gets what it thinks it should get
+            # 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]
             )

From d6d70640d133c8865cdd4fb308cab463bc2dd8fc Mon Sep 17 00:00:00 2001
From: Yuvi Panda <yuvipanda@gmail.com>
Date: Tue, 7 May 2024 10:46:33 -0700
Subject: [PATCH 17/20] Clarify docs

Co-authored-by: Simon Li <orpheus+devel@gmail.com>
---
 oauthenticator/oauth2.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index 1956a917..868f2259 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -1077,8 +1077,7 @@ 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 is meant to return the groups
-          directly.
+        - 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"].

From 6aae0d5ff67eccd2598ab04dcd8ef86bbe08560f Mon Sep 17 00:00:00 2001
From: YuviPanda <yuvipanda@gmail.com>
Date: Thu, 23 May 2024 13:06:43 -0700
Subject: [PATCH 18/20] Require manage_groups to be True

---
 oauthenticator/generic.py              | 12 ++++++
 oauthenticator/oauth2.py               | 39 +++++++++++++------
 oauthenticator/tests/test_generic.py   | 52 ++++++++++++++++++++++++--
 oauthenticator/tests/test_openshift.py | 26 +++++++++++--
 4 files changed, 110 insertions(+), 19 deletions(-)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index b7177b76..d7e92255 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -24,6 +24,11 @@ def _login_service_default(self):
         .. deprecated:: 16.4
 
         Use :attr:`auth_state_groups_key` instead.
+
+
+        .. versionchanged:: 16.4
+
+        :attr:`manage_groups` is now required to be `True` to use this functionality
         """,
     )
 
@@ -48,6 +53,13 @@ def _claim_groups_key_changed(self, change):
                 cls=self.__class__.__name__,
             )
         )
+
+        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'
+                )
+
         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(
diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index 868f2259..ce8fe2c6 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -333,6 +333,8 @@ class OAuthenticator(Authenticator):
         config=True,
         help="""
         Allow members of selected groups to log in.
+
+        Requires `manage_groups` to also be `True`.
         """,
     )
 
@@ -345,6 +347,8 @@ class OAuthenticator(Authenticator):
 
         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`.
         """,
     )
 
@@ -357,12 +361,21 @@ class OAuthenticator(Authenticator):
         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.
 
-        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.
+        Requires `manage_groups` to also be `True`.
         """,
     )
 
+    # @observe calls are set in __init__
+    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="""
@@ -1109,18 +1122,17 @@ async def update_auth_model(self, auth_model):
 
         Called by the :meth:`oauthenticator.OAuthenticator.authenticate`
         """
-        if self.manage_groups or self.admin_groups:
+        if self.manage_groups:
             auth_state = auth_model["auth_state"]
             user_groups = self.get_user_groups(auth_state)
 
-        if self.manage_groups:
             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)
+            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):
@@ -1207,7 +1219,7 @@ async def check_allowed(self, username, auth_model):
             return True
 
         # allow users who are members of allowed_groups
-        if self.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):
@@ -1265,6 +1277,11 @@ def __init__(self, **kwargs):
             self.observe(
                 self._deprecated_oauth_trait, names=list(self._deprecated_oauth_aliases)
             )
+
+        self.observe(
+            self._requires_manage_groups,
+            names=("auth_state_groups_key", "admin_groups", "allowed_groups"),
+        )
         super().__init__(**kwargs)
 
 
diff --git a/oauthenticator/tests/test_generic.py b/oauthenticator/tests/test_generic.py
index 20e30368..1f656a47 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,7 @@ 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"]})
@@ -376,6 +398,7 @@ async def test_generic_auth_state_groups_key_callable(
         "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"]})
@@ -391,6 +414,7 @@ async def test_generic_auth_state_groups_key_nested_strings(
     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"]})
@@ -401,6 +425,26 @@ async def test_generic_auth_state_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 07a61423..8421cf06 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,

From acb910a8656bf19f73dd349530bc63ba6b4689b4 Mon Sep 17 00:00:00 2001
From: YuviPanda <yuvipanda@gmail.com>
Date: Thu, 23 May 2024 13:32:51 -0700
Subject: [PATCH 19/20] Call observe directly

---
 oauthenticator/oauth2.py | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index ce8fe2c6..c36fb848 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -31,6 +31,7 @@
     Unicode,
     Union,
     default,
+    observe,
     validate,
 )
 
@@ -365,7 +366,7 @@ class OAuthenticator(Authenticator):
         """,
     )
 
-    # @observe calls are set in __init__
+    @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
@@ -1278,10 +1279,6 @@ def __init__(self, **kwargs):
                 self._deprecated_oauth_trait, names=list(self._deprecated_oauth_aliases)
             )
 
-        self.observe(
-            self._requires_manage_groups,
-            names=("auth_state_groups_key", "admin_groups", "allowed_groups"),
-        )
         super().__init__(**kwargs)
 
 

From 80c41bd95953f4d1c9ba6622f0c311172c410952 Mon Sep 17 00:00:00 2001
From: YuviPanda <yuvipanda@gmail.com>
Date: Mon, 10 Jun 2024 13:06:04 -0700
Subject: [PATCH 20/20] s/16.4/17.0/ as this is now a breaking change

---
 oauthenticator/generic.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index d7e92255..f699c16b 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -21,12 +21,12 @@ def _login_service_default(self):
         [Unicode(os.environ.get('OAUTH2_GROUPS_KEY', 'groups')), Callable()],
         config=True,
         help="""
-        .. deprecated:: 16.4
+        .. deprecated:: 17.0
 
         Use :attr:`auth_state_groups_key` instead.
 
 
-        .. versionchanged:: 16.4
+        .. versionchanged:: 17.0
 
         :attr:`manage_groups` is now required to be `True` to use this functionality
         """,
@@ -49,7 +49,7 @@ 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 16.4, use {cls}.auth_state_groups_key instead".format(
+            "{cls}.claim_groups_key is deprecated since OAuthenticator 17.0, use {cls}.auth_state_groups_key instead".format(
                 cls=self.__class__.__name__,
             )
         )