benjimin commented 10 months ago

Proposal

If an administrator chooses to configure c.GenericOAuthenticator.claim_groups_key, then the list of groups (from the OAuth token claim) should be made readily available in auth_state.

Setting c.Authenticator.manage_groups = True should also cause spawner.user.groups to be populated with the same list.

Use-case

This is so administrators can configure an OAuth provider to manage authentication and group membership, for example to customise the Jupyterhub server profile list based on (externally managed) group memberships.

This idea is explicitly mentioned in the Z2JH customisation guide (using OAuth group membership to decide whether to offer GPUs).

Is this a bug? Or a feature request?

At present claim_groups_key seems to have no effect unless admin_groups or allowed_groups are set. (The claim_groups_key docs give little indication if its purpose were intended to be so narrow.)

The docs for manage_groups lead one to expect that the authenticator should assign a list of group names to the user. 🐜

Alternative options

I think it is pragmatic to only make changes to GenericOAuthenticator at this stage. An alternative would be to make the changes somewhere deeper in the class hierarchy (potentially adjusting the interfaces), because administrators are likely to want other varieties of authenticator to also have similar functionality (of populating groups from externally-managed sources), and it would be advantageous if the expected behaviour and configuration was made consistent. (For example, GitHubOAuthenticator and GlobusOAuthenticator currently each implement their own peculiar class methods for configuring group management.)

A workaround I've seen deployed (similar example here) is to do a pip install of cognitojwt into the container image at runtime, to use to decode and verify an OAuth token (e.g., to extract the "cognito:groups" claim expressing AWS Cognito user-pool group-memberships). But this breaks containerisation (risking abrupt failure of the hub if a shared dependency releases a breaking change upstream on PyPI) and requires injecting a substantial amount of code in Z2JH config (where it is difficult to version control and distribute patches); it is far better if the authenticator class itself handles decoding and verifying the tokens as appropriate (facilitating a simpler API, the user.groups property, for customisations to work with).

Suggested solution

@jrouly has suggested that update_auth_model needs to unconditionally assign to auth_model["groups"].

This should ensure the groups field is returned by Authenticator.authenticate (per spec), and so it can be transferred to the user object during login handling.

welcome[bot] commented 10 months ago

Thank you for opening your first issue in this project! Engagement like this is essential for open source projects! :hugs:
If you haven't done so already, check out Jupyter's Code of Conduct. Also, please try to follow the issue template as it helps other other community members to contribute more effectively. welcome You can meet the other Jovyans by joining our Discourse forum. There is also an intro thread there where you can stop by and say Hi! :wave:
Welcome to the Jupyter community! :tada:

minrk commented 10 months ago

707 clarifies the docstring, mentioning that there's no connection between upstream service group membership and JupyterHub group membership (like all other OAuthenticators).

OAuthenticators don't implement manage_groups yet, though it certainly makes sense to do so, and claim_groups_key would be how group membership is determined.

krassowski commented 7 months ago

OAuthenticators don't implement manage_groups yet, though it certainly makes sense to do so, and claim_groups_key would be how group membership is determined.

I was confused for a moment here but then I saw this comment was written before https://github.com/jupyterhub/oauthenticator/pull/708 was merged.

Should this issue remain open?

manics commented 7 months ago

Thanks, looks like it was resolved by #708!