From 31f73ee2596aa05a6f1969f099d6e8897e4fac95 Mon Sep 17 00:00:00 2001 From: Luis Date: Thu, 29 Sep 2022 14:12:12 -0500 Subject: [PATCH] Standardize success parameter to always be first changelog: Internal, Attempts API, Standardize events --- .../idv/otp_verification_controller.rb | 2 +- app/controllers/idv/phone_controller.rb | 2 +- .../two_factor_authentication_controller.rb | 4 +- .../irs_attempts_api/tracker_events.rb | 20 ++--- .../idv/otp_verification_controller_spec.rb | 19 ++--- ...o_factor_authentication_controller_spec.rb | 74 ++++++++----------- 6 files changed, 54 insertions(+), 67 deletions(-) diff --git a/app/controllers/idv/otp_verification_controller.rb b/app/controllers/idv/otp_verification_controller.rb index d2c76cb5979..335b0b8ff11 100644 --- a/app/controllers/idv/otp_verification_controller.rb +++ b/app/controllers/idv/otp_verification_controller.rb @@ -20,8 +20,8 @@ def update result = phone_confirmation_otp_verification_form.submit(code: params[:code]) analytics.idv_phone_confirmation_otp_submitted(**result.to_h) irs_attempts_api_tracker.idv_phone_otp_submitted( - phone_number: idv_session.user_phone_confirmation_session.phone, success: result.success?, + phone_number: idv_session.user_phone_confirmation_session.phone, failure_reason: result.success? ? {} : result.extra.slice(:code_expired, :code_matches), ) diff --git a/app/controllers/idv/phone_controller.rb b/app/controllers/idv/phone_controller.rb index a0b6a80e4b1..3af426ccf82 100644 --- a/app/controllers/idv/phone_controller.rb +++ b/app/controllers/idv/phone_controller.rb @@ -32,8 +32,8 @@ def create result = idv_form.submit(step_params) analytics.idv_phone_confirmation_form_submitted(**result.to_h) irs_attempts_api_tracker.idv_phone_submitted( - phone_number: step_params[:phone], success: result.success?, + phone_number: step_params[:phone], failure_reason: irs_attempts_api_tracker.parse_failure_reason(result), ) flash[:error] = result.first_error_message if !result.success? diff --git a/app/controllers/users/two_factor_authentication_controller.rb b/app/controllers/users/two_factor_authentication_controller.rb index ce280650bac..245fcd93fa4 100644 --- a/app/controllers/users/two_factor_authentication_controller.rb +++ b/app/controllers/users/two_factor_authentication_controller.rb @@ -225,15 +225,15 @@ def track_events(otp_delivery_preference:) if UserSessionContext.reauthentication_context?(context) irs_attempts_api_tracker.mfa_login_phone_otp_sent( - reauthentication: true, success: @telephony_result.success?, + reauthentication: true, phone_number: parsed_phone.e164, otp_delivery_method: otp_delivery_preference, ) elsif UserSessionContext.authentication_context?(context) irs_attempts_api_tracker.mfa_login_phone_otp_sent( - reauthentication: false, success: @telephony_result.success?, + reauthentication: false, phone_number: parsed_phone.e164, otp_delivery_method: otp_delivery_preference, ) diff --git a/app/services/irs_attempts_api/tracker_events.rb b/app/services/irs_attempts_api/tracker_events.rb index f66c842380f..1c27353c067 100644 --- a/app/services/irs_attempts_api/tracker_events.rb +++ b/app/services/irs_attempts_api/tracker_events.rb @@ -215,14 +215,14 @@ def idv_phone_otp_sent_rate_limited end # Tracks when a user submits OTP code sent to their phone - # @param [String] phone_number # @param [Boolean] success + # @param [String] phone_number # @param [Hash>] failure_reason - def idv_phone_otp_submitted(phone_number:, success:, failure_reason: nil) + def idv_phone_otp_submitted(success:, phone_number:, failure_reason: nil) track_event( :idv_phone_otp_submitted, - phone_number: phone_number, success: success, + phone_number: phone_number, failure_reason: failure_reason, ) end @@ -246,14 +246,14 @@ def idv_phone_send_link_rate_limited(phone_number:) end # Tracks when the user submits their idv phone number - # @param [String] phone_number # @param [Boolean] success + # @param [String] phone_number # @param [Hash>] failure_reason - def idv_phone_submitted(phone_number:, success:, failure_reason: nil) + def idv_phone_submitted(success:, phone_number:, failure_reason: nil) track_event( :idv_phone_submitted, - phone_number: phone_number, success: success, + phone_number: phone_number, failure_reason: failure_reason, ) end @@ -460,8 +460,8 @@ def mfa_enroll_phone_otp_submitted(success:) end # Tracks when the user has attempted to enroll the piv cac MFA method to their account - # @param [String] subject_dn # @param [Boolean] success + # @param [String] subject_dn # @param [Hash>] failure_reason def mfa_enroll_piv_cac( success:, @@ -522,16 +522,16 @@ def mfa_login_backup_code(success:) ) end - # @param [Boolean] reauthentication - True if the user was already logged in # @param [Boolean] success - True if the OTP Verification was sent + # @param [Boolean] reauthentication - True if the user was already logged in # @param [String] phone_number - The user's phone_number used for multi-factor authentication # @param [String] otp_delivery_method - Either SMS or Voice # During a login attempt, an OTP code has been sent via SMS or Voice. - def mfa_login_phone_otp_sent(reauthentication:, success:, phone_number:, otp_delivery_method:) + def mfa_login_phone_otp_sent(success:, reauthentication:, phone_number:, otp_delivery_method:) track_event( :mfa_login_phone_otp_sent, - reauthentication: reauthentication, success: success, + reauthentication: reauthentication, phone_number: phone_number, otp_delivery_method: otp_delivery_method, ) diff --git a/spec/controllers/idv/otp_verification_controller_spec.rb b/spec/controllers/idv/otp_verification_controller_spec.rb index 6c434283cb9..6f10ccd9fb0 100644 --- a/spec/controllers/idv/otp_verification_controller_spec.rb +++ b/spec/controllers/idv/otp_verification_controller_spec.rb @@ -21,7 +21,6 @@ stub_analytics stub_attempts_tracker allow(@analytics).to receive(:track_event) - allow(@irs_attempts_api_tracker).to receive(:track_event) sign_in(user) stub_verify_steps_one_and_two(user) @@ -103,30 +102,28 @@ describe 'track irs analytics event' do context 'when the phone otp code is valid' do it 'captures success event' do - put :update, params: { code: phone_confirmation_otp_code } - - expect(@irs_attempts_api_tracker).to have_received(:track_event).with( - :idv_phone_otp_submitted, - phone_number: phone, + expect(@irs_attempts_api_tracker).to receive(:idv_phone_otp_submitted).with( success: true, + phone_number: phone, failure_reason: {}, ) + + put :update, params: { code: phone_confirmation_otp_code } end end context 'when the phone otp code is invalid' do it 'captures failure event' do - put :update, params: { code: '000' } - - expect(@irs_attempts_api_tracker).to have_received(:track_event).with( - :idv_phone_otp_submitted, - phone_number: phone, + expect(@irs_attempts_api_tracker).to receive(:idv_phone_otp_submitted).with( success: false, + phone_number: phone, failure_reason: { code_matches: false, code_expired: false, }, ) + + put :update, params: { code: '000' } end end end diff --git a/spec/controllers/users/two_factor_authentication_controller_spec.rb b/spec/controllers/users/two_factor_authentication_controller_spec.rb index 534f711f3e0..383ff9ce898 100644 --- a/spec/controllers/users/two_factor_authentication_controller_spec.rb +++ b/spec/controllers/users/two_factor_authentication_controller_spec.rb @@ -3,6 +3,8 @@ describe Users::TwoFactorAuthenticationController do include ActionView::Helpers::DateHelper + let(:otp_preference_sms) { { otp_delivery_preference: 'sms' } } + describe 'before_actions' do it 'includes the appropriate before_actions' do expect(subject).to have_actions( @@ -137,7 +139,7 @@ def index expect(Telephony::Test::Message.messages.length).to eq(1) expect(Telephony::Test::Call.calls.length).to eq(0) expect(response).to redirect_to( - login_two_factor_path(otp_delivery_preference: 'sms', reauthn: 'true'), + login_two_factor_path(**otp_preference_sms, reauthn: 'true'), ) end end @@ -213,7 +215,7 @@ def index expect(Telephony::Test::Message.messages.length).to eq(1) expect(Telephony::Test::Call.calls.length).to eq(0) expect(response). - to redirect_to login_two_factor_path(otp_delivery_preference: 'sms', reauthn: false) + to redirect_to login_two_factor_path(**otp_preference_sms, reauthn: false) end context 'when no options are enabled and available for use' do @@ -271,7 +273,12 @@ def index end describe '#send_code' do + let(:otp_delivery_form_sms) { { otp_delivery_selection_form: otp_preference_sms } } context 'when selecting SMS OTP delivery' do + let(:valid_phone_number) { { phone_number: '+12025551212' } } + let(:success_parameters) do + { success: true, **valid_phone_number, otp_delivery_method: 'sms' } + end before do @user = create(:user, :with_phone) sign_in_before_2fa(@user) @@ -280,7 +287,7 @@ def index end it 'sends OTP via SMS for sign in' do - get :send_code, params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } } + get :send_code, params: otp_delivery_form_sms expect(Telephony).to have_received(:send_authentication_otp).with( otp: subject.current_user.direct_otp, @@ -293,7 +300,7 @@ def index expect(subject.current_user.direct_otp).not_to eq(@old_otp) expect(subject.current_user.direct_otp).not_to be_nil expect(response).to redirect_to( - login_two_factor_path(otp_delivery_preference: 'sms', reauthn: false), + login_two_factor_path(**otp_preference_sms, reauthn: false), ) end @@ -303,7 +310,7 @@ def index analytics_hash = { success: true, errors: {}, - otp_delivery_preference: 'sms', + **otp_preference_sms, resend: 'true', context: 'authentication', country_code: 'US', @@ -317,25 +324,20 @@ def index expect(@analytics).to receive(:track_event). ordered. with('Telephony: OTP sent', hash_including( - resend: 'true', success: true, otp_delivery_preference: 'sms', + resend: 'true', success: true, **otp_preference_sms, )) - get :send_code, params: { otp_delivery_selection_form: - { otp_delivery_preference: 'sms', resend: 'true' } } + get :send_code, params: { + otp_delivery_selection_form: { **otp_preference_sms, resend: 'true' }, + } end it 'tracks the verification attempt event' do stub_attempts_tracker expect(@irs_attempts_api_tracker).to receive(:mfa_login_phone_otp_sent). - with( - phone_number: '+12025551212', - reauthentication: false, - success: true, - otp_delivery_method: 'sms', - ) + with(reauthentication: false, **success_parameters) - get :send_code, params: { otp_delivery_selection_form: - { otp_delivery_preference: 'sms' } } + get :send_code, params: otp_delivery_form_sms end it 'tracks the attempt event when user session context is reauthentication' do @@ -343,15 +345,9 @@ def index subject.user_session[:context] = 'reauthentication' expect(@irs_attempts_api_tracker).to receive(:mfa_login_phone_otp_sent). - with( - phone_number: '+12025551212', - reauthentication: true, - success: true, - otp_delivery_method: 'sms', - ) + with(reauthentication: true, **success_parameters) - get :send_code, params: { otp_delivery_selection_form: - { otp_delivery_preference: 'sms' } } + get :send_code, params: otp_delivery_form_sms end it 'calls OtpRateLimiter#exceeded_otp_send_limit? and #increment' do @@ -364,7 +360,7 @@ def index expect(otp_rate_limiter).to receive(:exceeded_otp_send_limit?).twice expect(otp_rate_limiter).to receive(:increment) - get :send_code, params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } } + get :send_code, params: otp_delivery_form_sms end it 'marks the user as locked out after too many attempts' do @@ -375,13 +371,13 @@ def index stub_attempts_tracker expect(@irs_attempts_api_tracker).to receive(:mfa_login_phone_otp_sent_rate_limited). - with(phone_number: '+12025551212') + with(**valid_phone_number) freeze_time do (IdentityConfig.store.otp_delivery_blocklist_maxretry + 1).times do get :send_code, params: { otp_delivery_selection_form: { - otp_delivery_preference: 'sms', + **otp_preference_sms, otp_make_default_number: nil, }, } @@ -400,9 +396,7 @@ def index expect(Telephony).to_not receive(:send_authentication_otp) expect(Telephony).to_not receive(:send_confirmation_otp) - get :send_code, params: { - otp_delivery_selection_form: { otp_delivery_preference: 'sms' }, - } + get :send_code, params: otp_delivery_form_sms end end @@ -415,9 +409,7 @@ def index end it 'redirects to the opt in controller' do - get :send_code, params: { - otp_delivery_selection_form: { otp_delivery_preference: 'sms' }, - } + get :send_code, params: otp_delivery_form_sms opt_out = PhoneNumberOptOut.create_or_find_with_phone( Telephony::Test::ErrorSimulator::OPT_OUT_PHONE_NUMBER, @@ -523,7 +515,7 @@ def index allow(Telephony).to receive(:send_confirmation_otp).and_call_original - get :send_code, params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } } + get :send_code, params: otp_delivery_form_sms expect(Telephony).to have_received(:send_confirmation_otp).with( otp: subject.current_user.direct_otp, @@ -544,7 +536,7 @@ def index expect(@irs_attempts_api_tracker).to receive(:mfa_enroll_phone_otp_sent). with({ phone_number: '+12025551213', success: true, otp_delivery_method: 'sms' }) - get :send_code, params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } } + get :send_code, params: otp_delivery_form_sms end it 'rate limits confirmation OTPs on sign up' do @@ -555,8 +547,7 @@ def index freeze_time do (IdentityConfig.store.phone_confirmation_max_attempts + 1).times do subject.user_session[:unconfirmed_phone] = '+1 (202) 555-1213' - get :send_code, - params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } } + get :send_code, params: otp_delivery_form_sms end timeout = distance_of_time_in_words( @@ -591,7 +582,7 @@ def index (IdentityConfig.store.otp_delivery_blocklist_maxretry + 1).times do get :send_code, params: { otp_delivery_selection_form: { - otp_delivery_preference: 'sms', + **otp_preference_sms, otp_make_default_number: nil, }, } @@ -609,8 +600,7 @@ def index freeze_time do (IdentityConfig.store.phone_confirmation_max_attempts + 1).times do subject.user_session[:unconfirmed_phone] = '+1 (202) 555-1213' - get :send_code, - params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } } + get :send_code, params: otp_delivery_form_sms end timeout = distance_of_time_in_words( @@ -633,7 +623,7 @@ def index subject.user_session[:unconfirmed_phone] = @unconfirmed_phone subject.user_session[:unconfirmed_phone] = '+1 (225) 555-1000' - get :send_code, params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } } + get :send_code, params: otp_delivery_form_sms expect(flash[:error]).to eq(I18n.t('telephony.error.friendly_message.generic')) end @@ -649,7 +639,7 @@ def index otp_delivery_selection_form: { otp_delivery_preference: 'pigeon' }, } - expect(response).to redirect_to login_two_factor_url(otp_delivery_preference: 'sms') + expect(response).to redirect_to login_two_factor_url(**otp_preference_sms) end end end