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

Add utility methods to User class #1259

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

divyansshhh
Copy link
Contributor

No description provided.

@divyansshhh
Copy link
Contributor Author

I feel user_to_cookie and user_from_cookie should be a part of the User class.

This allows users to use an auth handler class (which isn't an IdentityProvider) to use these methods. In the current design they will have to copy it adding to the maintainence overhead

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

Hi @divyansshhh - thanks for opening this pull request.

I agree with the idea of moving these to the User class, especially if developers find that more convenient. I do have some comments about the approach that I've included below.

jupyter_server/auth/identity.py Outdated Show resolved Hide resolved
jupyter_server/auth/identity.py Outdated Show resolved Hide resolved
@divyansshhh
Copy link
Contributor Author

Thank you for the suggestion, I've made the required changes.

Comment on lines +85 to +86
"""Serialize a user to a string for storage in a cookie
If overriding in a subclass, make sure to define from_string as well.
Copy link
Member

Choose a reason for hiding this comment

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

Let's not assume this is for cookie usage...

Suggested change
"""Serialize a user to a string for storage in a cookie
If overriding in a subclass, make sure to define from_string as well.
"""Serialize a user to a string
If overriding in a subclass, make sure to define the class method from_string as well.

Comment on lines +87 to +89
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.

I don't understand how this comment comes into play here. Is the help string and comment valid?

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment and docstring are out of date, behavior changed in #863.

Default is just the user's username.
"""
# default: username is enough
cookie = json.dumps(
Copy link
Member

Choose a reason for hiding this comment

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

No cookie...

Suggested change
cookie = json.dumps(
serialized_user = json.dumps(


@classmethod
def from_string(cls, serialized_user: str) -> User:
"""Inverse of user_to_cookie"""
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"""Inverse of user_to_cookie"""
"""Creates an instance of User from a serialized user instance (the inverse of to_string)"""

def from_string(cls, serialized_user: str) -> User:
"""Inverse of user_to_cookie"""
user = json.loads(serialized_user)
try:
Copy link
Member

Choose a reason for hiding this comment

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

Is the try/except necessary? Since loads can raise as well, we might as well let things fly (no try/except) unless we want to introduce a custom exception.

Copy link
Contributor

@minrk minrk left a comment

Choose a reason for hiding this comment

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

FWIW, this is a breaking change because IdentityProviders defining their own cookie storage will no longer have any effect, they will instead have to move their implementation to a custom User subclass.

I was checking with JupyterHub, which does indeed change the cookie serialization (only a token is stored, not any part of the user model), but it does so at the get_user level, so these methods are not used.

I think it's also relevant to keep the notion of serialization for cookie purposes, which is what these methods are really for. I guess it doesn't have to be specific to cookies, but any persistence mechanism, but the sole point of these methods is serialization of users to allow later reconstruction by this class. Since it can and often should include things like credentials, we should be clear that the serialized form shouldn't be used for logs, etc. or anything else other than passing to from_string.

Comment on lines +87 to +89
Default is just the user's username.
"""
# default: username is enough
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment and docstring are out of date, behavior changed in #863.

@kevin-bates
Copy link
Member

FWIW, this is a breaking change because IdentityProviders defining their own cookie storage will no longer have any effect, they will instead have to move their implementation to a custom User subclass.

Good point. I was too focused on the idea of moving serialization to the User class, which I still agree with in principle. However, as @minrk points out, we'd then need the ability to introduce User subclasses, and there is no such support for that (and the horse has left the barn).

@minrk
Copy link
Contributor

minrk commented May 24, 2023

we'd then need the ability to introduce User subclasses

We do actually have that, by extending get_user. Subclassing User is mentioned in the get_user docstring. JupyterHub relies on it.

Ultimately, I don't think is matters so much on which class the method lives, since it's the IdentityProvider that is responsible for selecting and calling it either way - it just means the User class and the IdentityProvider are even more deeply coupled.

If the IdentityProvider wants to customize how a logged-in user is persisted (the identity provider's responsibility), it must:

  1. override get_user to write fully custom cookie logic (what JupyterHub does, since its cookie logic is imported from its HubOAuth class), or
  2. define a custom User class with serialization methods, which in turn requires overriding get_user_cookie and get_user_token to return the new class before get_user persists it in the default cookie.

So this change currently makes it a bit more work to change user serialization. That would be mitigated if .user_class were an attribute on IdentityProvider (it should not be configurable, as the User subclass is deeply coupled to the IdentityProvider implementation), because then just registering the class with the overridden methods would be able to use the default get_user logic. That said, it is likely those IdentityProvider methods need to be overridden anyway if there's custom serialization to be done (more or different fields to persist need to come from somewhere).

While it's a breaking change in the strictest sense (removal of an API), the only case I know of (JupyterHub) that modifies exactly this behavior (how a User is persisted in a cookie), does so at a higher level, so it's unaffected by the change here. Really only cases of custom user models that want to preserve default cookie persistence should be affected, and I'm not sure they exist yet.

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.

3 participants