jupyterhub / oauthenticator

OAuth + JupyterHub Authenticator = OAuthenticator
https://oauthenticator.readthedocs.io
BSD 3-Clause "New" or "Revised" License
413 stars 365 forks source link

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

Closed consideRatio closed 1 year ago

consideRatio commented 1 year ago

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 commented 1 year ago

I think if you want to override add_user and essentially float a prototype implementation of the proposal in https://github.com/jupyterhub/jupyterhub/issues/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 commented 1 year ago

@minrk okay excellent, I agree on this approach!