diff --git a/Gemfile b/Gemfile index 4bfc01fa764..0a851079286 100644 --- a/Gemfile +++ b/Gemfile @@ -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 diff --git a/Gemfile.lock b/Gemfile.lock index 2e22b77122b..7f219b5e301 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -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 diff --git a/app/controllers/concerns/saml_idp_auth_concern.rb b/app/controllers/concerns/saml_idp_auth_concern.rb index a40172b71ce..ed7b993576a 100644 --- a/app/controllers/concerns/saml_idp_auth_concern.rb +++ b/app/controllers/concerns/saml_idp_auth_concern.rb @@ -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 @@ -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 diff --git a/app/controllers/saml_idp_controller.rb b/app/controllers/saml_idp_controller.rb index db8368526eb..9a9de7d9068 100644 --- a/app/controllers/saml_idp_controller.rb +++ b/app/controllers/saml_idp_controller.rb @@ -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, diff --git a/app/models/federated_protocols/saml.rb b/app/models/federated_protocols/saml.rb index fdbc36d12c7..c0983064dde 100644 --- a/app/models/federated_protocols/saml.rb +++ b/app/models/federated_protocols/saml.rb @@ -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 diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index cd2b15c6291..2317b33810f 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -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 @@ -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:, @@ -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, diff --git a/app/services/attribute_asserter.rb b/app/services/attribute_asserter.rb index fe8fa35e188..c31ff8b81f3 100644 --- a/app/services/attribute_asserter.rb +++ b/app/services/attribute_asserter.rb @@ -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) diff --git a/app/services/saml_request_validator.rb b/app/services/saml_request_validator.rb index cddcd59198b..2b21aadc0bc 100644 --- a/app/services/saml_request_validator.rb +++ b/app/services/saml_request_validator.rb @@ -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 @@ -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? + 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 @@ -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 @@ -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) diff --git a/spec/features/sign_in/multiple_vot_spec.rb b/spec/features/sign_in/multiple_vot_spec.rb index 3eeba73c15f..80f4bbf59d7 100644 --- a/spec/features/sign_in/multiple_vot_spec.rb +++ b/spec/features/sign_in/multiple_vot_spec.rb @@ -1,6 +1,7 @@ require 'rails_helper' RSpec.feature 'Sign in with multiple vectors of trust', allowed_extra_analytics: [:*] do + include SamlAuthHelper include OidcAuthHelper include IdvHelper include DocAuthHelper @@ -9,115 +10,287 @@ allow(IdentityConfig.store).to receive(:doc_auth_selfie_capture_enabled).and_return(true) end - context 'biometric and non-biometric proofing is acceptable' do - scenario 'identity proofing is not required if user is proofed with biometric' do - user = create(:user, :proofed_with_selfie) + context 'with OIDC' do + context 'biometric and non-biometric proofing is acceptable' do + scenario 'identity proofing is not required if user is proofed with biometric' do + user = create(:user, :proofed_with_selfie) - visit_idp_from_oidc_sp_with_vtr(vtr: ['C1.C2.P1.Pb', 'C1.C2.P1']) - sign_in_live_with_2fa(user) + visit_idp_from_oidc_sp_with_vtr(vtr: ['C1.C2.P1.Pb', 'C1.C2.P1']) + sign_in_live_with_2fa(user) - expect(current_path).to eq(sign_up_completed_path) - click_agree_and_continue + expect(current_path).to eq(sign_up_completed_path) + click_agree_and_continue - user_info = OpenidConnectUserInfoPresenter.new(user.identities.last).user_info + user_info = OpenidConnectUserInfoPresenter.new(user.identities.last).user_info - expect(user_info[:given_name]).to be_present - expect(user_info[:vot]).to eq('C1.C2.P1.Pb') - end + expect(user_info[:given_name]).to be_present + expect(user_info[:vot]).to eq('C1.C2.P1.Pb') + end - scenario 'identity proofing is not required if user is proofed without biometric' do - user = create(:user, :proofed) + scenario 'identity proofing is not required if user is proofed without biometric' do + user = create(:user, :proofed) - visit_idp_from_oidc_sp_with_vtr(vtr: ['C1.C2.P1.Pb', 'C1.C2.P1']) - sign_in_live_with_2fa(user) + visit_idp_from_oidc_sp_with_vtr(vtr: ['C1.C2.P1.Pb', 'C1.C2.P1']) + sign_in_live_with_2fa(user) - expect(current_path).to eq(sign_up_completed_path) - click_agree_and_continue + expect(current_path).to eq(sign_up_completed_path) + click_agree_and_continue - user_info = OpenidConnectUserInfoPresenter.new(user.identities.last).user_info + user_info = OpenidConnectUserInfoPresenter.new(user.identities.last).user_info - expect(user_info[:given_name]).to be_present - expect(user_info[:vot]).to eq('C1.C2.P1') - end + expect(user_info[:given_name]).to be_present + expect(user_info[:vot]).to eq('C1.C2.P1') + end - scenario 'identity proofing with biometric is required if user is not proofed', :js do - user = create(:user, :fully_registered) + scenario 'identity proofing with biometric is required if user is not proofed', :js do + user = create(:user, :fully_registered) - visit_idp_from_oidc_sp_with_vtr(vtr: ['C1.C2.P1.Pb', 'C1.C2.P1']) - sign_in_live_with_2fa(user) + visit_idp_from_oidc_sp_with_vtr(vtr: ['C1.C2.P1.Pb', 'C1.C2.P1']) + sign_in_live_with_2fa(user) - expect(current_path).to eq(idv_welcome_path) - complete_all_doc_auth_steps_before_password_step(with_selfie: true) - fill_in 'Password', with: user.password - click_continue - acknowledge_and_confirm_personal_key + expect(current_path).to eq(idv_welcome_path) + complete_all_doc_auth_steps_before_password_step(with_selfie: true) + fill_in 'Password', with: user.password + click_continue + acknowledge_and_confirm_personal_key - expect(current_path).to eq(sign_up_completed_path) - click_agree_and_continue + expect(current_path).to eq(sign_up_completed_path) + click_agree_and_continue - user_info = OpenidConnectUserInfoPresenter.new(user.identities.last).user_info + user_info = OpenidConnectUserInfoPresenter.new(user.identities.last).user_info - expect(user_info[:given_name]).to be_present - expect(user_info[:vot]).to eq('C1.C2.P1.Pb') + expect(user_info[:given_name]).to be_present + expect(user_info[:vot]).to eq('C1.C2.P1.Pb') + end end - end - context 'proofing or no proofing is acceptable (IALMAX)' do - scenario 'identity proofing is not required if the user is not proofed' do - user = create(:user, :fully_registered) + context 'proofing or no proofing is acceptable (IALMAX)' do + scenario 'identity proofing is not required if the user is not proofed' do + user = create(:user, :fully_registered) - visit_idp_from_oidc_sp_with_vtr( - vtr: ['C1.C2.P1', 'C1.C2'], - scope: 'openid email profile:name', - ) - sign_in_live_with_2fa(user) + visit_idp_from_oidc_sp_with_vtr( + vtr: ['C1.C2.P1', 'C1.C2'], + scope: 'openid email profile:name', + ) + sign_in_live_with_2fa(user) - expect(current_path).to eq(sign_up_completed_path) - click_agree_and_continue + expect(current_path).to eq(sign_up_completed_path) + click_agree_and_continue - user_info = OpenidConnectUserInfoPresenter.new(user.identities.last).user_info + user_info = OpenidConnectUserInfoPresenter.new(user.identities.last).user_info - expect(user_info[:given_name]).to_not be_present - expect(user_info[:vot]).to eq('C1.C2') - end + expect(user_info[:given_name]).to_not be_present + expect(user_info[:vot]).to eq('C1.C2') + end - scenario 'attributes are shared if the user is proofed' do - user = create(:user, :proofed) + scenario 'attributes are shared if the user is proofed' do + user = create(:user, :proofed) - visit_idp_from_oidc_sp_with_vtr( - vtr: ['C1.C2.P1', 'C1.C2'], - scope: 'openid email profile:name', - ) - sign_in_live_with_2fa(user) + visit_idp_from_oidc_sp_with_vtr( + vtr: ['C1.C2.P1', 'C1.C2'], + scope: 'openid email profile:name', + ) + sign_in_live_with_2fa(user) - expect(current_path).to eq(sign_up_completed_path) - click_agree_and_continue + expect(current_path).to eq(sign_up_completed_path) + click_agree_and_continue - user_info = OpenidConnectUserInfoPresenter.new(user.identities.last).user_info + user_info = OpenidConnectUserInfoPresenter.new(user.identities.last).user_info - expect(user_info[:given_name]).to be_present - expect(user_info[:vot]).to eq('C1.C2.P1') - end + expect(user_info[:given_name]).to be_present + expect(user_info[:vot]).to eq('C1.C2.P1') + end + + scenario 'identity proofing is not required if proofed user resets password' do + user = create(:user, :proofed) - scenario 'identity proofing is not required if proofed user resets password' do - user = create(:user, :proofed) + visit_idp_from_oidc_sp_with_vtr( + vtr: ['C1.C2.P1', 'C1.C2'], + scope: 'openid email profile:name', + ) + trigger_reset_password_and_click_email_link(user.email) + reset_password(user, 'new even better password') + user.password = 'new even better password' + sign_in_live_with_2fa(user) - visit_idp_from_oidc_sp_with_vtr( - vtr: ['C1.C2.P1', 'C1.C2'], - scope: 'openid email profile:name', - ) - trigger_reset_password_and_click_email_link(user.email) - reset_password(user, 'new even better password') - user.password = 'new even better password' - sign_in_live_with_2fa(user) + expect(current_path).to eq(sign_up_completed_path) + click_agree_and_continue - expect(current_path).to eq(sign_up_completed_path) - click_agree_and_continue + user_info = OpenidConnectUserInfoPresenter.new(user.identities.last).user_info - user_info = OpenidConnectUserInfoPresenter.new(user.identities.last).user_info + expect(user_info[:given_name]).to_not be_present + expect(user_info[:vot]).to eq('C1.C2') + end + end + end + + context 'with SAML' do + before do + if javascript_enabled? + service_provider = ServiceProvider.find_by(issuer: sp1_issuer) + acs_url = URI.parse(service_provider.acs_url) + acs_url.host = page.server.host + acs_url.port = page.server.port + service_provider.update(acs_url: acs_url.to_s) + end + end + + context 'biometric and non-biometric proofing is acceptable' do + scenario 'identity proofing is not required if user is proofed with biometric' do + user = create(:user, :proofed_with_selfie) + + visit_saml_authn_request_url( + overrides: { issuer: sp1_issuer, authn_context: ['C1.C2.P1.Pb', 'C1.C2.P1'] }, + ) + sign_in_live_with_2fa(user) + + click_submit_default + expect(current_path).to eq(sign_up_completed_path) + click_agree_and_continue + click_submit_default + + xmldoc = SamlResponseDoc.new('feature', 'response_assertion') + expect(xmldoc.assertion_statement_node.content).to eq('C1.C2.P1.Pb') + expect(xmldoc.attribute_node_for('vot').content).to eq('C1.C2.P1.Pb') + + first_name = xmldoc.attribute_node_for('first_name').content + expect(first_name).to_not be_blank + end + + scenario 'identity proofing is not required if user is proofed without biometric' do + user = create(:user, :proofed) + + visit_saml_authn_request_url( + overrides: { issuer: sp1_issuer, authn_context: ['C1.C2.P1.Pb', 'C1.C2.P1'] }, + ) + sign_in_live_with_2fa(user) + + click_submit_default + expect(current_path).to eq(sign_up_completed_path) + click_agree_and_continue + click_submit_default + + xmldoc = SamlResponseDoc.new('feature', 'response_assertion') + expect(xmldoc.assertion_statement_node.content).to eq('C1.C2.P1') + expect(xmldoc.attribute_node_for('vot').content).to eq('C1.C2.P1') + + first_name = xmldoc.attribute_node_for('first_name').content + expect(first_name).to_not be_blank + end + + scenario 'identity proofing with biometric is required if user is not proofed', :js do + user = create(:user, :fully_registered) + + visit_saml_authn_request_url( + overrides: { issuer: sp1_issuer, authn_context: ['C1.C2.P1.Pb', 'C1.C2.P1'] }, + ) + sign_in_live_with_2fa(user) + + expect(current_path).to eq(idv_welcome_path) + complete_all_doc_auth_steps_before_password_step(with_selfie: true) + fill_in 'Password', with: user.password + click_continue + acknowledge_and_confirm_personal_key + + expect(current_path).to eq(sign_up_completed_path) + click_agree_and_continue + + xmldoc = SamlResponseDoc.new('feature', 'response_assertion') + expect(xmldoc.assertion_statement_node.content).to eq('C1.C2.P1.Pb') + expect(xmldoc.attribute_node_for('vot').content).to eq('C1.C2.P1.Pb') + + first_name = xmldoc.attribute_node_for('first_name').content + expect(first_name).to_not be_blank + end + end - expect(user_info[:given_name]).to_not be_present - expect(user_info[:vot]).to eq('C1.C2') + context 'proofing or no proofing is acceptable (IALMAX)' do + scenario 'identity proofing is not required if the user is not proofed' do + user = create(:user, :fully_registered) + + visit_saml_authn_request_url( + overrides: { + issuer: sp1_issuer, + authn_context: [ + 'C1.C2.P1', + 'C1.C2', + "#{Saml::Idp::Constants::REQUESTED_ATTRIBUTES_CLASSREF}first_name", + ], + }, + ) + sign_in_live_with_2fa(user) + + click_submit_default + expect(current_path).to eq(sign_up_completed_path) + click_agree_and_continue + click_submit_default + + xmldoc = SamlResponseDoc.new('feature', 'response_assertion') + expect(xmldoc.assertion_statement_node.content).to eq('C1.C2') + expect(xmldoc.attribute_node_for('vot').content).to eq('C1.C2') + + first_name_node = xmldoc.attribute_node_for('first_name') + expect(first_name_node).to be_nil + end + + scenario 'attributes are shared if the user is proofed' do + user = create(:user, :proofed) + + visit_saml_authn_request_url( + overrides: { + issuer: sp1_issuer, + authn_context: [ + 'C1.C2.P1', + 'C1.C2', + "#{Saml::Idp::Constants::REQUESTED_ATTRIBUTES_CLASSREF}first_name", + ], + }, + ) + sign_in_live_with_2fa(user) + + click_submit_default + expect(current_path).to eq(sign_up_completed_path) + click_agree_and_continue + click_submit_default + + xmldoc = SamlResponseDoc.new('feature', 'response_assertion') + expect(xmldoc.assertion_statement_node.content).to eq('C1.C2.P1') + expect(xmldoc.attribute_node_for('vot').content).to eq('C1.C2.P1') + + first_name = xmldoc.attribute_node_for('first_name').content + expect(first_name).to_not be_blank + end + + scenario 'identity proofing is not required if proofed user resets password' do + user = create(:user, :proofed) + + visit_saml_authn_request_url( + overrides: { + issuer: sp1_issuer, + authn_context: [ + 'C1.C2.P1', + 'C1.C2', + "#{Saml::Idp::Constants::REQUESTED_ATTRIBUTES_CLASSREF}first_name", + ], + }, + ) + trigger_reset_password_and_click_email_link(user.email) + reset_password(user, 'new even better password') + user.password = 'new even better password' + sign_in_live_with_2fa(user) + + click_submit_default + expect(current_path).to eq(sign_up_completed_path) + click_agree_and_continue + click_submit_default + + xmldoc = SamlResponseDoc.new('feature', 'response_assertion') + expect(xmldoc.assertion_statement_node.content).to eq('C1.C2') + expect(xmldoc.attribute_node_for('vot').content).to eq('C1.C2') + + first_name_node = xmldoc.attribute_node_for('first_name') + expect(first_name_node).to be_nil + end end end end diff --git a/spec/services/saml_request_validator_spec.rb b/spec/services/saml_request_validator_spec.rb index e4b63023f75..e1800ffb5de 100644 --- a/spec/services/saml_request_validator_spec.rb +++ b/spec/services/saml_request_validator_spec.rb @@ -302,6 +302,24 @@ end end + context 'multiple VTR for identity proofing with unauthorized SP for identity proofing' do + let(:authn_context) { ['C1', 'C1.P1'] } + before { sp.update!(ial: 1) } + + 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 + context 'valid VTR but VTR is disallowed by config' do let(:use_vot_in_sp_requests) { false } let(:authn_context) { ['C1'] } @@ -337,4 +355,38 @@ end end end + + describe '#biometric_comparison_requested?' do + let(:sp) { ServiceProvider.find_by(issuer: 'http://localhost:3000') } + let(:name_id_format) { Saml::Idp::Constants::NAME_ID_FORMAT_PERSISTENT } + let(:authn_context) { [] } + + subject(:validator) do + validator = SamlRequestValidator.new + validator.call( + service_provider: sp, + authn_context: authn_context, + nameid_format: name_id_format, + ) + validator + end + + context 'biometric comparison was requested' do + let(:authn_context) { ['C1.C2.P1.Pb'] } + + it { expect(subject.biometric_comparison_requested?).to eq(true) } + end + + context 'biometric comparison was not requested' do + let(:authn_context) { ['C1.C2.P1'] } + + it { expect(subject.biometric_comparison_requested?).to eq(false) } + end + + context 'biometric comparison requested with multiple vectors of trust' do + let(:authn_context) { ['C1.C2.P1', 'C1.C2.P1.Pb'] } + + it { expect(subject.biometric_comparison_requested?).to eq(true) } + end + end end diff --git a/spec/support/fake_saml_request.rb b/spec/support/fake_saml_request.rb index 02e6b633417..a9837156047 100644 --- a/spec/support/fake_saml_request.rb +++ b/spec/support/fake_saml_request.rb @@ -38,7 +38,7 @@ def requested_aal_authn_context Saml::Idp::Constants::AAL2_AUTHN_CONTEXT_CLASSREF end - def requested_vtr_authn_context + def requested_vtr_authn_contexts nil end