-
Notifications
You must be signed in to change notification settings - Fork 335
Allow user to update identity values #1518
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
jupyter_server/auth/identity.py
Outdated
setattr(current_user, field, user_data[field]) | ||
|
||
# Persist changes (if applicable) | ||
self.set_login_cookie(handler, current_user) # Save updated user to cookie/session |
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.
This implementation assumes that get_user
is not overridden, which is the main thing IdentityProvider subclasses (e.g. JupyterHub) will do.
The result of this change as-is for JupyterHub will be that:
- the PATCH request succeeds
- an unused cookie is set
- the user model returned by
/api/me
is not actually changed
Probably the right thing to happen for JupyterHub which overrides get_user
but not this method (yet) is to hit a NotImplementedError. I see two ways to go about that:
- move this default implementation into the PasswordIdentityProvider, leaving the base class with NotImplementedError
- split the last
set_login_cookie
step to apersist_user_model
method, so the field validation and user model persistence don't have to be overridden at the same time
This method really does 3 things:
- validate keys (override via config works as you have it, base class can define this and it should work for all subclasses)
- validate values (not currently possible without full override of
update_user
) - persist changes (not currently possible with full override of
update_user
)
I think keeping it as a single method is okay, but that means the base class shouldn't have an implementation of it by default (It can still have it as a method, so subclasses can opt-in to default behavior). But if you want to slice it up so e.g. the key validation happens outside the typically overridden field, e.g.
def _update_user(self, ...):
# check updatable_fields
# only call update_user after validating
return self.update_user(...) # responsible for persistence, possibly _value_ validation
that would reduce the duplication required by subclasses. If you want, validate_update_user
could also be an overridable method.
raise web.HTTPError(500, "Identity provider not configured properly") | ||
|
||
try: | ||
updated_user = identity_provider.update_user(self, user_data) |
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.
I would probably pass self.current_user
here rather than self
, so it's not part of the update_user
API how we store the current user on the Handler. Then pass `self on the end for IdentityProviders that need to use Handler for persistence (some won't, e.g. JupyterHub).
Thanks @minrk for the feedbacks. To summarize, would it works if :
The 3 methods would be called in a try/except in the Is there any use case for this feature in Jupyterhub ? Probably |
That sounds like a good plan!
I think it makes sense to allow setting the fields JupyterHub doesn't understand. What's going to slow down the implementation is that JupyterHub needs to store this info somewhere. As it is now, JupyterHubIdentityProvider only persists a token in the cookie, and then fetches the whole user model from JupyterHub (because it may be updated). There isn't an obvious place to put this info, so I'll have to think about it. But as long as we get the behavior that it appropriately doesn't work in the meantime while it's not supported, that works for me. I opened jupyterhub/jupyterhub#5061 for tracking the JupyterHub implementation of this feature. |
e00ae8d
to
7d49971
Compare
7d49971
to
93385a9
Compare
I updated the PR as described above. It keeps one main |
…ield in user model
As suggested by @krassowski, there is probably no need for adding a new field in the user model. 9d1060d adds a |
Fixes #1514
This PR adds a new
PATCH
method to the api handler, to allow users to update their identity values. This handler is associated to aupdate_user
method in theIdentityProvider
, to effectively change the values.The fields that can be updated is an attribute of the
IdentityProvider
class, and can contain all the attributes ofUser
exceptusername
. By default only thecolor
can be updated.The
PasswordIdentityProvider
, which is the default identity provider, allows updating all the fields (except the username).