-
-
Notifications
You must be signed in to change notification settings - Fork 388
Added a local identity provider. #1333
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
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as spam.
This comment was marked as spam.
308f800
to
affd3f2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I tested it and it works, even without the --no-browser
option.
Happy to approve once you are done with changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for closing this out!
from jupyter_server.auth.identity import IdentityProvider, User | ||
|
||
|
||
def create_initials(username): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how long can username
be?
if it's bounded it's fine, but line 13 is a loop within a loop so if username
is large it could be expensive.
The variable name suggests it should be a small string in which case it's not really an issue.
Otherwise, one simple way to do it is to interrupt the loop on line 13 as soon as two consonants were found.
|
||
|
||
def create_initials(username): | ||
"""Creates initials combining first 2 consonants""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not sure we care, but generally python docstring end w/ a period .
color=None, | ||
) | ||
return user | ||
except OSError as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not using variable e
, interesting that the linter isn't picking this up...
provider = LocalIdentityProvider(log=log) | ||
|
||
user = provider.get_user(handler) | ||
user = await user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: variable redefinition, perhaps it's okay but just flagging it.
Also, why are we awaiting here? get_user
is not async
as far as I can tell.
Partially fixes #1181, long term we might want to make an update to jupyter-server for local installations.
This PR adds a local identity provider to switch user identity to local host credentials. The identity provider is only appropriate for local installations of Jupyter.
Note: Excluding the change to jupyter_ai.json, as I wasn't able to get this working, even though it is being copied to a directory in jupyter --paths.