Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consistently implement manage_groups #709

Open
minrk opened this issue Nov 28, 2023 · 5 comments · May be fixed by #735
Open

Consistently implement manage_groups #709

minrk opened this issue Nov 28, 2023 · 5 comments · May be fixed by #735

Comments

@minrk
Copy link
Member

minrk commented Nov 28, 2023

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
Copy link
Member Author

minrk commented Nov 28, 2023

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
Copy link
Member

manics commented Nov 28, 2023

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
Copy link
Contributor

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
Copy link
Member Author

minrk commented Nov 29, 2023

@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
Copy link
Collaborator

I think #735 'fixes' this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants