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

Implement MultiOAuthenticator #459

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sgaist
Copy link
Contributor

@sgaist sgaist commented Sep 23, 2021

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.

This authenticator allows to use several different
services to authenticate to one instance of JupyterHub.
@manics
Copy link
Member

manics commented Sep 23, 2021

Does MultiOAuthenticator only work with OAuthenticator derived authenticators, or would it work with others?

@sgaist
Copy link
Contributor Author

sgaist commented Sep 24, 2021

It's a good point you have here. I haven't thought about them.

As it is MultiOAuthenticator handles any authenticator that implements the Authenticator interface. It currently does not make use of any implementation specific details of OAuthenticator.

That said, there are some issues to address for the "local" authenticators:

  1. The local authenticators have no "name" to show on the button so we could make use of the login_service configuration to give them one to generate the button in the custom html. But the login.html template makes use of login_service to determine whether it should render a button or the login form so when we get there it will render a single button that proposes to login with the authenticator selected and then back to the list of buttons when clicked.
  2. The next issue is the login url used for the post action which comes from the settings rather than using the one from the authenticator.
  3. Following point 2, the authenticator_login_url generated for the template will be wrong as the authenticator has no idea of the url_scope and thus will return an invalid value.

For number 2, a change in the login.html template to use authenticator_login_url seems to be enough.

For number 3, I have tested an approach using a mixin class to overwrite login_url, logout_url and get_handlers. It is used to dynamically create a subclasses that is configured with the scope and appends it to the base_url. This simplifies the rest of the code a bit as well.

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 login_service when generating the buttons. This would allow to not modify the base class. We could also document the requirement to create a simple subclass adding that name.

Should I push the proof of concept on top of this merge request so you can compare the two implementation ?

sgaist added a commit to sgaist/jupyterhub that referenced this pull request Dec 16, 2021
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.
sgaist added a commit to sgaist/jupyterhub that referenced this pull request Dec 16, 2021
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.
sgaist added a commit to sgaist/jupyterhub that referenced this pull request Dec 21, 2021
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.
minrk pushed a commit to sgaist/jupyterhub that referenced this pull request Dec 22, 2021
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.
@sgaist
Copy link
Contributor Author

sgaist commented Jan 21, 2022

Small ping :-)

@abdelq
Copy link

abdelq commented Feb 28, 2022

Another ping. This is useful.

@tmtabor
Copy link

tmtabor commented May 17, 2022

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.

@sgaist
Copy link
Contributor Author

sgaist commented Jun 14, 2022

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.

@Ph0tonic
Copy link

Ph0tonic commented Jun 26, 2023

Hello,
Any updates or plan to integrate this feature ?
\cc @consideRatio and @sgaist

@Ph0tonic
Copy link

Hi,
After looking at this proposition, I wonder whether this code should be incorporated into jupyterhub. As it might be used by other authenticators not provided in this library.
\cc @sgaist and @manics

@sgaist
Copy link
Contributor Author

sgaist commented Jul 12, 2023

@Ph0tonic, while it was originally geared towards OAuth providers, it something that could indeed live in JupyterHub since it also supports other providers as it is not tied to the implementations of OAuth authenticators.

Maybe @minrk might want to also weigh in.

Copy link

@Ph0tonic Ph0tonic left a 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 = ""
Copy link

@Ph0tonic Ph0tonic Jul 12, 2023

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.

Suggested change
scope = ""
url_scope = ""

scope = ""

def login_url(self, base_url):
return super().login_url(url_path_join(base_url, self.scope))

Choose a reason for hiding this comment

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

Suggested change
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))

Choose a reason for hiding this comment

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

Suggested change
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

Choose a reason for hiding this comment

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

Suggested change
(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,

Choose a reason for hiding this comment

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

Suggested change
url_scope,
url_scope_authenticator,

configuration.pop("login_service")

class WrapperAuthenticator(URLScopeMixin, authenticator_klass):
scope = url_scope

Choose a reason for hiding this comment

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

Suggested change
scope = url_scope
url_scope = url_scope_authenticator

configuration.update(authenticator_configuration)

authenticator = WrapperAuthenticator(**configuration)

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.

Suggested change
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

Copy link

@Ph0tonic Ph0tonic Jul 13, 2023

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.

@Ph0tonic
Copy link

Ph0tonic commented Aug 2, 2023

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.

@manics
Copy link
Member

manics commented Aug 2, 2023

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.

@sgaist
Copy link
Contributor Author

sgaist commented Sep 22, 2023

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.

@Ph0tonic
Copy link

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 👍

@manics
Copy link
Member

manics commented Sep 24, 2023

@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 jupyterhub-contrib would be a good home but that's still being discussed jupyterhub/team-compass#519

@sgaist
Copy link
Contributor Author

sgaist commented Sep 24, 2023

@manics You're welcome !

jupyterhub-contrib sounds good. I was thinking about something similar so it's great to see the idea was already floating around.

I'll create the repo during the week.

@manics manics marked this pull request as draft September 27, 2023 09:27
@sgaist
Copy link
Contributor Author

sgaist commented Oct 2, 2023

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 ?
I think there might be two options here:

  • Yours which ensures that each backend provides a unique user name but with the drawback of having a "scoped" username shown.
  • "Merged users" but this one is tricky as you cannot be sure that every user uses the same username on multiple platforms. Also this would require changes to the user handling in JupyterHub itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants