Skip to content
Merged
12 changes: 10 additions & 2 deletions app/controllers/users/mfa_selection_controller.rb
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remind me why we have two different controllers for MFA setup? 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh something I want to refactor, the mfa_selection controller is for the controller to select/add additional mfa methods after the prompt if a user selects a single MFA method on account creation. I think originally the mfa selection had a few extra things that were extra/annoying (showing/graying out already selected mfa method, as well as two factor authentication setup controller will redirect you to account page if you have one authentication method already). We can definitely make this a lot cleaner.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you want to write a ticket summarizing the problem and maybe we can get that work prioritized?

Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,20 @@ class MfaSelectionController < ApplicationController
def index
two_factor_options_form
@after_setup_path = after_mfa_setup_path
@sign_up_mfa_selection_order_bucket = AbTests::SIGN_UP_MFA_SELECTION.bucket(current_user.uuid)
@presenter = two_factor_options_presenter
analytics.user_registration_2fa_additional_setup_visit
analytics.user_registration_2fa_additional_setup_visit(
sign_up_mfa_priority_bucket: @sign_up_mfa_selection_order_bucket,
)
end

def update
result = submit_form
analytics.user_registration_2fa_additional_setup(**result.to_h)
@sign_up_mfa_selection_order_bucket = AbTests::SIGN_UP_MFA_SELECTION.bucket(current_user.uuid)
analytics_hash = result.to_h.merge(
sign_up_mfa_priority_bucket: @sign_up_mfa_selection_order_bucket,
)
analytics.user_registration_2fa_additional_setup(**analytics_hash)

if result.success?
process_valid_form
Expand All @@ -38,6 +45,7 @@ def submit_form
def two_factor_options_presenter
TwoFactorOptionsPresenter.new(
user_agent: request.user_agent,
priority_bucket: @sign_up_mfa_selection_order_bucket,
user: current_user,
phishing_resistant_required: service_provider_mfa_policy.phishing_resistant_required?,
piv_cac_required: service_provider_mfa_policy.piv_cac_required?,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,20 @@ class TwoFactorAuthenticationSetupController < ApplicationController

def index
two_factor_options_form
@sign_up_mfa_selection_order_bucket = AbTests::SIGN_UP_MFA_SELECTION.bucket(current_user.uuid)
@presenter = two_factor_options_presenter
analytics.user_registration_2fa_setup_visit
analytics.user_registration_2fa_setup_visit(
sign_up_mfa_priority_bucket: @sign_up_mfa_selection_order_bucket,
)
end

def create
result = submit_form
analytics.user_registration_2fa_setup(**result.to_h)
@sign_up_mfa_selection_order_bucket = AbTests::SIGN_UP_MFA_SELECTION.bucket(current_user.uuid)
analytics_hash = result.to_h.merge(
sign_up_mfa_priority_bucket: @sign_up_mfa_selection_order_bucket,
)
analytics.user_registration_2fa_setup(**analytics_hash)
irs_attempts_api_tracker.mfa_enroll_options_selected(
success: result.success?,
mfa_device_types: @two_factor_options_form.selection,
Expand All @@ -42,6 +49,7 @@ def submit_form
def two_factor_options_presenter
TwoFactorOptionsPresenter.new(
user_agent: request.user_agent,
priority_bucket: @sign_up_mfa_selection_order_bucket,
user: current_user,
phishing_resistant_required: service_provider_mfa_policy.phishing_resistant_required?,
piv_cac_required: service_provider_mfa_policy.piv_cac_required?,
Expand Down
16 changes: 11 additions & 5 deletions app/presenters/two_factor_options_presenter.rb
Original file line number Diff line number Diff line change
@@ -1,19 +1,25 @@
class TwoFactorOptionsPresenter
include ActionView::Helpers::TranslationHelper

attr_reader :user
attr_reader :user, :priority_bucket

def initialize(user_agent:, user: nil, phishing_resistant_required: false,
piv_cac_required: false)
def initialize(user_agent:, user: nil, priority_bucket: :default,
phishing_resistant_required: false, piv_cac_required: false)
@user_agent = user_agent
@user = user
@priority_bucket = priority_bucket
@phishing_resistant_required = phishing_resistant_required
@piv_cac_required = piv_cac_required
end

def options
webauthn_platform_option + webauthn_option + piv_cac_option + totp_option +
phone_options + backup_code_option
if priority_bucket == :authentication_app_priority
totp_option + webauthn_platform_option + webauthn_option + piv_cac_option +
phone_options + backup_code_option
else
webauthn_platform_option + webauthn_option + piv_cac_option + totp_option +
phone_options + backup_code_option
end
end

def icon
Expand Down
28 changes: 23 additions & 5 deletions app/services/analytics_events.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2796,31 +2796,43 @@ def telephony_otp_sent(

# @param [Boolean] success
# @param [Hash] errors
# @param [String] sign_up_mfa_priority_bucket
# Tracks when the the user has selected and submitted additional MFA methods on user registration
def user_registration_2fa_additional_setup(success:, errors: nil, **extra)
def user_registration_2fa_additional_setup(success:,
sign_up_mfa_priority_bucket:,
errors: nil,
**extra)
track_event(
'User Registration: Additional 2FA Setup',
{
success: success,
sign_up_mfa_priority_bucket: sign_up_mfa_priority_bucket,
errors: errors,
**extra,
}.compact,
)
end

# Tracks when user visits additional MFA selection page
def user_registration_2fa_additional_setup_visit
track_event('User Registration: Additional 2FA Setup visited')
# @param [String] sign_up_mfa_priority_bucket
def user_registration_2fa_additional_setup_visit(sign_up_mfa_priority_bucket:, **extra)
track_event(
'User Registration: Additional 2FA Setup visited',
sign_up_mfa_priority_bucket: sign_up_mfa_priority_bucket,
**extra,
)
end

# @param [Boolean] success
# @param [Hash] errors
# @param [Integer] enabled_mfa_methods_count
# @param [Integer] selected_mfa_count
# @param ['voice', 'auth_app'] selection
# @param [String] sign_up_mfa_priority_bucket
# Tracks when the the user has selected and submitted MFA auth methods on user registration
def user_registration_2fa_setup(
success:,
sign_up_mfa_priority_bucket:,
errors: nil,
selected_mfa_count: nil,
enabled_mfa_methods_count: nil,
Expand All @@ -2834,6 +2846,7 @@ def user_registration_2fa_setup(
errors: errors,
selected_mfa_count: selected_mfa_count,
enabled_mfa_methods_count: enabled_mfa_methods_count,
sign_up_mfa_priority_bucket: sign_up_mfa_priority_bucket,
selection: selection,
**extra,
}.compact,
Expand Down Expand Up @@ -2903,8 +2916,13 @@ def user_registration_suggest_another_mfa_notice_skipped
end

# Tracks when user visits MFA selection page
def user_registration_2fa_setup_visit
track_event('User Registration: 2FA Setup visited')
# @param [String] sign_up_mfa_priority_bucket
def user_registration_2fa_setup_visit(sign_up_mfa_priority_bucket:, **extra)
track_event(
'User Registration: 2FA Setup visited',
sign_up_mfa_priority_bucket: sign_up_mfa_priority_bucket,
**extra,
)
end

# @param [Hash] vendor_status
Expand Down
1 change: 1 addition & 0 deletions config/application.yml.default
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,7 @@ session_timeout_warning_seconds: 150
session_total_duration_timeout_in_minutes: 720
ses_configuration_set_name: ''
set_remember_device_session_expiration: false
sign_up_mfa_selection_order_testing: '{ "default": 100, "authentication_app_priority": 0 }'
sign_in_a_b_testing: '{"default":100,"tabbed":0}'
sp_handoff_bounce_max_seconds: 2
show_user_attribute_deprecation_warnings: false
Expand Down
5 changes: 5 additions & 0 deletions config/initializers/ab_tests.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@ def self.in_person_cta_variant_testing_buckets
buckets
end

SIGN_UP_MFA_SELECTION = AbTestBucket.new(
experiment_name: 'MFA selection order: Auth app first',
buckets: IdentityConfig.store.sign_up_mfa_selection_order_testing,
)

IN_PERSON_CTA = AbTestBucket.new(
experiment_name: 'In-Person Proofing CTA',
buckets: in_person_cta_variant_testing_buckets,
Expand Down
4 changes: 4 additions & 0 deletions lib/identity_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,10 @@ def self.build_store(config_map)
config.add(:ses_configuration_set_name, type: :string)
config.add(:set_remember_device_session_expiration, type: :boolean)
config.add(:show_user_attribute_deprecation_warnings, type: :boolean)
config.add(
:sign_up_mfa_selection_order_testing, type: :json,
options: { symbolize_names: true }
)
config.add(:sign_in_a_b_testing, type: :json, options: { symbolize_names: true })
config.add(:skip_encryption_allowed_list, type: :json)
config.add(:sp_handoff_bounce_max_seconds, type: :integer)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
stub_analytics

expect(@analytics).to receive(:track_event).
with('User Registration: 2FA Setup visited')
with('User Registration: 2FA Setup visited', sign_up_mfa_priority_bucket: :default)

get :index
end
Expand Down Expand Up @@ -66,6 +66,7 @@
}
params = ActionController::Parameters.new(voice_params)
response = FormResponse.new(success: true, errors: {}, extra: { selection: ['voice'] })
analytics_hash = response.to_h.merge(sign_up_mfa_priority_bucket: :default)

form_params = { user: user, phishing_resistant_required: false, piv_cac_required: nil }
form = instance_double(TwoFactorOptionsForm)
Expand All @@ -77,7 +78,7 @@

patch :create, params: voice_params

expect(@analytics).to have_logged_event('User Registration: 2FA Setup', response.to_h)
expect(@analytics).to have_logged_event('User Registration: 2FA Setup', analytics_hash)
end

it 'tracks analytics event' do
Expand All @@ -89,6 +90,7 @@
selection: ['voice', 'auth_app'],
success: true,
selected_mfa_count: 2,
sign_up_mfa_priority_bucket: :default,
errors: {},
}

Expand Down
11 changes: 11 additions & 0 deletions spec/presenters/two_factor_options_presenter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -76,5 +76,16 @@
]
end
end
context 'when priority is authentication first in a/b testing bucket' do
let(:presenter) do
described_class.new(user_agent: user_agent, priority_bucket: :authentication_app_priority)
end

it 'supplies auth app as the first option' do
expect(presenter.options.first.class).to eq(
TwoFactorAuthentication::AuthAppSelectionPresenter,
)
end
end
end
end