From d9abb97ccfafdb91ba905ed33454b3bf74fc02d8 Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Thu, 16 Nov 2023 08:10:00 -0500 Subject: [PATCH 01/12] LG-11553 Remove recovery PII re-encryption from `PersonalKeyVerificationController` (#9602) The `PersonalKeyVerificationController` is used to verify a personal key as an MFA method and allow a user to sign in. When this is done a new personal key is issued. This controller had code for re-encrypting the users profile with the newly issued personal key. However, a user with an active profile was never able to reach this path. The `check_personal_key_enabled` calls `TwoFactorAuthentication::PersonalKeyPolicy#enabled?`. This method returns false if the user has any profiles. Since this code path is unreachable this commit removes it. I was not able to find any tests covering this re-encryption behavior. [skip changelog] --- .../personal_key_verification_controller.rb | 30 +------------------ app/services/analytics_events.rb | 7 ----- 2 files changed, 1 insertion(+), 36 deletions(-) diff --git a/app/controllers/two_factor_authentication/personal_key_verification_controller.rb b/app/controllers/two_factor_authentication/personal_key_verification_controller.rb index 83dfd1d788a..9765ee21eef 100644 --- a/app/controllers/two_factor_authentication/personal_key_verification_controller.rb +++ b/app/controllers/two_factor_authentication/personal_key_verification_controller.rb @@ -45,7 +45,7 @@ def handle_result(result) if result.success? _event, disavowal_token = create_user_event_with_disavowal(:personal_key_used) alert_user_about_personal_key_sign_in(disavowal_token) - generate_new_personal_key_for_verified_users_otherwise_retire_the_key_and_ensure_two_mfa + remove_personal_key handle_valid_otp else handle_invalid_otp(context: context, type: 'personal_key') @@ -57,16 +57,6 @@ def alert_user_about_personal_key_sign_in(disavowal_token) analytics.personal_key_alert_about_sign_in(**response.to_h) end - def generate_new_personal_key_for_verified_users_otherwise_retire_the_key_and_ensure_two_mfa - if password_reset_profile.present? - re_encrypt_profile_recovery_pii - elsif current_user.identity_verified? - user_session[:personal_key] = PersonalKeyGenerator.new(current_user).create - else - remove_personal_key - end - end - def remove_personal_key # for now we will regenerate a key and not show it to them so retire personal key page shows current_user.personal_key = PersonalKeyGenerator.new(current_user).create @@ -74,28 +64,10 @@ def remove_personal_key user_session.delete(:personal_key) end - def re_encrypt_profile_recovery_pii - analytics.personal_key_reactivation_sign_in - Pii::ReEncryptor.new(pii: pii, profile: password_reset_profile).perform - user_session[:personal_key] = password_reset_profile.personal_key - end - - def password_reset_profile - @password_reset_profile ||= current_user.password_reset_profile - end - - def pii - @pii ||= password_reset_profile.recover_pii(normalized_personal_key) - end - def personal_key_param params[:personal_key_form][:personal_key] end - def normalized_personal_key - @personal_key_form.personal_key - end - def handle_valid_otp handle_valid_verification_for_authentication_context( auth_method: TwoFactorAuthenticatable::AuthMethod::PERSONAL_KEY, diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 89923d5a914..90f75fa6764 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -3781,13 +3781,6 @@ def personal_key_reactivation track_event('Personal key reactivation: Account reactivated with personal key') end - # Account reactivated with personal key as MFA - def personal_key_reactivation_sign_in - track_event( - 'Personal key reactivation: Account reactivated with personal key as MFA', - ) - end - # @param [Boolean] success # @param [Hash] errors # @param [Hash] pii_like_keypaths From 8aff3ea68ef3c9dd6e5871a3f6491e1e4761790d Mon Sep 17 00:00:00 2001 From: Osman Latif <109746710+olatifflexion@users.noreply.github.com> Date: Thu, 16 Nov 2023 11:19:14 -0600 Subject: [PATCH 02/12] LG-11573: Add RISC events for account suspension, account reinstatement (#9594) * LG-11573: Add RISC events for account suspension, account reinstatement changelog: Internal, User suspension, Add RISC events for user suspension * feedback * feedback * feedback name changed --- app/models/user.rb | 8 +++++ .../account_disabled_event.rb | 31 +++++++++++++++++ .../account_enabled_event.rb | 18 ++++++++++ spec/models/user_spec.rb | 16 +++++++++ .../account_disabled_event_spec.rb | 34 +++++++++++++++++++ .../account_enabled_event_spec.rb | 33 ++++++++++++++++++ 6 files changed, 140 insertions(+) create mode 100644 app/services/push_notification/account_disabled_event.rb create mode 100644 app/services/push_notification/account_enabled_event.rb create mode 100644 spec/services/push_notification/account_disabled_event_spec.rb create mode 100644 spec/services/push_notification/account_enabled_event_spec.rb diff --git a/app/models/user.rb b/app/models/user.rb index e4c4f58391b..1c17fce179d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -122,6 +122,10 @@ def suspend! OutOfBandSessionAccessor.new(unique_session_id).destroy if unique_session_id update!(suspended_at: Time.zone.now, unique_session_id: nil) analytics.user_suspended(success: true) + + event = PushNotification::AccountDisabledEvent.new(user: self) + PushNotification::HttpPush.deliver(event) + email_addresses.map do |email_address| SuspendedEmail.create_from_email_address!(email_address) end @@ -134,6 +138,10 @@ def reinstate! end update!(reinstated_at: Time.zone.now) analytics.user_reinstated(success: true) + + event = PushNotification::AccountEnabledEvent.new(user: self) + PushNotification::HttpPush.deliver(event) + email_addresses.map do |email_address| SuspendedEmail.find_with_email(email_address.email)&.destroy end diff --git a/app/services/push_notification/account_disabled_event.rb b/app/services/push_notification/account_disabled_event.rb new file mode 100644 index 00000000000..d35ec8acd8a --- /dev/null +++ b/app/services/push_notification/account_disabled_event.rb @@ -0,0 +1,31 @@ +module PushNotification + # This is used for account suspension + class AccountDisabledEvent + EVENT_TYPE = 'https://schemas.openid.net/secevent/risc/event-type/account-disabled'.freeze + + attr_reader :user + + def initialize(user:) + @user = user + end + + def event_type + EVENT_TYPE + end + + def payload(iss_sub:) + { + subject: { + subject_type: 'iss-sub', + iss: Rails.application.routes.url_helpers.root_url, + sub: iss_sub, + }, + reason: 'account-suspension', + } + end + + def ==(other) + self.class == other.class && user == other.user + end + end +end diff --git a/app/services/push_notification/account_enabled_event.rb b/app/services/push_notification/account_enabled_event.rb new file mode 100644 index 00000000000..759d2436230 --- /dev/null +++ b/app/services/push_notification/account_enabled_event.rb @@ -0,0 +1,18 @@ +module PushNotification + # This is used for account reinstatement + class AccountEnabledEvent + include IssSubEvent + + EVENT_TYPE = 'https://schemas.openid.net/secevent/risc/event-type/account-enabled'.freeze + + attr_reader :user + + def initialize(user:) + @user = user + end + + def event_type + EVENT_TYPE + end + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index bfc0d2b908b..a37d56cf47e 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -923,6 +923,14 @@ user.suspend! end + it 'send account disabled push event' do + expect(PushNotification::HttpPush).to receive(:deliver).once. + with(PushNotification::AccountDisabledEvent.new( + user: user, + )) + user.suspend! + end + it 'logs out the suspended user from the active session' do # Add information to session store to allow `exists?` check to work as desired OutOfBandSessionAccessor.new(mock_session_id).put_pii( @@ -985,6 +993,14 @@ user.reinstate! end + it 'send account enabled push event' do + expect(PushNotification::HttpPush).to receive(:deliver).once. + with(PushNotification::AccountEnabledEvent.new( + user: user, + )) + user.reinstate! + end + it 'raises an error if the user is not currently suspended' do user.suspended_at = nil expect(user.analytics).to receive(:user_reinstated).with( diff --git a/spec/services/push_notification/account_disabled_event_spec.rb b/spec/services/push_notification/account_disabled_event_spec.rb new file mode 100644 index 00000000000..99ceec32c3d --- /dev/null +++ b/spec/services/push_notification/account_disabled_event_spec.rb @@ -0,0 +1,34 @@ +require 'rails_helper' + +RSpec.describe PushNotification::AccountDisabledEvent do + include Rails.application.routes.url_helpers + + subject(:event) do + PushNotification::AccountDisabledEvent.new(user: user) + end + + let(:user) { build(:user) } + + describe '#event_type' do + it 'is the RISC event type' do + expect(event.event_type).to eq(PushNotification::AccountDisabledEvent::EVENT_TYPE) + end + end + + describe '#payload' do + let(:iss_sub) { SecureRandom.uuid } + + subject(:payload) { event.payload(iss_sub: iss_sub) } + + it 'is a subject with the provided iss_sub and reason' do + expect(payload).to eq( + subject: { + subject_type: 'iss-sub', + sub: iss_sub, + iss: root_url, + }, + reason: 'account-suspension', + ) + end + end +end diff --git a/spec/services/push_notification/account_enabled_event_spec.rb b/spec/services/push_notification/account_enabled_event_spec.rb new file mode 100644 index 00000000000..17be59943c1 --- /dev/null +++ b/spec/services/push_notification/account_enabled_event_spec.rb @@ -0,0 +1,33 @@ +require 'rails_helper' + +RSpec.describe PushNotification::AccountEnabledEvent do + include Rails.application.routes.url_helpers + + subject(:event) do + PushNotification::AccountEnabledEvent.new(user: user) + end + + let(:user) { build(:user) } + + describe '#event_type' do + it 'is the RISC event type' do + expect(event.event_type).to eq(PushNotification::AccountEnabledEvent::EVENT_TYPE) + end + end + + describe '#payload' do + let(:iss_sub) { SecureRandom.uuid } + + subject(:payload) { event.payload(iss_sub: iss_sub) } + + it 'is a subject with the provided iss_sub ' do + expect(payload).to eq( + subject: { + subject_type: 'iss-sub', + sub: iss_sub, + iss: root_url, + }, + ) + end + end +end From 05b9e581d36646974b8767ef755b9c516a1d0dd7 Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Thu, 16 Nov 2023 13:02:58 -0500 Subject: [PATCH 03/12] LG-11534 Load the active profile from the session on broken personal key (#9601) In #9509 we added the ability to specify which profile to fetch PII from when reading PII from the session. This commit uses the active profiles PII when encrypting recovery PII for the active profile when the active profile has a broken personal key. changelog: Internal, Pending and active profile, The active profile PII is fetched with the PII cacher when a user with a broken personal key on their active profile signs in. --- app/controllers/application_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index a22eddd567c..a3ebcfb457f 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -198,7 +198,7 @@ def fix_broken_personal_key_url if pii_unlocked cacher = Pii::Cacher.new(current_user, user_session) profile = current_user.active_profile - user_session[:personal_key] = profile.encrypt_recovery_pii(cacher.fetch) + user_session[:personal_key] = profile.encrypt_recovery_pii(cacher.fetch(profile.id)) profile.save! analytics.broken_personal_key_regenerated From 82cd349989eddebcfd640535dfa5b14e8e0fee4f Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 16 Nov 2023 14:31:39 -0500 Subject: [PATCH 04/12] Restructure analytics error_details as hash (#9572) changelog: Internal, Analytics, Adjust format of analytics logging to improve querying support --- .../concerns/idv/verify_info_concern.rb | 11 +- app/controllers/concerns/idv_step_concern.rb | 5 +- .../users/verify_personal_key_controller.rb | 6 +- app/forms/idv/doc_pii_form.rb | 2 +- app/services/form_response.rb | 4 +- app/services/idv/flows/in_person_flow.rb | 5 +- app/services/irs_attempts_api/tracker.rb | 2 +- .../password_reset_token_validator.rb | 3 +- .../account_reset/cancel_controller_spec.rb | 6 +- .../delete_account_controller_spec.rb | 14 +-- .../idv/by_mail/enter_code_controller_spec.rb | 9 +- .../idv/image_uploads_controller_spec.rb | 12 +- .../idv/in_person/ssn_controller_spec.rb | 7 +- spec/controllers/idv/phone_controller_spec.rb | 16 +-- spec/controllers/idv/ssn_controller_spec.rb | 2 +- spec/controllers/saml_idp_controller_spec.rb | 10 +- .../email_confirmations_controller_spec.rb | 14 +-- .../sign_up/passwords_controller_spec.rb | 105 +++++++++--------- .../sign_up/registrations_controller_spec.rb | 2 +- .../otp_verification_controller_spec.rb | 6 +- ...rsonal_key_verification_controller_spec.rb | 2 +- .../webauthn_verification_controller_spec.rb | 12 +- .../users/edit_phone_controller_spec.rb | 2 +- .../users/passwords_controller_spec.rb | 19 ++-- .../users/phone_setup_controller_spec.rb | 8 +- .../users/reset_passwords_controller_spec.rb | 58 +++++----- .../users/totp_setup_controller_spec.rb | 2 +- .../verify_personal_key_controller_spec.rb | 10 +- .../users/webauthn_setup_controller_spec.rb | 5 +- spec/forms/idv/phone_form_spec.rb | 11 +- .../openid_connect_authorize_form_spec.rb | 2 +- spec/forms/otp_verification_form_spec.rb | 10 +- spec/forms/totp_setup_form_spec.rb | 4 +- spec/forms/webauthn_setup_form_spec.rb | 5 +- spec/forms/webauthn_verification_form_spec.rb | 24 ++-- spec/services/form_response_spec.rb | 48 +++++++- spec/support/doc_pii_helper.rb | 29 +++-- spec/support/fake_analytics.rb | 2 +- spec/support/fake_attempts_tracker.rb | 2 +- 39 files changed, 271 insertions(+), 225 deletions(-) diff --git a/app/controllers/concerns/idv/verify_info_concern.rb b/app/controllers/concerns/idv/verify_info_concern.rb index f361f25999d..2f40ce2daaf 100644 --- a/app/controllers/concerns/idv/verify_info_concern.rb +++ b/app/controllers/concerns/idv/verify_info_concern.rb @@ -192,9 +192,14 @@ def async_state_done(current_async_state) extra: { address_edited: !!idv_session.address_edited, address_line2_present: !pii[:address2].blank?, - pii_like_keypaths: [[:errors, :ssn], [:response_body, :first_name], - [:same_address_as_id], - [:state_id, :state_id_jurisdiction]], + pii_like_keypaths: [ + [:errors, :ssn], + [:proofing_results, :context, :stages, :resolution, :errors, :ssn], + [:proofing_results, :context, :stages, :residential_address, :errors, :ssn], + [:proofing_results, :context, :stages, :threatmetrix, :response_body, :first_name], + [:same_address_as_id], + [:proofing_results, :context, :stages, :state_id, :state_id_jurisdiction], + ], }, ) log_idv_verification_submitted_event( diff --git a/app/controllers/concerns/idv_step_concern.rb b/app/controllers/concerns/idv_step_concern.rb index 3ace1189618..0635ca4c020 100644 --- a/app/controllers/concerns/idv_step_concern.rb +++ b/app/controllers/concerns/idv_step_concern.rb @@ -110,7 +110,10 @@ def confirm_address_step_complete def extra_analytics_properties extra = { - pii_like_keypaths: [[:same_address_as_id], [:state_id, :state_id_jurisdiction]], + pii_like_keypaths: [ + [:same_address_as_id], + [:proofing_results, :context, :stages, :state_id, :state_id_jurisdiction], + ], } unless flow_session.dig(:pii_from_user, :same_address_as_id).nil? diff --git a/app/controllers/users/verify_personal_key_controller.rb b/app/controllers/users/verify_personal_key_controller.rb index 5c78899c73a..bd48cbe5168 100644 --- a/app/controllers/users/verify_personal_key_controller.rb +++ b/app/controllers/users/verify_personal_key_controller.rb @@ -29,7 +29,11 @@ def create analytics.personal_key_reactivation_submitted( **result.to_h, - pii_like_keypaths: [[:errors, :personal_key], [:error_details, :personal_key]], + pii_like_keypaths: [ + [:errors, :personal_key], + [:error_details, :personal_key], + [:error_details, :personal_key, :personal_key], + ], ) irs_attempts_api_tracker.personal_key_reactivation_submitted( success: result.success?, diff --git a/app/forms/idv/doc_pii_form.rb b/app/forms/idv/doc_pii_form.rb index 3616d2a8b94..e9eb97c01fc 100644 --- a/app/forms/idv/doc_pii_form.rb +++ b/app/forms/idv/doc_pii_form.rb @@ -49,10 +49,10 @@ def submit def self.pii_like_keypaths keypaths = [[:pii]] attrs = %i[name dob dob_min_age address1 state zipcode jurisdiction] - keypaths << attrs attrs.each do |k| keypaths << [:errors, k] keypaths << [:error_details, k] + keypaths << [:error_details, k, k] end keypaths end diff --git a/app/services/form_response.rb b/app/services/form_response.rb index 1804a571d26..e7947483ffd 100644 --- a/app/services/form_response.rb +++ b/app/services/form_response.rb @@ -56,7 +56,9 @@ def merge_arrays(_key, first, second) end def flatten_details(details) - details.transform_values { |errors| errors.pluck(:error) } + details.transform_values do |errors| + errors.map { |error| error[:type] || error[:error] }.index_with(true) + end end attr_reader :success diff --git a/app/services/idv/flows/in_person_flow.rb b/app/services/idv/flows/in_person_flow.rb index 76827bf6ddc..31f1626cc1d 100644 --- a/app/services/idv/flows/in_person_flow.rb +++ b/app/services/idv/flows/in_person_flow.rb @@ -47,7 +47,10 @@ def self.session_idv(session) def extra_analytics_properties extra = { - pii_like_keypaths: [[:same_address_as_id], [:state_id, :state_id_jurisdiction]], + pii_like_keypaths: [ + [:same_address_as_id], + [:proofing_results, :context, :stages, :state_id, :state_id_jurisdiction], + ], } unless @flow_session[:pii_from_user]&.[](:same_address_as_id).nil? extra[:same_address_as_id] = diff --git a/app/services/irs_attempts_api/tracker.rb b/app/services/irs_attempts_api/tracker.rb index 463ec760cdb..0cf1c7f3f1d 100644 --- a/app/services/irs_attempts_api/tracker.rb +++ b/app/services/irs_attempts_api/tracker.rb @@ -6,7 +6,7 @@ def track_event(event_type, metadata = {}) end def parse_failure_reason(result) - return result.to_h[:error_details] || result.errors.presence + return result.to_h[:error_details]&.transform_values(&:keys) || result.errors.presence end end end diff --git a/app/services/password_reset_token_validator.rb b/app/services/password_reset_token_validator.rb index 3ffce440c73..6b410353de8 100644 --- a/app/services/password_reset_token_validator.rb +++ b/app/services/password_reset_token_validator.rb @@ -17,6 +17,7 @@ def submit attr_accessor :user def valid_token - errors.add(:user, 'token_expired', type: :user) unless user.reset_password_period_valid? + return if user.reset_password_period_valid? + errors.add(:user, 'token_expired', type: :token_expired) end end diff --git a/spec/controllers/account_reset/cancel_controller_spec.rb b/spec/controllers/account_reset/cancel_controller_spec.rb index 488f187ad25..41677ed6cd4 100644 --- a/spec/controllers/account_reset/cancel_controller_spec.rb +++ b/spec/controllers/account_reset/cancel_controller_spec.rb @@ -39,7 +39,7 @@ success: false, errors: { token: [t('errors.account_reset.cancel_token_invalid', app_name: APP_NAME)] }, error_details: { - token: [t('errors.account_reset.cancel_token_invalid', app_name: APP_NAME)], + token: { cancel_token_invalid: true }, }, user_id: 'anonymous-uuid', } @@ -55,7 +55,7 @@ analytics_hash = { success: false, errors: { token: [t('errors.account_reset.cancel_token_missing', app_name: APP_NAME)] }, - error_details: { token: [:blank] }, + error_details: { token: { blank: true } }, user_id: 'anonymous-uuid', } @@ -100,7 +100,7 @@ success: false, errors: { token: [t('errors.account_reset.cancel_token_invalid', app_name: APP_NAME)] }, error_details: { - token: [t('errors.account_reset.cancel_token_invalid', app_name: APP_NAME)], + token: { cancel_token_invalid: true }, }, } expect(@analytics).to receive(:track_event). diff --git a/spec/controllers/account_reset/delete_account_controller_spec.rb b/spec/controllers/account_reset/delete_account_controller_spec.rb index 1f02030e28b..4f147b4ddf9 100644 --- a/spec/controllers/account_reset/delete_account_controller_spec.rb +++ b/spec/controllers/account_reset/delete_account_controller_spec.rb @@ -41,7 +41,7 @@ user_id: 'anonymous-uuid', success: false, errors: invalid_token_error, - error_details: invalid_token_error, + error_details: { token: { granted_token_invalid: true } }, mfa_method_counts: {}, pii_like_keypaths: [[:mfa_method_counts, :phone]], account_age_in_days: 0, @@ -60,7 +60,7 @@ user_id: 'anonymous-uuid', success: false, errors: { token: [t('errors.account_reset.granted_token_missing', app_name: APP_NAME)] }, - error_details: { token: [:blank] }, + error_details: { token: { blank: true } }, mfa_method_counts: {}, pii_like_keypaths: [[:mfa_method_counts, :phone]], account_age_in_days: 0, @@ -86,9 +86,7 @@ user_id: user.uuid, success: false, errors: { token: [t('errors.account_reset.granted_token_expired', app_name: APP_NAME)] }, - error_details: { - token: [t('errors.account_reset.granted_token_expired', app_name: APP_NAME)], - }, + error_details: { token: { granted_token_expired: true } }, mfa_method_counts: {}, pii_like_keypaths: [[:mfa_method_counts, :phone]], account_age_in_days: 2, @@ -114,7 +112,7 @@ user_id: 'anonymous-uuid', success: false, errors: invalid_token_error, - error_details: invalid_token_error, + error_details: { token: { granted_token_invalid: true } }, } expect(@analytics).to receive(:track_event). with('Account Reset: granted token validation', properties) @@ -134,9 +132,7 @@ user_id: user.uuid, success: false, errors: { token: [t('errors.account_reset.granted_token_expired', app_name: APP_NAME)] }, - error_details: { - token: [t('errors.account_reset.granted_token_expired', app_name: APP_NAME)], - }, + error_details: { token: { granted_token_expired: true } }, } expect(@analytics).to receive(:track_event). with('Account Reset: granted token validation', properties) diff --git a/spec/controllers/idv/by_mail/enter_code_controller_spec.rb b/spec/controllers/idv/by_mail/enter_code_controller_spec.rb index f5bd87d0052..358609656dd 100644 --- a/spec/controllers/idv/by_mail/enter_code_controller_spec.rb +++ b/spec/controllers/idv/by_mail/enter_code_controller_spec.rb @@ -166,7 +166,6 @@ describe '#create' do let(:otp_code_error_message) { { otp: [t('errors.messages.confirmation_code_incorrect')] } } - let(:otp_code_incorrect) { { otp: [:confirmation_code_incorrect] } } let(:success_properties) { { success: true, failure_reason: nil } } subject(:action) do @@ -387,11 +386,11 @@ which_letter: nil, letter_count: 1, attempts: 1, - error_details: otp_code_incorrect, + error_details: { otp: { confirmation_code_incorrect: true } }, pii_like_keypaths: [[:errors, :otp], [:error_details, :otp]], ) expect(@irs_attempts_api_tracker).to receive(:idv_gpo_verification_submitted). - with(success: false, failure_reason: otp_code_incorrect) + with(success: false, failure_reason: { otp: [:confirmation_code_incorrect] }) action @@ -425,7 +424,7 @@ which_letter: nil, letter_count: 1, attempts: 1, - error_details: otp_code_incorrect, + error_details: { otp: { confirmation_code_incorrect: true } }, pii_like_keypaths: [[:errors, :otp], [:error_details, :otp]], } expect(@analytics).to receive(:track_event).with( @@ -474,7 +473,7 @@ which_letter: nil, letter_count: 1, attempts: 1, - error_details: otp_code_incorrect, + error_details: { otp: { confirmation_code_incorrect: true } }, pii_like_keypaths: [[:errors, :otp], [:error_details, :otp]], ).exactly(max_attempts - 1).times expect(@analytics).to receive(:track_event).with( diff --git a/spec/controllers/idv/image_uploads_controller_spec.rb b/spec/controllers/idv/image_uploads_controller_spec.rb index 7da85f798e8..cba3d2687b8 100644 --- a/spec/controllers/idv/image_uploads_controller_spec.rb +++ b/spec/controllers/idv/image_uploads_controller_spec.rb @@ -62,7 +62,7 @@ front: ['Please fill in this field.'], }, error_details: { - front: [:blank], + front: { blank: true }, }, user_id: user.uuid, attempts: 1, @@ -123,7 +123,7 @@ front: [I18n.t('doc_auth.errors.not_a_file')], }, error_details: { - front: [I18n.t('doc_auth.errors.not_a_file')], + front: { not_a_file: true }, }, user_id: user.uuid, attempts: 1, @@ -261,7 +261,7 @@ limit: [I18n.t('errors.doc_auth.rate_limited_heading')], }, error_details: { - limit: [I18n.t('errors.doc_auth.rate_limited_heading')], + limit: { rate_limited: true }, }, user_id: user.uuid, attempts: IdentityConfig.store.doc_auth_max_attempts, @@ -603,7 +603,7 @@ name: [I18n.t('doc_auth.errors.alerts.full_name_check')], }, error_details: { - name: [I18n.t('doc_auth.errors.alerts.full_name_check')], + name: { name: true }, }, attention_with_barcode: false, user_id: user.uuid, @@ -703,7 +703,7 @@ state: [I18n.t('doc_auth.errors.general.no_liveness')], }, error_details: { - state: [:wrong_length], + state: { wrong_length: true }, }, attention_with_barcode: false, user_id: user.uuid, @@ -803,7 +803,7 @@ dob: [I18n.t('doc_auth.errors.alerts.birth_date_checks')], }, error_details: { - dob: [I18n.t('doc_auth.errors.alerts.birth_date_checks')], + dob: { dob: true }, }, attention_with_barcode: false, user_id: user.uuid, diff --git a/spec/controllers/idv/in_person/ssn_controller_spec.rb b/spec/controllers/idv/in_person/ssn_controller_spec.rb index 62002508c24..0e622b3a339 100644 --- a/spec/controllers/idv/in_person/ssn_controller_spec.rb +++ b/spec/controllers/idv/in_person/ssn_controller_spec.rb @@ -75,7 +75,10 @@ irs_reproofing: false, step: 'ssn', same_address_as_id: true, - pii_like_keypaths: [[:same_address_as_id], [:state_id, :state_id_jurisdiction]], + pii_like_keypaths: [ + [:same_address_as_id], + [:proofing_results, :context, :stages, :state_id, :state_id_jurisdiction], + ], }.merge(ab_test_args) end @@ -199,7 +202,7 @@ errors: { ssn: ['Enter a nine-digit Social Security number'], }, - error_details: { ssn: [:invalid] }, + error_details: { ssn: { invalid: true } }, same_address_as_id: true, pii_like_keypaths: [[:same_address_as_id], [:errors, :ssn], [:error_details, :ssn]], }.merge(ab_test_args) diff --git a/spec/controllers/idv/phone_controller_spec.rb b/spec/controllers/idv/phone_controller_spec.rb index bee6b83d0ec..90923f27064 100644 --- a/spec/controllers/idv/phone_controller_spec.rb +++ b/spec/controllers/idv/phone_controller_spec.rb @@ -239,12 +239,6 @@ allow(subject).to receive(:ab_test_analytics_buckets).and_return(ab_test_args) end context 'when form is invalid' do - let(:improbable_phone_error) do - { - phone: [:improbable_phone], - otp_delivery_preference: [:inclusion], - } - end let(:improbable_phone_message) { t('errors.messages.improbable_phone') } let(:improbable_otp_message) { 'is not included in the list' } let(:improbable_phone_number) { '703' } @@ -285,7 +279,10 @@ expect(@irs_attempts_api_tracker).to receive(:idv_phone_submitted).with( success: false, phone_number: improbable_phone_number, - failure_reason: improbable_phone_error, + failure_reason: { + phone: [:improbable_phone], + otp_delivery_preference: [:inclusion], + }, ) put :create, params: improbable_phone_form @@ -296,7 +293,10 @@ phone: [improbable_phone_message], otp_delivery_preference: [improbable_otp_message], }, - error_details: improbable_phone_error, + error_details: { + phone: { improbable_phone: true }, + otp_delivery_preference: { inclusion: true }, + }, pii_like_keypaths: [[:errors, :phone], [:error_details, :phone]], country_code: nil, area_code: nil, diff --git a/spec/controllers/idv/ssn_controller_spec.rb b/spec/controllers/idv/ssn_controller_spec.rb index 5a7921a1086..60a251e7f80 100644 --- a/spec/controllers/idv/ssn_controller_spec.rb +++ b/spec/controllers/idv/ssn_controller_spec.rb @@ -208,7 +208,7 @@ errors: { ssn: [t('idv.errors.pattern_mismatch.ssn')], }, - error_details: { ssn: [:invalid] }, + error_details: { ssn: { invalid: true } }, pii_like_keypaths: [[:same_address_as_id], [:errors, :ssn], [:error_details, :ssn]], }.merge(ab_test_args) end diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index bbeb92bea08..d55eb8d4ff4 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -765,7 +765,7 @@ def name_id_version(format_urn) analytics_hash = { success: false, errors: { authn_context: [t('errors.messages.unauthorized_authn_context')] }, - error_details: { authn_context: [:unauthorized_authn_context] }, + error_details: { authn_context: { unauthorized_authn_context: true } }, nameid_format: Saml::Idp::Constants::NAME_ID_FORMAT_PERSISTENT, authn_context: ['http://idmanagement.gov/ns/assurance/loa/5'], authn_context_comparison: 'exact', @@ -984,7 +984,7 @@ def name_id_version(format_urn) analytics_hash = { success: false, errors: { service_provider: [t('errors.messages.unauthorized_service_provider')] }, - error_details: { service_provider: [:unauthorized_service_provider] }, + error_details: { service_provider: { unauthorized_service_provider: true } }, nameid_format: Saml::Idp::Constants::NAME_ID_FORMAT_PERSISTENT, authn_context: request_authn_contexts, authn_context_comparison: 'exact', @@ -1026,8 +1026,8 @@ def name_id_version(format_urn) authn_context: [t('errors.messages.unauthorized_authn_context')], }, error_details: { - authn_context: [:unauthorized_authn_context], - service_provider: [:unauthorized_service_provider], + authn_context: { unauthorized_authn_context: true }, + service_provider: { unauthorized_service_provider: true }, }, nameid_format: Saml::Idp::Constants::NAME_ID_FORMAT_PERSISTENT, authn_context: ['http://idmanagement.gov/ns/assurance/loa/5'], @@ -1417,7 +1417,7 @@ def name_id_version(format_urn) analytics_hash = { success: false, errors: { nameid_format: [t('errors.messages.unauthorized_nameid_format')] }, - error_details: { nameid_format: [:unauthorized_nameid_format] }, + error_details: { nameid_format: { unauthorized_nameid_format: true } }, nameid_format: Saml::Idp::Constants::NAME_ID_FORMAT_EMAIL, authn_context: request_authn_contexts, authn_context_comparison: 'exact', diff --git a/spec/controllers/sign_up/email_confirmations_controller_spec.rb b/spec/controllers/sign_up/email_confirmations_controller_spec.rb index 017e7fd7eef..f7ef77d7a6f 100644 --- a/spec/controllers/sign_up/email_confirmations_controller_spec.rb +++ b/spec/controllers/sign_up/email_confirmations_controller_spec.rb @@ -2,12 +2,10 @@ RSpec.describe SignUp::EmailConfirmationsController do describe '#create' do - let(:token_not_found_error) { { confirmation_token: [:not_found] } } - let(:token_expired_error) { { confirmation_token: [:expired] } } let(:analytics_token_error_hash) do { success: false, - error_details: token_not_found_error, + error_details: { confirmation_token: { not_found: true } }, errors: { confirmation_token: ['not found'] }, user_id: nil, } @@ -16,7 +14,7 @@ { email: nil, success: false, - failure_reason: token_not_found_error, + failure_reason: { confirmation_token: [:not_found] }, } end @@ -117,7 +115,7 @@ analytics_hash = { success: false, errors: { confirmation_token: [t('errors.messages.expired')] }, - error_details: token_expired_error, + error_details: { confirmation_token: { expired: true } }, user_id: email_address.user.uuid, } @@ -127,7 +125,7 @@ expect(@irs_attempts_api_tracker).to receive(:user_registration_email_confirmation).with( email: email_address.email, success: false, - failure_reason: token_expired_error, + failure_reason: { confirmation_token: [:expired] }, ) get :create, params: { confirmation_token: 'foo' } @@ -149,7 +147,7 @@ analytics_hash = { success: false, errors: { confirmation_token: [t('errors.messages.expired')] }, - error_details: token_expired_error, + error_details: { confirmation_token: { expired: true } }, user_id: user.uuid, } @@ -159,7 +157,7 @@ expect(@irs_attempts_api_tracker).to receive(:user_registration_email_confirmation).with( email: email_address.email, success: false, - failure_reason: token_expired_error, + failure_reason: { confirmation_token: [:expired] }, ) get :create, params: { confirmation_token: 'foo' } diff --git a/spec/controllers/sign_up/passwords_controller_spec.rb b/spec/controllers/sign_up/passwords_controller_spec.rb index bb7a1a1e975..c89b399fee1 100644 --- a/spec/controllers/sign_up/passwords_controller_spec.rb +++ b/spec/controllers/sign_up/passwords_controller_spec.rb @@ -61,15 +61,6 @@ context 'with an invalid password' do let!(:user) { create(:user, :unconfirmed, confirmation_token: token) } - let(:analytics_hash) do - { - success: false, - errors: errors, - error_details: error_details, - user_id: user.uuid, - request_id_present: false, - } - end before do stub_analytics @@ -79,70 +70,74 @@ context 'with a password that is too short' do let(:password) { 'NewVal' } let(:password_confirmation) { 'NewVal' } - let(:errors) do - { - password: - [t( - 'errors.attributes.password.too_short', - count: Devise.password_length.first, - )], - password_confirmation: - [I18n.t('errors.messages.too_short', count: Devise.password_length.first)], - } - end - let(:error_details) do - { - password: [:too_short], - password_confirmation: [:too_short], - } - end it 'tracks an invalid password event' do - expect(@analytics).to receive(:track_event). - with( - 'User Registration: Email Confirmation', - { errors: {}, error_details: nil, success: true, user_id: user.uuid }, - ) - expect(@analytics).to receive(:track_event). - with('Password Creation', analytics_hash) - expect(@irs_attempts_api_tracker).to receive(:user_registration_password_submitted). with( success: false, - failure_reason: error_details, + failure_reason: { + password: [:too_short], + password_confirmation: [:too_short], + }, ) expect(@irs_attempts_api_tracker).not_to receive(:user_registration_email_confirmation) subject + + expect(@analytics).to have_logged_event( + 'User Registration: Email Confirmation', + errors: {}, + error_details: nil, + success: true, + user_id: user.uuid, + ) + expect(@analytics).to have_logged_event( + 'Password Creation', + success: false, + errors: { + password: [ + t('errors.attributes.password.too_short', count: Devise.password_length.first), + ], + password_confirmation: [ + t('errors.messages.too_short', count: Devise.password_length.first), + ], + }, + error_details: { + password: { too_short: true }, + password_confirmation: { too_short: true }, + }, + user_id: user.uuid, + request_id_present: false, + ) end end context 'when password confirmation does not match' do let(:password) { 'NewVal!dPassw0rd' } let(:password_confirmation) { 'bad match password' } - let(:errors) do - { - password_confirmation: - [t('errors.messages.password_mismatch')], - } - end - let(:error_details) do - { - password_confirmation: [t('errors.messages.password_mismatch')], - } - end it 'tracks invalid password_confirmation error' do - expect(@analytics).to receive(:track_event). - with( - 'User Registration: Email Confirmation', - { errors: {}, error_details: nil, success: true, user_id: user.uuid }, - ) - - expect(@analytics).to receive(:track_event). - with('Password Creation', analytics_hash) - subject + + expect(@analytics).to have_logged_event( + 'User Registration: Email Confirmation', + errors: {}, + error_details: nil, + success: true, + user_id: user.uuid, + ) + expect(@analytics).to have_logged_event( + 'Password Creation', + success: false, + errors: { + password_confirmation: [t('errors.messages.password_mismatch')], + }, + error_details: { + password_confirmation: { mismatch: true }, + }, + user_id: user.uuid, + request_id_present: false, + ) end end end diff --git a/spec/controllers/sign_up/registrations_controller_spec.rb b/spec/controllers/sign_up/registrations_controller_spec.rb index 2a6fe362664..acc0056867d 100644 --- a/spec/controllers/sign_up/registrations_controller_spec.rb +++ b/spec/controllers/sign_up/registrations_controller_spec.rb @@ -156,7 +156,7 @@ success: false, rate_limited: false, errors: { email: [t('valid_email.validations.email.invalid')] }, - error_details: { email: [:invalid] }, + error_details: { email: { invalid: true } }, email_already_exists: false, user_id: 'anonymous-uuid', domain_name: 'invalid', 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 f62578349b0..71bea1af56f 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -133,7 +133,7 @@ properties = { success: false, - error_details: { code: [:wrong_length, 'incorrect'] }, + error_details: { code: { wrong_length: true, incorrect: true } }, confirmation_for_add_phone: false, context: 'authentication', multi_factor_auth_method: 'sms', @@ -204,7 +204,7 @@ properties = { success: false, - error_details: { code: [:wrong_length, 'incorrect'] }, + error_details: { code: { wrong_length: true, incorrect: true } }, confirmation_for_add_phone: false, context: 'authentication', multi_factor_auth_method: 'sms', @@ -546,7 +546,7 @@ properties = { success: false, errors: nil, - error_details: { code: [:wrong_length, 'incorrect'] }, + error_details: { code: { wrong_length: true, incorrect: true } }, confirmation_for_add_phone: true, context: 'confirmation', multi_factor_auth_method: 'sms', diff --git a/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb index aac99a3f104..36312e154b7 100644 --- a/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb @@ -158,7 +158,7 @@ properties = { success: false, errors: { personal_key: [t('errors.messages.personal_key_incorrect')] }, - error_details: { personal_key: [:personal_key_incorrect] }, + error_details: { personal_key: { personal_key_incorrect: true } }, multi_factor_auth_method: 'personal-key', multi_factor_auth_method_created_at: personal_key_generated_at.strftime('%s%L'), } diff --git a/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb index 5fd496581a1..d61ff7e8acf 100644 --- a/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb @@ -223,7 +223,7 @@ result = { context: 'authentication', multi_factor_auth_method: 'webauthn', success: false, - error_details: { authenticator_data: ['invalid_authenticator_data'] }, + error_details: { authenticator_data: { invalid_authenticator_data: true } }, webauthn_configuration_id: webauthn_configuration.id, multi_factor_auth_method_created_at: webauthn_configuration.created_at. strftime('%s%L') } @@ -280,11 +280,11 @@ expect(@analytics).to receive(:track_mfa_submit_event).with( success: false, error_details: { - authenticator_data: [:blank], - client_data_json: [:blank], - signature: [:blank], - webauthn_configuration: [:blank], - webauthn_error: [webauthn_error], + authenticator_data: { blank: true }, + client_data_json: { blank: true }, + signature: { blank: true }, + webauthn_configuration: { blank: true }, + webauthn_error: { webauthn_error: true }, }, context: UserSessionContext::AUTHENTICATION_CONTEXT, multi_factor_auth_method: 'webauthn_platform', diff --git a/spec/controllers/users/edit_phone_controller_spec.rb b/spec/controllers/users/edit_phone_controller_spec.rb index fb460dfbd84..c9df63102fd 100644 --- a/spec/controllers/users/edit_phone_controller_spec.rb +++ b/spec/controllers/users/edit_phone_controller_spec.rb @@ -38,7 +38,7 @@ attributes = { success: false, errors: hash_including(:delivery_preference), - error_details: { delivery_preference: [:inclusion] }, + error_details: { delivery_preference: { inclusion: true } }, delivery_preference: 'noise', make_default_number: true, phone_configuration_id: phone_configuration.id, diff --git a/spec/controllers/users/passwords_controller_spec.rb b/spec/controllers/users/passwords_controller_spec.rb index 40eb505c572..cc20f6fa3eb 100644 --- a/spec/controllers/users/passwords_controller_spec.rb +++ b/spec/controllers/users/passwords_controller_spec.rb @@ -132,14 +132,12 @@ end it 'renders edit' do - password_short_error = { - password: [:too_short], - password_confirmation: [:too_short], - } - expect(@irs_attempts_api_tracker).to receive(:logged_in_password_change).with( success: false, - failure_reason: password_short_error, + failure_reason: { + password: [:too_short], + password_confirmation: [:too_short], + }, ) patch :update, params: { update_user_password_form: params } @@ -159,7 +157,10 @@ count: Devise.password_length.first, )], }, - error_details: password_short_error, + 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, @@ -190,7 +191,7 @@ expect(@irs_attempts_api_tracker).to receive(:logged_in_password_change).with( success: false, failure_reason: { - password_confirmation: [t('errors.messages.password_mismatch')], + password_confirmation: [:mismatch], }, ) @@ -203,7 +204,7 @@ password_confirmation: [t('errors.messages.password_mismatch')], }, error_details: { - password_confirmation: [t('errors.messages.password_mismatch')], + password_confirmation: { mismatch: true }, }, pending_profile_present: false, active_profile_present: false, diff --git a/spec/controllers/users/phone_setup_controller_spec.rb b/spec/controllers/users/phone_setup_controller_spec.rb index 43334bbbfe5..b898b40c7b5 100644 --- a/spec/controllers/users/phone_setup_controller_spec.rb +++ b/spec/controllers/users/phone_setup_controller_spec.rb @@ -70,10 +70,10 @@ ], }, error_details: { - phone: [ - :improbable_phone, - t('two_factor_authentication.otp_delivery_preference.voice_unsupported', location: ''), - ], + phone: { + improbable_phone: true, + voice_unsupported: true, + }, }, otp_delivery_preference: 'sms', area_code: nil, diff --git a/spec/controllers/users/reset_passwords_controller_spec.rb b/spec/controllers/users/reset_passwords_controller_spec.rb index 09c557774cf..bfa3b91d94a 100644 --- a/spec/controllers/users/reset_passwords_controller_spec.rb +++ b/spec/controllers/users/reset_passwords_controller_spec.rb @@ -5,7 +5,6 @@ t('errors.attributes.password.too_short.other', count: Devise.password_length.first) end let(:success_properties) { { success: true, failure_reason: nil } } - let(:token_expired_error) { 'token_expired' } describe '#edit' do let(:user) { instance_double('User', uuid: '123') } let(:email_address) { instance_double('EmailAddress') } @@ -24,7 +23,6 @@ end context 'no user matches token' do - let(:user_blank_error) { { user: [:blank] } } let(:token) { 'foo' } before do session[:reset_password_token] = token @@ -33,7 +31,7 @@ { success: false, errors: { user: ['invalid_token'] }, - error_details: user_blank_error, + error_details: { user: { blank: true } }, user_id: nil, } end @@ -41,7 +39,7 @@ it 'redirects to page where user enters email for password reset token' do expect(@irs_attempts_api_tracker).to receive(:forgot_password_email_confirmed).with( success: false, - failure_reason: user_blank_error, + failure_reason: { user: [:blank] }, ) get :edit @@ -54,7 +52,6 @@ end context 'token expired' do - let(:user_token_error) { { user: [token_expired_error] } } let(:token) { 'foo' } before do session[:reset_password_token] = token @@ -62,8 +59,8 @@ let(:analytics_hash) do { success: false, - errors: user_token_error, - error_details: user_token_error, + errors: { user: ['token_expired'] }, + error_details: { user: { token_expired: true } }, user_id: '123', } end @@ -76,12 +73,11 @@ end context 'no user matches token' do - let(:user_blank_error) { { user: [:blank] } } let(:analytics_hash) do { success: false, errors: { user: ['invalid_token'] }, - error_details: user_blank_error, + error_details: { user: { blank: true } }, user_id: nil, } end @@ -93,7 +89,7 @@ it 'redirects to page where user enters email for password reset token' do expect(@irs_attempts_api_tracker).to receive(:forgot_password_email_confirmed).with( success: false, - failure_reason: user_blank_error, + failure_reason: { user: [:blank] }, ) get :edit @@ -106,12 +102,11 @@ end context 'token expired' do - let(:user_token_error) { { user: [token_expired_error] } } let(:analytics_hash) do { success: false, - errors: user_token_error, - error_details: user_token_error, + errors: { user: ['token_expired'] }, + error_details: { user: { token_expired: true } }, user_id: '123', } end @@ -125,7 +120,7 @@ it 'redirects to page where user enters email for password reset token' do expect(@irs_attempts_api_tracker).to receive(:forgot_password_email_confirmed).with( success: false, - failure_reason: user_token_error, + failure_reason: { user: [:token_expired] }, ) get :edit @@ -192,18 +187,7 @@ end describe '#update' do - let(:password_short_error) { { password: [:too_short] } } - - let(:password_token_error) { { reset_password_token: [token_expired_error] } } context 'user submits new password after token expires' do - let(:reset_password_error_details) do - { - **password_short_error, - password_confirmation: [:too_short], - **password_token_error, - } - end - it 'redirects to page where user enters email for password reset token' do stub_analytics stub_attempts_tracker @@ -211,7 +195,11 @@ expect(@irs_attempts_api_tracker).to receive(:forgot_password_new_password_submitted).with( success: false, - failure_reason: reset_password_error_details, + failure_reason: { + password: [:too_short], + password_confirmation: [:too_short], + reset_password_token: [:token_expired], + }, ) raw_reset_token, db_confirmation_token = @@ -240,9 +228,13 @@ 'errors.messages.too_short.other', count: Devise.password_length.first, )], - **password_token_error, + reset_password_token: ['token_expired'], + }, + error_details: { + password: { too_short: true }, + password_confirmation: { too_short: true }, + reset_password_token: { token_expired: true }, }, - error_details: reset_password_error_details, user_id: user.uuid, profile_deactivated: false, pending_profile_invalidated: false, @@ -287,8 +279,8 @@ )], }, error_details: { - password: [:too_short], - password_confirmation: [:too_short], + password: { too_short: true }, + password_confirmation: { too_short: true }, }, user_id: user.uuid, profile_deactivated: false, @@ -340,7 +332,7 @@ password_confirmation: [t('errors.messages.password_mismatch')], }, error_details: { - password_confirmation: [t('errors.messages.password_mismatch')], + password_confirmation: { mismatch: true }, }, user_id: user.uuid, profile_deactivated: false, @@ -353,7 +345,7 @@ expect(@irs_attempts_api_tracker).to receive(:forgot_password_new_password_submitted).with( success: false, failure_reason: { - password_confirmation: [t('errors.messages.password_mismatch')], + password_confirmation: [:mismatch], }, ) @@ -709,7 +701,7 @@ analytics_hash = { success: false, errors: { email: [t('valid_email.validations.email.invalid')] }, - error_details: { email: [:invalid] }, + error_details: { email: { invalid: true } }, user_id: 'nonexistent-uuid', confirmed: false, active_profile: false, diff --git a/spec/controllers/users/totp_setup_controller_spec.rb b/spec/controllers/users/totp_setup_controller_spec.rb index be785f60bd8..e0ef5196936 100644 --- a/spec/controllers/users/totp_setup_controller_spec.rb +++ b/spec/controllers/users/totp_setup_controller_spec.rb @@ -224,7 +224,7 @@ result = { success: false, - error_details: { name: [:blank] }, + error_details: { name: { blank: true } }, errors: { name: [t('errors.messages.blank')] }, totp_secret_present: true, multi_factor_auth_method: 'totp', diff --git a/spec/controllers/users/verify_personal_key_controller_spec.rb b/spec/controllers/users/verify_personal_key_controller_spec.rb index 4a71520eb0a..ec87ddceb20 100644 --- a/spec/controllers/users/verify_personal_key_controller_spec.rb +++ b/spec/controllers/users/verify_personal_key_controller_spec.rb @@ -88,7 +88,13 @@ let(:personal_key_bad_params) { { personal_key: 'baaad' } } let(:personal_key_error) { { personal_key: [error_text] } } let(:failure_properties) { { success: false, failure_reason: personal_key_error } } - let(:pii_like_keypaths_errors) { [[:errors, :personal_key], [:error_details, :personal_key]] } + let(:pii_like_keypaths_errors) do + [ + [:errors, :personal_key], + [:error_details, :personal_key], + [:error_details, :personal_key, :personal_key], + ] + end let(:response_ok) { FormResponse.new(success: true, errors: {}) } let(:response_bad) { FormResponse.new(success: false, errors: personal_key_error, extra: {}) } @@ -163,7 +169,7 @@ expect(@analytics).to receive(:track_event).with( 'Personal key reactivation: Personal key form submitted', errors: { personal_key: ['Please fill in this field.', error_text] }, - error_details: { personal_key: [:blank, :personal_key_incorrect] }, + error_details: { personal_key: { blank: true, personal_key: true } }, success: false, pii_like_keypaths: pii_like_keypaths_errors, ).once diff --git a/spec/controllers/users/webauthn_setup_controller_spec.rb b/spec/controllers/users/webauthn_setup_controller_spec.rb index 511b99d99b9..aed625d9d2b 100644 --- a/spec/controllers/users/webauthn_setup_controller_spec.rb +++ b/spec/controllers/users/webauthn_setup_controller_spec.rb @@ -392,10 +392,7 @@ 'errors.webauthn_platform_setup.attestation_error', link: MarketingSite.contact_url, )] }, - error_details: { name: [I18n.t( - 'errors.webauthn_platform_setup.attestation_error', - link: MarketingSite.contact_url, - )] }, + error_details: { name: { attestation_error: true } }, in_account_creation_flow: false, mfa_method_counts: {}, multi_factor_auth_method: 'webauthn_platform', diff --git a/spec/forms/idv/phone_form_spec.rb b/spec/forms/idv/phone_form_spec.rb index 356d44dc48d..46d89945415 100644 --- a/spec/forms/idv/phone_form_spec.rb +++ b/spec/forms/idv/phone_form_spec.rb @@ -93,16 +93,7 @@ unregistered_phone = '+400258567234' result = subject.submit(phone: unregistered_phone) expect(result.success?).to eq false - expect(result.to_h).to include( - { - error_details: { - phone: [t( - 'two_factor_authentication.otp_delivery_preference.sms_unsupported', - location: 'Romania', - )], - }, - }, - ) + expect(result.to_h).to include(error_details: { phone: { sms_unsupported: true } }) end end diff --git a/spec/forms/openid_connect_authorize_form_spec.rb b/spec/forms/openid_connect_authorize_form_spec.rb index dce7824be51..74395927062 100644 --- a/spec/forms/openid_connect_authorize_form_spec.rb +++ b/spec/forms/openid_connect_authorize_form_spec.rb @@ -64,7 +64,7 @@ expect(result.to_h).to eq( success: false, errors: { response_type: ['is not included in the list'] }, - error_details: { response_type: [:inclusion] }, + error_details: { response_type: { inclusion: true } }, client_id: client_id, prompt: 'select_account', allow_prompt_login: true, diff --git a/spec/forms/otp_verification_form_spec.rb b/spec/forms/otp_verification_form_spec.rb index c4ec4fdd12c..32af977d9ff 100644 --- a/spec/forms/otp_verification_form_spec.rb +++ b/spec/forms/otp_verification_form_spec.rb @@ -45,7 +45,7 @@ expect(result.to_h).to eq( success: false, error_details: { - code: [:blank, :wrong_length], + code: { blank: true, wrong_length: true }, }, multi_factor_auth_method: 'otp_code', multi_factor_auth_method_created_at: phone_configuration.created_at.strftime('%s%L'), @@ -67,7 +67,7 @@ expect(result.to_h).to eq( success: false, error_details: { - code: ['user_otp_missing'], + code: { user_otp_missing: true }, }, multi_factor_auth_method: 'otp_code', multi_factor_auth_method_created_at: phone_configuration.created_at.strftime('%s%L'), @@ -89,7 +89,7 @@ expect(result.to_h).to eq( success: false, error_details: { - code: [:wrong_length, 'incorrect'], + code: { wrong_length: true, incorrect: true }, }, multi_factor_auth_method: 'otp_code', multi_factor_auth_method_created_at: phone_configuration.created_at.strftime('%s%L'), @@ -111,7 +111,7 @@ expect(result.to_h).to eq( success: false, error_details: { - code: ['pattern_mismatch', 'incorrect'], + code: { pattern_mismatch: true, incorrect: true }, }, multi_factor_auth_method: 'otp_code', multi_factor_auth_method_created_at: phone_configuration.created_at.strftime('%s%L'), @@ -136,7 +136,7 @@ expect(result.to_h).to eq( success: false, error_details: { - code: ['user_otp_expired'], + code: { user_otp_expired: true }, }, multi_factor_auth_method: 'otp_code', multi_factor_auth_method_created_at: phone_configuration.created_at.strftime('%s%L'), diff --git a/spec/forms/totp_setup_form_spec.rb b/spec/forms/totp_setup_form_spec.rb index 79a814512d9..6e2dcd7ac8f 100644 --- a/spec/forms/totp_setup_form_spec.rb +++ b/spec/forms/totp_setup_form_spec.rb @@ -80,7 +80,7 @@ expect(form.submit.to_h).to include( success: false, - error_details: { name: [:blank] }, + error_details: { name: { blank: true } }, errors: { name: [t('errors.messages.blank')] }, ) expect(user.auth_app_configurations.any?).to eq false @@ -95,7 +95,7 @@ expect(form2.submit.to_h).to include( success: false, - error_details: { name: [t('errors.piv_cac_setup.unique_name')] }, + error_details: { name: { unique_name: true } }, errors: { name: [t('errors.piv_cac_setup.unique_name')] }, ) end diff --git a/spec/forms/webauthn_setup_form_spec.rb b/spec/forms/webauthn_setup_form_spec.rb index 235307a57b9..e5207d3e6c7 100644 --- a/spec/forms/webauthn_setup_form_spec.rb +++ b/spec/forms/webauthn_setup_form_spec.rb @@ -215,10 +215,7 @@ 'errors.webauthn_setup.attestation_error', link: MarketingSite.contact_url, )] }, - error_details: { name: [I18n.t( - 'errors.webauthn_setup.attestation_error', - link: MarketingSite.contact_url, - )] }, + error_details: { name: { attestation_error: true } }, **extra_attributes, ) end diff --git a/spec/forms/webauthn_verification_form_spec.rb b/spec/forms/webauthn_verification_form_spec.rb index 2843d0cd3c4..448b6e707b4 100644 --- a/spec/forms/webauthn_verification_form_spec.rb +++ b/spec/forms/webauthn_verification_form_spec.rb @@ -68,7 +68,7 @@ it 'returns unsuccessful result' do expect(result.to_h).to eq( success: false, - error_details: { user: [:blank], webauthn_configuration: [:blank] }, + error_details: { user: { blank: true }, webauthn_configuration: { blank: true } }, multi_factor_auth_method: 'webauthn', webauthn_configuration_id: nil, ) @@ -82,8 +82,8 @@ expect(result.to_h).to eq( success: false, error_details: { - challenge: [:blank], - authenticator_data: ['invalid_authenticator_data'], + challenge: { blank: true }, + authenticator_data: { invalid_authenticator_data: true }, }, multi_factor_auth_method: 'webauthn', webauthn_configuration_id: webauthn_configuration.id, @@ -98,7 +98,7 @@ expect(result.to_h).to eq( success: false, error_details: { - authenticator_data: [:blank, 'invalid_authenticator_data'], + authenticator_data: { blank: true, invalid_authenticator_data: true }, }, multi_factor_auth_method: 'webauthn', webauthn_configuration_id: webauthn_configuration.id, @@ -113,8 +113,8 @@ expect(result.to_h).to eq( success: false, error_details: { - client_data_json: [:blank], - authenticator_data: ['invalid_authenticator_data'], + client_data_json: { blank: true }, + authenticator_data: { invalid_authenticator_data: true }, }, multi_factor_auth_method: 'webauthn', webauthn_configuration_id: webauthn_configuration.id, @@ -129,8 +129,8 @@ expect(result.to_h).to eq( success: false, error_details: { - signature: [:blank], - authenticator_data: ['invalid_authenticator_data'], + signature: { blank: true }, + authenticator_data: { invalid_authenticator_data: true }, }, multi_factor_auth_method: 'webauthn', webauthn_configuration_id: webauthn_configuration.id, @@ -144,7 +144,7 @@ it 'returns unsuccessful result' do expect(result.to_h).to eq( success: false, - error_details: { webauthn_configuration: [:blank] }, + error_details: { webauthn_configuration: { blank: true } }, multi_factor_auth_method: 'webauthn', webauthn_configuration_id: nil, ) @@ -157,7 +157,7 @@ it 'returns unsuccessful result including client-side webauthn error text' do expect(result.to_h).to eq( success: false, - error_details: { webauthn_error: [webauthn_error] }, + error_details: { webauthn_error: { webauthn_error: true } }, multi_factor_auth_method: 'webauthn', webauthn_configuration_id: webauthn_configuration.id, ) @@ -172,7 +172,7 @@ it 'returns unsuccessful result' do expect(result.to_h).to eq( success: false, - error_details: { authenticator_data: ['invalid_authenticator_data'] }, + error_details: { authenticator_data: { invalid_authenticator_data: true } }, multi_factor_auth_method: 'webauthn', webauthn_configuration_id: webauthn_configuration.id, ) @@ -188,7 +188,7 @@ it 'returns unsucessful result' do expect(result.to_h).to eq( success: false, - error_details: { authenticator_data: ['invalid_authenticator_data'] }, + error_details: { authenticator_data: { invalid_authenticator_data: true } }, multi_factor_auth_method: 'webauthn', webauthn_configuration_id: webauthn_configuration.id, ) diff --git a/spec/services/form_response_spec.rb b/spec/services/form_response_spec.rb index 9898d2e5eb7..76f3659702d 100644 --- a/spec/services/form_response_spec.rb +++ b/spec/services/form_response_spec.rb @@ -78,7 +78,9 @@ response2 = FormResponse.new(success: false, errors: errors2) combined_response = response1.merge(response2) - expect(combined_response.to_h[:error_details]).to eq(email_language: [:blank, :invalid]) + expect(combined_response.to_h[:error_details]).to eq( + email_language: { blank: true, invalid: true }, + ) end it 'merges hash and ActiveModel::Errors' do @@ -93,7 +95,7 @@ expect(combined_response.errors).to eq( email_language: ['Language cannot be blank', 'Language is not valid'], ) - expect(combined_response.to_h[:error_details]).to eq(email_language: [:blank]) + expect(combined_response.to_h[:error_details]).to eq(email_language: { blank: true }) end it 'returns true if one is false and one is true' do @@ -146,13 +148,32 @@ email_language: ['Language cannot be blank'], }, error_details: { - email_language: [:blank], + email_language: { blank: true }, }, } expect(response.to_h).to eq response_hash end + context 'without error type' do + it 'falls back to message as key for details' do + errors = ActiveModel::Errors.new(build_stubbed(:user)) + errors.add(:email_language, :blank) + response = FormResponse.new(success: false, errors: errors) + response_hash = { + success: false, + errors: { + email_language: [t('errors.messages.blank')], + }, + error_details: { + email_language: { blank: true }, + }, + } + + expect(response.to_h).to eq response_hash + end + end + it 'omits details if errors are empty' do errors = ActiveModel::Errors.new(build_stubbed(:user)) response = FormResponse.new(success: true, errors: errors) @@ -177,6 +198,25 @@ expect(combined_response.to_h).to eq response_hash end + context 'with error detail symbol defined as type option on error' do + it 'returns a hash with success, errors, and error_details keys' do + errors = ActiveModel::Errors.new(build_stubbed(:user)) + errors.add(:email_language, 'Language cannot be blank', type: :blank) + response = FormResponse.new(success: false, errors: errors) + response_hash = { + success: false, + errors: { + email_language: ['Language cannot be blank'], + }, + error_details: { + email_language: { blank: true }, + }, + } + + expect(response.to_h).to eq response_hash + end + end + context 'with serialize_error_details_only' do it 'excludes errors from the hash' do errors = ActiveModel::Errors.new(build_stubbed(:user)) @@ -190,7 +230,7 @@ expect(response.to_h).to eq( success: false, error_details: { - email_language: [:blank], + email_language: { blank: true }, }, ) end diff --git a/spec/support/doc_pii_helper.rb b/spec/support/doc_pii_helper.rb index a1d20041ab3..2f9be8fb391 100644 --- a/spec/support/doc_pii_helper.rb +++ b/spec/support/doc_pii_helper.rb @@ -2,14 +2,27 @@ module DocPiiHelper def pii_like_keypaths [ [:pii], - [:name, :dob, :dob_min_age, :address1, :state, :zipcode, :jurisdiction], - [:errors, :name], [:error_details, :name], - [:errors, :dob], [:error_details, :dob], - [:errors, :dob_min_age], [:error_details, :dob_min_age], - [:errors, :address1], [:error_details, :address1], - [:errors, :state], [:error_details, :state], - [:errors, :zipcode], [:error_details, :zipcode], - [:errors, :jurisdiction], [:error_details, :jurisdiction] + [:errors, :name], + [:error_details, :name], + [:error_details, :name, :name], + [:errors, :dob], + [:error_details, :dob], + [:error_details, :dob, :dob], + [:errors, :dob_min_age], + [:error_details, :dob_min_age], + [:error_details, :dob_min_age, :dob_min_age], + [:errors, :address1], + [:error_details, :address1], + [:error_details, :address1, :address1], + [:errors, :state], + [:error_details, :state], + [:error_details, :state, :state], + [:errors, :zipcode], + [:error_details, :zipcode], + [:error_details, :zipcode, :zipcode], + [:errors, :jurisdiction], + [:error_details, :jurisdiction], + [:error_details, :jurisdiction, :jurisdiction], ] end end diff --git a/spec/support/fake_analytics.rb b/spec/support/fake_analytics.rb index 07dddee9acc..bf1ce740c10 100644 --- a/spec/support/fake_analytics.rb +++ b/spec/support/fake_analytics.rb @@ -56,7 +56,7 @@ def track_event(event, original_attributes = {}) ERROR end - check_recursive.call(val, [key]) + check_recursive.call(val, current_keypath) end when Array value.each { |val| check_recursive.call(val, keypath) } diff --git a/spec/support/fake_attempts_tracker.rb b/spec/support/fake_attempts_tracker.rb index fa0add2a3cc..808467a9fb7 100644 --- a/spec/support/fake_attempts_tracker.rb +++ b/spec/support/fake_attempts_tracker.rb @@ -15,7 +15,7 @@ def track_event(event, attributes = {}) end def parse_failure_reason(result) - return result.to_h[:error_details] || result.errors.presence + return result.to_h[:error_details]&.transform_values(&:keys) || result.errors.presence end def track_mfa_submit_event(_attributes) From 314baf76f7ff724dc1e463113f6a2457665103b8 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Thu, 16 Nov 2023 12:34:37 -0800 Subject: [PATCH 05/12] Remove failure_reason from Attempts API stub (#9576) * Remove Tracker#parse_failure_reason **Why**: Simplifies codebase * Remove failure_reason entirely **Why**: Inconsitent structure, and completely unused * Remove some more unused fake tracker code changelog: Internal, Source code, Clean up unused error tracking code --- .../concerns/idv/verify_info_concern.rb | 5 +- .../concerns/unconfirmed_user_concern.rb | 2 - .../idv/by_mail/enter_code_controller.rb | 1 - .../idv/hybrid_handoff_controller.rb | 5 +- .../idv/otp_verification_controller.rb | 5 -- app/controllers/idv/phone_controller.rb | 2 - .../sign_up/email_confirmations_controller.rb | 1 - .../sign_up/passwords_controller.rb | 3 - .../sign_up/registrations_controller.rb | 1 - .../piv_cac_verification_controller.rb | 1 - app/controllers/users/passwords_controller.rb | 1 - ...piv_cac_authentication_setup_controller.rb | 1 - .../piv_cac_setup_from_sign_in_controller.rb | 1 - .../users/reset_passwords_controller.rb | 3 - .../two_factor_authentication_controller.rb | 2 - .../users/verify_personal_key_controller.rb | 1 - app/forms/idv/api_image_upload_form.rb | 1 - app/services/account_reset/track_irs_event.rb | 5 -- .../idv/steps/threat_metrix_step_helper.rb | 7 -- app/services/irs_attempts_api/tracker.rb | 4 - .../irs_attempts_api/tracker_events.rb | 90 ++++--------------- .../idv/by_mail/enter_code_controller_spec.rb | 4 +- .../idv/image_uploads_controller_spec.rb | 12 --- .../idv/otp_verification_controller_spec.rb | 7 -- spec/controllers/idv/phone_controller_spec.rb | 5 -- .../idv/verify_info_controller_spec.rb | 6 -- .../email_confirmations_controller_spec.rb | 5 -- .../sign_up/passwords_controller_spec.rb | 6 +- .../sign_up/registrations_controller_spec.rb | 3 +- .../piv_cac_verification_controller_spec.rb | 2 - .../users/passwords_controller_spec.rb | 9 +- ...ac_authentication_setup_controller_spec.rb | 5 -- .../users/reset_passwords_controller_spec.rb | 17 +--- ...o_factor_authentication_controller_spec.rb | 8 +- .../verify_personal_key_controller_spec.rb | 3 +- .../idv/doc_auth/hybrid_handoff_spec.rb | 2 - .../idv/doc_auth/verify_info_step_spec.rb | 4 - spec/forms/idv/api_image_upload_form_spec.rb | 1 - .../account_reset/delete_account_spec.rb | 6 -- .../validate_granted_token_spec.rb | 5 -- spec/support/fake_attempts_tracker.rb | 12 --- 41 files changed, 30 insertions(+), 234 deletions(-) diff --git a/app/controllers/concerns/idv/verify_info_concern.rb b/app/controllers/concerns/idv/verify_info_concern.rb index 2f40ce2daaf..2421aafd1c4 100644 --- a/app/controllers/concerns/idv/verify_info_concern.rb +++ b/app/controllers/concerns/idv/verify_info_concern.rb @@ -176,7 +176,6 @@ def process_async_state(current_async_state) log_idv_verification_submitted_event( success: false, - failure_reason: { idv_verification: [:timeout] }, ) end end @@ -204,7 +203,6 @@ def async_state_done(current_async_state) ) log_idv_verification_submitted_event( success: form_response.success?, - failure_reason: irs_attempts_api_tracker.parse_failure_reason(form_response), ) form_response.extra[:ssn_is_unique] = DuplicateSsnFinder.new( @@ -297,7 +295,7 @@ def idv_result_to_form_response( ) end - def log_idv_verification_submitted_event(success: false, failure_reason: nil) + def log_idv_verification_submitted_event(success: false) pii_from_doc = pii || {} irs_attempts_api_tracker.idv_verification_submitted( success: success, @@ -310,7 +308,6 @@ def log_idv_verification_submitted_event(success: false, failure_reason: nil) date_of_birth: pii_from_doc[:dob], address: pii_from_doc[:address1], ssn: idv_session.ssn, - failure_reason: failure_reason, ) end diff --git a/app/controllers/concerns/unconfirmed_user_concern.rb b/app/controllers/concerns/unconfirmed_user_concern.rb index 5742e67cff1..fff0089c3d0 100644 --- a/app/controllers/concerns/unconfirmed_user_concern.rb +++ b/app/controllers/concerns/unconfirmed_user_concern.rb @@ -28,7 +28,6 @@ def track_user_already_confirmed_event irs_attempts_api_tracker.user_registration_email_confirmation( email: @email_address.email, success: false, - failure_reason: { email: [:already_confirmed] }, ) end @@ -39,7 +38,6 @@ def stop_if_invalid_token irs_attempts_api_tracker.user_registration_email_confirmation( email: @email_address&.email, success: false, - failure_reason: irs_attempts_api_tracker.parse_failure_reason(result), ) process_unsuccessful_confirmation end diff --git a/app/controllers/idv/by_mail/enter_code_controller.rb b/app/controllers/idv/by_mail/enter_code_controller.rb index 5b7b9ecac84..d5061a69468 100644 --- a/app/controllers/idv/by_mail/enter_code_controller.rb +++ b/app/controllers/idv/by_mail/enter_code_controller.rb @@ -58,7 +58,6 @@ def create analytics.idv_verify_by_mail_enter_code_submitted(**result.to_h) irs_attempts_api_tracker.idv_gpo_verification_submitted( success: result.success?, - failure_reason: irs_attempts_api_tracker.parse_failure_reason(result), ) if !result.success? diff --git a/app/controllers/idv/hybrid_handoff_controller.rb b/app/controllers/idv/hybrid_handoff_controller.rb index 72d254e14dd..be83c31b68d 100644 --- a/app/controllers/idv/hybrid_handoff_controller.rb +++ b/app/controllers/idv/hybrid_handoff_controller.rb @@ -54,18 +54,15 @@ def handle_phone_submission telephony_result = send_link telephony_form_response = build_telephony_form_response(telephony_result) - failure_reason = nil if !telephony_result.success? - failure_reason = { telephony: [telephony_result.error.class.name.demodulize] } failure(telephony_form_response.errors[:message]) end irs_attempts_api_tracker.idv_phone_upload_link_sent( success: telephony_result.success?, phone_number: formatted_destination_phone, - failure_reason: failure_reason, ) - if !failure_reason + if telephony_result.success? redirect_to idv_link_sent_url else redirect_to idv_hybrid_handoff_url diff --git a/app/controllers/idv/otp_verification_controller.rb b/app/controllers/idv/otp_verification_controller.rb index 2e34cf10173..f5327b95876 100644 --- a/app/controllers/idv/otp_verification_controller.rb +++ b/app/controllers/idv/otp_verification_controller.rb @@ -20,14 +20,9 @@ def update result = phone_confirmation_otp_verification_form.submit(code: params[:code]) analytics.idv_phone_confirmation_otp_submitted(**result.to_h) - parsed_failure_reason = - (result.extra.slice(:code_expired) if result.extra[:code_expired]) || - (result.extra.slice(:code_matches) if !result.success? && !result.extra[:code_matches]) || - {} irs_attempts_api_tracker.idv_phone_otp_submitted( success: result.success?, phone_number: idv_session.user_phone_confirmation_session.phone, - failure_reason: parsed_failure_reason, ) if result.success? diff --git a/app/controllers/idv/phone_controller.rb b/app/controllers/idv/phone_controller.rb index 672a8aa2538..811661fd035 100644 --- a/app/controllers/idv/phone_controller.rb +++ b/app/controllers/idv/phone_controller.rb @@ -48,7 +48,6 @@ def create irs_attempts_api_tracker.idv_phone_submitted( success: result.success?, phone_number: step_params[:phone], - failure_reason: irs_attempts_api_tracker.parse_failure_reason(result), ) if result.success? submit_proofing_attempt @@ -96,7 +95,6 @@ def send_phone_confirmation_otp_and_handle_result phone_number: @idv_phone, success: result.success?, otp_delivery_method: idv_session.previous_phone_step_params[:otp_delivery_preference], - failure_reason: result.success? ? {} : otp_sent_tracker_error(result), ) if result.success? redirect_to idv_otp_verification_url diff --git a/app/controllers/sign_up/email_confirmations_controller.rb b/app/controllers/sign_up/email_confirmations_controller.rb index 41dca8a0de0..5123e10dabc 100644 --- a/app/controllers/sign_up/email_confirmations_controller.rb +++ b/app/controllers/sign_up/email_confirmations_controller.rb @@ -26,7 +26,6 @@ def process_successful_confirmation irs_attempts_api_tracker.user_registration_email_confirmation( email: @email_address&.email, success: true, - failure_reason: nil, ) redirect_to sign_up_enter_password_url(confirmation_token: @confirmation_token) end diff --git a/app/controllers/sign_up/passwords_controller.rb b/app/controllers/sign_up/passwords_controller.rb index 6db85b2effe..5fc4a7d1aa4 100644 --- a/app/controllers/sign_up/passwords_controller.rb +++ b/app/controllers/sign_up/passwords_controller.rb @@ -39,12 +39,9 @@ def render_page end def track_analytics(result) - failure_reason = irs_attempts_api_tracker.parse_failure_reason(result) - analytics.password_creation(**result.to_h) irs_attempts_api_tracker.user_registration_password_submitted( success: result.success?, - failure_reason: failure_reason, ) end diff --git a/app/controllers/sign_up/registrations_controller.rb b/app/controllers/sign_up/registrations_controller.rb index d228d97bde0..25d47ac5998 100644 --- a/app/controllers/sign_up/registrations_controller.rb +++ b/app/controllers/sign_up/registrations_controller.rb @@ -30,7 +30,6 @@ def create irs_attempts_api_tracker.user_registration_email_submitted( email: permitted_params[:email], success: result.success?, - failure_reason: irs_attempts_api_tracker.parse_failure_reason(result), ) if result.success? diff --git a/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb b/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb index c17654e382f..52555750d54 100644 --- a/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb +++ b/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb @@ -33,7 +33,6 @@ def process_token irs_attempts_api_tracker.mfa_login_piv_cac( success: result.success?, subject_dn: piv_cac_verification_form.x509_dn, - failure_reason: irs_attempts_api_tracker.parse_failure_reason(result), ) if result.success? handle_valid_piv_cac diff --git a/app/controllers/users/passwords_controller.rb b/app/controllers/users/passwords_controller.rb index 611c42f8b39..15cfcfcca77 100644 --- a/app/controllers/users/passwords_controller.rb +++ b/app/controllers/users/passwords_controller.rb @@ -22,7 +22,6 @@ def update analytics.password_changed(**result.to_h) irs_attempts_api_tracker.logged_in_password_change( success: result.success?, - failure_reason: irs_attempts_api_tracker.parse_failure_reason(result), ) if result.success? diff --git a/app/controllers/users/piv_cac_authentication_setup_controller.rb b/app/controllers/users/piv_cac_authentication_setup_controller.rb index 73cef8934b9..6229003e160 100644 --- a/app/controllers/users/piv_cac_authentication_setup_controller.rb +++ b/app/controllers/users/piv_cac_authentication_setup_controller.rb @@ -88,7 +88,6 @@ def process_piv_cac_setup irs_attempts_api_tracker.mfa_enroll_piv_cac( success: result.success?, subject_dn: user_piv_cac_form.x509_dn, - failure_reason: irs_attempts_api_tracker.parse_failure_reason(result), ) if result.success? process_valid_submission diff --git a/app/controllers/users/piv_cac_setup_from_sign_in_controller.rb b/app/controllers/users/piv_cac_setup_from_sign_in_controller.rb index eca3141fbcc..ebea1debf03 100644 --- a/app/controllers/users/piv_cac_setup_from_sign_in_controller.rb +++ b/app/controllers/users/piv_cac_setup_from_sign_in_controller.rb @@ -43,7 +43,6 @@ def process_piv_cac_setup irs_attempts_api_tracker.mfa_enroll_piv_cac( success: result.success?, subject_dn: user_piv_cac_form.x509_dn, - failure_reason: irs_attempts_api_tracker.parse_failure_reason(result), ) if result.success? process_valid_submission diff --git a/app/controllers/users/reset_passwords_controller.rb b/app/controllers/users/reset_passwords_controller.rb index 1738b3e00d2..8ea9158e821 100644 --- a/app/controllers/users/reset_passwords_controller.rb +++ b/app/controllers/users/reset_passwords_controller.rb @@ -31,7 +31,6 @@ def edit analytics.password_reset_token(**result.to_h) irs_attempts_api_tracker.forgot_password_email_confirmed( success: result.success?, - failure_reason: irs_attempts_api_tracker.parse_failure_reason(result), ) if result.success? @reset_password_form = ResetPasswordForm.new(build_user) @@ -52,7 +51,6 @@ def update analytics.password_reset_password(**result.to_h) irs_attempts_api_tracker.forgot_password_new_password_submitted( success: result.success?, - failure_reason: irs_attempts_api_tracker.parse_failure_reason(result), ) if result.success? @@ -117,7 +115,6 @@ def create_account_if_email_not_found irs_attempts_api_tracker.user_registration_email_submitted( email: email, success: result.success?, - failure_reason: irs_attempts_api_tracker.parse_failure_reason(result), ) create_user_event(:account_created, user) end diff --git a/app/controllers/users/two_factor_authentication_controller.rb b/app/controllers/users/two_factor_authentication_controller.rb index 14f823d11c3..d3b1602f3f1 100644 --- a/app/controllers/users/two_factor_authentication_controller.rb +++ b/app/controllers/users/two_factor_authentication_controller.rb @@ -232,7 +232,6 @@ def track_events(otp_delivery_preference:, otp_delivery_selection_result:) reauthentication: true, phone_number: parsed_phone.e164, otp_delivery_method: otp_delivery_preference, - failure_reason: irs_attempts_api_tracker.parse_failure_reason(@telephony_result), ) elsif UserSessionContext.authentication_or_reauthentication_context?(context) irs_attempts_api_tracker.mfa_login_phone_otp_sent( @@ -240,7 +239,6 @@ def track_events(otp_delivery_preference:, otp_delivery_selection_result:) reauthentication: false, phone_number: parsed_phone.e164, otp_delivery_method: otp_delivery_preference, - failure_reason: irs_attempts_api_tracker.parse_failure_reason(@telephony_result), ) elsif UserSessionContext.confirmation_context?(context) irs_attempts_api_tracker.mfa_enroll_phone_otp_sent( diff --git a/app/controllers/users/verify_personal_key_controller.rb b/app/controllers/users/verify_personal_key_controller.rb index bd48cbe5168..63406069873 100644 --- a/app/controllers/users/verify_personal_key_controller.rb +++ b/app/controllers/users/verify_personal_key_controller.rb @@ -37,7 +37,6 @@ def create ) irs_attempts_api_tracker.personal_key_reactivation_submitted( success: result.success?, - failure_reason: irs_attempts_api_tracker.parse_failure_reason(result), ) if result.success? handle_success(decrypted_pii: personal_key_form.decrypted_pii) diff --git a/app/forms/idv/api_image_upload_form.rb b/app/forms/idv/api_image_upload_form.rb index 6cde3e7eed7..b15b71c3af7 100644 --- a/app/forms/idv/api_image_upload_form.rb +++ b/app/forms/idv/api_image_upload_form.rb @@ -424,7 +424,6 @@ def track_event(response) last_name: pii_from_doc[:last_name], date_of_birth: pii_from_doc[:dob], address: pii_from_doc[:address1], - failure_reason: response.errors&.except(:hints)&.presence, ) end diff --git a/app/services/account_reset/track_irs_event.rb b/app/services/account_reset/track_irs_event.rb index 2081eab1d24..b858979091f 100644 --- a/app/services/account_reset/track_irs_event.rb +++ b/app/services/account_reset/track_irs_event.rb @@ -9,7 +9,6 @@ module AccountReset::TrackIrsEvent def track_irs_event irs_attempts_api_tracker.account_reset_account_deleted( success: success, - failure_reason: event_failure_reason.presence, ) end @@ -20,8 +19,4 @@ def irs_attempts_api_tracker def cookies request.cookie_jar end - - def event_failure_reason - errors.is_a?(ActiveModel::Errors) ? errors.messages.to_hash : errors - end end diff --git a/app/services/idv/steps/threat_metrix_step_helper.rb b/app/services/idv/steps/threat_metrix_step_helper.rb index 1723158478b..17b894bd81e 100644 --- a/app/services/idv/steps/threat_metrix_step_helper.rb +++ b/app/services/idv/steps/threat_metrix_step_helper.rb @@ -57,17 +57,10 @@ def log_irs_tmx_fraud_check_event(result, user) user: user, login_session_id: Digest::SHA1.hexdigest(user.unique_session_id.to_s), ) - - if (tmx_summary_reason_code = result.dig(:response_body, :tmx_summary_reason_code)) - failure_reason = { - tmx_summary_reason_code: tmx_summary_reason_code, - } - end end irs_attempts_api_tracker.idv_tmx_fraud_check( success: success, - failure_reason: failure_reason, ) end end diff --git a/app/services/irs_attempts_api/tracker.rb b/app/services/irs_attempts_api/tracker.rb index 0cf1c7f3f1d..2d0fc63cf23 100644 --- a/app/services/irs_attempts_api/tracker.rb +++ b/app/services/irs_attempts_api/tracker.rb @@ -4,9 +4,5 @@ class Tracker def track_event(event_type, metadata = {}) end - - def parse_failure_reason(result) - return result.to_h[:error_details]&.transform_values(&:keys) || result.errors.presence - end end end diff --git a/app/services/irs_attempts_api/tracker_events.rb b/app/services/irs_attempts_api/tracker_events.rb index 07003dca817..fa43c36cb42 100644 --- a/app/services/irs_attempts_api/tracker_events.rb +++ b/app/services/irs_attempts_api/tracker_events.rb @@ -3,13 +3,11 @@ module IrsAttemptsApi module TrackerEvents # @param [Boolean] success True if Account Successfully Deleted - # @param [Hash>] failure_reason displays why account deletion failed # A User confirms and deletes their Login.gov account after 24 hour period - def account_reset_account_deleted(success:, failure_reason: nil) + def account_reset_account_deleted(success:) track_event( :account_reset_account_deleted, success: success, - failure_reason: failure_reason, ) end @@ -30,12 +28,10 @@ def account_reset_request_submitted(success:) end # @param [Boolean] success - # @param [Hash>] failure_reason - def forgot_password_email_confirmed(success:, failure_reason: nil) + def forgot_password_email_confirmed(success:) track_event( :forgot_password_email_confirmed, success: success, - failure_reason: failure_reason, ) end @@ -58,12 +54,10 @@ def forgot_password_email_sent(email:) end # @param [Boolean] success - # @param [Hash>] failure_reason - def forgot_password_new_password_submitted(success:, failure_reason: nil) + def forgot_password_new_password_submitted(success:) track_event( :forgot_password_new_password_submitted, success: success, - failure_reason: failure_reason, ) end @@ -108,7 +102,6 @@ def idv_document_upload_rate_limited # @param [String] last_name # @param [String] date_of_birth # @param [String] address - # @param [Hash>] failure_reason # The document was uploaded during the IDV process def idv_document_upload_submitted( success:, @@ -122,8 +115,7 @@ def idv_document_upload_submitted( first_name: nil, last_name: nil, date_of_birth: nil, - address: nil, - failure_reason: nil + address: nil ) track_event( :idv_document_upload_submitted, @@ -139,7 +131,6 @@ def idv_document_upload_submitted( last_name: last_name, date_of_birth: date_of_birth, address: address, - failure_reason: failure_reason, ) end @@ -160,13 +151,11 @@ def idv_gpo_verification_rate_limited end # @param [Boolean] success - # @param [Hash>] failure_reason displays GPO submission failed # GPO verification submitted from Letter sent to verify address - def idv_gpo_verification_submitted(success:, failure_reason: nil) + def idv_gpo_verification_submitted(success:) track_event( :idv_gpo_verification_submitted, success: success, - failure_reason: failure_reason, ) end @@ -189,15 +178,13 @@ def idv_personal_key_generated # @param [Boolean] success # @param [String] phone_number # @param [String] otp_delivery_method - Either SMS or Voice - # @param [Hash>] failure_reason # Track when OTP is sent and what method chosen during idv flow. - def idv_phone_otp_sent(success:, phone_number:, otp_delivery_method:, failure_reason: nil) + def idv_phone_otp_sent(success:, phone_number:, otp_delivery_method:) track_event( :idv_phone_otp_sent, success: success, phone_number: phone_number, otp_delivery_method: otp_delivery_method, - failure_reason: failure_reason, ) end @@ -211,13 +198,11 @@ def idv_phone_otp_sent_rate_limited # Tracks when a user submits OTP code sent to their phone # @param [Boolean] success # @param [String] phone_number - # @param [Hash>] failure_reason - def idv_phone_otp_submitted(success:, phone_number:, failure_reason: nil) + def idv_phone_otp_submitted(success:, phone_number:) track_event( :idv_phone_otp_submitted, success: success, phone_number: phone_number, - failure_reason: failure_reason, ) end @@ -242,30 +227,25 @@ def idv_phone_send_link_rate_limited(phone_number:) # Tracks when the user submits their idv phone number # @param [Boolean] success # @param [String] phone_number - # @param [Hash>] failure_reason - def idv_phone_submitted(success:, phone_number:, failure_reason: nil) + def idv_phone_submitted(success:, phone_number:) track_event( :idv_phone_submitted, success: success, phone_number: phone_number, - failure_reason: failure_reason, ) end # @param [Boolean] success # @param [String] phone_number - # @param [Hash>] failure_reason # The phone number that the link was sent to during the IDV process def idv_phone_upload_link_sent( success:, - phone_number:, - failure_reason: nil + phone_number: ) track_event( :idv_phone_upload_link_sent, success: success, phone_number: phone_number, - failure_reason: failure_reason, ) end @@ -286,14 +266,12 @@ def idv_ssn_submitted(ssn:) end # @param [Boolean] success - # @param [Hash>] failure_reason # This event will capture the result of the TMX fraud check # during Identity Verification - def idv_tmx_fraud_check(success:, failure_reason: nil) + def idv_tmx_fraud_check(success:) track_event( :idv_tmx_fraud_check, success: success, - failure_reason: failure_reason, ) end @@ -316,7 +294,6 @@ def idv_verification_rate_limited(limiter_context:) # @param [String] date_of_birth # @param [String] address # @param [String] ssn - # @param [Hash>] failure_reason # The verification was submitted during the IDV process def idv_verification_submitted( success:, @@ -328,8 +305,7 @@ def idv_verification_submitted( last_name: nil, date_of_birth: nil, address: nil, - ssn: nil, - failure_reason: nil + ssn: nil ) track_event( :idv_verification_submitted, @@ -343,7 +319,6 @@ def idv_verification_submitted( date_of_birth: date_of_birth, address: address, ssn: ssn, - failure_reason: failure_reason, ) end @@ -357,13 +332,11 @@ def logged_in_account_purged(success:) end # @param [Boolean] success True if the password was successfully changed - # @param [Hash>] failure_reason # A logged-in user has attempted to change their password - def logged_in_password_change(success:, failure_reason: nil) + def logged_in_password_change(success:) track_event( :logged_in_password_change, success: success, - failure_reason: failure_reason, ) end @@ -462,17 +435,14 @@ def mfa_enroll_phone_otp_submitted(success:) # Tracks when the user has attempted to enroll the piv cac MFA method to their account # @param [Boolean] success # @param [String] subject_dn - # @param [Hash>] failure_reason def mfa_enroll_piv_cac( success:, - subject_dn: nil, - failure_reason: nil + subject_dn: nil ) track_event( :mfa_enroll_piv_cac, success: success, subject_dn: subject_dn, - failure_reason: failure_reason, ) end @@ -526,14 +496,12 @@ def mfa_login_backup_code(success:) # @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 - # @param [Hash>] failure_reason - reason for failure if success is false # During a login attempt, an OTP code has been sent via SMS or Voice. def mfa_login_phone_otp_sent( success:, reauthentication:, phone_number:, - otp_delivery_method:, - failure_reason: + otp_delivery_method: ) track_event( :mfa_login_phone_otp_sent, @@ -541,7 +509,6 @@ def mfa_login_phone_otp_sent( reauthentication: reauthentication, phone_number: phone_number, otp_delivery_method: otp_delivery_method, - failure_reason: failure_reason, ) end @@ -569,17 +536,14 @@ def mfa_login_phone_otp_submitted(reauthentication:, success:) # Tracks when the user has attempted to log in with the piv cac MFA method to their account # @param [Boolean] success # @param [String] subject_dn - # @param [Hash>] failure_reason def mfa_login_piv_cac( success:, - subject_dn: nil, - failure_reason: nil + subject_dn: nil ) track_event( :mfa_login_piv_cac, success: success, subject_dn: subject_dn, - failure_reason: failure_reason, ) end @@ -629,29 +593,24 @@ def personal_key_reactivation_rate_limited # Tracks when user has entered personal key after forgot password steps # @param [Boolean] success - # @param [Hash>] failure_reason - def personal_key_reactivation_submitted(success:, failure_reason: nil) + def personal_key_reactivation_submitted(success:) track_event( :personal_key_reactivation_submitted, success: success, - failure_reason: failure_reason, ) end # Tracks when user confirms registration email # @param [Boolean] success # @param [String] email - # @param [Hash>] failure_reason def user_registration_email_confirmation( success:, - email: nil, - failure_reason: nil + email: nil ) track_event( :user_registration_email_confirmation, success: success, email: email, - failure_reason: failure_reason, ) end @@ -672,31 +631,20 @@ def user_registration_email_submission_rate_limited( # Tracks when user submits registration email # @param [Boolean] success # @param [String] email - # @param [Hash>] failure_reason - def user_registration_email_submitted( - success:, - email:, - failure_reason: nil - ) + def user_registration_email_submitted(success:, email:) track_event( :user_registration_email_submitted, success: success, email: email, - failure_reason: failure_reason, ) end # Tracks when user submits registration password # @param [Boolean] success - # @param [Hash>] failure_reason - def user_registration_password_submitted( - success:, - failure_reason: nil - ) + def user_registration_password_submitted(success:) track_event( :user_registration_password_submitted, success: success, - failure_reason: failure_reason, ) end end diff --git a/spec/controllers/idv/by_mail/enter_code_controller_spec.rb b/spec/controllers/idv/by_mail/enter_code_controller_spec.rb index 358609656dd..9bc0a47e173 100644 --- a/spec/controllers/idv/by_mail/enter_code_controller_spec.rb +++ b/spec/controllers/idv/by_mail/enter_code_controller_spec.rb @@ -166,7 +166,7 @@ describe '#create' do let(:otp_code_error_message) { { otp: [t('errors.messages.confirmation_code_incorrect')] } } - let(:success_properties) { { success: true, failure_reason: nil } } + let(:success_properties) { { success: true } } subject(:action) do post( @@ -390,7 +390,7 @@ pii_like_keypaths: [[:errors, :otp], [:error_details, :otp]], ) expect(@irs_attempts_api_tracker).to receive(:idv_gpo_verification_submitted). - with(success: false, failure_reason: { otp: [:confirmation_code_incorrect] }) + with(success: false) action diff --git a/spec/controllers/idv/image_uploads_controller_spec.rb b/spec/controllers/idv/image_uploads_controller_spec.rb index cba3d2687b8..7f1efc5684a 100644 --- a/spec/controllers/idv/image_uploads_controller_spec.rb +++ b/spec/controllers/idv/image_uploads_controller_spec.rb @@ -148,7 +148,6 @@ document_issued: nil, document_number: nil, document_state: nil, - failure_reason: { front: ['The selection was not a valid file.'] }, first_name: nil, last_name: nil, success: false }, @@ -292,7 +291,6 @@ document_issued: nil, document_number: nil, document_state: nil, - failure_reason: { limit: ['We couldn’t verify your ID'] }, first_name: nil, last_name: nil, success: false }, @@ -429,7 +427,6 @@ expect(@irs_attempts_api_tracker).to receive(:track_event).with( :idv_document_upload_submitted, success: true, - failure_reason: nil, document_back_image_filename: nil, document_front_image_filename: nil, document_image_encryption_key: nil, @@ -458,7 +455,6 @@ :idv_document_upload_submitted, hash_including( success: true, - failure_reason: nil, document_back_image_filename: match(document_filename_regex), document_front_image_filename: match(document_filename_regex), document_image_encryption_key: match(base64_regex), @@ -525,8 +521,6 @@ expect(@irs_attempts_api_tracker).to receive(:track_event).with( :idv_document_upload_submitted, success: false, - failure_reason: { name: - ['We couldn’t read the full name on your ID. Try taking new pictures.'] }, document_state: 'ND', document_number: nil, document_issued: nil, @@ -625,8 +619,6 @@ expect(@irs_attempts_api_tracker).to receive(:track_event).with( :idv_document_upload_submitted, success: false, - failure_reason: { name: - ['We couldn’t read the full name on your ID. Try taking new pictures.'] }, document_state: 'ND', document_number: nil, document_issued: nil, @@ -725,8 +717,6 @@ expect(@irs_attempts_api_tracker).to receive(:track_event).with( :idv_document_upload_submitted, success: false, - failure_reason: { state: - ['Try taking new pictures.'] }, document_state: 'Maryland', document_number: nil, document_issued: nil, @@ -822,8 +812,6 @@ expect(@irs_attempts_api_tracker).to receive(:track_event).with( :idv_document_upload_submitted, success: false, - failure_reason: { dob: - ['We couldn’t read the birth date on your ID. Try taking new pictures.'] }, document_back_image_filename: nil, document_front_image_filename: nil, document_image_encryption_key: nil, diff --git a/spec/controllers/idv/otp_verification_controller_spec.rb b/spec/controllers/idv/otp_verification_controller_spec.rb index 12c31acbcf5..3c542b13646 100644 --- a/spec/controllers/idv/otp_verification_controller_spec.rb +++ b/spec/controllers/idv/otp_verification_controller_spec.rb @@ -170,7 +170,6 @@ expect(@irs_attempts_api_tracker).to receive(:idv_phone_otp_submitted).with( success: true, **phone_property, - failure_reason: {}, ) put :update, params: otp_code_param @@ -183,9 +182,6 @@ expect(@irs_attempts_api_tracker).to receive(:idv_phone_otp_submitted).with( success: false, **phone_property, - failure_reason: { - code_matches: false, - }, ) put :update, params: invalid_otp_code_param @@ -208,9 +204,6 @@ expect(@irs_attempts_api_tracker).to receive(:idv_phone_otp_submitted).with( success: false, **phone_property, - failure_reason: { - code_expired: true, - }, ) put :update, params: otp_code_param diff --git a/spec/controllers/idv/phone_controller_spec.rb b/spec/controllers/idv/phone_controller_spec.rb index 90923f27064..2c0d6a88298 100644 --- a/spec/controllers/idv/phone_controller_spec.rb +++ b/spec/controllers/idv/phone_controller_spec.rb @@ -279,10 +279,6 @@ expect(@irs_attempts_api_tracker).to receive(:idv_phone_submitted).with( success: false, phone_number: improbable_phone_number, - failure_reason: { - phone: [:improbable_phone], - otp_delivery_preference: [:inclusion], - }, ) put :create, params: improbable_phone_form @@ -329,7 +325,6 @@ expect(@irs_attempts_api_tracker).to receive(:idv_phone_submitted).with( success: true, phone_number: good_phone, - failure_reason: nil, ) phone_params = { diff --git a/spec/controllers/idv/verify_info_controller_spec.rb b/spec/controllers/idv/verify_info_controller_spec.rb index a0225e75013..4bcaa3dae30 100644 --- a/spec/controllers/idv/verify_info_controller_spec.rb +++ b/spec/controllers/idv/verify_info_controller_spec.rb @@ -190,8 +190,6 @@ document_capture_session end - let(:expected_failure_reason) { DocAuthHelper::SAMPLE_TMX_SUMMARY_REASON_CODE } - before do controller. idv_session.verify_info_step_document_capture_session_uuid = document_capture_session.uuid @@ -209,7 +207,6 @@ it 'it logs IRS idv_tmx_fraud_check event' do expect(@irs_attempts_api_tracker).to receive(:idv_tmx_fraud_check).with( success: true, - failure_reason: nil, ) get :show end @@ -226,7 +223,6 @@ it 'it logs IRS idv_tmx_fraud_check event' do expect(@irs_attempts_api_tracker).to receive(:idv_tmx_fraud_check).with( success: false, - failure_reason: expected_failure_reason, ) get :show end @@ -243,7 +239,6 @@ it 'it logs IRS idv_tmx_fraud_check event' do expect(@irs_attempts_api_tracker).to receive(:idv_tmx_fraud_check).with( success: false, - failure_reason: expected_failure_reason, ) get :show end @@ -260,7 +255,6 @@ it 'it logs IRS idv_tmx_fraud_check event' do expect(@irs_attempts_api_tracker).to receive(:idv_tmx_fraud_check).with( success: false, - failure_reason: expected_failure_reason, ) get :show end diff --git a/spec/controllers/sign_up/email_confirmations_controller_spec.rb b/spec/controllers/sign_up/email_confirmations_controller_spec.rb index f7ef77d7a6f..f3dddcaea73 100644 --- a/spec/controllers/sign_up/email_confirmations_controller_spec.rb +++ b/spec/controllers/sign_up/email_confirmations_controller_spec.rb @@ -14,7 +14,6 @@ { email: nil, success: false, - failure_reason: { confirmation_token: [:not_found] }, } end @@ -95,7 +94,6 @@ expect(@irs_attempts_api_tracker).to receive(:user_registration_email_confirmation).with( email: email_address.email, success: false, - failure_reason: { email: [:already_confirmed] }, ) get :create, params: { confirmation_token: 'foo' } @@ -125,7 +123,6 @@ expect(@irs_attempts_api_tracker).to receive(:user_registration_email_confirmation).with( email: email_address.email, success: false, - failure_reason: { confirmation_token: [:expired] }, ) get :create, params: { confirmation_token: 'foo' } @@ -157,7 +154,6 @@ expect(@irs_attempts_api_tracker).to receive(:user_registration_email_confirmation).with( email: email_address.email, success: false, - failure_reason: { confirmation_token: [:expired] }, ) get :create, params: { confirmation_token: 'foo' } @@ -227,7 +223,6 @@ expect(@irs_attempts_api_tracker).to receive(:user_registration_email_confirmation).with( email: email_address.email, success: true, - failure_reason: nil, ) get :create, params: { confirmation_token: 'foo' } diff --git a/spec/controllers/sign_up/passwords_controller_spec.rb b/spec/controllers/sign_up/passwords_controller_spec.rb index c89b399fee1..8f48a43c5bc 100644 --- a/spec/controllers/sign_up/passwords_controller_spec.rb +++ b/spec/controllers/sign_up/passwords_controller_spec.rb @@ -16,7 +16,7 @@ end let(:password) { 'NewVal!dPassw0rd' } let(:password_confirmation) { password } - let(:success_properties) { { success: true, failure_reason: nil } } + let(:success_properties) { { success: true } } context 'with valid password' do let!(:user) { create(:user, :unconfirmed, confirmation_token: token) } @@ -75,10 +75,6 @@ expect(@irs_attempts_api_tracker).to receive(:user_registration_password_submitted). with( success: false, - failure_reason: { - password: [:too_short], - password_confirmation: [:too_short], - }, ) expect(@irs_attempts_api_tracker).not_to receive(:user_registration_email_confirmation) diff --git a/spec/controllers/sign_up/registrations_controller_spec.rb b/spec/controllers/sign_up/registrations_controller_spec.rb index acc0056867d..33296dd4589 100644 --- a/spec/controllers/sign_up/registrations_controller_spec.rb +++ b/spec/controllers/sign_up/registrations_controller_spec.rb @@ -58,7 +58,7 @@ end describe '#create' do - let(:success_properties) { { success: true, failure_reason: nil } } + let(:success_properties) { { success: true } } context 'when registering with a new email' do it 'tracks successful user registration' do stub_analytics @@ -169,7 +169,6 @@ :user_registration_email_submitted, email: 'invalid@', success: false, - failure_reason: { email: [:invalid] }, ) post :create, params: { user: { email: 'invalid@', request_id: '', terms_accepted: '1' } } diff --git a/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb index 93d2529de60..442f3ca293a 100644 --- a/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb @@ -126,7 +126,6 @@ expect(@irs_attempts_api_tracker).to receive(:mfa_login_piv_cac).with( success: true, subject_dn: x509_subject, - failure_reason: nil, ) expect(@analytics).to receive(:track_event). @@ -230,7 +229,6 @@ expect(@irs_attempts_api_tracker).to receive(:mfa_login_piv_cac).with( success: false, subject_dn: bad_dn, - failure_reason: piv_cac_mismatch, ) expect(@analytics).to receive(:track_event). diff --git a/spec/controllers/users/passwords_controller_spec.rb b/spec/controllers/users/passwords_controller_spec.rb index cc20f6fa3eb..3da622b52e0 100644 --- a/spec/controllers/users/passwords_controller_spec.rb +++ b/spec/controllers/users/passwords_controller_spec.rb @@ -22,7 +22,7 @@ allow(@analytics).to receive(:track_event) expect(@irs_attempts_api_tracker).to receive(:logged_in_password_change). - with(failure_reason: nil, success: true) + with(success: true) params = { password: 'salty new password', @@ -134,10 +134,6 @@ it 'renders edit' do expect(@irs_attempts_api_tracker).to receive(:logged_in_password_change).with( success: false, - failure_reason: { - password: [:too_short], - password_confirmation: [:too_short], - }, ) patch :update, params: { update_user_password_form: params } @@ -190,9 +186,6 @@ it 'renders edit' do expect(@irs_attempts_api_tracker).to receive(:logged_in_password_change).with( success: false, - failure_reason: { - password_confirmation: [:mismatch], - }, ) patch :update, params: { update_user_password_form: params } diff --git a/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb b/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb index 7151f319f8e..4c294a70f4c 100644 --- a/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb +++ b/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb @@ -132,7 +132,6 @@ :mfa_enroll_piv_cac, success: true, subject_dn: 'some dn', - failure_reason: nil, ) get :new, params: { token: good_token } @@ -156,7 +155,6 @@ :mfa_enroll_piv_cac, success: true, subject_dn: 'some dn', - failure_reason: nil, ) get :new, params: { token: good_token } @@ -172,7 +170,6 @@ :mfa_enroll_piv_cac, success: true, subject_dn: 'some dn', - failure_reason: nil, ) get :new, params: { token: good_token } @@ -185,7 +182,6 @@ :mfa_enroll_piv_cac, success: true, subject_dn: 'some dn', - failure_reason: nil, ) get :new, params: { token: good_token } @@ -213,7 +209,6 @@ :mfa_enroll_piv_cac, success: false, subject_dn: nil, - failure_reason: { type: 'certificate.bad' }, ) get :new, params: { token: bad_token } diff --git a/spec/controllers/users/reset_passwords_controller_spec.rb b/spec/controllers/users/reset_passwords_controller_spec.rb index bfa3b91d94a..c77a019caf3 100644 --- a/spec/controllers/users/reset_passwords_controller_spec.rb +++ b/spec/controllers/users/reset_passwords_controller_spec.rb @@ -4,7 +4,7 @@ let(:password_error_message) do t('errors.attributes.password.too_short.other', count: Devise.password_length.first) end - let(:success_properties) { { success: true, failure_reason: nil } } + let(:success_properties) { { success: true } } describe '#edit' do let(:user) { instance_double('User', uuid: '123') } let(:email_address) { instance_double('EmailAddress') } @@ -39,7 +39,6 @@ it 'redirects to page where user enters email for password reset token' do expect(@irs_attempts_api_tracker).to receive(:forgot_password_email_confirmed).with( success: false, - failure_reason: { user: [:blank] }, ) get :edit @@ -89,7 +88,6 @@ it 'redirects to page where user enters email for password reset token' do expect(@irs_attempts_api_tracker).to receive(:forgot_password_email_confirmed).with( success: false, - failure_reason: { user: [:blank] }, ) get :edit @@ -120,7 +118,6 @@ it 'redirects to page where user enters email for password reset token' do expect(@irs_attempts_api_tracker).to receive(:forgot_password_email_confirmed).with( success: false, - failure_reason: { user: [:token_expired] }, ) get :edit @@ -195,11 +192,6 @@ expect(@irs_attempts_api_tracker).to receive(:forgot_password_new_password_submitted).with( success: false, - failure_reason: { - password: [:too_short], - password_confirmation: [:too_short], - reset_password_token: [:token_expired], - }, ) raw_reset_token, db_confirmation_token = @@ -292,10 +284,6 @@ with('Password Reset: Password Submitted', analytics_hash) expect(@irs_attempts_api_tracker).to receive(:forgot_password_new_password_submitted).with( success: false, - failure_reason: { - password: [:too_short], - password_confirmation: [:too_short], - }, ) put :update, params: { reset_password_form: form_params } @@ -344,9 +332,6 @@ with('Password Reset: Password Submitted', analytics_hash) expect(@irs_attempts_api_tracker).to receive(:forgot_password_new_password_submitted).with( success: false, - failure_reason: { - password_confirmation: [:mismatch], - }, ) put :update, params: { reset_password_form: form_params } diff --git a/spec/controllers/users/two_factor_authentication_controller_spec.rb b/spec/controllers/users/two_factor_authentication_controller_spec.rb index 8fc08267900..622e104260f 100644 --- a/spec/controllers/users/two_factor_authentication_controller_spec.rb +++ b/spec/controllers/users/two_factor_authentication_controller_spec.rb @@ -274,9 +274,7 @@ def index let(:default_parameters) do { **valid_phone_number, otp_delivery_method: 'sms' } end - let(:success_parameters) do - { success: true, **default_parameters, failure_reason: nil } - end + let(:success_parameters) { { success: true, **default_parameters } } before do @user = create(:user, :with_phone) @@ -402,9 +400,7 @@ def index stub_attempts_tracker expect(@irs_attempts_api_tracker).to receive(:mfa_login_phone_otp_sent). - with(reauthentication: false, **default_parameters, success: false, failure_reason: { - telephony: 'Telephony::OptOutError - Telephony::OptOutError', - }) + with(reauthentication: false, **default_parameters, success: false) get :send_code, params: otp_delivery_form_sms end diff --git a/spec/controllers/users/verify_personal_key_controller_spec.rb b/spec/controllers/users/verify_personal_key_controller_spec.rb index ec87ddceb20..e852fcc0c08 100644 --- a/spec/controllers/users/verify_personal_key_controller_spec.rb +++ b/spec/controllers/users/verify_personal_key_controller_spec.rb @@ -87,7 +87,7 @@ let(:error_text) { 'Incorrect personal key' } let(:personal_key_bad_params) { { personal_key: 'baaad' } } let(:personal_key_error) { { personal_key: [error_text] } } - let(:failure_properties) { { success: false, failure_reason: personal_key_error } } + let(:failure_properties) { { success: false } } let(:pii_like_keypaths_errors) do [ [:errors, :personal_key], @@ -127,7 +127,6 @@ stub_attempts_tracker expect(@irs_attempts_api_tracker).to receive(:personal_key_reactivation_submitted).with( - failure_reason: nil, success: true, ).once diff --git a/spec/features/idv/doc_auth/hybrid_handoff_spec.rb b/spec/features/idv/doc_auth/hybrid_handoff_spec.rb index c2fa0f44f69..21471799825 100644 --- a/spec/features/idv/doc_auth/hybrid_handoff_spec.rb +++ b/spec/features/idv/doc_auth/hybrid_handoff_spec.rb @@ -52,7 +52,6 @@ ).with( success: true, phone_number: '+1 415-555-0199', - failure_reason: nil, ) expect(Telephony).to receive(:send_doc_auth_link). @@ -92,7 +91,6 @@ expect(fake_attempts_tracker).to receive(:idv_phone_upload_link_sent).with( success: false, phone_number: '+1 225-555-1000', - failure_reason: { telephony: ['TelephonyError'] }, ) fill_in :doc_auth_phone, with: '225-555-1000' diff --git a/spec/features/idv/doc_auth/verify_info_step_spec.rb b/spec/features/idv/doc_auth/verify_info_step_spec.rb index 512407f2d8b..182f6544011 100644 --- a/spec/features/idv/doc_auth/verify_info_step_spec.rb +++ b/spec/features/idv/doc_auth/verify_info_step_spec.rb @@ -86,7 +86,6 @@ it 'logs analytics and attempts tracker events on submit' do expect(fake_attempts_tracker).to receive(:idv_verification_submitted).with( success: true, - failure_reason: nil, **fake_pii_details, ssn: DocAuthHelper::GOOD_SSN, ) @@ -102,7 +101,6 @@ it 'does not proceed to the next page if resolution fails' do expect(fake_attempts_tracker).to receive(:idv_verification_submitted).with( success: false, - failure_reason: { ssn: ['Unverified SSN.'] }, **fake_pii_details, ssn: DocAuthHelper::SSN_THAT_FAILS_RESOLUTION, ) @@ -120,7 +118,6 @@ it 'does not proceed to the next page if resolution raises an exception' do expect(fake_attempts_tracker).to receive(:idv_verification_submitted).with( success: false, - failure_reason: nil, **fake_pii_details, ssn: DocAuthHelper::SSN_THAT_RAISES_EXCEPTION, ) @@ -371,7 +368,6 @@ it 'tracks attempts tracker event with failure reason' do expect(fake_attempts_tracker).to receive(:idv_verification_submitted).with( success: false, - failure_reason: { idv_verification: [:timeout] }, **fake_pii_details, ssn: DocAuthHelper::GOOD_SSN, ) diff --git a/spec/forms/idv/api_image_upload_form_spec.rb b/spec/forms/idv/api_image_upload_form_spec.rb index 91b6f680cbf..b92875d83b5 100644 --- a/spec/forms/idv/api_image_upload_form_spec.rb +++ b/spec/forms/idv/api_image_upload_form_spec.rb @@ -89,7 +89,6 @@ document_issued: '2019-12-31', document_number: '1111111111111', document_state: 'MT', - failure_reason: nil, first_name: 'FAKEY', last_name: 'MCFAKERSON', success: true, diff --git a/spec/services/account_reset/delete_account_spec.rb b/spec/services/account_reset/delete_account_spec.rb index fe561d2dbc5..582236929f7 100644 --- a/spec/services/account_reset/delete_account_spec.rb +++ b/spec/services/account_reset/delete_account_spec.rb @@ -3,10 +3,6 @@ RSpec.describe AccountReset::DeleteAccount do include AccountResetHelper - let(:expired_token_message) do - t('errors.account_reset.granted_token_expired', app_name: APP_NAME) - end - let(:expired_token_error) { { token: [expired_token_message] } } let(:user) { create(:user) } let(:request) { FakeRequest.new } let(:analytics) { FakeAnalytics.new } @@ -57,7 +53,6 @@ it 'logs attempts api event with success true if the token is good' do expect(fake_attempts_tracker).to receive(:account_reset_account_deleted).with( success: true, - failure_reason: nil, ) create_account_reset_request_for(user, service_provider.issuer) @@ -69,7 +64,6 @@ it 'logs attempts api event with failure reason if the token is expired' do expect(fake_attempts_tracker).to receive(:account_reset_account_deleted).with( success: false, - failure_reason: expired_token_error, ) create_account_reset_request_for(user, service_provider.issuer) diff --git a/spec/services/account_reset/validate_granted_token_spec.rb b/spec/services/account_reset/validate_granted_token_spec.rb index cde471c1c7b..de23fbaf3d3 100644 --- a/spec/services/account_reset/validate_granted_token_spec.rb +++ b/spec/services/account_reset/validate_granted_token_spec.rb @@ -3,10 +3,6 @@ RSpec.describe AccountReset::ValidateGrantedToken do include AccountResetHelper - let(:expired_token_message) do - t('errors.account_reset.granted_token_expired', app_name: APP_NAME) - end - let(:expired_token_error) { { token: [expired_token_message] } } let(:user) { create(:user) } let(:request) { FakeRequest.new } let(:analytics) { FakeAnalytics.new } @@ -33,7 +29,6 @@ it 'logs attempts api event with failure reason if the token is expired' do expect(fake_attempts_tracker).to receive(:account_reset_account_deleted).with( success: false, - failure_reason: expired_token_error, ) create_account_reset_request_for(user, service_provider.issuer) diff --git a/spec/support/fake_attempts_tracker.rb b/spec/support/fake_attempts_tracker.rb index 808467a9fb7..768a9c045ff 100644 --- a/spec/support/fake_attempts_tracker.rb +++ b/spec/support/fake_attempts_tracker.rb @@ -13,17 +13,5 @@ def track_event(event, attributes = {}) events[event] << attributes nil end - - def parse_failure_reason(result) - return result.to_h[:error_details]&.transform_values(&:keys) || result.errors.presence - end - - def track_mfa_submit_event(_attributes) - # no-op - end - - def browser_attributes - {} - end end end From e958e9dfda40f1d5758dcf8c980aba257c004def Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 16 Nov 2023 16:11:54 -0500 Subject: [PATCH 06/12] Use Icon List component for requested attributes consent (#9555) * Support tag options for IconListItemComponent * Use IconListComponent for requested attributes changelog: Internal, Components, Use consistent design system component for icon list * Use ButtonComponent for auth confirmation * Remove seemingly-unnecessary assertion content scoping --- app/assets/stylesheets/components/_index.scss | 1 - app/assets/stylesheets/components/_list.scss | 17 ------ app/components/icon_list_component.rb | 5 +- .../icon_list_item_component.html.erb | 4 +- .../_requested_attributes.html.erb | 32 ------------ app/views/sign_up/completions/show.html.erb | 38 ++++++++++++-- .../authorization_confirmation/new.html.erb | 50 ++++++++++-------- spec/components/icon_list_component_spec.rb | 18 ++++++- .../authorization_confirmation_spec.rb | 6 +-- .../saml/authorization_confirmation_spec.rb | 6 +-- spec/features/saml/ial1_sso_spec.rb | 15 +++--- .../idv_examples/sp_requested_attributes.rb | 52 +++++++++---------- 12 files changed, 115 insertions(+), 129 deletions(-) delete mode 100644 app/assets/stylesheets/components/_list.scss delete mode 100644 app/views/sign_up/completions/_requested_attributes.html.erb diff --git a/app/assets/stylesheets/components/_index.scss b/app/assets/stylesheets/components/_index.scss index 9ff1d8eb4b7..0c37bdfb6b8 100644 --- a/app/assets/stylesheets/components/_index.scss +++ b/app/assets/stylesheets/components/_index.scss @@ -13,7 +13,6 @@ @forward 'hr'; @forward 'icon'; @forward 'language-picker'; -@forward 'list'; @forward 'modal'; @forward 'nav'; @forward 'page-heading'; diff --git a/app/assets/stylesheets/components/_list.scss b/app/assets/stylesheets/components/_list.scss deleted file mode 100644 index 4e69fe67efd..00000000000 --- a/app/assets/stylesheets/components/_list.scss +++ /dev/null @@ -1,17 +0,0 @@ -.success-bullets { - .success-bullet { - padding: 1rem 1rem 1rem 0; - - &::before { - background-image: url('/alert/success.svg'); - background-repeat: no-repeat; - content: ''; - float: left; - height: 1rem; - margin-top: 0.33rem; - padding-right: 1.5rem; - vertical-align: middle; - width: 1rem; - } - } -} diff --git a/app/components/icon_list_component.rb b/app/components/icon_list_component.rb index 58a37885fd0..72b468f86ee 100644 --- a/app/components/icon_list_component.rb +++ b/app/components/icon_list_component.rb @@ -19,11 +19,12 @@ def css_class end class IconListItemComponent < BaseComponent - attr_reader :icon, :color + attr_reader :icon, :color, :tag_options - def initialize(icon:, color:) + def initialize(icon:, color:, **tag_options) @icon = icon @color = color + @tag_options = tag_options end def icon_css_class diff --git a/app/components/icon_list_item_component.html.erb b/app/components/icon_list_item_component.html.erb index 05bab773005..128a16fccb8 100644 --- a/app/components/icon_list_item_component.html.erb +++ b/app/components/icon_list_item_component.html.erb @@ -1,6 +1,6 @@ -
  • +<%= content_tag(:li, **tag_options, class: [*tag_options[:class], 'usa-icon-list__item']) do %> <%= content_tag(:div, class: icon_css_class) do %> <%= render IconComponent.new(icon: icon) %> <% end %>
    <%= content %>
    -
  • +<% end %> diff --git a/app/views/sign_up/completions/_requested_attributes.html.erb b/app/views/sign_up/completions/_requested_attributes.html.erb deleted file mode 100644 index 3933b2379a7..00000000000 --- a/app/views/sign_up/completions/_requested_attributes.html.erb +++ /dev/null @@ -1,32 +0,0 @@ -
      - <% pii.each do |attribute_key, attribute_value| %> - <% next if attribute_value.blank? %> -
    • -
      - <%= t("help_text.requested_attributes.#{attribute_key}") %> -
      -
      - <% if attribute_value.is_a? Array %> -
        - <% attribute_value.each do |item| %> -
      • <%= item %>
      • - <% end %> -
      - <% elsif attribute_key == :social_security_number %> - <%= render( - 'shared/masked_text', - text: attribute_value, - masked_text: SsnFormatter.format_masked(attribute_value), - accessible_masked_text: t( - 'idv.accessible_labels.masked_ssn', - first_number: attribute_value[0], - last_number: attribute_value[-1], - ), - ) %> - <% else %> - <%= attribute_value.to_s %> - <% end %> -
      -
    • - <% end %> -
    diff --git a/app/views/sign_up/completions/show.html.erb b/app/views/sign_up/completions/show.html.erb index c4b22e8af79..86d6e4bf864 100644 --- a/app/views/sign_up/completions/show.html.erb +++ b/app/views/sign_up/completions/show.html.erb @@ -3,16 +3,44 @@ <%= image_tag asset_url(@presenter.image_name), width: 140, alt: @presenter.image_alt, class: 'margin-bottom-2' %> -<%= render PageHeadingComponent.new(class: 'tablet:margin-right-1 tablet:margin-left-1 text-center') do %> +<%= render PageHeadingComponent.new(class: 'text-center') do %> <%= @presenter.heading %> <% end %> -

    +

    <%= @presenter.intro.html_safe %>

    -
    - <%= render 'sign_up/completions/requested_attributes', pii: @presenter.pii %> -
    + +<%= render IconListComponent.new(icon: :check_circle, color: :success, class: 'border-bottom border-primary-light') do |c| %> + <% @presenter.pii.each do |attribute_key, attribute_value| %> + <% next if attribute_value.blank? %> + <% c.with_item(class: 'padding-y-2 border-top border-primary-light') do %> + + <%= t("help_text.requested_attributes.#{attribute_key}") %> + + <% if attribute_value.is_a? Array %> +
      + <% attribute_value.each do |item| %> +
    • <%= item %>
    • + <% end %> +
    + <% elsif attribute_key == :social_security_number %> + <%= render( + 'shared/masked_text', + text: attribute_value, + masked_text: SsnFormatter.format_masked(attribute_value), + accessible_masked_text: t( + 'idv.accessible_labels.masked_ssn', + first_number: attribute_value[0], + last_number: attribute_value[-1], + ), + ) %> + <% else %> + <%= attribute_value.to_s %> + <% end %> + <% end %> + <% end %> +<% end %> <% if !@multiple_factors_enabled %> <%= render(AlertComponent.new(type: :warning, class: 'margin-bottom-4')) do %> diff --git a/app/views/users/authorization_confirmation/new.html.erb b/app/views/users/authorization_confirmation/new.html.erb index 04d03ab86f3..ea34d93ec14 100644 --- a/app/views/users/authorization_confirmation/new.html.erb +++ b/app/views/users/authorization_confirmation/new.html.erb @@ -6,30 +6,34 @@ <%= render PageHeadingComponent.new.with_content(decorated_sp_session.new_session_heading) %> <% end %> -
    -
    - <%= t('user_authorization_confirmation.currently_logged_in') %> -
    -
    -
      -
    • - - <%= t('help_text.requested_attributes.email') %> - - - <%= @email %> - -
    • -
    -
    +
    + <%= t('user_authorization_confirmation.currently_logged_in') %> +
    +<%= render IconListComponent.new(icon: :check_circle, color: :success, class: 'border-bottom border-primary-light') do |c| %> + <% c.with_item(class: 'padding-y-2 border-top border-primary-light') do %> + + <%= t('help_text.requested_attributes.email') %> + + <%= @email %> + <% end %> +<% end %> -
    - <%= button_to(user_authorization_confirmation_path, class: 'usa-button usa-button--big usa-button--wide', method: :post) { t('user_authorization_confirmation.continue') } %> -
    -
    +
    + <%= render ButtonComponent.new( + action: ->(**tag_options, &block) do + button_to(user_authorization_confirmation_path, method: :post, **tag_options, &block) + end, + big: true, + wide: true, + ).with_content(t('user_authorization_confirmation.continue')) %> +
    <%= t('user_authorization_confirmation.or') %>
    -
    - <%= button_to(reset_user_authorization_path, class: 'usa-button usa-button--big usa-button--wide', method: :delete) { t('user_authorization_confirmation.sign_in') } %> -
    + <%= render ButtonComponent.new( + action: ->(**tag_options, &block) do + button_to(reset_user_authorization_path, method: :delete, **tag_options, &block) + end, + big: true, + wide: true, + ).with_content(t('user_authorization_confirmation.sign_in')) %>
    diff --git a/spec/components/icon_list_component_spec.rb b/spec/components/icon_list_component_spec.rb index b414b631c23..599ede1b38c 100644 --- a/spec/components/icon_list_component_spec.rb +++ b/spec/components/icon_list_component_spec.rb @@ -16,9 +16,11 @@ end context 'with additional tag options' do - it 'applies tag options to wrapper element' do - rendered = render_inline IconListComponent.new(class: 'custom-class', data: { foo: 'bar' }) + subject(:rendered) do + render_inline IconListComponent.new(class: 'custom-class', data: { foo: 'bar' }) + end + it 'applies tag options to wrapper element' do expect(rendered).to have_css('.usa-icon-list.custom-class[data-foo="bar"]') end end @@ -65,5 +67,17 @@ expect(rendered).to have_css('.usa-icon use[href$=".svg#cancel"]', count: 1) end end + + context 'with additional tag options on item' do + subject(:rendered) do + render_inline IconListComponent.new(icon: :cancel) do |c| + c.with_item(class: 'custom-class', data: { foo: 'bar' }) { 'First' } + end + end + + it 'applies tag options to wrapper element' do + expect(rendered).to have_css('.usa-icon-list__item.custom-class[data-foo="bar"]') + end + end end end diff --git a/spec/features/openid_connect/authorization_confirmation_spec.rb b/spec/features/openid_connect/authorization_confirmation_spec.rb index 308ed93dade..86e3ec3123f 100644 --- a/spec/features/openid_connect/authorization_confirmation_spec.rb +++ b/spec/features/openid_connect/authorization_confirmation_spec.rb @@ -95,10 +95,8 @@ def create_user_and_remember_device perform_in_browser(:two) do confirm_email_in_a_different_browser(email) expect(current_path).to eq sign_up_completed_path - within('.requested-attributes') do - expect(page).to have_content t('help_text.requested_attributes.email') - expect(page).to have_content email - end + expect(page).to have_content t('help_text.requested_attributes.email') + expect(page).to have_content email click_agree_and_continue diff --git a/spec/features/saml/authorization_confirmation_spec.rb b/spec/features/saml/authorization_confirmation_spec.rb index 269be1ebd93..a9e075f2ad6 100644 --- a/spec/features/saml/authorization_confirmation_spec.rb +++ b/spec/features/saml/authorization_confirmation_spec.rb @@ -107,10 +107,8 @@ def create_user_and_remember_device perform_in_browser(:two) do confirm_email_in_a_different_browser(email) expect(current_path).to eq sign_up_completed_path - within('.requested-attributes') do - expect(page).to have_content t('help_text.requested_attributes.email') - expect(page).to have_content email - end + expect(page).to have_content t('help_text.requested_attributes.email') + expect(page).to have_content email click_agree_and_continue diff --git a/spec/features/saml/ial1_sso_spec.rb b/spec/features/saml/ial1_sso_spec.rb index af747adb3c8..e2ca596dfe9 100644 --- a/spec/features/saml/ial1_sso_spec.rb +++ b/spec/features/saml/ial1_sso_spec.rb @@ -16,15 +16,12 @@ perform_in_browser(:two) do confirm_email_in_a_different_browser(email) expect(current_path).to eq sign_up_completed_path - within('.requested-attributes') do - expect(page).to have_content t('help_text.requested_attributes.email') - expect(page).to have_content email - expect(page).to_not have_content t('help_text.requested_attributes.address') - expect(page).to_not have_content t('help_text.requested_attributes.birthdate') - expect(page).to_not have_content t('help_text.requested_attributes.phone') - expect(page). - to_not have_content t('help_text.requested_attributes.social_security_number') - end + expect(page).to have_content t('help_text.requested_attributes.email') + expect(page).to have_content email + expect(page).to_not have_content t('help_text.requested_attributes.address') + expect(page).to_not have_content t('help_text.requested_attributes.birthdate') + expect(page).to_not have_content t('help_text.requested_attributes.phone') + expect(page).to_not have_content t('help_text.requested_attributes.social_security_number') click_agree_and_continue diff --git a/spec/support/idv_examples/sp_requested_attributes.rb b/spec/support/idv_examples/sp_requested_attributes.rb index 53e942c1d2f..d2ee8560579 100644 --- a/spec/support/idv_examples/sp_requested_attributes.rb +++ b/spec/support/idv_examples/sp_requested_attributes.rb @@ -22,22 +22,20 @@ expect(current_path).to eq(sign_up_completed_path) - within('.requested-attributes') do - expect(page).to have_content t('help_text.requested_attributes.email') - expect(page).to have_content user.email - expect(page).to_not have_content t('help_text.requested_attributes.address') - expect(page).to_not have_content t('help_text.requested_attributes.birthdate') - expect(page).to have_content t('help_text.requested_attributes.full_name') - expect(page).to have_content 'FAKEY MCFAKERSON' - expect(page).to have_content t('help_text.requested_attributes.phone') - expect(page).to have_content '+1 202-555-1212' - expect(page).to have_content t('help_text.requested_attributes.social_security_number') - expect(page).to have_css( - '.masked-text__text', - text: DocAuthHelper::GOOD_SSN, - visible: :hidden, - ) - end + expect(page).to have_content t('help_text.requested_attributes.email') + expect(page).to have_content user.email + expect(page).to_not have_content t('help_text.requested_attributes.address') + expect(page).to_not have_content t('help_text.requested_attributes.birthdate') + expect(page).to have_content t('help_text.requested_attributes.full_name') + expect(page).to have_content 'FAKEY MCFAKERSON' + expect(page).to have_content t('help_text.requested_attributes.phone') + expect(page).to have_content '+1 202-555-1212' + expect(page).to have_content t('help_text.requested_attributes.social_security_number') + expect(page).to have_css( + '.masked-text__text', + text: DocAuthHelper::GOOD_SSN, + visible: :hidden, + ) end end @@ -88,18 +86,16 @@ expect(current_path).to eq(sign_up_completed_path) - within('.requested-attributes') do - expect(page).to have_content t('help_text.requested_attributes.email') - expect(page).to have_content user.email - expect(page).to_not have_content t('help_text.requested_attributes.address') - expect(page).to_not have_content t('help_text.requested_attributes.birthdate') - expect(page).to have_content t('help_text.requested_attributes.full_name') - expect(page).to have_content 'FAKEY MCFAKERSON' - expect(page).to have_content t('help_text.requested_attributes.phone') - expect(page).to have_content '+1 202-555-1212' - expect(page).to have_content t('help_text.requested_attributes.social_security_number') - expect(page).to have_content DocAuthHelper::GOOD_SSN - end + expect(page).to have_content t('help_text.requested_attributes.email') + expect(page).to have_content user.email + expect(page).to_not have_content t('help_text.requested_attributes.address') + expect(page).to_not have_content t('help_text.requested_attributes.birthdate') + expect(page).to have_content t('help_text.requested_attributes.full_name') + expect(page).to have_content 'FAKEY MCFAKERSON' + expect(page).to have_content t('help_text.requested_attributes.phone') + expect(page).to have_content '+1 202-555-1212' + expect(page).to have_content t('help_text.requested_attributes.social_security_number') + expect(page).to have_content DocAuthHelper::GOOD_SSN end end end From 4e018397ddf59aac85a865283d8c71fd400fee6c Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Thu, 16 Nov 2023 17:40:48 -0800 Subject: [PATCH 07/12] Update AAMVA test scripts (#9608) - Require optparse, test it - Use pretty-printing changelog: Internal, Testing, Add AAMVA test script covered by specs --- bin/aamva-test-cert | 75 +++++++++++++++++++------------- bin/aamva-test-connectivity | 2 +- spec/bin/aamva-test-cert_spec.rb | 54 +++++++++++++++++++++++ 3 files changed, 99 insertions(+), 32 deletions(-) create mode 100644 spec/bin/aamva-test-cert_spec.rb diff --git a/bin/aamva-test-cert b/bin/aamva-test-cert index 0d5ff682dae..fe66d0079db 100755 --- a/bin/aamva-test-cert +++ b/bin/aamva-test-cert @@ -3,38 +3,51 @@ ENV['LOGIN_TASK_LOG_LEVEL'] ||= 'warn' require_relative '../config/environment.rb' require 'aamva_test' - -auth_url = nil -verification_url = nil - -parser = OptionParser.new do |opts| - opts.banner = <<~EOM - Usage: #{$PROGRAM_NAME} --auth-url=AUTH_URL --verification-url=VERIFICATION_URL - - Tests AAMVA certificate against cert environment - - Options: - EOM - - opts.on('--auth-url=AUTH_URL', 'sets the auth url') do |url| - auth_url = url - end - - opts.on('--verification-url=VERIFICATION_URL', 'sets the verification url') do |url| - verification_url = url - end - - opts.on('--help', 'prints this help message') do - puts opts - exit 0 +require 'optparse' +require 'pp' + +class AamvaTestCert + def run(out: STDOUT, argv: ARGV) + auth_url = nil + verification_url = nil + show_help = false + + parser = OptionParser.new do |opts| + opts.banner = <<~EOM + Usage: #{$PROGRAM_NAME} --auth-url=AUTH_URL --verification-url=VERIFICATION_URL + + Tests AAMVA certificate against cert environment + + Options: + EOM + + opts.on('--auth-url=AUTH_URL', 'sets the auth url') do |url| + auth_url = url + end + + opts.on('--verification-url=VERIFICATION_URL', 'sets the verification url') do |url| + verification_url = url + end + + opts.on('--help', 'prints this help message') do + show_help = true + end + end + + parser.parse!(argv) + + if show_help + out.puts parser + exit 0 + elsif !auth_url || !verification_url + out.puts parser + exit 1 + else + PP.pp(AamvaTest.new.test_cert(auth_url:, verification_url:), out) + end end end -parser.parse!(ARGV) - -if !auth_url || !verification_url - puts parser - exit 1 +if $PROGRAM_NAME == __FILE__ + AamvaTestCert.new.run end - -puts AamvaTest.new.test_cert(auth_url:, verification_url:) diff --git a/bin/aamva-test-connectivity b/bin/aamva-test-connectivity index 9bf5c4cd461..bdb20e77ec1 100755 --- a/bin/aamva-test-connectivity +++ b/bin/aamva-test-connectivity @@ -4,4 +4,4 @@ ENV['LOGIN_TASK_LOG_LEVEL'] ||= 'warn' require_relative '../config/environment.rb' require 'aamva_test' -puts AamvaTest.new.test_connectivity +p AamvaTest.new.test_connectivity diff --git a/spec/bin/aamva-test-cert_spec.rb b/spec/bin/aamva-test-cert_spec.rb new file mode 100644 index 00000000000..12658ac3abc --- /dev/null +++ b/spec/bin/aamva-test-cert_spec.rb @@ -0,0 +1,54 @@ +require 'rails_helper' +load Rails.root.join('bin/aamva-test-cert') + +RSpec.describe AamvaTestCert do + let(:fake_aamva_test) do + Class.new do + def test_cert(auth_url:, verification_url:) + [auth_url, verification_url] + end + end + end + + before { stub_const('AamvaTest', fake_aamva_test) } + + subject(:instance) { AamvaTestCert.new } + + describe '#run' do + subject(:run) { instance.run(out: out, argv: argv) } + let(:out) { StringIO.new('') } + let(:argv) { [] } + + context 'missing arguments' do + let(:argv) { [] } + + it 'exits uncleanly' do + expect(instance).to receive(:exit).with(1) + + run + end + end + + context '--help' do + let(:argv) { %w[--help] } + + it 'exits cleanly and prints help' do + expect(instance).to receive(:exit).with(0) + + run + + expect(out.string).to include('Usage:') + end + end + + context 'required arguments' do + let(:argv) { %w[--auth-url a --verification-url b] } + + it 'pretty-prints the result' do + run + + expect(JSON.parse(out.string)).to eq(%w[a b]) + end + end + end +end From 992c728f3e5056b6bdad7e7dbb029d45453da6d8 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Fri, 17 Nov 2023 08:20:02 -0500 Subject: [PATCH 08/12] LG 11432 Prevent duplicate F/T setup if user hits back button on second MFA prompt (#9587) * changelog: User-Facing Improvements, Webauthn, Prevent duplicate F/T setup on second MFA prompt * add spec coverage for platform auth redirect * move test to a function * move conditional to before_action * clean up validate platform authenticator method --- app/controllers/users/webauthn_setup_controller.rb | 12 ++++++++++++ .../users/webauthn_setup_controller_spec.rb | 12 ++++++++++++ 2 files changed, 24 insertions(+) diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index 4f898eb9f89..d21ea2839ef 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/app/controllers/users/webauthn_setup_controller.rb @@ -10,6 +10,7 @@ class WebauthnSetupController < ApplicationController before_action :apply_secure_headers_override before_action :set_webauthn_setup_presenter before_action :confirm_recently_authenticated_2fa + before_action :validate_existing_platform_authenticator helper_method :in_multi_mfa_selection_flow? @@ -108,6 +109,17 @@ def show_delete private + def validate_existing_platform_authenticator + if platform_authenticator? && in_account_creation_flow? && + current_user.webauthn_configurations.platform_authenticators.present? + redirect_to authentication_methods_setup_path + end + end + + def platform_authenticator? + params[:platform] == 'true' + end + def set_webauthn_setup_presenter @presenter = SetupPresenter.new( current_user: current_user, diff --git a/spec/controllers/users/webauthn_setup_controller_spec.rb b/spec/controllers/users/webauthn_setup_controller_spec.rb index aed625d9d2b..96758345f05 100644 --- a/spec/controllers/users/webauthn_setup_controller_spec.rb +++ b/spec/controllers/users/webauthn_setup_controller_spec.rb @@ -230,6 +230,18 @@ expect(additional_mfa_check).to be_truthy end end + + context 'when the back button is clicked after platform is added' do + let(:user) { create(:user, :with_webauthn_platform) } + before do + controller.user_session[:in_account_creation_flow] = true + end + it 'should redirect to authentication methods setup' do + get :new, params: { platform: true } + + expect(response).to redirect_to(authentication_methods_setup_path) + end + end end describe 'multiple MFA handling' do From 515a9197a6ebfeea31c63ef68bd4aad0cb3cb479 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Fri, 17 Nov 2023 08:28:09 -0500 Subject: [PATCH 09/12] LG 11145 Break up MFA selection presenter classes for Phone Presenters (#9560) * changelog: Internal, tech debt, Break up MFA presenter class for phone * split phone, voice, and sms presenter classes up * add tests cases for sign_in and set_up phone presenter class * split setup signin presenter spec for voice and sms * lint fix * remove old phone selection presenter spec * rename phone presenter in spec * remove unneeded configuration variable, leverage user for type method * remove info method from phone sub classes * update options presenter spec with newly split classes * merge sms and voice presenters * revise specs according to merged classes * remove deprecated spec and lint fix * change info to switch and fix regression with disabled? method * lint fix * fix spec * remove deprecated translations from setup presenter * move reader :method to phone sign in presenter * fix lint * fix lint * clean up selection presenter class * remove unneeded configuration setting * remove configuration from set up presenter spec * add sms and voice outage spec, standardize spec syntax * clarify some syntax --- app/models/phone_configuration.rb | 6 +- .../selection_presenter.rb | 20 +--- ...rb => set_up_phone_selection_presenter.rb} | 10 +- .../sign_in_phone_selection_presenter.rb | 55 +++++++++++ .../sms_selection_presenter.rb | 22 ----- .../voice_selection_presenter.rb | 22 ----- .../two_factor_options_presenter.rb | 4 +- .../locales/two_factor_authentication/en.yml | 2 - .../locales/two_factor_authentication/es.yml | 2 - .../locales/two_factor_authentication/fr.yml | 2 - ... set_up_phone_selection_presenter_spec.rb} | 11 +-- .../sign_in_phone_selection_presenter_spec.rb | 92 +++++++++++++++++++ .../sms_selection_presenter_spec.rb | 52 ----------- .../voice_selection_presenter_spec.rb | 52 ----------- ...two_factor_login_options_presenter_spec.rb | 12 +-- .../two_factor_options_presenter_spec.rb | 4 +- 16 files changed, 169 insertions(+), 199 deletions(-) rename app/presenters/two_factor_authentication/{phone_selection_presenter.rb => set_up_phone_selection_presenter.rb} (61%) create mode 100644 app/presenters/two_factor_authentication/sign_in_phone_selection_presenter.rb delete mode 100644 app/presenters/two_factor_authentication/sms_selection_presenter.rb delete mode 100644 app/presenters/two_factor_authentication/voice_selection_presenter.rb rename spec/presenters/two_factor_authentication/{phone_selection_presenter_spec.rb => set_up_phone_selection_presenter_spec.rb} (82%) create mode 100644 spec/presenters/two_factor_authentication/sign_in_phone_selection_presenter_spec.rb delete mode 100644 spec/presenters/two_factor_authentication/sms_selection_presenter_spec.rb delete mode 100644 spec/presenters/two_factor_authentication/voice_selection_presenter_spec.rb diff --git a/app/models/phone_configuration.rb b/app/models/phone_configuration.rb index 15311c0ffef..be629ce82e0 100644 --- a/app/models/phone_configuration.rb +++ b/app/models/phone_configuration.rb @@ -22,11 +22,13 @@ def selection_presenters capabilities = PhoneNumberCapabilities.new(phone, phone_confirmed: !!confirmed_at?) if capabilities.supports_sms? - options << TwoFactorAuthentication::SmsSelectionPresenter.new(user:, configuration: self) + options << TwoFactorAuthentication::SignInPhoneSelectionPresenter. + new(user:, configuration: self, method: :sms) end if capabilities.supports_voice? - options << TwoFactorAuthentication::VoiceSelectionPresenter.new(user:, configuration: self) + options << TwoFactorAuthentication::SignInPhoneSelectionPresenter. + new(user:, configuration: self, method: :voice) end options diff --git a/app/presenters/two_factor_authentication/selection_presenter.rb b/app/presenters/two_factor_authentication/selection_presenter.rb index 3a52380e482..3926937cd01 100644 --- a/app/presenters/two_factor_authentication/selection_presenter.rb +++ b/app/presenters/two_factor_authentication/selection_presenter.rb @@ -68,27 +68,11 @@ def disabled? private def login_label(type) - case type - when 'sms' - t('two_factor_authentication.login_options.sms') - when 'voice' - t('two_factor_authentication.login_options.voice') - else - raise "Unsupported login method: #{type}" - end + raise "Unsupported login method: #{type}" end def setup_label(type) - case type - when 'phone' - t('two_factor_authentication.two_factor_choice_options.phone') - when 'sms' - t('two_factor_authentication.two_factor_choice_options.sms') - when 'voice' - t('two_factor_authentication.two_factor_choice_options.voice') - else - raise "Unsupported setup method: #{type}" - end + raise "Unsupported setup method: #{type}" end def login_info(type) diff --git a/app/presenters/two_factor_authentication/phone_selection_presenter.rb b/app/presenters/two_factor_authentication/set_up_phone_selection_presenter.rb similarity index 61% rename from app/presenters/two_factor_authentication/phone_selection_presenter.rb rename to app/presenters/two_factor_authentication/set_up_phone_selection_presenter.rb index 398fb43d38c..b57bf159a8d 100644 --- a/app/presenters/two_factor_authentication/phone_selection_presenter.rb +++ b/app/presenters/two_factor_authentication/set_up_phone_selection_presenter.rb @@ -1,15 +1,11 @@ module TwoFactorAuthentication - class PhoneSelectionPresenter < SelectionPresenter + class SetUpPhoneSelectionPresenter < SetUpSelectionPresenter def method :phone end - def type - if MfaContext.new(configuration&.user).phone_configurations.many? - "#{super}_#{configuration.id}" - else - super - end + def label + t('two_factor_authentication.two_factor_choice_options.phone') end def info diff --git a/app/presenters/two_factor_authentication/sign_in_phone_selection_presenter.rb b/app/presenters/two_factor_authentication/sign_in_phone_selection_presenter.rb new file mode 100644 index 00000000000..207b9a82e71 --- /dev/null +++ b/app/presenters/two_factor_authentication/sign_in_phone_selection_presenter.rb @@ -0,0 +1,55 @@ +module TwoFactorAuthentication + class SignInPhoneSelectionPresenter < SignInSelectionPresenter + attr_reader :configuration, :user, :method + + def initialize(user:, configuration:, method:) + @user = user + @configuration = configuration + @method = method + end + + def type + if MfaContext.new(configuration&.user).phone_configurations.many? + "#{super}_#{configuration.id}" + else + super + end + end + + def label + if method.to_s == 'sms' + t('two_factor_authentication.login_options.sms') + elsif method.to_s == 'voice' + t('two_factor_authentication.login_options.voice') + end + end + + def info + case method + when :sms + t( + 'two_factor_authentication.login_options.sms_info_html', + phone: configuration.masked_phone, + ) + when :voice + t( + 'two_factor_authentication.login_options.voice_info_html', + phone: configuration.masked_phone, + ) + else + t('two_factor_authentication.two_factor_choice_options.phone_info') + end + end + + def disabled? + case method + when :sms + OutageStatus.new.vendor_outage?(:sms) + when :voice + OutageStatus.new.vendor_outage?(:voice) + else + OutageStatus.new.all_phone_vendor_outage? + end + end + end +end diff --git a/app/presenters/two_factor_authentication/sms_selection_presenter.rb b/app/presenters/two_factor_authentication/sms_selection_presenter.rb deleted file mode 100644 index 3226ae6adc3..00000000000 --- a/app/presenters/two_factor_authentication/sms_selection_presenter.rb +++ /dev/null @@ -1,22 +0,0 @@ -module TwoFactorAuthentication - class SmsSelectionPresenter < PhoneSelectionPresenter - def method - :sms - end - - def info - if configuration.present? - t( - 'two_factor_authentication.login_options.sms_info_html', - phone: configuration.masked_phone, - ) - else - super - end - end - - def disabled? - OutageStatus.new.vendor_outage?(:sms) - end - end -end diff --git a/app/presenters/two_factor_authentication/voice_selection_presenter.rb b/app/presenters/two_factor_authentication/voice_selection_presenter.rb deleted file mode 100644 index 369878c5e5d..00000000000 --- a/app/presenters/two_factor_authentication/voice_selection_presenter.rb +++ /dev/null @@ -1,22 +0,0 @@ -module TwoFactorAuthentication - class VoiceSelectionPresenter < PhoneSelectionPresenter - def method - :voice - end - - def info - if configuration.present? - t( - 'two_factor_authentication.login_options.voice_info_html', - phone: configuration.masked_phone, - ) - else - super - end - end - - def disabled? - OutageStatus.new.vendor_outage?(:voice) - end - end -end diff --git a/app/presenters/two_factor_options_presenter.rb b/app/presenters/two_factor_options_presenter.rb index 7466c3c8f3d..56a4567a328 100644 --- a/app/presenters/two_factor_options_presenter.rb +++ b/app/presenters/two_factor_options_presenter.rb @@ -34,7 +34,7 @@ def all_user_selected_options [ TwoFactorAuthentication::SetUpWebauthnPlatformSelectionPresenter.new(user: user), TwoFactorAuthentication::SetUpAuthAppSelectionPresenter.new(user: user), - TwoFactorAuthentication::PhoneSelectionPresenter.new(user: user), + TwoFactorAuthentication::SetUpPhoneSelectionPresenter.new(user: user), TwoFactorAuthentication::SetUpBackupCodeSelectionPresenter.new(user: user), TwoFactorAuthentication::SetUpWebauthnSelectionPresenter.new(user: user), TwoFactorAuthentication::SetUpPivCacSelectionPresenter.new(user: user), @@ -116,7 +116,7 @@ def phone_options if piv_cac_required? || phishing_resistant_only? || IdentityConfig.store.hide_phone_mfa_signup return [] else - [TwoFactorAuthentication::PhoneSelectionPresenter.new(user: user)] + [TwoFactorAuthentication::SetUpPhoneSelectionPresenter.new(user: user)] end end diff --git a/config/locales/two_factor_authentication/en.yml b/config/locales/two_factor_authentication/en.yml index 13a73ae2513..bac52964f9d 100644 --- a/config/locales/two_factor_authentication/en.yml +++ b/config/locales/two_factor_authentication/en.yml @@ -163,8 +163,6 @@ en: (toll) phone numbers. piv_cac: Government employee ID piv_cac_info: PIV/CAC cards for government and military employees. Desktop only. - sms: Text message / SMS - voice: Phone call webauthn: Security key webauthn_info: A physical device, often shaped like a USB drive, that you plug in to your device. diff --git a/config/locales/two_factor_authentication/es.yml b/config/locales/two_factor_authentication/es.yml index 2f079bd6027..c2a30013de9 100644 --- a/config/locales/two_factor_authentication/es.yml +++ b/config/locales/two_factor_authentication/es.yml @@ -173,8 +173,6 @@ es: piv_cac: Identificación de empleado gubernamental piv_cac_info: Credenciales PIV/CAC para empleados gubernamentales y del ejército. Únicamente versión de escritorio. - sms: Mensaje de texto / SMS - voice: Llamada telefónica webauthn: Clave de seguridad webauthn_info: Un dispositivo físico que suele tener la forma de una unidad USB y se conecta a su dispositivo. diff --git a/config/locales/two_factor_authentication/fr.yml b/config/locales/two_factor_authentication/fr.yml index ce17a3ce3a3..52dee824d9f 100644 --- a/config/locales/two_factor_authentication/fr.yml +++ b/config/locales/two_factor_authentication/fr.yml @@ -182,8 +182,6 @@ fr: piv_cac: Carte d’identification des employés du gouvernement piv_cac_info: Cartes PIV/CAC pour les fonctionnaires et les militaires. Bureau uniquement. - sms: SMS - voice: Appel téléphonique webauthn: Clef de sécurité webauthn_info: Un appareil physique, souvent en forme de clé USB, que vous branchez sur votre appareil. diff --git a/spec/presenters/two_factor_authentication/phone_selection_presenter_spec.rb b/spec/presenters/two_factor_authentication/set_up_phone_selection_presenter_spec.rb similarity index 82% rename from spec/presenters/two_factor_authentication/phone_selection_presenter_spec.rb rename to spec/presenters/two_factor_authentication/set_up_phone_selection_presenter_spec.rb index b22167d42f3..8c9e9137218 100644 --- a/spec/presenters/two_factor_authentication/phone_selection_presenter_spec.rb +++ b/spec/presenters/two_factor_authentication/set_up_phone_selection_presenter_spec.rb @@ -1,15 +1,13 @@ require 'rails_helper' -RSpec.describe TwoFactorAuthentication::PhoneSelectionPresenter do +RSpec.describe TwoFactorAuthentication::SetUpPhoneSelectionPresenter do let(:user_without_mfa) { create(:user) } let(:user_with_mfa) { create(:user, :with_phone) } - let(:presenter_with_mfa) { described_class.new(configuration: phone, user: user_with_mfa) } - let(:presenter_without_mfa) { described_class.new(configuration: phone, user: user_without_mfa) } + let(:presenter_with_mfa) { described_class.new(user: user_with_mfa) } + let(:presenter_without_mfa) { described_class.new(user: user_without_mfa) } describe '#info' do context 'when a user does not have a phone configuration (first time)' do - let(:phone) { nil } - it 'includes a note about choosing voice or sms' do expect(presenter_without_mfa.info). to include(t('two_factor_authentication.two_factor_choice_options.phone_info')) @@ -28,8 +26,6 @@ end describe '#disabled?' do - let(:phone) { build(:phone_configuration, phone: '+1 888 867-5309') } - it { expect(presenter_without_mfa.disabled?).to eq(false) } context 'all phone vendor outage' do @@ -42,7 +38,6 @@ end describe '#mfa_configuration' do - let(:phone) { nil } it 'returns an empty string when user has not configured this authenticator' do expect(presenter_without_mfa.mfa_configuration_description).to eq('') end diff --git a/spec/presenters/two_factor_authentication/sign_in_phone_selection_presenter_spec.rb b/spec/presenters/two_factor_authentication/sign_in_phone_selection_presenter_spec.rb new file mode 100644 index 00000000000..360ab52daf5 --- /dev/null +++ b/spec/presenters/two_factor_authentication/sign_in_phone_selection_presenter_spec.rb @@ -0,0 +1,92 @@ +require 'rails_helper' + +RSpec.describe TwoFactorAuthentication::SignInPhoneSelectionPresenter do + let(:user) { create(:user, :with_phone) } + let(:configuration) { create(:phone_configuration, user: user) } + + let(:presenter) do + described_class.new(user: user, configuration: configuration, method: 'phone') + end + + describe '#type' do + it 'returns phone appended with configuration id' do + expect(presenter.type).to eq "phone_#{configuration.id}" + end + end + + describe '#info' do + it 'raises with missing translation' do + expect(presenter.info).to eq( + t('two_factor_authentication.two_factor_choice_options.phone_info'), + ) + end + context 'with sms method' do + let(:presenter) do + described_class.new(user: user, configuration: configuration, method: :sms) + end + it 'returns the correct translation for sms' do + expect(presenter.info).to eq( + t( + 'two_factor_authentication.login_options.sms_info_html', + phone: configuration.masked_phone, + ), + ) + end + end + context 'with voice method' do + let(:presenter) do + described_class.new(user: user, configuration: configuration, method: :voice) + end + it 'returns the correct translation for voice' do + expect(presenter.info).to eq( + t( + 'two_factor_authentication.login_options.voice_info_html', + phone: configuration.masked_phone, + ), + ) + end + end + end + + describe '#disabled?' do + let(:user_without_mfa) { create(:user) } + let(:phone) { build(:phone_configuration, phone: '+1 888 867-5309') } + let(:presenter_without_mfa) do + described_class.new(configuration: phone, user: user_without_mfa, method: 'phone') + end + it { expect(presenter_without_mfa.disabled?).to eq(false) } + + context 'all phone vendor outage' do + before do + allow_any_instance_of(OutageStatus).to receive(:all_phone_vendor_outage?).and_return(true) + end + + it { expect(presenter_without_mfa.disabled?).to eq(true) } + end + + context 'voice vendor outage' do + let(:presenter_without_mfa) do + described_class.new(configuration: phone, user: user, method: method) + end + let(:method) { :voice } + before do + allow_any_instance_of(OutageStatus).to receive(:vendor_outage?).with(:voice). + and_return(true) + end + + it { expect(presenter_without_mfa.disabled?).to eq(true) } + end + + context 'sms vendor outage' do + let(:presenter_without_mfa) do + described_class.new(configuration: phone, user: user, method: method) + end + let(:method) { :sms } + before do + allow_any_instance_of(OutageStatus).to receive(:vendor_outage?).with(:sms).and_return(true) + end + + it { expect(presenter_without_mfa.disabled?).to eq(true) } + end + end +end diff --git a/spec/presenters/two_factor_authentication/sms_selection_presenter_spec.rb b/spec/presenters/two_factor_authentication/sms_selection_presenter_spec.rb deleted file mode 100644 index 54064ce07d3..00000000000 --- a/spec/presenters/two_factor_authentication/sms_selection_presenter_spec.rb +++ /dev/null @@ -1,52 +0,0 @@ -require 'rails_helper' - -RSpec.describe TwoFactorAuthentication::SmsSelectionPresenter do - let(:subject) { described_class.new(configuration: phone, user: user) } - let(:user) { build(:user) } - - describe '#type' do - context 'when a user has only one phone configuration' do - let(:user) { create(:user, :with_phone) } - let(:phone) { MfaContext.new(user).phone_configurations.first } - - it 'returns sms' do - expect(subject.type).to eq 'sms' - end - end - - context 'when a user has more than one phone configuration' do - let(:user) { create(:user, :with_phone) } - let(:phone) do - record = create(:phone_configuration, user: user) - user.reload - record - end - - it 'returns sms:id' do - expect(subject.type).to eq "sms_#{phone.id}" - end - end - end - - describe '#info' do - context 'when a user has a phone configuration' do - let(:phone) { build(:phone_configuration, phone: '+1 888 867-5309') } - it 'includes the masked the number' do - expect(subject.info).to include('(***) ***-5309') - end - end - end - - describe '#disabled?' do - let(:phone) { build(:phone_configuration, phone: '+1 888 867-5309') } - it { expect(subject.disabled?).to eq(false) } - - context 'sms vendor outage' do - before do - allow_any_instance_of(OutageStatus).to receive(:vendor_outage?).with(:sms).and_return(true) - end - - it { expect(subject.disabled?).to eq(true) } - end - end -end diff --git a/spec/presenters/two_factor_authentication/voice_selection_presenter_spec.rb b/spec/presenters/two_factor_authentication/voice_selection_presenter_spec.rb deleted file mode 100644 index edb1d87d6d3..00000000000 --- a/spec/presenters/two_factor_authentication/voice_selection_presenter_spec.rb +++ /dev/null @@ -1,52 +0,0 @@ -require 'rails_helper' - -RSpec.describe TwoFactorAuthentication::VoiceSelectionPresenter do - let(:subject) { described_class.new(configuration: phone, user: user) } - let(:user) { build(:user) } - describe '#type' do - context 'when a user has only one phone configuration' do - let(:user) { create(:user, :with_phone) } - let(:phone) { MfaContext.new(user).phone_configurations.first } - - it 'returns voice' do - expect(subject.type).to eq 'voice' - end - end - - context 'when a user has more than one phone configuration' do - let(:user) { create(:user, :with_phone) } - let(:phone) do - record = create(:phone_configuration, user: user) - user.reload - record - end - - it 'returns voice:id' do - expect(subject.type).to eq "voice_#{phone.id}" - end - end - end - - describe '#info' do - context 'when a user has a phone configuration' do - let(:phone) { build(:phone_configuration, phone: '+1 888 867-5309') } - it 'includes the masked the number' do - expect(subject.info).to include('(***) ***-5309') - end - end - end - - describe '#disabled?' do - let(:phone) { build(:phone_configuration, phone: '+1 888 867-5309') } - it { expect(subject.disabled?).to eq(false) } - - context 'voice vendor outage' do - before do - allow_any_instance_of(OutageStatus).to receive(:vendor_outage?).with(:voice). - and_return(true) - end - - it { expect(subject.disabled?).to eq(true) } - end - end -end diff --git a/spec/presenters/two_factor_login_options_presenter_spec.rb b/spec/presenters/two_factor_login_options_presenter_spec.rb index d1ff9ad4dfc..1386d500dd9 100644 --- a/spec/presenters/two_factor_login_options_presenter_spec.rb +++ b/spec/presenters/two_factor_login_options_presenter_spec.rb @@ -82,8 +82,8 @@ it 'returns classes for mfas associated with account' do expect(options_classes).to eq( [ - TwoFactorAuthentication::SmsSelectionPresenter, - TwoFactorAuthentication::VoiceSelectionPresenter, + TwoFactorAuthentication::SignInPhoneSelectionPresenter, + TwoFactorAuthentication::SignInPhoneSelectionPresenter, TwoFactorAuthentication::SignInWebauthnSelectionPresenter, TwoFactorAuthentication::SignInBackupCodeSelectionPresenter, TwoFactorAuthentication::SignInPivCacSelectionPresenter, @@ -114,8 +114,8 @@ it 'returns all mfas associated with account' do expect(options_classes).to eq( [ - TwoFactorAuthentication::SmsSelectionPresenter, - TwoFactorAuthentication::VoiceSelectionPresenter, + TwoFactorAuthentication::SignInPhoneSelectionPresenter, + TwoFactorAuthentication::SignInPhoneSelectionPresenter, TwoFactorAuthentication::SignInWebauthnSelectionPresenter, TwoFactorAuthentication::SignInBackupCodeSelectionPresenter, TwoFactorAuthentication::SignInPivCacSelectionPresenter, @@ -145,8 +145,8 @@ it 'returns all mfas associated with account' do expect(options_classes).to eq( [ - TwoFactorAuthentication::SmsSelectionPresenter, - TwoFactorAuthentication::VoiceSelectionPresenter, + TwoFactorAuthentication::SignInPhoneSelectionPresenter, + TwoFactorAuthentication::SignInPhoneSelectionPresenter, TwoFactorAuthentication::SignInWebauthnSelectionPresenter, TwoFactorAuthentication::SignInBackupCodeSelectionPresenter, TwoFactorAuthentication::SignInPivCacSelectionPresenter, diff --git a/spec/presenters/two_factor_options_presenter_spec.rb b/spec/presenters/two_factor_options_presenter_spec.rb index d644a20b6e4..97233969597 100644 --- a/spec/presenters/two_factor_options_presenter_spec.rb +++ b/spec/presenters/two_factor_options_presenter_spec.rb @@ -27,7 +27,7 @@ expect(presenter.options.map(&:class)).to eq [ TwoFactorAuthentication::SetUpWebauthnPlatformSelectionPresenter, TwoFactorAuthentication::SetUpAuthAppSelectionPresenter, - TwoFactorAuthentication::PhoneSelectionPresenter, + TwoFactorAuthentication::SetUpPhoneSelectionPresenter, TwoFactorAuthentication::SetUpBackupCodeSelectionPresenter, TwoFactorAuthentication::SetUpWebauthnSelectionPresenter, TwoFactorAuthentication::SetUpPivCacSelectionPresenter, @@ -76,7 +76,7 @@ expect(presenter.options.map(&:class)).to eq [ TwoFactorAuthentication::SetUpWebauthnPlatformSelectionPresenter, TwoFactorAuthentication::SetUpAuthAppSelectionPresenter, - TwoFactorAuthentication::PhoneSelectionPresenter, + TwoFactorAuthentication::SetUpPhoneSelectionPresenter, TwoFactorAuthentication::SetUpBackupCodeSelectionPresenter, TwoFactorAuthentication::SetUpWebauthnSelectionPresenter, TwoFactorAuthentication::SetUpPivCacSelectionPresenter, From 1611540dcf0c03defba52757d9acd2bb424d68c1 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 17 Nov 2023 11:18:37 -0500 Subject: [PATCH 10/12] Refactor WebauthnVerificationForm to handle error messages (#9613) changelog: Internal, Code Quality, Move error messages for WebAuthn verification to form class --- .../webauthn_verification_controller.rb | 22 +++----- app/forms/webauthn_verification_form.rb | 52 +++++++++++++++---- spec/forms/webauthn_verification_form_spec.rb | 16 ++---- 3 files changed, 51 insertions(+), 39 deletions(-) diff --git a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb index 0edb08b8fb1..28ba92782ba 100644 --- a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb +++ b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb @@ -36,7 +36,7 @@ def handle_webauthn_result(result) if result.success? handle_valid_webauthn else - handle_invalid_webauthn + handle_invalid_webauthn(result) end end @@ -54,24 +54,12 @@ def handle_valid_webauthn redirect_to after_sign_in_path_for(current_user) end - def handle_invalid_webauthn + def handle_invalid_webauthn(result) + flash[:error] = result.first_error_message + if platform_authenticator? - flash[:error] = t( - 'two_factor_authentication.webauthn_error.try_again', - link: view_context.link_to( - t('two_factor_authentication.webauthn_error.additional_methods_link'), - login_two_factor_options_path, - ), - ) redirect_to login_two_factor_webauthn_url(platform: 'true') else - flash[:error] = t( - 'two_factor_authentication.webauthn_error.connect_html', - link_html: view_context.link_to( - t('two_factor_authentication.webauthn_error.additional_methods_link'), - login_two_factor_options_path, - ), - ) redirect_to login_two_factor_webauthn_url end end @@ -124,6 +112,8 @@ def analytics_properties def form @form ||= WebauthnVerificationForm.new( user: current_user, + platform_authenticator: platform_authenticator?, + url_options:, challenge: user_session[:webauthn_challenge], protocol: request.protocol, authenticator_data: params[:authenticator_data], diff --git a/app/forms/webauthn_verification_form.rb b/app/forms/webauthn_verification_form.rb index fa51beca58a..56b13c77334 100644 --- a/app/forms/webauthn_verification_form.rb +++ b/app/forms/webauthn_verification_form.rb @@ -1,19 +1,28 @@ # The WebauthnVerificationForm class is responsible for validating webauthn verification input class WebauthnVerificationForm include ActiveModel::Model + include ActionView::Helpers::UrlHelper + include ActionView::Helpers::TranslationHelper + include Rails.application.routes.url_helpers - validates :user, presence: true - validates :challenge, presence: true - validates :authenticator_data, presence: true - validates :client_data_json, presence: true - validates :signature, presence: true - validates :webauthn_configuration, presence: true + validates :challenge, + :authenticator_data, + :client_data_json, + :signature, + :webauthn_configuration, + presence: { message: proc { |object| object.instance_eval { generic_error_message } } } validate :validate_assertion_response validate :validate_webauthn_error + attr_reader :url_options, :platform_authenticator + + alias_method :platform_authenticator?, :platform_authenticator + def initialize( + user:, + platform_authenticator:, + url_options:, protocol:, - user: nil, challenge: nil, authenticator_data: nil, client_data_json: nil, @@ -22,6 +31,9 @@ def initialize( webauthn_error: nil ) @user = user + @platform_authenticator = platform_authenticator + @url_options = url_options + @protocol = protocol @challenge = challenge @protocol = protocol @authenticator_data = authenticator_data @@ -43,7 +55,7 @@ def submit def webauthn_configuration return @webauthn_configuration if defined?(@webauthn_configuration) - @webauthn_configuration = user&.webauthn_configurations&.find_by(credential_id: credential_id) + @webauthn_configuration = user.webauthn_configurations.find_by(credential_id: credential_id) end # this gives us a hook to override the domain embedded in the attestation test object @@ -64,12 +76,12 @@ def self.domain_name def validate_assertion_response return if webauthn_error.present? || webauthn_configuration.blank? || valid_assertion_response? - errors.add(:authenticator_data, 'invalid_authenticator_data', type: :invalid_authenticator_data) + errors.add(:authenticator_data, :invalid_authenticator_data, message: generic_error_message) end def validate_webauthn_error return if webauthn_error.blank? - errors.add(:webauthn_error, webauthn_error, type: :webauthn_error) + errors.add(:webauthn_error, :webauthn_error, message: generic_error_message) end def valid_assertion_response? @@ -99,6 +111,26 @@ def public_key webauthn_configuration&.credential_public_key end + def generic_error_message + if platform_authenticator? + t( + 'two_factor_authentication.webauthn_error.try_again', + link: link_to( + t('two_factor_authentication.webauthn_error.additional_methods_link'), + login_two_factor_options_path, + ), + ) + else + t( + 'two_factor_authentication.webauthn_error.connect_html', + link_html: link_to( + t('two_factor_authentication.webauthn_error.additional_methods_link'), + login_two_factor_options_path, + ), + ) + end + end + def extra_analytics_attributes auth_method = if webauthn_configuration&.platform_authenticator 'webauthn_platform' diff --git a/spec/forms/webauthn_verification_form_spec.rb b/spec/forms/webauthn_verification_form_spec.rb index 448b6e707b4..5a32fcba680 100644 --- a/spec/forms/webauthn_verification_form_spec.rb +++ b/spec/forms/webauthn_verification_form_spec.rb @@ -1,6 +1,7 @@ require 'rails_helper' RSpec.describe WebauthnVerificationForm do + include Rails.application.routes.url_helpers include WebAuthnHelper let(:user) { create(:user) } @@ -22,6 +23,8 @@ subject(:form) do WebauthnVerificationForm.new( user: user, + platform_authenticator:, + url_options: {}, challenge: challenge, protocol: protocol, authenticator_data: authenticator_data, @@ -62,19 +65,6 @@ end context 'when the input is invalid' do - context 'when user is missing' do - let(:user) { nil } - - it 'returns unsuccessful result' do - expect(result.to_h).to eq( - success: false, - error_details: { user: { blank: true }, webauthn_configuration: { blank: true } }, - multi_factor_auth_method: 'webauthn', - webauthn_configuration_id: nil, - ) - end - end - context 'when challenge is missing' do let(:challenge) { nil } From 7f80d9d560199174afb2caa59dd3d1d7e27adadb Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 17 Nov 2023 11:44:31 -0500 Subject: [PATCH 11/12] Add analytics property for WebAuthn sign-in frontend error (#9611) * Add analytics property for WebAuthn sign-in frontend error changelog: Internal, Analytics, Add analytics property for WebAuthn sign-in frontend error * Document frontend_error --- app/forms/webauthn_verification_form.rb | 3 ++- app/services/analytics_events.rb | 3 +++ .../webauthn_verification_controller_spec.rb | 1 + spec/forms/webauthn_verification_form_spec.rb | 16 ++++++++++++++-- 4 files changed, 20 insertions(+), 3 deletions(-) diff --git a/app/forms/webauthn_verification_form.rb b/app/forms/webauthn_verification_form.rb index 56b13c77334..a099e96a97c 100644 --- a/app/forms/webauthn_verification_form.rb +++ b/app/forms/webauthn_verification_form.rb @@ -141,6 +141,7 @@ def extra_analytics_attributes { multi_factor_auth_method: auth_method, webauthn_configuration_id: webauthn_configuration&.id, - } + frontend_error: webauthn_error.presence, + }.compact end end diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 90f75fa6764..46099747227 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -3123,6 +3123,7 @@ def logout_initiated( # @param [String] area_code # @param [String] country_code # @param [String] phone_fingerprint the hmac fingerprint of the phone number formatted as e164 + # @param [String] frontend_error Name of error that occurred in frontend during submission # Multi-Factor Authentication def multi_factor_auth( success:, @@ -3140,6 +3141,7 @@ def multi_factor_auth( area_code: nil, country_code: nil, phone_fingerprint: nil, + frontend_error: nil, **extra ) track_event( @@ -3159,6 +3161,7 @@ def multi_factor_auth( area_code: area_code, country_code: country_code, phone_fingerprint: phone_fingerprint, + frontend_error:, **extra, ) end diff --git a/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb index d61ff7e8acf..b5a99a46986 100644 --- a/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb @@ -291,6 +291,7 @@ multi_factor_auth_method_created_at: second_webauthn_platform_configuration.created_at.strftime('%s%L'), webauthn_configuration_id: nil, + frontend_error: 'NotAllowedError', ) patch :confirm, params: params diff --git a/spec/forms/webauthn_verification_form_spec.rb b/spec/forms/webauthn_verification_form_spec.rb index 5a32fcba680..db840d198e9 100644 --- a/spec/forms/webauthn_verification_form_spec.rb +++ b/spec/forms/webauthn_verification_form_spec.rb @@ -62,6 +62,18 @@ ) end end + + context 'with client-side webauthn error as blank string' do + let(:webauthn_error) { '' } + + it 'returns successful result excluding frontend_error' do + expect(result.to_h).to eq( + success: true, + multi_factor_auth_method: 'webauthn', + webauthn_configuration_id: webauthn_configuration.id, + ) + end + end end context 'when the input is invalid' do @@ -136,13 +148,12 @@ success: false, error_details: { webauthn_configuration: { blank: true } }, multi_factor_auth_method: 'webauthn', - webauthn_configuration_id: nil, ) end end context 'when a client-side webauthn error is present' do - let(:webauthn_error) { 'Unexpected error!' } + let(:webauthn_error) { 'NotAllowedError' } it 'returns unsuccessful result including client-side webauthn error text' do expect(result.to_h).to eq( @@ -150,6 +161,7 @@ error_details: { webauthn_error: { webauthn_error: true } }, multi_factor_auth_method: 'webauthn', webauthn_configuration_id: webauthn_configuration.id, + frontend_error: webauthn_error, ) end end From 3f46adc15a097a07ca8ef410455f02c3b7dd237a Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 17 Nov 2023 11:59:58 -0500 Subject: [PATCH 12/12] Finalize cleanup for MFA selection presenters (#9612) * Update missed references to SignIn base selection presenter * Remove unused SelectionPresenter * Swap base presenters to raising NotImplementedError * Swap phone method comparison to use symbol Consistency with logic elsewhere in class * Raise on missing type method in base presenter classes * Consolidate presenter classes to define only type method * Rename phone selection presenter method to delivery_method * Add changelog changelog: Internal, Code Quality, Remove unused code related to MFA selection presenters * Update call sites to use new delivery_method constructor argument * Use setup-specific string for WebAuthn setup presenter --- app/models/phone_configuration.rb | 4 +- .../selection_presenter.rb | 115 -------------- .../set_up_auth_app_selection_presenter.rb | 2 +- .../set_up_backup_code_selection_presenter.rb | 2 +- .../set_up_phone_selection_presenter.rb | 2 +- .../set_up_piv_cac_selection_presenter.rb | 2 +- .../set_up_selection_presenter.rb | 6 +- ...p_webauthn_platform_selection_presenter.rb | 2 +- .../set_up_webauthn_selection_presenter.rb | 4 +- .../sign_in_auth_app_selection_presenter.rb | 2 +- ...sign_in_backup_code_selection_presenter.rb | 2 +- ...ign_in_personal_key_selection_presenter.rb | 4 +- .../sign_in_phone_selection_presenter.rb | 18 +-- .../sign_in_piv_cac_selection_presenter.rb | 2 +- .../sign_in_selection_presenter.rb | 6 +- ...n_webauthn_platform_selection_presenter.rb | 4 +- .../sign_in_webauthn_selection_presenter.rb | 2 +- .../selection_presenter_spec.rb | 140 ------------------ ...et_up_auth_app_selection_presenter_spec.rb | 2 +- ...set_up_piv_cac_selection_presenter_spec.rb | 2 +- .../set_up_selection_presenter_spec.rb | 20 +-- ...authn_platform_selection_presenter_spec.rb | 2 +- ...et_up_webauthn_selection_presenter_spec.rb | 2 +- ...gn_in_auth_app_selection_presenter_spec.rb | 2 +- ...n_personal_key_selection_presenter_spec.rb | 2 +- .../sign_in_phone_selection_presenter_spec.rb | 123 +++++++++------ ...ign_in_piv_cac_selection_presenter.spec.rb | 2 +- .../sign_in_selection_presenter_spec.rb | 22 +-- ...authn_platform_selection_presenter_spec.rb | 2 +- ...gn_in_webauthn_selection_presenter_spec.rb | 2 +- 30 files changed, 137 insertions(+), 365 deletions(-) delete mode 100644 app/presenters/two_factor_authentication/selection_presenter.rb delete mode 100644 spec/presenters/two_factor_authentication/selection_presenter_spec.rb diff --git a/app/models/phone_configuration.rb b/app/models/phone_configuration.rb index be629ce82e0..43c2e5ccb22 100644 --- a/app/models/phone_configuration.rb +++ b/app/models/phone_configuration.rb @@ -23,12 +23,12 @@ def selection_presenters if capabilities.supports_sms? options << TwoFactorAuthentication::SignInPhoneSelectionPresenter. - new(user:, configuration: self, method: :sms) + new(user:, configuration: self, delivery_method: :sms) end if capabilities.supports_voice? options << TwoFactorAuthentication::SignInPhoneSelectionPresenter. - new(user:, configuration: self, method: :voice) + new(user:, configuration: self, delivery_method: :voice) end options diff --git a/app/presenters/two_factor_authentication/selection_presenter.rb b/app/presenters/two_factor_authentication/selection_presenter.rb deleted file mode 100644 index 3926937cd01..00000000000 --- a/app/presenters/two_factor_authentication/selection_presenter.rb +++ /dev/null @@ -1,115 +0,0 @@ -module TwoFactorAuthentication - class SelectionPresenter - include ActionView::Helpers::TranslationHelper - - attr_reader :configuration, :user - - def initialize(user:, configuration: nil) - @user = user - @configuration = configuration - end - - def render_in(view_context, &block) - view_context.capture(&block) - end - - def type - method.to_s - end - - def label - if @configuration.present? - login_label(method.to_s) - else - setup_label(method.to_s) - end - end - - def info - if @configuration.present? - login_info(method.to_s) - else - setup_info(method.to_s) - end - end - - def mfa_configuration_count - 0 - end - - def mfa_configuration_description - return '' if mfa_configuration_count == 0 - if single_configuration_only? - t('two_factor_authentication.two_factor_choice_options.no_count_configuration_added') - else - t( - 'two_factor_authentication.two_factor_choice_options.configurations_added', - count: mfa_configuration_count, - ) - end - end - - def mfa_added_label - if single_configuration_only? - '' - else - "(#{mfa_configuration_description})" - end - end - - def single_configuration_only? - false - end - - def disabled? - configuration.blank? && single_configuration_only? && mfa_configuration_count > 0 - end - - private - - def login_label(type) - raise "Unsupported login method: #{type}" - end - - def setup_label(type) - raise "Unsupported setup method: #{type}" - end - - def login_info(type) - case type - when 'auth_app' - t('two_factor_authentication.login_options.auth_app_info') - when 'backup_code' - t('two_factor_authentication.login_options.backup_code_info') - when 'piv_cac' - t('two_factor_authentication.login_options.piv_cac_info') - when 'webauthn' - t('two_factor_authentication.login_options.webauthn_info') - when 'webauthn_platform' - t('two_factor_authentication.login_options.webauthn_platform_info', app_name: APP_NAME) - else - raise "Unsupported login method: #{type}" - end - end - - def setup_info(type) - case type - when 'auth_app' - t('two_factor_authentication.two_factor_choice_options.auth_app_info') - when 'backup_code' - t('two_factor_authentication.two_factor_choice_options.backup_code_info') - when 'piv_cac' - t('two_factor_authentication.two_factor_choice_options.piv_cac_info') - when 'webauthn' - t('two_factor_authentication.two_factor_choice_options.webauthn_info') - when 'webauthn_platform' - t( - 'two_factor_authentication.two_factor_choice_options.webauthn_platform_info', - app_name: APP_NAME, - ) - else - raise "Unsupported setup method: #{type}" - end - end - end -end diff --git a/app/presenters/two_factor_authentication/set_up_auth_app_selection_presenter.rb b/app/presenters/two_factor_authentication/set_up_auth_app_selection_presenter.rb index 0763712895d..d2226caba96 100644 --- a/app/presenters/two_factor_authentication/set_up_auth_app_selection_presenter.rb +++ b/app/presenters/two_factor_authentication/set_up_auth_app_selection_presenter.rb @@ -1,6 +1,6 @@ module TwoFactorAuthentication class SetUpAuthAppSelectionPresenter < SetUpSelectionPresenter - def method + def type :auth_app end diff --git a/app/presenters/two_factor_authentication/set_up_backup_code_selection_presenter.rb b/app/presenters/two_factor_authentication/set_up_backup_code_selection_presenter.rb index 890ad3248a9..cf4046ffc3a 100644 --- a/app/presenters/two_factor_authentication/set_up_backup_code_selection_presenter.rb +++ b/app/presenters/two_factor_authentication/set_up_backup_code_selection_presenter.rb @@ -1,6 +1,6 @@ module TwoFactorAuthentication class SetUpBackupCodeSelectionPresenter < SetUpSelectionPresenter - def method + def type :backup_code end diff --git a/app/presenters/two_factor_authentication/set_up_phone_selection_presenter.rb b/app/presenters/two_factor_authentication/set_up_phone_selection_presenter.rb index b57bf159a8d..111270c8946 100644 --- a/app/presenters/two_factor_authentication/set_up_phone_selection_presenter.rb +++ b/app/presenters/two_factor_authentication/set_up_phone_selection_presenter.rb @@ -1,6 +1,6 @@ module TwoFactorAuthentication class SetUpPhoneSelectionPresenter < SetUpSelectionPresenter - def method + def type :phone end diff --git a/app/presenters/two_factor_authentication/set_up_piv_cac_selection_presenter.rb b/app/presenters/two_factor_authentication/set_up_piv_cac_selection_presenter.rb index 4022165b49f..76bf98f2724 100644 --- a/app/presenters/two_factor_authentication/set_up_piv_cac_selection_presenter.rb +++ b/app/presenters/two_factor_authentication/set_up_piv_cac_selection_presenter.rb @@ -1,6 +1,6 @@ module TwoFactorAuthentication class SetUpPivCacSelectionPresenter < SetUpSelectionPresenter - def method + def type :piv_cac end diff --git a/app/presenters/two_factor_authentication/set_up_selection_presenter.rb b/app/presenters/two_factor_authentication/set_up_selection_presenter.rb index 0c4e798eb89..d118a0af06c 100644 --- a/app/presenters/two_factor_authentication/set_up_selection_presenter.rb +++ b/app/presenters/two_factor_authentication/set_up_selection_presenter.rb @@ -13,15 +13,15 @@ def render_in(view_context, &block) end def type - method.to_s + raise NotImplementedError end def label - raise "Unsupported setup method: #{type}" + raise NotImplementedError end def info - raise "Unsupported setup method: #{type}" + raise NotImplementedError end def mfa_added_label diff --git a/app/presenters/two_factor_authentication/set_up_webauthn_platform_selection_presenter.rb b/app/presenters/two_factor_authentication/set_up_webauthn_platform_selection_presenter.rb index 278fb3b1ad9..0ed7f1158c5 100644 --- a/app/presenters/two_factor_authentication/set_up_webauthn_platform_selection_presenter.rb +++ b/app/presenters/two_factor_authentication/set_up_webauthn_platform_selection_presenter.rb @@ -5,7 +5,7 @@ def initialize(user:, configuration: nil) @configuration = configuration end - def method + def type :webauthn_platform end diff --git a/app/presenters/two_factor_authentication/set_up_webauthn_selection_presenter.rb b/app/presenters/two_factor_authentication/set_up_webauthn_selection_presenter.rb index 8f95676d18f..268f594e07d 100644 --- a/app/presenters/two_factor_authentication/set_up_webauthn_selection_presenter.rb +++ b/app/presenters/two_factor_authentication/set_up_webauthn_selection_presenter.rb @@ -1,6 +1,6 @@ module TwoFactorAuthentication class SetUpWebauthnSelectionPresenter < SetUpSelectionPresenter - def method + def type :webauthn end @@ -13,7 +13,7 @@ def label end def info - t('two_factor_authentication.login_options.webauthn_info') + t('two_factor_authentication.two_factor_choice_options.webauthn_info') end def mfa_configuration_count diff --git a/app/presenters/two_factor_authentication/sign_in_auth_app_selection_presenter.rb b/app/presenters/two_factor_authentication/sign_in_auth_app_selection_presenter.rb index 59e268502ba..a3fc889ecdd 100644 --- a/app/presenters/two_factor_authentication/sign_in_auth_app_selection_presenter.rb +++ b/app/presenters/two_factor_authentication/sign_in_auth_app_selection_presenter.rb @@ -1,6 +1,6 @@ module TwoFactorAuthentication class SignInAuthAppSelectionPresenter < SignInSelectionPresenter - def method + def type :auth_app end diff --git a/app/presenters/two_factor_authentication/sign_in_backup_code_selection_presenter.rb b/app/presenters/two_factor_authentication/sign_in_backup_code_selection_presenter.rb index 40b65dcff3b..b9e315c4719 100644 --- a/app/presenters/two_factor_authentication/sign_in_backup_code_selection_presenter.rb +++ b/app/presenters/two_factor_authentication/sign_in_backup_code_selection_presenter.rb @@ -1,6 +1,6 @@ module TwoFactorAuthentication class SignInBackupCodeSelectionPresenter < SignInSelectionPresenter - def method + def type :backup_code end diff --git a/app/presenters/two_factor_authentication/sign_in_personal_key_selection_presenter.rb b/app/presenters/two_factor_authentication/sign_in_personal_key_selection_presenter.rb index 4a863f85540..b0fd6aeb722 100644 --- a/app/presenters/two_factor_authentication/sign_in_personal_key_selection_presenter.rb +++ b/app/presenters/two_factor_authentication/sign_in_personal_key_selection_presenter.rb @@ -1,6 +1,6 @@ module TwoFactorAuthentication - class SignInPersonalKeySelectionPresenter < SelectionPresenter - def method + class SignInPersonalKeySelectionPresenter < SignInSelectionPresenter + def type :personal_key end diff --git a/app/presenters/two_factor_authentication/sign_in_phone_selection_presenter.rb b/app/presenters/two_factor_authentication/sign_in_phone_selection_presenter.rb index 207b9a82e71..f9c27d2409b 100644 --- a/app/presenters/two_factor_authentication/sign_in_phone_selection_presenter.rb +++ b/app/presenters/two_factor_authentication/sign_in_phone_selection_presenter.rb @@ -1,31 +1,31 @@ module TwoFactorAuthentication class SignInPhoneSelectionPresenter < SignInSelectionPresenter - attr_reader :configuration, :user, :method + attr_reader :configuration, :user, :delivery_method - def initialize(user:, configuration:, method:) + def initialize(user:, configuration:, delivery_method:) @user = user @configuration = configuration - @method = method + @delivery_method = delivery_method end def type if MfaContext.new(configuration&.user).phone_configurations.many? - "#{super}_#{configuration.id}" + "#{delivery_method}_#{configuration.id}".to_sym else - super + delivery_method || :phone end end def label - if method.to_s == 'sms' + if delivery_method == :sms t('two_factor_authentication.login_options.sms') - elsif method.to_s == 'voice' + elsif delivery_method == :voice t('two_factor_authentication.login_options.voice') end end def info - case method + case delivery_method when :sms t( 'two_factor_authentication.login_options.sms_info_html', @@ -42,7 +42,7 @@ def info end def disabled? - case method + case delivery_method when :sms OutageStatus.new.vendor_outage?(:sms) when :voice diff --git a/app/presenters/two_factor_authentication/sign_in_piv_cac_selection_presenter.rb b/app/presenters/two_factor_authentication/sign_in_piv_cac_selection_presenter.rb index a3f38c6044b..8b709c2c87b 100644 --- a/app/presenters/two_factor_authentication/sign_in_piv_cac_selection_presenter.rb +++ b/app/presenters/two_factor_authentication/sign_in_piv_cac_selection_presenter.rb @@ -1,6 +1,6 @@ module TwoFactorAuthentication class SignInPivCacSelectionPresenter < SignInSelectionPresenter - def method + def type :piv_cac end diff --git a/app/presenters/two_factor_authentication/sign_in_selection_presenter.rb b/app/presenters/two_factor_authentication/sign_in_selection_presenter.rb index fbffd1a0960..f954551b94a 100644 --- a/app/presenters/two_factor_authentication/sign_in_selection_presenter.rb +++ b/app/presenters/two_factor_authentication/sign_in_selection_presenter.rb @@ -14,15 +14,15 @@ def render_in(view_context, &block) end def type - method.to_s + raise NotImplementedError end def label - raise "Unsupported login method: #{type}" + raise NotImplementedError end def info - raise "Unsupported login method: #{type}" + raise NotImplementedError end def disabled? diff --git a/app/presenters/two_factor_authentication/sign_in_webauthn_platform_selection_presenter.rb b/app/presenters/two_factor_authentication/sign_in_webauthn_platform_selection_presenter.rb index b1632c78998..85c6b710991 100644 --- a/app/presenters/two_factor_authentication/sign_in_webauthn_platform_selection_presenter.rb +++ b/app/presenters/two_factor_authentication/sign_in_webauthn_platform_selection_presenter.rb @@ -1,6 +1,6 @@ module TwoFactorAuthentication - class SignInWebauthnPlatformSelectionPresenter < SelectionPresenter - def method + class SignInWebauthnPlatformSelectionPresenter < SignInSelectionPresenter + def type :webauthn_platform end diff --git a/app/presenters/two_factor_authentication/sign_in_webauthn_selection_presenter.rb b/app/presenters/two_factor_authentication/sign_in_webauthn_selection_presenter.rb index 542ea6a1289..b38ed74d4d1 100644 --- a/app/presenters/two_factor_authentication/sign_in_webauthn_selection_presenter.rb +++ b/app/presenters/two_factor_authentication/sign_in_webauthn_selection_presenter.rb @@ -1,6 +1,6 @@ module TwoFactorAuthentication class SignInWebauthnSelectionPresenter < SignInSelectionPresenter - def method + def type :webauthn end diff --git a/spec/presenters/two_factor_authentication/selection_presenter_spec.rb b/spec/presenters/two_factor_authentication/selection_presenter_spec.rb deleted file mode 100644 index 009b7180107..00000000000 --- a/spec/presenters/two_factor_authentication/selection_presenter_spec.rb +++ /dev/null @@ -1,140 +0,0 @@ -require 'rails_helper' - -RSpec.describe TwoFactorAuthentication::SelectionPresenter do - let(:placeholder_presenter_class) do - Class.new(TwoFactorAuthentication::SelectionPresenter) do - def method - :missing - end - end - end - - let(:user) { build(:user) } - - subject(:presenter) { described_class.new(user: user) } - - describe '#render_in' do - it 'renders captured block content' do - view_context = ActionController::Base.new.view_context - - expect(view_context).to receive(:capture) do |*args, &block| - expect(block.call).to eq('content') - end - - presenter.render_in(view_context) { 'content' } - end - end - - describe '#disabled?' do - let(:single_configuration_only) {} - let(:mfa_configuration_count) {} - - before do - allow(presenter).to receive(:single_configuration_only?).and_return(single_configuration_only) - allow(presenter).to receive(:mfa_configuration_count).and_return(mfa_configuration_count) - end - - context 'without single configuration restriction' do - let(:single_configuration_only) { false } - - it 'is an mfa that allows multiple configurations' do - expect(presenter.disabled?).to eq(false) - end - end - - context 'with single configuration only' do - let(:single_configuration_only) { true } - - context 'with default mfa count implementation' do - before do - allow(presenter).to receive(:mfa_configuration_count).and_call_original - end - - it 'is mfa with unimplemented mfa count and single config' do - expect(presenter.disabled?).to eq(false) - end - end - - context 'with no configured mfas' do - let(:mfa_configuration_count) { 0 } - - it 'is configured with no mfa' do - expect(presenter.disabled?).to eq(false) - end - end - - context 'with at least one configured mfa' do - let(:mfa_configuration_count) { 1 } - - it 'is mfa with at least one configured' do - expect(presenter.disabled?).to eq(true) - end - end - end - context 'with configuration' do - let(:single_configuration_only) { true } - let(:mfa_configuration_count) { 1 } - let(:configuration) { create(:phone_configuration, user: user) } - before do - allow(presenter).to receive(:configuration).and_return(configuration) - end - it { expect(presenter.disabled?).to eq(false) } - end - end - - describe '#single_configuration_only?' do - it { expect(presenter.single_configuration_only?).to eq(false) } - end - - describe '#mfa_added_label' do - subject(:mfa_added_label) { presenter.mfa_added_label } - before do - allow(presenter).to receive(:mfa_configuration_count).and_return('1') - end - it 'is a count of configured MFAs' do - expect(presenter.mfa_added_label).to include('added') - end - - context 'with single configuration only' do - before do - allow(presenter).to receive(:single_configuration_only?).and_return(true) - end - - it 'is an empty string' do - expect(presenter.mfa_added_label).to eq('') - end - end - end - - describe '#label' do - context 'with no configuration' do - it 'raises with missing translation' do - expect { placeholder_presenter_class.new(user: user).label }.to raise_error(RuntimeError) - end - end - - context 'with configuration' do - it 'raises with missing translation' do - expect do - placeholder_presenter_class.new(configuration: 1, user: user).label - end.to raise_error(RuntimeError) - end - end - end - - describe '#info' do - context 'with no configuration' do - it 'raises with missing translation' do - expect { placeholder_presenter_class.new(user: user).info }.to raise_error(RuntimeError) - end - end - - context 'with configuration' do - it 'raises with missing translation' do - expect do - placeholder_presenter_class.new(configuration: 1, user: user).info - end.to raise_error(RuntimeError) - end - end - end -end diff --git a/spec/presenters/two_factor_authentication/set_up_auth_app_selection_presenter_spec.rb b/spec/presenters/two_factor_authentication/set_up_auth_app_selection_presenter_spec.rb index ada58bceb8d..6118cacb57c 100644 --- a/spec/presenters/two_factor_authentication/set_up_auth_app_selection_presenter_spec.rb +++ b/spec/presenters/two_factor_authentication/set_up_auth_app_selection_presenter_spec.rb @@ -13,7 +13,7 @@ describe '#type' do it 'returns auth_app' do - expect(presenter_without_mfa.type).to eq 'auth_app' + expect(presenter_without_mfa.type).to eq :auth_app end end diff --git a/spec/presenters/two_factor_authentication/set_up_piv_cac_selection_presenter_spec.rb b/spec/presenters/two_factor_authentication/set_up_piv_cac_selection_presenter_spec.rb index cb5744107ba..79cc227b621 100644 --- a/spec/presenters/two_factor_authentication/set_up_piv_cac_selection_presenter_spec.rb +++ b/spec/presenters/two_factor_authentication/set_up_piv_cac_selection_presenter_spec.rb @@ -12,7 +12,7 @@ describe '#type' do it 'returns piv_cac' do - expect(presenter_without_mfa.type).to eq 'piv_cac' + expect(presenter_without_mfa.type).to eq :piv_cac end end diff --git a/spec/presenters/two_factor_authentication/set_up_selection_presenter_spec.rb b/spec/presenters/two_factor_authentication/set_up_selection_presenter_spec.rb index eec0b2649ba..4b74dbb20fa 100644 --- a/spec/presenters/two_factor_authentication/set_up_selection_presenter_spec.rb +++ b/spec/presenters/two_factor_authentication/set_up_selection_presenter_spec.rb @@ -2,11 +2,7 @@ RSpec.describe TwoFactorAuthentication::SetUpSelectionPresenter do let(:placeholder_presenter_class) do - Class.new(TwoFactorAuthentication::SetUpSelectionPresenter) do - def method - :missing - end - end + Class.new(TwoFactorAuthentication::SetUpSelectionPresenter) end let(:user) { build(:user) } @@ -93,15 +89,21 @@ def method end end + describe '#type' do + it 'raises with missing implementation' do + expect { placeholder_presenter_class.new(user:).type }.to raise_error(NotImplementedError) + end + end + describe '#label' do - it 'raises with missing translation' do - expect { placeholder_presenter_class.new(user: user).label }.to raise_error(RuntimeError) + it 'raises with missing implementation' do + expect { placeholder_presenter_class.new(user:).label }.to raise_error(NotImplementedError) end end describe '#info' do - it 'raises with missing translation' do - expect { placeholder_presenter_class.new(user: user).info }.to raise_error(RuntimeError) + it 'raises with missing implementation' do + expect { placeholder_presenter_class.new(user:).info }.to raise_error(NotImplementedError) end end end diff --git a/spec/presenters/two_factor_authentication/set_up_webauthn_platform_selection_presenter_spec.rb b/spec/presenters/two_factor_authentication/set_up_webauthn_platform_selection_presenter_spec.rb index 65fd19dcf3f..879e8509f11 100644 --- a/spec/presenters/two_factor_authentication/set_up_webauthn_platform_selection_presenter_spec.rb +++ b/spec/presenters/two_factor_authentication/set_up_webauthn_platform_selection_presenter_spec.rb @@ -19,7 +19,7 @@ describe '#type' do it 'returns webauthn_platform' do - expect(presenter_without_mfa.type).to eq 'webauthn_platform' + expect(presenter_without_mfa.type).to eq :webauthn_platform end end diff --git a/spec/presenters/two_factor_authentication/set_up_webauthn_selection_presenter_spec.rb b/spec/presenters/two_factor_authentication/set_up_webauthn_selection_presenter_spec.rb index 620868ca79d..1e74d55f580 100644 --- a/spec/presenters/two_factor_authentication/set_up_webauthn_selection_presenter_spec.rb +++ b/spec/presenters/two_factor_authentication/set_up_webauthn_selection_presenter_spec.rb @@ -13,7 +13,7 @@ describe '#type' do it 'returns webauthn' do - expect(presenter_without_mfa.type).to eq 'webauthn' + expect(presenter_without_mfa.type).to eq :webauthn end end diff --git a/spec/presenters/two_factor_authentication/sign_in_auth_app_selection_presenter_spec.rb b/spec/presenters/two_factor_authentication/sign_in_auth_app_selection_presenter_spec.rb index 5fe3027a668..857de98bd20 100644 --- a/spec/presenters/two_factor_authentication/sign_in_auth_app_selection_presenter_spec.rb +++ b/spec/presenters/two_factor_authentication/sign_in_auth_app_selection_presenter_spec.rb @@ -10,7 +10,7 @@ describe '#type' do it 'returns auth_app' do - expect(presenter.type).to eq 'auth_app' + expect(presenter.type).to eq :auth_app end end diff --git a/spec/presenters/two_factor_authentication/sign_in_personal_key_selection_presenter_spec.rb b/spec/presenters/two_factor_authentication/sign_in_personal_key_selection_presenter_spec.rb index b2993d839e5..98cbdf07633 100644 --- a/spec/presenters/two_factor_authentication/sign_in_personal_key_selection_presenter_spec.rb +++ b/spec/presenters/two_factor_authentication/sign_in_personal_key_selection_presenter_spec.rb @@ -10,7 +10,7 @@ subject(:type) { presenter.type } it 'returns personal key type' do - expect(type).to eq 'personal_key' + expect(type).to eq :personal_key end end diff --git a/spec/presenters/two_factor_authentication/sign_in_phone_selection_presenter_spec.rb b/spec/presenters/two_factor_authentication/sign_in_phone_selection_presenter_spec.rb index 360ab52daf5..c70580f29fb 100644 --- a/spec/presenters/two_factor_authentication/sign_in_phone_selection_presenter_spec.rb +++ b/spec/presenters/two_factor_authentication/sign_in_phone_selection_presenter_spec.rb @@ -1,29 +1,56 @@ require 'rails_helper' RSpec.describe TwoFactorAuthentication::SignInPhoneSelectionPresenter do - let(:user) { create(:user, :with_phone) } + let(:user) { create(:user) } let(:configuration) { create(:phone_configuration, user: user) } + let(:delivery_method) { nil } let(:presenter) do - described_class.new(user: user, configuration: configuration, method: 'phone') + described_class.new(user:, configuration:, delivery_method:) end describe '#type' do - it 'returns phone appended with configuration id' do - expect(presenter.type).to eq "phone_#{configuration.id}" + context 'without a defined delivery method' do + let(:delivery_method) { nil } + + it 'returns generic phone type' do + expect(presenter.type).to eq :phone + end + end + + context 'with delivery method' do + let(:delivery_method) { :sms } + + context 'with user having a single configuration' do + it 'returns delivery method' do + expect(presenter.type).to eq :sms + end + end + + context 'with user having multiple configurations' do + let(:user) { create(:user, :with_phone) } + + it 'returns delivery method appended with configuration id' do + expect(presenter.type).to eq "sms_#{configuration.id}".to_sym + end + end end end describe '#info' do - it 'raises with missing translation' do - expect(presenter.info).to eq( - t('two_factor_authentication.two_factor_choice_options.phone_info'), - ) - end - context 'with sms method' do - let(:presenter) do - described_class.new(user: user, configuration: configuration, method: :sms) + context 'without a defined delivery method' do + let(:delivery_method) { nil } + + it 'returns the correct translation for setup' do + expect(presenter.info).to eq( + t('two_factor_authentication.two_factor_choice_options.phone_info'), + ) end + end + + context 'with sms delivery method' do + let(:delivery_method) { :sms } + it 'returns the correct translation for sms' do expect(presenter.info).to eq( t( @@ -33,10 +60,10 @@ ) end end - context 'with voice method' do - let(:presenter) do - described_class.new(user: user, configuration: configuration, method: :voice) - end + + context 'with voice delivery method' do + let(:delivery_method) { :voice } + it 'returns the correct translation for voice' do expect(presenter.info).to eq( t( @@ -49,44 +76,50 @@ end describe '#disabled?' do - let(:user_without_mfa) { create(:user) } let(:phone) { build(:phone_configuration, phone: '+1 888 867-5309') } - let(:presenter_without_mfa) do - described_class.new(configuration: phone, user: user_without_mfa, method: 'phone') - end - it { expect(presenter_without_mfa.disabled?).to eq(false) } - context 'all phone vendor outage' do - before do - allow_any_instance_of(OutageStatus).to receive(:all_phone_vendor_outage?).and_return(true) - end + context 'without a defined delivery method' do + let(:delivery_method) { nil } - it { expect(presenter_without_mfa.disabled?).to eq(true) } - end + it { expect(presenter.disabled?).to eq(false) } - context 'voice vendor outage' do - let(:presenter_without_mfa) do - described_class.new(configuration: phone, user: user, method: method) - end - let(:method) { :voice } - before do - allow_any_instance_of(OutageStatus).to receive(:vendor_outage?).with(:voice). - and_return(true) - end + context 'all phone vendor outage' do + before do + allow_any_instance_of(OutageStatus).to receive(:all_phone_vendor_outage?).and_return(true) + end - it { expect(presenter_without_mfa.disabled?).to eq(true) } + it { expect(presenter.disabled?).to eq(true) } + end end - context 'sms vendor outage' do - let(:presenter_without_mfa) do - described_class.new(configuration: phone, user: user, method: method) - end - let(:method) { :sms } - before do - allow_any_instance_of(OutageStatus).to receive(:vendor_outage?).with(:sms).and_return(true) + context 'with sms delivery method' do + let(:delivery_method) { :sms } + + it { expect(presenter.disabled?).to eq(false) } + + context 'sms vendor outage' do + before do + allow_any_instance_of(OutageStatus).to receive(:vendor_outage?).with(:sms). + and_return(true) + end + + it { expect(presenter.disabled?).to eq(true) } end + end - it { expect(presenter_without_mfa.disabled?).to eq(true) } + context 'with voice delivery method' do + let(:delivery_method) { :voice } + + it { expect(presenter.disabled?).to eq(false) } + + context 'voice vendor outage' do + before do + allow_any_instance_of(OutageStatus).to receive(:vendor_outage?).with(:voice). + and_return(true) + end + + it { expect(presenter.disabled?).to eq(true) } + end end end end diff --git a/spec/presenters/two_factor_authentication/sign_in_piv_cac_selection_presenter.spec.rb b/spec/presenters/two_factor_authentication/sign_in_piv_cac_selection_presenter.spec.rb index 214bf954038..e2d6d5fa8bb 100644 --- a/spec/presenters/two_factor_authentication/sign_in_piv_cac_selection_presenter.spec.rb +++ b/spec/presenters/two_factor_authentication/sign_in_piv_cac_selection_presenter.spec.rb @@ -10,7 +10,7 @@ describe '#type' do it 'returns piv_cac' do - expect(presenter.type).to eq 'piv_cac' + expect(presenter.type).to eq :piv_cac end end diff --git a/spec/presenters/two_factor_authentication/sign_in_selection_presenter_spec.rb b/spec/presenters/two_factor_authentication/sign_in_selection_presenter_spec.rb index e31720e78fa..d0ce9e0810c 100644 --- a/spec/presenters/two_factor_authentication/sign_in_selection_presenter_spec.rb +++ b/spec/presenters/two_factor_authentication/sign_in_selection_presenter_spec.rb @@ -2,11 +2,7 @@ RSpec.describe TwoFactorAuthentication::SignInSelectionPresenter do let(:placeholder_presenter_class) do - Class.new(TwoFactorAuthentication::SignInSelectionPresenter) do - def method - :missing - end - end + Class.new(TwoFactorAuthentication::SignInSelectionPresenter) end let(:user) { build(:user) } @@ -27,24 +23,20 @@ def method end describe '#label' do - it 'raises with missing translation' do - expect do - presenter.label - end.to raise_error(RuntimeError) + it 'raises with missing implementation' do + expect { presenter.label }.to raise_error(NotImplementedError) end end describe '#type' do - it 'returns missing as type' do - expect(presenter.type).to eq('missing') + it 'raises with missing implementation' do + expect { presenter.type }.to raise_error(NotImplementedError) end end describe '#info' do - it 'raises with missing translation' do - expect do - presenter.info - end.to raise_error(RuntimeError) + it 'raises with missing implementation' do + expect { presenter.info }.to raise_error(NotImplementedError) end end end diff --git a/spec/presenters/two_factor_authentication/sign_in_webauthn_platform_selection_presenter_spec.rb b/spec/presenters/two_factor_authentication/sign_in_webauthn_platform_selection_presenter_spec.rb index 14b7400f57e..2744d974f6b 100644 --- a/spec/presenters/two_factor_authentication/sign_in_webauthn_platform_selection_presenter_spec.rb +++ b/spec/presenters/two_factor_authentication/sign_in_webauthn_platform_selection_presenter_spec.rb @@ -10,7 +10,7 @@ describe '#type' do it 'returns webauthn_platform' do - expect(presenter.type).to eq 'webauthn_platform' + expect(presenter.type).to eq :webauthn_platform end end diff --git a/spec/presenters/two_factor_authentication/sign_in_webauthn_selection_presenter_spec.rb b/spec/presenters/two_factor_authentication/sign_in_webauthn_selection_presenter_spec.rb index 4676124561f..7bc097bb33d 100644 --- a/spec/presenters/two_factor_authentication/sign_in_webauthn_selection_presenter_spec.rb +++ b/spec/presenters/two_factor_authentication/sign_in_webauthn_selection_presenter_spec.rb @@ -10,7 +10,7 @@ describe '#type' do it 'returns webauthn' do - expect(presenter.type).to eq 'webauthn' + expect(presenter.type).to eq :webauthn end end