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

User service #12926

Merged
merged 9 commits into from
Oct 28, 2022
Merged

User service #12926

merged 9 commits into from
Oct 28, 2022

Conversation

hbcarlos
Copy link
Member

@hbcarlos hbcarlos commented Aug 9, 2022

This is ready for review. I'm keeping it as a draft because we need to update the jupyter-server dependency since this need server v2.

References

Fixes #11678

Code changes

Removes the ICurrentUser token and creates a new service manager to fetch the user's identity and permissions.

User-facing changes

N/A

Backwards-incompatible changes

N/A

@hbcarlos hbcarlos self-assigned this Aug 9, 2022
@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@ellisonbg
Copy link
Contributor

Could we keep ICurrentUser and have the default implementation call the backend service. There may be situations (jupyter lite for example) where it makes more sense to customize this in the frontend.

@fcollonval
Copy link
Member

Could we keep ICurrentUser and have the default implementation call the backend service. There may be situations (jupyter lite for example) where it makes more sense to customize this in the frontend.

Could you elaborate on the need @ellisonbg ? I fear that this will create inconsistency in remix because accessing ICurrentUser may then not match accessing the user information from the service manager. Moreover it will increase the security sensible API for attackers by replacing ICurrentUser to change their permissions.

Note: JupyterLite will deal with user authentication using its custom service manager for the user endpoint (similarly to other Jupyter server endpoints): https://github.com/jupyterlite/jupyterlite/blob/6b8458dcfe051a89df191bcfe6b5a09d16194b7c/packages/server/src/app.ts#L25

@jtpio
Copy link
Member

jtpio commented Oct 26, 2022

There may be situations (jupyter lite for example) where it makes more sense to customize this in the frontend.

Most likely JupyterLite or other custom apps can handle that via their own ServiceManager.

@hbcarlos
Copy link
Member Author

The workflow Test Minimum Versions fails because the install-minimums action from the maintainer tools package replaces >= with == to force installing the minimum version of the dependencies.

The problem is that jupyterlab depends in "importlib-metadata>=3.6;python_version<\"3.10\"" wich minium version is ==3.6 but jupyterlab-server depends in "importlib-metadata>=4.8.3;python_version<\"3.10\"" wich creates a conflict, see:
https://github.com/jupyterlab/jupyterlab/actions/runs/3340448609/jobs/5530556688

  The conflict is caused by:
      jupyterlab[test] 4.0.0a30 depends on importlib-metadata>=3.6; python_version < "3.10"
      importlib-resources 1.4.0 depends on importlib-metadata; python_version < "3.8"
      jupyterlab-server 2.16.0 depends on importlib-metadata>=4.8.3; python_version < "3.10"
      The user requested (constraint) importlib-metadata==3.6

I bumped importlib-metadata>=3.6 to importlib-metadata>=4.8.3 in e0de694.

@hbcarlos
Copy link
Member Author

In jp-server v2, the terminal was moved to an extension. Some examples of standalone apps are configured to not load extensions. See:

# Turn off the Jupyter configuration system so configuration files on disk do
# not affect this app. This helps this app to truly be standalone.
os.environ["JUPYTER_NO_CONFIG"] = "1"

load_other_extensions = False

That makes the tests of the examples fail because the terminal doesn't work.
I changed the examples to load extensions in 2be6161.

@hbcarlos hbcarlos marked this pull request as ready for review October 28, 2022 12:14
@fcollonval
Copy link
Member

In jp-server v2, the terminal was moved to an extension. Some examples of standalone apps are configured to not load extensions. See:

# Turn off the Jupyter configuration system so configuration files on disk do
# not affect this app. This helps this app to truly be standalone.
os.environ["JUPYTER_NO_CONFIG"] = "1"

load_other_extensions = False

That makes the tests of the examples fail because the terminal doesn't work. I changed the examples to load extensions in 2be6161.

Would you mind opening an issue because we definitely need to do a follow up that support missing terminal feature in @jupyterlab/services?

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @hbcarlos

@fcollonval fcollonval merged commit f9d01fc into jupyterlab:master Oct 28, 2022
@fcollonval
Copy link
Member

@meeseeksdev please backport to 3.6.x

@lumberbot-app
Copy link

lumberbot-app bot commented Oct 28, 2022

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 3.6.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 f9d01fc38b5d3ab7359a97fc59d8a13115fea541
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #12926: User service'
  1. Push to a named branch:
git push YOURFORK 3.6.x:auto-backport-of-pr-12926-on-3.6.x
  1. Create a PR against branch 3.6.x, I would have named this PR:

"Backport PR #12926 on branch 3.6.x (User service)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@@ -25,7 +27,8 @@ class ExampleApp(LabServerApp):
extension_url = "/lab"
default_url = "/lab"
name = __name__
load_other_extensions = False
# In jupyter-server v2 terminals are an extension
load_other_extensions = True
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to keep that one to False as one of the main purposes of these examples is to show how to create standalone applications.

If that makes the tests fail maybe the test should be updated to not expect terminals anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

The tests check that JupyterLab starts without throwing an error in the dev console.
We could make the terminal service optional or catch the error and show a warning.

But I'm not sure what is the best solution.

Copy link
Member

Choose a reason for hiding this comment

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

I'm looking into it.

@fcollonval fcollonval mentioned this pull request Oct 28, 2022
23 tasks
fcollonval pushed a commit to fcollonval/jupyterlab that referenced this pull request Oct 28, 2022
fcollonval added a commit that referenced this pull request Oct 31, 2022
* Backport PR #12926: User service

* Fix integrity

* Handle missing user service

* Fix tests

* Integrity fix

Co-authored-by: Carlos Herrero <contact@carloshb.com>
hbcarlos added a commit to hbcarlos/jupyterlab that referenced this pull request Jan 29, 2023
* Creates a new service to fetch the identity of the user

* Change ICurrentUser to a service

* Pin jp-server v2

* Explicit test dependencies

* Fixes user tests

* Bumps importlib-metadata and fixes lint

* Bumps notebook_shim

* Fixes the examples

* Fixes the examples

Co-authored-by: Frédéric Collonval <fcollonval@gmail.com>
hbcarlos added a commit to hbcarlos/jupyterlab that referenced this pull request Jan 29, 2023
* Creates a new service to fetch the identity of the user

* Change ICurrentUser to a service

* Pin jp-server v2

* Explicit test dependencies

* Fixes user tests

* Bumps importlib-metadata and fixes lint

* Bumps notebook_shim

* Fixes the examples

* Fixes the examples

Co-authored-by: Frédéric Collonval <fcollonval@gmail.com>
hbcarlos added a commit to hbcarlos/jupyterlab that referenced this pull request Jan 29, 2023
* Creates a new service to fetch the identity of the user

* Change ICurrentUser to a service

* Pin jp-server v2

* Explicit test dependencies

* Fixes user tests

* Bumps importlib-metadata and fixes lint

* Bumps notebook_shim

* Fixes the examples

* Fixes the examples

Co-authored-by: Frédéric Collonval <fcollonval@gmail.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switching from user extension to user service
4 participants