Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[FIX] notebookapp, auth:get_secure_cookie kwargs#3778

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
minrk merged 1 commit intojupyter:masterfrombeledouxdenis:master-get_secure_cookie_options
Jul 30, 2018
Merged

[FIX] notebookapp, auth:get_secure_cookie kwargs#3778

minrk merged 1 commit intojupyter:masterfrombeledouxdenis:master-get_secure_cookie_options
Jul 30, 2018

Conversation

@beledouxdenis
Copy link
Contributor

Per Tornado's documentation:

By default, Tornado’s secure cookies expire after 30 days.
To change this, use the expires_days keyword argument to
set_secure_cookie and the max_age_days argument to get_secure_cookie.
These two values are passed separately so that you may
e.g. have a cookie that is valid for 30 days for most purposes,
but for certain sensitive actions
(such as changing billing information)
you use a smaller max_age_days when reading the cookie.

With the current implementation inauth/login.py,
this is possible to pass theexpires_days option
but not possible to enforce it as this is not possible
to passmax_age_days toget_secure_cookie

This makes impossible to set the cookie expiration without
using a customLoginHandler.

This revision is about adding the possibility to pass options
to Tornado'sget_secure_cookie method,
so it can be possible to set the cookies expiration,
among others.

In my opinion,get_cookie_options is a weird naming given the options to pass toset_secure_cookie are calledcookie_options. That said, I am not sure what is your policy regarding the retro-compatibility of the settings, ifcookie_options can be renamedset_cookie_options or not. Anyway we first have to discuss the feasibility of this change.

@minrk
Copy link
Member

get_cookie_options is a little funky, since I would expect it to be a callable itself on first glance. For passthrough to other APIs, we often give configurables names likeget_secure_cookie_kwargs to communicate "these are kwargs that will be passed unmodified to get_secure_cookie." The other option is to expose just a single Float configurable forcookie_max_age_days, since there aren't as many useful options to expose for get_cookie as there are for set_cookie.

Per Tornado's documentation:>By default, Tornado’s secure cookies expire after 30 days.>To change this, use the expires_days keyword argument to>set_secure_cookie and the max_age_days argument to get_secure_cookie.>These two values are passed separately so that you may>e.g. have a cookie that is valid for 30 days for most purposes,>but for certain sensitive actions>(such as changing billing information)>you use a smaller max_age_days when reading the cookie.With the current implementation in `auth/login.py`,this is possible to pass the `expires_days` optionbut not possible to enforce it as this is not possibleto pass `max_age_days` to `get_secure_cookie`This makes impossible to set the cookie expiration withoutusing a custom `LoginHandler`.This revision is about adding the possibility to pass optionsto Tornado's `get_secure_cookie` method,so it can be possible to set the cookies expiration,among others.
@beledouxdenis
Copy link
ContributorAuthor

Thank you for your review :)

I updated the PR.

I chose theget_secure_cookie_kwargs option: To be future-proof in case Tornardo adds new arguments or in case someone is interested by the already existingmin_version argument.
http://www.tornadoweb.org/en/stable/_modules/tornado/web.html#RequestHandler.get_secure_cookie

That said, for my own purposes, I am indeed only interested to themax_age_days argument. Therefore, if you rather like thecookie_max_age_days naming, just let me know and I will update the PR.

@minrkminrk added this to the5.7 milestoneJul 30, 2018
@minrkminrk merged commitceaf1c1 intojupyter:masterJul 30, 2018
@minrk
Copy link
Member

Thanks!

@beledouxdenis
Copy link
ContributorAuthor

Thank you for having considered the PR : )

Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

5.7

Development

Successfully merging this pull request may close these issues.

2 participants

@beledouxdenis@minrk

[8]ページ先頭

©2009-2025 Movatter.jp