Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor ial/aal determination #11160

Merged
merged 1 commit into from
Sep 3, 2024
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
6 changes: 1 addition & 5 deletions app/controllers/concerns/saml_idp_auth_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 ||
Sgtpluck marked this conversation as resolved.
Show resolved Hide resolved
default_aal_context
end
end

def requested_ial_authn_context
Sgtpluck marked this conversation as resolved.
Show resolved Hide resolved
saml_request.requested_ial_authn_context || default_ial_context
end

def link_identity_from_session_data
IdentityLinker.
new(current_user, saml_request_service_provider).
Expand Down
14 changes: 8 additions & 6 deletions app/controllers/saml_idp_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Sgtpluck marked this conversation as resolved.
Show resolved Hide resolved
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],
Expand All @@ -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'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not in scope to your change, but interesting that we return 'none' here instead of nil 🤔

end
end

def handle_successful_handoff
Expand Down
4 changes: 0 additions & 4 deletions app/models/federated_protocols/oidc.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
24 changes: 16 additions & 8 deletions app/models/federated_protocols/saml.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

module FederatedProtocols
class Saml
IAL_PREFIX = %r{^http://idmanagement.gov/ns/assurance/ial}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] it looks like the LOA prefix is not being included. we should probably add that back as this is a refactor, not a code change

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, i understand it's maybe not what we want, but if we're going to make a change that affects partner requests or responses, we have to consider it fully and not just include it in a refactor. feel free to make a ticket for it!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell (and the tests), it only affects the logged analytics event, and doesn't affect what we send to partners.

Copy link
Member Author

@vrajmohan vrajmohan Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! once you add the LOA prefix back in i'll approve

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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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) &&
Expand Down
4 changes: 2 additions & 2 deletions app/services/analytics_events.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5845,7 +5845,7 @@ def rules_of_use_visit
# @param [String] endpoint
# @param [Boolean] idv
# @param [Boolean] finish_profile
# @param [Integer] requested_ial
# @param [String] requested_ial
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch on these two

# @param [Boolean] request_signed
# @param [String] matching_cert_serial
# matches the request certificate in a successful, signed request
Expand Down Expand Up @@ -5890,7 +5890,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
Expand Down
16 changes: 10 additions & 6 deletions app/services/attribute_asserter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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
Expand Down
3 changes: 0 additions & 3 deletions spec/features/idv/analytics_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -979,9 +979,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
Expand Down
4 changes: 0 additions & 4 deletions spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
68 changes: 68 additions & 0 deletions spec/models/federated_protocols/saml_spec.rb
Original file line number Diff line number Diff line change
@@ -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
Sgtpluck marked this conversation as resolved.
Show resolved Hide resolved
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