diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index e248d9c117c..cc8dc881e7b 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -12,9 +12,10 @@ def show end def create - result = OtpVerificationForm.new(current_user, form_params[:code].strip).submit + result = OtpVerificationForm.new(current_user, sanitized_otp_code).submit + properties = result.to_h.merge(analytics_properties) - analytics.track_event(Analytics::MULTI_FACTOR_AUTH, result.to_h.merge(analytics_properties)) + analytics.track_event(mfa_event_name, properties) if result.success? handle_valid_otp @@ -62,10 +63,20 @@ def phone user_session[:unconfirmed_phone] end + def sanitized_otp_code + form_params[:code].strip + end + def form_params params.permit(:code) end + def mfa_event_name + return Analytics::MULTI_FACTOR_AUTH_SETUP if context == 'confirmation' + + Analytics::MULTI_FACTOR_AUTH + end + def analytics_properties { context: context, diff --git a/app/controllers/users/piv_cac_authentication_setup_controller.rb b/app/controllers/users/piv_cac_authentication_setup_controller.rb index 31395b8713c..bf50e1d9c85 100644 --- a/app/controllers/users/piv_cac_authentication_setup_controller.rb +++ b/app/controllers/users/piv_cac_authentication_setup_controller.rb @@ -53,7 +53,7 @@ def two_factor_enabled? def process_piv_cac_setup result = user_piv_cac_form.submit - analytics.track_event(Analytics::USER_REGISTRATION_PIV_CAC_ENABLED, result.to_h) + analytics.track_event(Analytics::MULTI_FACTOR_AUTH_SETUP, result.to_h) if result.success? process_valid_submission else diff --git a/app/controllers/users/totp_setup_controller.rb b/app/controllers/users/totp_setup_controller.rb index ce49317be2d..006936e0ab1 100644 --- a/app/controllers/users/totp_setup_controller.rb +++ b/app/controllers/users/totp_setup_controller.rb @@ -16,7 +16,7 @@ def new def confirm result = TotpSetupForm.new(current_user, new_totp_secret, params[:code].strip).submit - analytics.track_event(Analytics::TOTP_SETUP, result.to_h) + analytics.track_event(Analytics::MULTI_FACTOR_AUTH_SETUP, result.to_h) if result.success? process_valid_code diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index 24ee92ad16f..0a84bcfc580 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/app/controllers/users/webauthn_setup_controller.rb @@ -14,7 +14,7 @@ def new def confirm form = WebauthnSetupForm.new(current_user, user_session) result = form.submit(request.protocol, params) - analytics.track_event(Analytics::WEBAUTHN_SETUP_SUBMITTED, result.to_h) + analytics.track_event(Analytics::MULTI_FACTOR_AUTH_SETUP, result.to_h) if result.success? process_valid_webauthn else diff --git a/app/forms/totp_setup_form.rb b/app/forms/totp_setup_form.rb index f895be2385f..8fcdd7d6e0a 100644 --- a/app/forms/totp_setup_form.rb +++ b/app/forms/totp_setup_form.rb @@ -29,6 +29,9 @@ def process_valid_submission end def extra_analytics_attributes - { totp_secret_present: secret.present? } + { + totp_secret_present: secret.present?, + multi_factor_auth_method: 'totp', + } end end diff --git a/app/forms/user_piv_cac_setup_form.rb b/app/forms/user_piv_cac_setup_form.rb index 52e98dd8d3c..f6a6b897c4c 100644 --- a/app/forms/user_piv_cac_setup_form.rb +++ b/app/forms/user_piv_cac_setup_form.rb @@ -10,7 +10,11 @@ class UserPivCacSetupForm def submit success = valid? && valid_token? - FormResponse.new(success: success && process_valid_submission, errors: {}) + FormResponse.new( + success: success && process_valid_submission, + errors: {}, + extra: extra_analytics_attributes + ) end private @@ -76,4 +80,10 @@ def user_has_no_piv_cac true end end + + def extra_analytics_attributes + { + multi_factor_auth_method: 'piv_cac', + } + end end diff --git a/app/forms/webauthn_setup_form.rb b/app/forms/webauthn_setup_form.rb index 776e2498a04..1b6b6f33920 100644 --- a/app/forms/webauthn_setup_form.rb +++ b/app/forms/webauthn_setup_form.rb @@ -82,6 +82,9 @@ def create_user_event end def extra_analytics_attributes - { mfa_method_counts: MfaContext.new(user).enabled_two_factor_configuration_counts_hash } + { + mfa_method_counts: MfaContext.new(user).enabled_two_factor_configuration_counts_hash, + multi_factor_auth_method: 'webauthn', + } end end diff --git a/app/services/analytics.rb b/app/services/analytics.rb index fbd816d7d66..d096f4fbd65 100644 --- a/app/services/analytics.rb +++ b/app/services/analytics.rb @@ -100,6 +100,7 @@ def browser MULTI_FACTOR_AUTH_OPTION_LIST_VISIT = 'Multi-Factor Authentication: option list visited'.freeze MULTI_FACTOR_AUTH_PHONE_SETUP = 'Multi-Factor Authentication: phone setup'.freeze MULTI_FACTOR_AUTH_MAX_SENDS = 'Multi-Factor Authentication: max otp sends reached'.freeze + MULTI_FACTOR_AUTH_SETUP = 'Multi-Factor Authentication Setup'.freeze OPENID_CONNECT_BEARER_TOKEN = 'OpenID Connect: bearer token authentication'.freeze OPENID_CONNECT_REQUEST_AUTHORIZATION = 'OpenID Connect: authorization request'.freeze OPENID_CONNECT_TOKEN = 'OpenID Connect: token'.freeze @@ -121,7 +122,6 @@ def browser SAML_AUTH = 'SAML Auth'.freeze SESSION_TIMED_OUT = 'Session Timed Out'.freeze SIGN_IN_PAGE_VISIT = 'Sign in page visited'.freeze - TOTP_SETUP = 'TOTP Setup'.freeze TOTP_SETUP_VISIT = 'TOTP Setup Visited'.freeze TOTP_USER_DISABLED = 'TOTP: User Disabled TOTP'.freeze TWILIO_PHONE_VALIDATION_FAILED = 'Twilio Phone Validation Failed'.freeze @@ -139,10 +139,8 @@ def browser USER_REGISTRATION_PHONE_SETUP_VISIT = 'User Registration: phone setup visited'.freeze USER_REGISTRATION_PERSONAL_KEY_VISIT = 'User Registration: personal key visited'.freeze USER_REGISTRATION_PIV_CAC_DISABLED = 'User Registration: piv cac disabled'.freeze - USER_REGISTRATION_PIV_CAC_ENABLED = 'User Registration: piv cac enabled'.freeze USER_REGISTRATION_PIV_CAC_SETUP_VISIT = 'User Registration: piv cac setup visited'.freeze WEBAUTHN_DELETED = 'WebAuthn Deleted'.freeze WEBAUTHN_SETUP_VISIT = 'WebAuthn Setup Visited'.freeze - WEBAUTHN_SETUP_SUBMITTED = 'WebAuthn Setup Submitted'.freeze # rubocop:enable Metrics/LineLength end diff --git a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb index 2ea0bf80d33..0f03e440a64 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -307,7 +307,7 @@ } expect(@analytics).to have_received(:track_event). - with(Analytics::MULTI_FACTOR_AUTH, properties) + with(Analytics::MULTI_FACTOR_AUTH_SETUP, properties) expect(subject).to have_received(:create_user_event).with(:phone_changed) expect(subject).to have_received(:create_user_event).exactly(:once) subject.current_user.email_addresses.each do |email_address| @@ -352,7 +352,7 @@ } expect(@analytics).to have_received(:track_event). - with(Analytics::MULTI_FACTOR_AUTH, properties) + with(Analytics::MULTI_FACTOR_AUTH_SETUP, properties) end end end @@ -388,7 +388,7 @@ } expect(@analytics).to have_received(:track_event). - with(Analytics::MULTI_FACTOR_AUTH, properties) + with(Analytics::MULTI_FACTOR_AUTH_SETUP, properties) expect(subject).to have_received(:create_user_event).with(:phone_confirmed) expect(subject).to have_received(:create_user_event).exactly(:once) diff --git a/spec/controllers/users/totp_setup_controller_spec.rb b/spec/controllers/users/totp_setup_controller_spec.rb index df773dd2d08..fc3b0a76e16 100644 --- a/spec/controllers/users/totp_setup_controller_spec.rb +++ b/spec/controllers/users/totp_setup_controller_spec.rb @@ -94,8 +94,8 @@ stub_sign_in(user) stub_analytics allow(@analytics).to receive(:track_event) + subject.user_session[:new_totp_secret] = 'abcdehij' - get :new patch :confirm, params: { code: 123 } end @@ -108,30 +108,24 @@ success: false, errors: {}, totp_secret_present: true, + multi_factor_auth_method: 'totp', } - expect(@analytics).to have_received(:track_event).with(Analytics::TOTP_SETUP, result) + + expect(@analytics).to have_received(:track_event). + with(Analytics::MULTI_FACTOR_AUTH_SETUP, result) end end context 'when user presents correct code' do before do user = build(:user, personal_key: 'ABCD-DEFG-HIJK-LMNO') + secret = ROTP::Base32.random_base32 stub_sign_in(user) stub_analytics allow(@analytics).to receive(:track_event) + subject.user_session[:new_totp_secret] = secret - code = '123455' - totp_secret = 'abdef' - subject.user_session[:new_totp_secret] = totp_secret - form = instance_double(TotpSetupForm) - - allow(TotpSetupForm).to receive(:new). - with(subject.current_user, totp_secret, code).and_return(form) - response = FormResponse.new(success: true, errors: {}) - allow(form).to receive(:submit).and_return(response) - - get :new - patch :confirm, params: { code: code } + patch :confirm, params: { code: generate_totp_code(secret) } end it 'redirects to account_path with a success message' do @@ -142,8 +136,12 @@ result = { success: true, errors: {}, + totp_secret_present: true, + multi_factor_auth_method: 'totp', } - expect(@analytics).to have_received(:track_event).with(Analytics::TOTP_SETUP, result) + + expect(@analytics).to have_received(:track_event). + with(Analytics::MULTI_FACTOR_AUTH_SETUP, result) end end end @@ -154,8 +152,8 @@ stub_sign_in_before_2fa stub_analytics allow(@analytics).to receive(:track_event) + subject.user_session[:new_totp_secret] = 'abcdehij' - get :new patch :confirm, params: { code: 123 } end @@ -168,29 +166,22 @@ success: false, errors: {}, totp_secret_present: true, + multi_factor_auth_method: 'totp', } - expect(@analytics).to have_received(:track_event).with(Analytics::TOTP_SETUP, result) + expect(@analytics).to have_received(:track_event). + with(Analytics::MULTI_FACTOR_AUTH_SETUP, result) end end context 'when user presents correct code' do before do + secret = ROTP::Base32.random_base32 stub_sign_in_before_2fa stub_analytics allow(@analytics).to receive(:track_event) + subject.user_session[:new_totp_secret] = secret - code = '123455' - totp_secret = 'abdef' - subject.user_session[:new_totp_secret] = totp_secret - form = instance_double(TotpSetupForm) - - allow(TotpSetupForm).to receive(:new). - with(subject.current_user, totp_secret, code).and_return(form) - response = FormResponse.new(success: true, errors: {}) - allow(form).to receive(:submit).and_return(response) - - get :new - patch :confirm, params: { code: code } + patch :confirm, params: { code: generate_totp_code(secret) } end it 'redirects to personal key page with a success message' do @@ -201,8 +192,12 @@ result = { success: true, errors: {}, + totp_secret_present: true, + multi_factor_auth_method: 'totp', } - expect(@analytics).to have_received(:track_event).with(Analytics::TOTP_SETUP, result) + + expect(@analytics).to have_received(:track_event). + with(Analytics::MULTI_FACTOR_AUTH_SETUP, result) end end @@ -224,8 +219,11 @@ success: false, errors: {}, totp_secret_present: false, + multi_factor_auth_method: 'totp', } - expect(@analytics).to have_received(:track_event).with(Analytics::TOTP_SETUP, result) + + expect(@analytics).to have_received(:track_event). + with(Analytics::MULTI_FACTOR_AUTH_SETUP, result) end end end diff --git a/spec/controllers/users/webauthn_setup_controller_spec.rb b/spec/controllers/users/webauthn_setup_controller_spec.rb index 39b4cc72bdb..6e99a3a9110 100644 --- a/spec/controllers/users/webauthn_setup_controller_spec.rb +++ b/spec/controllers/users/webauthn_setup_controller_spec.rb @@ -88,9 +88,14 @@ end it 'tracks the submission' do - result = { success: true, errors: {}, mfa_method_counts: { auth_app: 1, phone: 1 } } + result = { + success: true, + errors: {}, + mfa_method_counts: { auth_app: 1, phone: 1 }, + multi_factor_auth_method: 'webauthn', + } expect(@analytics).to receive(:track_event). - with(Analytics::WEBAUTHN_SETUP_SUBMITTED, result) + with(Analytics::MULTI_FACTOR_AUTH_SETUP, result) patch :confirm, params: params end diff --git a/spec/forms/totp_setup_form_spec.rb b/spec/forms/totp_setup_form_spec.rb index 7c9693f1c34..7d82658f786 100644 --- a/spec/forms/totp_setup_form_spec.rb +++ b/spec/forms/totp_setup_form_spec.rb @@ -10,9 +10,13 @@ it 'returns FormResponse with success: true' do form = TotpSetupForm.new(user, secret, code) result = instance_double(FormResponse) + extra = { + totp_secret_present: true, + multi_factor_auth_method: 'totp', + } expect(FormResponse).to receive(:new). - with(success: true, errors: {}, extra: { totp_secret_present: true }).and_return(result) + with(success: true, errors: {}, extra: extra).and_return(result) expect(Event).to receive(:create). with(user_id: user.id, event_type: :authenticator_enabled) expect(form.submit).to eq result @@ -24,9 +28,13 @@ it 'returns FormResponse with success: false' do form = TotpSetupForm.new(user, secret, 'kode') result = instance_double(FormResponse) + extra = { + totp_secret_present: true, + multi_factor_auth_method: 'totp', + } expect(FormResponse).to receive(:new). - with(success: false, errors: {}, extra: { totp_secret_present: true }).and_return(result) + with(success: false, errors: {}, extra: extra).and_return(result) expect(Event).to_not receive(:create) expect(form.submit).to eq result expect(user.reload.totp_enabled?).to eq false @@ -39,9 +47,13 @@ it 'returns FormResponse with success: false' do form = TotpSetupForm.new(user, nil, 'kode') result = instance_double(FormResponse) + extra = { + totp_secret_present: false, + multi_factor_auth_method: 'totp', + } expect(FormResponse).to receive(:new). - with(success: false, errors: {}, extra: { totp_secret_present: false }).and_return(result) + with(success: false, errors: {}, extra: extra).and_return(result) expect(Event).to_not receive(:create) expect(form.submit).to eq result expect(user.reload.totp_enabled?).to eq false diff --git a/spec/forms/user_piv_cac_setup_form_spec.rb b/spec/forms/user_piv_cac_setup_form_spec.rb index ec5be960b35..b8e7427d12c 100644 --- a/spec/forms/user_piv_cac_setup_form_spec.rb +++ b/spec/forms/user_piv_cac_setup_form_spec.rb @@ -25,9 +25,10 @@ it 'returns FormResponse with success: true' do result = instance_double(FormResponse) + extra = { multi_factor_auth_method: 'piv_cac' } expect(FormResponse).to receive(:new). - with(success: true, errors: {}).and_return(result) + with(success: true, errors: {}, extra: extra).and_return(result) expect(Event).to receive(:create). with(user_id: user.id, event_type: :piv_cac_enabled) expect(form.submit).to eq result @@ -41,9 +42,10 @@ it 'returns FormResponse with success: false' do result = instance_double(FormResponse) + extra = { multi_factor_auth_method: 'piv_cac' } expect(FormResponse).to receive(:new). - with(success: false, errors: {}).and_return(result) + with(success: false, errors: {}, extra: extra).and_return(result) expect(Event).to_not receive(:create) expect(form.submit).to eq result expect(TwoFactorAuthentication::PivCacPolicy.new(user.reload).enabled?).to eq true @@ -57,9 +59,10 @@ it 'returns FormResponse with success: false' do result = instance_double(FormResponse) + extra = { multi_factor_auth_method: 'piv_cac' } expect(FormResponse).to receive(:new). - with(success: false, errors: {}).and_return(result) + with(success: false, errors: {}, extra: extra).and_return(result) expect(Event).to_not receive(:create) expect(form.submit).to eq result expect(TwoFactorAuthentication::PivCacPolicy.new(user.reload).enabled?).to eq false @@ -73,9 +76,10 @@ allow(user).to receive(:save!).and_raise(PG::UniqueViolation) result = instance_double(FormResponse) + extra = { multi_factor_auth_method: 'piv_cac' } expect(FormResponse).to receive(:new). - with(success: false, errors: {}).and_return(result) + with(success: false, errors: {}, extra: extra).and_return(result) expect(Event).to_not receive(:create) expect(form.submit).to eq result expect(TwoFactorAuthentication::PivCacPolicy.new(user.reload).enabled?).to eq false @@ -93,9 +97,10 @@ it 'returns FormResponse with success: false' do result = instance_double(FormResponse) + extra = { multi_factor_auth_method: 'piv_cac' } expect(FormResponse).to receive(:new). - with(success: false, errors: {}).and_return(result) + with(success: false, errors: {}, extra: extra).and_return(result) expect(Event).to_not receive(:create) expect(form.submit).to eq result expect(TwoFactorAuthentication::PivCacPolicy.new(user.reload).enabled?).to eq false @@ -112,9 +117,10 @@ it 'returns FormResponse with success: false' do result = instance_double(FormResponse) + extra = { multi_factor_auth_method: 'piv_cac' } expect(FormResponse).to receive(:new). - with(success: false, errors: {}).and_return(result) + with(success: false, errors: {}, extra: extra).and_return(result) expect(Event).to_not receive(:create) expect(form.submit).to eq result expect(form.error_type).to eq 'token.invalid' @@ -126,9 +132,10 @@ it 'returns FormResponse with success: false' do result = instance_double(FormResponse) + extra = { multi_factor_auth_method: 'piv_cac' } expect(FormResponse).to receive(:new). - with(success: false, errors: {}).and_return(result) + with(success: false, errors: {}, extra: extra).and_return(result) expect(Event).to_not receive(:create) expect(form.submit).to eq result expect(TwoFactorAuthentication::PivCacPolicy.new(user.reload).enabled?).to eq false diff --git a/spec/forms/webauthn_setup_form_spec.rb b/spec/forms/webauthn_setup_form_spec.rb index f13a45d7256..fd73b2680d9 100644 --- a/spec/forms/webauthn_setup_form_spec.rb +++ b/spec/forms/webauthn_setup_form_spec.rb @@ -17,7 +17,10 @@ client_data_json: client_data_json, name: 'mykey', } - extra_attributes = { mfa_method_counts: { webauthn: 1 } } + extra_attributes = { + mfa_method_counts: { webauthn: 1 }, + multi_factor_auth_method: 'webauthn', + } expect(FormResponse).to receive(:new). with(success: true, errors: {}, extra: extra_attributes).and_return(result) @@ -33,7 +36,10 @@ client_data_json: client_data_json, name: 'mykey', } - extra_attributes = { mfa_method_counts: {} } + extra_attributes = { + mfa_method_counts: {}, + multi_factor_auth_method: 'webauthn', + } expect(FormResponse).to receive(:new). with(success: false, errors: {}, extra: extra_attributes).and_return(result) @@ -50,7 +56,10 @@ client_data_json: client_data_json, name: 'mykey', } - extra_attributes = { mfa_method_counts: {} } + extra_attributes = { + mfa_method_counts: {}, + multi_factor_auth_method: 'webauthn', + } expect(FormResponse).to receive(:new). with(success: false, extra: extra_attributes, errors: