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
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 35 additions & 33 deletions jupyter_server/auth/identity.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,39 @@ def fill_defaults(self):
if not self.display_name:
self.display_name = self.name

def to_string(self) -> str:
"""Serialize a user to a string for storage in a cookie
If overriding in a subclass, make sure to define from_string as well.
Comment on lines +85 to +86
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.

Default is just the user's username.
"""
# default: username is enough
Comment on lines +87 to +89
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.

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(

{
"username": self.username,
"name": self.name,
"display_name": self.display_name,
"initials": self.initials,
"color": self.color,
}
)
return cookie

@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)"""

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.

return User(
user["username"],
user["name"],
user["display_name"],
user["initials"],
None,
user["color"],
)
except:
raise


def _backward_compat_user(got_user: Any) -> User:
"""Backward-compatibility for LoginHandler.get_user
Expand Down Expand Up @@ -290,37 +323,6 @@ def get_handlers(self) -> list:
handlers.append((r"/logout", self.logout_handler_class))
return handlers

def user_to_cookie(self, user: User) -> str:
"""Serialize a user to a string for storage in a cookie

If overriding in a subclass, make sure to define user_from_cookie as well.

Default is just the user's username.
"""
# default: username is enough
cookie = json.dumps(
{
"username": user.username,
"name": user.name,
"display_name": user.display_name,
"initials": user.initials,
"color": user.color,
}
)
return cookie

def user_from_cookie(self, cookie_value: str) -> User | None:
"""Inverse of user_to_cookie"""
user = json.loads(cookie_value)
return User(
user["username"],
user["name"],
user["display_name"],
user["initials"],
None,
user["color"],
)

def get_cookie_name(self, handler: JupyterHandler) -> str:
"""Return the login cookie name

Expand All @@ -347,7 +349,7 @@ def set_login_cookie(self, handler: JupyterHandler, user: User) -> None:
cookie_options.setdefault("secure", True)
cookie_options.setdefault("path", handler.base_url)
cookie_name = self.get_cookie_name(handler)
handler.set_secure_cookie(cookie_name, self.user_to_cookie(user), **cookie_options)
handler.set_secure_cookie(cookie_name, user.to_string(), **cookie_options)

def _force_clear_cookie(
self, handler: JupyterHandler, name: str, path: str = "/", domain: str | None = None
Expand Down Expand Up @@ -404,7 +406,7 @@ def get_user_cookie(self, handler: JupyterHandler) -> User | None | Awaitable[Us
user_cookie = _user_cookie.decode()
# TODO: try/catch in case of change in config?
try:
return self.user_from_cookie(user_cookie)
return User.from_string(user_cookie)
except Exception as e:
# log bad cookie itself, only at debug-level
self.log.debug(f"Error unpacking user from cookie: cookie={user_cookie}", exc_info=True)
Expand Down