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 session expiry message #5582

Merged
merged 8 commits into from
Oct 27, 2020

Conversation

sheonhan
Copy link
Contributor

@sheonhan sheonhan commented Oct 15, 2020

Status

Ready for Review

Description of Changes

Fixes #5197

  • Use two new properties (generate_flow_record and login_record) to distinguish users who have logged in before or those who have gone through the codename generation flow.

Testing

  • Added a test to check session timeout messages aren't displayed after SESSION_EXPIRATION_MINUTES.

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make test) pass in the development container

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

@eloquence
Copy link
Member

eloquence commented Oct 15, 2020

Hi @sheonhan, thanks for working on this! :) It looks like you accidentally included a few unrelated comits in this PR: https://github.com/freedomofpress/securedrop/pull/5582/commits

Just let us know if you need help resolving.

@sheonhan
Copy link
Contributor Author

Hi @eloquence, thanks for your quick reply. Looks like I made a mistake while trying to rebase it 😅 . Will clean up and push it again with clean commits!

@sheonhan sheonhan force-pushed the 5197-fix-session-expiry-message branch from 136b503 to 78654c0 Compare October 15, 2020 01:06
@sheonhan sheonhan marked this pull request as draft October 15, 2020 02:10
@sheonhan sheonhan marked this pull request as ready for review October 17, 2020 02:40
@sheonhan
Copy link
Contributor Author

I addressed this issue by creating two new properties (generate_flow_record and login_record) to distinguish users who have logged in before or those who have gone through the codename generation flow.

But I wasn't sure if persisting two properties across sessions—even though they are simple boolean values—is discouraged from a security standpoint. I'd love feedbacks on whether this is an acceptable approach.

Copy link
Contributor

@rmol rmol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had one suggestion for the property naming. Otherwise this looks good.

securedrop/source_app/__init__.py Outdated Show resolved Hide resolved
@rmol rmol merged commit bb80bfc into freedomofpress:develop Oct 27, 2020
@eloquence eloquence added this to the 1.7.0 milestone Jan 5, 2021
@kushaldas kushaldas mentioned this pull request Jan 18, 2021
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

session expiry message shows up when not logged in
3 participants