Skip to content
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
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ gem 'rqrcode'
gem 'ruby-progressbar'
gem 'ruby-saml'
gem 'safe_target_blank', '>= 1.0.2'
gem 'saml_idp', github: '18F/saml_idp', tag: '0.20.2-18f'
gem 'saml_idp', github: '18F/saml_idp', tag: '0.21.0-18f'
gem 'scrypt'
gem 'simple_form', '>= 5.0.2'
gem 'stringex', require: false
Expand Down
6 changes: 3 additions & 3 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@ GIT

GIT
remote: https://github.com/18F/saml_idp.git
revision: dd8643b16c8214f7b791763538180d043af7ef65
tag: 0.20.2-18f
revision: 33275d69f7609e448942d6e3ce5c27779920995f
tag: 0.21.0-18f
specs:
saml_idp (0.20.2.pre.18f)
saml_idp (0.21.0.pre.18f)
activesupport
builder
faraday
Expand Down
9 changes: 6 additions & 3 deletions app/controllers/concerns/saml_idp_auth_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ module SamlIdpAuthConcern
private

def block_biometric_requests_in_production
if @saml_request_validator.parsed_vector_of_trust&.biometric_comparison? &&
if @saml_request_validator.biometric_comparison_requested? &&
!FeatureManagement.idv_allow_selfie_check?
render_not_acceptable
end
Expand Down Expand Up @@ -130,9 +130,12 @@ def default_ial_context
end

def response_authn_context
saml_request.requested_vtr_authn_context ||
if saml_request.requested_vtr_authn_contexts.present?
resolved_authn_context_result.expanded_component_values
else
saml_request.requested_aal_authn_context ||
default_aal_context
default_aal_context
end
end

def requested_ial_authn_context
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/saml_idp_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ def log_external_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_vtr_authn_context: saml_request&.requested_vtr_authn_context,
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,
Expand Down
2 changes: 1 addition & 1 deletion app/models/federated_protocols/saml.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ def acr_values
end

def vtr
[request.requested_vtr_authn_context] if request.requested_vtr_authn_context.present?
request.requested_vtr_authn_contexts.presence
end

def requested_attributes
Expand Down
6 changes: 3 additions & 3 deletions app/services/analytics_events.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4918,7 +4918,7 @@ def saml_auth(
# @param [Integer] requested_ial
# @param [Array] authn_context
# @param [String, nil] requested_aal_authn_context
# @param [String, nil] requested_vtr_authn_context
# @param [String, nil] requested_vtr_authn_contexts
# @param [Boolean] force_authn
# @param [Boolean] final_auth_request
# @param [String] service_provider
Expand All @@ -4928,7 +4928,7 @@ def saml_auth_request(
requested_ial:,
authn_context:,
requested_aal_authn_context:,
requested_vtr_authn_context:,
requested_vtr_authn_contexts:,
force_authn:,
final_auth_request:,
service_provider:,
Expand All @@ -4941,7 +4941,7 @@ def saml_auth_request(
requested_ial: requested_ial,
authn_context: authn_context,
requested_aal_authn_context: requested_aal_authn_context,
requested_vtr_authn_context: requested_vtr_authn_context,
requested_vtr_authn_contexts: requested_vtr_authn_contexts,
force_authn: force_authn,
final_auth_request: final_auth_request,
service_provider: service_provider,
Expand Down
2 changes: 1 addition & 1 deletion app/services/attribute_asserter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def build
add_all_emails(attrs) if bundle.include? :all_emails
add_bundle(attrs) if should_add_proofed_attributes?
add_verified_at(attrs) if bundle.include?(:verified_at) && ial2_service_provider?
if authn_request.requested_vtr_authn_context.present?
if authn_request.requested_vtr_authn_contexts.present?
add_vot(attrs)
else
add_aal(attrs)
Expand Down
26 changes: 16 additions & 10 deletions app/services/saml_request_validator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,8 @@ def call(service_provider:, authn_context:, nameid_format:, authn_context_compar
FormResponse.new(success: valid?, errors: errors, extra: extra_analytics_attributes)
end

def parsed_vector_of_trust
return @parsed_vector_of_trust if defined?(@parsed_vector_of_trust)

@parsed_vector_of_trust = begin
Vot::Parser.new(vector_of_trust: vtr.first).parse if !vtr.blank?
rescue Vot::Parser::ParseException
nil
end
def biometric_comparison_requested?
!!parsed_vectors_of_trust&.any?(&:biometric_comparison?)
end

private
Expand All @@ -45,6 +39,18 @@ def extra_analytics_attributes
}
end

def parsed_vectors_of_trust
return @parsed_vectors_of_trust if defined?(@parsed_vectors_of_trust)

@parsed_vectors_of_trust = begin
if vtr.is_a?(Array) && !vtr.empty?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The Array class check is confusing to me, what happens if it is not an array?
I'd expect something like Array(vtr).map { ... } which would seamlessly handle arrays and single items

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I merged before I saw this comment. I think this could be cleared up with a simple #blank? check. We want to fall back to ACR values if it is an empty array or nil. I'll follow up in a new PR

vtr.map { |vot| Vot::Parser.new(vector_of_trust: vot).parse }
end
rescue Vot::Parser::ParseException
nil
end
end

# This checks that the SP matches something in the database
# SamlIdpAuthConcern#check_sp_active checks that it's currently active
def authorized_service_provider
Expand All @@ -65,7 +71,7 @@ def authorized_authn_context
end

def parsable_vtr
if !vtr.blank? && parsed_vector_of_trust.blank?
if !vtr.blank? && parsed_vectors_of_trust.blank?
errors.add(:authn_context, :unauthorized_authn_context, type: :unauthorized_authn_context)
end
end
Expand Down Expand Up @@ -95,7 +101,7 @@ def step_up_comparison?
end

def identity_proofing_requested?
return true if parsed_vector_of_trust&.identity_proofing?
return true if parsed_vectors_of_trust&.any?(&:identity_proofing?)

authn_context.each do |classref|
return true if Saml::Idp::Constants::IAL2_AUTHN_CONTEXTS.include?(classref)
Expand Down
Loading