jupyterhub / oauthenticator

OAuth + JupyterHub Authenticator = OAuthenticator
https://oauthenticator.readthedocs.io
BSD 3-Clause "New" or "Revised" License
413 stars 365 forks source link

[Generic] Add support for `manage_groups` #708

Closed benjimin closed 8 months ago

benjimin commented 10 months ago

Addresses #706

welcome[bot] commented 10 months ago

Thanks for submitting your first pull request! You are awesome! :hugs:
If you haven't done so already, check out Jupyter's Code of Conduct. welcome 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! :wave:
Welcome to the Jupyter community! :tada:

minrk commented 10 months ago

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 commented 10 months ago

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_groupskey cognito:groups does not exist in the user token".

(The instrumented logs affirm that manage_groups is True.)

The auth_model has structure:

I then decoded the JWT _idtoken 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 _oauthuser 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 commented 10 months ago

user_info is populated from the oauthenticator.OAuthenticator.userdata_url endpoint: https://github.com/jupyterhub/oauthenticator/blob/ee52a6dca8da76f72ea74ace43a2d205839f831d/oauthenticator/oauth2.py#L861-L864

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 https://github.com/jupyterhub/oauthenticator/blob/ee52a6dca8da76f72ea74ace43a2d205839f831d/oauthenticator/generic.py#L141-L142

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

yuvipanda commented 9 months ago

Just wanted to check in to see if there's anything more to do to move this along

manics commented 8 months ago

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 commented 8 months ago

@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.

welcome[bot] commented 8 months ago

Congrats on your first merged pull request in this project! :tada: congrats Thank you for contributing, we are very proud of you! :heart: