Require manage_groups to be True

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(

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="""
@@ -1079,18 +1092,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):
@@ -1169,7 +1181,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):
@@ -1227,6 +1239,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)
 
 

oauthenticator/tests/test_generic.py

@@ -1,8 +1,9 @@
 import json
+import re
 from functools import partial
 
 import jwt
-from pytest import fixture, mark
+from pytest import fixture, mark, raises
 from traitlets.config import Config
 
 from ..generic import GenericOAuthenticator
@@ -102,16 +103,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,
@@ -121,6 +133,7 @@ def get_authenticator_variant(generic_client, userdata_from_id_token):
             {
                 "allowed_users": {"not-test-user"},
                 "admin_users": {"user1"},
+                "manage_groups": True,
             },
             True,
             True,
@@ -130,6 +143,7 @@ def get_authenticator_variant(generic_client, userdata_from_id_token):
             {
                 "allowed_groups": {"group1"},
                 "admin_groups": {"group1"},
+                "manage_groups": True,
             },
             True,
             True,
@@ -139,6 +153,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,
@@ -148,6 +163,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,
@@ -157,6 +173,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,
@@ -166,6 +183,7 @@ def get_authenticator_variant(generic_client, userdata_from_id_token):
             {
                 "admin_users": {"user1"},
                 "admin_groups": {"group1"},
+                "manage_groups": True,
             },
             True,
             True,
@@ -175,6 +193,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,
@@ -184,6 +203,7 @@ def get_authenticator_variant(generic_client, userdata_from_id_token):
             {
                 "admin_users": {"not-test-user"},
                 "admin_groups": {"group1"},
+                "manage_groups": True,
             },
             True,
             True,
@@ -193,6 +213,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,
@@ -310,6 +331,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"]})
@@ -325,6 +347,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"]})
@@ -343,6 +366,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"]})
@@ -358,6 +382,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"]})
@@ -368,6 +393,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",
     [

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,