Skip to content

Clean up feature flag for second factor reauthentication #8707

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 2 commits into from
Jul 5, 2023
Merged

Clean up feature flag for second factor reauthentication #8707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
24fd453
Clean up feature flag for second factor reauthentication
Jul 3, 2023
1b1c49d
remove unused methods
Jul 3, 2023
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
2 changes: 1 addition & 1 deletion app/controllers/accounts/personal_keys_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ class PersonalKeysController < ApplicationController

before_action :confirm_two_factor_authenticated
before_action :prompt_for_password_if_pii_locked
before_action :confirm_recently_authenticated
before_action :confirm_recently_authenticated_2fa

def new
analytics.profile_personal_key_visit
Expand Down
30 changes: 0 additions & 30 deletions app/controllers/concerns/reauthentication_required_concern.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,6 @@
module ReauthenticationRequiredConcern
include MfaSetupConcern

def confirm_recently_authenticated
if IdentityConfig.store.reauthentication_for_second_factor_management_enabled
confirm_recently_authenticated_2fa
else
@reauthn = reauthn?
return unless user_signed_in?
return if recently_authenticated?

prompt_for_current_password
end
end

def confirm_recently_authenticated_2fa
Copy link
Contributor

@aduth aduth Jul 5, 2023

Choose a reason for hiding this comment

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

Out of curiosity, was the _2fa suffix added to help with clarity, or just something we added to avoid conflict with the original method name (i.e. we'd want to circle back and rename to drop it)?

Personally I think it's an improvement though 👍

Copy link
Contributor Author

@mitchellhenke mitchellhenke Jul 5, 2023

Choose a reason for hiding this comment

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

It was added to differentiate from the previous method (which intended to require both password and 2FA).

I like the idea of keeping it as well for descriptiveness' sake.

@reauthn = reauthn?
return unless user_fully_authenticated?
Expand All @@ -38,31 +26,13 @@ def recently_authenticated?
authn_at > Time.zone.now - IdentityConfig.store.reauthn_window
end

def prompt_for_current_password
store_location(request.url)
user_session[:context] = 'reauthentication'
user_session[:factor_to_change] = factor_from_controller_name
Copy link
Contributor

@aduth aduth Jul 3, 2023

Choose a reason for hiding this comment

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

This is the only place this is assigned, but it's still referenced in a view? 🤔

identity-idp/app/views/mfa_confirmation/new.html.erb

Line 7 in 77311e9

<%= t('help_text.change_factor', factor: user_session[:factor_to_change]) %>

Copy link
Contributor Author

@mitchellhenke mitchellhenke Jul 5, 2023

Choose a reason for hiding this comment

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

Yeah, this controller has been re-used in other ways where it's not changing factors and the change_factor does not exist. It's also not translated and the content should be re-written to be more generic.

Copy link
Contributor Author

@mitchellhenke mitchellhenke Jul 5, 2023

Choose a reason for hiding this comment

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

https://gsa-tts.slack.com/archives/CEUQ9FXNJ/p1643994476402979 is a Slack thread that discusses it a bit (with an associated ticket that's linked above that line)

Copy link
Contributor

@aduth aduth Jul 5, 2023

Choose a reason for hiding this comment

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

I'll un-icebox that ticket and get it in front of the team.

user_session[:current_password_required] = true
redirect_to user_password_confirm_url
end

def prompt_for_second_factor
store_location(request.url)
user_session[:context] = 'reauthentication'

redirect_to login_two_factor_options_path(reauthn: true)
end

def factor_from_controller_name
{
# see LG-5701, translate these
Copy link
Contributor

@aduth aduth Jul 5, 2023

Choose a reason for hiding this comment

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

Can this ticket be cancelled?

Copy link
Contributor Author

@mitchellhenke mitchellhenke Jul 5, 2023

Choose a reason for hiding this comment

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

I think so, yeah. I can do that.

Copy link
Contributor

@zachmargolis zachmargolis Jul 5, 2023

Choose a reason for hiding this comment

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

The part about the untranslated string can be cancelled, but does the copy on the page still say "sign in with your phone" for all other MFAs? If so then that part of the ticket should stay

Copy link
Contributor Author

@mitchellhenke mitchellhenke Jul 5, 2023

Choose a reason for hiding this comment

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

My hope is to remove this page since it'll be unused after this PR

Copy link
Contributor

@zachmargolis zachmargolis Jul 5, 2023

Choose a reason for hiding this comment

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

ah right!!! thx

Copy link
Contributor Author

@mitchellhenke mitchellhenke Jul 5, 2023

Choose a reason for hiding this comment

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

#8722

'emails' => 'email',
'passwords' => 'password',
'phones' => 'phone',
'personal_keys' => 'personal key',
}[controller_name]
end

def store_location(url)
user_session[:stored_location] = url
end
Expand Down
6 changes: 1 addition & 5 deletions app/controllers/concerns/remember_device_concern.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,7 @@ def save_remember_device_preference
end

def check_remember_device_preference
if IdentityConfig.store.reauthentication_for_second_factor_management_enabled
return unless UserSessionContext.authentication_context?(context)
else
return unless UserSessionContext.authentication_or_reauthentication_context?(context)
end
return unless UserSessionContext.authentication_context?(context)
return if remember_device_cookie.nil?
return unless remember_device_cookie.valid_for_user?(
user: current_user,
Expand Down
4 changes: 1 addition & 3 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 @@ -11,9 +11,7 @@ class BackupCodeSetupController < ApplicationController
before_action :set_backup_code_setup_presenter
before_action :apply_secure_headers_override
before_action :authorize_backup_code_disable, only: [:delete]
before_action :confirm_recently_authenticated_2fa, except: [:reminder], if: -> do
IdentityConfig.store.reauthentication_for_second_factor_management_enabled
end
before_action :confirm_recently_authenticated_2fa, except: [:reminder]

helper_method :in_multi_mfa_selection_flow?

Expand Down
2 changes: 1 addition & 1 deletion app/controllers/users/edit_phone_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ class EditPhoneController < ApplicationController
before_action :confirm_two_factor_authenticated
before_action :confirm_user_can_edit_phone
before_action :confirm_user_can_remove_phone, only: %i[destroy]
before_action :confirm_recently_authenticated
before_action :confirm_recently_authenticated_2fa

def edit
analytics.phone_change_viewed
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/users/emails_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ class EmailsController < ApplicationController
before_action :authorize_user_to_edit_email, except: %i[add show verify resend]
before_action :check_max_emails_per_account, only: %i[show add]
before_action :retain_confirmed_emails, only: %i[delete]
before_action :confirm_recently_authenticated
before_action :confirm_recently_authenticated_2fa

def show
@add_user_email_form = AddUserEmailForm.new
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/users/passwords_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class PasswordsController < ApplicationController

before_action :confirm_two_factor_authenticated
before_action :capture_password_if_pii_requested_but_locked
before_action :confirm_recently_authenticated
before_action :confirm_recently_authenticated_2fa

def edit
@update_user_password_form = UpdateUserPasswordForm.new(current_user)
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/users/phones_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class PhonesController < ApplicationController
before_action :redirect_if_phone_vendor_outage
before_action :check_max_phone_numbers_per_account, only: %i[add create]
before_action :allow_csp_recaptcha_src, if: :recaptcha_enabled?
before_action :confirm_recently_authenticated
before_action :confirm_recently_authenticated_2fa

def add
user_session[:phone_id] = nil
Expand Down
4 changes: 1 addition & 3 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
Expand Up @@ -12,9 +12,7 @@ class PivCacAuthenticationSetupController < ApplicationController
before_action :authorize_piv_cac_disable, only: :delete
before_action :set_piv_cac_setup_csp_form_action_uris, only: :new
before_action :cap_piv_cac_count, only: %i[new submit_new_piv_cac]
before_action :confirm_recently_authenticated_2fa, if: -> do
IdentityConfig.store.reauthentication_for_second_factor_management_enabled
end
before_action :confirm_recently_authenticated_2fa

helper_method :in_multi_mfa_selection_flow?

Expand Down
2 changes: 1 addition & 1 deletion app/controllers/users/piv_cac_setup_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class PivCacSetupController < ApplicationController
include ReauthenticationRequiredConcern

before_action :confirm_two_factor_authenticated
before_action :confirm_recently_authenticated
before_action :confirm_recently_authenticated_2fa

def delete; end

Expand Down
4 changes: 1 addition & 3 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
Expand Up @@ -6,9 +6,7 @@ class PivCacSetupFromSignInController < ApplicationController
include ReauthenticationRequiredConcern

before_action :confirm_two_factor_authenticated
before_action :confirm_recently_authenticated_2fa, if: -> do
IdentityConfig.store.reauthentication_for_second_factor_management_enabled
end
before_action :confirm_recently_authenticated_2fa
before_action :apply_secure_headers_override, only: :success
before_action :set_piv_cac_setup_csp_form_action_uris, only: :prompt

Expand Down
4 changes: 1 addition & 3 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 @@ -10,9 +10,7 @@ class TotpSetupController < ApplicationController
before_action :set_totp_setup_presenter
before_action :apply_secure_headers_override
before_action :cap_auth_app_count, only: %i[new confirm]
before_action :confirm_recently_authenticated_2fa, if: -> do
IdentityConfig.store.reauthentication_for_second_factor_management_enabled
end
before_action :confirm_recently_authenticated_2fa

helper_method :in_multi_mfa_selection_flow?

Expand Down
4 changes: 1 addition & 3 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 @@ -9,9 +9,7 @@ class WebauthnSetupController < ApplicationController
before_action :confirm_user_authenticated_for_2fa_setup
before_action :apply_secure_headers_override
before_action :set_webauthn_setup_presenter
before_action :confirm_recently_authenticated_2fa, if: -> do
IdentityConfig.store.reauthentication_for_second_factor_management_enabled
end
before_action :confirm_recently_authenticated_2fa

helper_method :in_multi_mfa_selection_flow?

Expand Down
2 changes: 0 additions & 2 deletions config/application.yml.default
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,6 @@ rack_mini_profiler: false
rack_timeout_service_timeout_seconds: 15
rails_mailer_previews_enabled: false
reauthn_window: 120
reauthentication_for_second_factor_management_enabled: true
recaptcha_enterprise_api_key: ''
recaptcha_enterprise_project_id: ''
recaptcha_site_key_v2: ''
Expand Down Expand Up @@ -463,7 +462,6 @@ production:
phone_recaptcha_mock_validator: false
piv_cac_verify_token_secret:
session_encryptor_alert_enabled: true
reauthentication_for_second_factor_management_enabled: false
recurring_jobs_disabled_names: "[]"
redis_throttle_url: redis://redis.login.gov.internal:6379/1
redis_url: redis://redis.login.gov.internal:6379
Expand Down
1 change: 0 additions & 1 deletion lib/identity_config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,6 @@ def self.build_store(config_map)
config.add(:rack_timeout_service_timeout_seconds, type: :integer)
config.add(:rails_mailer_previews_enabled, type: :boolean)
config.add(:reauthn_window, type: :integer)
config.add(:reauthentication_for_second_factor_management_enabled, type: :boolean)
config.add(:recaptcha_enterprise_project_id, type: :string)
config.add(:recaptcha_enterprise_api_key, type: :string)
config.add(:recaptcha_site_key_v2, type: :string)
Expand Down
2 changes: 1 addition & 1 deletion spec/controllers/accounts/personal_keys_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
it 'require recent reauthn' do
expect(subject).to have_actions(
:before,
:confirm_recently_authenticated,
:confirm_recently_authenticated_2fa,
:prompt_for_password_if_pii_locked,
)
end
Expand Down
45 changes: 0 additions & 45 deletions spec/controllers/concerns/reauthentication_required_concern_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,51 +3,6 @@
RSpec.describe ReauthenticationRequiredConcern, type: :controller do
let(:user) { create(:user, :fully_registered, email: 'old_email@example.com') }

describe '#confirm_recently_authenticated' do
controller ApplicationController do
include ReauthenticationRequiredConcern

before_action :confirm_recently_authenticated

def index
render plain: 'Hello'
end
end

before(:each) do
stub_sign_in(user)
allow(IdentityConfig.store).to receive(
:reauthentication_for_second_factor_management_enabled,
).and_return(false)
end

context 'recently authenticated' do
it 'allows action' do
get :index

expect(response.body).to eq 'Hello'
end
end

context 'authenticated outside the authn window' do
before do
controller.user_session[:authn_at] -= IdentityConfig.store.reauthn_window
end

it 'redirects to password confirmation' do
get :index

expect(response).to redirect_to user_password_confirm_url
end

it 'sets context to authentication' do
get :index

expect(controller.user_session[:context]).to eq 'reauthentication'
end
end
end

describe '#confirm_recently_authenticated_2fa' do
controller ApplicationController do
include ReauthenticationRequiredConcern
Expand Down