-
Notifications
You must be signed in to change notification settings - Fork 81
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 explicit cookie config #1278
Conversation
@amplifi You mentioned (in concerned issue) that time duration for session timeout should not be greater than 24 hours. In your PR you are taking it to be around 5 days. |
@khantaalaman This PR sets the SESSION_COOKIE_AGE to 7200 seconds, which is the equivalent of two hours. https://docs.djangoproject.com/en/1.10/ref/settings/#std:setting-SESSION_COOKIE_AGE |
@amplifi Sorry, my bad. Don't know why I thought its minutes. |
@khantaalaman No worries! I appreciate the second set of eyes. |
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.
The settings should only be added to the production
settings. The dev VM doesn't do HTTPS, so you can't login locally to test things.
@oliverroick Agreed; moved the config to prod and opening an issue to add SSL to the dev VM. |
Proposed changes in this pull request
Sets explicit configuration for session length to address #1273. Also requires session cookie and CSRF token to be treated as secure by the browser.
Session length is two hours in seconds; alternative expiry suggestions?
When should this PR be merged
Should be thoroughly tested on desktop and mobile browsers before merging.
Risks
Session expiry too low could negatively impact UX, while too high poses a security risk. Needs to be evaluated in the context of our most common use cases.
Follow up actions
Implement idle timeout for sessions.
Checklist (for reviewing)
General
migration
label if a new migration is added.Functionality
Code
Tests
Documentation