From b9ac0b4279acdc518c79f589bb7dce1947509ee5 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Mon, 24 Jun 2024 16:39:46 -0400 Subject: [PATCH 01/18] changelog: Upcoming Features, Authentication, let users who's passwords are compromised to change their password --- app/controllers/application_controller.rb | 2 +- app/controllers/concerns/password_concern.rb | 25 +++++++++++ .../users/password_compromised_controller.rb | 41 ++++++++++++++++++- app/controllers/users/passwords_controller.rb | 23 ++--------- app/controllers/users/sessions_controller.rb | 4 +- app/services/analytics_events.rb | 4 ++ .../users/password_compromised/show.html.erb | 41 +++++++++++++------ config/locales/en.yml | 1 + config/locales/es.yml | 1 + config/locales/fr.yml | 1 + config/locales/zh.yml | 1 + config/routes.rb | 1 + .../users/sessions_controller_spec.rb | 8 ++-- 13 files changed, 111 insertions(+), 42 deletions(-) create mode 100644 app/controllers/concerns/password_concern.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 14ece152c6a..407ab28efed 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -225,7 +225,7 @@ def fix_broken_personal_key_url def after_sign_in_path_for(_user) return rules_of_use_path if !current_user.accepted_rules_of_use_still_valid? return user_please_call_url if current_user.suspended? - return user_password_compromised_url if session[:redirect_to_password_compromised].present? + return user_password_compromised_url if session[:redirect_to_change_password].present? return authentication_methods_setup_url if user_needs_sp_auth_method_setup? return login_add_piv_cac_prompt_url if session[:needs_to_setup_piv_cac_after_sign_in].present? return fix_broken_personal_key_url if current_user.broken_personal_key? diff --git a/app/controllers/concerns/password_concern.rb b/app/controllers/concerns/password_concern.rb new file mode 100644 index 00000000000..e7b58f9680d --- /dev/null +++ b/app/controllers/concerns/password_concern.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module PasswordConcern + extend ActiveSupport::Concern + + def send_password_reset_risc_event + event = PushNotification::PasswordResetEvent.new(user: current_user) + PushNotification::HttpPush.deliver(event) + end + + def create_event_and_notify_user_about_password_change + _event, disavowal_token = create_user_event_with_disavowal(:password_changed) + UserAlerts::AlertUserAboutPasswordChange.call(current_user, disavowal_token) + end + + def forbidden_passwords + current_user.email_addresses.flat_map do |email_address| + ForbiddenPasswords.new(email_address.email).call + end + end + + def user_params + params.require(:update_user_password_form).permit(:password, :password_confirmation) + end + end diff --git a/app/controllers/users/password_compromised_controller.rb b/app/controllers/users/password_compromised_controller.rb index 78b63f4764d..4e67a24bf60 100644 --- a/app/controllers/users/password_compromised_controller.rb +++ b/app/controllers/users/password_compromised_controller.rb @@ -2,18 +2,55 @@ module Users class PasswordCompromisedController < ApplicationController + include PasswordConcern before_action :confirm_two_factor_authenticated before_action :verify_feature_toggle_on def show - session.delete(:redirect_to_password_compromised) - @after_sign_in_path = after_sign_in_path_for(current_user) + session.delete(:redirect_to_change_password) + @update_user_password_form = UpdateUserPasswordForm.new(current_user) + @forbidden_passwords = forbidden_passwords analytics.user_password_compromised_visited end + def update + @update_user_password_form = UpdateUserPasswordForm.new(current_user, user_session) + + result = @update_user_password_form.submit(user_params) + + analytics.password_changed(**result.to_h) + analytics.password_compromised_password_changed(**result.to_h) + + if result.success? + handle_valid_password + else + handle_invalid_password + end + end + def verify_feature_toggle_on redirect_to after_sign_in_path_for(current_user) unless FeatureManagement.check_password_enabled? end + + def handle_valid_password + send_password_reset_risc_event + create_event_and_notify_user_about_password_change + + bypass_sign_in current_user + + flash[:info] = t('notices.password_changed') + if @update_user_password_form.personal_key.present? + user_session[:personal_key] = @update_user_password_form.personal_key + redirect_to manage_personal_key_url + else + redirect_to after_sign_in_path_for(current_user) + end + end + + def handle_invalid_password + @forbidden_passwords = forbidden_passwords + render :show + end end end diff --git a/app/controllers/users/passwords_controller.rb b/app/controllers/users/passwords_controller.rb index f921cd995e3..8e9d95dabce 100644 --- a/app/controllers/users/passwords_controller.rb +++ b/app/controllers/users/passwords_controller.rb @@ -3,6 +3,7 @@ module Users class PasswordsController < ApplicationController include ReauthenticationRequiredConcern + include PasswordConcern before_action :confirm_two_factor_authenticated before_action :capture_password_if_pii_present_but_locked @@ -11,9 +12,7 @@ class PasswordsController < ApplicationController def edit analytics.edit_password_visit @update_user_password_form = UpdateUserPasswordForm.new(current_user) - @forbidden_passwords = current_user.email_addresses.flat_map do |email_address| - ForbiddenPasswords.new(email_address.email).call - end + @forbidden_passwords = forbidden_passwords end def update @@ -39,10 +38,6 @@ def capture_password_if_pii_present_but_locked redirect_to capture_password_url end - def user_params - params.require(:update_user_password_form).permit(:password, :password_confirmation) - end - def handle_valid_password send_password_reset_risc_event create_event_and_notify_user_about_password_change @@ -59,25 +54,13 @@ def handle_valid_password end end - def send_password_reset_risc_event - event = PushNotification::PasswordResetEvent.new(user: current_user) - PushNotification::HttpPush.deliver(event) - end - - def create_event_and_notify_user_about_password_change - _event, disavowal_token = create_user_event_with_disavowal(:password_changed) - UserAlerts::AlertUserAboutPasswordChange.call(current_user, disavowal_token) - end - def handle_invalid_password # If the form is submitted with a password that's too short (based on # our Devise config) but that zxcvbn treats as strong enough, then we # need to provide our custom forbidden passwords data that zxcvbn needs, # otherwise the JS will throw an exception and the password strength # meter will not appear. - @forbidden_passwords = current_user.email_addresses.flat_map do |email_address| - ForbiddenPasswords.new(email_address.email).call - end + @forbidden_passwords = forbidden_passwords render :edit end end diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index 315af26fffa..94b008ba183 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -116,9 +116,9 @@ def handle_valid_authentication user_id: current_user.id, email: auth_params[:email], ) - check_password_compromised user_session[:platform_authenticator_available] = params[:platform_authenticator_available] == 'true' + check_password_compromised redirect_to next_url_after_valid_authentication end @@ -205,7 +205,7 @@ def check_password_compromised return if current_user.password_compromised_checked_at.present? || !eligible_for_password_lookup? - session[:redirect_to_password_compromised] = + session[:redirect_to_change_password] = PwnedPasswords::LookupPassword.call(auth_params[:password]) update_user_password_compromised_checked_at end diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 3fc8a440dcd..4354c384880 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -4363,6 +4363,10 @@ def password_changed(success:, errors:, error_details: nil, **extra) track_event('Password Changed', success:, errors:, error_details:, **extra) end + def password_compromised_password_changed(success:, errors:, error_details: nil, **extra) + track_event(:password_compromised_password_changed, success:, errors:, error_details:, **extra) + end + # @param [Boolean] success Whether form validation was successful # @param [Hash] errors Errors resulting from form validation # @param [Hash] error_details Details for errors that occurred in unsuccessful submission diff --git a/app/views/users/password_compromised/show.html.erb b/app/views/users/password_compromised/show.html.erb index 3ffd9914932..627a297910f 100644 --- a/app/views/users/password_compromised/show.html.erb +++ b/app/views/users/password_compromised/show.html.erb @@ -1,15 +1,30 @@ -<%= render( - 'idv/shared/error', - heading: 'Password Compromised', - ) do %> -

- PASSWORD COMPROMISED -

+<% self.title = t('titles.edit_info.password') %> + +<%= render AlertComponent.new( + type: :warning, + id: 'password-compromised', + class: 'margin-bottom-4', + ) do + t('users.password_compromised.warning', app_name: APP_NAME) + end +%> + +<%= render PageHeadingComponent.new.with_content(t('headings.edit_info.password')) %> + + +<%= simple_form_for(@update_user_password_form, url: user_password_compromised_path, method: :post) do |f| %> + <%= f.error_notification %> + <%= render PasswordConfirmationComponent.new( + form: f, + password_label: t('forms.passwords.edit.labels.password'), + forbidden_passwords: @forbidden_passwords, + field_options: { + input_html: { + aria: { describedby: 'password-description' }, + }, + }, + ) %> + <%= f.submit t('forms.passwords.edit.buttons.submit'), class: 'display-block margin-top-5 margin-bottom-4' %> <% end %> -
- <%= link_to( - @after_sign_in_path, - class: 'usa-button usa-button--wide usa-button--big margin-bottom-3', - ) { 'SKIP' } %> -
\ No newline at end of file +<%= render 'shared/password_accordion' %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 3c931ad39a4..199fa29dd1b 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1910,6 +1910,7 @@ users.delete.bullet_4: We will notify the agencies you access with %{app_name} t users.delete.heading: Are you sure you want to delete your account? users.delete.instructions: Enter your password to confirm that you want to delete your account. users.delete.subheading: 'If you delete your account:' +users.password_compromised.warning: We found your password in a data breach on another site or app. %{app_name} requires you to change your password to protect your account. users.personal_key.accessible_labels.code_example: A personal key example with 16 characters users.personal_key.accessible_labels.preview: Personal key preview users.personal_key.confirmation_error: You’ve entered an incorrect personal key. diff --git a/config/locales/es.yml b/config/locales/es.yml index e32ab74ce44..db54dadc492 100644 --- a/config/locales/es.yml +++ b/config/locales/es.yml @@ -1921,6 +1921,7 @@ users.delete.bullet_4: Notificaremos a las agencias a las que acceda con %{app_n users.delete.heading: '¿Está seguro de que desea eliminar su cuenta?' users.delete.instructions: Ingrese su contraseña para confirmar que desea eliminar su cuenta. users.delete.subheading: 'Si elimina su cuenta:' +users.password_compromised.warning: Hemos encontrado su contraseña en una filtración de datos en otro sitio o aplicación. %{app_name} requiere que cambie su contraseña para proteger su cuenta. users.personal_key.accessible_labels.code_example: Un ejemplo de clave personal con 16 caracteres users.personal_key.accessible_labels.preview: Vista previa de la clave personal users.personal_key.confirmation_error: Ingresó una clave personal incorrecta. diff --git a/config/locales/fr.yml b/config/locales/fr.yml index a70d855ed5f..cc6f68d2e63 100644 --- a/config/locales/fr.yml +++ b/config/locales/fr.yml @@ -1910,6 +1910,7 @@ users.delete.bullet_4: Nous informerons les organismes auxquels vous accédez av users.delete.heading: Êtes-vous sûr de vouloir supprimer votre compte ? users.delete.instructions: Saisissez votre mot de passe pour confirmer que vous souhaitez supprimer votre compte. users.delete.subheading: 'Si vous supprimez votre compte :' +users.password_compromised.warning: Nous avons trouvé votre mot de passe dans le cadre d’une fuite de données sur un autre site Web ou une application. %{app_name} vous demande de modifier votre mot de passe pour protéger votre compte. users.personal_key.accessible_labels.code_example: Un exemple de clé personnelle à 16 caractères users.personal_key.accessible_labels.preview: Aperçu de clé personnelle users.personal_key.confirmation_error: Vous avez saisi une clé personnelle erronée. diff --git a/config/locales/zh.yml b/config/locales/zh.yml index a8b5be89b5e..1506001b7cc 100644 --- a/config/locales/zh.yml +++ b/config/locales/zh.yml @@ -1926,6 +1926,7 @@ users.delete.bullet_4: 我们将通知你使用 %{app_name} 访问的政府机 users.delete.heading: 你确定要删除账户吗? users.delete.instructions: 输入密码来确认你要删除账户。 users.delete.subheading: 如果你删除自己的账户: +users.password_compromised.warning: 另一个网站或应用程序的数据泄露中我们发现有你的密码。%{app_name} 要求你更改密码来保护你的账户。 users.personal_key.accessible_labels.code_example: 有 16 个字符的个人密钥示例 users.personal_key.accessible_labels.preview: 个人密钥预览 users.personal_key.confirmation_error: 你输入的个人密钥不对 diff --git a/config/routes.rb b/config/routes.rb index 478efd2cd7e..19f76f286e8 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -297,6 +297,7 @@ get '/user_please_call' => 'users/please_call#show' get '/user_password_compromised' => 'users/password_compromised#show' + post '/user_password_compromised' => 'users/password_compromised#update' post '/sign_up/create_password' => 'sign_up/passwords#create', as: :sign_up_create_password get '/sign_up/email/confirm' => 'sign_up/email_confirmations#create', diff --git a/spec/controllers/users/sessions_controller_spec.rb b/spec/controllers/users/sessions_controller_spec.rb index 6a9f029888a..af15443646e 100644 --- a/spec/controllers/users/sessions_controller_spec.rb +++ b/spec/controllers/users/sessions_controller_spec.rb @@ -262,7 +262,7 @@ it 'stores in session redirect to check compromise' do post :create, params: { user: { email: user.email, password: user.password } } - expect(controller.session[:redirect_to_password_compromised]).to be_truthy + expect(controller.session[:redirect_to_change_password]).to be_truthy end end @@ -280,7 +280,7 @@ it 'does not update the user ' do post :create, params: { user: { email: user.email, password: user.password } } - expect(controller.session[:redirect_to_password_compromised]).to be_falsey + expect(controller.session[:redirect_to_change_password]).to be_falsey end end end @@ -307,7 +307,7 @@ it 'stores in session false to attempt to redirect password compromised' do post :create, params: { user: { email: user.email, password: user.password } } - expect(controller.session[:redirect_to_password_compromised]).to be_falsey + expect(controller.session[:redirect_to_change_password]).to be_falsey end end @@ -325,7 +325,7 @@ it 'does not update the user ' do post :create, params: { user: { email: user.email, password: user.password } } - expect(controller.session[:redirect_to_password_compromised]).to be_falsey + expect(controller.session[:redirect_to_change_password]).to be_falsey end end end From b9a05edc4b3d192aec789b754ef047ea4253719b Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Tue, 25 Jun 2024 13:19:47 -0400 Subject: [PATCH 02/18] add password compromised spec and edit objects --- app/controllers/concerns/password_concern.rb | 2 +- .../users/password_compromised_controller.rb | 6 +- app/services/analytics_events.rb | 4 - .../password_compromised_controller_spec.rb | 127 ++++++++++++++++++ .../users/passwords_controller_spec.rb | 2 +- 5 files changed, 131 insertions(+), 10 deletions(-) create mode 100644 spec/controllers/users/password_compromised_controller_spec.rb diff --git a/app/controllers/concerns/password_concern.rb b/app/controllers/concerns/password_concern.rb index e7b58f9680d..544bef43bd2 100644 --- a/app/controllers/concerns/password_concern.rb +++ b/app/controllers/concerns/password_concern.rb @@ -22,4 +22,4 @@ def forbidden_passwords def user_params params.require(:update_user_password_form).permit(:password, :password_confirmation) end - end +end diff --git a/app/controllers/users/password_compromised_controller.rb b/app/controllers/users/password_compromised_controller.rb index 4e67a24bf60..943a9963571 100644 --- a/app/controllers/users/password_compromised_controller.rb +++ b/app/controllers/users/password_compromised_controller.rb @@ -17,10 +17,8 @@ def update @update_user_password_form = UpdateUserPasswordForm.new(current_user, user_session) result = @update_user_password_form.submit(user_params) - - analytics.password_changed(**result.to_h) - analytics.password_compromised_password_changed(**result.to_h) - + updated_result = result.to_h.merge({ original_password_compromised: true }) + analytics.password_changed(**updated_result) if result.success? handle_valid_password else diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 4354c384880..3fc8a440dcd 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -4363,10 +4363,6 @@ def password_changed(success:, errors:, error_details: nil, **extra) track_event('Password Changed', success:, errors:, error_details:, **extra) end - def password_compromised_password_changed(success:, errors:, error_details: nil, **extra) - track_event(:password_compromised_password_changed, success:, errors:, error_details:, **extra) - end - # @param [Boolean] success Whether form validation was successful # @param [Hash] errors Errors resulting from form validation # @param [Hash] error_details Details for errors that occurred in unsuccessful submission diff --git a/spec/controllers/users/password_compromised_controller_spec.rb b/spec/controllers/users/password_compromised_controller_spec.rb new file mode 100644 index 00000000000..23dfb453df4 --- /dev/null +++ b/spec/controllers/users/password_compromised_controller_spec.rb @@ -0,0 +1,127 @@ +require 'rails_helper' + +RSpec.describe Users::PasswordCompromisedController, allowed_extra_analytics: [:*] do + describe '#show' do + let(:user) { create(:user) } + context 'check_password_enabled is not enabled' do + before do + allow(FeatureManagement).to receive(:check_password_enabled?).and_return(false) + stub_sign_in(user) + stub_analytics + end + + it 'redirects to account page' do + get :show + expect(response).to redirect_to account_url + end + end + + context 'check_password_enabled is enabled' do + before do + allow(FeatureManagement).to receive(:check_password_enabled?).and_return(true) + stub_sign_in(user) + stub_analytics + session[:redirect_to_change_password] = true + end + + it 'renders password compromised page' do + get :show + expect(response).to render_template(:show) + expect(session[:redirect_to_change_password]).to be_nil + end + + it 'logs analytics for password compromised visited' do + get :show + expect(@analytics).to have_logged_event(:user_password_compromised_visited) + end + end + end + + describe '#update' do + let(:user) { create(:user) } + context 'check_password_enabled is not enabled' do + before do + allow(FeatureManagement).to receive(:check_password_enabled?).and_return(false) + stub_sign_in(user) + stub_analytics + end + + it 'redirects to account page' do + post :update + expect(response).to redirect_to account_url + end + end + + context 'check_password_enabled is enabled' do + let(:password) { 'salty new password' } + let(:params) do + { + password: password, + password_confirmation: password, + } + end + before do + allow(FeatureManagement).to receive(:check_password_enabled?).and_return(true) + stub_sign_in(user) + stub_analytics + end + + context 'proper password form submission' do + it 'updates the user password and logs analytic event' do + allow(@analytics).to receive(:track_event) + patch :update, params: { update_user_password_form: params } + + expect(@analytics).to have_received(:track_event).with( + 'Password Changed', + success: true, + errors: {}, + error_details: nil, + pending_profile_present: false, + active_profile_present: false, + user_id: subject.current_user.uuid, + original_password_compromised: true, + ) + expect(response).to redirect_to account_url + expect(flash[:info]).to eq t('notices.password_changed') + end + + it 'sends email notifying user of password change' do + allow(@analytics).to receive(:track_event) + + patch :update, params: { update_user_password_form: params } + expect_delivered_email_count(1) + end + + it 'sends a security event' do + security_event = PushNotification::PasswordResetEvent.new(user: user) + expect(PushNotification::HttpPush).to receive(:deliver).with(security_event) + + patch :update, params: { update_user_password_form: params } + end + end + + context 'improper password form submission' do + let(:password) { 'new' } + let(:params) do + { + password: password, + password_confirmation: password, + } + end + + it 'does not create a password_changed user Event' do + stub_sign_in + + expect(controller).to_not receive(:create_user_event) + + patch :update, params: { update_user_password_form: params } + end + + it 'renders show page' do + patch :update, params: { update_user_password_form: params } + expect(response).to render_template(:show) + end + end + end + end +end diff --git a/spec/controllers/users/passwords_controller_spec.rb b/spec/controllers/users/passwords_controller_spec.rb index 548cb918a6d..7b1a4b8d8e6 100644 --- a/spec/controllers/users/passwords_controller_spec.rb +++ b/spec/controllers/users/passwords_controller_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' RSpec.describe Users::PasswordsController, allowed_extra_analytics: [:*] do - context 'user visits add an email address page' do + context 'user visits edit password page' do let(:user) { create(:user) } before do stub_sign_in(user) From d4e032ffd0484fdc7b30a0ed3d0429bd7c691763 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Tue, 25 Jun 2024 13:24:49 -0400 Subject: [PATCH 03/18] sign in spec updated --- spec/features/users/sign_in_spec.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/spec/features/users/sign_in_spec.rb b/spec/features/users/sign_in_spec.rb index 20fceb7bd6b..6f7de6f2760 100644 --- a/spec/features/users/sign_in_spec.rb +++ b/spec/features/users/sign_in_spec.rb @@ -989,6 +989,20 @@ expect(current_path).to eq user_password_compromised_path end + + it 'should redirect user to account page after editing password' do + visit new_user_session_path + fill_in_credentials_and_submit(user.email, user.password) + fill_in_code_with_last_phone_otp + click_submit_default + + password = 'sugary pickles' + fill_in t('forms.passwords.edit.labels.password'), with: password + fill_in t('components.password_confirmation.confirm_label'), with: password + click_button t('forms.passwords.edit.buttons.submit') + + expect(current_path).to eq account_path + end end context 'user is not chosen to check if password compromised' do From 9b4d4766fcee56d33c63629e4dbbb1e93e735a33 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Tue, 25 Jun 2024 13:26:40 -0400 Subject: [PATCH 04/18] change to use user_password_params --- app/controllers/concerns/password_concern.rb | 2 +- app/controllers/users/password_compromised_controller.rb | 2 +- app/controllers/users/passwords_controller.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/concerns/password_concern.rb b/app/controllers/concerns/password_concern.rb index 544bef43bd2..42f6912e7f2 100644 --- a/app/controllers/concerns/password_concern.rb +++ b/app/controllers/concerns/password_concern.rb @@ -19,7 +19,7 @@ def forbidden_passwords end end - def user_params + def user_password_params params.require(:update_user_password_form).permit(:password, :password_confirmation) end end diff --git a/app/controllers/users/password_compromised_controller.rb b/app/controllers/users/password_compromised_controller.rb index 943a9963571..e334bf61ebb 100644 --- a/app/controllers/users/password_compromised_controller.rb +++ b/app/controllers/users/password_compromised_controller.rb @@ -16,7 +16,7 @@ def show def update @update_user_password_form = UpdateUserPasswordForm.new(current_user, user_session) - result = @update_user_password_form.submit(user_params) + result = @update_user_password_form.submit(user_password_params) updated_result = result.to_h.merge({ original_password_compromised: true }) analytics.password_changed(**updated_result) if result.success? diff --git a/app/controllers/users/passwords_controller.rb b/app/controllers/users/passwords_controller.rb index 8e9d95dabce..e63e5ae4f62 100644 --- a/app/controllers/users/passwords_controller.rb +++ b/app/controllers/users/passwords_controller.rb @@ -18,7 +18,7 @@ def edit def update @update_user_password_form = UpdateUserPasswordForm.new(current_user, user_session) - result = @update_user_password_form.submit(user_params) + result = @update_user_password_form.submit(user_password_params) analytics.password_changed(**result.to_h) From 9e60712513ed46259b1c495d44f472d4ee3c0140 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Wed, 26 Jun 2024 11:02:14 -0400 Subject: [PATCH 05/18] address comments --- app/controllers/concerns/password_concern.rb | 2 +- app/controllers/users/password_compromised_controller.rb | 8 ++++---- app/forms/update_user_password_form.rb | 4 +++- app/views/users/password_compromised/show.html.erb | 6 ------ 4 files changed, 8 insertions(+), 12 deletions(-) diff --git a/app/controllers/concerns/password_concern.rb b/app/controllers/concerns/password_concern.rb index 42f6912e7f2..be4f5bb7542 100644 --- a/app/controllers/concerns/password_concern.rb +++ b/app/controllers/concerns/password_concern.rb @@ -16,7 +16,7 @@ def create_event_and_notify_user_about_password_change def forbidden_passwords current_user.email_addresses.flat_map do |email_address| ForbiddenPasswords.new(email_address.email).call - end + end.uniq end def user_password_params diff --git a/app/controllers/users/password_compromised_controller.rb b/app/controllers/users/password_compromised_controller.rb index e334bf61ebb..c5457f1417f 100644 --- a/app/controllers/users/password_compromised_controller.rb +++ b/app/controllers/users/password_compromised_controller.rb @@ -14,11 +14,10 @@ def show end def update - @update_user_password_form = UpdateUserPasswordForm.new(current_user, user_session) + @update_user_password_form = UpdateUserPasswordForm.new(current_user, user_session, true) result = @update_user_password_form.submit(user_password_params) - updated_result = result.to_h.merge({ original_password_compromised: true }) - analytics.password_changed(**updated_result) + analytics.password_changed(**result) if result.success? handle_valid_password else @@ -34,7 +33,8 @@ def verify_feature_toggle_on def handle_valid_password send_password_reset_risc_event create_event_and_notify_user_about_password_change - + # Changing the password hash terminates the warden session, and bypass_sign_in ensures + # that the user remains authenticated. bypass_sign_in current_user flash[:info] = t('notices.password_changed') diff --git a/app/forms/update_user_password_form.rb b/app/forms/update_user_password_form.rb index eeb72f2f9ec..213fda91422 100644 --- a/app/forms/update_user_password_form.rb +++ b/app/forms/update_user_password_form.rb @@ -6,9 +6,10 @@ class UpdateUserPasswordForm delegate :personal_key, to: :encryptor - def initialize(user, user_session = nil) + def initialize(user, user_session = nil, original_password_compromised = false) @user = user @user_session = user_session + @original_password_compromised = original_password_compromised @validate_confirmation = true end @@ -48,6 +49,7 @@ def extra_analytics_attributes active_profile_present: user.active_profile.present?, pending_profile_present: user.pending_profile.present?, user_id: user.uuid, + original_password_compromised: original_password_compromised, } end end diff --git a/app/views/users/password_compromised/show.html.erb b/app/views/users/password_compromised/show.html.erb index 627a297910f..e28854706d8 100644 --- a/app/views/users/password_compromised/show.html.erb +++ b/app/views/users/password_compromised/show.html.erb @@ -2,7 +2,6 @@ <%= render AlertComponent.new( type: :warning, - id: 'password-compromised', class: 'margin-bottom-4', ) do t('users.password_compromised.warning', app_name: APP_NAME) @@ -18,11 +17,6 @@ form: f, password_label: t('forms.passwords.edit.labels.password'), forbidden_passwords: @forbidden_passwords, - field_options: { - input_html: { - aria: { describedby: 'password-description' }, - }, - }, ) %> <%= f.submit t('forms.passwords.edit.buttons.submit'), class: 'display-block margin-top-5 margin-bottom-4' %> <% end %> From 556a3d8b77b80ce402db890dce371dfc209e65d5 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Fri, 28 Jun 2024 10:30:46 -0400 Subject: [PATCH 06/18] change up to use password controller --- app/controllers/application_controller.rb | 2 +- app/controllers/concerns/password_concern.rb | 25 ---- .../users/password_compromised_controller.rb | 54 -------- app/controllers/users/passwords_controller.rb | 47 ++++++- app/forms/update_user_password_form.rb | 8 +- app/presenters/update_password_presenter.rb | 23 ++++ app/services/analytics_events.rb | 5 +- app/views/users/passwords/edit.html.erb | 24 +++- config/routes.rb | 2 - .../password_compromised_controller_spec.rb | 127 ------------------ .../users/passwords_controller_spec.rb | 119 ++++++++++++++++ 11 files changed, 210 insertions(+), 226 deletions(-) delete mode 100644 app/controllers/concerns/password_concern.rb delete mode 100644 app/controllers/users/password_compromised_controller.rb create mode 100644 app/presenters/update_password_presenter.rb delete mode 100644 spec/controllers/users/password_compromised_controller_spec.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 407ab28efed..4736fa9b479 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -225,7 +225,7 @@ def fix_broken_personal_key_url def after_sign_in_path_for(_user) return rules_of_use_path if !current_user.accepted_rules_of_use_still_valid? return user_please_call_url if current_user.suspended? - return user_password_compromised_url if session[:redirect_to_change_password].present? + return manage_password_url if session[:redirect_to_change_password].present? return authentication_methods_setup_url if user_needs_sp_auth_method_setup? return login_add_piv_cac_prompt_url if session[:needs_to_setup_piv_cac_after_sign_in].present? return fix_broken_personal_key_url if current_user.broken_personal_key? diff --git a/app/controllers/concerns/password_concern.rb b/app/controllers/concerns/password_concern.rb deleted file mode 100644 index be4f5bb7542..00000000000 --- a/app/controllers/concerns/password_concern.rb +++ /dev/null @@ -1,25 +0,0 @@ -# frozen_string_literal: true - -module PasswordConcern - extend ActiveSupport::Concern - - def send_password_reset_risc_event - event = PushNotification::PasswordResetEvent.new(user: current_user) - PushNotification::HttpPush.deliver(event) - end - - def create_event_and_notify_user_about_password_change - _event, disavowal_token = create_user_event_with_disavowal(:password_changed) - UserAlerts::AlertUserAboutPasswordChange.call(current_user, disavowal_token) - end - - def forbidden_passwords - current_user.email_addresses.flat_map do |email_address| - ForbiddenPasswords.new(email_address.email).call - end.uniq - end - - def user_password_params - params.require(:update_user_password_form).permit(:password, :password_confirmation) - end -end diff --git a/app/controllers/users/password_compromised_controller.rb b/app/controllers/users/password_compromised_controller.rb deleted file mode 100644 index c5457f1417f..00000000000 --- a/app/controllers/users/password_compromised_controller.rb +++ /dev/null @@ -1,54 +0,0 @@ -# frozen_string_literal: true - -module Users - class PasswordCompromisedController < ApplicationController - include PasswordConcern - before_action :confirm_two_factor_authenticated - before_action :verify_feature_toggle_on - - def show - session.delete(:redirect_to_change_password) - @update_user_password_form = UpdateUserPasswordForm.new(current_user) - @forbidden_passwords = forbidden_passwords - analytics.user_password_compromised_visited - end - - def update - @update_user_password_form = UpdateUserPasswordForm.new(current_user, user_session, true) - - result = @update_user_password_form.submit(user_password_params) - analytics.password_changed(**result) - if result.success? - handle_valid_password - else - handle_invalid_password - end - end - - def verify_feature_toggle_on - redirect_to after_sign_in_path_for(current_user) unless - FeatureManagement.check_password_enabled? - end - - def handle_valid_password - send_password_reset_risc_event - create_event_and_notify_user_about_password_change - # Changing the password hash terminates the warden session, and bypass_sign_in ensures - # that the user remains authenticated. - bypass_sign_in current_user - - flash[:info] = t('notices.password_changed') - if @update_user_password_form.personal_key.present? - user_session[:personal_key] = @update_user_password_form.personal_key - redirect_to manage_personal_key_url - else - redirect_to after_sign_in_path_for(current_user) - end - end - - def handle_invalid_password - @forbidden_passwords = forbidden_passwords - render :show - end - end -end diff --git a/app/controllers/users/passwords_controller.rb b/app/controllers/users/passwords_controller.rb index e63e5ae4f62..dfa7da8b99c 100644 --- a/app/controllers/users/passwords_controller.rb +++ b/app/controllers/users/passwords_controller.rb @@ -3,20 +3,25 @@ module Users class PasswordsController < ApplicationController include ReauthenticationRequiredConcern - include PasswordConcern before_action :confirm_two_factor_authenticated before_action :capture_password_if_pii_present_but_locked before_action :confirm_recently_authenticated_2fa def edit - analytics.edit_password_visit + @update_password_presenter = UpdatePasswordPresenter.new( + user: current_user, + required_password_change: required_password_change, + ) + analytics.edit_password_visit(required_password_change: required_password_change) @update_user_password_form = UpdateUserPasswordForm.new(current_user) - @forbidden_passwords = forbidden_passwords end def update - @update_user_password_form = UpdateUserPasswordForm.new(current_user, user_session) + @update_user_password_form = UpdateUserPasswordForm.new( + current_user, user_session, + required_password_change + ) result = @update_user_password_form.submit(user_password_params) @@ -38,7 +43,12 @@ def capture_password_if_pii_present_but_locked redirect_to capture_password_url end + def required_password_change + @required_password_change ||= session[:redirect_to_change_password] + end + def handle_valid_password + session.delete(:redirect_to_change_password) send_password_reset_risc_event create_event_and_notify_user_about_password_change # Changing the password hash terminates the warden session, and bypass_sign_in ensures @@ -50,7 +60,15 @@ def handle_valid_password user_session[:personal_key] = @update_user_password_form.personal_key redirect_to manage_personal_key_url else - redirect_to account_url + redirect_to account_or_after_sign_in_path + end + end + + def account_or_after_sign_in_path + if @required_password_change + after_sign_in_path_for(current_user) + else + account_path end end @@ -60,8 +78,25 @@ def handle_invalid_password # need to provide our custom forbidden passwords data that zxcvbn needs, # otherwise the JS will throw an exception and the password strength # meter will not appear. - @forbidden_passwords = forbidden_passwords + @update_password_presenter = UpdatePasswordPresenter.new( + user: current_user, + required_password_change: required_password_change, + ) render :edit end + + def send_password_reset_risc_event + event = PushNotification::PasswordResetEvent.new(user: current_user) + PushNotification::HttpPush.deliver(event) + end + + def create_event_and_notify_user_about_password_change + _event, disavowal_token = create_user_event_with_disavowal(:password_changed) + UserAlerts::AlertUserAboutPasswordChange.call(current_user, disavowal_token) + end + + def user_password_params + params.require(:update_user_password_form).permit(:password, :password_confirmation) + end end end diff --git a/app/forms/update_user_password_form.rb b/app/forms/update_user_password_form.rb index 213fda91422..e4662d195b9 100644 --- a/app/forms/update_user_password_form.rb +++ b/app/forms/update_user_password_form.rb @@ -6,10 +6,10 @@ class UpdateUserPasswordForm delegate :personal_key, to: :encryptor - def initialize(user, user_session = nil, original_password_compromised = false) + def initialize(user, user_session = nil, required_password_change = false) @user = user @user_session = user_session - @original_password_compromised = original_password_compromised + @required_password_change = required_password_change @validate_confirmation = true end @@ -23,7 +23,7 @@ def submit(params) private - attr_reader :user, :user_session + attr_reader :user, :user_session, :required_password_change def process_valid_submission user.update!(password: password) @@ -49,7 +49,7 @@ def extra_analytics_attributes active_profile_present: user.active_profile.present?, pending_profile_present: user.pending_profile.present?, user_id: user.uuid, - original_password_compromised: original_password_compromised, + required_password_change: required_password_change.present?, } end end diff --git a/app/presenters/update_password_presenter.rb b/app/presenters/update_password_presenter.rb new file mode 100644 index 00000000000..59cec9181cd --- /dev/null +++ b/app/presenters/update_password_presenter.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +class UpdatePasswordPresenter + attr_reader :user, :required_password_change + def initialize(user:, required_password_change: false) + @user = user + @required_password_change = required_password_change + end + + def forbidden_passwords + user.email_addresses.flat_map do |email_address| + ForbiddenPasswords.new(email_address.email).call + end.uniq + end + + def submit_text + if required_password_change + t('forms.passwords.edit.buttons.submit') + else + t('forms.buttons.submit.update') + end + end +end diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 699e56d39c0..13a434bdc85 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -392,9 +392,10 @@ def doc_auth_warning(message: nil, **extra) ) end + # @param [Boolean] required_password_change if user forced to change password # When a user views the edit password page - def edit_password_visit - track_event('Edit Password Page Visited') + def edit_password_visit(required_password_change: false) + track_event('Edit Password Page Visited', required_password_change: required_password_change) end # @param [Boolean] success diff --git a/app/views/users/passwords/edit.html.erb b/app/views/users/passwords/edit.html.erb index cadbc7353d0..8616fbb132d 100644 --- a/app/views/users/passwords/edit.html.erb +++ b/app/views/users/passwords/edit.html.erb @@ -1,17 +1,29 @@ <% self.title = t('titles.edit_info.password') %> +<% if update_password_presenter.required_password_change %> + <%= render AlertComponent.new( + type: :warning, + class: 'margin-bottom-4', + ) do + t('users.password_compromised.warning', app_name: APP_NAME) + end + %> +<% end %> + <%= render PageHeadingComponent.new.with_content(t('headings.edit_info.password')) %> -

- <%= t('instructions.password.info.lead_html', min_length: Devise.password_length.first) %> -

+<% unless update_password_presenter.required_password_change %> +

+ <%= t('instructions.password.info.lead_html', min_length: Devise.password_length.first) %> +

+<% end %> <%= simple_form_for(@update_user_password_form, url: manage_password_path, method: :patch) do |f| %> <%= f.error_notification %> <%= render PasswordConfirmationComponent.new( form: f, password_label: t('forms.passwords.edit.labels.password'), - forbidden_passwords: @forbidden_passwords, + forbidden_passwords: update_password_presenter.forbidden_passwords, field_options: { input_html: { aria: { describedby: 'password-description' }, @@ -23,4 +35,6 @@ <%= render 'shared/password_accordion' %> -<%= render 'shared/cancel', link: account_path %> +<% unless update_password_presenter.required_password_change %> + <%= render 'shared/cancel', link: account_path %> +<% end %> diff --git a/config/routes.rb b/config/routes.rb index e349cff646b..9b951a99434 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -295,8 +295,6 @@ get '/confirm_backup_codes' => 'users/backup_code_setup#confirm_backup_codes' get '/user_please_call' => 'users/please_call#show' - get '/user_password_compromised' => 'users/password_compromised#show' - post '/user_password_compromised' => 'users/password_compromised#update' post '/sign_up/create_password' => 'sign_up/passwords#create', as: :sign_up_create_password get '/sign_up/email/confirm' => 'sign_up/email_confirmations#create', diff --git a/spec/controllers/users/password_compromised_controller_spec.rb b/spec/controllers/users/password_compromised_controller_spec.rb deleted file mode 100644 index 23dfb453df4..00000000000 --- a/spec/controllers/users/password_compromised_controller_spec.rb +++ /dev/null @@ -1,127 +0,0 @@ -require 'rails_helper' - -RSpec.describe Users::PasswordCompromisedController, allowed_extra_analytics: [:*] do - describe '#show' do - let(:user) { create(:user) } - context 'check_password_enabled is not enabled' do - before do - allow(FeatureManagement).to receive(:check_password_enabled?).and_return(false) - stub_sign_in(user) - stub_analytics - end - - it 'redirects to account page' do - get :show - expect(response).to redirect_to account_url - end - end - - context 'check_password_enabled is enabled' do - before do - allow(FeatureManagement).to receive(:check_password_enabled?).and_return(true) - stub_sign_in(user) - stub_analytics - session[:redirect_to_change_password] = true - end - - it 'renders password compromised page' do - get :show - expect(response).to render_template(:show) - expect(session[:redirect_to_change_password]).to be_nil - end - - it 'logs analytics for password compromised visited' do - get :show - expect(@analytics).to have_logged_event(:user_password_compromised_visited) - end - end - end - - describe '#update' do - let(:user) { create(:user) } - context 'check_password_enabled is not enabled' do - before do - allow(FeatureManagement).to receive(:check_password_enabled?).and_return(false) - stub_sign_in(user) - stub_analytics - end - - it 'redirects to account page' do - post :update - expect(response).to redirect_to account_url - end - end - - context 'check_password_enabled is enabled' do - let(:password) { 'salty new password' } - let(:params) do - { - password: password, - password_confirmation: password, - } - end - before do - allow(FeatureManagement).to receive(:check_password_enabled?).and_return(true) - stub_sign_in(user) - stub_analytics - end - - context 'proper password form submission' do - it 'updates the user password and logs analytic event' do - allow(@analytics).to receive(:track_event) - patch :update, params: { update_user_password_form: params } - - expect(@analytics).to have_received(:track_event).with( - 'Password Changed', - success: true, - errors: {}, - error_details: nil, - pending_profile_present: false, - active_profile_present: false, - user_id: subject.current_user.uuid, - original_password_compromised: true, - ) - expect(response).to redirect_to account_url - expect(flash[:info]).to eq t('notices.password_changed') - end - - it 'sends email notifying user of password change' do - allow(@analytics).to receive(:track_event) - - patch :update, params: { update_user_password_form: params } - expect_delivered_email_count(1) - end - - it 'sends a security event' do - security_event = PushNotification::PasswordResetEvent.new(user: user) - expect(PushNotification::HttpPush).to receive(:deliver).with(security_event) - - patch :update, params: { update_user_password_form: params } - end - end - - context 'improper password form submission' do - let(:password) { 'new' } - let(:params) do - { - password: password, - password_confirmation: password, - } - end - - it 'does not create a password_changed user Event' do - stub_sign_in - - expect(controller).to_not receive(:create_user_event) - - patch :update, params: { update_user_password_form: params } - end - - it 'renders show page' do - patch :update, params: { update_user_password_form: params } - expect(response).to render_template(:show) - end - end - end - end -end diff --git a/spec/controllers/users/passwords_controller_spec.rb b/spec/controllers/users/passwords_controller_spec.rb index 7b1a4b8d8e6..a50ec50b5c8 100644 --- a/spec/controllers/users/passwords_controller_spec.rb +++ b/spec/controllers/users/passwords_controller_spec.rb @@ -11,6 +11,27 @@ get :edit expect(@analytics).to have_logged_event('Edit Password Page Visited') end + + context 'redirect_to_change_password is set to true' do + before do + stub_sign_in(user) + stub_analytics + session[:redirect_to_change_password] = true + end + + it 'renders password compromised with required_password_change set to true' do + get :edit + expect(response).to render_template(:edit) + end + + it 'logs analytics for password compromised visited' do + get :edit + expect(@analytics).to have_logged_event( + 'Edit Password Page Visited', + required_password_change: true, + ) + end + end end describe '#update' do @@ -34,6 +55,7 @@ pending_profile_present: false, active_profile_present: false, user_id: subject.current_user.uuid, + required_password_change: false, ) expect(response).to redirect_to account_url expect(flash[:info]).to eq t('notices.password_changed') @@ -110,6 +132,53 @@ subject: t('devise.mailer.password_updated.subject'), ) end + context 'redirect_to_change_password is set to true' do + let(:user) { create(:user) } + let(:password) { 'salty new password' } + let(:params) do + { + password: password, + password_confirmation: password, + } + end + before do + stub_sign_in(user) + stub_analytics + session[:redirect_to_change_password] = true + end + + it 'updates the user password and logs analytic event' do + allow(@analytics).to receive(:track_event) + patch :update, params: { update_user_password_form: params } + + expect(@analytics).to have_received(:track_event).with( + 'Password Changed', + success: true, + errors: {}, + error_details: nil, + pending_profile_present: false, + active_profile_present: false, + user_id: subject.current_user.uuid, + required_password_change: true, + ) + expect(response).to redirect_to account_url + expect(flash[:info]).to eq t('notices.password_changed') + end + + it 'sends email notifying user of password change' do + allow(@analytics).to receive(:track_event) + + patch :update, params: { update_user_password_form: params } + expect_delivered_email_count(1) + end + + it 'sends a security event' do + security_event = PushNotification::PasswordResetEvent.new(user: user) + expect(PushNotification::HttpPush).to receive(:deliver).with(security_event) + + patch :update, params: { update_user_password_form: params } + end + end end context 'form returns failure' do @@ -129,6 +198,54 @@ patch :update, params: { update_user_password_form: params } end + context 'redirect_to_change_password is set to true' do + let(:user) { create(:user) } + let(:password) { 'false' } + let(:params) do + { + password: password, + password_confirmation: 'false', + } + end + + before do + stub_sign_in(user) + stub_analytics + session[:redirect_to_change_password] = true + end + + it 'renders edit' do + allow(@analytics).to receive(:track_event) + patch :update, params: { update_user_password_form: params } + + expect(@analytics).to have_received(:track_event).with( + 'Password Changed', + success: false, + errors: { + password: [ + t( + 'errors.attributes.password.too_short.other', + count: Devise.password_length.first, + ), + ], + password_confirmation: [t( + 'errors.messages.too_short.other', + count: Devise.password_length.first, + )], + }, + error_details: { + password: { too_short: true }, + password_confirmation: { too_short: true }, + }, + pending_profile_present: false, + active_profile_present: false, + user_id: subject.current_user.uuid, + required_password_change: true, + ) + expect(response).to render_template(:edit) + end + end + context 'when password is too short' do before do stub_sign_in @@ -161,6 +278,7 @@ pending_profile_present: false, active_profile_present: false, user_id: subject.current_user.uuid, + required_password_change: false, ) expect(response).to render_template(:edit) end @@ -198,6 +316,7 @@ pending_profile_present: false, active_profile_present: false, user_id: subject.current_user.uuid, + required_password_change: false, ) expect(response).to render_template(:edit) end From ada95a2b750b4b41fef50fd833917dd14e665b27 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Mon, 1 Jul 2024 09:57:42 -0400 Subject: [PATCH 07/18] password compromised updates --- app/presenters/update_password_presenter.rb | 4 +-- .../users/password_compromised/show.html.erb | 24 --------------- spec/features/users/sign_in_spec.rb | 4 +-- .../update_password_presenter_spec.rb | 29 +++++++++++++++++++ 4 files changed, 33 insertions(+), 28 deletions(-) delete mode 100644 app/views/users/password_compromised/show.html.erb create mode 100644 spec/presenters/update_password_presenter_spec.rb diff --git a/app/presenters/update_password_presenter.rb b/app/presenters/update_password_presenter.rb index 59cec9181cd..d293b76559e 100644 --- a/app/presenters/update_password_presenter.rb +++ b/app/presenters/update_password_presenter.rb @@ -15,9 +15,9 @@ def forbidden_passwords def submit_text if required_password_change - t('forms.passwords.edit.buttons.submit') + I18n.t('forms.passwords.edit.buttons.submit') else - t('forms.buttons.submit.update') + I18n.t('forms.buttons.submit.update') end end end diff --git a/app/views/users/password_compromised/show.html.erb b/app/views/users/password_compromised/show.html.erb deleted file mode 100644 index e28854706d8..00000000000 --- a/app/views/users/password_compromised/show.html.erb +++ /dev/null @@ -1,24 +0,0 @@ -<% self.title = t('titles.edit_info.password') %> - -<%= render AlertComponent.new( - type: :warning, - class: 'margin-bottom-4', - ) do - t('users.password_compromised.warning', app_name: APP_NAME) - end -%> - -<%= render PageHeadingComponent.new.with_content(t('headings.edit_info.password')) %> - - -<%= simple_form_for(@update_user_password_form, url: user_password_compromised_path, method: :post) do |f| %> - <%= f.error_notification %> - <%= render PasswordConfirmationComponent.new( - form: f, - password_label: t('forms.passwords.edit.labels.password'), - forbidden_passwords: @forbidden_passwords, - ) %> - <%= f.submit t('forms.passwords.edit.buttons.submit'), class: 'display-block margin-top-5 margin-bottom-4' %> -<% end %> - -<%= render 'shared/password_accordion' %> diff --git a/spec/features/users/sign_in_spec.rb b/spec/features/users/sign_in_spec.rb index 6f7de6f2760..b67c78ed3c7 100644 --- a/spec/features/users/sign_in_spec.rb +++ b/spec/features/users/sign_in_spec.rb @@ -981,7 +981,7 @@ allow(IdentityConfig.store).to receive(:compromised_password_randomizer_threshold). and_return(2) end - it 'should bring user to compromised password page' do + it 'should bring user to manage password page with warning' do visit new_user_session_path fill_in_credentials_and_submit(user.email, user.password) fill_in_code_with_last_phone_otp @@ -990,7 +990,7 @@ expect(current_path).to eq user_password_compromised_path end - it 'should redirect user to account page after editing password' do + it 'should redirect user to after_sign_in_path after editing password' do visit new_user_session_path fill_in_credentials_and_submit(user.email, user.password) fill_in_code_with_last_phone_otp diff --git a/spec/presenters/update_password_presenter_spec.rb b/spec/presenters/update_password_presenter_spec.rb new file mode 100644 index 00000000000..af76bc5b1d2 --- /dev/null +++ b/spec/presenters/update_password_presenter_spec.rb @@ -0,0 +1,29 @@ +require 'rails_helper' + +RSpec.describe UpdatePasswordPresenter do + let(:view) { double(:view, link_to: '') } + let(:user) { create(:user) } + let(:required_password_change) { false } + let(:presenter) { described_class.new(user: user, required_password_change:) } + + describe '#submit_text' do + context 'required_password_change set to true' do + let(:required_password_change) { true } + it 'returns change password text' do + expect(presenter.submit_text).to eq t('forms.passwords.edit.buttons.submit') + end + end + + context 'required_password_change is set to false' do + it 'returns update text' do + expect(presenter.submit_text).to eq t('forms.buttons.submit.update') + end + end + end + + describe '#forbidden_passwords' do + it 'returns forbidden passwords for user' do + expect(presenter.forbidden_passwords).to include(user.email_addresses.first.email) + end + end +end From f0df22ebe98aadf2ecdfa51e491538160e943785 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Mon, 1 Jul 2024 12:21:01 -0400 Subject: [PATCH 08/18] fix lint and use global variable --- app/services/analytics_events.rb | 2 +- app/views/users/passwords/edit.html.erb | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 13a434bdc85..ae771f2ed4c 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -394,7 +394,7 @@ def doc_auth_warning(message: nil, **extra) # @param [Boolean] required_password_change if user forced to change password # When a user views the edit password page - def edit_password_visit(required_password_change: false) + def edit_password_visit(required_password_change: false, **extra) track_event('Edit Password Page Visited', required_password_change: required_password_change) end diff --git a/app/views/users/passwords/edit.html.erb b/app/views/users/passwords/edit.html.erb index 8616fbb132d..e6d349ef561 100644 --- a/app/views/users/passwords/edit.html.erb +++ b/app/views/users/passwords/edit.html.erb @@ -1,6 +1,6 @@ <% self.title = t('titles.edit_info.password') %> -<% if update_password_presenter.required_password_change %> +<% if @update_password_presenter.required_password_change %> <%= render AlertComponent.new( type: :warning, class: 'margin-bottom-4', @@ -23,7 +23,7 @@ <%= render PasswordConfirmationComponent.new( form: f, password_label: t('forms.passwords.edit.labels.password'), - forbidden_passwords: update_password_presenter.forbidden_passwords, + forbidden_passwords: @update_password_presenter.forbidden_passwords, field_options: { input_html: { aria: { describedby: 'password-description' }, @@ -35,6 +35,6 @@ <%= render 'shared/password_accordion' %> -<% unless update_password_presenter.required_password_change %> +<% unless @update_password_presenter.required_password_change %> <%= render 'shared/cancel', link: account_path %> <% end %> From d9dfe064b95ad7b34b1ddd3bb5f12df18c64aef9 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Mon, 1 Jul 2024 13:11:53 -0400 Subject: [PATCH 09/18] update edit spec and global variable --- app/services/analytics_events.rb | 2 +- app/views/users/passwords/edit.html.erb | 4 ++-- .../users/passwords/edit.html.erb_spec.rb | 22 +++++++++++++++++++ 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index ae771f2ed4c..fc68e303d6c 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -394,7 +394,7 @@ def doc_auth_warning(message: nil, **extra) # @param [Boolean] required_password_change if user forced to change password # When a user views the edit password page - def edit_password_visit(required_password_change: false, **extra) + def edit_password_visit(required_password_change: false, **_extra) track_event('Edit Password Page Visited', required_password_change: required_password_change) end diff --git a/app/views/users/passwords/edit.html.erb b/app/views/users/passwords/edit.html.erb index e6d349ef561..a0de5391b4b 100644 --- a/app/views/users/passwords/edit.html.erb +++ b/app/views/users/passwords/edit.html.erb @@ -12,7 +12,7 @@ <%= render PageHeadingComponent.new.with_content(t('headings.edit_info.password')) %> -<% unless update_password_presenter.required_password_change %> +<% unless @update_password_presenter.required_password_change %>

<%= t('instructions.password.info.lead_html', min_length: Devise.password_length.first) %>

@@ -30,7 +30,7 @@ }, }, ) %> - <%= f.submit t('forms.buttons.submit.update'), class: 'display-block margin-top-5 margin-bottom-4' %> + <%= f.submit @update_password_presenter.submit_text, class: 'display-block margin-top-5 margin-bottom-4' %> <% end %> <%= render 'shared/password_accordion' %> diff --git a/spec/views/users/passwords/edit.html.erb_spec.rb b/spec/views/users/passwords/edit.html.erb_spec.rb index 0fafe9d9916..56a744d17e8 100644 --- a/spec/views/users/passwords/edit.html.erb_spec.rb +++ b/spec/views/users/passwords/edit.html.erb_spec.rb @@ -1,10 +1,16 @@ require 'rails_helper' RSpec.describe 'users/passwords/edit.html.erb' do + let(:required_password_change) { false } + before do user = User.new allow(view).to receive(:current_user).and_return(user) @update_user_password_form = UpdateUserPasswordForm.new(user) + @update_password_presenter = UpdatePasswordPresenter.new( + user: user, + required_password_change: required_password_change, + ) end it 'has a localized title' do @@ -35,4 +41,20 @@ ), ) end + + context 'required password change' do + let(:required_password_change) { true } + + it 'has alert component content for required password change' do + render + + expect(rendered).to have_content t('users.password_compromised.warning', app_name: APP_NAME) + end + + it 'has a submit content for submission page' do + render + + expect(rendered).to have_content I18n.t('forms.passwords.edit.buttons.submit') + end + end end From ffba90b069ed393600493e987c044cb197f7cbe7 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Mon, 1 Jul 2024 14:06:44 -0400 Subject: [PATCH 10/18] fix the sign in spec --- spec/features/users/sign_in_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/users/sign_in_spec.rb b/spec/features/users/sign_in_spec.rb index b67c78ed3c7..4e2a65a283b 100644 --- a/spec/features/users/sign_in_spec.rb +++ b/spec/features/users/sign_in_spec.rb @@ -987,7 +987,7 @@ fill_in_code_with_last_phone_otp click_submit_default - expect(current_path).to eq user_password_compromised_path + expect(current_path).to eq manage_password_path end it 'should redirect user to after_sign_in_path after editing password' do From 93911fea118d961265cdc3502928250378687e43 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Mon, 1 Jul 2024 14:29:39 -0400 Subject: [PATCH 11/18] fix form spec --- spec/forms/update_user_password_form_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/forms/update_user_password_form_spec.rb b/spec/forms/update_user_password_form_spec.rb index 8125285f912..1ba6a2ed147 100644 --- a/spec/forms/update_user_password_form_spec.rb +++ b/spec/forms/update_user_password_form_spec.rb @@ -57,6 +57,7 @@ active_profile_present: false, pending_profile_present: false, user_id: user.uuid, + required_password_change: false, ) end From 14fdbbe8be48ac900d1cd7154f771bc871a40938 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Tue, 2 Jul 2024 13:57:37 -0400 Subject: [PATCH 12/18] address comments, update test to better redirect --- app/controllers/users/passwords_controller.rb | 24 +++++++------------ app/controllers/users/sessions_controller.rb | 7 +++--- app/presenters/update_password_presenter.rb | 2 ++ app/services/analytics_events.rb | 8 +++++-- app/views/users/passwords/edit.html.erb | 6 ++--- spec/features/users/sign_in_spec.rb | 12 ++++++---- .../users/passwords/edit.html.erb_spec.rb | 5 ++++ 7 files changed, 36 insertions(+), 28 deletions(-) diff --git a/app/controllers/users/passwords_controller.rb b/app/controllers/users/passwords_controller.rb index dfa7da8b99c..6c2a853f94a 100644 --- a/app/controllers/users/passwords_controller.rb +++ b/app/controllers/users/passwords_controller.rb @@ -11,16 +11,16 @@ class PasswordsController < ApplicationController def edit @update_password_presenter = UpdatePasswordPresenter.new( user: current_user, - required_password_change: required_password_change, + required_password_change: required_password_change?, ) - analytics.edit_password_visit(required_password_change: required_password_change) + analytics.edit_password_visit(required_password_change: required_password_change?) @update_user_password_form = UpdateUserPasswordForm.new(current_user) end def update @update_user_password_form = UpdateUserPasswordForm.new( current_user, user_session, - required_password_change + required_password_change? ) result = @update_user_password_form.submit(user_password_params) @@ -43,12 +43,11 @@ def capture_password_if_pii_present_but_locked redirect_to capture_password_url end - def required_password_change - @required_password_change ||= session[:redirect_to_change_password] + def required_password_change? + session[:redirect_to_change_password] == true end def handle_valid_password - session.delete(:redirect_to_change_password) send_password_reset_risc_event create_event_and_notify_user_about_password_change # Changing the password hash terminates the warden session, and bypass_sign_in ensures @@ -59,16 +58,11 @@ def handle_valid_password if @update_user_password_form.personal_key.present? user_session[:personal_key] = @update_user_password_form.personal_key redirect_to manage_personal_key_url + elsif required_password_change? + session.delete(:redirect_to_change_password) + redirect_to after_sign_in_path_for(current_user) else - redirect_to account_or_after_sign_in_path - end - end - - def account_or_after_sign_in_path - if @required_password_change - after_sign_in_path_for(current_user) - else - account_path + redirect_to account_path end end diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index 83771b9d6be..a8422bb2ccd 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -229,11 +229,10 @@ def sign_in_params end def check_password_compromised - return if current_user.password_compromised_checked_at.present? || - !eligible_for_password_lookup? + # return if current_user.password_compromised_checked_at.present? || + # !eligible_for_password_lookup? - session[:redirect_to_change_password] = - PwnedPasswords::LookupPassword.call(auth_params[:password]) + session[:redirect_to_change_password] = true update_user_password_compromised_checked_at end diff --git a/app/presenters/update_password_presenter.rb b/app/presenters/update_password_presenter.rb index d293b76559e..4c8c8effe85 100644 --- a/app/presenters/update_password_presenter.rb +++ b/app/presenters/update_password_presenter.rb @@ -2,6 +2,8 @@ class UpdatePasswordPresenter attr_reader :user, :required_password_change + + alias_method :required_password_change?, :required_password_change def initialize(user:, required_password_change: false) @user = user @required_password_change = required_password_change diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index fc68e303d6c..c5b11d952cf 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -394,8 +394,12 @@ def doc_auth_warning(message: nil, **extra) # @param [Boolean] required_password_change if user forced to change password # When a user views the edit password page - def edit_password_visit(required_password_change: false, **_extra) - track_event('Edit Password Page Visited', required_password_change: required_password_change) + def edit_password_visit(required_password_change: false, **extra) + track_event( + 'Edit Password Page Visited', + required_password_change: required_password_change, + **extra, + ) end # @param [Boolean] success diff --git a/app/views/users/passwords/edit.html.erb b/app/views/users/passwords/edit.html.erb index a0de5391b4b..60adec37d67 100644 --- a/app/views/users/passwords/edit.html.erb +++ b/app/views/users/passwords/edit.html.erb @@ -1,6 +1,6 @@ <% self.title = t('titles.edit_info.password') %> -<% if @update_password_presenter.required_password_change %> +<% if @update_password_presenter.required_password_change? %> <%= render AlertComponent.new( type: :warning, class: 'margin-bottom-4', @@ -12,7 +12,7 @@ <%= render PageHeadingComponent.new.with_content(t('headings.edit_info.password')) %> -<% unless @update_password_presenter.required_password_change %> +<% unless @update_password_presenter.required_password_change? %>

<%= t('instructions.password.info.lead_html', min_length: Devise.password_length.first) %>

@@ -35,6 +35,6 @@ <%= render 'shared/password_accordion' %> -<% unless @update_password_presenter.required_password_change %> +<% unless @update_password_presenter.required_password_change? %> <%= render 'shared/cancel', link: account_path %> <% end %> diff --git a/spec/features/users/sign_in_spec.rb b/spec/features/users/sign_in_spec.rb index 4e2a65a283b..bcb3c1a0f53 100644 --- a/spec/features/users/sign_in_spec.rb +++ b/spec/features/users/sign_in_spec.rb @@ -982,7 +982,7 @@ and_return(2) end it 'should bring user to manage password page with warning' do - visit new_user_session_path + visit_idp_from_sp_with_ial1(:oidc) fill_in_credentials_and_submit(user.email, user.password) fill_in_code_with_last_phone_otp click_submit_default @@ -991,17 +991,21 @@ end it 'should redirect user to after_sign_in_path after editing password' do - visit new_user_session_path + visit_idp_from_sp_with_ial1(:oidc) fill_in_credentials_and_submit(user.email, user.password) fill_in_code_with_last_phone_otp click_submit_default - password = 'sugary pickles' + expect(current_path).to eq manage_password_path + + password = 'salty pickles' fill_in t('forms.passwords.edit.labels.password'), with: password fill_in t('components.password_confirmation.confirm_label'), with: password click_button t('forms.passwords.edit.buttons.submit') - expect(current_path).to eq account_path + click_agree_and_continue + + expect(oidc_redirect_url).to start_with('http://localhost:7654/auth/result') end end diff --git a/spec/views/users/passwords/edit.html.erb_spec.rb b/spec/views/users/passwords/edit.html.erb_spec.rb index 56a744d17e8..d0374744162 100644 --- a/spec/views/users/passwords/edit.html.erb_spec.rb +++ b/spec/views/users/passwords/edit.html.erb_spec.rb @@ -56,5 +56,10 @@ expect(rendered).to have_content I18n.t('forms.passwords.edit.buttons.submit') end + + it 'does not have cancel content for submission page' do + render + expect(rendered).to_not have_content(t('links.cancel')) + end end end From a5893a0815d9ea5bf65c4d48037a0531375fbe36 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Tue, 2 Jul 2024 16:05:42 -0400 Subject: [PATCH 13/18] remove former check --- app/controllers/users/sessions_controller.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index a8422bb2ccd..83771b9d6be 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -229,10 +229,11 @@ def sign_in_params end def check_password_compromised - # return if current_user.password_compromised_checked_at.present? || - # !eligible_for_password_lookup? + return if current_user.password_compromised_checked_at.present? || + !eligible_for_password_lookup? - session[:redirect_to_change_password] = true + session[:redirect_to_change_password] = + PwnedPasswords::LookupPassword.call(auth_params[:password]) update_user_password_compromised_checked_at end From 96a02e060b5cbb90a9909da4d80ce6e98332e8df Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Tue, 2 Jul 2024 17:14:49 -0400 Subject: [PATCH 14/18] make sure invalid password method is correct --- app/controllers/users/passwords_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/users/passwords_controller.rb b/app/controllers/users/passwords_controller.rb index 6c2a853f94a..61914b35bf2 100644 --- a/app/controllers/users/passwords_controller.rb +++ b/app/controllers/users/passwords_controller.rb @@ -74,7 +74,7 @@ def handle_invalid_password # meter will not appear. @update_password_presenter = UpdatePasswordPresenter.new( user: current_user, - required_password_change: required_password_change, + required_password_change: required_password_change?, ) render :edit end From 13cfe98a713ffa28a19a2a62591a61742dbfd5d4 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Wed, 3 Jul 2024 10:20:22 -0400 Subject: [PATCH 15/18] Update to make aria described conditional --- app/presenters/update_password_presenter.rb | 11 +++++++++- app/views/users/passwords/edit.html.erb | 6 +---- .../update_password_presenter_spec.rb | 22 +++++++++++++++++++ .../users/passwords/edit.html.erb_spec.rb | 12 ++++++++++ 4 files changed, 45 insertions(+), 6 deletions(-) diff --git a/app/presenters/update_password_presenter.rb b/app/presenters/update_password_presenter.rb index 4c8c8effe85..e2f135aebd7 100644 --- a/app/presenters/update_password_presenter.rb +++ b/app/presenters/update_password_presenter.rb @@ -15,8 +15,17 @@ def forbidden_passwords end.uniq end + def aria_described_by_if_eligible + return {} if required_password_change? + { + input_html: { + aria: { describedby: 'password-description' }, + }, + } + end + def submit_text - if required_password_change + if required_password_change? I18n.t('forms.passwords.edit.buttons.submit') else I18n.t('forms.buttons.submit.update') diff --git a/app/views/users/passwords/edit.html.erb b/app/views/users/passwords/edit.html.erb index 60adec37d67..5b9c5de7bd9 100644 --- a/app/views/users/passwords/edit.html.erb +++ b/app/views/users/passwords/edit.html.erb @@ -24,11 +24,7 @@ form: f, password_label: t('forms.passwords.edit.labels.password'), forbidden_passwords: @update_password_presenter.forbidden_passwords, - field_options: { - input_html: { - aria: { describedby: 'password-description' }, - }, - }, + field_options: @update_password_presenter.aria_described_by_if_eligible, ) %> <%= f.submit @update_password_presenter.submit_text, class: 'display-block margin-top-5 margin-bottom-4' %> <% end %> diff --git a/spec/presenters/update_password_presenter_spec.rb b/spec/presenters/update_password_presenter_spec.rb index af76bc5b1d2..0389b1e517f 100644 --- a/spec/presenters/update_password_presenter_spec.rb +++ b/spec/presenters/update_password_presenter_spec.rb @@ -26,4 +26,26 @@ expect(presenter.forbidden_passwords).to include(user.email_addresses.first.email) end end + + describe '#aria_described_by_if_eligible' do + context 'required_password_change set to true' do + let(:required_password_change) { true } + + it 'returns empty hash' do + expect(presenter.aria_described_by_if_eligible).to be_falsey + end + end + + context 'required_password_change set to false' do + it 'should return has with aria-described' do + expect(presenter.aria_described_by_if_eligible).to eq( + { + input_html: { + aria: { describedby: 'password-description' }, + }, + }, + ) + end + end + end end diff --git a/spec/views/users/passwords/edit.html.erb_spec.rb b/spec/views/users/passwords/edit.html.erb_spec.rb index d0374744162..1a16c07b557 100644 --- a/spec/views/users/passwords/edit.html.erb_spec.rb +++ b/spec/views/users/passwords/edit.html.erb_spec.rb @@ -42,6 +42,12 @@ ) end + it 'has aria described by' do + render + + expect(rendered).to have_selector('[aria-describedby="password-description"]') + end + context 'required password change' do let(:required_password_change) { true } @@ -61,5 +67,11 @@ render expect(rendered).to_not have_content(t('links.cancel')) end + + it 'aria described by is blank' do + render + + expect(rendered).to_not have_selector('[aria-describedby="password-description"]') + end end end From 6baa4263c9c58be2053a74affa0fd4b03c897d87 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Mon, 8 Jul 2024 10:11:18 -0400 Subject: [PATCH 16/18] update specs and use hash format for attributes instead of positional arguments --- .../sign_up/passwords_controller.rb | 2 +- app/controllers/users/passwords_controller.rb | 7 ++++--- .../users/reset_passwords_controller.rb | 4 ++-- app/forms/password_form.rb | 2 +- app/forms/reset_password_form.rb | 2 +- app/forms/update_user_password_form.rb | 4 ++-- spec/forms/password_form_spec.rb | 6 +++--- spec/forms/reset_password_form_spec.rb | 20 +++++++++---------- spec/forms/update_user_password_form_spec.rb | 2 +- .../update_password_presenter_spec.rb | 2 +- .../shared_examples/password_strength.rb | 10 +++++----- .../devise/passwords/edit.html.erb_spec.rb | 2 +- .../sign_up/passwords/new.html.erb_spec.rb | 2 +- .../users/passwords/edit.html.erb_spec.rb | 2 +- 14 files changed, 34 insertions(+), 33 deletions(-) diff --git a/app/controllers/sign_up/passwords_controller.rb b/app/controllers/sign_up/passwords_controller.rb index bc46df04310..78a123a7319 100644 --- a/app/controllers/sign_up/passwords_controller.rb +++ b/app/controllers/sign_up/passwords_controller.rb @@ -63,7 +63,7 @@ def process_successful_password_creation end def password_form - @password_form ||= PasswordForm.new(@user) + @password_form ||= PasswordForm.new(user: @user) end def process_unsuccessful_password_creation diff --git a/app/controllers/users/passwords_controller.rb b/app/controllers/users/passwords_controller.rb index 61914b35bf2..1667142e54e 100644 --- a/app/controllers/users/passwords_controller.rb +++ b/app/controllers/users/passwords_controller.rb @@ -14,13 +14,14 @@ def edit required_password_change: required_password_change?, ) analytics.edit_password_visit(required_password_change: required_password_change?) - @update_user_password_form = UpdateUserPasswordForm.new(current_user) + @update_user_password_form = UpdateUserPasswordForm.new(user: current_user) end def update @update_user_password_form = UpdateUserPasswordForm.new( - current_user, user_session, - required_password_change? + user: current_user, + user_session: user_session, + required_password_change: required_password_change?, ) result = @update_user_password_form.submit(user_password_params) diff --git a/app/controllers/users/reset_passwords_controller.rb b/app/controllers/users/reset_passwords_controller.rb index 6735cf17fb8..180e8239821 100644 --- a/app/controllers/users/reset_passwords_controller.rb +++ b/app/controllers/users/reset_passwords_controller.rb @@ -32,7 +32,7 @@ def edit analytics.password_reset_token(**result.to_h) if result.success? - @reset_password_form = ResetPasswordForm.new(build_user) + @reset_password_form = ResetPasswordForm.new(user: build_user) @forbidden_passwords = forbidden_passwords(token_user.email_addresses) else handle_invalid_or_expired_token(result) @@ -43,7 +43,7 @@ def edit # PUT /resource/password def update self.resource = user_matching_token(user_params[:reset_password_token]) - @reset_password_form = ResetPasswordForm.new(resource) + @reset_password_form = ResetPasswordForm.new(user: resource) result = @reset_password_form.submit(user_params) diff --git a/app/forms/password_form.rb b/app/forms/password_form.rb index 897923d0038..a0dcfdc99d3 100644 --- a/app/forms/password_form.rb +++ b/app/forms/password_form.rb @@ -4,7 +4,7 @@ class PasswordForm include ActiveModel::Model include FormPasswordValidator - def initialize(user) + def initialize(user:) @user = user @validate_confirmation = true end diff --git a/app/forms/reset_password_form.rb b/app/forms/reset_password_form.rb index 6718fea8da3..adcf084577e 100644 --- a/app/forms/reset_password_form.rb +++ b/app/forms/reset_password_form.rb @@ -8,7 +8,7 @@ class ResetPasswordForm validate :valid_token - def initialize(user) + def initialize(user:) @user = user @reset_password_token = @user.reset_password_token @validate_confirmation = true diff --git a/app/forms/update_user_password_form.rb b/app/forms/update_user_password_form.rb index e4662d195b9..ba1eee0f8e3 100644 --- a/app/forms/update_user_password_form.rb +++ b/app/forms/update_user_password_form.rb @@ -6,7 +6,7 @@ class UpdateUserPasswordForm delegate :personal_key, to: :encryptor - def initialize(user, user_session = nil, required_password_change = false) + def initialize(user:, user_session: nil, required_password_change: false) @user = user @user_session = user_session @required_password_change = required_password_change @@ -49,7 +49,7 @@ def extra_analytics_attributes active_profile_present: user.active_profile.present?, pending_profile_present: user.pending_profile.present?, user_id: user.uuid, - required_password_change: required_password_change.present?, + required_password_change: required_password_change, } end end diff --git a/spec/forms/password_form_spec.rb b/spec/forms/password_form_spec.rb index 0003a5f052f..74e76e5bffe 100644 --- a/spec/forms/password_form_spec.rb +++ b/spec/forms/password_form_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' RSpec.describe PasswordForm, type: :model do - subject(:form) { described_class.new(user) } + subject(:form) { described_class.new(user: user) } let(:user) { build_stubbed(:user, uuid: '123') } let(:password) { 'Valid Password!' } let(:password_confirmation) { password } @@ -33,7 +33,7 @@ end context 'with password confirmation' do - subject(:form) { described_class.new(user) } + subject(:form) { described_class.new(user: user) } let(:password_confirmation) { password } @@ -63,7 +63,7 @@ end context 'with password confirmation' do - subject(:form) { described_class.new(user) } + subject(:form) { described_class.new(user: user) } context 'when the passwords are invalid' do let(:password_confirmation) { password } diff --git a/spec/forms/reset_password_form_spec.rb b/spec/forms/reset_password_form_spec.rb index 2600e6ec59f..e2bafd0b332 100644 --- a/spec/forms/reset_password_form_spec.rb +++ b/spec/forms/reset_password_form_spec.rb @@ -20,7 +20,7 @@ user = build_stubbed(:user, uuid: '123') allow(user).to receive(:reset_password_period_valid?).and_return(false) - form = ResetPasswordForm.new(user) + form = ResetPasswordForm.new(user: user) password = 'valid password' @@ -47,7 +47,7 @@ user = build_stubbed(:user, uuid: '123') allow(user).to receive(:reset_password_period_valid?).and_return(true) - form = ResetPasswordForm.new(user) + form = ResetPasswordForm.new(user: user) password = 'invalid' @@ -81,7 +81,7 @@ user = create(:user, uuid: '123') allow(user).to receive(:reset_password_period_valid?).and_return(true) - form = ResetPasswordForm.new(user) + form = ResetPasswordForm.new(user: user) password = 'valid password' result = nil @@ -108,7 +108,7 @@ user = build_stubbed(:user, uuid: '123') allow(user).to receive(:reset_password_period_valid?).and_return(false) - form = ResetPasswordForm.new(user) + form = ResetPasswordForm.new(user: user) password = 'short' @@ -143,7 +143,7 @@ it 'returns a hash with errors' do user = User.new - form = ResetPasswordForm.new(user) + form = ResetPasswordForm.new(user: user) errors = { password: [ t('errors.attributes.password.too_short.other', count: Devise.password_length.first), @@ -178,7 +178,7 @@ user = profile.user user.update(reset_password_sent_at: Time.zone.now) - form = ResetPasswordForm.new(user) + form = ResetPasswordForm.new(user: user) result = form.submit(params) @@ -193,7 +193,7 @@ user = create(:user) user.update(reset_password_sent_at: Time.zone.now) - form = ResetPasswordForm.new(user) + form = ResetPasswordForm.new(user: user) result = form.submit(params) @@ -208,7 +208,7 @@ user = profile.user user.update(reset_password_sent_at: Time.zone.now) - form = ResetPasswordForm.new(user) + form = ResetPasswordForm.new(user: user) result = form.submit(params) expect(result.success?).to eq(true) @@ -224,7 +224,7 @@ user = create(:user) user.update(reset_password_sent_at: Time.zone.now) - form = ResetPasswordForm.new(user) + form = ResetPasswordForm.new(user: user) result = form.submit(params) @@ -244,7 +244,7 @@ confirmed_at: Time.zone.now ) - form = ResetPasswordForm.new(user) + form = ResetPasswordForm.new(user: user) result = form.submit(params) diff --git a/spec/forms/update_user_password_form_spec.rb b/spec/forms/update_user_password_form_spec.rb index 1ba6a2ed147..d9a01f90637 100644 --- a/spec/forms/update_user_password_form_spec.rb +++ b/spec/forms/update_user_password_form_spec.rb @@ -11,7 +11,7 @@ } end let(:subject) do - UpdateUserPasswordForm.new(user, user_session) + UpdateUserPasswordForm.new(user: user, user_session: user_session) end it_behaves_like 'password validation' diff --git a/spec/presenters/update_password_presenter_spec.rb b/spec/presenters/update_password_presenter_spec.rb index 0389b1e517f..ee28cdc163d 100644 --- a/spec/presenters/update_password_presenter_spec.rb +++ b/spec/presenters/update_password_presenter_spec.rb @@ -32,7 +32,7 @@ let(:required_password_change) { true } it 'returns empty hash' do - expect(presenter.aria_described_by_if_eligible).to be_falsey + expect(presenter.aria_described_by_if_eligible).to be_empty end end diff --git a/spec/support/shared_examples/password_strength.rb b/spec/support/shared_examples/password_strength.rb index 311d44f0bd5..261cf48dbd7 100644 --- a/spec/support/shared_examples/password_strength.rb +++ b/spec/support/shared_examples/password_strength.rb @@ -2,7 +2,7 @@ it 'does not allow a password that is common and/or needs more words' do user = build_stubbed(:user, email: 'test@test.com', uuid: '123') allow(user).to receive(:reset_password_period_valid?).and_return(true) - form = form_class.constantize.new(user) + form = form_class.constantize.new(user: user) password = 'password foo' errors = { password: ['Your password is not strong enough.' \ @@ -24,7 +24,7 @@ it 'does not allow a password that needs more words' do user = build_stubbed(:user, email: 'test@test.com', uuid: '123') allow(user).to receive(:reset_password_period_valid?).and_return(true) - form = form_class.constantize.new(user) + form = form_class.constantize.new(user: user) password = 'benevolentman' errors = { password: ['Your password is not strong enough.' \ @@ -45,7 +45,7 @@ it 'does not allow a password equal to a word from the user email' do user = build_stubbed(:user, email: 'janedoelongname@example.com', uuid: '123') allow(user).to receive(:reset_password_period_valid?).and_return(true) - form = form_class.constantize.new(user) + form = form_class.constantize.new(user: user) password = 'janedoelongname' errors = { password: ['Your password is not strong enough.' \ @@ -66,7 +66,7 @@ it 'does not allow a password that is the user email' do user = build_stubbed(:user, email: 'custom@benevolent.com', uuid: '123') allow(user).to receive(:reset_password_period_valid?).and_return(true) - form = form_class.constantize.new(user) + form = form_class.constantize.new(user: user) password = 'custom@benevolent.com' errors = { password: ['Your password is not strong enough.' \ @@ -86,7 +86,7 @@ it 'does not allow a password that does not have the minimum number of graphemes' do user = build_stubbed(:user, email: 'custom@example.com', uuid: '123') allow(user).to receive(:reset_password_period_valid?).and_return(true) - form = form_class.constantize.new(user) + form = form_class.constantize.new(user: user) password = 'a7K!hf🇺🇸🇺🇸🇺🇸' errors = { password: [t('errors.attributes.password.too_short', count: 12)], diff --git a/spec/views/devise/passwords/edit.html.erb_spec.rb b/spec/views/devise/passwords/edit.html.erb_spec.rb index 89fd751d9d2..d25cfbb5569 100644 --- a/spec/views/devise/passwords/edit.html.erb_spec.rb +++ b/spec/views/devise/passwords/edit.html.erb_spec.rb @@ -3,7 +3,7 @@ RSpec.describe 'devise/passwords/edit.html.erb' do before do user = build_stubbed(:user, :fully_registered) - @reset_password_form = ResetPasswordForm.new(user) + @reset_password_form = ResetPasswordForm.new(user: user) end it 'has a localized title' do diff --git a/spec/views/sign_up/passwords/new.html.erb_spec.rb b/spec/views/sign_up/passwords/new.html.erb_spec.rb index 7131adce80b..9c240c33efe 100644 --- a/spec/views/sign_up/passwords/new.html.erb_spec.rb +++ b/spec/views/sign_up/passwords/new.html.erb_spec.rb @@ -9,7 +9,7 @@ allow(view).to receive(:request_id).and_return(nil) @email_address = user.email_addresses.first - @password_form = PasswordForm.new(user) + @password_form = PasswordForm.new(user: user) render end diff --git a/spec/views/users/passwords/edit.html.erb_spec.rb b/spec/views/users/passwords/edit.html.erb_spec.rb index 1a16c07b557..76eace3e8db 100644 --- a/spec/views/users/passwords/edit.html.erb_spec.rb +++ b/spec/views/users/passwords/edit.html.erb_spec.rb @@ -6,7 +6,7 @@ before do user = User.new allow(view).to receive(:current_user).and_return(user) - @update_user_password_form = UpdateUserPasswordForm.new(user) + @update_user_password_form = UpdateUserPasswordForm.new(user: user) @update_password_presenter = UpdatePasswordPresenter.new( user: user, required_password_change: required_password_change, From a5a68a3dec3b9bd15beb9446dda4728f0f74d99f Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Mon, 8 Jul 2024 10:51:53 -0400 Subject: [PATCH 17/18] fix reset password form spec --- spec/forms/reset_password_form_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/forms/reset_password_form_spec.rb b/spec/forms/reset_password_form_spec.rb index e2bafd0b332..8873e3b8856 100644 --- a/spec/forms/reset_password_form_spec.rb +++ b/spec/forms/reset_password_form_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' RSpec.describe ResetPasswordForm, type: :model do - subject { ResetPasswordForm.new(build_stubbed(:user, uuid: '123')) } + subject { ResetPasswordForm.new(user: build_stubbed(:user, uuid: '123')) } let(:password) { 'a good and powerful password' } let(:password_confirmation) { password } From db4182d57aa6ddfe3244944462fb06b7ae7f4c23 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Mon, 29 Jul 2024 09:10:48 -0400 Subject: [PATCH 18/18] remove conditional for password strength description --- app/views/users/passwords/edit.html.erb | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/app/views/users/passwords/edit.html.erb b/app/views/users/passwords/edit.html.erb index 5b9c5de7bd9..67733f3c156 100644 --- a/app/views/users/passwords/edit.html.erb +++ b/app/views/users/passwords/edit.html.erb @@ -12,11 +12,9 @@ <%= render PageHeadingComponent.new.with_content(t('headings.edit_info.password')) %> -<% unless @update_password_presenter.required_password_change? %> -

- <%= t('instructions.password.info.lead_html', min_length: Devise.password_length.first) %> -

-<% end %> +

+ <%= t('instructions.password.info.lead_html', min_length: Devise.password_length.first) %> +

<%= simple_form_for(@update_user_password_form, url: manage_password_path, method: :patch) do |f| %> <%= f.error_notification %>