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

feat: implement single prefix #32

Merged
merged 3 commits into from
Oct 18, 2024
Merged

feat: implement single prefix #32

merged 3 commits into from
Oct 18, 2024

Conversation

sgaist
Copy link
Member

@sgaist sgaist commented Jul 19, 2024

Describe your changes

This PR implement a new setting for the MultiAuthenticator class: username_prefix.

When set, only this prefix will be used across all Authenticators configured.

It can be useful when there's a need to use an alternate login method while keeping the same username.

INFO: the service_name feature is deprecated in favor of subclassing authenticator classes and setting the login_service property. It's more verbose however it makes the intention clearer. It will be removed in a future release.

WARNING: This can have security implications !!!

If two different users, using each a different login service have the same username returned (e.g. User1 on GitLab gets UserXYZ and User2 on GitHub also get UserXYZ, then they will have access to the same account on the JupyterHub deployment.

Issue ticket number and link

Closes #27

@sgaist
Copy link
Member Author

sgaist commented Jul 19, 2024

@minrk @manics PTAL

I have implemented the prefix as we discussed but I would like to have your opinion on the current code.

@@ -66,6 +67,12 @@ class MultiAuthenticator(Authenticator):
for JupyterHub"""

authenticators = List(help="The subauthenticators to use", config=True)
username_prefix = Unicode(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option (not better/worse, just another option) is to have this as a Callable(...) which takes the authenticator as a parameter and returns the prefix, with the default being the current behaviour.

The advantage is flexibility since you could have a mix of shared and unique prefixes, the downside is someone has to define a callable instead of a string if they want a common/no prefix c.MultiAuthenticator.username_prefix=lambda a: ""

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an interesting take !

@lahwaacz since you are behind the feature request, I would like to know whether you would prefer @manics suggestion over the current implementation.

It is better to subclass the authenticator of interest and set the
login_service property on the subclass.

Support for service_name will be removed in a feature version.
This allows for an administrator to set a prefix that will
be used for all authenticators configured.

It can be useful when there's a need to use an alternate login
method while keeping the same username.

**WARNING: This can have security implications !!!**

If two different users, using each a different login service
have the same username returned (e.g. User1 on GitLab gets UserXYZ
and User2 on GitHub also get UserXYZ, then they will have access
to the same account on the JupyterHub deployment.
@sgaist sgaist force-pushed the implement_single_prefix branch from 5b42393 to 83ad5ed Compare October 18, 2024 07:45
@sgaist sgaist merged commit edfa7e5 into main Oct 18, 2024
29 checks passed
@sgaist sgaist deleted the implement_single_prefix branch October 18, 2024 07:46
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

Successfully merging this pull request may close these issues.

How to avoid using username_prefix?
3 participants