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

[FIX] notebookapp, auth: get_secure_cookie kwargs #3778

Merged
merged 1 commit into from
Jul 30, 2018
Merged

[FIX] notebookapp, auth: get_secure_cookie kwargs #3778

merged 1 commit into from
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 in auth/login.py,
this is possible to pass the expires_days option
but not possible to enforce it as this is not possible
to pass max_age_days to get_secure_cookie

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

This revision is about adding the possibility to pass options
to Tornado's get_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 to set_secure_cookie are called cookie_options. That said, I am not sure what is your policy regarding the retro-compatibility of the settings, if cookie_options can be renamed set_cookie_options or not. Anyway we first have to discuss the feasibility of this change.

@minrk
Copy link
Member

minrk commented Jul 18, 2018

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 like get_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 for cookie_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` option
but not possible to enforce it as this is not possible
to pass `max_age_days` to `get_secure_cookie`

This makes impossible to set the cookie expiration without
using a custom `LoginHandler`.

This revision is about adding the possibility to pass options
to Tornado's `get_secure_cookie` method,
so it can be possible to set the cookies expiration,
among others.
@beledouxdenis
Copy link
Contributor Author

Thank you for your review :)

I updated the PR.

I chose the get_secure_cookie_kwargs option: To be future-proof in case Tornardo adds new arguments or in case someone is interested by the already existing min_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 the max_age_days argument. Therefore, if you rather like the cookie_max_age_days naming, just let me know and I will update the PR.

@minrk minrk added this to the 5.7 milestone Jul 30, 2018
@minrk minrk merged commit ceaf1c1 into jupyter:master Jul 30, 2018
@minrk
Copy link
Member

minrk commented Jul 30, 2018

Thanks!

@beledouxdenis
Copy link
Contributor Author

Thank you for having considered the PR : )

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants