[Generic] Add support for manage_groups

[benjimin]

Addresses #706

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

[minrk]

Great!

This works for me. If anyone has an objection to bumping the minimum hub version to 2.2 (March, 2022), the check can be manage_groups = getattr(self, "manage_groups", False).

[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 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?

[manics]

user_info is populated from the oauthenticator.OAuthenticator.userdata_url endpoint:

oauthenticator/oauthenticator/oauth2.py

Lines 861 to 864 in ee52a6d

	async def token_to_user(self, token_info):
	"""
	Determines who the logged-in user by sending a "GET" request to
	:data:`oauthenticator.OAuthenticator.userdata_url` using the `access_token`.

https://docs.aws.amazon.com/cognito/latest/developerguide/userinfo-endpoint.html

claim_groups_key can be a callable but currently it only has user_info as a parameter

oauthenticator/oauthenticator/generic.py

Lines 141 to 142 in ee52a6d

	if callable(self.claim_groups_key):
	return set(self.claim_groups_key(user_info))

Maybe overriding subclassing and overiding get_user_groups in your config file is the easiest short term solution?

[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 #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: #720
If you rebase on main you can assume the manage_groups property always exists.

[welcome]

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️