Skip to content

Improve consistency when marking users as fully authenticated after successful multi-factor authenticator setup #8431

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mitchellhenke merged 1 commit into from
May 19, 2023
Merged

Improve consistency when marking users as fully authenticated after successful multi-factor authenticator setup #8431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions app/controllers/concerns/two_factor_authenticatable_methods.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ def handle_valid_otp_for_context(auth_method:)
if UserSessionContext.authentication_or_reauthentication_context?(context)
handle_valid_otp_for_authentication_context(auth_method: auth_method)
elsif UserSessionContext.confirmation_context?(context)
handle_valid_verification_for_confirmation_context
handle_valid_verification_for_confirmation_context(auth_method: auth_method)
end
end

Expand Down Expand Up @@ -168,9 +168,9 @@ def update_invalid_user
current_user.increment_second_factor_attempts_count!
end

def handle_valid_verification_for_confirmation_context
user_session[:authn_at] = Time.zone.now
@next_mfa_setup_path = next_setup_path
Copy link
Copy Markdown
Contributor Author

@mitchellhenke mitchellhenke May 18, 2023

Choose a reason for hiding this comment

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

This line is only used by the otp_verification_controller when in a confirmation context, so it was moved there. Ideally I would have moved it in #8424

def handle_valid_verification_for_confirmation_context(auth_method:)
user_session[:auth_method] = auth_method
mark_user_session_authenticated(:valid_2fa_confirmation)
Copy link
Copy Markdown
Contributor

@aduth aduth May 19, 2023

Choose a reason for hiding this comment

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

I'm trying to follow how OTP previously would have set user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION] to false, or whether it would have (or even if it needed to?). Just flagging since that will be happening now.

Copy link
Copy Markdown
Contributor Author

@mitchellhenke mitchellhenke May 19, 2023

Choose a reason for hiding this comment

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

Previously it wouldn't have for phone OTP confirmation. We did do it when confirming other methods though, so this brings it all in line to always set it.

Copy link
Copy Markdown
Contributor

@aduth aduth May 19, 2023

Choose a reason for hiding this comment

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

Also, I assume this may cause some additional analytics to be logged via mark_user_session_authenticated's call to mark_user_session_authenticated_analytics. Should we have specs for that?

Copy link
Copy Markdown
Contributor Author

@mitchellhenke mitchellhenke May 19, 2023

Choose a reason for hiding this comment

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

Yep, they were causing a failure, added a few changes to add specs around it.

reset_second_factor_attempts_count
end

Expand Down
1 change: 1 addition & 0 deletions app/controllers/two_factor_authentication/otp_verification_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ def create
def handle_valid_confirmation_otp
assign_phone
track_mfa_added
@next_mfa_setup_path = next_setup_path
flash[:success] = t('notices.phone_confirmed')
end

Expand Down
10 changes: 3 additions & 7 deletions app/controllers/users/backup_code_setup_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,14 +107,10 @@ def set_backup_code_setup_presenter
)
end

def mark_user_as_fully_authenticated
user_session[:auth_method] = TwoFactorAuthenticatable::AuthMethod::BACKUP_CODE
user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION] = false
user_session[:authn_at] = Time.zone.now
end

def save_backup_codes
mark_user_as_fully_authenticated
handle_valid_verification_for_confirmation_context(
auth_method: TwoFactorAuthenticatable::AuthMethod::BACKUP_CODE,
)
generator.save(user_session[:backup_codes])
event = PushNotification::RecoveryInformationChangedEvent.new(user: current_user)
PushNotification::HttpPush.deliver(event)
Expand Down
13 changes: 4 additions & 9 deletions app/controllers/users/piv_cac_authentication_setup_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module Users
class PivCacAuthenticationSetupController < ApplicationController
include UserAuthenticator
include TwoFactorAuthenticatableMethods
include PivCacConcern
include MfaSetupConcern
include RememberDeviceConcern
Expand Down Expand Up @@ -118,7 +118,9 @@ def user_piv_cac_form
end

def process_valid_submission
mark_user_as_fully_authenticated
handle_valid_verification_for_confirmation_context(
auth_method: TwoFactorAuthenticatable::AuthMethod::PIV_CAC,
)
flash[:success] = t('notices.piv_cac_configured')
save_piv_cac_information(
subject: user_piv_cac_form.x509_dn,
Expand All @@ -132,13 +134,6 @@ def process_valid_submission
redirect_to next_setup_path || final_path
end

def mark_user_as_fully_authenticated
user_session[:auth_method] = TwoFactorAuthenticatable::AuthMethod::PIV_CAC

user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION] = false
user_session[:authn_at] = Time.zone.now
end

def track_mfa_method_added
mfa_user = MfaContext.new(current_user)
analytics.multi_factor_auth_added_piv_cac(
Expand Down
4 changes: 4 additions & 0 deletions app/controllers/users/piv_cac_setup_from_sign_in_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
module Users
class PivCacSetupFromSignInController < ApplicationController
include TwoFactorAuthenticatableMethods
include PivCacConcern
include SecureHeadersConcern
include ReauthenticationRequiredConcern
Expand Down Expand Up @@ -67,6 +68,9 @@ def process_invalid_submission
end

def process_valid_submission
handle_valid_verification_for_confirmation_context(
auth_method: TwoFactorAuthenticatable::AuthMethod::PIV_CAC,
)
session.delete(:needs_to_setup_piv_cac_after_sign_in)
save_piv_cac_information(
subject: user_piv_cac_form.x509_dn,
Expand Down
9 changes: 3 additions & 6 deletions app/controllers/users/totp_setup_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,9 @@ def store_totp_secret_in_session

def process_valid_code
create_events
mark_user_as_fully_authenticated
handle_valid_verification_for_confirmation_context(
auth_method: TwoFactorAuthenticatable::AuthMethod::TOTP,
)
handle_remember_device
flash[:success] = t('notices.totp_configured')
user_session.delete(:new_totp_secret)
Expand Down Expand Up @@ -117,11 +119,6 @@ def revoke_otp_secret_key
PushNotification::HttpPush.deliver(event)
end

def mark_user_as_fully_authenticated
user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION] = false
user_session[:authn_at] = Time.zone.now
end

def process_invalid_code
flash[:error] = if totp_setup_form.name_taken
t('errors.piv_cac_setup.unique_name')
Expand Down
18 changes: 6 additions & 12 deletions app/controllers/users/webauthn_setup_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,12 +158,17 @@ def process_valid_webauthn(form)
platform_authenticator: form.platform_authenticator?,
enabled_mfa_methods_count: mfa_user.enabled_mfa_methods_count,
)
mark_user_as_fully_authenticated(form)
handle_remember_device
if form.platform_authenticator?
handle_valid_verification_for_confirmation_context(
auth_method: TwoFactorAuthenticatable::AuthMethod::WEBAUTHN_PLATFORM,
)
Funnel::Registration::AddMfa.call(current_user.id, 'webauthn_platform', analytics)
flash[:success] = t('notices.webauthn_platform_configured')
else
handle_valid_verification_for_confirmation_context(
auth_method: TwoFactorAuthenticatable::AuthMethod::WEBAUTHN,
)
Funnel::Registration::AddMfa.call(current_user.id, 'webauthn', analytics)
flash[:success] = t('notices.webauthn_configured')
end
Expand Down Expand Up @@ -192,17 +197,6 @@ def process_invalid_webauthn(form)
render :new
end

def mark_user_as_fully_authenticated(form)
if form.platform_authenticator?
user_session[:auth_method] = TwoFactorAuthenticatable::AuthMethod::WEBAUTHN_PLATFORM
else
user_session[:auth_method] = TwoFactorAuthenticatable::AuthMethod::WEBAUTHN
end

user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION] = false
user_session[:authn_at] = Time.zone.now
end

def new_params
params.permit(:platform, :error)
end
Expand Down
2 changes: 2 additions & 0 deletions spec/controllers/users/backup_code_setup_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
Funnel::Registration::AddMfa.call(user.id, 'phone', analytics)
expect(PushNotification::HttpPush).to receive(:deliver).
with(PushNotification::RecoveryInformationChangedEvent.new(user: user))
expect(@analytics).to receive(:track_event).
with('User marked authenticated', { authentication_type: :valid_2fa_confirmation })
expect(@analytics).to receive(:track_event).
with('Backup Code Setup Visited', {
success: true,
Expand Down
6 changes: 6 additions & 0 deletions spec/controllers/users/webauthn_setup_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@
sign_up_mfa_selection_order_bucket: nil,
pii_like_keypaths: [[:mfa_method_counts, :phone]],
}
expect(@analytics).to receive(:track_event).
with('User marked authenticated', { authentication_type: :valid_2fa_confirmation })
expect(@analytics).to receive(:track_event).
with('Multi-Factor Authentication Setup', result)

Expand Down Expand Up @@ -210,6 +212,8 @@
end
it 'should log expected events' do
Funnel::Registration::AddMfa.call(user.id, 'phone', @analytics)
expect(@analytics).to receive(:track_event).
with('User marked authenticated', { authentication_type: :valid_2fa_confirmation })
expect(@analytics).to receive(:track_event).with(
'Multi-Factor Authentication Setup',
{
Expand Down Expand Up @@ -261,6 +265,8 @@
}
end
it 'should log expected events' do
expect(@analytics).to receive(:track_event).
with('User marked authenticated', { authentication_type: :valid_2fa_confirmation })
expect(@analytics).to receive(:track_event).with(
'User Registration: User Fully Registered',
{ mfa_method: 'webauthn_platform' },
Expand Down