-
Notifications
You must be signed in to change notification settings - Fork 365
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
Implement MultiOAuthenticator #459
base: main
Are you sure you want to change the base?
Conversation
This authenticator allows to use several different services to authenticate to one instance of JupyterHub.
Does |
It's a good point you have here. I haven't thought about them. As it is That said, there are some issues to address for the "local" authenticators:
For number 2, a change in the For number 3, I have tested an approach using a mixin class to overwrite For number 1, it's a bit more tricky. One possible solution could be to handle a MultiAuthenticator specific "service name" configuration parameter that could be used in place of Should I push the proof of concept on top of this merge request so you can compare the two implementation ? |
This patch is related to the implementation of the MultiAuthenticator in jupyterhub/oauthenticator#459 The issue will be triggerd when mixing local and oauth providers. With multiple providers the template generates a set of buttons to choose from to continue the login process. For OAuth, the user will be sent to the provider login page and the redirect at the end will continue nicely the process. Now for the tricky part, using a local provider (e.g. PAM), the user will be redirected to the "same page" thus the same template will be rendered but this time to show the username/password dialog. This will trip the workflow because of the action URL coming from the settings and not from the authenticator. Therefore when the button is clicked, the user will come back to the original multiple choice page rather than continue the login.
This patch is related to the implementation of the MultiAuthenticator in jupyterhub/oauthenticator#459 The issue will be triggered when using more than one local provider or mixing with oauth providers. With multiple providers the template generates a set of buttons to choose from to continue the login process. For OAuth, the user will be sent to the provider login page and the redirect at the end will continue nicely the process. Now for the tricky part: using a local provider (e.g. PAM), the user will be redirected to the "same page" thus the same template will be rendered but this time to show the username/password dialog. This will trip the workflow because of the action URL coming from the settings and not from the authenticator. Therefore when the button is clicked, the user will come back to the original multiple choice page rather than continue the login.
This patch is related to the implementation of the MultiAuthenticator in jupyterhub/oauthenticator#459 The issue will be triggered when using more than one local provider or mixing with oauth providers. With multiple providers the template generates a set of buttons to choose from to continue the login process. For OAuth, the user will be sent to the provider login page and the redirect at the end will continue nicely the process. Now for the tricky part: using a local provider (e.g. PAM), the user will be redirected to the "same page" thus the same template will be rendered but this time to show the username/password dialog. This will trip the workflow because of the action URL coming from the settings and not from the authenticator. Therefore when the button is clicked, the user will come back to the original multiple choice page rather than continue the login.
This patch is related to the implementation of the MultiAuthenticator in jupyterhub/oauthenticator#459 The issue will be triggered when using more than one local provider or mixing with oauth providers. With multiple providers the template generates a set of buttons to choose from to continue the login process. For OAuth, the user will be sent to the provider login page and the redirect at the end will continue nicely the process. Now for the tricky part: using a local provider (e.g. PAM), the user will be redirected to the "same page" thus the same template will be rendered but this time to show the username/password dialog. This will trip the workflow because of the action URL coming from the settings and not from the authenticator. Therefore when the button is clicked, the user will come back to the original multiple choice page rather than continue the login.
Small ping :-) |
Another ping. This is useful. |
This is very useful! However, as far as I can tell, it doesn't seem to have any means of handling username collisions. What happens when one person's username on one service is the same as someone else's username on another service? Some sort of prefix, or other configurable transform, may be needed for the resulting JupyterHub username. |
Thanks ! I think that, from an architecture point of view, it is something that should be handled on the JupyterHub side since it's there that the user management happens. The role of the authenticator is to handle the authentication process but not the user management. |
Hello, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
Yes, indeed, let's see what maintainers think of it.
I tested your code locally with a double configuration Gitlab
and Github
and found a few issues.
Happy to help and provide more information if needed.
class URLScopeMixin(object): | ||
"""Mixin class that adds the""" | ||
|
||
scope = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be renamed to avoid collision with scope
variable defined in oauth2
, like this, it breaks with gitlab.
scope = "" | |
url_scope = "" |
scope = "" | ||
|
||
def login_url(self, base_url): | ||
return super().login_url(url_path_join(base_url, self.scope)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return super().login_url(url_path_join(base_url, self.scope)) | |
return super().login_url(url_path_join(base_url, self.url_scope)) |
return super().login_url(url_path_join(base_url, self.scope)) | ||
|
||
def logout_url(self, base_url): | ||
return super().logout_url(url_path_join(base_url, self.scope)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return super().logout_url(url_path_join(base_url, self.scope)) | |
return super().logout_url(url_path_join(base_url, self.url_scope)) |
def get_handlers(self, app): | ||
handlers = super().get_handlers(app) | ||
return [ | ||
(url_path_join(self.scope, path), handler) for path, handler in handlers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(url_path_join(self.scope, path), handler) for path, handler in handlers | |
(url_path_join(self.url_scope, path), handler) for path, handler in handlers |
self._authenticators = [] | ||
for ( | ||
authenticator_klass, | ||
url_scope, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
url_scope, | |
url_scope_authenticator, |
configuration.pop("login_service") | ||
|
||
class WrapperAuthenticator(URLScopeMixin, authenticator_klass): | ||
scope = url_scope |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scope = url_scope | |
url_scope = url_scope_authenticator |
configuration.update(authenticator_configuration) | ||
|
||
authenticator = WrapperAuthenticator(**configuration) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When providing additional config values for example (allowed_users
) :
(GitHubOAuthenticator, '/github', {
'service_name': 'github',
'client_id': '...',
'client_secret': '...',
'allowed_users': {'rene.dumont'},
}),
Those values are not set in the created Authenticator. I am not sure for which reason. I found a way to fix this issue, see below but I am sure there is a better way linked with the way configuration
is built.
traits = authenticator.traits() | |
for key, value in authenticator_configuration.items(): | |
trait = traits.get(key, None) | |
if hasattr(authenticator, key) and trait and trait.this_class == Authenticator and trait.metadata.get('config', False): | |
setattr(authenticator, key, value) | |
|
||
class WrapperAuthenticator(URLScopeMixin, authenticator_klass): | ||
scope = url_scope | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The username conflict might also be managed in the wrapper by adding a prefix to the username.
I managed to make it work with the following code:
class WrapperAuthenticator(URLScopeMixin, authenticator_klass):
scope = url_scope
username_prefix = service_name+'_'
async def authenticate(self, handler, data=None, **kwargs):
response = await super().authenticate(handler, data, **kwargs)
if response is None:
return None
elif type(response) == str:
return self.username_prefix+response
else:
response['name'] = self.username_prefix+response['name']
return response
# def normalize_username(self, username):
# print("normalize username :",username, super().normalize_username(username))
# return self.username_prefix+super().normalize_username(username)
def check_allowed(self, username, authentication=None):
print("check allowed provided :",username)
return super().check_allowed(username.removeprefix(self.username_prefix), authentication)
def check_blocked_users(self, username, authentication=None):
print("check blocked provided :",username)
return super().check_allowed(username.removeprefix(self.username_prefix), authentication)
As you can see, I initially tried to add the prefix by overriding normalize_username
but it appears to be called inside oauth2
. This call is breaking the admin check done in oauth2:authorize
. One drawback is the display of this username's prefix in the upper corner of jupyterhub.
Hi, any update on this PR ? Should we make a PR in jupyterhub ? We could manage conflicting users in a different way if it was managed by jupyterhub. For example, by configuring an authenticator id and by storing it with the username in the db. |
My vote is to put this in a new repository/package that has oauthenticator as a dependency, collaboratively work on it there, test it in production, and then review whether some or all of it should be brought into JupyterHub. |
Hi guys, Sorry for the late reply. The plan sounds good to me. @manics would that repo be under the jupyterhub organisation or my own space ? I have done some preparation work taking my fork of oauthenticator apart to keep only the MultiAuthenticator class to follow the same project structure. The only part that might have some issue is the GitHub pipeline as I haven't used them yet actively. |
I am available to help and test, let me know once you have a new repo 👍 |
@sgaist Thanks for working on this! Starting with your own personal repo will be the easiest way for now, and we can transfer it later if we want. Personally I think something |
@manics You're welcome !
I'll create the repo during the week. |
Hi, As promised: https://github.com/idiap/multiauthenticator @Ph0tonic: thanks for the reviews and suggestions done here, I integrated your scope fixes already. And I have found a simpler way for the configuration issue. Can you test that ? Can you also open an issue over there to discuss the username collision problem ?
|
This authenticator allows to use several different services to authenticate to one instance of JupyterHub.
The code is based on the suggested implementation that can be found on #136 and more specifically #136 (comment).
Due to the merge of jupyterhub/jupyterhub#3315, the support for showing multiple links is already supported.
This is the first step as discussed in this JupyterHub discourse thread.