-
Notifications
You must be signed in to change notification settings - Fork 11
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
Rename jhub-apps access_token
cookie to be explicit
#474
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
e0333f3
to
811d209
Compare
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 is really good, thank you! Just one question: do we need to keep reading the old cookie for one release cycle just in case to reduce disruption to existing users?
auth_by_param = APIKeyQuery(name="token", auto_error=False) | ||
|
||
auth_by_cookie = APIKeyCookie(name="access_token") | ||
auth_by_cookie = APIKeyCookie(name=JHUB_APPS_AUTH_COOKIE_NAME) |
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.
What do you think about:
auth_by_cookie = APIKeyCookie(name=JHUB_APPS_AUTH_COOKIE_NAME) | |
auth_by_cookie = APIKeyCookie(name=JHUB_APPS_AUTH_COOKIE_NAME) | |
auth_by_cookie_deprecated = APIKeyCookie(name="access_token") # will be removed in next version |
And adding or auth_by_cookie_deprecated
in line 48?
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.
In general I'm in favor of not disrupting user workflow. However this would only shift the disruption to a later date unless the user gets warned or notified in some way that they depend on deprecated behavior.
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.
However this would only shift the disruption to a later date unless the user gets warned or notified in some way that they depend on deprecated behavior
the new cookie would be set once they log in using this version, and I think the cookies only have 7 days lifetime so think the proposed deprecation solution is fine; also the disruption would be minimal, probably just accepting the oauth screen which we are removing on nebari either way
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.
So we are only talking about the disruption of users needing to login again once? If so, I wouldn't bother with trying to read the old one. This is just minor inconvenience and for that we need to have another PR plus review.
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.
Thanks Amit! This cost me quite some time this morning until you realized that access_token
was not coming from JupyterHub, but rather from here.
@@ -9,7 +9,7 @@ | |||
logger = structlog.get_logger(__name__) | |||
|
|||
|
|||
def create_access_token(data: dict, expires_delta: typing.Optional[timedelta] = None): | |||
def _create_access_token(data: dict, expires_delta: typing.Optional[timedelta] = None): |
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 don't know if there is a BC policy in place, but this is BC breaking.
@@ -22,7 +22,7 @@ def create_access_token(data: dict, expires_delta: typing.Optional[timedelta] = | |||
return encoded_jwt | |||
|
|||
|
|||
def get_jhub_token_from_jwt_token(token): | |||
def _get_jhub_token_from_jwt_token(token): |
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.
Same here.
Reference Issues or PRs
At the moment the name of the cookie is
access_token
, which is a very generic name. It would be better to prefix it with the app it belongs to, so as to avoid confusion while development.What does this implement/fix?
Put a
x
in the boxes that applyTesting
Documentation
Access-centered content checklist
Text styling
H1
or#
in markdown).Non-text content
Any other comments?