Update PasswordsControllerTest to use modern Rails IntegrationTest #5291

martinemde · 2024-12-01T16:45:57Z

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.

codecov · 2024-12-01T16:53:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.12%. Comparing base (a8172d8) to head (58dded6).
Report is 4 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

martinemde

Since this is a sensitive controller, here's a summary of changes outside of the tests.

martinemde · 2024-12-01T16:46:54Z

app/controllers/passwords_controller.rb

@@ -38,10 +38,11 @@ def update
      @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")


Add a notice on success (though I have observed that it doesn't always display)

martinemde · 2024-12-01T16:47:25Z

app/controllers/passwords_controller.rb

      redirect_to signed_in? ? dashboard_path : sign_in_path
    else
      flash.now[:alert] = t(".failure")
-      render :edit
+      render :edit, status: :unprocessable_entity


Send a code when rendering after post/put.

martinemde · 2024-12-01T16:47:56Z

app/controllers/passwords_controller.rb

@@ -65,6 +66,7 @@ def ensure_email_present

  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?


Being extra cautious about blank tokens just in case there's ever a blank token in the database.

martinemde · 2024-12-01T16:48:55Z

app/controllers/passwords_controller.rb

@@ -98,8 +100,7 @@ def reset_params
  end

  def mfa_failure(message)
-    flash.now.alert = message
-    render template: "multifactor_auths/prompt", status: :unauthorized
+    prompt_mfa(alert: message, status: :unauthorized)


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.

martinemde · 2024-12-01T16:49:35Z

app/models/user.rb

@@ -212,12 +212,12 @@ def total_rubygems_count

  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)


Corrects an inconsistency where the expiration was still in the future after the token was cleared.

martinemde · 2024-12-01T16:50:36Z

app/models/user.rb

  def valid_confirmation_token?
-    token_expires_at > Time.zone.now
+    confirmation_token.present? && Time.zone.now.before?(token_expires_at)


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.

martinemde · 2024-12-01T16:50:52Z

app/views/multifactor_auths/prompt.html.erb

@@ -19,7 +19,7 @@
        <div class="text_field">
          <% 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" %>


Following 1Password's guidelines.

martinemde · 2024-12-01T16:53:50Z

test/factories/user.rb

+    after :create do |user, evaluator|
+      user.confirm_email! if evaluator.email_confirmed != false && evaluator.unconfirmed_email.blank?
+    end


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.

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?

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.

martinemde commented Dec 1, 2024

View reviewed changes

martinemde requested review from colby-swandale and segiddins December 1, 2024 18:28

martinemde force-pushed the martinemde/modernize-password-controller-tests branch 8 times, most recently from 0d7f8e1 to c47f971 Compare December 5, 2024 21:31

martinemde requested a review from simi December 10, 2024 01:40

simi approved these changes Jan 11, 2025

View reviewed changes

Update PasswordsControllerTest to use modern Rails IntegrationTest

58dded6

simi force-pushed the martinemde/modernize-password-controller-tests branch from c47f971 to 58dded6 Compare January 11, 2025 16:19

martinemde merged commit 160db8e into master Jan 11, 2025
22 checks passed

martinemde deleted the martinemde/modernize-password-controller-tests branch January 11, 2025 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update PasswordsControllerTest to use modern Rails IntegrationTest #5291

Update PasswordsControllerTest to use modern Rails IntegrationTest #5291

martinemde commented Dec 1, 2024 •

edited

Loading

codecov bot commented Dec 1, 2024 •

edited

Loading

martinemde left a comment

martinemde Dec 1, 2024

martinemde Dec 1, 2024

martinemde Dec 1, 2024

martinemde Dec 1, 2024

martinemde Dec 1, 2024

martinemde Dec 1, 2024

martinemde Dec 1, 2024

martinemde Dec 1, 2024 •

edited

Loading

colby-swandale Dec 13, 2024

martinemde Dec 13, 2024 •

edited

Loading

Update PasswordsControllerTest to use modern Rails IntegrationTest #5291

Update PasswordsControllerTest to use modern Rails IntegrationTest #5291

Conversation

martinemde commented Dec 1, 2024 • edited Loading

codecov bot commented Dec 1, 2024 • edited Loading

Codecov Report

martinemde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinemde Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinemde Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

martinemde commented Dec 1, 2024 •

edited

Loading

codecov bot commented Dec 1, 2024 •

edited

Loading

martinemde Dec 1, 2024 •

edited

Loading

martinemde Dec 13, 2024 •

edited

Loading