From 6e8b29e88b4d27f1294a4fa4cf127493cadf921f Mon Sep 17 00:00:00 2001 From: Vraj Mohan Date: Tue, 3 Sep 2024 14:10:50 -0700 Subject: [PATCH] Perform ial and aal determination in the idp app (#11160) This was unnecessarily leaking into the saml_idp gem. A separate PR (https://github.com/18F/saml_idp/pull/117) in that repo will remove these concerns from there. See https://gitlab.login.gov/lg-people/Melba/backlog-fy24/-/issues/95 changelog: Internal, Refactoring, Refactor saml_request.requested_ial_authn_context calls to single place --- .../concerns/saml_idp_auth_concern.rb | 6 +- app/controllers/saml_idp_controller.rb | 14 ++-- app/models/federated_protocols/oidc.rb | 4 -- app/models/federated_protocols/saml.rb | 24 ++++--- app/services/analytics_events.rb | 4 +- app/services/attribute_asserter.rb | 16 +++-- spec/features/idv/analytics_spec.rb | 3 - .../idv/hybrid_mobile/hybrid_mobile_spec.rb | 4 -- spec/models/federated_protocols/saml_spec.rb | 68 +++++++++++++++++++ 9 files changed, 105 insertions(+), 38 deletions(-) create mode 100644 spec/models/federated_protocols/saml_spec.rb diff --git a/app/controllers/concerns/saml_idp_auth_concern.rb b/app/controllers/concerns/saml_idp_auth_concern.rb index 1a055c5cac2..cc87d04087a 100644 --- a/app/controllers/concerns/saml_idp_auth_concern.rb +++ b/app/controllers/concerns/saml_idp_auth_concern.rb @@ -125,15 +125,11 @@ def response_authn_context if saml_request.requested_vtr_authn_contexts.present? resolved_authn_context_result.expanded_component_values else - saml_request.requested_aal_authn_context || + FederatedProtocols::Saml.new(saml_request).aal || default_aal_context end end - def requested_ial_authn_context - saml_request.requested_ial_authn_context || default_ial_context - end - def link_identity_from_session_data IdentityLinker. new(current_user, saml_request_service_provider). diff --git a/app/controllers/saml_idp_controller.rb b/app/controllers/saml_idp_controller.rb index 7f953a9b418..362106b8e35 100644 --- a/app/controllers/saml_idp_controller.rb +++ b/app/controllers/saml_idp_controller.rb @@ -146,7 +146,7 @@ def log_external_saml_auth_request analytics.saml_auth_request( requested_ial: requested_ial, authn_context: saml_request&.requested_authn_contexts, - requested_aal_authn_context: saml_request&.requested_aal_authn_context, + requested_aal_authn_context: FederatedProtocols::Saml.new(saml_request).aal, requested_vtr_authn_contexts: saml_request&.requested_vtr_authn_contexts.presence, force_authn: saml_request&.force_authn?, final_auth_request: sp_session[:final_auth_request], @@ -156,11 +156,13 @@ def log_external_saml_auth_request end def requested_ial - requested_ial_acr = FederatedProtocols::Saml.new(saml_request).ial - requested_ial_component = Vot::AcrComponentValues.by_name[requested_ial_acr] - return 'ialmax' if requested_ial_component&.requirements&.include?(:ialmax) - - saml_request&.requested_ial_authn_context || 'none' + saml_protocol = FederatedProtocols::Saml.new(saml_request) + requested_ial_acr = saml_protocol.ial + if requested_ial_acr == ::Saml::Idp::Constants::IALMAX_AUTHN_CONTEXT_CLASSREF + return 'ialmax' + else + saml_protocol.requested_ial_authn_context.presence || 'none' + end end def handle_successful_handoff diff --git a/app/models/federated_protocols/oidc.rb b/app/models/federated_protocols/oidc.rb index 60673b75728..9364cc38f5f 100644 --- a/app/models/federated_protocols/oidc.rb +++ b/app/models/federated_protocols/oidc.rb @@ -30,10 +30,6 @@ def requested_attributes OpenidConnectAttributeScoper.new(request.scope).requested_attributes end - def biometric_comparison_required? - request.biometric_comparison_required? - end - def service_provider request.service_provider end diff --git a/app/models/federated_protocols/saml.rb b/app/models/federated_protocols/saml.rb index 997e79dd048..a87face7dea 100644 --- a/app/models/federated_protocols/saml.rb +++ b/app/models/federated_protocols/saml.rb @@ -2,6 +2,10 @@ module FederatedProtocols class Saml + IAL_PREFIX = %r{^http://idmanagement.gov/ns/assurance/ial} + LOA_PREFIX = %r{^http://idmanagement.gov/ns/assurance/loa} + AAL_PREFIX = %r{^http://idmanagement.gov/ns/assurance/aal|urn:gov:gsa:ac:classes:sp:PasswordProtectedTransport:duo} + def initialize(request) @request = request end @@ -14,12 +18,20 @@ def ial if ialmax_requested_with_authn_context_comparison? ::Saml::Idp::Constants::IALMAX_AUTHN_CONTEXT_CLASSREF else - request.requested_ial_authn_context || default_authn_context + requested_ial_authn_context || default_ial_authn_context + end + end + + def requested_ial_authn_context + request.requested_authn_contexts.find do |classref| + IAL_PREFIX.match?(classref) || LOA_PREFIX.match?(classref) end end def aal - request.requested_aal_authn_context + request.requested_authn_contexts.find do |classref| + AAL_PREFIX.match?(classref) + end end def acr_values @@ -43,15 +55,11 @@ def service_provider current_service_provider end - def biometric_comparison_required? - false - end - private attr_reader :request - def default_authn_context + def default_ial_authn_context if current_service_provider&.ial ::Saml::Idp::Constants::AUTHN_CONTEXT_IAL_TO_CLASSREF[current_service_provider.ial] else @@ -81,7 +89,7 @@ def current_service_provider def ialmax_requested_with_authn_context_comparison? return unless (current_service_provider&.ial || 1) > 1 - acr_component_value = Vot::AcrComponentValues.by_name[request.requested_ial_authn_context] + acr_component_value = Vot::AcrComponentValues.by_name[requested_ial_authn_context] return unless acr_component_value.present? !acr_component_value.requirements.include?(:identity_proofing) && diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 40ff7c31086..5a537124665 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -5873,7 +5873,7 @@ def rules_of_use_visit # @param [String] endpoint # @param [Boolean] idv # @param [Boolean] finish_profile - # @param [Integer] requested_ial + # @param [String] requested_ial # @param [Boolean] request_signed # @param [String] matching_cert_serial # matches the request certificate in a successful, signed request @@ -5918,7 +5918,7 @@ def saml_auth( ) end - # @param [Integer] requested_ial + # @param [String] requested_ial # @param [Array] authn_context # @param [String, nil] requested_aal_authn_context # @param [String, nil] requested_vtr_authn_contexts diff --git a/app/services/attribute_asserter.rb b/app/services/attribute_asserter.rb index 125a720d3be..0707bb0cca3 100644 --- a/app/services/attribute_asserter.rb +++ b/app/services/attribute_asserter.rb @@ -42,7 +42,7 @@ def build add_vot(attrs) else add_aal(attrs) - add_ial(attrs) if authn_request.requested_ial_authn_context || !service_provider.ial.nil? + add_ial(attrs) end add_x509(attrs) if bundle.include?(:x509_presented) && x509_data @@ -145,7 +145,7 @@ def add_vot(attrs) end def add_aal(attrs) - requested_context = authn_request.requested_aal_authn_context + requested_context = requested_aal_authn_context requested_aal_level = Saml::Idp::Constants::AUTHN_CONTEXT_CLASSREF_TO_AAL[requested_context] aal_level = requested_aal_level || service_provider.default_aal || ::Idp::Constants::DEFAULT_AAL context = Saml::Idp::Constants::AUTHN_CONTEXT_AAL_TO_CLASSREF[aal_level] @@ -220,12 +220,16 @@ def bundle ).map(&:to_sym) end - def authn_request_bundle - SamlRequestParser.new(authn_request).requested_attributes + def requested_ial_authn_context + FederatedProtocols::Saml.new(authn_request).requested_ial_authn_context + end + + def requested_aal_authn_context + FederatedProtocols::Saml.new(authn_request).aal end - def authn_context - authn_request.requested_ial_authn_context + def authn_request_bundle + SamlRequestParser.new(authn_request).requested_attributes end def x509_data diff --git a/spec/features/idv/analytics_spec.rb b/spec/features/idv/analytics_spec.rb index 2f82fd48333..0fcaa24c1e7 100644 --- a/spec/features/idv/analytics_spec.rb +++ b/spec/features/idv/analytics_spec.rb @@ -981,9 +981,6 @@ def wait_for_event(event, wait) context 'Happy selfie path' do before do - allow_any_instance_of(FederatedProtocols::Oidc). - to receive(:biometric_comparison_required?). - and_return(true) allow_any_instance_of(DocAuth::Response).to receive(:selfie_status).and_return(:success) perform_in_browser(:desktop) do diff --git a/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb b/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb index 198433f9887..7ee08561b25 100644 --- a/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb +++ b/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb @@ -362,10 +362,6 @@ end context 'barcode read error on desktop, redo document capture on mobile' do - before do - allow_any_instance_of(FederatedProtocols::Oidc). - to receive(:biometric_comparison_required?).and_return(true) - end it 'continues to ssn on desktop when user selects Continue', js: true do user = nil diff --git a/spec/models/federated_protocols/saml_spec.rb b/spec/models/federated_protocols/saml_spec.rb new file mode 100644 index 00000000000..a365a5d737d --- /dev/null +++ b/spec/models/federated_protocols/saml_spec.rb @@ -0,0 +1,68 @@ +require 'rails_helper' + +module FederatedProtocols + RSpec.describe Saml do + let(:authn_contexts) { [] } + let(:saml_request) { instance_double(SamlIdp::Request) } + + subject { FederatedProtocols::Saml.new saml_request } + + before do + allow(saml_request).to receive(:requested_authn_contexts).and_return(authn_contexts) + end + + describe '#aal' do + context 'when no aal context is requested' do + it 'returns nil' do + expect(subject.aal).to be_nil + end + end + + context 'when the only context requested is aal' do + let(:aal) { 'http://idmanagement.gov/ns/assurance/aal/2' } + let(:authn_contexts) { [aal] } + + it 'returns the requested aal' do + expect(subject.aal).to eq(aal) + end + end + + context 'when multiple contexts are requested including aal' do + let(:aal) { 'http://idmanagement.gov/ns/assurance/aal/2' } + let(:ial) { 'http://idmanagement.gov/ns/assurance/ial/1' } + let(:authn_contexts) { [ial, aal] } + + it 'returns the requested aal' do + expect(subject.aal).to eq(aal) + end + end + end + + describe '#requested_ial_authn_context' do + context 'when no ial context is requested' do + it 'returns nil' do + expect(subject.requested_ial_authn_context).to be_nil + end + end + + context 'when the only context requested is ial' do + let(:ial) { 'http://idmanagement.gov/ns/assurance/ial/2' } + let(:authn_contexts) { [ial] } + + it 'returns the requested ial' do + expect(subject.requested_ial_authn_context).to eq(ial) + end + end + + context 'when multiple contexts are requested including ial' do + let(:aal) { 'http://idmanagement.gov/ns/assurance/aal/2' } + let(:ial) { 'http://idmanagement.gov/ns/assurance/ial/1' } + let(:authn_contexts) { [ial, aal] } + + it 'returns the requested ial' do + expect(subject.requested_ial_authn_context).to eq(ial) + end + end + end + end +end