From 3aee367b9bab8f3d8c9d58bfda58edd89b168da5 Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Tue, 19 Dec 2023 15:00:05 -0500 Subject: [PATCH] Revert "Revert "LG-11697 Store whether a biometric comparison is required in the SP session (#9759)" (#9804)" This reverts commit 558ebd6b85d971d053f1a56450b9c1720c8158ed. The reverted commit here was reverting the changes in #9759. That change had issues with compatibility which were addressed in the changes in the reverted commit and deployed. The deployment of those changes makes this commit safe to merge. [skip changelog] --- app/forms/openid_connect_authorize_form.rb | 15 ++++++- app/models/federated_protocols/oidc.rb | 4 ++ app/models/federated_protocols/saml.rb | 4 ++ app/models/service_provider_request.rb | 6 ++- .../service_provider_request_handler.rb | 1 + .../service_provider_request_proxy.rb | 8 +++- app/services/store_sp_metadata_in_session.rb | 1 + .../authorization_controller_spec.rb | 9 +++++ spec/controllers/saml_idp_controller_spec.rb | 2 + .../openid_connect_authorize_form_spec.rb | 2 + .../store_sp_metadata_in_session_spec.rb | 39 +++++++++++++++++++ 11 files changed, 86 insertions(+), 5 deletions(-) diff --git a/app/forms/openid_connect_authorize_form.rb b/app/forms/openid_connect_authorize_form.rb index 6906ef1e049..9000ec7c518 100644 --- a/app/forms/openid_connect_authorize_form.rb +++ b/app/forms/openid_connect_authorize_form.rb @@ -17,7 +17,15 @@ class OpenidConnectAuthorizeForm state ].freeze - ATTRS = [:unauthorized_scope, :acr_values, :scope, :verified_within, *SIMPLE_ATTRS].freeze + ATTRS = [ + :unauthorized_scope, + :acr_values, + :scope, + :verified_within, + :biometric_comparison_required, + *SIMPLE_ATTRS, + ].freeze + AALS_BY_PRIORITY = [Saml::Idp::Constants::AAL2_HSPD12_AUTHN_CONTEXT_CLASSREF, Saml::Idp::Constants::AAL3_HSPD12_AUTHN_CONTEXT_CLASSREF, Saml::Idp::Constants::AAL2_PHISHING_RESISTANT_AUTHN_CONTEXT_CLASSREF, @@ -55,6 +63,7 @@ def initialize(params) @prompt ||= 'select_account' @scope = parse_to_values(params[:scope], scopes) @unauthorized_scope = check_for_unauthorized_scope(params) + @biometric_comparison_required = params[:biometric_comparison_required].to_s == 'true' if verified_within_allowed? @duration_parser = DurationParser.new(params[:verified_within]) @@ -130,6 +139,10 @@ def requested_aal_value :ial2_or_greater?, :ial2_requested? + def biometric_comparison_required? + @biometric_comparison_required + end + private attr_reader :identity, :success diff --git a/app/models/federated_protocols/oidc.rb b/app/models/federated_protocols/oidc.rb index 74b82e1697b..33b92251cf3 100644 --- a/app/models/federated_protocols/oidc.rb +++ b/app/models/federated_protocols/oidc.rb @@ -20,6 +20,10 @@ 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 0840edfb97c..ecc0dea6569 100644 --- a/app/models/federated_protocols/saml.rb +++ b/app/models/federated_protocols/saml.rb @@ -26,6 +26,10 @@ def service_provider current_service_provider end + def biometric_comparison_required? + false + end + private attr_reader :request diff --git a/app/models/service_provider_request.rb b/app/models/service_provider_request.rb index ba4fe894ff1..06f39863ddf 100644 --- a/app/models/service_provider_request.rb +++ b/app/models/service_provider_request.rb @@ -2,7 +2,8 @@ class ServiceProviderRequest # WARNING - Modification of these params requires particular care # since these objects are serialized to/from Redis and may be present # upon deployment - attr_accessor :uuid, :issuer, :url, :ial, :aal, :requested_attributes + attr_accessor :uuid, :issuer, :url, :ial, :aal, :requested_attributes, + :biometric_comparison_required def initialize( uuid: nil, @@ -11,7 +12,7 @@ def initialize( ial: nil, aal: nil, requested_attributes: [], - biometric_comparison_required: false # rubocop:disable Lint/UnusedMethodArgument + biometric_comparison_required: false ) @uuid = uuid @issuer = issuer @@ -19,6 +20,7 @@ def initialize( @ial = ial @aal = aal @requested_attributes = requested_attributes&.map(&:to_s) + @biometric_comparison_required = biometric_comparison_required end def ==(other) diff --git a/app/services/service_provider_request_handler.rb b/app/services/service_provider_request_handler.rb index d23aecbf837..089293b8f77 100644 --- a/app/services/service_provider_request_handler.rb +++ b/app/services/service_provider_request_handler.rb @@ -64,6 +64,7 @@ def attributes ial: protocol.ial, aal: protocol.aal, requested_attributes: protocol.requested_attributes, + biometric_comparison_required: protocol.biometric_comparison_required?, uuid: request_id, url: url, } diff --git a/app/services/service_provider_request_proxy.rb b/app/services/service_provider_request_proxy.rb index 0840f2ae695..d39e615c085 100644 --- a/app/services/service_provider_request_proxy.rb +++ b/app/services/service_provider_request_proxy.rb @@ -33,7 +33,8 @@ def self.find_or_create_by(uuid:) return obj if obj spr = ServiceProviderRequest.new( uuid: uuid, issuer: nil, url: nil, ial: nil, - aal: nil, requested_attributes: nil + aal: nil, requested_attributes: nil, + biometric_comparison_required: false ) yield(spr) create( @@ -43,12 +44,15 @@ def self.find_or_create_by(uuid:) ial: spr.ial, aal: spr.aal, requested_attributes: spr.requested_attributes, + biometric_comparison_required: spr.biometric_comparison_required, ) end def self.create(hash) uuid = hash[:uuid] - obj = hash.slice(:issuer, :url, :ial, :aal, :requested_attributes) + obj = hash.slice( + :issuer, :url, :ial, :aal, :requested_attributes, :biometric_comparison_required + ) write(obj, uuid) hash_to_spr(obj, uuid) end diff --git a/app/services/store_sp_metadata_in_session.rb b/app/services/store_sp_metadata_in_session.rb index fc44045b9df..13c052b2646 100644 --- a/app/services/store_sp_metadata_in_session.rb +++ b/app/services/store_sp_metadata_in_session.rb @@ -36,6 +36,7 @@ def update_session request_url: sp_request.url, request_id: sp_request.uuid, requested_attributes: sp_request.requested_attributes, + biometric_comparison_required: sp_request.biometric_comparison_required, } end diff --git a/spec/controllers/openid_connect/authorization_controller_spec.rb b/spec/controllers/openid_connect/authorization_controller_spec.rb index aab1adbc70d..6b362094969 100644 --- a/spec/controllers/openid_connect/authorization_controller_spec.rb +++ b/spec/controllers/openid_connect/authorization_controller_spec.rb @@ -995,8 +995,17 @@ request_id: sp_request_id, request_url: request.original_url, requested_attributes: %w[], + biometric_comparison_required: false, ) end + + it 'sets biometric_comparison_required to true if biometric comparison is required' do + params[:biometric_comparison_required] = true + + action + + expect(session[:sp][:biometric_comparison_required]).to eq(true) + end end end end diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index 58abedb78b7..2a29333ab46 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -1127,6 +1127,7 @@ def name_id_version(format_urn) request_url: @stored_request_url.gsub('authpost', 'auth'), request_id: sp_request_id, requested_attributes: ['email'], + biometric_comparison_required: false, ) end @@ -1158,6 +1159,7 @@ def name_id_version(format_urn) request_url: @saml_request.request.original_url.gsub('authpost', 'auth'), request_id: sp_request_id, requested_attributes: ['email'], + biometric_comparison_required: false, ) end diff --git a/spec/forms/openid_connect_authorize_form_spec.rb b/spec/forms/openid_connect_authorize_form_spec.rb index d986634faa4..fafe7188723 100644 --- a/spec/forms/openid_connect_authorize_form_spec.rb +++ b/spec/forms/openid_connect_authorize_form_spec.rb @@ -14,6 +14,7 @@ code_challenge: code_challenge, code_challenge_method: code_challenge_method, verified_within: verified_within, + biometric_comparison_required: biometric_comparison_required, ) end @@ -33,6 +34,7 @@ let(:code_challenge) { nil } let(:code_challenge_method) { nil } let(:verified_within) { nil } + let(:biometric_comparison_required) { nil } describe '#submit' do subject(:result) { form.submit } diff --git a/spec/services/store_sp_metadata_in_session_spec.rb b/spec/services/store_sp_metadata_in_session_spec.rb index 773b7e2a881..6504e43d929 100644 --- a/spec/services/store_sp_metadata_in_session_spec.rb +++ b/spec/services/store_sp_metadata_in_session_spec.rb @@ -20,6 +20,7 @@ sp_request.ial = Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF sp_request.url = 'http://issuer.gov' sp_request.requested_attributes = %w[email] + sp_request.biometric_comparison_required = false end instance = StoreSpMetadataInSession.new(session: app_session, request_id: request_id) @@ -34,6 +35,7 @@ request_url: 'http://issuer.gov', request_id: request_id, requested_attributes: %w[email], + biometric_comparison_required: false, } instance.call @@ -51,6 +53,7 @@ sp_request.aal = Saml::Idp::Constants::AAL3_AUTHN_CONTEXT_CLASSREF sp_request.url = 'http://issuer.gov' sp_request.requested_attributes = %w[email] + sp_request.biometric_comparison_required = false end instance = StoreSpMetadataInSession.new(session: app_session, request_id: request_id) @@ -65,6 +68,7 @@ request_url: 'http://issuer.gov', request_id: request_id, requested_attributes: %w[email], + biometric_comparison_required: false, } instance.call @@ -82,6 +86,7 @@ sp_request.aal = Saml::Idp::Constants::AAL2_PHISHING_RESISTANT_AUTHN_CONTEXT_CLASSREF sp_request.url = 'http://issuer.gov' sp_request.requested_attributes = %w[email] + sp_request.biometric_comparison_required = false end instance = StoreSpMetadataInSession.new(session: app_session, request_id: request_id) @@ -96,6 +101,40 @@ request_url: 'http://issuer.gov', request_id: request_id, requested_attributes: %w[email], + biometric_comparison_required: false, + } + + instance.call + expect(app_session[:sp]).to eq app_session_hash + end + end + + context 'when biometric comparison is requested' do + it 'sets the session[:sp] hash' do + app_session = {} + request_id = SecureRandom.uuid + ServiceProviderRequestProxy.find_or_create_by(uuid: request_id) do |sp_request| + sp_request.issuer = 'issuer' + sp_request.ial = Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF + sp_request.aal = Saml::Idp::Constants::AAL3_AUTHN_CONTEXT_CLASSREF + sp_request.url = 'http://issuer.gov' + sp_request.requested_attributes = %w[email] + sp_request.biometric_comparison_required = true + end + instance = StoreSpMetadataInSession.new(session: app_session, request_id: request_id) + + app_session_hash = { + issuer: 'issuer', + aal_level_requested: 3, + piv_cac_requested: false, + phishing_resistant_requested: true, + ial: 2, + ial2: true, + ialmax: false, + request_url: 'http://issuer.gov', + request_id: request_id, + requested_attributes: %w[email], + biometric_comparison_required: true, } instance.call