-
Notifications
You must be signed in to change notification settings - Fork 310
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
decode bytes from secure cookie #562
Conversation
The get_secure_cookie interface returns bytes not str https://www.tornadoweb.org/en/stable/web.html#tornado.web.RequestHandler.get_secure_cookie This fixes a minor issue where `handler.get_current_user` would return bytes rather than str.
Codecov Report
@@ Coverage Diff @@
## master #562 +/- ##
==========================================
- Coverage 76.93% 76.91% -0.02%
==========================================
Files 109 109
Lines 9948 9950 +2
Branches 1078 1079 +1
==========================================
Hits 7653 7653
- Misses 1913 1914 +1
- Partials 382 383 +1
Continue to review full report at Codecov.
|
@@ -172,6 +172,8 @@ def get_user(cls, handler): | |||
if user_id is None: | |||
get_secure_cookie_kwargs = handler.settings.get('get_secure_cookie_kwargs', {}) | |||
user_id = handler.get_secure_cookie(handler.cookie_name, **get_secure_cookie_kwargs ) | |||
if user_id: | |||
user_id = user_id.decode() |
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.
Question
Should this branch set handler._token_authenticated = True
.
Technically it is "cookie authenticated", however, the cookie is based on an earlier token.
I ask because it can be useful for a handler to know when token authorisation is in use, for example:
class MyExtensionHandler(ExtensionHandlerMixin, JupyterHandler):
@tornado.web.authenticated
def get(self):
user = self.get_current_user()
if self.token_authenticated:
user = getpass.getuser()
self.write(f'hello {user}')
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.
Opened #566 to follow up on this later.
(I think the Linux/PyPy3 test failure is spurious, could someone re-run it) |
Kicked! |
Sorted. |
Thanks, @oliver-sanders. This looks good to me, but I need to check against JupyterHub's unit tests before merging (hopefully sometime today). We need to add Jupyterhub to our downstream tests (using this configuration). |
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.
Tested against JupyterHub and everything passes. 👍 LGTM.
The get_secure_cookie interface returns bytes not str.
https://www.tornadoweb.org/en/stable/web.html#tornado.web.RequestHandler.get_secure_cookie
This fixes a minor issue where
handler.get_current_user
would return bytes rather than str.Instructions to replicate
Log the result of
handler.get_current_user()
, minimal example:Remove the token from the URL and reload the page (forces server to load the cookie value).
You should see the same token (because it was cashed in the cookie) but as bytes:
After this PR both should be str.