From 1b12fee7b48bb2d89e18fe441451e05c2610ca16 Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Wed, 14 Feb 2024 12:19:37 -0500 Subject: [PATCH] LG-12261 Stop reading from `sp_session[:ial]` In a previous commit the `resolved_authn_context_result` was introduced to return a `Vot::Parser::Result` object that described the requirements for the current SP request considering SP default options. This is intended to be used to replace the keys in the `sp_session` that serve this purpose including the `ial` key. This commit replaces places where the `sp_session[:ial]` value is read with new reads to the `resolved_authn_context_result`. [skip changelog] --- app/controllers/application_controller.rb | 7 +++-- .../concerns/billable_event_trackable.rb | 2 +- .../concerns/idv_session_concern.rb | 8 +---- .../concerns/verify_sp_attributes_concern.rb | 12 ++++++- .../authorization_controller.rb | 6 ---- app/controllers/saml_idp_controller.rb | 7 ----- .../sign_up/completions_controller.rb | 2 +- app/controllers/users/passwords_controller.rb | 4 +-- .../users/piv_cac_login_controller.rb | 11 +------ .../concerns/billable_event_trackable_spec.rb | 4 +-- spec/controllers/idv_controller_spec.rb | 8 ++--- .../sign_up/completions_controller_spec.rb | 19 +++++++++--- .../users/piv_cac_login_controller_spec.rb | 31 ++++++++++++++++--- 13 files changed, 69 insertions(+), 52 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 43c77f0d1d8..9ef280afe4c 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -387,8 +387,11 @@ def set_locale I18n.locale = LocaleChooser.new(params[:locale], request).locale end - def sp_session_ial - sp_session[:ial].presence || 1 + def pii_requested_but_locked? + if resolved_authn_context_result.identity_proofing? || resolved_authn_context_result.ialmax? + current_user.identity_verified? && + !Pii::Cacher.new(current_user, user_session).exists_in_session? + end end def mfa_policy diff --git a/app/controllers/concerns/billable_event_trackable.rb b/app/controllers/concerns/billable_event_trackable.rb index 0c9784d0fde..13ac781b6f5 100644 --- a/app/controllers/concerns/billable_event_trackable.rb +++ b/app/controllers/concerns/billable_event_trackable.rb @@ -41,7 +41,7 @@ def mark_current_session_billed def session_has_been_billed_flag_key issuer = sp_session[:issuer] - if sp_session_ial == 1 + if !resolved_authn_context_result.identity_proofing? "auth_counted_#{issuer}ial1" else "auth_counted_#{issuer}" diff --git a/app/controllers/concerns/idv_session_concern.rb b/app/controllers/concerns/idv_session_concern.rb index 08f16cfb9a3..44dd58fd60b 100644 --- a/app/controllers/concerns/idv_session_concern.rb +++ b/app/controllers/concerns/idv_session_concern.rb @@ -53,13 +53,7 @@ def redirect_unless_idv_session_user def redirect_unless_sp_requested_verification return if !IdentityConfig.store.idv_sp_required return if idv_session_user.profiles.any? - - ial_context = IalContext.new( - ial: sp_session_ial, - service_provider: sp_from_sp_session, - user: idv_session_user, - ) - return if ial_context.ial2_or_greater? + return if resolved_authn_context_result.identity_proofing? redirect_to account_url end diff --git a/app/controllers/concerns/verify_sp_attributes_concern.rb b/app/controllers/concerns/verify_sp_attributes_concern.rb index 6a168bb07b8..d47dbedd138 100644 --- a/app/controllers/concerns/verify_sp_attributes_concern.rb +++ b/app/controllers/concerns/verify_sp_attributes_concern.rb @@ -22,7 +22,7 @@ def update_verified_attributes current_user, current_sp, ).link_identity( - ial: sp_session_ial, + ial: linked_identity_ial, verified_attributes: sp_session[:requested_attributes], last_consented_at: Time.zone.now, clear_deleted_at: true, @@ -62,6 +62,16 @@ def verified_after_consent?(last_estimated_consent) verification_timestamp.present? && last_estimated_consent < verification_timestamp end + def linked_identity_ial + if resolved_authn_context_result.ialmax? + 0 + elsif resolved_authn_context_result.identity_proofing? + 2 + else + 1 + end + end + def find_sp_session_identity current_user&.identities&.find_by(service_provider: sp_session[:issuer]) end diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index a1171123193..2fb4d9de2ce 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -189,12 +189,6 @@ def store_request ).call end - def pii_requested_but_locked? - sp_session && sp_session_ial > 1 && - current_user.identity_verified? && - !Pii::Cacher.new(current_user, user_session).exists_in_session? - end - def track_events event_ial_context = IalContext.new( ial: @authorize_form.ial, diff --git a/app/controllers/saml_idp_controller.rb b/app/controllers/saml_idp_controller.rb index 18ddd1488f3..d3265294ac8 100644 --- a/app/controllers/saml_idp_controller.rb +++ b/app/controllers/saml_idp_controller.rb @@ -113,13 +113,6 @@ def set_devise_failure_redirect_for_concurrent_session_logout request.env['devise_session_limited_failure_redirect_url'] = request.url end - def pii_requested_but_locked? - if resolved_authn_context_result.identity_proofing_or_ialmax? - current_user.identity_verified? && - !Pii::Cacher.new(current_user, user_session).exists_in_session? - end - end - def capture_analytics analytics_payload = result.to_h.merge( endpoint: api_saml_auth_path(path_year: params[:path_year]), diff --git a/app/controllers/sign_up/completions_controller.rb b/app/controllers/sign_up/completions_controller.rb index 912a7ac01a7..95c5cc5fb53 100644 --- a/app/controllers/sign_up/completions_controller.rb +++ b/app/controllers/sign_up/completions_controller.rb @@ -126,7 +126,7 @@ def pii end def send_in_person_completion_survey - return unless sp_session_ial == ::Idp::Constants::IAL2 + return unless resolved_authn_context_result.identity_proofing? Idv::InPerson::CompletionSurveySender.send_completion_survey( current_user, diff --git a/app/controllers/users/passwords_controller.rb b/app/controllers/users/passwords_controller.rb index 15cfcfcca77..ae838e6078e 100644 --- a/app/controllers/users/passwords_controller.rb +++ b/app/controllers/users/passwords_controller.rb @@ -3,7 +3,7 @@ class PasswordsController < ApplicationController include ReauthenticationRequiredConcern before_action :confirm_two_factor_authenticated - before_action :capture_password_if_pii_requested_but_locked + before_action :capture_password_if_pii_present_but_locked before_action :confirm_recently_authenticated_2fa def edit @@ -33,7 +33,7 @@ def update private - def capture_password_if_pii_requested_but_locked + def capture_password_if_pii_present_but_locked return unless current_user.identity_verified? && !Pii::Cacher.new(current_user, user_session).exists_in_session? user_session[:stored_location] = request.url diff --git a/app/controllers/users/piv_cac_login_controller.rb b/app/controllers/users/piv_cac_login_controller.rb index d30ba4f1787..7f627d58b9c 100644 --- a/app/controllers/users/piv_cac_login_controller.rb +++ b/app/controllers/users/piv_cac_login_controller.rb @@ -79,8 +79,7 @@ def process_valid_submission end def next_step - if ial_context.ial2_requested? && current_user.identity_verified? && - !Pii::Cacher.new(current_user, user_session).exists_in_session? + if pii_requested_but_locked? capture_password_url elsif !current_user.accepted_rules_of_use_still_valid? rules_of_use_path @@ -89,14 +88,6 @@ def next_step end end - def ial_context - @ial_context ||= IalContext.new( - ial: sp_session_ial, - service_provider: current_sp, - user: piv_cac_login_form.user, - ) - end - def process_invalid_submission session[:needs_to_setup_piv_cac_after_sign_in] = true if piv_cac_login_form.valid_token? diff --git a/spec/controllers/concerns/billable_event_trackable_spec.rb b/spec/controllers/concerns/billable_event_trackable_spec.rb index f8cc8f9bc4b..a7ee097981e 100644 --- a/spec/controllers/concerns/billable_event_trackable_spec.rb +++ b/spec/controllers/concerns/billable_event_trackable_spec.rb @@ -9,7 +9,7 @@ :request_id, :user_session, :sp_session, - :sp_session_ial, + :resolved_authn_context_result, :session, ) do include BillableEventTrackable @@ -33,7 +33,7 @@ current_user:, request_id:, user_session: {}, - sp_session_ial: 1, + resolved_authn_context_result: double(identity_proofing?: false), sp_session: { issuer: current_sp.issuer, }, diff --git a/spec/controllers/idv_controller_spec.rb b/spec/controllers/idv_controller_spec.rb index 03cf1467c77..ca92d38cd8c 100644 --- a/spec/controllers/idv_controller_spec.rb +++ b/spec/controllers/idv_controller_spec.rb @@ -194,13 +194,13 @@ describe 'SP for IdV requirement' do let(:current_sp) { create(:service_provider) } - let(:ial) { 2 } + let(:acr_values) { Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF } let(:user) { build(:user, password: ControllerHelper::VALID_PASSWORD) } before do stub_sign_in(user) if current_sp.present? - session[:sp] = { issuer: current_sp.issuer, ial: ial } + session[:sp] = { issuer: current_sp.issuer, acr_values: acr_values } else session[:sp] = {} end @@ -237,7 +237,7 @@ end context 'with an SP context that does not require IdV' do - let(:ial) { 1 } + let(:acr_values) { Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF } context 'when an SP is required' do let(:idv_sp_required) { true } @@ -266,7 +266,7 @@ end context 'with an SP context that requires IdV' do - let(:ial) { 2 } + let(:acr_values) { Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF } context 'when an SP is required' do let(:idv_sp_required) { true } diff --git a/spec/controllers/sign_up/completions_controller_spec.rb b/spec/controllers/sign_up/completions_controller_spec.rb index bc6fe2fc3f4..1e818a35233 100644 --- a/spec/controllers/sign_up/completions_controller_spec.rb +++ b/spec/controllers/sign_up/completions_controller_spec.rb @@ -17,6 +17,7 @@ stub_sign_in(user) subject.session[:sp] = { issuer: current_sp.issuer, + acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, } get :show @@ -31,6 +32,7 @@ stub_sign_in(user) subject.session[:sp] = { issuer: current_sp.issuer, + acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, ial2: false, requested_attributes: [:email], request_url: 'http://localhost:3000', @@ -67,6 +69,7 @@ stub_sign_in(user) subject.session[:sp] = { issuer: current_sp.issuer, + acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, ial2: true, requested_attributes: [:email], request_url: 'http://localhost:3000', @@ -141,6 +144,7 @@ stub_sign_in(user) subject.session[:sp] = { issuer: current_sp.issuer, + acr_values: Saml::Idp::Constants::IALMAX_AUTHN_CONTEXT_CLASSREF, ial2: false, ialmax: true, requested_attributes: [:email], @@ -220,6 +224,7 @@ stub_sign_in(user) subject.session[:sp] = { issuer: sp.issuer, ial2: false, + acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, requested_attributes: [:email], request_url: 'http://localhost:3000' } get :show @@ -245,6 +250,7 @@ it 'tracks analytics' do stub_sign_in(user) subject.session[:sp] = { + acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, ial2: false, issuer: 'foo', request_url: 'http://example.com', @@ -271,7 +277,7 @@ stub_sign_in(user) subject.session[:sp] = { issuer: 'foo', - ial: 1, + acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, request_url: 'http://example.com', requested_attributes: ['email'], } @@ -290,6 +296,7 @@ it 'redirects to account page if the session request_url is removed' do stub_sign_in(user) subject.session[:sp] = { + acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, ial2: false, issuer: 'foo', requested_attributes: ['email'], @@ -306,6 +313,7 @@ DisposableEmailDomain.create(name: 'temporary.com') stub_sign_in(user) subject.session[:sp] = { + acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, ial2: false, issuer: 'foo', request_url: 'http://example.com', @@ -343,6 +351,7 @@ sp = create(:service_provider, issuer: 'https://awesome') subject.session[:sp] = { issuer: sp.issuer, + acr_values: Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF, ial2: true, request_url: 'http://example.com', requested_attributes: ['email'], @@ -371,7 +380,7 @@ sp = create(:service_provider, issuer: 'https://awesome') subject.session[:sp] = { issuer: sp.issuer, - ial: 2, + acr_values: Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF, request_url: 'http://example.com', requested_attributes: %w[email first_name verified_at], } @@ -395,12 +404,11 @@ sp = create(:service_provider, issuer: 'https://awesome') subject.session[:sp] = { issuer: sp.issuer, - ial: 2, + acr_values: Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF, request_url: 'http://example.com', requested_attributes: %w[email first_name verified_at], } allow(@linker).to receive(:link_identity).with( - ial: 2, verified_attributes: %w[email first_name verified_at], last_consented_at: now, clear_deleted_at: true, @@ -428,6 +436,7 @@ ial2: false, issuer: 'foo', request_url: 'http://example.com', + acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, } patch :update end @@ -441,6 +450,7 @@ ial2: false, issuer: 'foo', request_url: 'http://example.com', + acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, } expect(original_profile.activated_at).to be_present @@ -468,6 +478,7 @@ subject.session[:sp] = { ial2: false, request_url: 'http://example.com', + acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, } patch :update diff --git a/spec/controllers/users/piv_cac_login_controller_spec.rb b/spec/controllers/users/piv_cac_login_controller_spec.rb index 3210a464570..220184e2027 100644 --- a/spec/controllers/users/piv_cac_login_controller_spec.rb +++ b/spec/controllers/users/piv_cac_login_controller_spec.rb @@ -44,7 +44,12 @@ context 'with a valid token' do let(:service_provider) { create(:service_provider) } - let(:sp_session) { { ial: 1, issuer: service_provider.issuer } } + let(:sp_session) do + { + acr_values: Saml::Idp::Constants::AAL2_AUTHN_CONTEXT_CLASSREF, + issuer: service_provider.issuer, + } + end let(:nonce) { SecureRandom.base64(20) } let(:data) do { @@ -175,7 +180,12 @@ end context 'ial2 service_level' do - let(:sp_session) { { ial: Idp::Constants::IAL2, issuer: service_provider.issuer } } + let(:sp_session) do + { + acr_values: Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF, + issuer: service_provider.issuer, + } + end it 'redirects to account' do expect(response).to redirect_to(account_url) @@ -184,7 +194,10 @@ context 'ial_max service level' do let(:sp_session) do - { ial: Idp::Constants::IAL_MAX, issuer: service_provider.issuer } + { + acr_values: Saml::Idp::Constants::IALMAX_AUTHN_CONTEXT_CLASSREF, + issuer: service_provider.issuer, + } end it 'redirects to the after_sign_in_path_for' do @@ -203,7 +216,12 @@ end context 'ial2 service_level' do - let(:sp_session) { { ial: Idp::Constants::IAL2, issuer: service_provider.issuer } } + let(:sp_session) do + { + acr_values: Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF, + issuer: service_provider.issuer, + } + end it 'redirects to the capture_password_url' do expect(response).to redirect_to(capture_password_url) @@ -212,7 +230,10 @@ context 'ial_max service_level' do let(:sp_session) do - { ial: Idp::Constants::IAL_MAX, issuer: service_provider.issuer } + { + acr_values: Saml::Idp::Constants::IALMAX_AUTHN_CONTEXT_CLASSREF, + issuer: service_provider.issuer, + } end it 'redirects to the capture_password_url' do