Introduce timeout on OTP page #3325
Conversation
| session.delete(:mfa_user) | ||
|
|
||
| if @user&.mfa_enabled? && @user&.otp_verified?(params[:otp]) | ||
| if @user&.mfa_enabled? && @user&.otp_verified?(params[:otp]) && !session_expired? |
There was a problem hiding this comment.
This expression is getting complex, it might be time to extract a variable or method here.
There was a problem hiding this comment.
I might also suggest calling this session_active? and reversing the polarity so that you don't need to negate it in this boolean expression.
There was a problem hiding this comment.
Addressed these in latest commits.
There was a problem hiding this comment.
LGTM, thanks for the changes.
config/locales/en.yml
Outdated
| multifactor_auths: | ||
| recovery_code_html: 'You can use a valid <a href="https://guides.rubygems.org/setting-up-multifactor-authentication/#using-recovery-codes-and-re-setup-a-previously-enabled-mfa", class="t-link">recovery code</a> if you have lost access to your multi-factor authentication device or to your security device.' | ||
| incorrect_otp: Your OTP code is incorrect. | ||
| session_expired: Your session has expired. |
There was a problem hiding this comment.
Should this make reference to the OTP?
There was a problem hiding this comment.
The OTP itself isn't expiring, but rather the time spent on the page. I updated the text to make it more granular - specified the login page session is what expired.
Codecov Report
@@ Coverage Diff @@
## master #3325 +/- ##
==========================================
+ Coverage 98.52% 98.57% +0.05%
==========================================
Files 113 113
Lines 3390 3378 -12
==========================================
- Hits 3340 3330 -10
+ Misses 50 48 -2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
2100b56 to
ad15a1b
Compare
8fee191 to
7acf558
Compare
| context "when OTP is correct but session expired" do | ||
| setup do | ||
| @controller.session[:mfa_user] = @user.id | ||
| @controller.session[:mfa_expires_at] = 30.minutes.ago |
There was a problem hiding this comment.
To make it more "real-world", you can utilize time traveling helpers (https://api.rubyonrails.org/classes/ActiveSupport/Testing/TimeHelpers.html#method-i-travel).
There was a problem hiding this comment.
TIL! Will make the change now, thanks :)
56582be to
1dc2390
Compare
| setup do | ||
| @controller.session[:mfa_user] = @user.id | ||
| @controller.session[:mfa_expires_at] = 15.minutes.from_now | ||
| travel 30.minutes |
There was a problem hiding this comment.
You need to "travel back" as well or use the block form (travel method).
My intention was to stop changing session manually (since it should be done in controller). So instead of changing session[:mfa_expires_at] directly, you can:
- login and get redirected to OTP page
- visit the otp page -> that should trigger the controller part setting
session[:mfa_expires_at] - travel to the future and submit the login form (doing the post request)
- ensure it failed to login
That way this test would reflect the "real-word" flow.
There was a problem hiding this comment.
Added an integration test :)
a396e15 to
d22d994
Compare
|
|
||
| def login_failure(message) | ||
| StatsD.increment "login.failure" | ||
| session[:mfa_expires_at] = nil |
There was a problem hiding this comment.
ee1fb18 to
4fcd900
Compare
Fix page timeout to be 15 minutes Small fixes as recommended Add locals, clear session on error, fix test Rubocop to the rescue Update test Add integration test Rubocop Add ensure to make sure session deletes Time-out webauthn Linting Code complexity garbage Switch to private method
68ef94a to
9ef137e
Compare
Rubygems.or 8000 g suffered a vulnerability where the OTP page didn't timeout, allowing an OTP code to be used for login even after password was changed. This PR introduces a timeout to the page by allowing the user to only have 15 minutes from the time OTP was request to when they can submit.
Closes #3279
If more than 15 minutes is taken by the user to enter an OTP, the user will not be logged in, and will be directed back to the login page with an error informing them the session expired.