From a419d8c6959db8727c1ff0c7ac29b4c347d6fc8c Mon Sep 17 00:00:00 2001 From: Andrew Duthie <1779930+aduth@users.noreply.github.com> Date: Thu, 31 Oct 2024 07:40:47 -0400 Subject: [PATCH] Log if password matches current for password reset event (#11423) * Log password changed * Document analytics property * Add spec coverage for password_different * changelog: Internal, Analytics, Add logging property for unchanged password reset of verified account * Typo: Profile deactivated * Invert password_different to password_matches_existing Co-authored-by: Zach Margolis * Refactor form specs for updated expectations * Add form test case for password_matches_existing * A/B test subset of password resets for logging * Remove inaccurate route verb Co-authored-by: Zach Margolis --------- Co-authored-by: Zach Margolis --- .../users/reset_passwords_controller.rb | 11 +- app/forms/reset_password_form.rb | 8 +- app/services/analytics_events.rb | 3 + config/application.yml.default | 1 + config/initializers/ab_tests.rb | 6 + lib/identity_config.rb | 1 + .../users/reset_passwords_controller_spec.rb | 54 ++++ spec/forms/reset_password_form_spec.rb | 264 ++++++++---------- 8 files changed, 193 insertions(+), 155 deletions(-) diff --git a/app/controllers/users/reset_passwords_controller.rb b/app/controllers/users/reset_passwords_controller.rb index 180e8239821..47fb962aac9 100644 --- a/app/controllers/users/reset_passwords_controller.rb +++ b/app/controllers/users/reset_passwords_controller.rb @@ -3,6 +3,8 @@ module Users class ResetPasswordsController < Devise::PasswordsController include AuthorizationCountConcern + include AbTestingConcern + before_action :store_sp_metadata_in_session, only: [:edit] before_action :store_token_in_session, only: [:edit] @@ -40,10 +42,15 @@ def edit end end - # PUT /resource/password def update self.resource = user_matching_token(user_params[:reset_password_token]) - @reset_password_form = ResetPasswordForm.new(user: resource) + @reset_password_form = ResetPasswordForm.new( + user: resource, + log_password_matches_existing: ab_test_bucket( + :LOG_PASSWORD_RESET_MATCHES_EXISTING, + user: resource, + ) == :log, + ) result = @reset_password_form.submit(user_params) diff --git a/app/forms/reset_password_form.rb b/app/forms/reset_password_form.rb index adcf084577e..35d8ff890d1 100644 --- a/app/forms/reset_password_form.rb +++ b/app/forms/reset_password_form.rb @@ -5,11 +5,14 @@ class ResetPasswordForm include FormPasswordValidator attr_accessor :reset_password_token + private attr_reader :log_password_matches_existing + alias_method :log_password_matches_existing?, :log_password_matches_existing validate :valid_token - def initialize(user:) + def initialize(user:, log_password_matches_existing: false) @user = user + @log_password_matches_existing = log_password_matches_existing @reset_password_token = @user.reset_password_token @validate_confirmation = true @active_profile = user.active_profile @@ -47,6 +50,8 @@ def handle_valid_password end def update_user + @password_matches_existing = user.valid_password?(password) if log_password_matches_existing? + attributes = { password: password } ActiveRecord::Base.transaction do @@ -87,6 +92,7 @@ def extra_analytics_attributes { user_id: user.uuid, profile_deactivated: active_profile.present?, + password_matches_existing: @password_matches_existing, pending_profile_invalidated: pending_profile.present?, pending_profile_pending_reasons: (pending_profile&.pending_reasons || [])&.join(','), } diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 3c09355e945..0db605f53ae 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -5713,6 +5713,7 @@ def password_reset_email( # @param [String] pending_profile_pending_reasons Comma-separated list of the pending states # associated with the associated profile. # @param [Hash] error_details Details for errors that occurred in unsuccessful submission + # @param [Boolean] password_matches_existing Whether the password is same as the user's current # The user changed the password for their account via the password reset flow def password_reset_password( success:, @@ -5720,6 +5721,7 @@ def password_reset_password( profile_deactivated:, pending_profile_invalidated:, pending_profile_pending_reasons:, + password_matches_existing:, error_details: {}, **extra ) @@ -5731,6 +5733,7 @@ def password_reset_password( profile_deactivated:, pending_profile_invalidated:, pending_profile_pending_reasons:, + password_matches_existing:, **extra, ) end diff --git a/config/application.yml.default b/config/application.yml.default index c425f2d8503..0a414886319 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -226,6 +226,7 @@ lexisnexis_trueid_username: test_username lexisnexis_username: test_username ################################################################### lockout_period_in_minutes: 10 +log_password_reset_matches_existing_ab_test_percent: 0 log_to_stdout: false login_otp_confirmation_max_attempts: 10 logins_per_email_and_ip_bantime: 60 diff --git a/config/initializers/ab_tests.rb b/config/initializers/ab_tests.rb index 5a970f79907..09053b2ad63 100644 --- a/config/initializers/ab_tests.rb +++ b/config/initializers/ab_tests.rb @@ -81,4 +81,10 @@ def self.all SecureRandom.alphanumeric(8) end end.freeze + + LOG_PASSWORD_RESET_MATCHES_EXISTING = AbTest.new( + experiment_name: 'Log password_matches_existing event property on password reset', + should_log: ['Password Reset: Password Submitted'].to_set, + buckets: { log: IdentityConfig.store.log_password_reset_matches_existing_ab_test_percent }, + ).freeze end diff --git a/lib/identity_config.rb b/lib/identity_config.rb index 35847e7f090..791bb08c57e 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -235,6 +235,7 @@ def self.store config.add(:lexisnexis_trueid_username, type: :string) config.add(:lexisnexis_username, type: :string) config.add(:lockout_period_in_minutes, type: :integer) + config.add(:log_password_reset_matches_existing_ab_test_percent, type: :integer) config.add(:log_to_stdout, type: :boolean) config.add(:login_otp_confirmation_max_attempts, type: :integer) config.add(:logins_per_email_and_ip_bantime, type: :integer) diff --git a/spec/controllers/users/reset_passwords_controller_spec.rb b/spec/controllers/users/reset_passwords_controller_spec.rb index 9f08e5edf0b..d3563f9ff27 100644 --- a/spec/controllers/users/reset_passwords_controller_spec.rb +++ b/spec/controllers/users/reset_passwords_controller_spec.rb @@ -1,6 +1,8 @@ require 'rails_helper' RSpec.describe Users::ResetPasswordsController, devise: true do + include AbTestsHelper + let(:password_error_message) do t('errors.attributes.password.too_short.other', count: Devise.password_length.first) end @@ -402,6 +404,58 @@ expect(user.active_profile.present?).to eq false expect(response).to redirect_to new_user_session_path end + + context 'proofed user submits same password as current' do + let(:user) { create(:user, :proofed) } + let(:password) { user.password } + + it 'logs event indicating profile deactivated while password the same' do + stub_analytics + + reset_password_token = user.set_reset_password_token + + put :update, params: { + reset_password_form: { + password:, + password_confirmation: password, + reset_password_token:, + }, + } + + expect(@analytics).to have_logged_event( + 'Password Reset: Password Submitted', + hash_not_including(password_matches_existing: be_in([true, false])), + ) + end + + context 'when in ab test for logging password matches existing' do + before do + allow(controller).to receive(:ab_test_bucket).with( + :LOG_PASSWORD_RESET_MATCHES_EXISTING, + user:, + ).and_return(:log) + end + + it 'logs event indicating profile deactivated while password the same' do + stub_analytics + + reset_password_token = user.set_reset_password_token + + put :update, params: { + reset_password_form: { + password:, + password_confirmation: password, + reset_password_token:, + }, + } + + expect(@analytics).to have_logged_event( + 'Password Reset: Password Submitted', + hash_including(profile_deactivated: true, password_matches_existing: true), + ) + end + end + end end context 'unconfirmed user submits valid new password' do diff --git a/spec/forms/reset_password_form_spec.rb b/spec/forms/reset_password_form_spec.rb index 8873e3b8856..df25444e89f 100644 --- a/spec/forms/reset_password_form_spec.rb +++ b/spec/forms/reset_password_form_spec.rb @@ -1,7 +1,9 @@ require 'rails_helper' RSpec.describe ResetPasswordForm, type: :model do - subject { ResetPasswordForm.new(user: build_stubbed(:user, uuid: '123')) } + let(:user) { create(:user, uuid: '123') } + let(:log_password_matches_existing) { false } + subject(:form) { ResetPasswordForm.new(user:, log_password_matches_existing:) } let(:password) { 'a good and powerful password' } let(:password_confirmation) { password } @@ -15,202 +17,168 @@ it_behaves_like 'password validation' describe '#submit' do + subject(:result) { form.submit(params) } + context 'when the password is valid but the token has expired' do - it 'returns a hash with errors' do - user = build_stubbed(:user, uuid: '123') + before do allow(user).to receive(:reset_password_period_valid?).and_return(false) + end - form = ResetPasswordForm.new(user: user) - - password = 'valid password' - - errors = { reset_password_token: ['token_expired'] } - - extra = { user_id: '123', profile_deactivated: false } - - expect( - form.submit( - password: password, - password_confirmation: password, - ).to_h, - ).to include( + it 'returns a hash with errors' do + expect(result.to_h).to eq( success: false, - errors: errors, - error_details: hash_including(*errors.keys), - **extra, + errors: { reset_password_token: ['token_expired'] }, + error_details: { reset_password_token: { token_expired: true } }, + user_id: '123', + profile_deactivated: false, + pending_profile_invalidated: false, + pending_profile_pending_reasons: '', + password_matches_existing: nil, ) end end context 'when the password is invalid and token is valid' do - it 'returns a hash with errors' do - user = build_stubbed(:user, uuid: '123') - allow(user).to receive(:reset_password_period_valid?).and_return(true) - - form = ResetPasswordForm.new(user: user) - - password = 'invalid' + let(:password) { 'invalid' } - errors = { - password: - ["Password must be at least #{Devise.password_length.first} characters long"], - password_confirmation: [I18n.t( - 'errors.messages.too_short', - count: Devise.password_length.first, - )], - } - - extra = { user_id: '123', profile_deactivated: false } + before do + allow(user).to receive(:reset_password_period_valid?).and_return(true) + end - expect( - form.submit( - password: password, - password_confirmation: password, - ).to_h, - ).to include( + it 'returns a hash with errors' do + expect(result.to_h).to eq( success: false, - errors: errors, - error_details: hash_including(*errors.keys), - **extra, + errors: { + password: + ["Password must be at least #{Devise.password_length.first} characters long"], + password_confirmation: [I18n.t( + 'errors.messages.too_short', + count: Devise.password_length.first, + )], + }, + error_details: { + password: { too_short: true }, + password_confirmation: { too_short: true }, + }, + user_id: '123', + profile_deactivated: false, + pending_profile_invalidated: false, + pending_profile_pending_reasons: '', + password_matches_existing: nil, ) end end context 'when both the password and token are valid' do - it 'sets the user password to the submitted password' do - user = create(:user, uuid: '123') + before do allow(user).to receive(:reset_password_period_valid?).and_return(true) + end - form = ResetPasswordForm.new(user: user) - password = 'valid password' - - result = nil - expect do - result = form.submit( - password: password, - password_confirmation: password, - ).to_h - end.to(change { user.reload.encrypted_password_digest }) + it 'sets the user password to the submitted password' do + expect { result }.to change { user.reload.encrypted_password_digest } - expect(result).to eq( + expect(result.to_h).to eq( success: true, errors: {}, user_id: '123', profile_deactivated: false, pending_profile_invalidated: false, pending_profile_pending_reasons: '', + password_matches_existing: nil, ) end end context 'when both the password and token are invalid' do - it 'returns a hash with errors' do - user = build_stubbed(:user, uuid: '123') + let(:password) { 'short' } + + before do allow(user).to receive(:reset_password_period_valid?).and_return(false) + end - form = ResetPasswordForm.new(user: user) - - password = 'short' - - errors = { - password: [ - t('errors.attributes.password.too_short.other', count: Devise.password_length.first), - ], - password_confirmation: [I18n.t( - 'errors.messages.too_short', - count: Devise.password_length.first, - )], - reset_password_token: ['token_expired'], - } - - extra = { user_id: '123', profile_deactivated: false } - - expect( - form.submit( - password: password, - password_confirmation: password, - ).to_h, - ).to include( + it 'returns a hash with errors' do + expect(result.to_h).to eq( success: false, - errors: errors, - error_details: hash_including(*errors.keys), - **extra, + errors: { + password: [ + t('errors.attributes.password.too_short.other', count: Devise.password_length.first), + ], + password_confirmation: [ + t('errors.messages.too_short', count: Devise.password_length.first), + ], + reset_password_token: ['token_expired'], + }, + error_details: { + password: { too_short: true }, + password_confirmation: { too_short: true }, + reset_password_token: { token_expired: true }, + }, + user_id: '123', + profile_deactivated: false, + pending_profile_invalidated: false, + pending_profile_pending_reasons: '', + password_matches_existing: nil, ) end end context 'when the user does not exist in the db' do + let(:user) { User.new } + it 'returns a hash with errors' do - user = User.new - - form = ResetPasswordForm.new(user: user) - errors = { - password: [ - t('errors.attributes.password.too_short.other', count: Devise.password_length.first), - ], - password_confirmation: [I18n.t( - 'errors.messages.too_short', - count: Devise.password_length.first, - )], - reset_password_token: ['invalid_token'], - } - - extra = { user_id: nil, profile_deactivated: false } - password = 'short' - - expect( - form.submit( - password: password, - password_confirmation: password, - ).to_h, - ).to include( + expect(result.to_h).to eq( success: false, - errors: errors, - error_details: hash_including(*errors.keys), - **extra, + errors: { reset_password_token: ['invalid_token'] }, + error_details: { reset_password_token: { invalid_token: true } }, + user_id: nil, + profile_deactivated: false, + pending_profile_invalidated: false, + pending_profile_pending_reasons: '', + password_matches_existing: nil, ) end end context 'when the user has an active profile' do + let(:user) { create(:user, :proofed, reset_password_sent_at: Time.zone.now) } + it 'deactivates the profile' do - profile = create(:profile, :active, :verified) - user = profile.user - user.update(reset_password_sent_at: Time.zone.now) + expect(result.success?).to eq(true) + expect(result.extra[:profile_deactivated]).to eq(true) + expect(user.profiles.any?(&:active?)).to eq(false) + end - form = ResetPasswordForm.new(user: user) + context 'when the password is same as current' do + let(:password) { user.password } - result = form.submit(params) + it 'does not include extra detail for password matching existing' do + expect(result.extra[:password_matches_existing]).to eq(nil) + end - expect(result.success?).to eq(true) - expect(result.extra[:profile_deactivated]).to eq(true) - expect(profile.reload.active?).to eq(false) + context 'when initialized to log password_matches_existing' do + let(:log_password_matches_existing) { true } + + it 'includes extra detail for password matching existing' do + expect(result.extra[:password_matches_existing]).to eq(true) + end + end end end context 'when the user does not have an active profile' do - it 'includes that the profile was not deactivated in the form response' do - user = create(:user) - user.update(reset_password_sent_at: Time.zone.now) - - form = ResetPasswordForm.new(user: user) - - result = form.submit(params) + let(:user) { create(:user, reset_password_sent_at: Time.zone.now) } + it 'includes that the profile was not deactivated in the form response' do expect(result.success?).to eq(true) expect(result.extra[:profile_deactivated]).to eq(false) end end context 'when the user has a pending profile' do - it 'includes that the profile was not deactivated in the form response' do - profile = create(:profile, :verify_by_mail_pending, :in_person_verification_pending) - user = profile.user - user.update(reset_password_sent_at: Time.zone.now) - - form = ResetPasswordForm.new(user: user) - result = form.submit(params) + let(:profile) { create(:profile, :verify_by_mail_pending, :in_person_verification_pending) } + let(:user) { create(:user, reset_password_sent_at: Time.zone.now, profiles: [profile]) } + it 'includes that the profile was not deactivated in the form response' do expect(result.success?).to eq(true) expect(result.extra[:pending_profile_invalidated]).to eq(true) expect(result.extra[:pending_profile_pending_reasons]).to eq( @@ -220,14 +188,9 @@ end context 'when the user does not have a pending profile' do - it 'includes that the profile was not deactivated in the form response' do - user = create(:user) - user.update(reset_password_sent_at: Time.zone.now) - - form = ResetPasswordForm.new(user: user) - - result = form.submit(params) + let(:user) { create(:user, reset_password_sent_at: Time.zone.now) } + it 'includes that the profile was not deactivated in the form response' do expect(result.success?).to eq(true) expect(result.extra[:pending_profile_invalidated]).to eq(false) expect(result.extra[:pending_profile_pending_reasons]).to eq('') @@ -235,19 +198,16 @@ end context 'when the unconfirmed email address has been confirmed by another account' do - it 'does not raise an error and is not successful' do - user = create(:user, :unconfirmed) - user.update(reset_password_sent_at: Time.zone.now) - user2 = create(:user) + let(:user) { create(:user, :unconfirmed, reset_password_sent_at: Time.zone.now) } + + before do create( - :email_address, email: user.email_addresses.first.email, user_id: user2.id, - confirmed_at: Time.zone.now + :user, + email_addresses: [create(:email_address, email: user.email_addresses.first.email)], ) + end - form = ResetPasswordForm.new(user: user) - - result = form.submit(params) - + it 'does not raise an error and is not successful' do expect(result.success?).to eq(false) expect(result.errors).to eq({ reset_password_token: ['token_expired'] }) end