jupyterhub / oauthenticator

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

[All] Add `userdata_from_id_token` as alternative to `userdata_url` #725

Closed benjimin closed 6 months ago

benjimin commented 7 months ago

This PR adds support for extracting claims from the id token, instead of by exchanging the access token with the userinfo endpoint. Addresses #487.

This is needed because some claims may only be available via the id token and not via the userinfo endpoint, for example, AWS Cognito only provides the group membership claim via the id token (i.e., needed to complete support for #706).

For background, OAuth2 was a standard for obtaining an access token ("authorisation not authentication") and OIDC extended this in two ways: by standardising a userinfo endpoint and by accompanying the access token with an id token. (Note this is only the core of OIDC; complete OIDC support also entails discovery mechanisms, etc.) To date, oauthenticator has always discarded the id token that it receives, and relied on the userinfo endpoint for performing authentication.

This PR requires the feature to be explicitly enabled by the admin, without introducing any additional config attribs. It uses a library which is already used elsewhere by JupyterHub.

minrk commented 7 months ago

Thank you! I think this is very sensible. Perhaps config would be clearer if we used an explicit special value, like e.g. userdata_url = ".id_token", rather than encoding this behavior as None, or even userdata_from_id_token = True that takes precedence over userdata_url.

consideRatio commented 7 months ago

I think userdata_from_id_token = true is a good choice here

Thank you! I think this is very sensible. Perhaps config would be clearer if we used an explicit special value, like e.g. userdata_url = ".id_token", rather than encoding this behavior as None, or even userdata_from_id_token = True that takes precedence over userdata_url.

I think having a dedicated toggle userdata_from_id_token = True would be good, it would provide us with a clear place to document this behavior and make it findable

consideRatio commented 7 months ago

Is pyjwr an optional dep for azuread ? Then we can clean it up there. If that optinal dependency is empty, let's then leave it empty and comment its no longer in use.

Also i think we should add a lower bound to pyjwts major version, so we depend on pyjwt>=2 because i recall migrating to v2 from v1 was needed elsewhere

minrk commented 7 months ago

Is pyjwt an optional dep for azuread?

it is, we can promote it to an unconditional dependency with the same bounds.

manics commented 6 months ago

Do you think we could add a test for this to help with future maintenance?

minrk commented 6 months ago

@benjimin thanks for the test!

benjimin commented 6 months ago

Thanks for the assist and support!