[All] Move group management from generic to base oauthenticator (allowed_groups, admin_groups, auth_state_groups_key)

[yuvipanda]

Motivating use cases

External identity providers providing JupyterHub memberships is an extremely useful feature that should be present not just for GenericOAuthenticator but for all authenticators. But to do so in helpful ways, this PR considers two motivating use cases:

1. A hub using Auth0OAuthenticator. In Auth0, scopes granted are what is used to provide the notion of 'can the user perform this task?', which can be used as group membership. This is what auth0 recommends, there is currently no other way to do 'groups' in Auth0. scope is inside auth_model, but not auth_state, since scope is granted each time the user is logged in.
2. In GitHubOAuthenticator, we put the list of teams the user is in inside auth_state. This is the perfect piece of information to use for group membership.
3. oauth_user gets put inside auth_state, and in general auth_state is a good place for this kinda group information to be in. Authenticators can put arbitrary stuff inside auth_state and use them as they wish.

Approaches considered and rejected

1. Each authenticator figures out how groups work within it. This was tried out in gh: Allow setting JupyterHub groups from GitHub teams #739, and rejected by @minrk in [All] Move group management from generic to base oauthenticator (allowed_groups, admin_groups, auth_state_groups_key) #735 (comment). I agree with this rejection.
2. Allow group information to be picked up from the auth_model with a auth_model_groups_key. This would be same as the current claim_groups_key, but pick from the entire auth_model instead of just from the returned user object. This was the tack this PR was taking, primarily because I thought we needed it to handle use case 1 mentioned earlier. But turns out I was wrong - I had thought that scope was part of auth_model but not auth_state, but we do! And regardless, I also realized we don't expose auth_model anywhere, but we do expose auth_state. And I had a TODO for 'document what is in auth_model', and while trying to do that, decided we shouldn't expose that to configurable tweaks like this for now. So that was reverted in b337015 and a different approach was taken.

Approach this PR takes

The general approach to group management is:

1. Authenticator puts something that may be relevant to group membership inside auth_state.
2. There's an auth_state_groups_key that can be either a callable or dotted specification that generates a list of groups from something in auth_state.

This handles case (2) because list of teams is already in auth_state. And can handle (1) by us putting scope in some form inside auth_state. This also provides a clear extensible mechanism in the future for all group management - get it into auth_state (where it can be used for anything), and pick that out with auth_state_groups_key.

Backwards compatibility

• The existing claim_groups_key behavior is preserved, by being passed on to auth_state_groups_key in the base. It has been marked as deprecated. This is not a backwards compatibility break.
• All group related functionality now requires manage_groups to be True, which was not the case earlier. Before this, if manage_groups is false but any of the group related authorization functionality (allowed_groups and admin_groups) is used, they control group related behavior but don't show up as JupyterHub groups. This causes confusion, as the 'groups' field in the admin panel will be empty, and possible other group related behavior (such as future profile list filtering, for example) would not respect these groups. We basically would end up with two group concepts - First class JupyterHub groups (which will show up in admin panel, API, can be edited by admins, etc) as well as second class 'Authenticator' groups (which are only used for authorization and 'disappear' after that). I think this is unnecessary complication, and this is a good time to remove this distinction. Now, any kind of group related authorization functionality requires manage_groups to be True, and we are back to having only one notion of 'group'. We also remove the confusing part where you may have allowed_groups set to something, manually modify the groups the user is a part of in JupyterHub admin, and it silently has no effect. This is a breaking change for people who used groups functionality but set manage_groups to be False. However, I think that usage is fairly minor, because of the confusing behavior it causes. I have added the 'breaking' label here regardless.

Breaking change

• All group related functionality (allowed_groups, admin_groups, claims_group_key and auth_state_groups_key) now also requires manage_groups to be set to True

TODO

• Update docs to refer to new property
• Reword the docstring for auth_state_groups_key

Fixes #709

[yuvipanda]

All this functionality would be extremely useful to all the authenticators, so should be in base.

I think an important question now becomes - what is the point of GenericOAuthenticator? Is that just here for backwards compatibility?

I think the answer should be 'yes'. And in the future, anything that relies on OAuth2 functionality should just go to the base OAuth2 provider, and anything specific to any of the providers can go in their own files. And we suggest people who currently use Generic migrate to just using the base OAuth2 provider.

[minrk]

what is the point of GenericOAuthenticator?

I believe GenericOAuthenticator should eventually go away and be merged into the base class, as long as we can do it smoothly and it doesn't complicate things unnecessarily (I don't think it will). I think the big refactor in #526 was a big step toward making that feasible.

Not having done that yet, going forward in general I think we should avoid new features in Generic in favor of putting them in the base class, at least in most cases.

[minrk]

Good idea! I think it makes sense to promote this to the base class. I only think we should perhaps be a little more considered in naming config all classes will inherit rather than only on Generic.

[minrk]

I don't really want to start a naming conversation when this is just moving existing config up one level, but I think this config name is very unclear, and defining new config on a class is an opportunity to not inherit the confusing name. claim and key are really synonyms, so it's odd to have both at all, let alone one on either side of the field they populate. Could it be just groups_key (generic, Pythonic) or groups_claim (OIDC-specific, jargony, I suspect unclear to most) or userdata_groups_key to more clearly specify that it's the key in userdata where this info will be found?

If we rename, Generic can keep its claims_group_key as a deprecated trait that assigns to self.userdata_groups_key with a warning.

[consideRatio]

userdata_groups_key sounds good to me as well

[yuvipanda]

The base one is called auth_state_groups_key, as the direct analog to userdata_groups_key, since now it gets info out of auth_state instead of userdata. Generic keeps its claims_group_key and issues a warning.

[yuvipanda]

I was thinking about how we can do a very common thing - sync GitHub team memberships and org memberships to JupyterHub groups. With this PR as is, that's actually not possible - team memberships are not part of userdata! But they are part of the auth_model (because there's a populate_teams_in_auth_model property already).

I think there are two paths forward here for groups to work everywhere:

1. Lift that into the base Authenticator class, but the key is for things from auth_model, not userdata. This also allows for using things like scope (which can control group membership with Auth0, from another community we are working with). We can name this appropriately, and the keep a deprecated claims_group_key in Generic that notifies it's deprecated but works the way it already does.
2. gh: Allow setting JupyterHub groups from GitHub teams #739 or similar for each authenticator we want to support good groups.

My preference is to do (1)! There's a clean backwards compat story, and the model of 'put stuff into auth_state, that is available via auth_model, and you can pull stuff out of that for groups' seems clean enough to explain.

[minrk]

Option 1 sounds like a great choice, and also a great opportunity to not inherit the name while preserving compatibility.

[yuvipanda]

This went through a few iterations, but is ready for review again! I've updated the PR body with more detailed information as well. Please take another look when you got time, @minrk :)

[manics]

If we've agreed on the design can you also review the docs?
For example if claim_groups_key is deprecated we should update the example

oauthenticator/docs/source/tutorials/provider-specific-setup/providers/generic.md

Line 38 in ed6b97e

c.GenericOAuthenticator.claim_groups_key = "groups"

[manics]

Does upstream provider mean JupyterHub?

[yuvipanda]

@manics I copied this from the current docstring (

oauthenticator/oauthenticator/generic.py

Line 30 in 79db03c

This configures how group membership in the upstream provider is determined

) but I agree it's actually quite confusing. I'll think about it and try to rewrite it.

[yuvipanda]

@manics I spent some time rewording this, and this led me to the conclusion that we should just not allow usage of these without manage_groups also being True. I've added a more full rationale under 'Backwards compatibility' describing it. I've also rewritten the docstring now! Let me know what you think :)

[yuvipanda]

After trying to write clear docs for the traitlet, I came to the conclusion that we shouldn't allow usage of these without manage_groups also being True. I've updated a paragraph under Backwards compatibiltiy with my rationale.

With that, this is ready to go!

[yuvipanda]

I found #709, which I think is closed by this PR. And my change with respect to manage_groups reflects:

do we need to deal with additional groups, not specified upstream somehow? I'd say no, at least for now.

Which I totally agree with. Groups should have a 'single source of truth'.

[manics]

🎆
I've opened #745 as a remidner to update our docs

[yuvipanda]

yay, thank you @manics

[benjimin]

@yuvipanda there are numerous hooks to facilitate local customisation. What API should local code in these hooks use to access the list of groups that the user is member to?

For example, depending on group membership, I want to filter my profile list and adjust singleuser server settings. If my code runs in auth_state_hook, options_form, pre_spawn_hook or modify_pod_hook, then it will be passed the spawner instance, or if runs in post_auth_hook it will be passed the authenticator instance and auth_model (not auth_state). I take it that accessing auth_model["groups"] would be discouraged? Accessing spawner.user.groupsyields the same as user.orm_user.groups, a list of SQAlchemy objects rather than a trivial set of strings to test membership in. Should customisations try to work with that, or try to call something like spawner.authenticator.get_user_groups(spawner.user.get_auth_state())?