diff --git a/app/controllers/api/verify/password_reset_controller.rb b/app/controllers/api/verify/password_reset_controller.rb index f3cb957e9f6..2b4d6d69ca9 100644 --- a/app/controllers/api/verify/password_reset_controller.rb +++ b/app/controllers/api/verify/password_reset_controller.rb @@ -17,7 +17,12 @@ def create def reset_password(email, request_id) sign_out - RequestPasswordReset.new(email: email, request_id: request_id, analytics: analytics).perform + RequestPasswordReset.new( + email: email, + request_id: request_id, + analytics: analytics, + irs_attempts_api_tracker: irs_attempts_api_tracker, + ).perform session[:email] = email end end diff --git a/app/controllers/idv/forgot_password_controller.rb b/app/controllers/idv/forgot_password_controller.rb index 7926736bc36..938fb7bff83 100644 --- a/app/controllers/idv/forgot_password_controller.rb +++ b/app/controllers/idv/forgot_password_controller.rb @@ -20,7 +20,12 @@ def update def reset_password(email, request_id) sign_out - RequestPasswordReset.new(email: email, request_id: request_id, analytics: analytics).perform + RequestPasswordReset.new( + email: email, + request_id: request_id, + analytics: analytics, + irs_attempts_api_tracker: irs_attempts_api_tracker, + ).perform # The user/email is always found so... session[:email] = email redirect_to forgot_password_url(request_id: request_id) diff --git a/app/controllers/users/reset_passwords_controller.rb b/app/controllers/users/reset_passwords_controller.rb index 9e8ec01ebdf..ee6bf002f0f 100644 --- a/app/controllers/users/reset_passwords_controller.rb +++ b/app/controllers/users/reset_passwords_controller.rb @@ -89,6 +89,7 @@ def create_account_if_email_not_found email: email, request_id: request_id, analytics: analytics, + irs_attempts_api_tracker: irs_attempts_api_tracker, ).perform return unless result diff --git a/app/services/irs_attempts_api/tracker_events.rb b/app/services/irs_attempts_api/tracker_events.rb index 950e5c9bfe0..c6574139f13 100644 --- a/app/services/irs_attempts_api/tracker_events.rb +++ b/app/services/irs_attempts_api/tracker_events.rb @@ -43,6 +43,26 @@ def email_and_password_auth(email:, success:) ) end + # The user has exceeded the rate limit for password reset emails + # @param [String] email The user's email address + def forgot_password_email_rate_limited(email:) + track_event( + :forgot_password_email_rate_limited, + email: email, + ) + end + + # Tracks when the user has requested a forgot password email + # @param [String] email The submitted email address + # @param [Boolean] success True if the forgot password email was sent + def forgot_password_email_sent(email:, success:) + track_event( + :forgot_password_email_sent, + email: email, + success: success, + ) + end + # @param [Boolean] success # @param [Hash>] failure_reason def forgot_password_email_confirmed(success:, failure_reason: nil) diff --git a/app/services/request_password_reset.rb b/app/services/request_password_reset.rb index a6d3c4c8873..be4c4c45d49 100644 --- a/app/services/request_password_reset.rb +++ b/app/services/request_password_reset.rb @@ -1,6 +1,7 @@ RequestPasswordReset = RedactedStruct.new( - :email, :request_id, :analytics, keyword_init: true, - allowed_members: [:request_id] + :email, :request_id, :analytics, :irs_attempts_api_tracker, + keyword_init: true, + allowed_members: [:request_id] ) do def perform if user_should_receive_registration_email? @@ -18,12 +19,15 @@ def perform def send_reset_password_instructions if Throttle.new(user: user, throttle_type: :reset_password_email).throttled_else_increment? analytics.throttler_rate_limit_triggered(throttle_type: :reset_password_email) + irs_attempts_api_tracker.forgot_password_email_rate_limited(email: email) else token = user.set_reset_password_token UserMailer.reset_password_instructions(user, email, token: token).deliver_now_or_later event = PushNotification::RecoveryActivatedEvent.new(user: user) PushNotification::HttpPush.deliver(event) + + irs_attempts_api_tracker.forgot_password_email_sent(email: email, success: true) end end diff --git a/spec/controllers/idv/forgot_password_controller_spec.rb b/spec/controllers/idv/forgot_password_controller_spec.rb index 12d0c09a074..e472730ff49 100644 --- a/spec/controllers/idv/forgot_password_controller_spec.rb +++ b/spec/controllers/idv/forgot_password_controller_spec.rb @@ -8,25 +8,40 @@ end describe '#new' do - it 'tracks the event in analytics when referer is nil' do + before do stub_sign_in stub_analytics - expect(@analytics).to receive(:track_event).with('IdV: forgot password visited') + allow(@analytics).to receive(:track_event) + end + it 'tracks the event in analytics when referer is nil' do get :new + + expect(@analytics).to have_received(:track_event).with('IdV: forgot password visited') end end describe '#update' do - it 'tracks an analytics event' do - user = create(:user) + let(:user) { create(:user) } + + before do stub_sign_in(user) stub_analytics + stub_attempts_tracker + allow(@analytics).to receive(:track_event) + allow(@irs_attempts_api_tracker).to receive(:track_event) + end - expect(@analytics).to receive(:track_event).with('IdV: forgot password confirmed') - + it 'tracks analytics events' do post :update + + expect(@analytics).to have_received(:track_event).with('IdV: forgot password confirmed') + expect(@irs_attempts_api_tracker).to have_received(:track_event).with( + :forgot_password_email_sent, + email: user.email, + success: true, + ) end end end diff --git a/spec/controllers/users/reset_passwords_controller_spec.rb b/spec/controllers/users/reset_passwords_controller_spec.rb index c956301c219..bd56e822017 100644 --- a/spec/controllers/users/reset_passwords_controller_spec.rb +++ b/spec/controllers/users/reset_passwords_controller_spec.rb @@ -5,21 +5,25 @@ "This password is too short (minimum is #{Devise.password_length.first} characters)" end describe '#edit' do - context 'no user matches token' do - it 'redirects to page where user enters email for password reset token' do - stub_analytics - stub_attempts_tracker - allow(@analytics).to receive(:track_event) - allow(@irs_attempts_api_tracker).to receive(:track_event) - - get :edit, params: { reset_password_token: 'foo' } + before do + stub_analytics + stub_attempts_tracker + allow(@analytics).to receive(:track_event) + allow(@irs_attempts_api_tracker).to receive(:track_event) + end - analytics_hash = { + context 'no user matches token' do + let(:analytics_hash) do + { success: false, errors: { user: ['invalid_token'] }, error_details: { user: [:blank] }, user_id: nil, } + end + + it 'redirects to page where user enters email for password reset token' do + get :edit, params: { reset_password_token: 'foo' } expect(@analytics).to have_received(:track_event). with('Password Reset: Token Submitted', analytics_hash) @@ -35,24 +39,23 @@ end context 'token expired' do - it 'redirects to page where user enters email for password reset token' do - stub_analytics - stub_attempts_tracker - allow(@analytics).to receive(:track_event) - allow(@irs_attempts_api_tracker).to receive(:track_event) - - user = instance_double('User', uuid: '123') - allow(User).to receive(:with_reset_password_token).with('foo').and_return(user) - allow(user).to receive(:reset_password_period_valid?).and_return(false) - - get :edit, params: { reset_password_token: 'foo' } - - analytics_hash = { + let(:analytics_hash) do + { success: false, errors: { user: ['token_expired'] }, error_details: { user: ['token_expired'] }, user_id: '123', } + end + let(:user) { instance_double('User', uuid: '123') } + + before do + allow(User).to receive(:with_reset_password_token).with('foo').and_return(user) + allow(user).to receive(:reset_password_period_valid?).and_return(false) + end + + it 'redirects to page where user enters email for password reset token' do + get :edit, params: { reset_password_token: 'foo' } expect(@analytics).to have_received(:track_event). with('Password Reset: Token Submitted', analytics_hash) @@ -68,17 +71,17 @@ context 'token is valid' do render_views + let(:user) { instance_double('User', uuid: '123') } + let(:email_address) { instance_double('EmailAddress') } - it 'displays the form to enter a new password and disallows indexing' do + before do stub_analytics - stub_attempts_tracker - allow(@irs_attempts_api_tracker).to receive(:track_event) - - user = instance_double('User', uuid: '123') - email_address = instance_double('EmailAddress') allow(User).to receive(:with_reset_password_token).with('foo').and_return(user) allow(user).to receive(:reset_password_period_valid?).and_return(true) allow(user).to receive(:email_addresses).and_return([email_address]) + end + + it 'displays the form to enter a new password and disallows indexing' do expect(email_address).to receive(:email).twice forbidden = instance_double(ForbiddenPasswords) @@ -376,11 +379,12 @@ describe '#create' do context 'no user matches email' do + let(:email) { 'nonexistent@example.com' } + it 'send an email to tell the user they do not have an account yet' do stub_analytics stub_attempts_tracker allow(@irs_attempts_api_tracker).to receive(:track_event) - email = 'nonexistent@example.com' expect do put :create, params: { @@ -422,54 +426,85 @@ end context 'user exists' do - it 'sends password reset email to user and tracks event' do - stub_analytics - - user = create(:user, :signed_up, email: 'test@example.com') - - analytics_hash = { + let(:email) { 'test@example.com' } + let!(:user) { create(:user, :signed_up, email: email) } + let(:analytics_hash) do + { success: true, errors: {}, user_id: user.uuid, confirmed: true, active_profile: false, } + end - expect(@analytics).to receive(:track_event). - with('Password Reset: Email Submitted', analytics_hash) + before do + stub_analytics + stub_attempts_tracker + allow(@analytics).to receive(:track_event) + allow(@irs_attempts_api_tracker).to receive(:track_event) + end + it 'sends password reset email to user and tracks event' do expect do - put :create, params: { password_reset_email_form: { email: 'Test@example.com' } } + put :create, params: { password_reset_email_form: { email: email } } end.to change { ActionMailer::Base.deliveries.count }.by(1) + expect(@analytics).to have_received(:track_event). + with('Password Reset: Email Submitted', analytics_hash) + + expect(@irs_attempts_api_tracker).to have_received(:track_event).with( + :forgot_password_email_sent, + email: email, + success: true, + ) + expect(response).to redirect_to forgot_password_path end end context 'user exists but is unconfirmed' do - it 'sends password reset email to user and tracks event' do - stub_analytics - - user = create(:user, :unconfirmed) - - analytics_hash = { + let(:user) { create(:user, :unconfirmed) } + let(:analytics_hash) do + { success: true, errors: {}, user_id: user.uuid, confirmed: false, active_profile: false, } + end + let(:params) do + { + password_reset_email_form: { + email: user.email, + }, + } + end - expect(@analytics).to receive(:track_event). - with('Password Reset: Email Submitted', analytics_hash) + before do + stub_analytics + stub_attempts_tracker + allow(@analytics).to receive(:track_event) + allow(@irs_attempts_api_tracker).to receive(:track_event) + end - params = { password_reset_email_form: { email: user.email } } + it 'sends password reset email to user and tracks event' do expect { put :create, params: params }. to change { ActionMailer::Base.deliveries.count }.by(1) + expect(@analytics).to have_received(:track_event). + with('Password Reset: Email Submitted', analytics_hash) + expect(ActionMailer::Base.deliveries.last.subject). to eq t('user_mailer.reset_password_instructions.subject') + expect(@irs_attempts_api_tracker).to have_received(:track_event).with( + :forgot_password_email_sent, + email: user.email, + success: true, + ) + expect(response).to redirect_to forgot_password_path end end @@ -477,6 +512,8 @@ context 'user is verified' do it 'captures in analytics that the user was verified' do stub_analytics + stub_attempts_tracker + allow(@irs_attempts_api_tracker).to receive(:track_event) user = create(:user, :signed_up) create(:profile, :active, :verified, user: user) @@ -494,6 +531,12 @@ params = { password_reset_email_form: { email: user.email } } put :create, params: params + + expect(@irs_attempts_api_tracker).to have_received(:track_event).with( + :forgot_password_email_sent, + email: user.email, + success: true, + ) end end diff --git a/spec/services/request_password_reset_spec.rb b/spec/services/request_password_reset_spec.rb index 58b2fe9a7a2..6f48f6f993b 100644 --- a/spec/services/request_password_reset_spec.rb +++ b/spec/services/request_password_reset_spec.rb @@ -4,6 +4,17 @@ describe '#perform' do let(:user) { create(:user) } let(:email) { user.email_addresses.first.email } + let(:irs_attempts_api_tracker) do + instance_double( + IrsAttemptsApi::Tracker, + forgot_password_email_sent: true, + forgot_password_email_rate_limited: true, + ) + end + + before do + allow(IrsAttemptsApi::Tracker).to receive(:new).and_return(irs_attempts_api_tracker) + end context 'when the user is not found' do it 'sends the account registration email' do @@ -22,7 +33,10 @@ send_sign_up_email_confirmation, ) - RequestPasswordReset.new(email: email).perform + RequestPasswordReset.new( + email: email, + irs_attempts_api_tracker: irs_attempts_api_tracker, + ).perform user = User.find_with_email(email) expect(user).to be_present expect(RegistrationLog.first.user_id).to eq(user.id) @@ -30,7 +44,14 @@ end context 'when the user is found and confirmed' do - it 'sends password reset instructions' do + subject(:perform) do + described_class.new( + email: email, + irs_attempts_api_tracker: irs_attempts_api_tracker, + ).perform + end + + before do allow(UserMailer).to receive(:reset_password_instructions). and_wrap_original do |impl, user, email, options| token = options.fetch(:token) @@ -40,8 +61,10 @@ impl.call(user, email, **options) end + end - expect { RequestPasswordReset.new(email: email).perform }. + it 'sends password reset instructions' do + expect { subject }. to(change { user.reload.reset_password_token }) end @@ -49,7 +72,13 @@ expect(PushNotification::HttpPush).to receive(:deliver). with(PushNotification::RecoveryActivatedEvent.new(user: user)) - RequestPasswordReset.new(email: email).perform + subject + end + + it 'calls irs tracking method forgot_password_email_sent ' do + subject + + expect(irs_attempts_api_tracker).to have_received(:forgot_password_email_sent) end end @@ -65,7 +94,12 @@ impl.call(user, email, **options) end - expect { RequestPasswordReset.new(email: email).perform }. + expect { + RequestPasswordReset.new( + email: email, + irs_attempts_api_tracker: irs_attempts_api_tracker, + ).perform + }. to(change { user.reload.reset_password_token }) end end @@ -93,13 +127,17 @@ send_sign_up_email_confirmation, ) - RequestPasswordReset.new(email: unconfirmed_email_address.email).perform + RequestPasswordReset.new( + email: unconfirmed_email_address.email, + ).perform end it 'does not send a recovery activated push event' do expect(PushNotification::HttpPush).to_not receive(:deliver) - RequestPasswordReset.new(email: unconfirmed_email_address.email).perform + RequestPasswordReset.new( + email: unconfirmed_email_address.email, + ).perform end end @@ -112,10 +150,14 @@ end it 'always finds the user with the confirmed email address' do - form = RequestPasswordReset.new(email: email) + form = RequestPasswordReset.new( + email: email, + irs_attempts_api_tracker: irs_attempts_api_tracker, + ) form.perform expect(form.send(:user)).to eq(@user_confirmed) + expect(irs_attempts_api_tracker).to have_received(:forgot_password_email_sent) end end @@ -125,18 +167,33 @@ max_attempts = IdentityConfig.store.reset_password_email_max_attempts max_attempts.times do - expect { RequestPasswordReset.new(email: email, analytics: analytics).perform }. + expect { + RequestPasswordReset.new( + email: email, + analytics: analytics, + irs_attempts_api_tracker: irs_attempts_api_tracker, + ).perform + }. to(change { user.reload.reset_password_token }) end # extra time, throttled - expect { RequestPasswordReset.new(email: email, analytics: analytics).perform }. + expect { + RequestPasswordReset.new( + email: email, + analytics: analytics, + irs_attempts_api_tracker: irs_attempts_api_tracker, + ).perform + }. to_not(change { user.reload.reset_password_token }) expect(analytics).to have_logged_event( 'Throttler Rate Limit Triggered', throttle_type: :reset_password_email, ) + expect(irs_attempts_api_tracker).to have_received(:forgot_password_email_rate_limited).with( + email: email, + ) end it 'only sends a push notification when the attempts have not been throttled' do @@ -147,12 +204,24 @@ exactly(max_attempts).times max_attempts.times do - expect { RequestPasswordReset.new(email: email, analytics: analytics).perform }. + expect { + RequestPasswordReset.new( + email: email, + analytics: analytics, + irs_attempts_api_tracker: irs_attempts_api_tracker, + ).perform + }. to(change { user.reload.reset_password_token }) end # extra time, throttled - expect { RequestPasswordReset.new(email: email, analytics: analytics).perform }. + expect { + RequestPasswordReset.new( + email: email, + analytics: analytics, + irs_attempts_api_tracker: irs_attempts_api_tracker, + ).perform + }. to_not(change { user.reload.reset_password_token }) end end