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 ba795817..bc3fae33 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=""" @@ -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) diff --git a/oauthenticator/tests/test_generic.py b/oauthenticator/tests/test_generic.py index 7f48681c..ded8a827 100644 --- a/oauthenticator/tests/test_generic.py +++ b/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", [ 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,