Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
28 changes: 25 additions & 3 deletions app/controllers/concerns/remember_device_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
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.

This was changed since now handle_valid_otp removes the mfa_device_remembered flag.

end

def remember_device_cookie
Expand All @@ -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
5 changes: 4 additions & 1 deletion app/controllers/concerns/two_factor_authenticatable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
8 changes: 7 additions & 1 deletion app/controllers/openid_connect/authorization_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ module OpenidConnect
class AuthorizationController < ApplicationController
include AccountRecoverable
include FullyAuthenticatable
include RememberDeviceConcern
include VerifyProfileConcern
include VerifySPAttributesConcern

Expand All @@ -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?
Expand All @@ -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
Expand Down
8 changes: 7 additions & 1 deletion app/controllers/saml_idp_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand All @@ -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)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
17 changes: 17 additions & 0 deletions app/decorators/service_provider_session_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions app/decorators/session_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions app/services/remember_device_cookie.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand Down
9 changes: 6 additions & 3 deletions config/application.yml.example
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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'
Expand Down Expand Up @@ -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'
Expand Down
3 changes: 2 additions & 1 deletion config/initializers/figaro.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
2 changes: 1 addition & 1 deletion config/locales/forms/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion config/locales/forms/es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion config/locales/forms/fr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
1 change: 1 addition & 0 deletions spec/controllers/saml_idp_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
22 changes: 22 additions & 0 deletions spec/decorators/service_provider_session_decorator_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
6 changes: 6 additions & 0 deletions spec/decorators/session_decorator_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading