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

Username collision #5

Closed
Ph0tonic opened this issue Oct 3, 2023 · 3 comments
Closed

Username collision #5

Ph0tonic opened this issue Oct 3, 2023 · 3 comments

Comments

@Ph0tonic
Copy link
Contributor

Ph0tonic commented Oct 3, 2023

Hi,
Thank you very much for your work.
As previously discussed here (jupyterhub/oauthenticator#459 (comment)), we have a username collision issue to manage.

I mainly see 2 ways of managing it.

  1. Handle it in Jupyterhub
  2. Add a prefix in the authenticator's wrapper

I managed to make the second option work and opened a PR to comment #6.
For this second solution, I saw 2 main options to implement the fix, first by overriding the function normalize_username. However, this function might be called multiple times due to the implementation inside OAuthenticator. Therefore, we can't directly add a simple prefix. We should first check if a prefix exists and be 100% sure that it's indeed a prefix. The second solution was to override authenticate, check_allowed and check_blocked_user to have the prefix added or removed.

The drawback of the prefix is that it is displayed on jupyterhub page in the top right corner with the prefix.

I had a look at how to handle the name collision in Jupyterhub directly and if we want to do that, I think that we should change the database to store in the user table an additional field named authenticator. And during the authentication, instead of returning a single username or an object with 2 fields name and admin (to be check). We could also return an additional field named authenticator.

Jupyterhub code to check user duplication :
https://github.com/jupyterhub/jupyterhub/blob/29e954c407c017c045fc534410ffcd1ec6318727/jupyterhub/apihandlers/users.py#L333

@manics
Copy link
Contributor

manics commented Oct 3, 2023

I had a look at how to handle the name collision in Jupyterhub directly and if we want to do that, I think that we should change the database to store in the user table an additional field named authenticator

This change would have to cascade down to everywhere a unique identifier is used, e.g. http://<jupyterhub>/user/<authenticator-prefix><username>/, and would therefore be a big breaking change for other authenticators and spawners. What do you see as the benefits of doing it in JupyterHub instead of in this authenticator?

@Ph0tonic
Copy link
Contributor Author

Ph0tonic commented Oct 5, 2023

This change would have to cascade down to everywhere a unique identifier is used, e.g. http://<jupyterhub>/user/<authenticator-prefix><username>/, and would therefore be a big breaking change for other authenticators and spawners. What do you see as the benefits of doing it in JupyterHub instead of in this authenticator?

Hum, the main advantage of being in Jupyterhub is that it would not require any other library and would therefore be an official feature. But I agree that it might have deep impacts if the id used is the username, we would be a breaking change to generate a proper id and it would be much easier to use a simple prefix for now.

@sgaist
Copy link
Member

sgaist commented Oct 30, 2023

Fixed by #6

@sgaist sgaist closed this as completed Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants