Consistently implement manage_groups

minrk commented 9 months ago

Proposed change

Several (not all!) OAuthenticators have some form of retrieving 'group' membership. Authenticators can also return a list of JupyterHub groups to assign a user to. If Authenticator.manage_groups is enabled, then this is always the exact membership list (groups cannot be assigned any other way).

Issues to consider:

consistency of configuration, defaults across OAuthenticator
I susepct we want the ability to transform group names (e.g. turning github team jupyterhub/binder-team into gh-jupyterhub--binder-team or just binder-team, etc.)
behavior when manage_groups is True (assignment is strict and removes membership outside the list) vs False (fetching info is unnecessary and has no effect)
not all OAuthenticators have a notion of groups, and should still behave sensibly
do we need to deal with additional groups, not specified upstream somehow? I'd say no, at least for now.

Alternative options

Let authenticators implement these one by one, allowing inconsistency (easier to accept small contributions, but ultimately leads to inconsistency and confusion and a breaking refactor like #526)

Currently, we have #573 and #708.

Who would use this feature?

OAuthenticator users who want to use group membership (mostly for permissions, I would imagine)

(Optional): Suggest a solution

specify that fetching group info should only be done if self.manage_groups: to avoid costly requests or unnecessary errors
standardize config / hook for transforming upstream groups to jupyterhub group names (default can be no transform or some simple normalization)

minrk commented 9 months ago

Updated because I had the wrong behavior when manage_groups is False. It has no effect - we should still avoid fetching info we don't use, but it doesn't assign groups unconditionally.

manics commented 9 months ago

I agree with making groups managed by either JupyterHub or the Authenticator but not both, since it leads to too many conflicts, and the correct resolution will be different for every deployment. If someone does need additional groups I think the recommendation could be that they extend the authenticator, e.g. by augmenting/filtering the groups returned by the authenticator in a subclass? If we're supporting transformation of group names via a callable perhaps this could be

def transform_group_names(groupnames: List): List
    # Override to transform group names, or add/remove groups
    return groupnames

benjimin commented 9 months ago

I think having a way to transform group names is over-engineering it, because JupyterHub does not expose group names externally in a human-visible way. (Note JupyterHub disables all group management UI if groups are managed by the authenticator.)

The group membership from the token is very useful to be able to access in internal customisation code (for selectively granting access to different profile options), and it is also used in configuration (for granting login access or admin access), but adding a name transformer would introduce an entire unnecessary layer of potential confusion for administrators configuring the system.

minrk commented 9 months ago

@benjimin that's a fair point, and I was mainly thinking of the namespace collision of having jupyterhub groups and provider groups, but of course that can't happen if manage_groups is True, which is currently the only way to have groups defined from the provider. This could conceivably still be an issue in MultiAuthenticator, but that's a special case (for now). It could also be an issue if upstream group names are illegal/unsafe (this is why we have normalization of usernames, for example).

I think it's a good idea to wait to implement such a thing until we find that it's actually needed, rather than solving what's currently a hypothetical issue.

yuvipanda commented 3 months ago

I think #735 'fixes' this.

jupyterhub / oauthenticator