Various fixes for allowed_groups and admin_groups

docs/source/tutorials/provider-specific-setup/providers/generic.md

@@ -35,7 +35,7 @@ c.GenericOAuthenticator.userdata_url = "https://accounts.example.com/auth/realms
 #
 c.GenericOAuthenticator.scope = ["openid", "email", "groups"]
 c.GenericOAuthenticator.username_claim = "email"
-c.GenericOAuthenticator.claim_groups_key = "groups"
+c.GenericOAuthenticator.auth_state_groups_key = "oauth_user.groups"
 
 # Authorization
 # -------------

oauthenticator/azuread.py

@@ -23,15 +23,26 @@ def _username_claim_default(self):
         return "name"
 
     user_groups_claim = Unicode(
-        "groups",
+        "",
         config=True,
         help="""
-        Name of claim containing user group memberships.
+        .. deprecated:: 17.0
 
-        Will populate JupyterHub groups if Authenticator.manage_groups is True.
+            Use :attr:`auth_state_groups_key` instead.
         """,
     )
 
+    @default('auth_state_groups_key')
+    def _auth_state_groups_key_default(self):
+        key = ""
+        if self.user_groups_claim:
+            key = f"{self.user_auth_state_key}.{self.user_groups_claim}"
+            cls = self.__class__.__name__
+            self.log.warning(
+                f"{cls}.user_groups_claim is deprecated in OAuthenticator 17. Use {cls}.auth_state_groups_key={key!r}"
+            )
+        return key
+
     tenant_id = Unicode(
         config=True,
         help="""
@@ -55,15 +66,6 @@ def _authorize_url_default(self):
     def _token_url_default(self):
         return f"https://login.microsoftonline.com/{self.tenant_id}/oauth2/token"
 
-    async def update_auth_model(self, auth_model, **kwargs):
-        auth_model = await super().update_auth_model(auth_model, **kwargs)
-
-        if getattr(self, "manage_groups", False):
-            user_info = auth_model["auth_state"][self.user_auth_state_key]
-            auth_model["groups"] = user_info[self.user_groups_claim]
-
-        return auth_model
-
     async def token_to_user(self, token_info):
         id_token = token_info['id_token']
         decoded = jwt.decode(

oauthenticator/globus.py

@@ -342,7 +342,7 @@ async def update_auth_model(self, auth_model):
         to False makes it be revoked.
         """
         user_groups = set()
-        if self.allowed_globus_groups or self.admin_globus_groups:
+        if self.allowed_globus_groups or self.admin_globus_groups or self.manage_groups:
             tokens = self.get_globus_tokens(auth_model["auth_state"]["token_response"])
             user_groups = await self._fetch_users_groups(tokens)
         # sets are not JSONable, cast to list for auth_state

oauthenticator/oauth2.py

@@ -1103,7 +1103,7 @@ def build_auth_state_dict(self, token_info, user_info):
         Called by the :meth:`oauthenticator.OAuthenticator.authenticate`
 
         .. versionchanged:: 17.0
-            This method be async.
+            This method may be async.
         """
 
         # We know for sure the `access_token` key exists, oterwise we would have errored out already
@@ -1133,6 +1133,8 @@ async 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.
 
+        Only called when :attr:`manage_groups` is True.
+
         - If auth_state_groups_key is a callable, it returns the list of groups directly.
           Callable may be async.
         - If auth_state_groups_key is a nested dictionary key like
@@ -1168,11 +1170,23 @@ async def update_auth_model(self, auth_model):
             - `name`: the normalized username
             - `admin`: the admin status (True/False/None), where None means it
                 should be unchanged.
-            - `auth_state`: the dictionary of of auth state
-                returned by :meth:`oauthenticator.OAuthenticator.build_auth_state_dict`
+            - `auth_state`: the auth state dictionary,
+              returned by :meth:`oauthenticator.OAuthenticator.build_auth_state_dict`
 
         Called by the :meth:`oauthenticator.OAuthenticator.authenticate`
         """
+        # NOTE: this base implementation should _not_ be updated to do anything
+        # subclasses should have full control without calling super()
+        return auth_model
+
+    async def _apply_managed_groups(self, auth_model):
+        """Applies managed_groups logic
+
+        Called after `update_auth_model` to populate the `groups` field.
+        Only called if `manage_groups` is True.
+
+        The public method for subclasses to override is `.get_user_groups`.
+        """
         if self.manage_groups:
             auth_state = auth_model["auth_state"]
             user_groups = self.get_user_groups(auth_state)
@@ -1244,7 +1258,10 @@ async def authenticate(self, handler, data=None, **kwargs):
 
         # update the auth_model with info to later authorize the user in
         # check_allowed, such as admin status and group memberships
-        return await self.update_auth_model(auth_model)
+        auth_model = await self.update_auth_model(auth_model)
+        if self.manage_groups:
+            auth_model = await self._apply_managed_groups(auth_model)
+        return auth_model
 
     async def check_allowed(self, username, auth_model):
         """
@@ -1289,12 +1306,8 @@ async def check_allowed(self, username, auth_model):
             return True
 
         # allow users who are members of allowed_groups
-        if self.manage_groups and self.allowed_groups:
-            auth_state = auth_model["auth_state"]
-            user_groups = self.get_user_groups(auth_state)
-            if isawaitable(user_groups):
-                user_groups = await user_groups
-            if any(user_groups & self.allowed_groups):
+        if self.manage_groups and self.allowed_groups and auth_model.get("groups"):
+            if set(auth_model["groups"]) & self.allowed_groups:
                 return True
 
         # users should be explicitly allowed via config, otherwise they aren't

oauthenticator/tests/test_auth0.py

@@ -29,6 +29,7 @@ def user_model():
     return {
         "email": "[email protected]",
         "name": "user1",
+        "groups": ["group1"],
     }
 
 
@@ -62,6 +63,47 @@ def user_model():
             True,
             True,
         ),
+        # common tests with allowed_groups and manage_groups
+        (
+            "20",
+            {
+                "allowed_groups": {"group1"},
+                "auth_state_groups_key": "auth0_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            None,
+        ),
+        (
+            "21",
+            {
+                "allowed_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "auth0_user.groups",
+                "manage_groups": True,
+            },
+            False,
+            None,
+        ),
+        (
+            "22",
+            {
+                "admin_groups": {"group1"},
+                "auth_state_groups_key": "auth0_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            True,
+        ),
+        (
+            "23",
+            {
+                "admin_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "auth0_user.groups",
+                "manage_groups": True,
+            },
+            False,
+            False,
+        ),
     ],
 )
 async def test_auth0(
@@ -84,7 +126,10 @@ async def test_auth0(
 
     if expect_allowed:
         assert auth_model
-        assert set(auth_model) == {"name", "admin", "auth_state"}
+        if authenticator.manage_groups:
+            assert set(auth_model) == {"name", "admin", "auth_state", "groups"}
+        else:
+            assert set(auth_model) == {"name", "admin", "auth_state"}
         assert auth_model["admin"] == expect_admin
         auth_state = auth_model["auth_state"]
         assert json.dumps(auth_state)

oauthenticator/tests/test_azuread.py

@@ -49,6 +49,7 @@ def user_model(tenant_id, client_id, name):
                 "96000b2c-7333-4f6e-a2c3-e7608fa2d131",
                 "a992b3d5-1966-4af4-abed-6ef021417be4",
                 "ceb90a42-030f-44f1-a0c7-825b572a3b07",
+                "group1",
             ],
             # different from 'groups' for tests
             "grp": [
@@ -118,7 +119,11 @@ def user_model(tenant_id, client_id, name):
         # test user_groups_claim
         (
             "30",
-            {"allow_all": True, "manage_groups": True},
+            {
+                "allow_all": True,
+                "auth_state_groups_key": "user.groups",
+                "manage_groups": True,
+            },
             True,
             None,
         ),
@@ -127,11 +132,52 @@ def user_model(tenant_id, client_id, name):
             {
                 "allow_all": True,
                 "manage_groups": True,
-                "user_groups_claim": "grp",
+                "auth_state_groups_key": "user.grp",
+            },
+            True,
+            None,
+        ),
+        # common tests with allowed_groups and manage_groups
+        (
+            "40",
+            {
+                "allowed_groups": {"group1"},
+                "auth_state_groups_key": "user.groups",
+                "manage_groups": True,
             },
             True,
             None,
         ),
+        (
+            "41",
+            {
+                "allowed_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "user.groups",
+                "manage_groups": True,
+            },
+            False,
+            None,
+        ),
+        (
+            "42",
+            {
+                "admin_groups": {"group1"},
+                "auth_state_groups_key": "user.groups",
+                "manage_groups": True,
+            },
+            True,
+            True,
+        ),
+        (
+            "43",
+            {
+                "admin_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "user.groups",
+                "manage_groups": True,
+            },
+            False,
+            False,
+        ),
     ],
 )
 async def test_azuread(
@@ -178,9 +224,11 @@ async def test_azuread(
         assert auth_model["name"] == user_info[authenticator.username_claim]
         if manage_groups:
             groups = auth_model['groups']
-            assert groups == user_info[authenticator.user_groups_claim]
+            assert (
+                groups == user_info[authenticator.auth_state_groups_key.rsplit(".")[-1]]
+            )
     else:
-        assert auth_model == None
+        assert auth_model is None
 
 
 async def test_tenant_id_from_env():

oauthenticator/tests/test_bitbucket.py

@@ -45,6 +45,7 @@ def user_model(username):
     """
     return {
         "username": username,
+        "groups": ["group1"],
     }
 
 
@@ -80,6 +81,47 @@ def user_model(username):
             True,
             True,
         ),
+        # common tests with allowed_groups and manage_groups
+        (
+            "20",
+            {
+                "allowed_groups": {"group1"},
+                "auth_state_groups_key": "bitbucket_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            None,
+        ),
+        (
+            "21",
+            {
+                "allowed_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "bitbucket_user.groups",
+                "manage_groups": True,
+            },
+            False,
+            None,
+        ),
+        (
+            "22",
+            {
+                "admin_groups": {"group1"},
+                "auth_state_groups_key": "bitbucket_user.groups",
+                "manage_groups": True,
+            },
+            True,
+            True,
+        ),
+        (
+            "23",
+            {
+                "admin_groups": {"test-user-not-in-group"},
+                "auth_state_groups_key": "bitbucket_user.groups",
+                "manage_groups": True,
+            },
+            False,
+            False,
+        ),
     ],
 )
 async def test_bitbucket(
@@ -101,7 +143,10 @@ async def test_bitbucket(
 
     if expect_allowed:
         assert auth_model
-        assert set(auth_model) == {"name", "admin", "auth_state"}
+        if authenticator.manage_groups:
+            assert set(auth_model) == {"name", "admin", "auth_state", "groups"}
+        else:
+            assert set(auth_model) == {"name", "admin", "auth_state"}
         assert auth_model["admin"] == expect_admin
         auth_state = auth_model["auth_state"]
         assert json.dumps(auth_state)