Repository: jupyterhub/oauthenticator
Pull request #708 (2023-11-28 12:20:58)

[Generic] Add support for `manage_groups`
Addresses #706

welcome[bot]: Thanks for submitting your first pull request! You are awesome! :hugs:
<br>If you haven't done so already, check out [Jupyter's Code of Conduct](https://github.com/jupyter/governance/blob/master/conduct/code_of_conduct.md).
![welcome](https://raw.githubusercontent.com/jupyterhub/.github/master/images/welcome.jpg)
You can meet the other [Jovyans](https://jupyter.readthedocs.io/en/latest/community/content-community.html?highlight=jovyan#what-is-a-jovyan) by joining our [Discourse forum](http://discourse.jupyter.org/). There is also a intro thread there where you can stop by and say Hi! :wave:
<br>Welcome to the Jupyter community! :tada:

minrk: Added test to exercise this, which resulted in fixing a bug in the type of the groups field, and matching #710 in handling pre-hub-2.2 behavior.
benjimin: Hmm... something might not be quite right yet..

I just tried testing this, by instrumenting `update_auth_model` (to log inputs `self.manage_groups` and `auth_model`) and `get_user_groups` (to log inputs `self.claim_groups_key` and `user_info`). I then built [a container image](https://hub.docker.com/repository/docker/cesiumfrog/k8s-hub/general) using a Dockerfile like:

```
FROM jupyterhub/k8s-hub:3.2.1
USER root
RUN pip uninstall oauthenticator -y && pip install git+https://github.com/benjimin/oauthenticator.git@debug#egg=oauthenticator
```
I tested this with my AWS Cognito user pool, making sure to set:

```
c.GenericOAuthenticator.allow_existing_users = True
c.Authenticator.manage_groups = True
c.GenericOAuthenticator.claim_groups_key = "cognito:groups"
```
When I log in, the pod logs include the message (from `generic:147`) "**_The claim_groups_key cognito:groups does not exist in the user token_**".

(The instrumented logs affirm that `manage_groups` is `True`.)

The `auth_model` has structure:
* name
* admin
* auth_state
  * access_token
  * refresh_token
  * id_token
  * scope
  * token_response
    * id_token (appears identical to above)
    * access_token
    * refresh_token
    * expires_in
    * token_type (="Bearer")
  * oauth_user
    * sub
    * email_verified (`'true'`)
    * email
    * username (identical to sub)

I then decoded the JWT _id_token_ from the instrumented logs. The payload included all the expected claims, such as `email`, `sub` (identical to claim `cognito:username`), and particularly included claims:
```
  "cognito:groups": [
    "trusted-group",
    "dev-group",
    "internal-group"
  ],
  "email_verified": true,
  "token_use": "id",
```
(FWIW, I confirmed the `cognito:groups` claim happens to also be present in the access token as well, although the email-related claims are not present there and it also differs in containing `"token_use": "access"`.) 

(In the instrumented logs, `user_info` appears identical to _oauth_user_ above: a subset of claims from the token but critically omitting `cognito:groups`.)

So as far as AWS Cognito is concerned, the group membership is definitely available in the OAuth (OIDC?) ID token, but it  unfortunately seems to get stripped out from what is passed to `get_user_groups`.

The `oauth_user`/`user_info` dictionary seems to be generated by `OAuthenticator.token_to_user` (which some of the non-generic authenticators appear to override). Does it ignore the _ID token_ and instead parse JSON from another user-info HTTP endpoint authorised via the _access token_?
yuvipanda: Just wanted to check in to see if there's anything more to do to move this along
manics: I've opened a PR to bump the minimum JupyterHub version https://github.com/jupyterhub/oauthenticator/pull/720
I think it will make this PR a lot clearer!
manics: @benjimin Sorry for the back and forth, but we've bumped the minimum version of JupyterHub: https://github.com/jupyterhub/oauthenticator/pull/720
If you rebase on `main` you can assume the `manage_groups` property always exists.

Patch:
diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index 31945d6..4dd88bb 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -27,8 +27,8 @@ class GenericOAuthenticator(OAuthenticator):
         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.
-        It has no effect on its own, and is not related to users' _JupyterHub_ group membership.
+        for use by `allowed_groups`, `admin_groups`, etc. If `manage_groups` is True,
+        this will also determine users' _JupyterHub_ group membership.
         """,
     )
 
@@ -154,15 +154,22 @@ class GenericOAuthenticator(OAuthenticator):
         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
-            user_info = auth_model["auth_state"][self.user_auth_state_key]
-            user_groups = self.get_user_groups(user_info)
             auth_model["admin"] = bool(user_groups & self.admin_groups)
 
         return auth_model


Test patch:
diff --git a/oauthenticator/tests/test_generic.py b/oauthenticator/tests/test_generic.py
index 42dc778..fcc9665 100644
--- a/oauthenticator/tests/test_generic.py
+++ b/oauthenticator/tests/test_generic.py
@@ -151,6 +151,15 @@ def get_authenticator(generic_client):
             False,
             False,
         ),
+        (
+            "20",
+            {
+                "manage_groups": True,
+                "allow_all": True,
+            },
+            True,
+            None,
+        ),
     ],
 )
 async def test_generic(
@@ -166,6 +175,9 @@ async def test_generic(
     c.GenericOAuthenticator = Config(class_config)
     c.GenericOAuthenticator.username_claim = "username"
     authenticator = get_authenticator(config=c)
+    manage_groups = False
+    if "manage_groups" in class_config:
+        manage_groups = authenticator.manage_groups
 
     handled_user_model = user_model("user1")
     handler = generic_client.handler_for_user(handled_user_model)
@@ -173,7 +185,10 @@ async def test_generic(
 
     if expect_allowed:
         assert auth_model
-        assert set(auth_model) == {"name", "admin", "auth_state"}
+        expected_keys = {"name", "admin", "auth_state"}
+        if manage_groups:
+            expected_keys.add("groups")
+        assert set(auth_model) == expected_keys
         assert auth_model["admin"] == expect_admin
         auth_state = auth_model["auth_state"]
         assert json.dumps(auth_state)
@@ -183,6 +198,9 @@ async def test_generic(
         assert "scope" in auth_state
         user_info = auth_state[authenticator.user_auth_state_key]
         assert auth_model["name"] == user_info[authenticator.username_claim]
+        if manage_groups:
+            assert auth_model["groups"] == user_info[authenticator.claim_groups_key]
+
     else:
         assert auth_model == None