-
Notifications
You must be signed in to change notification settings - Fork 166
Allow user to stay on account locked page #804
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
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
does this expectation test the change we are making in this PR?
It seems like it might but I am not sure. Perhaps adding more info to the spec description would clarify?
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 expectation seems correct, but I think the 'why' of this PR might be slightly confusing. It restores the session warning modal when the user has started the 2FA process, but not finished it.
When I updated the account locked screen, I removed that modal from the sign in flow entirely (i.e. a user will only see if after they have successfully 2FA'd).
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.
This expectation is testing a separate change. I suppose I should have split them up. One change was fixing the bug that redirected the user to the sign in page after being on the lockout page for about a minute. This change here is to make sure the session timeout modal appears on the 2FA page, which was the previous behavior before @el-mapache changed it in #772, but we didn't have a test for it.
@el-mapache, correct me if I'm wrong, but it sounded like you made this change because you thought it had something to do with the redirection from the lockout page to the sign in page. Is that correct, or was there a separate reason for this change?
Uh oh!
There was an error while loading. Please reload this page.
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.
I made the change because previously, on the account locked page, the user would be signed out and returned to the sign in screen early, as you note in your PR description.
When this happened, the session timeout modal would briefly appear. I used the
user_fully_authenticated?method so that that session timeout wouldn't appear unless the user had successfully completed the 2FA process.I thought this was acceptable behavior because the user would still have their session automatically expired if they left their 2FA process open for an extended period of time without completing the sign in process.
Sorry to make the extra work 😬
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.
No worries. The change I made here will solve the problem you mentioned without having to change the conditional in
application.html.slim. Only showing the timeout warning modal to fully authed users will negatively affect UX in my opinion, which is why I think we should revert it back.Thoughts?
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.
Yes, i agree! I was conflating that modal with a user being logged in, not just having an active session. Your solution is definitely cleaner since it uses the same logic we had and lets the framework render the correct partial based on the data it's provided.