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

singleuser auth as server extension #3888

Merged
merged 9 commits into from Feb 15, 2023

Conversation

minrk
Copy link
Member

@minrk minrk commented May 5, 2022

This is a development branch, testing JupyterHub singleuser auth as a Jupyter Server extension

Jupyter Server 2.0 introduces two new APIs:

  • IdentityProvider for configuring authentication
  • Authorizer for configuring authorization (enables granular access, such as read-only)

This PR aims to implement these APIs for JupyterHub, without (almost) any use of monkeypatching or private APIs.
So far, I've got ~everything working in manual tests, aside from disable_user_config. I'm not 100% sure how feasible that's going to be, but hopefully I can manage.

It doesn't remove or change SingleUserNotebookApp. It mostly copies code from there (essentially a fork of the singleuser code), so that the old code can be deprecated and removed at a later time.

Co-developing this with jupyter-server/jupyter_server#825 as a way to validate that everything JupyterHub needs to fit in a server extension is covered.

TODO:

  • complete implementation
  • test/demonstrate custom Authorizer that isn't the default JupyterHub Authorizer to make sure this is possible
  • add extension + server 2.0 to test matrix
  • figure out what to do for SingleUserNotebookApp, which is likely to be incompatible with server 2.0, and defaults in jupyterhub.singleuser
  • figure out disable_user_config
  • default to extension when running with Jupyter Server 2

@minrk minrk changed the title [WIP] singleuser server extension [WIP] singleuser auth as server extension May 5, 2022
@minrk minrk added enhancement new new features and removed enhancement labels Jun 8, 2022
@minrk minrk force-pushed the server-extension branch 2 times, most recently from fafd565 to 8b03147 Compare June 17, 2022 11:59
@minrk minrk force-pushed the server-extension branch 3 times, most recently from 242e24c to a5c5e47 Compare January 19, 2023 14:29
@minrk minrk force-pushed the server-extension branch 2 times, most recently from 41d45e9 to f3afac7 Compare February 2, 2023 15:08
@minrk minrk marked this pull request as ready for review February 2, 2023 15:12
@minrk minrk changed the title [WIP] singleuser auth as server extension singleuser auth as server extension Feb 2, 2023
mostly a copy (fork) of singleuser app
using public APIs instead of lots of patching.

opt-in via `JUPYTERHUB_SINGLEUSER_EXTENSION=1`

related changes:

- stop running a test single-user server in a thread. It's complicated and fragile.
  Instead, run it normally, and get the info we need from a custom handler registered via an extension
  via the `full_spawn` fixture
@minrk
Copy link
Member Author

minrk commented Feb 2, 2023

Finally got this working! (writing before CI is green, but I'm optimistic)

Key changes:

  • the only change to the existing monkeypatch-app code is loading the custom page.html from a shared file
  • In testing, the StubSingleUserServer which ran a singleuser server in a thread for easier assertions/introspection was too hard to keep working in all possible combinations. So I elected for a simpler, cleaner, more realistic launch of subprocess servers, and an extension to enable inspecting it over the network. This works fine and is a lot simpler.

The rest is just implementing the same behavior via standard server-2.0 public APIs instead of the monkeypatching.

Questions:

  • should the extension be the default if jupyter-server 2 is present? I think maybe yes. The only reason not to is Server 2 is out, so this is a change in behavior (I had hoped to finish this PR while 2 was in prerelease, but life got in the way).

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

Amazing work on this @minrk! I think it seems very thoroughly done!

My review notes mostly include details:

  • request for additional details in docstrings
  • questions about TODO notes
  • small refactoring considerations for readability
  • questions about things I don't understand

I've not reviewed things below line ~260 or so in the extension.py file, a bit into the code that you describe is copied from the mixin - everything else though!

jupyterhub/__init__.py Show resolved Hide resolved
jupyterhub/singleuser/__init__.py Outdated Show resolved Hide resolved
jupyterhub/singleuser/__init__.py Outdated Show resolved Hide resolved
jupyterhub/singleuser/__init__.py Show resolved Hide resolved
jupyterhub/tests/test_singleuser.py Outdated Show resolved Hide resolved
jupyterhub/singleuser/extension.py Show resolved Hide resolved
jupyterhub/singleuser/extension.py Outdated Show resolved Hide resolved
jupyterhub/singleuser/extension.py Outdated Show resolved Hide resolved
jupyterhub/singleuser/extension.py Outdated Show resolved Hide resolved
jupyterhub/singleuser/extension.py Show resolved Hide resolved
@minrk
Copy link
Member Author

minrk commented Feb 8, 2023

@consideRatio thanks for the thoughtful review! I'll work on addressing the points now.

@minrk
Copy link
Member Author

minrk commented Feb 8, 2023

had to re-sync with main to get the updated black config

found some fixes required to run on ServerApp to affect extensions,
which were not affected before
- more thorough docstrings, comments
- add missing `check_hub_version` call
- remove duplicate HubAuth instance on authorizer
@minrk
Copy link
Member Author

minrk commented Feb 14, 2023

@consideRatio thanks for the suggestions! I've added detail where I could in docstrings and comments based on your review. I also updated the extension to be usd by default with Jupyter Server 2, which for JupyterHub 4 I think is the right thing to do.

@consideRatio
Copy link
Member

The new changes looks great to me! I really love the docstring additions!!!

There is a timeout failure in the tests to resolve, but from what I could tell this looked good!

@minrk
Copy link
Member Author

minrk commented Feb 14, 2023

Because this has been going for so long, I'm going to do the default-switch in its own PR, rather than tack on another change. I realize changing the default also means changing the test matrix to make sure the cases are still covered.

@minrk
Copy link
Member Author

minrk commented Feb 14, 2023

@consideRatio good catch on the check_hub_version! Adding the call that should have been there is the reason for the hang, because the URL attribute needed to be updated for the extension, since it's on a different object. Should be okay now.

This was referenced Feb 15, 2023
matches subclass log behavior
Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

Wow amazing work on this!!! Super happy about how this is now an extension of jupyter-server!

@consideRatio consideRatio merged commit c9d52ce into jupyterhub:main Feb 15, 2023
@minrk minrk deleted the server-extension branch February 16, 2023 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants