Update PasswordsControllerTest to use modern Rails IntegrationTest #5291
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5291 +/- ##
=======================================
Coverage 97.12% 97.12%
=======================================
Files 458 458
Lines 9588 9589 +1
=======================================
+ Hits 9312 9313 +1
Misses 276 276 ☔ View full report in Codecov by Sentry. |
| @user.reset_api_key! if reset_params[:reset_api_key] == "true" # singular | ||
| @user.api_keys.expire_all! if reset_params[:reset_api_keys] == "true" # plural | ||
| delete_password_reset_session | ||
| flash[:notice] = t(".success") |
There was a problem hiding this comment.
Add a notice on success (though I have observed that it doesn't always display)
| else | ||
| flash.now[:alert] = t(".failure") | ||
| render :edit | ||
| render :edit, status: :unprocessable_entity |
There was a problem hiding this comment.
Send a code when rendering after post/put.
|
|
||
| def validate_confirmation_token | ||
| confirmation_token = params.permit(:token).fetch(:token, "").to_s | ||
| return login_failure(t("passwords.edit.token_failure")) if confirmation_token.blank? |
There was a problem hiding this comment.
Being extra cautious about blank tokens just in case there's ever a blank token in the database.
| def mfa_failure(message) | ||
| flash.now.alert = message | ||
| render template: "multifactor_auths/prompt", status: :unauthorized | ||
| prompt_mfa(alert: message, status: :unauthorized) |
There was a problem hiding this comment.
This fixes a bug that I found where the otp form/webauthn form would lose the token in its url after a failed OTP submit.
app/models/user.rb
Outdated
| def confirm_email! | ||
| return false if unconfirmed_email && !update_email | ||
| update!(email_confirmed: true, confirmation_token: nil) | ||
| update!(email_confirmed: true, confirmation_token: nil, token_expires_at: Time.now) |
There was a problem hiding this comment.
Corrects an inconsistency where the expiration was still in the future after the token was cleared.
| # confirmation token expires after 3 hours | ||
| def valid_confirmation_token? | ||
| token_expires_at > Time.zone.now | ||
| confirmation_token.present? && Time.zone.now.before?(token_expires_at) |
There was a problem hiding this comment.
It seemed really strange that valid_confirmation_token? would return true after clearing the token. This is functionally the same with how it's used in the controllers, but helps the method behave as I would expect.
| <% if @user.totp_enabled? %> | ||
| <%= label_tag :otp, t(".otp_or_recovery"), class: 'form__label' %> | ||
| <%= text_field_tag :otp, '', class: 'form__input', autofocus: true, autocomplete: :off %> | ||
| <%= text_field_tag :otp, '', class: 'form__input', autofocus: true, autocomplete: "one-time-code" %> |
There was a problem hiding this comment.
Following 1Password's guidelines.
| after :create do |user, evaluator| | ||
| user.confirm_email! if evaluator.email_confirmed != false && evaluator.unconfirmed_email.blank? | ||
| end |
There was a problem hiding this comment.
I noticed the user factory was making users with "confirmed emails" that still had valid unexpired confirmation_tokens. This allowed some of the password tests to pass because the factory user was already in "password reset" state.
Change the factory to use the standard process, making the factory user more like an live user.
There was a problem hiding this comment.
Do we need this for every test? Can we set the correct state and then specifically for the password/confirmation token tests, we set the state required there?
There was a problem hiding this comment.
The problem is that creating a user without a token doesn't work. If you pass in nil you always get a token due to user hooks.
This confirmed state is the correct state for every other test. In the password tests we must run forgot_password! in many of them (something that was previously skipped because the factory state was wrong). It used to be that all users were in a sort of mixed email partially confirmed password reset state.
We could generate a factory user with email confirmed and no token (the standard user state for most other tests), but it would require preventing the "generate token" hook from running on factory created users. Instead, this seemed like the cleanest and most canonical solution.
0d7f8e1 to
c47f971
Compare
c47f971 to
58dded6
Compare
I was working on a vulnerability report that ended up being nothing. Just to be sure, I went through and increased the coverage on the password controller and converted it to use newer rails recommendations for functional tests instead of controller tests.