Skip to content
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.

Sign up for GitHub

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

Update PasswordsControllerTest to use modern Rails IntegrationTest #5291

Merged
merged 1 commit into from
Jan 11, 2025
Merged

Update PasswordsControllerTest to use modern Rails IntegrationTest #5291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions app/controllers/passwords_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Send a code when rendering after post/put.

end
end

Expand All @@ -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?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@user = User.find_by(confirmation_token:)
return login_failure(t("passwords.edit.token_failure")) unless @user&.valid_confirmation_token?
sign_out if signed_in? && @user != current_user
Expand All @@ -82,7 +84,7 @@ def password_reset_session_verified

def validate_password_reset_session
return login_failure(t("passwords.edit.token_failure")) if session[:password_reset_verified].nil?
return login_failure(t("verification_expired")) if session[:password_reset_verified] < Time.current
return login_failure(t("verification_expired")) if Time.current.after?(session[:password_reset_verified])
@user = User.find_by(id: session[:password_reset_verified_user])
login_failure(t("verification_expired")) unless @user
end
Expand All @@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end

def login_failure(alert)
Expand Down
6 changes: 3 additions & 3 deletions app/models/user.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.zone.now)
end

# confirmation token expires after 15 minutes
# 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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end

def generate_confirmation_token(reset_unconfirmed_email: true)
Expand Down
2 changes: 1 addition & 1 deletion app/views/multifactor_auths/prompt.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" %>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following 1Password's guidelines.

<% elsif @user.webauthn_only_with_recovery? %>
<%= text_field_tag :otp,
'',
Expand Down
1 change: 1 addition & 0 deletions config/locales/de.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,7 @@ de:
Ändern deines Passworts.
failure_on_missing_email: Die E-Mail darf nicht leer sein.
update:
success:
failure: Dein Passwort konnte nicht geändert werden. Bitte versuche es erneut.
multifactor_auths:
session_expired: Deine Sitzung auf der Anmeldeseite ist abgelaufen.
Expand Down
3 changes: 2 additions & 1 deletion config/locales/en.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,7 @@ en:
success: You will receive an email within the next few minutes. It contains instructions for changing your password.
failure_on_missing_email: Email can't be blank.
update:
success: Your password has been changed.
failure: Your password could not be changed. Please try again.
multifactor_auths:
session_expired: Your login page session has expired.
Expand Down Expand Up @@ -516,7 +517,7 @@ en:
otp_code: OTP code
otp_or_recovery: OTP or recovery code
recovery_code: Recovery code
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.'
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.'
security_device: Security Device
verify_code: Verify code
totps:
Expand Down
1 change: 1 addition & 0 deletions config/locales/es.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,7 @@ es:
success:
failure_on_missing_email:
update:
success:
failure:
multifactor_auths:
session_expired: Ha expirado tu sesión en la página de acceso.
Expand Down
1 change: 1 addition & 0 deletions config/locales/fr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,6 +493,7 @@ fr:
success:
failure_on_missing_email:
update:
success:
failure:
multifactor_auths:
session_expired:
Expand Down
1 change: 1 addition & 0 deletions config/locales/ja.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,7 @@ ja:
success: 数分でEメールが届きます。メールにはパスワードを変更する手順が記載されています。
failure_on_missing_email: Eメールは空にできません。
update:
success:
failure: パスワードを変更できませんでした。もう一度お試しください。
multifactor_auths:
session_expired: ログインページのセッションが期限切れになりました。
Expand Down
1 change: 1 addition & 0 deletions config/locales/nl.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,7 @@ nl:
success:
failure_on_missing_email:
update:
success:
failure:
multifactor_auths:
session_expired:
Expand Down
1 change: 1 addition & 0 deletions config/locales/pt-BR.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,7 @@ pt-BR:
success:
failure_on_missing_email:
update:
success:
failure:
multifactor_auths:
session_expired:
Expand Down
1 change: 1 addition & 0 deletions config/locales/zh-CN.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,7 @@ zh-CN:
success:
failure_on_missing_email:
update:
success:
failure:
multifactor_auths:
session_expired: 您的登录页面会话已过期。
Expand Down
1 change: 1 addition & 0 deletions config/locales/zh-TW.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,7 @@ zh-TW:
success:
failure_on_missing_email:
update:
success:
failure:
multifactor_auths:
session_expired: 您的登入頁面工作階段已過期。
Expand Down
4 changes: 4 additions & 0 deletions test/factories/user.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@
end
mfa_hashed_recovery_codes { mfa_recovery_codes.map { |code| BCrypt::Password.create(code) } }

after :create do |user, evaluator|
user.confirm_email! if evaluator.email_confirmed != false && evaluator.unconfirmed_email.blank?
end
Comment on lines +16 to +18
Copy link
Member Author

@martinemde martinemde Dec 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@martinemde martinemde Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


trait :unconfirmed do
email_confirmed { false }
unconfirmed_email { "#{SecureRandom.hex(8)}#{email}" }
Expand Down
Loading
Loading