Add utility methods to User class

[divyansshhh]

No description provided.

[divyansshhh]

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

[kevin-bates]

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.

[divyansshhh]

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

[kevin-bates]

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.

[kevin-bates]

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

[minrk]

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

[kevin-bates]

No cookie...

Suggested change

	cookie = json.dumps(
	serialized_user = json.dumps(

[kevin-bates]

Suggested change

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

[kevin-bates]

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.

[minrk]

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.

[minrk]

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

[kevin-bates]

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]

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.