Login page #75

hbcarlos · 2021-09-23T17:30:55Z

Added a new plugin that registers a new endpoint for the login page.

The login page has three ways of login, GitHub, token or anonymous.

The GitHub login throws a server error when redirecting, looks like the error comes from FastAPI Users when it tries to retrieve the user information from GitHub. I'll debug tomorrow.

Regarding the login with the token, I'm not sure how it works.

welcome · 2021-09-23T17:30:56Z

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

plugins/login/fps_login/routes.py

davidbrochart · 2021-09-24T08:19:41Z

The GitHub login throws a server error when redirecting, looks like the error comes from FastAPI Users when it tries to retrieve the user information from GitHub. I'll debug tomorrow.

It works on my side. You need to pass your client_id and client_secret e.g. on the command line:

jupyverse --authenticator.client_id=my_client_id --authenticator.client_secret=my_client_secret

hbcarlos · 2021-09-24T09:39:24Z

Thanks, @davidbrochart! I was using the conf.toml as explained in the jupyverse-auth extension.

davidbrochart · 2021-09-24T09:40:07Z

Thanks, @davidbrochart! I was using the conf.toml as explained in the jupyverse-auth extension.

It should work too.

hbcarlos · 2021-09-24T09:41:11Z

How do the token works? How can I redirect the user to /lab adding the token provided by the user?

davidbrochart · 2021-09-24T09:44:07Z

It should work out of the box with FastAPI-Users, provided that you configured your GitHub OAuth App correctly:

hbcarlos · 2021-09-24T10:27:11Z

I have the correct callback URL, and it fails.
I think the error is because the app doesn't have enough permissions. When granting permission to Jupyverse to read my GitHub profile, it only asks for permission to read public information of my profile (my email is private). Then when logging in, there is an error when FastAPI_users tries to retrieve my email.

hbcarlos · 2021-09-24T20:24:26Z

Tests are failing because instead of returning 404 when the user doesn't have permissions, we redirect to the logging page, which returns a 200.

hbcarlos · 2021-09-24T20:49:09Z

Changed test to check if the server returns an HTML page instead of the 404.

davidbrochart

That looks great @hbcarlos !
On thing though, anonymous login doesn't seem to work for me (no effect when clicking), but maybe it's for a next PR?

plugins/login/fps_login/static/index.html

tests/test_auth.py

hbcarlos · 2021-09-25T07:23:48Z

I can take a look at anonymous login on this PR too, should be an easy fix.

hbcarlos · 2021-09-25T07:51:01Z

Regarding the anonymous login, when using the flag --authenticator.mode=noauth automatically redirects to /lab without showing the login page. And if we use user or token, clicking in the anonymous link redirects to /lab in the login page, but we don't have permission, so we are redirected to the login page again.

davidbrochart · 2021-09-25T09:43:02Z

Regarding the anonymous login, when using the flag --authenticator.mode=noauth automatically redirects to /lab without showing the login page.

Yes, that's the expected behavior.

And if we use user or token, clicking in the anonymous link redirects to /lab in the login page, but we don't have permission, so we are redirected to the login page again.

That's because we first have to register the anonymous user.

hbcarlos · 2021-09-25T09:56:24Z

Oooh, got it. I'll implement it on this PR.

hbcarlos · 2021-09-26T12:06:26Z

I've been looking into the anonymous user, and I don't think it is the right approach to the problem.
With this solution, we don't have an anonymous user. We are just registering a new user with random values.
The proper solution would be to let users in and assign a cookie to identify them but not register them in the database.

davidbrochart · 2021-09-26T14:12:55Z

With this solution, we don't have an anonymous user. We are just registering a new user with random values.

Yes, the system is user-based, even if it's not a "real" user. The token user is also a user, with a token as his ID.

The proper solution would be to let users in and assign a cookie to identify them but not register them in the database.

But then other users won't be able to "see" anonymous users? For me, an anonymous user is just a regular user (maybe with less permissions) who didn't want to bother registering himself. There is no reason for him not to be in the database.

hbcarlos · 2021-09-28T14:24:07Z

Thanks, @davidbrochart. You are right.
We need to identify anonymous users somehow. Even if they don't have an identity, we need to know who is connected.

Can we merge this PR and open a new one to tackle the role-based access control? That would make it much easier to know what every user can do.

davidbrochart · 2021-10-06T13:38:23Z

Since there are quite a lot of changes in this last commit, could you explain a bit?

hbcarlos · 2021-10-06T13:57:42Z

Yes, yes. It would be best to chat and show you the code because I changed everything even though it was fine. Sorry for that.

davidbrochart · 2021-10-06T13:59:59Z

Sure, but it's also nice to have it written here, even a summary. At a high-level, what are you trying to achieve?

hbcarlos · 2021-10-06T14:09:04Z

Okay, I'll write it tomorrow. I need to do some more changes.

davidbrochart

IIUC both noauth and token authentication modes use the same "guest user"? I like that because it solves the settings persistence issue mentioned in #61 (comment).

plugins/login/fps_login/static/index.html

plugins/auth/fps_auth/backends.py

plugins/auth/fps_auth/routes.py

hbcarlos · 2021-10-12T14:04:58Z

I'm not sure why the Check release is failing.

ERROR: Could not find a version that satisfies the requirement fps-login (from jupyverse) (from versions: none)
ERROR: No matching distribution found for fps-login

hbcarlos · 2021-10-14T08:29:49Z

The test in windows fails because we use termios doesn't work on Windows. How is it possible that the test didn't fail in other PRs?

https://stackoverflow.com/a/52063932

davidbrochart · 2021-10-14T08:37:44Z

How is it possible that the test didn't fail in other PRs?

We don't install fps-terminals in the main branch.

hbcarlos · 2021-10-14T08:43:26Z

Thanks, @davidbrochart!

It is still missing the check release test that I don't know why it is failing.

davidbrochart · 2021-10-14T08:51:25Z

I think we can ignore that, as fps-login has not been published on PyPI yet.

hbcarlos · 2021-10-15T11:52:04Z

What is missing to merge this PR and continue with Authorization?

README.md

tests/test_auth.py

plugins/login/pyproject.toml

plugins/login/fps_login/__init__.py

davidbrochart · 2021-10-15T15:24:27Z

The CI failure is caused by FPS master, see jupyter-server/fps#49.
cc @adriendelsalle

davidbrochart · 2021-10-18T08:00:09Z

tests/test_auth.py

+    expected = 200
+    content_type = "text/html; charset=utf-8"
+    if auth_mode in ["token", "user"]:
+        expected = 404


I think it would be better to return a 401 when no login plugin is installed.

Yes, but that depends on auth_config.login_url if it exists, will always redirect.

I would say that's the responsibility of the user, if they set a login_url without installing a login plugin, it's a 404, otherwise it's a 401?

But the login_url is in fps_auth config and defaults to /login. Is there a way to default to None and then the fps_login plugin modifies the auth_config to set the desired URL?

…ccording to the new behaviour

welcome · 2021-10-18T12:37:35Z

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

davidbrochart · 2021-10-18T12:37:53Z

Thanks @hbcarlos !

hbcarlos marked this pull request as draft September 23, 2021 17:33

davidbrochart force-pushed the login branch from 451abfd to c680581 Compare September 24, 2021 07:37

davidbrochart reviewed Sep 24, 2021

View reviewed changes

plugins/login/fps_login/routes.py Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

hbcarlos marked this pull request as ready for review September 24, 2021 20:53

davidbrochart approved these changes Sep 25, 2021

View reviewed changes

plugins/login/fps_login/static/index.html Outdated Show resolved Hide resolved

tests/test_auth.py Outdated Show resolved Hide resolved

davidbrochart force-pushed the login branch from 59d2e60 to f3d547e Compare September 25, 2021 09:57

hbcarlos marked this pull request as draft October 6, 2021 14:01

davidbrochart requested changes Oct 6, 2021

View reviewed changes

hbcarlos force-pushed the login branch from 3ac788d to 4680e68 Compare October 7, 2021 14:19

hbcarlos force-pushed the login branch 2 times, most recently from cf6cee4 to f9da6da Compare October 12, 2021 14:22

davidbrochart requested changes Oct 15, 2021

View reviewed changes

hbcarlos force-pushed the login branch from 247546b to 73c69c3 Compare October 15, 2021 17:32

davidbrochart reviewed Oct 18, 2021

View reviewed changes

hbcarlos added 8 commits October 18, 2021 13:44

Login page

1a3d566

Update scope when requesting github access and include static dir

6e7bd5e

Add token auth to login page

37194e8

Anonymous login using cookies

53b6a80

Change the order of plugins for installation in CI and change tests a…

d1cf113

…ccording to the new behaviour

Enable single user server by default, and update README

cbcf093

Adds support for Jupyter releaser

00df347

Review. Makes the login_url optional

b20070f

hbcarlos force-pushed the login branch from c57aa22 to b20070f Compare October 18, 2021 11:46

hbcarlos marked this pull request as ready for review October 18, 2021 11:54

davidbrochart self-requested a review October 18, 2021 12:37

davidbrochart approved these changes Oct 18, 2021

View reviewed changes

davidbrochart merged commit b03b142 into jupyter-server:main Oct 18, 2021

hbcarlos deleted the login branch October 18, 2021 12:38

This was referenced Oct 18, 2021

Delete fps.toml #106

Closed

Per-user settings/workspace #61

Closed

Login page #75

Login page #75

Conversation

hbcarlos commented Sep 23, 2021 • edited Loading

welcome bot commented Sep 23, 2021

davidbrochart commented Sep 24, 2021

hbcarlos commented Sep 24, 2021

davidbrochart commented Sep 24, 2021

hbcarlos commented Sep 24, 2021

davidbrochart commented Sep 24, 2021 • edited Loading

hbcarlos commented Sep 24, 2021

This comment has been minimized.

hbcarlos commented Sep 24, 2021

hbcarlos commented Sep 24, 2021

davidbrochart left a comment

Choose a reason for hiding this comment

hbcarlos commented Sep 25, 2021

hbcarlos commented Sep 25, 2021

davidbrochart commented Sep 25, 2021

hbcarlos commented Sep 25, 2021

hbcarlos commented Sep 26, 2021

davidbrochart commented Sep 26, 2021 • edited Loading

hbcarlos commented Sep 28, 2021

davidbrochart commented Oct 6, 2021

hbcarlos commented Oct 6, 2021

davidbrochart commented Oct 6, 2021

hbcarlos commented Oct 6, 2021

davidbrochart left a comment

Choose a reason for hiding this comment

hbcarlos commented Oct 12, 2021

hbcarlos commented Oct 14, 2021 • edited Loading

davidbrochart commented Oct 14, 2021

hbcarlos commented Oct 14, 2021

davidbrochart commented Oct 14, 2021

hbcarlos commented Oct 15, 2021

davidbrochart commented Oct 15, 2021

davidbrochart Oct 18, 2021

Choose a reason for hiding this comment

hbcarlos Oct 18, 2021

Choose a reason for hiding this comment

davidbrochart Oct 18, 2021

Choose a reason for hiding this comment

hbcarlos Oct 18, 2021

Choose a reason for hiding this comment

welcome bot commented Oct 18, 2021

davidbrochart commented Oct 18, 2021

hbcarlos commented Sep 23, 2021 •

edited

Loading

davidbrochart commented Sep 24, 2021 •

edited

Loading

davidbrochart commented Sep 26, 2021 •

edited

Loading

hbcarlos commented Oct 14, 2021 •

edited

Loading