jupyter_server icon indicating copy to clipboard operation
jupyter_server copied to clipboard

Add utility methods to User class

Open divyansshhh opened this issue 2 years ago • 4 comments

divyansshhh avatar Apr 17 '23 18:04 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

divyansshhh avatar Apr 17 '23 18:04 divyansshhh

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

divyansshhh avatar Apr 18 '23 18:04 divyansshhh

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).

kevin-bates avatar May 23 '23 22:05 kevin-bates

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.

minrk avatar May 24 '23 13:05 minrk