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

Adds anonymous users #863

Merged
merged 5 commits into from
Jul 26, 2022
Merged

Adds anonymous users #863

merged 5 commits into from
Jul 26, 2022

Conversation

hbcarlos
Copy link
Contributor

@hbcarlos hbcarlos commented Jun 2, 2022

I created the PR starting from #825.

Adds the anonymous users logic used in JupyterLab. It stores the user info in the cookie to persist between sessions as was done with the user id.

@welcome
Copy link

welcome bot commented Jun 2, 2022

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.
welcome
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! 🎉

@hbcarlos hbcarlos marked this pull request as draft June 2, 2022 15:34
@vidartf
Copy link
Member

vidartf commented Jul 8, 2022

#825 is now merged, so this can be rebased and continued now :)

@hbcarlos hbcarlos marked this pull request as ready for review July 8, 2022 16:03
@hbcarlos
Copy link
Contributor Author

hbcarlos commented Jul 8, 2022

Hey, @vidartf thanks! This is ready for review.

@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2022

Codecov Report

Merging #863 (b050a8e) into main (e59610b) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #863      +/-   ##
==========================================
+ Coverage   72.22%   72.26%   +0.04%     
==========================================
  Files          65       65              
  Lines        7985     7997      +12     
  Branches     1335     1335              
==========================================
+ Hits         5767     5779      +12     
  Misses       1811     1811              
  Partials      407      407              
Impacted Files Coverage Δ
jupyter_server/auth/identity.py 83.38% <100.00%> (+0.45%) ⬆️
jupyter_server/auth/utils.py 93.93% <100.00%> (+0.83%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e59610b...b050a8e. Read the comment docs.



# Using JupyterLab CSS variable because the colors may change with the theme
user_colors = [
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep this JupyterLab-specific color list in the frontend code and sample it from there when the user is "anonymous", not in Jupyter Server? Jupyter Server should remain frontend-agnostic, but these css variables are specific to JupyterLab.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for looking at this @Zsailer.
I just removed the list of colors, I will keep them on the frontend.

@Zsailer
Copy link
Member

Zsailer commented Jul 8, 2022

Thanks, @hbcarlos! Great stuff here!

Just a minor comment about "who" is responsible for choosing colors. I think this should depend on the frontend, not server.

Zsailer
Zsailer previously approved these changes Jul 18, 2022
@Zsailer
Copy link
Member

Zsailer commented Jul 26, 2022

The downstream tests in nbclassic is unrelated. Merging here after updating the branch. Thanks @hbcarlos!

@Zsailer Zsailer merged commit 0c22000 into jupyter-server:main Jul 26, 2022
@welcome
Copy link

welcome bot commented Jul 26, 2022

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@hbcarlos
Copy link
Contributor Author

Thanks @Zsailer!

@hbcarlos hbcarlos deleted the identity branch July 27, 2022 09:16
mlhenderson pushed a commit to mlhenderson/jupyter_server that referenced this pull request Jul 28, 2022
* Adds anonymous users

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* removes unused import

* Removes random colors

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Zachary Sailer <zsailer@apple.com>
@@ -290,11 +292,28 @@ def user_to_cookie(self, user: User) -> str:
Default is just the user's username.
"""
# default: username is enough
Copy link
Member

Choose a reason for hiding this comment

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

This comment and the docstring are now out of date :) I think also we should clarify whether this cookie format is now a required, de-facto standard by JupyterLab (will lab stop working if this cookie structure is broken?).

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.

4 participants