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

[All] Don't allow existing users when allowed_users is configured #619

Closed
consideRatio opened this issue Jun 22, 2023 · 2 comments · Fixed by #631
Closed

[All] Don't allow existing users when allowed_users is configured #619

consideRatio opened this issue Jun 22, 2023 · 2 comments · Fixed by #631
Assignees

Comments

@consideRatio
Copy link
Member

With #594 it has become practical to use allowed_users alongside for example GenericOAuthenticator.allowed_groups, where being part of one or the other will allow a user to login, which makes allowed_users be something that is more widely adopted.

But use of allowed_users comes with a non-obvious behavior from the base class' Authenticator.add_user that is called on startup for all existing users and newly created users. It adds all users found in the database to the allowed_users, but only if self.allowed_users. In other words, if someone didn't have allowed_users configured, and then configures it to include a single user, they end up allowing all users in the jupyterhub databsae of users. This database can include users that was once logged in at a point in time when allowed_groups perhaps included user group A and B, but it now only includes user group A.

I'd like to not need to try communicate warnings about this behavior, but instead make allowed_users not be appended with existing users when its configured unless users explicitly opts to allow_existing_users.

Related discussion

@minrk
Copy link
Member

minrk commented Jun 22, 2023

I think if you want to override add_user and essentially float a prototype implementation of the proposal in jupyterhub/jupyterhub#4483 implemented directly in oauthenticator (where the issue is more pressing and surprising), I think that's sensible. It has the benefit of working with older releases of JupyterHub as well, rather than forcing a JupyterHub update to get the desired behavior, which is generally more disruptive than updating an authenticator.

The upgrade process should be transparent, and would then be removing the declaration of the allow_existing_users trait once JupyterHub 5 (or whatever) can be required. All other behavior will remain the same.

@consideRatio
Copy link
Member Author

@minrk okay excellent, I agree on this approach!

@consideRatio consideRatio changed the title Discussion: don't allow existing users when allowed_users is configured [All] Don't allow existing users when allowed_users is configured Jun 22, 2023
@consideRatio consideRatio self-assigned this Jun 26, 2023
@minrk minrk closed this as completed in #631 Jul 4, 2023
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.

2 participants