Allow user to stay on account locked page#804
Conversation
**Why**: For a better UX, we want to leave the user on the account locked page for the duration of the lockout period. **How**: - Reverse the order of events in `handle_second_factor_locked_user`. The reason is that application.html.slim wasn't called after signing the user out, and therefore the timeout JS was still active. By reversing the order, we force a new call to the layout template, which then sees that a current user does not exist, and therefore does not trigger the timeout JS. - Revert the change from `current_user` to `user_fully_authenticated?` in the application layout to allow users who only sign in with email and password to see the time out warning modal.
| end | ||
|
|
||
| scenario 'user who enters OTP incorrectly 3 times is locked out for OTP validity period' do | ||
| scenario 'user who enters OTP incorrectly 3 times is locked out for OTP validity period', js: true do |
| scenario 'user who enters OTP incorrectly 3 times is locked out for OTP validity period' do | ||
| scenario 'user who enters OTP incorrectly 3 times is locked out for OTP validity period', js: true do | ||
| allow(Figaro.env).to receive(:session_check_frequency).and_return('1') | ||
| allow(Figaro.env).to receive(:session_check_delay).and_return('2') |
There was a problem hiding this comment.
I am not seeing how these additions are needed in this spec file
There was a problem hiding this comment.
These settings are needed to make sure the test fails if the handle_second_factor_locked_user method is not doing the right thing. I made sure the test failed with the previous code before fixing it. Without these overrides, the default settings would apply, which would take too long to trigger the session timeout calls. The fact that the session timeout JS was still active after the lockout is what caused the bug I am fixing now. See my commit message for an explanation of the bug.
| sign_in_user(user) | ||
| visit user_two_factor_authentication_path | ||
|
|
||
| expect(page).to have_css('#session-timeout-msg') |
There was a problem hiding this comment.
does this expectation test the change we are making in this PR?
we want to leave the user on the account locked page for the duration of the lockout period.
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.
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.
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?
There was a problem hiding this comment.
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.
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.
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.
monfresh
force-pushed
the
mb-account-locked-fix
branch
from
891890a to
35aab63
Compare
|
@zachmargolis @jessieay Are my changes and responses satisfactory? |
zachmargolis
left a comment
zachmargolis
left a comment
There was a problem hiding this comment.
@monfresh LGTM! I don't have much context on the user flow/experience, but the rubies look good to me!
|
+1 to @zachmargolis ! |
* Allow user to stay on account locked page **Why**: For a better UX, we want to leave the user on the account locked page for the duration of the lockout period. **How**: - Reverse the order of events in `handle_second_factor_locked_user`. The reason is that application.html.slim wasn't called after signing the user out, and therefore the timeout JS was still active. By reversing the order, we force a new call to the layout template, which then sees that a current user does not exist, and therefore does not trigger the timeout JS. - Revert the change from `current_user` to `user_fully_authenticated?` in the application layout to allow users who only sign in with email and password to see the time out warning modal.
* Allow user to stay on account locked page **Why**: For a better UX, we want to leave the user on the account locked page for the duration of the lockout period. **How**: - Reverse the order of events in `handle_second_factor_locked_user`. The reason is that application.html.slim wasn't called after signing the user out, and therefore the timeout JS was still active. By reversing the order, we force a new call to the layout template, which then sees that a current user does not exist, and therefore does not trigger the timeout JS. - Revert the change from `current_user` to `user_fully_authenticated?` in the application layout to allow users who only sign in with email and password to see the time out warning modal.
4 participants
Why: For a better UX, we want to leave the user on the account
locked page for the duration of the lockout period.
How:
handle_second_factor_locked_user.The reason is that application.html.slim wasn't called after signing
the user out, and therefore the timeout JS was still active.
By reversing the order, we force a new call to the layout template,
which then sees that a current user does not exist, and therefore does
not trigger the timeout JS.
current_usertouser_fully_authenticated?in the application layout to allow users who only sign in with email and
password to see the time out warning modal.