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

consolidate auth config on IdentityProvider #825

Merged
merged 17 commits into from
Jul 7, 2022

Conversation

minrk
Copy link
Contributor

@minrk minrk commented May 2, 2022

Following #671, we now have a proper Configurable class for managing authentication. Our previous extension point for this was a custom LoginHandler with various overridable class methods (check password, is login available, etc.).

My rough plan:

  • migrate class methods on LoginHandler to instance methods on IdentityProvider
  • migrate some (or maybe all) login-related config from ServerApp to IdentityProvider, e.g. cookie_name, password, login_handler_class
  • figure out if and how to keep deprecated custom LoginHandler approach working when overridden (or not, since it's 2.0?). One way might be a custom LegacyIdentityProvider that calls the custom LoginHandler classmethods. Alternative is to fail at startup and point folks to the IdentityProvider API. That might be best, because complicated deprecated auth is more likely to be buggy and incorrect auth.

Methods to migrate from LoginHandler to IdentityProvider, mostly unchanged (except from classmethod to instancementhod):

  • get_user
  • get_user_cookie
  • get_user_token
  • get_token
  • user_for_token
  • is_token_authenticated
  • should_check_origin
  • validate_security
  • set_login_cookie
  • password_from_settings

Changed methods:

  • password_from_settings() -> IdentityProvider.password config
  • get_login_available() split into properties auth_enabled (auth required at all) and login_available (login handler exists) since it makes sense to have a server with auth enabled, but no login page.

New methods:

  • IdentityProvider.process_login_form (now called from LoginFormhandler.post)
  • generate_anonymous_user (creates anonymous user, single place to override default UUID behavior)

Config to migrate to IdentityProvider:

  • ServerApp.token
  • ServerApp.password
  • ServerApp.require_password (handling moved to validate_security)
  • ServerApp.allow_password_change
  • ServerApp.cookie_options
  • ServerApp.get_secure_cookie_kwargs
  • ServerApp.tornado_settings["cookie_name"]
  • ServerApp.tornado_settings["secure_cookie"]

BaseHandler methods to migrate to IdentityProvider:

  • cookie_name
  • clear_login_cookie

@minrk
Copy link
Contributor Author

minrk commented May 2, 2022

One question as I shift the default implementation over to IdentityProvider classes: The current default authentication behavior allows token-based auth and password-based login via the login page form. To what degree do these belong on the base class vs a default PasswordIdentityProvider subclass?

Options I see:

  1. base IdentityProvider class is just the current default behavior. Downside: includes password implementation likely not used by subclasses
  2. base class includes token, but password is added in Password. Downside: IdentityProvider.token config always exists, but may be ignored by subclass (e.g. JupyterHub)
  3. base class is incomplete (no token, no password, not a functional class without subclassing), and
    a. single PasswordIdentityProvider adds token and password
    b. TokenIdentityProvider(IdentityProvider) adds token, PasswordIdentityProvider(TokenIdentityProvider) adds password to token. Smallest granularity, but hard to see a use because PasswordIdentityProvider without a password set is identical to TokenIdentityProvider.

My current inclination is 2., because an IdentityProvider without token support is unlikely to make sense. Exactly how tokens are implemented is easy to override.

@vidartf
Copy link
Member

vidartf commented May 3, 2022

Is there a plan to make any changes to the "/login" endpoint? In the current LoginHandler, it has both the general auth methods as well as handling any requests to "/login" if get_current_user() returns a falsy value.

@minrk
Copy link
Contributor Author

minrk commented May 4, 2022

Is there a plan to make any changes to the "/login" endpoint?

Not to the endpoint itself, but to the Handler, yes. All of the general methods will be deprecated or removed. The question remains exactly how much to break vs deprecate.

I think that JupyterHub is really the vast majority of custom auth use in Jupyter server, in which case I think breaking is fine. But if there are significant auth users out there, we can try to keep the 'old way' working via deprecations. It'll be a little tricky, but doable. I think the main case to not keeping it working via deprecation is that because it's security related, complicated deprecations may lead to bugs, and security bugs are bad!

@minrk minrk force-pushed the deprecate-login-config branch 2 times, most recently from 0a1fc22 to 285ab87 Compare May 4, 2022 12:54
@minrk minrk force-pushed the deprecate-login-config branch 2 times, most recently from c772d5d to ff1fa6e Compare May 6, 2022 18:13
@codecov-commenter
Copy link

codecov-commenter commented May 6, 2022

Codecov Report

Attention: Patch coverage is 79.66574% with 73 lines in your changes missing coverage. Please review.

Project coverage is 72.28%. Comparing base (e4bf162) to head (d783030).
Report is 398 commits behind head on main.

Files with missing lines Patch % Lines
jupyter_server/auth/identity.py 81.25% 30 Missing and 18 partials ⚠️
jupyter_server/serverapp.py 70.68% 10 Missing and 7 partials ⚠️
jupyter_server/base/handlers.py 68.00% 7 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #825      +/-   ##
==========================================
+ Coverage   71.51%   72.28%   +0.77%     
==========================================
  Files          65       65              
  Lines        7712     7974     +262     
  Branches     1289     1332      +43     
==========================================
+ Hits         5515     5764     +249     
  Misses       1804     1804              
- Partials      393      406      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@minrk minrk marked this pull request as ready for review May 9, 2022 10:23
minrk added 2 commits May 9, 2022 13:45
now that we have a configurable class to represent identity, most of the LoginHandler's custom methods belong there

Migrates:

- ServerApp.password, token config
- all LoginHandler class methods

Adds:

- LegacyIdentityProvider, LegacyLoginHandler to preserve old behavior with new API, triggered with ServerApp.login_handler_class is overridden, but not identity_provider_class.
mypy understands `isinstance(obj, Awaitable)`, but not `isawaitable(obj)`
minrk added 4 commits May 9, 2022 14:02
- JupyterHandler.cookie_name -> IdentityProvider.get_cookie_name(handler)
- JupyterHandler.clear_login_cookie -> IdentityProvider.clear_login_cookie
- JupyterHandler.force_clear_cookie -> IdentityProvider._force_clear_cookie
- ServerApp.cookie_options -> IdentityProvider.cookie_options
- ServerApp.get_secure_cookie_kwargs -> IdentityProvider.get_secure_cookie_kwargs
- ServerApp.tornado_settings[secure_cookie] -> IdentityProvider.secure_cookie
@minrk
Copy link
Contributor Author

minrk commented May 9, 2022

I've done my best to avoid changing any behavior or introducing new features in this PR. The only changes are meant to consolidate auth-related config and implementations spread across a few places (JupyterHandler, ServerApp, LoginHandler, a couple only settable directly in tornado_settings) onto the new IdentityProvider. Names and defaults are all preserved, with the exceptions:

  • JupyterHandler.cookie_name property is now IdentityProvider.get_cookie_name(handler) because the default behavior takes the request into account
  • ServerApp.password is now IdentityProvider.hashed_password, since it's the hashed password, not the password itself

And a couple of new methods/properties to make implementations more overrideable:

  • Added IdentityProvider.process_login_form(handler) to more easily override the body of the login form inputs without a custom login_handler_class
  • Added IdentityProvider.auth_enabled splitting login_available (login handler should be registered) from whether anonymous unauthenticated access is allowed (previous login_available was a single bool indicating both)
  • Added generate_anonymous_user to more easily override the anonymous-user generation. The default scheme is unchanged.
  • user_[to|from]_cookie to handle serialization to/from cookie strings (e.g. JupyterHub would only serialize the access token)

General considerations:

  • I definitely could move more of the default implementation off of the base class and onto the non-base default PasswordIdentityProvider.
  • I could also make some sub-steps private methods rather than public ones (e.g. get_user_cookie and get_user_token could be private, requiring override of the full get_user routine to change anything). For reference, JupyterHub will override get_user, so the other methods won't be used.

@minrk minrk changed the title [WIP] migrate LoginHandler config to IdentityProvider consolidate auth config on IdentityProvider May 9, 2022
@hbcarlos hbcarlos mentioned this pull request Jun 2, 2022
@minrk minrk requested a review from blink1073 July 1, 2022 18:10
Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Great work! The API changes look sensible to me.

self.tornado_settings["token"] = self.token

# deprecate accessing these directly, in favor of identity_provider?
self.tornado_settings["cookie_options"] = self.identity_provider.cookie_options
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe only add these if we're using the LegacyIdentityProvider?

@Zsailer
Copy link
Member

Zsailer commented Jul 5, 2022

Thank you, @minrk. This looks great. Really appreciate all your work here!

@Zsailer Zsailer merged commit 6fea5d8 into jupyter-server:main Jul 7, 2022
@minrk minrk deleted the deprecate-login-config branch July 7, 2022 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants