diff --git a/.rubocop.yml b/.rubocop.yml index 43b9c52497c..11d3b732a5e 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -56,6 +56,7 @@ Metrics/ClassLength: - spec/**/* - app/controllers/application_controller.rb - app/controllers/openid_connect/authorization_controller.rb + - app/controllers/saml_idp_controller.rb - app/controllers/users/confirmations_controller.rb - app/controllers/users/sessions_controller.rb - app/controllers/users/two_factor_authentication_controller.rb diff --git a/app/controllers/concerns/remember_device_concern.rb b/app/controllers/concerns/remember_device_concern.rb index 59542d5d7c1..77ec8d58660 100644 --- a/app/controllers/concerns/remember_device_concern.rb +++ b/app/controllers/concerns/remember_device_concern.rb @@ -12,8 +12,12 @@ def save_remember_device_preference def check_remember_device_preference return unless authentication_context? return if remember_device_cookie.nil? - return unless remember_device_cookie.valid_for_user?(current_user) - handle_valid_otp + return unless remember_device_cookie.valid_for_user?( + user: current_user, + expiration_interval: decorated_session.mfa_expiration_interval + ) + + handle_valid_remember_device_cookie end def remember_device_cookie @@ -24,9 +28,27 @@ def remember_device_cookie ) end + def remember_device_expired_for_sp? + return false unless user_session[:mfa_device_remembered] + return true if remember_device_cookie.nil? + + !remember_device_cookie.valid_for_user?( + user: current_user, + expiration_interval: decorated_session.mfa_expiration_interval + ) + end + private + def handle_valid_remember_device_cookie + user_session[:mfa_device_remembered] = true + mark_user_session_authenticated + bypass_sign_in current_user + redirect_to after_otp_verification_confirmation_url + reset_otp_session_data + end + def remember_device_cookie_expiration - Figaro.env.remember_device_expiration_days.to_i.days.from_now + Figaro.env.remember_device_expiration_hours_aal_1.to_i.hours.from_now end end diff --git a/app/controllers/concerns/two_factor_authenticatable.rb b/app/controllers/concerns/two_factor_authenticatable.rb index a2950c04c4d..52df8a0072f 100644 --- a/app/controllers/concerns/two_factor_authenticatable.rb +++ b/app/controllers/concerns/two_factor_authenticatable.rb @@ -53,8 +53,10 @@ def current_password_required? def check_already_authenticated return unless initial_authentication_context? + return unless user_fully_authenticated? + return if remember_device_expired_for_sp? - redirect_to after_otp_verification_confirmation_url if user_fully_authenticated? + redirect_to after_otp_verification_confirmation_url end def reset_attempt_count_if_user_no_longer_locked_out @@ -76,6 +78,7 @@ def handle_valid_otp handle_valid_otp_for_confirmation_context end save_remember_device_preference + user_session.delete(:mfa_device_remembered) redirect_to after_otp_verification_confirmation_url reset_otp_session_data diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index 176e53b5917..eedcd658ef4 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -2,6 +2,7 @@ module OpenidConnect class AuthorizationController < ApplicationController include AccountRecoverable include FullyAuthenticatable + include RememberDeviceConcern include VerifyProfileConcern include VerifySPAttributesConcern @@ -10,9 +11,9 @@ class AuthorizationController < ApplicationController before_action :force_login_if_prompt_param_is_login_and_request_is_external, only: [:index] before_action :store_request, only: [:index] before_action :apply_secure_headers_override, only: [:index] + before_action :confirm_user_is_authenticated_with_fresh_mfa, only: :index def index - return confirm_two_factor_authenticated(request_id) unless user_fully_authenticated? link_identity_to_service_provider return redirect_to account_recovery_setup_url if piv_cac_enabled_but_not_multiple_mfa_enabled? return redirect_to_account_or_verify_profile_url if profile_or_identity_needs_verification? @@ -22,6 +23,11 @@ def index private + def confirm_user_is_authenticated_with_fresh_mfa + return confirm_two_factor_authenticated(request_id) unless user_fully_authenticated? + redirect_to user_two_factor_authentication_url if remember_device_expired_for_sp? + end + def link_identity_to_service_provider @authorize_form.link_identity_to_service_provider(current_user, session.id) end diff --git a/app/controllers/saml_idp_controller.rb b/app/controllers/saml_idp_controller.rb index b900e084fca..ceae80d7630 100644 --- a/app/controllers/saml_idp_controller.rb +++ b/app/controllers/saml_idp_controller.rb @@ -8,14 +8,15 @@ class SamlIdpController < ApplicationController include SamlIdpLogoutConcern include AccountRecoverable include FullyAuthenticatable + include RememberDeviceConcern include VerifyProfileConcern include VerifySPAttributesConcern skip_before_action :verify_authenticity_token before_action :validate_saml_logout_request, only: :logout + before_action :confirm_user_is_authenticated_with_fresh_mfa, only: :auth def auth - return confirm_two_factor_authenticated(request_id) unless user_fully_authenticated? link_identity_from_session_data capture_analytics return redirect_to account_recovery_setup_url if piv_cac_enabled_but_not_multiple_mfa_enabled? @@ -40,6 +41,11 @@ def logout private + def confirm_user_is_authenticated_with_fresh_mfa + return confirm_two_factor_authenticated(request_id) unless user_fully_authenticated? + redirect_to user_two_factor_authentication_url if remember_device_expired_for_sp? + end + def validate_saml_logout_request(raw_saml_request = params[:SAMLRequest]) request_valid = saml_request_valid?(raw_saml_request) 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 9aa44bd3804..f3af110fede 100644 --- a/app/controllers/two_factor_authentication/personal_key_verification_controller.rb +++ b/app/controllers/two_factor_authentication/personal_key_verification_controller.rb @@ -69,6 +69,7 @@ def handle_valid_otp handle_valid_otp_for_authentication_context redirect_to manage_personal_key_url reset_otp_session_data + user_session.delete(:mfa_device_remembered) end end end 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 f55e61da3ab..40161f93c33 100644 --- a/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb +++ b/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb @@ -36,6 +36,7 @@ def handle_valid_piv_cac handle_valid_otp_for_authentication_context redirect_to next_step reset_otp_session_data + user_session.delete(:mfa_device_remembered) end def next_step diff --git a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb index 0d58b3ab58f..a06768c8f17 100644 --- a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb +++ b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb @@ -26,6 +26,7 @@ def handle_valid_webauthn handle_valid_otp_for_authentication_context redirect_to after_otp_verification_confirmation_url reset_otp_session_data + user_session.delete(:mfa_device_remembered) end def handle_invalid_webauthn diff --git a/app/decorators/service_provider_session_decorator.rb b/app/decorators/service_provider_session_decorator.rb index 79356c75600..a8d768ea8a2 100644 --- a/app/decorators/service_provider_session_decorator.rb +++ b/app/decorators/service_provider_session_decorator.rb @@ -119,10 +119,27 @@ def sp_alert_learn_more custom_alert? ? CUSTOM_SP_ALERTS.dig(sp_name, :learn_more) : 'https://login.gov/help/' end + # :reek:DuplicateMethodCall + def mfa_expiration_interval + aal_1_expiration = Figaro.env.remember_device_expiration_hours_aal_1.to_i.hours + aal_2_expiration = Figaro.env.remember_device_expiration_hours_aal_2.to_i.hours + return aal_2_expiration if sp_aal > 1 + return aal_2_expiration if sp_ial > 1 + aal_1_expiration + end + private attr_reader :sp, :view_context, :sp_session, :service_provider_request + def sp_aal + sp.aal || 1 + end + + def sp_ial + sp.ial || 1 + end + def custom_alert? CUSTOM_ALERT_SP_NAMES.include?(sp_name) end diff --git a/app/decorators/session_decorator.rb b/app/decorators/session_decorator.rb index 1490fe2a5a7..aeec4ebd470 100644 --- a/app/decorators/session_decorator.rb +++ b/app/decorators/session_decorator.rb @@ -31,6 +31,10 @@ def cancel_link_url view_context.root_url end + def mfa_expiration_interval + Figaro.env.remember_device_expiration_hours_aal_1.to_i.hours + end + def failure_to_proof_url; end def sp_msg; end diff --git a/app/services/remember_device_cookie.rb b/app/services/remember_device_cookie.rb index 84fabb18674..9fdea974e8a 100644 --- a/app/services/remember_device_cookie.rb +++ b/app/services/remember_device_cookie.rb @@ -32,17 +32,17 @@ def to_json }.to_json end - def valid_for_user?(user) + def valid_for_user?(user:, expiration_interval:) return false if user.id != user_id return false if user_has_changed_phone?(user) - return false if expired? + return false if expired?(expiration_interval) true end private - def expired? - created_at < Figaro.env.remember_device_expiration_days.to_i.days.ago + def expired?(interval) + created_at < interval.ago end def user_has_changed_phone?(user) diff --git a/app/views/two_factor_authentication/otp_verification/show.html.slim b/app/views/two_factor_authentication/otp_verification/show.html.slim index 48cb79737bb..141193beec4 100644 --- a/app/views/two_factor_authentication/otp_verification/show.html.slim +++ b/app/views/two_factor_authentication/otp_verification/show.html.slim @@ -26,7 +26,7 @@ p == @presenter.phone_number_message inline-block.span style="white-space:nowrap;" = check_box_tag 'remember_device', true, false, class: 'my2 ml2 mr1' = label_tag 'remember_device', - t('forms.messages.remember_device', duration: Figaro.env.remember_device_expiration_days), + t('forms.messages.remember_device'), class: 'blue' br - if @presenter.update_phone_link.present? diff --git a/config/application.yml.example b/config/application.yml.example index 633d3cd82ae..91f15b899fb 100644 --- a/config/application.yml.example +++ b/config/application.yml.example @@ -166,7 +166,8 @@ development: recaptcha_secret_key: 'key2' redis_url: 'redis://localhost:6379/0' redis_throttle_url: 'redis://localhost:6379/1' - remember_device_expiration_days: '30' + remember_device_expiration_hours_aal_1: '720' + remember_device_expiration_hours_aal_2: '12' requests_per_ip_limit: '300' requests_per_ip_period: '300' requests_per_ip_track_only_mode: 'false' @@ -287,7 +288,8 @@ production: recaptcha_secret_key: 'key2' redis_url: 'redis://redis.login.gov.internal:6379' redis_throttle_url: 'redis://redis.login.gov.internal:6379/1' - remember_device_expiration_days: '30' + remember_device_expiration_hours_aal_1: '720' + remember_device_expiration_hours_aal_2: '12' requests_per_ip_limit: '300' requests_per_ip_period: '300' requests_per_ip_track_only_mode: 'true' @@ -410,7 +412,8 @@ test: recaptcha_secret_key: 'key2' redis_url: 'redis://localhost:6379/0' redis_throttle_url: 'redis://localhost:6379/1' - remember_device_expiration_days: '30' + remember_device_expiration_hours_aal_1: '720' + remember_device_expiration_hours_aal_2: '12' requests_per_ip_limit: '4' requests_per_ip_period: '60' requests_per_ip_track_only_mode: 'false' diff --git a/config/initializers/figaro.rb b/config/initializers/figaro.rb index 92533f3f513..2247c90acad 100644 --- a/config/initializers/figaro.rb +++ b/config/initializers/figaro.rb @@ -38,7 +38,8 @@ 'requests_per_ip_limit', 'requests_per_ip_period', 'requests_per_ip_track_only_mode', - 'remember_device_expiration_days', + 'remember_device_expiration_hours_aal_1', + 'remember_device_expiration_hours_aal_2', 'saml_passphrase', 'scrypt_cost', 'secret_key_base', diff --git a/config/locales/forms/en.yml b/config/locales/forms/en.yml index 8a7411bed5a..b99b121b5fa 100644 --- a/config/locales/forms/en.yml +++ b/config/locales/forms/en.yml @@ -21,7 +21,7 @@ en: show_hdr: Create a strong password example: 'example:' messages: - remember_device: Remember this browser for %{duration} days + remember_device: Remember this browser passwords: edit: buttons: diff --git a/config/locales/forms/es.yml b/config/locales/forms/es.yml index cca50b57715..bc629b2679e 100644 --- a/config/locales/forms/es.yml +++ b/config/locales/forms/es.yml @@ -21,7 +21,7 @@ es: show_hdr: Crear una contraseña segura example: 'ejemplo:' messages: - remember_device: Recuerde este navegador por %{duration} días + remember_device: Recuerde este navegador passwords: edit: buttons: diff --git a/config/locales/forms/fr.yml b/config/locales/forms/fr.yml index faca7d38bb8..527aac2afc2 100644 --- a/config/locales/forms/fr.yml +++ b/config/locales/forms/fr.yml @@ -21,7 +21,7 @@ fr: show_hdr: Créez un mot de passe fort example: 'exemple:' messages: - remember_device: Enregistrer ce navigateur pendant %{duration} jours + remember_device: Enregistrer ce navigateur passwords: edit: buttons: diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index ae19078e6fe..c734de4d5e7 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -901,6 +901,7 @@ def stub_auth it 'tracks the authentication and IdV redirection event' do stub_analytics stub_auth + allow(controller).to receive(:remember_device_expired_for_sp?).and_return(false) allow(controller).to receive(:identity_needs_verification?).and_return(true) allow(controller).to receive(:saml_request).and_return(FakeSamlRequest.new) allow(controller).to receive(:saml_request_id). diff --git a/spec/decorators/service_provider_session_decorator_spec.rb b/spec/decorators/service_provider_session_decorator_spec.rb index 111b65e3e23..38eb02285e3 100644 --- a/spec/decorators/service_provider_session_decorator_spec.rb +++ b/spec/decorators/service_provider_session_decorator_spec.rb @@ -247,4 +247,26 @@ subject.sp_return_url end end + + describe 'mfa_expiration_interval' do + context 'with an AAL2 sp' do + before do + allow(sp).to receive(:aal).and_return(2) + end + + it { expect(subject.mfa_expiration_interval).to eq(12.hours) } + end + + context 'with an IAL2 sp' do + before do + allow(sp).to receive(:ial).and_return(2) + end + + it { expect(subject.mfa_expiration_interval).to eq(12.hours) } + end + + context 'with an sp that is not AAL2 or IAL2' do + it { expect(subject.mfa_expiration_interval).to eq(30.days) } + end + end end diff --git a/spec/decorators/session_decorator_spec.rb b/spec/decorators/session_decorator_spec.rb index 0e6f8bb07b3..95048ff48c0 100644 --- a/spec/decorators/session_decorator_spec.rb +++ b/spec/decorators/session_decorator_spec.rb @@ -56,4 +56,10 @@ expect(decorator.cancel_link_url).to eq 'http://www.example.com' end end + + describe '#mfa_expiration_interval' do + it 'returns the AAL1 expiration interval' do + expect(subject.mfa_expiration_interval).to eq(30.days) + end + end end diff --git a/spec/features/two_factor_authentication/remember_device_sp_expiration_spec.rb b/spec/features/two_factor_authentication/remember_device_sp_expiration_spec.rb new file mode 100644 index 00000000000..e426091941a --- /dev/null +++ b/spec/features/two_factor_authentication/remember_device_sp_expiration_spec.rb @@ -0,0 +1,128 @@ +require 'rails_helper' + +shared_examples 'expiring remember device for an sp config' do |expiration_time, protocol| + before do + user # Go through the signup flow and remember user before visiting SP + end + + context 'signing in' do + it "does not require MFA before #{expiration_time.inspect}" do + Timecop.travel(expiration_time.from_now - 1.day) do + visit_idp_from_sp_with_loa1(protocol) + sign_in_user(user) + + expect(page).to have_current_path(sign_up_completed_path) + end + end + + it "does require MFA after #{expiration_time.inspect}" do + Timecop.travel(expiration_time.from_now + 1.day) do + visit_idp_from_sp_with_loa1(protocol) + sign_in_user(user) + + expect(page).to have_content(t('two_factor_authentication.header_text')) + expect(current_path).to eq(login_two_factor_path(otp_delivery_preference: :sms)) + + click_submit_default + + expect(page).to have_current_path(sign_up_completed_path) + end + end + end + + context 'visiting while already signed in' do + it "does not require MFA before #{expiration_time.inspect}" do + Timecop.travel(expiration_time.from_now - 1.day) do + sign_in_user(user) + visit_idp_from_sp_with_loa1(protocol) + + expect(page).to have_current_path(sign_up_completed_path) + end + end + + it "does require MFA after #{expiration_time.inspect}" do + Timecop.travel(expiration_time.from_now + 1.day) do + if expiration_time == 30.days + sign_in_live_with_2fa(user) + visit_idp_from_sp_with_loa1(protocol) + else + sign_in_user(user) + visit_idp_from_sp_with_loa1(protocol) + + expect(page).to have_content(t('two_factor_authentication.header_text')) + expect(current_path).to eq(login_two_factor_path(otp_delivery_preference: :sms)) + + click_submit_default + end + + expect(page).to have_current_path(sign_up_completed_path) + end + end + end +end + +feature 'remember device sp expiration' do + include SamlAuthHelper + + let(:user) do + user_record = sign_up_and_set_password + user_record.password = Features::SessionHelper::VALID_PASSWORD + select_2fa_option('sms') + fill_in :user_phone_form_phone, with: '2025551212' + click_send_security_code + check :remember_device + click_submit_default + click_acknowledge_personal_key + first(:link, t('links.sign_out')).click + user_record + end + + before do + allow(FeatureManagement).to receive(:prefill_otp_codes?).and_return(true) + allow(SmsOtpSenderJob).to receive(:perform_now) + allow(Figaro.env).to receive(:otp_delivery_blocklist_maxretry).and_return('1000') + + ServiceProvider.from_issuer('urn:gov:gsa:openidconnect:sp:server').update!( + aal: aal, + ial: ial + ) + ServiceProvider.from_issuer('http://localhost:3000').update!( + aal: aal, + ial: ial + ) + end + + context 'signing into an SP' do + context 'with an AAL2 SP' do + let(:aal) { 2 } + let(:ial) { 1 } + + it_behaves_like 'expiring remember device for an sp config', 12.hours, :oidc + it_behaves_like 'expiring remember device for an sp config', 12.hours, :saml + end + + context 'with an IAL2 SP' do + let(:aal) { 1 } + let(:ial) { 2 } + + it_behaves_like 'expiring remember device for an sp config', 12.hours, :oidc + it_behaves_like 'expiring remember device for an sp config', 12.hours, :saml + end + + context 'with an AAL2 and IAL2 SP' do + let(:aal) { 2 } + let(:ial) { 2 } + + it_behaves_like 'expiring remember device for an sp config', 12.hours, :oidc + it_behaves_like 'expiring remember device for an sp config', 12.hours, :saml + end + + context 'with an AAL1 and IAL1 SP' do + let(:aal) { 1 } + let(:ial) { 1 } + + it_behaves_like 'expiring remember device for an sp config', 30.days, :oidc + it_behaves_like 'expiring remember device for an sp config', 30.days, :saml + end + end +end diff --git a/spec/features/two_factor_authentication/remember_device_spec.rb b/spec/features/two_factor_authentication/remember_device_spec.rb index 06c166dd3e3..fbbdf048ee5 100644 --- a/spec/features/two_factor_authentication/remember_device_spec.rb +++ b/spec/features/two_factor_authentication/remember_device_spec.rb @@ -91,7 +91,7 @@ def remember_device_and_sign_out_user it 'requires 2FA and does not offer the option to remember device' do expect(current_path).to eq(idv_otp_verification_path) expect(page).to_not have_content( - t('forms.messages.remember_device', duration: Figaro.env.remember_device_expiration_days!) + t('forms.messages.remember_device') ) end end @@ -108,7 +108,7 @@ def remember_device_and_sign_out_user sign_in_user(user) expect(current_path).to eq(login_two_factor_authenticator_path) expect(page).to_not have_content( - t('forms.messages.remember_device', duration: Figaro.env.remember_device_expiration_days!) + t('forms.messages.remember_device') ) end end diff --git a/spec/services/remember_device_cookie_spec.rb b/spec/services/remember_device_cookie_spec.rb index d076b462fd7..8b90774bd0b 100644 --- a/spec/services/remember_device_cookie_spec.rb +++ b/spec/services/remember_device_cookie_spec.rb @@ -61,22 +61,32 @@ end end - describe '#valid_for_user?(user)' do + describe '#valid_for?(user:, expiration_interval:)' do + let(:expiration_interval) { 30.days } + + subject do + cookie = described_class.new(user_id: user.id, created_at: created_at) + cookie.valid_for_user?(user: user, expiration_interval: expiration_interval) + end + context 'when the token is valid' do - it { expect(subject.valid_for_user?(user)).to eq(true) } + it { expect(subject).to eq(true) } end context 'when the token is expired' do - let(:created_at) { (Figaro.env.remember_device_expiration_days.to_i + 1).days.ago } + let(:created_at) { (expiration_interval + 1).days.ago } - it { expect(subject.valid_for_user?(user)).to eq(false) } + it { expect(subject).to eq(false) } end context 'when the token does not refer to the current user' do it 'returns false' do other_user = create(:user, :with_phone, with: { confirmed_at: 90.days.ago }) + cookie = described_class.new(user_id: user.id, created_at: created_at) - expect(subject.valid_for_user?(other_user)).to eq(false) + expect( + cookie.valid_for_user?(user: other_user, expiration_interval: expiration_interval) + ).to eq(false) end end @@ -84,7 +94,7 @@ let(:created_at) { 5.days.ago } let(:phone_confirmed_at) { 4.days.ago } - it { expect(subject.valid_for_user?(user)).to eq(false) } + it { expect(subject).to eq(false) } end end end diff --git a/spec/support/controller_helper.rb b/spec/support/controller_helper.rb index 2dd9b7464c4..40a5f1b18ff 100644 --- a/spec/support/controller_helper.rb +++ b/spec/support/controller_helper.rb @@ -21,6 +21,7 @@ def stub_sign_in(user = build(:user, password: VALID_PASSWORD)) allow(controller).to receive(:current_user).and_return(user) allow(controller).to receive(:confirm_two_factor_authenticated).and_return(true) allow(controller).to receive(:user_fully_authenticated?).and_return(true) + allow(controller).to receive(:remember_device_expired_for_sp?).and_return(false) user end diff --git a/spec/support/shared_examples/remember_device.rb b/spec/support/shared_examples/remember_device.rb index dbfa29aafd4..01fee25a868 100644 --- a/spec/support/shared_examples/remember_device.rb +++ b/spec/support/shared_examples/remember_device.rb @@ -9,7 +9,7 @@ it 'requires 2FA on sign in after expiration' do user = remember_device_and_sign_out_user - days_to_travel = (Figaro.env.remember_device_expiration_days.to_i + 1).days.from_now + days_to_travel = (Figaro.env.remember_device_expiration_hours_aal_1.to_i + 1).hours.from_now Timecop.travel days_to_travel do sign_in_user(user)