From a9f39b5cce160bebe1846787cfa633af2906c51a Mon Sep 17 00:00:00 2001 From: davida marion Date: Wed, 16 Oct 2024 10:02:24 -0400 Subject: [PATCH 01/12] Update saml_request_validator to allow unknown authncontexts --- app/services/saml_request_validator.rb | 5 +- spec/services/saml_request_validator_spec.rb | 97 ++++++++++++++------ 2 files changed, 75 insertions(+), 27 deletions(-) diff --git a/app/services/saml_request_validator.rb b/app/services/saml_request_validator.rb index 740d67a5f60..44826eb879c 100644 --- a/app/services/saml_request_validator.rb +++ b/app/services/saml_request_validator.rb @@ -88,7 +88,10 @@ def valid_authn_context? next true if classref.match?(SamlIdp::Request::VTR_REGEXP) && IdentityConfig.store.use_vot_in_sp_requests end - authn_contexts.all? do |classref| + # SAML requests are allowed to "default" to the integration's IAL default. + return true if authn_contexts.empty? + + authn_contexts.any? do |classref| valid_contexts.include?(classref) end end diff --git a/spec/services/saml_request_validator_spec.rb b/spec/services/saml_request_validator_spec.rb index c675f3a0b43..120c1264d67 100644 --- a/spec/services/saml_request_validator_spec.rb +++ b/spec/services/saml_request_validator_spec.rb @@ -2,7 +2,8 @@ RSpec.describe SamlRequestValidator do describe '#call' do - let(:sp) { ServiceProvider.find_by(issuer: 'http://localhost:3000') } + let(:issuer) { 'http://localhost:3000' } + let(:sp) { ServiceProvider.find_by(issuer:) } let(:name_id_format) { Saml::Idp::Constants::NAME_ID_FORMAT_PERSISTENT } let(:authn_context) { [Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF] } let(:comparison) { 'exact' } @@ -31,15 +32,27 @@ ).and_return( use_vot_in_sp_requests, ) + allow(IdentityConfig.store).to receive( + :allowed_biometric_ial_providers + ).and_return([issuer]) + allow(IdentityConfig.store).to receive( + :allowed_valid_authn_contexts_semantic_providers, + ).and_return([issuer]) end context 'valid authn context and sp and authorized nameID format' do - it 'returns FormResponse with success: true' do - expect(response.to_h).to include( - success: true, - errors: {}, - **extra, - ) + [ + Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, + Saml::Idp::Constants::IAL_AUTH_ONLY_ACR, + ].each do |ial_value| + let(:authn_context) { [ial_value] } + it 'returns FormResponse with success: true' do + expect(response.to_h).to include( + success: true, + errors: {}, + **extra, + ) + end end context 'ialmax authncontext and ialmax provider' do @@ -59,6 +72,17 @@ end end + context 'no authn context and sp and authorized nameID format' do + let(:authn_context) { [] } + it 'returns FormResponse with success: true' do + expect(response.to_h).to include( + success: true, + errors: {}, + **extra, + ) + end + end + context 'valid authn context and invalid sp and authorized nameID format' do let(:sp) { ServiceProvider.find_by(issuer: 'foo') } @@ -180,8 +204,8 @@ end end - context 'invalid authn context and valid sp and authorized nameID format' do - context 'unknown auth context' do + context 'unknown context and valid sp and authorized nameID format' do + context 'only the unknown authn_context is requested' do let(:authn_context) { ['IAL1'] } it 'returns FormResponse with success: false' do @@ -196,22 +220,39 @@ **extra, ) end - end - context 'authn context is ial2 when sp is ial 1' do - let(:authn_context) { [Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF] } + context 'unknwn authn_context requested along with a valid one' do + let(:authn_context) { ['IAL1', Saml::Idp::Constants::IAL_AUTH_ONLY_ACR] } - it 'returns FormResponse with success: false' do - errors = { - authn_context: [t('errors.messages.unauthorized_authn_context')], - } + it 'returns FormResponse with success: true' do + expect(response.to_h).to include( + success: true, + errors: {}, + **extra, + ) + end + end + end - expect(response.to_h).to include( - success: false, - errors: errors, - error_details: hash_including(*errors.keys), - **extra, - ) + context 'authn context is ial2 when sp is ial 1' do + [ + Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF, + Saml::Idp::Constants::IAL_VERIFIED_ACR, + ].each do |ial_value| + let(:authn_context) { [ial_value] } + + it 'returns FormResponse with success: false' do + errors = { + authn_context: [t('errors.messages.unauthorized_authn_context')], + } + + expect(response.to_h).to include( + success: false, + errors: errors, + error_details: hash_including(*errors.keys), + **extra, + ) + end end end @@ -237,9 +278,8 @@ context "when the IAL requested is #{facial_match_ial}" do context 'when the service provider is allowed to use facial match ials' do - let(:sp) { create(:service_provider, :idv) } - before do + sp.update(ial: 2) allow_any_instance_of(ServiceProvider).to receive(:facial_match_ial_allowed?). and_return(true) end @@ -281,14 +321,19 @@ it_behaves_like 'allows facial match IAL only if sp is authorized', Saml::Idp::Constants::IAL2_BIO_PREFERRED_AUTHN_CONTEXT_CLASSREF + it_behaves_like 'allows facial match IAL only if sp is authorized', + Saml::Idp::Constants::IAL_VERIFIED_FACIAL_MATCH_PREFERRED_ACR + + it_behaves_like 'allows facial match IAL only if sp is authorized', + Saml::Idp::Constants::IAL_VERIFIED_FACIAL_MATCH_REQUIRED_ACR + shared_examples 'allows semantic IAL only if sp is authorized' do |semantic_ial| let(:authn_context) { [semantic_ial] } context "when the IAL requested is #{semantic_ial}" do context 'when the service provider is allowed to use semantic ials' do - let(:sp) { create(:service_provider, :idv) } - before do + sp.update(ial: 2) allow_any_instance_of(ServiceProvider). to receive(:semantic_authn_contexts_allowed?). and_return(true) From 834154b5b3702b0e9cb351ce00e6bd6ed00a437b Mon Sep 17 00:00:00 2001 From: davida marion Date: Wed, 16 Oct 2024 12:22:03 -0400 Subject: [PATCH 02/12] Update parser to allow/ignore unknown contexts --- app/services/vot/parser.rb | 15 +- .../application_controller_spec.rb | 14 ++ .../openid_connect_authorize_form_spec.rb | 148 +++++++++++++----- spec/services/vot/parser_spec.rb | 99 ++++++++++-- 4 files changed, 211 insertions(+), 65 deletions(-) diff --git a/app/services/vot/parser.rb b/app/services/vot/parser.rb index 3f135fad867..4b768f3ab7b 100644 --- a/app/services/vot/parser.rb +++ b/app/services/vot/parser.rb @@ -4,8 +4,6 @@ module Vot class Parser class ParseException < StandardError; end - class UnsupportedComponentsException < ParseException; end - class DuplicateComponentsException < ParseException; end Result = Data.define( @@ -87,8 +85,7 @@ def initial_components @initial_components ||= component_string.split(component_separator).map do |component_name| component_map.fetch(component_name) rescue KeyError - raise_unsupported_component_exception(component_name) - end + end.compact end def component_separator @@ -113,16 +110,6 @@ def validate_component_uniqueness!(component_values) end end - def raise_unsupported_component_exception(component_value_name) - if vector_of_trust.present? - raise UnsupportedComponentsException, - "'#{vector_of_trust}' contains unknown component '#{component_value_name}'" - else - raise UnsupportedComponentsException, - "'#{acr_values}' contains unknown acr value '#{component_value_name}'" - end - end - def raise_duplicate_component_exception if vector_of_trust.present? raise DuplicateComponentsException, "'#{vector_of_trust}' contains duplicate components" diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index be7775ef85d..77d69e7d2a8 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -486,6 +486,20 @@ def index expect(result.identity_proofing?).to eq(true) end + context 'when an unknow acr value is passed in' do + let(:acr_values) do + [ + 'http://idmanagement.gov/ns/assurance/aal/1', + 'unknown-acr-value', + ].join(' ') + end + + it 'returns a resolved authn context result' do + expect(result.aal2?).to eq(true) + expect(result.identity_proofing?).to eq(true) + end + end + context 'without an SP' do let(:sp) { nil } let(:sp_session) { nil } diff --git a/spec/forms/openid_connect_authorize_form_spec.rb b/spec/forms/openid_connect_authorize_form_spec.rb index 6fb26fda4d7..a830865af87 100644 --- a/spec/forms/openid_connect_authorize_form_spec.rb +++ b/spec/forms/openid_connect_authorize_form_spec.rb @@ -18,7 +18,7 @@ ) end - let(:acr_values) { nil } + let(:acr_values) { nil } let(:vtr) { ['C1'].to_json } let(:client_id) { 'urn:gov:gsa:openidconnect:test' } let(:nonce) { SecureRandom.hex } @@ -187,6 +187,36 @@ end end + context 'with unknown acr_values' do + let(:acr_values) { 'unknown-value' } + let(:vtr) { nil } + + it 'has errors' do + expect(valid?).to eq(false) + expect(form.errors[:acr_values]). + to include(t('openid_connect.authorization.errors.no_valid_acr_values')) + end + + context 'with a known IAL value' do + before do + allow(IdentityConfig.store).to receive( + :allowed_valid_authn_contexts_semantic_providers + ).and_return(client_id) + end + let(:acr_values) do + [ + 'unknown-value', + Saml::Idp::Constants::IAL_AUTH_ONLY_ACR + ].join(' ') + end + + it 'is valid' do + expect(valid?).to eq(true) + end + end + + end + context 'with ialmax requested' do let(:acr_values) { Saml::Idp::Constants::IALMAX_AUTHN_CONTEXT_CLASSREF } let(:vtr) { nil } @@ -211,42 +241,53 @@ end end - shared_examples 'allows facial match IAL only if sp is authorized' do |facial_match_ial| - let(:acr_values) { facial_match_ial } + context 'when facial match is requested' do + shared_examples 'allows facial match IAL only if sp is authorized' do |facial_match_ial| + let(:acr_values) { facial_match_ial } - context "when the IAL requested is #{facial_match_ial}" do - context 'when the service provider is allowed to use facial match ials' do - before do - allow_any_instance_of(ServiceProvider).to receive(:facial_match_ial_allowed?). - and_return(true) + context "when the IAL requested is #{facial_match_ial}" do + context 'when the service provider is allowed to use facial match ials' do + before do + allow(IdentityConfig.store).to receive( + :allowed_biometric_ial_providers + ).and_return([client_id]) + end + + it 'succeeds validation' do + expect(form).to be_valid + end end - it 'succeeds validation' do - expect(form).to be_valid + context 'when the service provider is not allowed to use facial match ials' do + it 'fails with a not authorized error' do + expect(form).not_to be_valid + expect(form.errors[:acr_values]). + to include(t('openid_connect.authorization.errors.no_auth')) + end end end + end - context 'when the service provider is not allowed to use facial match ials' do - before do - allow_any_instance_of(ServiceProvider).to receive(:facial_match_ial_allowed?). - and_return(false) - end + it_behaves_like 'allows facial match IAL only if sp is authorized', + Saml::Idp::Constants::IAL2_BIO_PREFERRED_AUTHN_CONTEXT_CLASSREF - it 'fails with a not authorized error' do - expect(form).not_to be_valid - expect(form.errors[:acr_values]). - to include(t('openid_connect.authorization.errors.no_auth')) - end + it_behaves_like 'allows facial match IAL only if sp is authorized', + Saml::Idp::Constants::IAL2_BIO_REQUIRED_AUTHN_CONTEXT_CLASSREF + + context 'when using semantic acr_values' do + before do + allow(IdentityConfig.store).to receive( + :allowed_valid_authn_contexts_semantic_providers + ).and_return([client_id]) end + it_behaves_like 'allows facial match IAL only if sp is authorized', + Saml::Idp::Constants::IAL_VERIFIED_FACIAL_MATCH_PREFERRED_ACR + + it_behaves_like 'allows facial match IAL only if sp is authorized', + Saml::Idp::Constants::IAL_VERIFIED_FACIAL_MATCH_REQUIRED_ACR end end - it_behaves_like 'allows facial match IAL only if sp is authorized', - Saml::Idp::Constants::IAL2_BIO_PREFERRED_AUTHN_CONTEXT_CLASSREF - - it_behaves_like 'allows facial match IAL only if sp is authorized', - Saml::Idp::Constants::IAL2_BIO_REQUIRED_AUTHN_CONTEXT_CLASSREF - context 'with aal but not ial requested via acr_values' do let(:acr_values) { Saml::Idp::Constants::AAL3_AUTHN_CONTEXT_CLASSREF } let(:vtr) { nil } @@ -433,22 +474,39 @@ end describe '#acr_values' do - let(:acr_values) do + let(:vtr) { nil } + let(:acr_value_list) do [ - 'http://idmanagement.gov/ns/assurance/loa/1', 'http://idmanagement.gov/ns/assurance/aal/3', - 'fake_value', - ].join(' ') + 'http://idmanagement.gov/ns/assurance/loa/1', + ] end - let(:vtr) { nil } + let(:acr_values) { acr_value_list.join(' ') } it 'is parsed into an array of valid ACR values' do - expect(form.acr_values).to eq( - %w[ - http://idmanagement.gov/ns/assurance/loa/1 - http://idmanagement.gov/ns/assurance/aal/3 - ], - ) + expect(form.acr_values).to eq acr_value_list + end + + context 'when an unknown acr value is included' do + let(:acr_value_list) do + [ + 'http://idmanagement.gov/ns/assurance/loa/1', + 'http://idmanagement.gov/ns/assurance/aal/3', + ] + end + let(:acr_values) { (acr_value_list + ['fake-value']).join(' ') } + + it 'is parsed into an array of valid ACR values' do + expect(form.acr_values).to eq acr_value_list + end + end + + context 'when the only value is an unknown acr value' do + let(:acr_values) { 'fake_value' } + + it 'returns an empty array for acr_values' do + expect(form.acr_values).to eq([]) + end end end @@ -546,6 +604,22 @@ expect(requested_aal_value).to eq(phishing_resistant) end end + + context 'when no values are passed in' do + let(:acr_values) { '' } + + it 'returns the default AAL value' do + expect(form.requested_aal_value).to eq Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF + end + end + + context 'when only an unknown value is passed in' do + let(:acr_values) { 'fake-value' } + + it 'returns the default AAL value' do + expect(form.requested_aal_value).to eq Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF + end + end end end diff --git a/spec/services/vot/parser_spec.rb b/spec/services/vot/parser_spec.rb index 43e301f3ade..1d7225d6ca5 100644 --- a/spec/services/vot/parser_spec.rb +++ b/spec/services/vot/parser_spec.rb @@ -96,24 +96,95 @@ end end - context 'when input includes unrecognized components' do - let(:acr_values) { 'i-am-not-an-acr-value' } - it 'raises an exception' do - expect { Vot::Parser.new(acr_values:).parse }.to raise_exception( - Vot::Parser::UnsupportedComponentsException, - /'i-am-not-an-acr-value'$/, - ) - end + context 'when a vector includes unrecognized components' do + let(:acr_values) { 'unknown-acr-value' } - context 'with vectors of trust' do + context 'only an unknown acr_value is passed in' do it 'raises an exception' do - vector_of_trust = 'C1.C2.Xx' - - expect { Vot::Parser.new(vector_of_trust:).parse }.to raise_exception( - Vot::Parser::UnsupportedComponentsException, - /'Xx'$/, + expect { Vot::Parser.new(acr_values:).parse }.to raise_exception( + Vot::Parser::ParseException, + 'VoT parser called without VoT or ACR values', ) end + + context 'when a known and valid acr_value is passed in as well' do + let(:acr_values) do + [ + 'unknown-acr-value', + Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, + ].join(' ') + end + + it 'parses ACR values to component values' do + result = Vot::Parser.new(acr_values:).parse + + expect(result.component_values.map(&:name).join(' ')).to eq( + Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF + ) + expect(result.aal2?).to eq(false) + expect(result.phishing_resistant?).to eq(false) + expect(result.hspd12?).to eq(false) + expect(result.identity_proofing?).to eq(false) + expect(result.facial_match?).to eq(false) + expect(result.ialmax?).to eq(false) + expect(result.enhanced_ipp?).to eq(false) + end + + context 'with semantic acr_values' do + let(:acr_values) do + [ + 'unknown-acr-value', + Saml::Idp::Constants::IAL_AUTH_ONLY_ACR, + ].join(' ') + end + + it 'parses ACR values to component values' do + result = Vot::Parser.new(acr_values:).parse + + expect(result.component_values.map(&:name).join(' ')).to eq( + Saml::Idp::Constants::IAL_AUTH_ONLY_ACR + ) + expect(result.aal2?).to eq(false) + expect(result.phishing_resistant?).to eq(false) + expect(result.hspd12?).to eq(false) + expect(result.identity_proofing?).to eq(false) + expect(result.facial_match?).to eq(false) + expect(result.ialmax?).to eq(false) + expect(result.enhanced_ipp?).to eq(false) + end + end + end + end + + context 'with vectors of trust' do + context 'only an unknown VoT is passed in' do + it 'raises an exception' do + vector_of_trust = 'Xx' + + expect { Vot::Parser.new(vector_of_trust:).parse }.to raise_exception( + Vot::Parser::ParseException, + 'VoT parser called without VoT or ACR values', + ) + end + end + + context 'along with a known vector' + it 'parses the vector' do + vector_of_trust = 'C1.C2.Xx' + + result = Vot::Parser.new(vector_of_trust:).parse + + expect(result.component_values.map(&:name).join(' ')).to eq( + 'C1 C2' + ) + expect(result.aal2?).to eq(true) + expect(result.phishing_resistant?).to eq(false) + expect(result.hspd12?).to eq(false) + expect(result.identity_proofing?).to eq(false) + expect(result.facial_match?).to eq(false) + expect(result.ialmax?).to eq(false) + expect(result.enhanced_ipp?).to eq(false) + end end end From 9b9a7e8954ce03c1cf601574f3bb78d0bf92f3b7 Mon Sep 17 00:00:00 2001 From: davida marion Date: Wed, 16 Oct 2024 14:27:51 -0400 Subject: [PATCH 03/12] Add tests of application_controller_spec --- .../application_controller_spec.rb | 29 +++++++++++++------ 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 77d69e7d2a8..38fa02bfa3f 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -486,17 +486,28 @@ def index expect(result.identity_proofing?).to eq(true) end - context 'when an unknow acr value is passed in' do - let(:acr_values) do - [ - 'http://idmanagement.gov/ns/assurance/aal/1', - 'unknown-acr-value', - ].join(' ') + context 'when an unknown acr value is passed in' do + let(:acr_values) { 'unknown-acr-value' } + + it 'raises an exception' do + expect { result }.to raise_exception( + Vot::Parser::ParseException, + 'VoT parser called without VoT or ACR values', + ) end - it 'returns a resolved authn context result' do - expect(result.aal2?).to eq(true) - expect(result.identity_proofing?).to eq(true) + context 'with a known acr value' do + let(:acr_values) do + [ + 'http://idmanagement.gov/ns/assurance/aal/1', + 'unknown-acr-value', + ].join(' ') + end + + it 'returns a resolved authn context result' do + expect(result.aal2?).to eq(true) + expect(result.identity_proofing?).to eq(true) + end end end From 96e58caddbfc317be4e26ac3ff0135b9a1dca9c1 Mon Sep 17 00:00:00 2001 From: davida marion Date: Thu, 17 Oct 2024 10:53:27 -0400 Subject: [PATCH 04/12] changelog: User-Facing Improvements, Integration Experience, Allowing and ignoring unknown authn_context values --- .../openid_connect_authorize_form_spec.rb | 19 +-- spec/services/attribute_asserter_spec.rb | 14 ++ spec/services/authn_context_resolver_spec.rb | 124 ++++++++++++------ spec/services/saml_request_validator_spec.rb | 4 +- spec/services/vot/parser_spec.rb | 9 +- 5 files changed, 119 insertions(+), 51 deletions(-) diff --git a/spec/forms/openid_connect_authorize_form_spec.rb b/spec/forms/openid_connect_authorize_form_spec.rb index a830865af87..4cc17f406dd 100644 --- a/spec/forms/openid_connect_authorize_form_spec.rb +++ b/spec/forms/openid_connect_authorize_form_spec.rb @@ -18,7 +18,7 @@ ) end - let(:acr_values) { nil } + let(:acr_values) { nil } let(:vtr) { ['C1'].to_json } let(:client_id) { 'urn:gov:gsa:openidconnect:test' } let(:nonce) { SecureRandom.hex } @@ -200,13 +200,13 @@ context 'with a known IAL value' do before do allow(IdentityConfig.store).to receive( - :allowed_valid_authn_contexts_semantic_providers + :allowed_valid_authn_contexts_semantic_providers, ).and_return(client_id) end let(:acr_values) do [ 'unknown-value', - Saml::Idp::Constants::IAL_AUTH_ONLY_ACR + Saml::Idp::Constants::IAL_AUTH_ONLY_ACR, ].join(' ') end @@ -214,7 +214,6 @@ expect(valid?).to eq(true) end end - end context 'with ialmax requested' do @@ -249,7 +248,7 @@ context 'when the service provider is allowed to use facial match ials' do before do allow(IdentityConfig.store).to receive( - :allowed_biometric_ial_providers + :allowed_biometric_ial_providers, ).and_return([client_id]) end @@ -277,7 +276,7 @@ context 'when using semantic acr_values' do before do allow(IdentityConfig.store).to receive( - :allowed_valid_authn_contexts_semantic_providers + :allowed_valid_authn_contexts_semantic_providers, ).and_return([client_id]) end it_behaves_like 'allows facial match IAL only if sp is authorized', @@ -609,7 +608,9 @@ let(:acr_values) { '' } it 'returns the default AAL value' do - expect(form.requested_aal_value).to eq Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF + expect(form.requested_aal_value).to eq( + Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF, + ) end end @@ -617,7 +618,9 @@ let(:acr_values) { 'fake-value' } it 'returns the default AAL value' do - expect(form.requested_aal_value).to eq Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF + expect(form.requested_aal_value).to eq( + Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF, + ) end end end diff --git a/spec/services/attribute_asserter_spec.rb b/spec/services/attribute_asserter_spec.rb index e74d4328d20..3903cbf66fb 100644 --- a/spec/services/attribute_asserter_spec.rb +++ b/spec/services/attribute_asserter_spec.rb @@ -107,6 +107,20 @@ it 'gets UUID from Service Provider' do expect(get_asserted_attribute(user, :uuid)).to eq user.last_identity.uuid end + + context 'when authn_context includes an unknown value' do + let(:authn_context) do + [ + ial_value, + 'unknown/authn/context', + ] + end + + it 'includes all requested attributes + uuid' do + expect(user.asserted_attributes.keys). + to eq(%i[uuid email phone first_name verified_at aal ial]) + end + end end context 'custom bundle includes dob' do diff --git a/spec/services/authn_context_resolver_spec.rb b/spec/services/authn_context_resolver_spec.rb index 60e4f4f6417..d3bb6b20570 100644 --- a/spec/services/authn_context_resolver_spec.rb +++ b/spec/services/authn_context_resolver_spec.rb @@ -48,8 +48,8 @@ vtr = ['C2.Pb'] acr_values = [ - 'http://idmanagement.gov/ns/assurance/aal/2', - 'http://idmanagement.gov/ns/assurance/ial/2', + Saml::Idp::Constants::AAL2_AUTHN_CONTEXT_CLASSREF, + Saml::Idp::Constants::IAL_VERIFIED_ACR, ].join(' ') result = AuthnContextResolver.new( @@ -152,8 +152,8 @@ context 'with no service provider' do it 'parses an ACR value into requirements' do acr_values = [ - 'http://idmanagement.gov/ns/assurance/aal/2', - 'http://idmanagement.gov/ns/assurance/ial/1', + Saml::Idp::Constants::AAL2_AUTHN_CONTEXT_CLASSREF, + Saml::Idp::Constants::IAL_AUTH_ONLY_ACR, ].join(' ') result = AuthnContextResolver.new( @@ -175,7 +175,7 @@ it 'properly parses an ACR value without an AAL ACR' do acr_values = [ - 'http://idmanagement.gov/ns/assurance/ial/1', + Saml::Idp::Constants::IAL_AUTH_ONLY_ACR, ].join(' ') result = AuthnContextResolver.new( @@ -197,7 +197,7 @@ it 'properly parses an ACR value without an IAL ACR' do acr_values = [ - 'http://idmanagement.gov/ns/assurance/aal/2', + Saml::Idp::Constants::AAL2_AUTHN_CONTEXT_CLASSREF, ].join(' ') result = AuthnContextResolver.new( @@ -223,8 +223,8 @@ service_provider = build(:service_provider, default_aal: 2) acr_values = [ - 'http://idmanagement.gov/ns/assurance/aal/1', - 'http://idmanagement.gov/ns/assurance/ial/1', + Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF, + Saml::Idp::Constants::IAL_AUTH_ONLY_ACR, ].join(' ') result = AuthnContextResolver.new( @@ -241,7 +241,7 @@ service_provider = build(:service_provider, default_aal: 2) acr_values = [ - 'http://idmanagement.gov/ns/assurance/ial/1', + Saml::Idp::Constants::IAL_AUTH_ONLY_ACR, ].join(' ') result = AuthnContextResolver.new( @@ -259,7 +259,7 @@ service_provider = build(:service_provider, default_aal: 3) acr_values = [ - 'http://idmanagement.gov/ns/assurance/ial/1', + Saml::Idp::Constants::IAL_AUTH_ONLY_ACR, ].join(' ') result = AuthnContextResolver.new( @@ -295,10 +295,10 @@ let(:service_provider) { build(:service_provider, ial: 2) } subject do AuthnContextResolver.new( - user: user, - service_provider: service_provider, + user:, + service_provider:, vtr: nil, - acr_values: acr_values, + acr_values:, ) end @@ -307,8 +307,8 @@ context 'if IAL ACR value is present' do let(:acr_values) do [ - 'http://idmanagement.gov/ns/assurance/ial/1', - 'http://idmanagement.gov/ns/assurance/aal/1', + Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, + Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF, ].join(' ') end @@ -318,12 +318,12 @@ end end - context 'if multiple IAL ACR values are present' do + context 'when multiple IAL ACR values are present' do let(:acr_values) do [ - 'http://idmanagement.gov/ns/assurance/ial/1', - 'http://idmanagement.gov/ns/assurance/ial/2', - 'http://idmanagement.gov/ns/assurance/aal/1', + Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, + Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF, + Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF, ].join(' ') end @@ -331,12 +331,28 @@ expect(result.identity_proofing?).to be true expect(result.aal2?).to be true end + + context 'when one of the acr values is unknown' do + let(:acr_values) do + [ + Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, + Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF, + Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF, + 'unknown-acr-value', + ].join(' ') + end + + it 'ignores the unknown value and uses the highest IAL ACR' do + expect(result.identity_proofing?).to eq(true) + expect(result.aal2?).to eq(true) + end + end end context 'if No IAL ACR is present' do let(:acr_values) do [ - 'http://idmanagement.gov/ns/assurance/aal/1', + Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF, ].join(' ') end @@ -346,12 +362,23 @@ end end + context 'when the only ACR value is unknown' do + let(:acr_values) { 'unknown-acr-value' } + + it 'errors out as if there were no values' do + expect { result }.to raise_error Vot::Parser::ParseException + end + end + context 'if requesting facial match comparison' do - let(:bio_value) { 'required' } + let(:bio_acr_value) do + Saml::Idp::Constants::IAL2_BIO_REQUIRED_AUTHN_CONTEXT_CLASSREF + end + let(:acr_values) do [ - "http://idmanagement.gov/ns/assurance/ial/2?bio=#{bio_value}", - 'http://idmanagement.gov/ns/assurance/aal/1', + bio_acr_value, + Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF, ].join(' ') end @@ -391,7 +418,9 @@ end context 'with facial match comparison is preferred' do - let(:bio_value) { 'preferred' } + let(:bio_acr_value) do + Saml::Idp::Constants::IAL2_BIO_PREFERRED_AUTHN_CONTEXT_CLASSREF + end context 'when the user is already verified' do context 'without facial match comparison' do @@ -478,7 +507,7 @@ context 'with no service provider' do it 'parses an ACR value into requirements' do acr_values = [ - 'http://idmanagement.gov/ns/assurance/aal/2', + Saml::Idp::Constants::AAL2_AUTHN_CONTEXT_CLASSREF, Saml::Idp::Constants::IAL_AUTH_ONLY_ACR, ] @@ -525,7 +554,7 @@ it 'properly parses an ACR value without an IAL ACR' do acr_values = [ - 'http://idmanagement.gov/ns/assurance/aal/2', + Saml::Idp::Constants::AAL2_AUTHN_CONTEXT_CLASSREF, ] resolver = AuthnContextResolver.new( user: user, @@ -561,8 +590,8 @@ context 'if IAL ACR value is present' do let(:acr_values) do [ - 'http://idmanagement.gov/ns/assurance/ial/1', - 'http://idmanagement.gov/ns/assurance/aal/1', + Saml::Idp::Constants::IAL_AUTH_ONLY_ACR, + Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF, ] end @@ -572,12 +601,12 @@ end end - context 'if multiple IAL ACR values are present' do + context 'when multiple IAL ACR values are present' do let(:acr_values) do [ - 'http://idmanagement.gov/ns/assurance/ial/1', - 'urn:acr.login.gov:verified', - 'http://idmanagement.gov/ns/assurance/aal/1', + Saml::Idp::Constants::IAL_AUTH_ONLY_ACR, + Saml::Idp::Constants::IAL_VERIFIED_ACR, + Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF, ] end @@ -585,12 +614,28 @@ expect(result.identity_proofing?).to be true expect(result.aal2?).to be true end + + context 'when one of the acr values is unknown' do + let(:acr_values) do + [ + Saml::Idp::Constants::IAL_AUTH_ONLY_ACR, + Saml::Idp::Constants::IAL_VERIFIED_ACR, + Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF, + 'unknown-acr-value', + ] + end + + it 'ignores the unknown value and uses the highest IAL ACR' do + expect(result.identity_proofing?).to eq(true) + expect(result.aal2?).to eq(true) + end + end end context 'if No IAL ACR is present' do let(:acr_values) do [ - 'http://idmanagement.gov/ns/assurance/aal/1', + Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF, ] end @@ -616,11 +661,14 @@ end context 'if requesting facial match comparison' do - let(:bio_value) { 'required' } + let(:bio_acr_value) do + Saml::Idp::Constants::IAL_VERIFIED_FACIAL_MATCH_REQUIRED_ACR + end + let(:acr_values) do [ - "urn:acr.login.gov:verified-facial-match-#{bio_value}", - 'http://idmanagement.gov/ns/assurance/aal/1', + bio_acr_value, + Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF, ] end @@ -674,7 +722,9 @@ end context 'with facial match comparison is preferred' do - let(:bio_value) { 'preferred' } + let(:bio_acr_value) do + Saml::Idp::Constants::IAL_VERIFIED_FACIAL_MATCH_PREFERRED_ACR + end context 'when the user is already verified' do context 'without facial match comparison' do diff --git a/spec/services/saml_request_validator_spec.rb b/spec/services/saml_request_validator_spec.rb index 120c1264d67..09817a9c81c 100644 --- a/spec/services/saml_request_validator_spec.rb +++ b/spec/services/saml_request_validator_spec.rb @@ -33,7 +33,7 @@ use_vot_in_sp_requests, ) allow(IdentityConfig.store).to receive( - :allowed_biometric_ial_providers + :allowed_biometric_ial_providers, ).and_return([issuer]) allow(IdentityConfig.store).to receive( :allowed_valid_authn_contexts_semantic_providers, @@ -239,7 +239,7 @@ Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF, Saml::Idp::Constants::IAL_VERIFIED_ACR, ].each do |ial_value| - let(:authn_context) { [ial_value] } + let(:authn_context) { [ial_value] } it 'returns FormResponse with success: false' do errors = { diff --git a/spec/services/vot/parser_spec.rb b/spec/services/vot/parser_spec.rb index 1d7225d6ca5..a95aa0fd170 100644 --- a/spec/services/vot/parser_spec.rb +++ b/spec/services/vot/parser_spec.rb @@ -119,7 +119,7 @@ result = Vot::Parser.new(acr_values:).parse expect(result.component_values.map(&:name).join(' ')).to eq( - Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF + Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, ) expect(result.aal2?).to eq(false) expect(result.phishing_resistant?).to eq(false) @@ -142,7 +142,7 @@ result = Vot::Parser.new(acr_values:).parse expect(result.component_values.map(&:name).join(' ')).to eq( - Saml::Idp::Constants::IAL_AUTH_ONLY_ACR + Saml::Idp::Constants::IAL_AUTH_ONLY_ACR, ) expect(result.aal2?).to eq(false) expect(result.phishing_resistant?).to eq(false) @@ -168,14 +168,14 @@ end end - context 'along with a known vector' + context 'along with a known vector' do it 'parses the vector' do vector_of_trust = 'C1.C2.Xx' result = Vot::Parser.new(vector_of_trust:).parse expect(result.component_values.map(&:name).join(' ')).to eq( - 'C1 C2' + 'C1 C2', ) expect(result.aal2?).to eq(true) expect(result.phishing_resistant?).to eq(false) @@ -185,6 +185,7 @@ expect(result.ialmax?).to eq(false) expect(result.enhanced_ipp?).to eq(false) end + end end end From 36ca7b900f0a453485237cf5a9e345d439e5ab65 Mon Sep 17 00:00:00 2001 From: davida marion Date: Thu, 17 Oct 2024 11:43:23 -0400 Subject: [PATCH 05/12] Updates some hard-coded balues to use constants --- spec/controllers/application_controller_spec.rb | 4 ++-- spec/services/vot/parser_spec.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 38fa02bfa3f..d3a0a0bff8f 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -477,7 +477,7 @@ def index let(:vtr) { nil } let(:acr_values) do [ - 'http://idmanagement.gov/ns/assurance/aal/1', + Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF, ].join(' ') end @@ -499,7 +499,7 @@ def index context 'with a known acr value' do let(:acr_values) do [ - 'http://idmanagement.gov/ns/assurance/aal/1', + Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF, 'unknown-acr-value', ].join(' ') end diff --git a/spec/services/vot/parser_spec.rb b/spec/services/vot/parser_spec.rb index a95aa0fd170..6065776e08b 100644 --- a/spec/services/vot/parser_spec.rb +++ b/spec/services/vot/parser_spec.rb @@ -96,7 +96,7 @@ end end - context 'when a vector includes unrecognized components' do + context 'when input includes unrecognized components' do let(:acr_values) { 'unknown-acr-value' } context 'only an unknown acr_value is passed in' do From 1bd35d81c0e918ad0e7345260f0a608953ae0c19 Mon Sep 17 00:00:00 2001 From: davida marion Date: Thu, 17 Oct 2024 11:45:42 -0400 Subject: [PATCH 06/12] Actually adds those values to the commit --- spec/forms/openid_connect_authorize_form_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/forms/openid_connect_authorize_form_spec.rb b/spec/forms/openid_connect_authorize_form_spec.rb index 4cc17f406dd..380037b22f4 100644 --- a/spec/forms/openid_connect_authorize_form_spec.rb +++ b/spec/forms/openid_connect_authorize_form_spec.rb @@ -476,8 +476,8 @@ let(:vtr) { nil } let(:acr_value_list) do [ - 'http://idmanagement.gov/ns/assurance/aal/3', - 'http://idmanagement.gov/ns/assurance/loa/1', + Saml::Idp::Constants::AAL3_AUTHN_CONTEXT_CLASSREF, + Saml::Idp::Constants::LOA1_AUTHN_CONTEXT_CLASSREF, ] end let(:acr_values) { acr_value_list.join(' ') } @@ -489,8 +489,8 @@ context 'when an unknown acr value is included' do let(:acr_value_list) do [ - 'http://idmanagement.gov/ns/assurance/loa/1', - 'http://idmanagement.gov/ns/assurance/aal/3', + Saml::Idp::Constants::LOA1_AUTHN_CONTEXT_CLASSREF, + Saml::Idp::Constants::AAL3_AUTHN_CONTEXT_CLASSREF, ] end let(:acr_values) { (acr_value_list + ['fake-value']).join(' ') } From e9874f0430ab97a065f62f96c20488ea0457591a Mon Sep 17 00:00:00 2001 From: davida marion Date: Fri, 18 Oct 2024 09:10:58 -0400 Subject: [PATCH 07/12] Add unknown_authn_contexts to three events --- .../authorization_controller.rb | 8 +++ app/controllers/saml_idp_controller.rb | 22 ++++++- app/services/analytics_events.rb | 9 +++ .../authorization_controller_spec.rb | 59 +++++++++++++++++++ spec/controllers/saml_idp_controller_spec.rb | 33 ++++++++++- 5 files changed, 127 insertions(+), 4 deletions(-) diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index 930f7e25b42..01944cf0858 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -174,6 +174,7 @@ def pre_validate_authorize_form user_fully_authenticated: user_fully_authenticated?, referer: request.referer, vtr_param: params[:vtr], + unknown_authn_contexts:, ), ) return if result.success? @@ -258,5 +259,12 @@ def redirect_user(redirect_uri, issuer, user_uuid) def sp_handoff_bouncer @sp_handoff_bouncer ||= SpHandoffBouncer.new(sp_session) end + + def unknown_authn_contexts + return nil if params[:vtr].present? || params[:acr_values].blank? + + (params[:acr_values].split - Saml::Idp::Constants::VALID_AUTHN_CONTEXTS). + join(' ').presence + end end end diff --git a/app/controllers/saml_idp_controller.rb b/app/controllers/saml_idp_controller.rb index b4278026379..29518576d42 100644 --- a/app/controllers/saml_idp_controller.rb +++ b/app/controllers/saml_idp_controller.rb @@ -131,6 +131,7 @@ def capture_analytics request_signed: saml_request.signed?, matching_cert_serial:, requested_nameid_format: saml_request.name_id_format, + unknown_authn_contexts:, ) if result.success? && saml_request.signed? @@ -151,12 +152,13 @@ def log_external_saml_auth_request analytics.saml_auth_request( requested_ial: requested_ial, - authn_context: saml_request&.requested_authn_contexts, + authn_context: requested_authn_contexts, 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], service_provider: saml_request&.issuer, + unknown_authn_contexts:, user_fully_authenticated: user_fully_authenticated?, ) end @@ -227,4 +229,22 @@ def resolved_authn_context_int_ial def require_path_year render_not_found if params[:path_year].blank? end + + def unknown_authn_contexts + return nil if saml_request.requested_vtr_authn_contexts.present? + return nil if requested_authn_contexts.blank? + + (requested_authn_contexts - + Saml::Idp::Constants::VALID_AUTHN_CONTEXTS).reject do |authn_context| + authn_context.match(req_attrs_regexp) + end.join(' ').presence + end + + def requested_authn_contexts + @request_authn_contexts || saml_request&.requested_authn_contexts + end + + def req_attrs_regexp + 'http:\/\/idmanagement.gov\/ns\/requested_attributes\?ReqAttr=' + end end diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index d410f516c4e..dd36dd0d19b 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -5526,6 +5526,7 @@ def openid_connect_bearer_token(success:, ial:, client_id:, errors:, error_detai # @param [String, nil] vtr_param # @param [Boolean] unauthorized_scope # @param [Boolean] user_fully_authenticated + # @param [String] unknown_authn_contexts space separated list of unknown contexts def openid_connect_request_authorization( success:, errors:, @@ -5542,6 +5543,7 @@ def openid_connect_request_authorization( unauthorized_scope:, user_fully_authenticated:, error_details: nil, + unknown_authn_contexts: nil, **extra ) track_event( @@ -5561,6 +5563,7 @@ def openid_connect_request_authorization( vtr_param:, unauthorized_scope:, user_fully_authenticated:, + unknown_authn_contexts:, **extra, ) end @@ -6337,6 +6340,7 @@ def rules_of_use_visit # matches the request certificate in a successful, signed request # @param [Hash] cert_error_details Details for errors that occurred because of an invalid # signature + # @param [String] unknown_authn_contexts space separated list of unknown contexts def saml_auth( success:, errors:, @@ -6353,6 +6357,7 @@ def saml_auth( matching_cert_serial:, error_details: nil, cert_error_details: nil, + unknown_authn_contexts: nil, **extra ) track_event( @@ -6372,6 +6377,7 @@ def saml_auth( request_signed:, matching_cert_serial:, cert_error_details:, + unknown_authn_contexts:, **extra, ) end @@ -6383,6 +6389,7 @@ def saml_auth( # @param [Boolean] force_authn # @param [Boolean] final_auth_request # @param [String] service_provider + # @param [String] unknown_authn_contexts space separated list of unknown contexts # @param [Boolean] user_fully_authenticated # An external request for SAML Authentication was received def saml_auth_request( @@ -6393,6 +6400,7 @@ def saml_auth_request( force_authn:, final_auth_request:, service_provider:, + unknown_authn_contexts:, user_fully_authenticated:, **extra ) @@ -6405,6 +6413,7 @@ def saml_auth_request( force_authn:, final_auth_request:, service_provider:, + unknown_authn_contexts:, user_fully_authenticated:, **extra, ) diff --git a/spec/controllers/openid_connect/authorization_controller_spec.rb b/spec/controllers/openid_connect/authorization_controller_spec.rb index 16686795bc6..0590bca9983 100644 --- a/spec/controllers/openid_connect/authorization_controller_spec.rb +++ b/spec/controllers/openid_connect/authorization_controller_spec.rb @@ -2029,6 +2029,65 @@ end end + context 'when there are unknown acr_values params' do + let(:unknown_value) { 'unknown-acr-value' } + let(:acr_values) { unknown_value } + + context 'when there is only an unknown acr_value' do + it 'tracks the event with errors' do + stub_analytics + + action + + expect(@analytics).to have_logged_event( + 'OpenID Connect: authorization request', + success: false, + client_id:, + prompt:, + allow_prompt_login: true, + unauthorized_scope: false, + errors: hash_including(:acr_values), + error_details: hash_including(:acr_values), + user_fully_authenticated: true, + acr_values: '', + code_challenge_present: false, + scope: 'openid profile', + unknown_authn_contexts: unknown_value, + ) + end + + context 'when there is also a valid acr_value' do + let(:known_value) { Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF } + let(:acr_values) do + [ + unknown_value, + known_value, + ].join(' ') + end + + it 'tracks the event' do + stub_analytics + + action + expect(@analytics).to have_logged_event( + 'OpenID Connect: authorization request', + success: true, + client_id:, + prompt:, + allow_prompt_login: true, + unauthorized_scope: false, + user_fully_authenticated: true, + acr_values: known_value, + code_challenge_present: false, + scope: 'openid profile', + unknown_authn_contexts: unknown_value, + errors: {}, + ) + end + end + end + end + context 'vtr with invalid params that do not interfere with the redirect_uri' do let(:acr_values) { nil } let(:vtr) { ['C1'].to_json } diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index 5a19104dccb..fc5b2b31810 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -970,15 +970,22 @@ def name_id_version(format_urn) end context 'authn_context is invalid' do - it 'renders an error page' do + let(:unknown_value) do + 'http://idmanagement.gov/ns/assurance/loa/5' + end + let(:authn_context) { unknown_value } + + before do stub_analytics saml_get_auth( saml_settings( - overrides: { authn_context: 'http://idmanagement.gov/ns/assurance/loa/5' }, + overrides: { authn_context: }, ), ) + end + it 'renders an error page' do expect(controller).to render_template('saml_idp/auth/error') expect(response.status).to eq(400) expect(response.body).to include(t('errors.messages.unauthorized_authn_context')) @@ -989,7 +996,7 @@ def name_id_version(format_urn) errors: { authn_context: [t('errors.messages.unauthorized_authn_context')] }, error_details: { authn_context: { unauthorized_authn_context: true } }, nameid_format: Saml::Idp::Constants::NAME_ID_FORMAT_PERSISTENT, - authn_context: ['http://idmanagement.gov/ns/assurance/loa/5'], + authn_context: [unknown_value], authn_context_comparison: 'exact', service_provider: 'http://localhost:3000', request_signed: true, @@ -998,9 +1005,29 @@ def name_id_version(format_urn) idv: false, finish_profile: false, matching_cert_serial: saml_test_sp_cert_serial, + unknown_authn_contexts: unknown_value, ), ) end + + context 'there is also a valid authn_context' do + let(:authn_context) do + [ + Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, + unknown_value, + ] + end + + it 'logs the unknown authn_context value' do + expect(response.status).to eq(302) + expect(@analytics).to have_logged_event( + 'SAML Auth Request', + hash_including( + unknown_authn_contexts: unknown_value, + ), + ) + end + end end context 'authn_context scenarios' do From 50e7efffe51795de27f9f91dd26a2521c2ea3079 Mon Sep 17 00:00:00 2001 From: davida marion Date: Fri, 18 Oct 2024 09:36:53 -0400 Subject: [PATCH 08/12] Add missing test --- app/controllers/saml_idp_controller.rb | 2 +- spec/controllers/saml_idp_controller_spec.rb | 22 ++++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/app/controllers/saml_idp_controller.rb b/app/controllers/saml_idp_controller.rb index 29518576d42..73f8e843834 100644 --- a/app/controllers/saml_idp_controller.rb +++ b/app/controllers/saml_idp_controller.rb @@ -245,6 +245,6 @@ def requested_authn_contexts end def req_attrs_regexp - 'http:\/\/idmanagement.gov\/ns\/requested_attributes\?ReqAttr=' + Regexp.escape(Saml::Idp::Constants::REQUESTED_ATTRIBUTES_CLASSREF) end end diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index fc5b2b31810..b04cdb5593e 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -1027,6 +1027,28 @@ def name_id_version(format_urn) ), ) end + + context 'when it includes the ReqAttributes AuthnContext' do + let(:authn_context) do + [ + Saml::Idp::Constants::REQUESTED_ATTRIBUTES_CLASSREF, + Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, + unknown_value, + ] + end + + it 'logs the unknown authn_context value' do + expect(response.status).to eq(302) + expect(@analytics).to have_logged_event( + 'SAML Auth Request', + hash_including( + unknown_authn_contexts: unknown_value, + ), + ) + end + + end + end end From 66187e67a0c0469ce594189675d4ff26a485d7e0 Mon Sep 17 00:00:00 2001 From: davida marion Date: Fri, 18 Oct 2024 10:02:18 -0400 Subject: [PATCH 09/12] Fix lint --- spec/controllers/saml_idp_controller_spec.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index b04cdb5593e..9777cfbc734 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -1046,9 +1046,7 @@ def name_id_version(format_urn) ), ) end - end - end end From 52a697bc26fb39fc2d57df7f93f6808bb228029a Mon Sep 17 00:00:00 2001 From: davida marion Date: Mon, 21 Oct 2024 13:54:43 -0400 Subject: [PATCH 10/12] PR feedback --- app/controllers/saml_idp_controller.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/controllers/saml_idp_controller.rb b/app/controllers/saml_idp_controller.rb index 73f8e843834..a945f66259f 100644 --- a/app/controllers/saml_idp_controller.rb +++ b/app/controllers/saml_idp_controller.rb @@ -234,12 +234,15 @@ def unknown_authn_contexts return nil if saml_request.requested_vtr_authn_contexts.present? return nil if requested_authn_contexts.blank? - (requested_authn_contexts - - Saml::Idp::Constants::VALID_AUTHN_CONTEXTS).reject do |authn_context| + unmatched_authn_contexts.reject do |authn_context| authn_context.match(req_attrs_regexp) end.join(' ').presence end + def unmatched_authn_contexts + requested_authn_contexts - Saml::Idp::Constants::VALID_AUTHN_CONTEXTS + end + def requested_authn_contexts @request_authn_contexts || saml_request&.requested_authn_contexts end From 4f1136e18ec095d40656744b284dd263f2b1d228 Mon Sep 17 00:00:00 2001 From: "Davida (she/they)" Date: Tue, 22 Oct 2024 08:10:56 -0400 Subject: [PATCH 11/12] Update spec/services/saml_request_validator_spec.rb Co-authored-by: Nadya Primak --- spec/services/saml_request_validator_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/services/saml_request_validator_spec.rb b/spec/services/saml_request_validator_spec.rb index 09817a9c81c..b367dc9c279 100644 --- a/spec/services/saml_request_validator_spec.rb +++ b/spec/services/saml_request_validator_spec.rb @@ -72,7 +72,7 @@ end end - context 'no authn context and sp and authorized nameID format' do + context 'no authn context and valid sp and authorized nameID format' do let(:authn_context) { [] } it 'returns FormResponse with success: true' do expect(response.to_h).to include( From 07f8e595599ed6e07ef4eb987040bdddfa91f2be Mon Sep 17 00:00:00 2001 From: "Davida (she/they)" Date: Tue, 22 Oct 2024 08:11:05 -0400 Subject: [PATCH 12/12] Update spec/services/saml_request_validator_spec.rb Co-authored-by: Nadya Primak --- spec/services/saml_request_validator_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/services/saml_request_validator_spec.rb b/spec/services/saml_request_validator_spec.rb index b367dc9c279..73ceaedc9a2 100644 --- a/spec/services/saml_request_validator_spec.rb +++ b/spec/services/saml_request_validator_spec.rb @@ -221,7 +221,7 @@ ) end - context 'unknwn authn_context requested along with a valid one' do + context 'unknown authn_context requested along with a valid one' do let(:authn_context) { ['IAL1', Saml::Idp::Constants::IAL_AUTH_ONLY_ACR] } it 'returns FormResponse with success: true' do