diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index acbb0d18a13..200bb874405 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -6,6 +6,7 @@ class ApplicationController < ActionController::Base include LocaleHelper include VerifySpAttributesConcern include EffectiveUser + include SecondMfaReminderConcern # Prevent CSRF attacks by raising an exception. # For APIs, you may want to use :null_session instead. @@ -213,6 +214,7 @@ def after_sign_in_path_for(_user) return fix_broken_personal_key_url if current_user.broken_personal_key? return user_session.delete(:stored_location) if user_session.key?(:stored_location) return reactivate_account_url if user_needs_to_reactivate_account? + return second_mfa_reminder_url if user_needs_second_mfa_reminder? return sp_session_request_url_with_updated_params if sp_session.key?(:request_url) signed_in_url end diff --git a/app/controllers/concerns/mfa_setup_concern.rb b/app/controllers/concerns/mfa_setup_concern.rb index 1f0a290e660..3dffc7cea0a 100644 --- a/app/controllers/concerns/mfa_setup_concern.rb +++ b/app/controllers/concerns/mfa_setup_concern.rb @@ -12,6 +12,7 @@ def next_setup_path mfa_method_counts: mfa_context.enabled_two_factor_configuration_counts_hash, enabled_mfa_methods_count: mfa_context.enabled_mfa_methods_count, pii_like_keypaths: [[:mfa_method_counts, :phone]], + second_mfa_reminder_conversion: user_session.delete(:second_mfa_reminder_conversion), success: true, ) end diff --git a/app/controllers/concerns/second_mfa_reminder_concern.rb b/app/controllers/concerns/second_mfa_reminder_concern.rb new file mode 100644 index 00000000000..fd2f33282a4 --- /dev/null +++ b/app/controllers/concerns/second_mfa_reminder_concern.rb @@ -0,0 +1,30 @@ +module SecondMfaReminderConcern + def user_needs_second_mfa_reminder? + return false unless IdentityConfig.store.second_mfa_reminder_enabled + return false if user_has_dismissed_second_mfa_reminder? || user_has_multiple_mfa_methods? + exceeded_sign_in_count_for_second_mfa_reminder? || exceeded_account_age_for_second_mfa_reminder? + end + + private + + def user_has_dismissed_second_mfa_reminder? + current_user.second_mfa_reminder_dismissed_at.present? + end + + def user_has_multiple_mfa_methods? + MfaContext.new(current_user).enabled_mfa_methods_count > 1 + end + + def exceeded_sign_in_count_for_second_mfa_reminder? + current_user.sign_in_count(since: second_mfa_reminder_account_age_cutoff) >= + IdentityConfig.store.second_mfa_reminder_sign_in_count + end + + def exceeded_account_age_for_second_mfa_reminder? + current_user.created_at.before?(second_mfa_reminder_account_age_cutoff) + end + + def second_mfa_reminder_account_age_cutoff + IdentityConfig.store.second_mfa_reminder_account_age_in_days.days.ago + end +end diff --git a/app/controllers/users/second_mfa_reminder_controller.rb b/app/controllers/users/second_mfa_reminder_controller.rb new file mode 100644 index 00000000000..80f2f49b713 --- /dev/null +++ b/app/controllers/users/second_mfa_reminder_controller.rb @@ -0,0 +1,33 @@ +module Users + class SecondMfaReminderController < ApplicationController + include SecureHeadersConcern + + before_action :confirm_two_factor_authenticated + before_action :apply_secure_headers_override + + def new + analytics.second_mfa_reminder_visit + end + + def create + analytics.second_mfa_reminder_dismissed(opted_to_add: opted_to_add?) + current_user.update(second_mfa_reminder_dismissed_at: Time.zone.now) + user_session[:second_mfa_reminder_conversion] = true if opted_to_add? + redirect_to dismiss_redirect_path + end + + private + + def opted_to_add? + params[:add_method].present? + end + + def dismiss_redirect_path + if opted_to_add? + authentication_methods_setup_path + else + after_sign_in_path_for(current_user) + end + end + end +end diff --git a/app/controllers/users/two_factor_authentication_setup_controller.rb b/app/controllers/users/two_factor_authentication_setup_controller.rb index 479d06be738..fd5e88918df 100644 --- a/app/controllers/users/two_factor_authentication_setup_controller.rb +++ b/app/controllers/users/two_factor_authentication_setup_controller.rb @@ -16,8 +16,7 @@ def index def create result = submit_form - analytics_hash = result.to_h - analytics.user_registration_2fa_setup(**analytics_hash) + analytics.user_registration_2fa_setup(**result.to_h) irs_attempts_api_tracker.mfa_enroll_options_selected( success: result.success?, mfa_device_types: @two_factor_options_form.selection, diff --git a/app/models/user.rb b/app/models/user.rb index 653b3edd461..193a5adb8a1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -390,6 +390,17 @@ def has_devices? !recent_devices.empty? end + # Returns the number of times the user has signed in, corresponding to the `sign_in_before_2fa` + # event. + # + # A `since` time argument is required, to optimize performance based on database indices for + # querying a user's events. + # + # @param [ActiveSupport::TimeWithZone] since Time window to query user's events + def sign_in_count(since:) + events.where(event_type: :sign_in_before_2fa).where(created_at: since..).count + end + def second_last_signed_in_at events.where(event_type: 'sign_in_after_2fa'). order(created_at: :desc).limit(2).pluck(:created_at).second diff --git a/app/presenters/two_factor_options_presenter.rb b/app/presenters/two_factor_options_presenter.rb index 37ce24c5408..4e76fd9d586 100644 --- a/app/presenters/two_factor_options_presenter.rb +++ b/app/presenters/two_factor_options_presenter.rb @@ -64,6 +64,14 @@ def skip_path after_mfa_setup_path if two_factor_enabled? && show_skip_additional_mfa_link? end + def skip_label + if user_has_dismissed_second_mfa_reminder? + t('links.cancel') + else + t('mfa.skip') + end + end + private def piv_cac_option @@ -114,4 +122,8 @@ def phishing_resistant_only? def mfa_policy @mfa_policy ||= MfaPolicy.new(user) end + + def user_has_dismissed_second_mfa_reminder? + user.second_mfa_reminder_dismissed_at.present? + end end diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index ff4370f3215..d2d817cf6d0 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -3689,6 +3689,17 @@ def saml_auth_request( ) end + # User dismissed the second MFA reminder page + # @param [Boolean] opted_to_add Whether the user chose to add a method + def second_mfa_reminder_dismissed(opted_to_add:, **extra) + track_event('Second MFA Reminder Dismissed', opted_to_add:, **extra) + end + + # User visited the second MFA reminder page + def second_mfa_reminder_visit + track_event('Second MFA Reminder Visited') + end + # Tracks when security event is received # @param [Boolean] success # @param [String] error_code @@ -4138,6 +4149,7 @@ def user_registration_enter_email_visit # @param [Boolean] success # @param [Hash] mfa_method_counts # @param [Integer] enabled_mfa_methods_count + # @param [Boolean] second_mfa_reminder_conversion Whether it is a result of second MFA reminder. # @param [Hash] pii_like_keypaths # Tracks when a user has completed MFA setup def user_registration_mfa_setup_complete( @@ -4145,6 +4157,7 @@ def user_registration_mfa_setup_complete( mfa_method_counts:, enabled_mfa_methods_count:, pii_like_keypaths:, + second_mfa_reminder_conversion: nil, **extra ) track_event( @@ -4154,6 +4167,7 @@ def user_registration_mfa_setup_complete( mfa_method_counts: mfa_method_counts, enabled_mfa_methods_count: enabled_mfa_methods_count, pii_like_keypaths: pii_like_keypaths, + second_mfa_reminder_conversion:, **extra, }.compact, ) diff --git a/app/views/users/second_mfa_reminder/new.html.erb b/app/views/users/second_mfa_reminder/new.html.erb new file mode 100644 index 00000000000..ddd11767062 --- /dev/null +++ b/app/views/users/second_mfa_reminder/new.html.erb @@ -0,0 +1,33 @@ +<% title t('users.second_mfa_reminder.heading') %> + +<%= render StatusPageComponent.new(status: :info, icon: :question) do |c| %> + <% c.with_header { t('users.second_mfa_reminder.heading') } %> + +

<%= t('users.second_mfa_reminder.description') %>

+ +
+
+ <%= render ButtonComponent.new( + action: ->(**tag_options, &block) do + button_to( + second_mfa_reminder_path, + **tag_options, + params: { add_method: true }, + &block + ) + end, + big: true, + full_width: true, + class: 'margin-bottom-2', + ).with_content(t('users.second_mfa_reminder.add_method')) %> + <%= render ButtonComponent.new( + action: ->(**tag_options, &block) do + button_to(second_mfa_reminder_path, **tag_options, &block) + end, + outline: true, + big: true, + full_width: true, + ).with_content(t('users.second_mfa_reminder.continue', sp_name: decorated_session.sp_name || APP_NAME)) %> +
+
+<% end %> diff --git a/app/views/users/two_factor_authentication_setup/index.html.erb b/app/views/users/two_factor_authentication_setup/index.html.erb index 1506a1d37b3..3656ac0ab8d 100644 --- a/app/views/users/two_factor_authentication_setup/index.html.erb +++ b/app/views/users/two_factor_authentication_setup/index.html.erb @@ -52,7 +52,7 @@ <% if @presenter.skip_path || !@presenter.two_factor_enabled? %> <%= render PageFooterComponent.new do %> <% if @presenter.skip_path %> - <%= link_to t('mfa.skip'), @presenter.skip_path %> + <%= link_to @presenter.skip_label, @presenter.skip_path %> <% elsif !@presenter.two_factor_enabled? %> <%= link_to t('links.cancel_account_creation'), sign_up_cancel_path %> <% end %> diff --git a/config/application.yml.default b/config/application.yml.default index 4f23277751d..e48f2cf8c7c 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -285,6 +285,9 @@ rules_of_use_updated_at: '2022-01-19T00:00:00Z' # Production has a newer timesta s3_public_reports_enabled: false s3_reports_enabled: false saml_secret_rotation_enabled: false +second_mfa_reminder_account_age_in_days: 30 +second_mfa_reminder_enabled: true +second_mfa_reminder_sign_in_count: 10 seed_agreements_data: true service_provider_request_ttl_hours: 24 session_check_delay: 30 @@ -463,6 +466,7 @@ production: s3_reports_enabled: true saml_endpoint_configs: '[]' scrypt_cost: 10000$8$1$ + second_mfa_reminder_enabled: false secret_key_base: seed_agreements_data: false session_encryption_key: diff --git a/config/locales/users/en.yml b/config/locales/users/en.yml index 5e54cdf9c2d..ef0a53f022c 100644 --- a/config/locales/users/en.yml +++ b/config/locales/users/en.yml @@ -36,6 +36,12 @@ en: overview_html: We’ve updated our %{link_html}. Please review and check the box below to continue. + second_mfa_reminder: + add_method: Add an authentication method + continue: Continue to %{sp_name} + description: Your account only has a single authentication method. Avoid being + locked out of your account by adding another authentication method. + heading: Improve your account security suspended_sign_in_account: contact_details: We couldn’t sign you in. Please call our contact center at %{contact_number}. diff --git a/config/locales/users/es.yml b/config/locales/users/es.yml index ff569db7d83..5aabdd789e1 100644 --- a/config/locales/users/es.yml +++ b/config/locales/users/es.yml @@ -37,6 +37,13 @@ es: overview_html: Actualizamos nuestro %{link_html}. Revise y marque la casilla a continuación para continuar. + second_mfa_reminder: + add_method: Agregar un método de autenticación + continue: Continúa con %{sp_name} + description: Su cuenta sólo tiene un método de autenticación. Para evitar que + bloqueen su cuenta, puede que necesites usar otro método de + autenticación. + heading: Aumente la seguridad de su cuenta suspended_sign_in_account: contact_details: No pudimos iniciar tu sesión. Por favor, llama a nuestro centro de contacto al %{contact_number}. diff --git a/config/locales/users/fr.yml b/config/locales/users/fr.yml index a4dcbd3cc0d..0cfc1594246 100644 --- a/config/locales/users/fr.yml +++ b/config/locales/users/fr.yml @@ -39,6 +39,13 @@ fr: overview_html: Nous avons mis à jour notre %{link_html}. Veuillez consulter et cocher la case ci-dessous pour continuer. + second_mfa_reminder: + add_method: Ajouter une méthode d’authentification + continue: Continuer vers %{sp_name} + description: Votre compte ne dispose que d’une seule méthode d’authentification. + Évitez le verrouillage de votre compte en ajoutant une autre méthode + d’authentification. + heading: Améliorer la sécurité de votre compte suspended_sign_in_account: contact_details: Nous n’avons pas pu vous connecter. Merci d’appeler notre centre de contact au %{contact_number}. diff --git a/config/routes.rb b/config/routes.rb index 6b4fdebabde..defc3c7a8b0 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -207,6 +207,9 @@ get '/rules_of_use' => 'users/rules_of_use#new' post '/rules_of_use' => 'users/rules_of_use#create' + get '/second_mfa_reminder' => 'users/second_mfa_reminder#new' + post '/second_mfa_reminder' => 'users/second_mfa_reminder#create' + get '/piv_cac' => 'users/piv_cac_authentication_setup#new', as: :setup_piv_cac get '/piv_cac_error' => 'users/piv_cac_authentication_setup#error', as: :setup_piv_cac_error delete '/piv_cac' => 'users/piv_cac_authentication_setup#delete', as: :disable_piv_cac diff --git a/db/primary_migrate/20230831124437_add_sign_in_count_and_second_mfa_reminder_dismissed_at_to_users.rb b/db/primary_migrate/20230831124437_add_sign_in_count_and_second_mfa_reminder_dismissed_at_to_users.rb new file mode 100644 index 00000000000..8a4ac0c943f --- /dev/null +++ b/db/primary_migrate/20230831124437_add_sign_in_count_and_second_mfa_reminder_dismissed_at_to_users.rb @@ -0,0 +1,5 @@ +class AddSignInCountAndSecondMfaReminderDismissedAtToUsers < ActiveRecord::Migration[7.0] + def change + add_column :users, :second_mfa_reminder_dismissed_at, :datetime + end +end diff --git a/db/schema.rb b/db/schema.rb index 7df4d6dbe7f..2e1b1e1b8ac 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2023_08_14_130423) do +ActiveRecord::Schema[7.0].define(version: 2023_08_31_124437) do # These are extensions that must be enabled in order to support this database enable_extension "pg_stat_statements" enable_extension "pgcrypto" @@ -608,6 +608,7 @@ t.datetime "reinstated_at" t.string "encrypted_password_digest_multi_region" t.string "encrypted_recovery_code_digest_multi_region" + t.datetime "second_mfa_reminder_dismissed_at" t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true t.index ["uuid"], name: "index_users_on_uuid", unique: true end diff --git a/lib/identity_config.rb b/lib/identity_config.rb index ceed0812342..c3bb51fa3c3 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -411,6 +411,9 @@ def self.build_store(config_map) config.add(:saml_endpoint_configs, type: :json, options: { symbolize_names: true }) config.add(:saml_secret_rotation_enabled, type: :boolean) config.add(:scrypt_cost, type: :string) + config.add(:second_mfa_reminder_account_age_in_days, type: :integer) + config.add(:second_mfa_reminder_enabled, type: :boolean) + config.add(:second_mfa_reminder_sign_in_count, type: :integer) config.add(:secret_key_base, type: :string) config.add(:seed_agreements_data, type: :boolean) config.add(:service_provider_request_ttl_hours, type: :integer) diff --git a/spec/controllers/concerns/mfa_setup_concern_spec.rb b/spec/controllers/concerns/mfa_setup_concern_spec.rb index ecd2000e05b..4cddc5c6786 100644 --- a/spec/controllers/concerns/mfa_setup_concern_spec.rb +++ b/spec/controllers/concerns/mfa_setup_concern_spec.rb @@ -5,14 +5,41 @@ include MfaSetupConcern end - describe '#show_skip_additional_mfa_link?' do - let(:user) { create(:user, :fully_registered) } + let(:user) { create(:user, :fully_registered) } - subject(:show_skip_additional_mfa_link?) { controller.show_skip_additional_mfa_link? } + before do + allow(controller).to receive(:current_user).and_return(user) + end + + describe '#next_setup_path' do + subject(:next_setup_path) { controller.next_setup_path } + + context 'when user converts from second mfa reminder' do + let(:user) { create(:user, :fully_registered, :with_phone, :with_backup_code) } + + before do + stub_sign_in_before_2fa(user) + stub_analytics + controller.user_session[:second_mfa_reminder_conversion] = true + controller.user_session[:mfa_selections] = [] + end + + it 'tracks analytics event' do + next_setup_path - before do - allow(controller).to receive(:current_user).and_return(user) + expect(@analytics).to have_logged_event( + 'User Registration: MFA Setup Complete', + success: true, + mfa_method_counts: { phone: 1, backup_codes: 10 }, + enabled_mfa_methods_count: 2, + second_mfa_reminder_conversion: true, + ) + end end + end + + describe '#show_skip_additional_mfa_link?' do + subject(:show_skip_additional_mfa_link?) { controller.show_skip_additional_mfa_link? } it 'returns true' do expect(show_skip_additional_mfa_link?).to eq(true) diff --git a/spec/controllers/concerns/second_mfa_reminder_concern_spec.rb b/spec/controllers/concerns/second_mfa_reminder_concern_spec.rb new file mode 100644 index 00000000000..e79b4691a2a --- /dev/null +++ b/spec/controllers/concerns/second_mfa_reminder_concern_spec.rb @@ -0,0 +1,76 @@ +require 'rails_helper' + +RSpec.describe SecondMfaReminderConcern do + let(:test_class) do + Class.new do + include SecondMfaReminderConcern + + attr_reader :current_user + + def initialize(current_user:) + @current_user = current_user + end + end + end + let(:user) { build(:user) } + let(:instance) { test_class.new(current_user: user) } + + describe '#user_needs_second_mfa_reminder?' do + subject(:user_needs_second_mfa_reminder) { instance.user_needs_second_mfa_reminder? } + + context 'user has already dismissed second mfa reminder' do + let(:user) { build(:user, second_mfa_reminder_dismissed_at: Time.zone.now) } + + it { expect(user_needs_second_mfa_reminder).to eq(false) } + end + + context 'user has multiple mfas configured' do + let(:user) { build(:user, :with_phone, :with_piv_or_cac) } + + it { expect(user_needs_second_mfa_reminder).to eq(false) } + end + + context 'user has single mfa configured' do + let(:user) { build(:user, :with_phone) } + + it { expect(user_needs_second_mfa_reminder).to eq(false) } + + context 'user has signed in more times than the threshold for reminder' do + let(:user) do + user = create(:user, :with_phone) + 2.times { user.events.create(event_type: :sign_in_before_2fa, created_at: Time.zone.now) } + user + end + + before do + allow(IdentityConfig.store).to receive(:second_mfa_reminder_sign_in_count).and_return(2) + end + + it { expect(user_needs_second_mfa_reminder).to eq(true) } + end + + context 'user has exceeded account age threshold for reminder' do + let(:user) { build(:user, :with_phone, created_at: 11.days.ago) } + + before do + allow(IdentityConfig.store).to receive(:second_mfa_reminder_account_age_in_days). + and_return(10) + end + + it { expect(user_needs_second_mfa_reminder).to eq(true) } + end + + context 'user meets threshold but feature is disabled' do + let(:user) { build(:user, :with_phone, created_at: 11.days.ago) } + + before do + allow(IdentityConfig.store).to receive(:second_mfa_reminder_account_age_in_days). + and_return(10) + allow(IdentityConfig.store).to receive(:second_mfa_reminder_enabled).and_return(false) + end + + it { expect(user_needs_second_mfa_reminder).to eq(false) } + end + end + end +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 cdf70de6e9a..20052aa7760 100644 --- a/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb @@ -123,73 +123,80 @@ controller.user_session[:webauthn_challenge] = webauthn_challenge end - it 'tracks a valid non-platform authenticator submission' do - create( - :webauthn_configuration, - user: controller.current_user, - credential_id: credential_id, - credential_public_key: credential_public_key, - ) - allow(WebauthnVerificationForm).to receive(:domain_name).and_return('localhost:3000') - webauthn_configuration = controller.current_user.webauthn_configurations.first - result = { - context: 'authentication', - multi_factor_auth_method: 'webauthn', - success: true, - webauthn_configuration_id: webauthn_configuration.id, - multi_factor_auth_method_created_at: webauthn_configuration.created_at.strftime('%s%L'), - } + context 'with a valid submission' do + let!(:webauthn_configuration) do + create( + :webauthn_configuration, + user: controller.current_user, + credential_id: credential_id, + credential_public_key: credential_public_key, + ) + controller.current_user.webauthn_configurations.first + end + let(:result) do + { + context: 'authentication', + multi_factor_auth_method: 'webauthn', + success: true, + webauthn_configuration_id: webauthn_configuration.id, + multi_factor_auth_method_created_at: webauthn_configuration.created_at.strftime('%s%L'), + } + end - expect(@analytics).to receive(:track_mfa_submit_event). - with(result) - expect(@analytics).to receive(:track_event). - with('User marked authenticated', authentication_type: :valid_2fa) + before do + allow(WebauthnVerificationForm).to receive(:domain_name).and_return('localhost:3000') + end - expect(@irs_attempts_api_tracker).to receive(:track_event).with( - :mfa_login_webauthn_roaming, - success: true, - ) + it 'tracks a valid submission' do + expect(@analytics).to receive(:track_mfa_submit_event). + with(result) + expect(@analytics).to receive(:track_event). + with('User marked authenticated', authentication_type: :valid_2fa) - patch :confirm, params: params + expect(@irs_attempts_api_tracker).to receive(:track_event).with( + :mfa_login_webauthn_roaming, + success: true, + ) - expect(subject.user_session[:auth_method]).to eq( - TwoFactorAuthenticatable::AuthMethod::WEBAUTHN, - ) - expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq false - end + patch :confirm, params: params - it 'tracks a valid platform authenticator submission' do - create( - :webauthn_configuration, - :platform_authenticator, - user: controller.current_user, - credential_id: credential_id, - credential_public_key: credential_public_key, - ) - allow(WebauthnVerificationForm).to receive(:domain_name).and_return('localhost:3000') - webauthn_configuration = controller.current_user.webauthn_configurations.first - result = { - context: 'authentication', - multi_factor_auth_method: 'webauthn_platform', - success: true, - webauthn_configuration_id: webauthn_configuration.id, - multi_factor_auth_method_created_at: webauthn_configuration.created_at.strftime('%s%L'), - } - expect(@analytics).to receive(:track_mfa_submit_event). - with(result) - expect(@analytics).to receive(:track_event). - with('User marked authenticated', authentication_type: :valid_2fa) + expect(subject.user_session[:auth_method]).to eq( + TwoFactorAuthenticatable::AuthMethod::WEBAUTHN, + ) + expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq false + end - expect(@irs_attempts_api_tracker).to receive(:track_event).with( - :mfa_login_webauthn_platform, - success: true, - ) + context 'with platform authenticator' do + let!(:webauthn_configuration) do + create( + :webauthn_configuration, + :platform_authenticator, + user: controller.current_user, + credential_id: credential_id, + credential_public_key: credential_public_key, + ) + controller.current_user.webauthn_configurations.first + end + let(:result) { super().merge(multi_factor_auth_method: 'webauthn_platform') } - patch :confirm, params: params - expect(subject.user_session[:auth_method]).to eq( - TwoFactorAuthenticatable::AuthMethod::WEBAUTHN_PLATFORM, - ) - expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq false + it 'tracks a valid submission' do + expect(@analytics).to receive(:track_mfa_submit_event). + with(result) + expect(@analytics).to receive(:track_event). + with('User marked authenticated', authentication_type: :valid_2fa) + + expect(@irs_attempts_api_tracker).to receive(:track_event).with( + :mfa_login_webauthn_platform, + success: true, + ) + + patch :confirm, params: params + expect(subject.user_session[:auth_method]).to eq( + TwoFactorAuthenticatable::AuthMethod::WEBAUTHN_PLATFORM, + ) + expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq false + end + end end it 'tracks an invalid submission' do diff --git a/spec/controllers/users/second_mfa_reminder_controller_spec.rb b/spec/controllers/users/second_mfa_reminder_controller_spec.rb new file mode 100644 index 00000000000..6d5ed096253 --- /dev/null +++ b/spec/controllers/users/second_mfa_reminder_controller_spec.rb @@ -0,0 +1,114 @@ +require 'rails_helper' + +RSpec.describe Users::SecondMfaReminderController do + let(:user) do + create( + :user, + created_at: (IdentityConfig.store.second_mfa_reminder_account_age_in_days + 1).days.ago, + ) + end + + before do + stub_sign_in(user) if user + stub_analytics + end + + describe '#new' do + subject(:response) { get :new } + + it 'logs an event' do + response + + expect(@analytics).to have_logged_event('Second MFA Reminder Visited') + end + + context 'signed out' do + let(:user) { nil } + + it 'redirects to sign in' do + expect(response).to redirect_to(new_user_session_path) + end + + it 'does not log an event' do + expect(@analytics).not_to have_logged_event('Second MFA Reminder Visited') + end + end + end + + describe '#create' do + let(:params) {} + subject(:response) { post :create, params: } + + context 'user declined' do + let(:params) { {} } + + it 'logs an event' do + response + + expect(@analytics).to have_logged_event( + 'Second MFA Reminder Dismissed', + opted_to_add: false, + ) + end + + it 'updates user to acknowledge dismissal of prompt' do + freeze_time do + expect { response }.to change { user.reload.second_mfa_reminder_dismissed_at }. + from(nil).to(Time.zone.now) + end + end + + it 'redirects to after-signin path' do + expect(response).to redirect_to(account_path) + end + + it 'does not assign session value' do + response + + expect(controller.user_session[:second_mfa_reminder_conversion]).to be_nil + end + end + + context 'user opted to add' do + let(:params) { { add_method: true } } + + it 'logs an event' do + response + + expect(@analytics).to have_logged_event( + 'Second MFA Reminder Dismissed', + opted_to_add: true, + ) + end + + it 'updates user to acknowledge dismissal of prompt' do + freeze_time do + expect { response }.to change { user.reload.second_mfa_reminder_dismissed_at }. + from(nil).to(Time.zone.now) + end + end + + it 'redirects to authentication methods setup' do + expect(response).to redirect_to(authentication_methods_setup_path) + end + + it 'assigns session value' do + response + + expect(controller.user_session[:second_mfa_reminder_conversion]).to eq(true) + end + end + + context 'signed out' do + let(:user) { nil } + + it 'redirects to sign in' do + expect(response).to redirect_to(new_user_session_path) + end + + it 'does not log an event' do + expect(@analytics).not_to have_logged_event('Second MFA Reminder Dismissed') + end + end + end +end diff --git a/spec/controllers/users/two_factor_authentication_controller_spec.rb b/spec/controllers/users/two_factor_authentication_controller_spec.rb index 8cc1d3109d8..8fc08267900 100644 --- a/spec/controllers/users/two_factor_authentication_controller_spec.rb +++ b/spec/controllers/users/two_factor_authentication_controller_spec.rb @@ -114,7 +114,7 @@ def index user = create(:user, :with_phone, **with_default_phone) stub_sign_in_before_2fa(user) - time1 = Time.zone.local(2023, 12, 13, 0, 0, 0) + time1 = Time.zone.local(2022, 12, 13, 0, 0, 0) cookies.encrypted[:remember_device] = { value: RememberDeviceCookie.new(user_id: user.id, created_at: time1).to_json, expires: time1 + 10.seconds, diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 1423b2ae404..fa42f7a32c6 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -12,6 +12,7 @@ confirmation_sent_at { 5.minutes.ago } end + created_at { Time.zone.now } accepted_terms_at { Time.zone.now if email } after(:build) do |user, evaluator| diff --git a/spec/features/idv/steps/gpo_step_spec.rb b/spec/features/idv/steps/gpo_step_spec.rb index 48e80fc2b55..dc55f16ab3b 100644 --- a/spec/features/idv/steps/gpo_step_spec.rb +++ b/spec/features/idv/steps/gpo_step_spec.rb @@ -13,6 +13,8 @@ and_return(minimum_wait_for_letter) allow(IdentityConfig.store).to receive(:gpo_max_profile_age_to_send_letter_in_days). and_return(max_days_before_resend_disabled) + allow(IdentityConfig.store).to receive(:second_mfa_reminder_account_age_in_days). + and_return(days_passed + 1) end it 'redirects to and completes the review step when the user chooses to verify by letter', :js do diff --git a/spec/features/remember_device/sp_expiration_spec.rb b/spec/features/remember_device/sp_expiration_spec.rb index e3fa086fc19..333ce427294 100644 --- a/spec/features/remember_device/sp_expiration_spec.rb +++ b/spec/features/remember_device/sp_expiration_spec.rb @@ -108,6 +108,8 @@ def visit_sp(protocol, aal) before do allow(IdentityConfig.store).to receive(:otp_delivery_blocklist_maxretry).and_return(1000) + allow(IdentityConfig.store).to receive(:second_mfa_reminder_account_age_in_days). + and_return([AAL1_REMEMBER_DEVICE_EXPIRATION, AAL2_REMEMBER_DEVICE_EXPIRATION].max.in_days + 2) ServiceProvider.find_by(issuer: OidcAuthHelper::OIDC_IAL1_ISSUER).update!( default_aal: aal, diff --git a/spec/features/two_factor_authentication/second_mfa_reminder_spec.rb b/spec/features/two_factor_authentication/second_mfa_reminder_spec.rb new file mode 100644 index 00000000000..b2d15ec3e93 --- /dev/null +++ b/spec/features/two_factor_authentication/second_mfa_reminder_spec.rb @@ -0,0 +1,95 @@ +require 'rails_helper' + +RSpec.feature 'Second MFA Reminder' do + include OidcAuthHelper + + let(:service_provider) { ServiceProvider.find_by(issuer: OidcAuthHelper::OIDC_IAL1_ISSUER) } + let(:user) { create(:user, :fully_registered, :with_phone) } + + before do + allow(IdentityConfig.store).to receive(:second_mfa_reminder_sign_in_count).and_return(2) + allow(IdentityConfig.store).to receive(:second_mfa_reminder_account_age_in_days).and_return(5) + IdentityLinker.new(user, service_provider).link_identity(verified_attributes: %w[openid email]) + end + + context 'user with single mfa' do + it 'does not prompt the user on sign in' do + sign_in_user(user) + fill_in_code_with_last_phone_otp + click_submit_default + + expect(page).to have_current_path(account_path) + end + + context 'after sign in count threshold' do + before do + sign_in_user(user) + fill_in_code_with_last_phone_otp + click_submit_default + first(:button, t('links.sign_out')).click + end + + it 'prompts the user on sign in and allows them to continue', :js do + # This spec includes regression coverage for a scenario where the user would be redirected + # immediately to the partner, requiring CSP header overrides that are not enforced if not + # using the JavaScript driver. + + visit_idp_from_ial1_oidc_sp + sign_in_user(user) + + expect(page).to have_current_path(second_mfa_reminder_path) + + click_on t('users.second_mfa_reminder.continue', sp_name: service_provider.friendly_name) + + expect(current_url).to start_with(service_provider.redirect_uris.first) + end + end + + context 'after age threshold' do + before { travel 6.days } + + it 'prompts the user on sign in and allows them to add an authentication method' do + sign_in_user(user) + fill_in_code_with_last_phone_otp + click_submit_default + + click_on t('users.second_mfa_reminder.add_method') + + expect(page).to have_current_path(authentication_methods_setup_url) + end + end + + context 'user already acknowledged reminder' do + before do + travel 6.days + sign_in_user(user) + fill_in_code_with_last_phone_otp + click_submit_default + click_button t('users.second_mfa_reminder.continue', sp_name: APP_NAME) + first(:button, t('links.sign_out')).click + end + + it 'does not prompt the user on sign in' do + sign_in_user(user) + + expect(page).to have_current_path(account_path) + end + end + end + + context 'user with multiple mfas who would otherwise be candidate' do + let(:user) { create(:user, :fully_registered, :with_phone, :with_authentication_app) } + + before do + travel 6.days + end + + it 'does not prompt the user on sign in' do + sign_in_user(user) + fill_in_code_with_last_totp(user) + click_submit_default + + expect(page).to have_current_path(account_path) + end + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 0a211ae5066..2cb1d792a04 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1490,6 +1490,20 @@ def it_should_not_send_survey end end + describe '#sign_in_count' do + it 'returns sign-in event count since the given time' do + freeze_time do + user = create(:user) + user.events.create(event_type: :sign_in_before_2fa, created_at: 1.day.ago) + user.events.create(event_type: :email_changed, created_at: 1.day.ago) + user.events.create(event_type: :sign_in_before_2fa, created_at: 2.days.ago) + user.events.create(event_type: :sign_in_before_2fa, created_at: 3.days.ago) + + expect(user.sign_in_count(since: 2.days.ago)).to eq(2) + end + end + end + describe '#second_last_signed_in_at' do it 'returns second most recent full authentication event' do user = create(:user) diff --git a/spec/presenters/two_factor_options_presenter_spec.rb b/spec/presenters/two_factor_options_presenter_spec.rb index 383fdeb3595..ff6aba336cc 100644 --- a/spec/presenters/two_factor_options_presenter_spec.rb +++ b/spec/presenters/two_factor_options_presenter_spec.rb @@ -105,6 +105,22 @@ end end + describe '#skip_label' do + subject(:skip_label) { presenter.skip_label } + + it 'is "Skip"' do + expect(skip_label).to eq(t('mfa.skip')) + end + + context 'user has dismissed second mfa reminder' do + let(:user) { build(:user, second_mfa_reminder_dismissed_at: Time.zone.now) } + + it 'is "Cancel"' do + expect(skip_label).to eq(t('links.cancel')) + end + end + end + describe '#show_skip_additional_mfa_link?' do it 'returns true' do expect(presenter.show_skip_additional_mfa_link?).to eq(true) diff --git a/spec/views/users/second_mfa_reminder/new.html.erb_spec.rb b/spec/views/users/second_mfa_reminder/new.html.erb_spec.rb new file mode 100644 index 00000000000..e05588af401 --- /dev/null +++ b/spec/views/users/second_mfa_reminder/new.html.erb_spec.rb @@ -0,0 +1,25 @@ +require 'rails_helper' + +RSpec.describe 'users/second_mfa_reminder/new.html.erb' do + subject(:rendered) { render } + + let(:sp_name) {} + + before do + decorated_session = double + allow(decorated_session).to receive(:sp_name).and_return(sp_name) + allow(view).to receive(:decorated_session).and_return(decorated_session) + end + + it 'renders with fallback app name for continue button' do + expect(rendered).to have_button(t('users.second_mfa_reminder.continue', sp_name: APP_NAME)) + end + + context 'with sp name' do + let(:sp_name) { 'Example SP' } + + it 'renders with sp name for continue button' do + expect(rendered).to have_button(t('users.second_mfa_reminder.continue', sp_name:)) + end + end +end