From e0d4a581446945eb7955c7276be8be29d2008242 Mon Sep 17 00:00:00 2001 From: Vraj Mohan Date: Wed, 29 May 2024 11:59:41 -0700 Subject: [PATCH] Refactor NameID format related tests **Why**: 1. The tests are hard to understand and do not clearly show the different contexts 2. The tests assert on a side effect (the analytics event posted) rather than the actual desired behavior (the return values) 3. The tests are actually misleading - they conflate "unspecified" and unsupported **How**: - Refactor tests with clear contexts and shared examples - Assert on actual return value and not on a side effect - Clean up test case descriptions - Assert that the correct attributes are logged in the analytics event changelog: Internal, Automated Testing, Refactor NameID format related tests --- lib/saml_idp_constants.rb | 1 + spec/controllers/saml_idp_controller_spec.rb | 360 +++++++------------ 2 files changed, 132 insertions(+), 229 deletions(-) diff --git a/lib/saml_idp_constants.rb b/lib/saml_idp_constants.rb index b50d4e02454..46b37ae8bb4 100644 --- a/lib/saml_idp_constants.rb +++ b/lib/saml_idp_constants.rb @@ -30,6 +30,7 @@ module Constants NAME_ID_FORMAT_PERSISTENT = 'urn:oasis:names:tc:SAML:2.0:nameid-format:persistent' NAME_ID_FORMAT_EMAIL = 'urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress' + NAME_ID_FORMAT_UNSPECIFIED = 'urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified' VALID_NAME_ID_FORMATS = [NAME_ID_FORMAT_PERSISTENT, NAME_ID_FORMAT_EMAIL].freeze REQUESTED_ATTRIBUTES_CLASSREF = 'http://idmanagement.gov/ns/requested_attributes?ReqAttr=' diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index 3c92c442849..e7cebb5d1ca 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -1,6 +1,6 @@ require 'rails_helper' -RSpec.describe SamlIdpController, allowed_extra_analytics: [:*] do +RSpec.describe SamlIdpController do include SamlAuthHelper render_views @@ -121,7 +121,7 @@ # the RubySAML library won't let us pass an empty string in as the certificate # element, so this test substitutes a SAMLRequest that has that element blank let(:blank_cert_element_req) do - <<-XML.gsub(/^[\s\t]*|[\s\t]*\n/, '') + <<-XML.gsub(/^[\s]+|[\s]+\n/, '') http://localhost:3000 @@ -1441,52 +1441,6 @@ def name_id_version(format_urn) end end - context 'service provider uses email NameID format and is allowed to use email' do - let(:user) { create(:user, :fully_registered) } - - before do - settings = saml_settings( - overrides: { - issuer: sp1_issuer, - name_identifier_format: Saml::Idp::Constants::NAME_ID_FORMAT_EMAIL, - }, - ) - ServiceProvider. - find_by(issuer: settings.issuer). - update!(email_nameid_format_allowed: true) - generate_saml_response(user, settings) - end - - # Testing the element when the SP is configured to use a - # NameID format of emailAddress rather than the default persistent UUID. - context 'Subject' do - let(:subject) do - xmldoc.subject_nodeset[0] - end - - it 'has a saml:Subject element' do - expect(subject).to_not be_nil - end - - context 'NameID' do - let(:name_id) { subject.at('//ds:NameID', ds: Saml::XML::Namespaces::ASSERTION) } - - it 'has a saml:NameID element' do - expect(name_id).to_not be_nil - end - - it 'has a format attribute defining the NameID to be email' do - expect(name_id.attributes['Format'].value). - to eq(Saml::Idp::Constants::NAME_ID_FORMAT_EMAIL) - end - - it 'has NameID value of the email address of the user making the AuthN Request' do - expect(name_id.children.first.to_s).to eq(user.email) - end - end - end - end - context 'no matching cert from the SAML request' do let(:user) { create(:user, :fully_registered) } @@ -1550,7 +1504,7 @@ def name_id_version(format_urn) # the RubySAML library won't let us pass an empty string in as the certificate # element, so this test substitutes a SAMLRequest that has that element blank let(:blank_cert_element_req) do - <<-XML.gsub(/^[\s\t]*|[\s\t]*\n/, '') + <<-XML.gsub(/^[\s]+|[\s]+\n/, '') http://localhost:3000 @@ -1665,222 +1619,170 @@ def name_id_version(format_urn) end end - context 'nameid_format is missing' do + describe 'NameID format' do let(:user) { create(:user, :fully_registered) } + let(:subject_element) { xmldoc.subject_nodeset[0] } + let(:name_id) { subject_element.at('//ds:NameID', ds: Saml::XML::Namespaces::ASSERTION) } + let(:auth_settings) { saml_settings(overrides: { name_identifier_format: }) } + let(:name_identifier_format) { nil } + let(:email_allowed) { nil } + let(:use_legacy_name_id_behavior) { nil } before do stub_analytics - allow(@analytics).to receive(:track_event) + service_provider = ServiceProvider.find_by(issuer: auth_settings.issuer) + IdentityLinker.new(user, service_provider).link_identity + service_provider.update!( + use_legacy_name_id_behavior:, + email_nameid_format_allowed: email_allowed, + ) end - it 'defaults to persistent' do - auth_settings = saml_settings(overrides: { name_identifier_format: nil }) - service_provider = build(:service_provider, issuer: auth_settings.issuer) - IdentityLinker.new(user, service_provider).link_identity - user.identities.last.update!(verified_attributes: ['email']) - generate_saml_response(user, auth_settings) + shared_examples_for 'sends the UUID' do + it 'sends the UUID' do + generate_saml_response(user, auth_settings) - expect(response.status).to eq(200) + expect(response.status).to eq(200) + expect(name_id.attributes['Format'].value). + to eq(Saml::Idp::Constants::NAME_ID_FORMAT_PERSISTENT) + expect(name_id.children.first.to_s).to eq(user.last_identity.uuid) + expect(@analytics).to have_logged_event( + 'SAML Auth', + hash_including( + { nameid_format: Saml::Idp::Constants::NAME_ID_FORMAT_PERSISTENT, success: true }, + ), + ) + end + end - analytics_hash = { - success: true, - errors: {}, - error_details: nil, - nameid_format: Saml::Idp::Constants::NAME_ID_FORMAT_PERSISTENT, - authn_context: request_authn_contexts, - authn_context_comparison: 'exact', - requested_ial: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, - service_provider: 'http://localhost:3000', - endpoint: "/api/saml/auth#{path_year}", - idv: false, - finish_profile: false, - request_signed: true, - matching_cert_serial: saml_test_sp_cert_serial, - } + shared_examples_for 'sends the email' do + it 'sends the email' do + generate_saml_response(user, auth_settings) - expect(@analytics).to have_received(:track_event). - with('SAML Auth', analytics_hash) + expect(response.status).to eq(200) + expect(name_id.attributes['Format'].value). + to eq(Saml::Idp::Constants::NAME_ID_FORMAT_EMAIL) + expect(name_id.children.first.to_s).to eq(user.email) + expect(@analytics).to have_logged_event( + 'SAML Auth', + hash_including( + { nameid_format: Saml::Idp::Constants::NAME_ID_FORMAT_EMAIL, success: true }, + ), + ) + end end - it 'defaults to email when added to issuers_with_email_nameid_format' do - auth_settings = saml_settings( - overrides: { - issuer: sp1_issuer, - name_identifier_format: nil, - }, - ) - ServiceProvider. - find_by(issuer: auth_settings.issuer). - update!(email_nameid_format_allowed: true) - IdentityLinker.new(user, sp1).link_identity - user.identities.last.update!(verified_attributes: ['email']) - generate_saml_response(user, auth_settings) + shared_examples_for 'returns an unauthorized nameid error' do |invalid_format| + it 'returns an error' do + generate_saml_response(user, auth_settings) - expect(response.status).to eq(200) + expect(controller).to render_template('saml_idp/auth/error') + expect(response.status).to eq(400) + expect(response.body).to include(t('errors.messages.unauthorized_nameid_format')) + expect(@analytics).to have_logged_event( + 'SAML Auth', + hash_including( + { nameid_format: invalid_format, success: false }, + ), + ) + end + end - analytics_hash = { - success: true, - errors: {}, - error_details: nil, - nameid_format: Saml::Idp::Constants::NAME_ID_FORMAT_EMAIL, - authn_context: request_authn_contexts, - authn_context_comparison: 'exact', - requested_ial: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, - service_provider: auth_settings.issuer, - endpoint: "/api/saml/auth#{path_year}", - idv: false, - finish_profile: false, - request_signed: true, - matching_cert_serial: saml_test_sp_cert_serial, - } + context 'when the NameID format has the value "unspecified"' do + let(:name_identifier_format) { Saml::Idp::Constants::NAME_ID_FORMAT_UNSPECIFIED } - expect(@analytics).to have_received(:track_event). - with('SAML Auth', analytics_hash) - end - end + context 'when the service provider is not configured with use_legacy_name_id_behavior' do + let(:use_legacy_name_id_behavior) { false } - context 'service provider uses email NameID format but is not allowed to use email' do - it 'returns an error' do - stub_analytics - allow(@analytics).to receive(:track_event) + it_behaves_like 'sends the UUID' + end + context 'when the service provider is configured with use_legacy_name_id_behavior' do + let(:use_legacy_name_id_behavior) { true } + it 'sends the id, not the UUID' do + generate_saml_response(user, auth_settings) - auth_settings = saml_settings( - overrides: { name_identifier_format: Saml::Idp::Constants::NAME_ID_FORMAT_EMAIL }, - ) - saml_get_auth(auth_settings) + expect(response.status).to eq(200) - expect(controller).to render_template('saml_idp/auth/error') - expect(response.status).to eq(400) - expect(response.body).to include(t('errors.messages.unauthorized_nameid_format')) + expect(name_id.attributes['Format'].value). + to eq(Saml::Idp::Constants::NAME_ID_FORMAT_PERSISTENT) - analytics_hash = { - success: false, - errors: { nameid_format: [t('errors.messages.unauthorized_nameid_format')] }, - error_details: { nameid_format: { unauthorized_nameid_format: true } }, - nameid_format: Saml::Idp::Constants::NAME_ID_FORMAT_EMAIL, - authn_context: request_authn_contexts, - authn_context_comparison: 'exact', - service_provider: 'http://localhost:3000', - request_signed: true, - requested_ial: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, - endpoint: "/api/saml/auth#{path_year}", - idv: false, - finish_profile: false, - matching_cert_serial: saml_test_sp_cert_serial, - } + expect(name_id.children.first.to_s).to eq(user.id.to_s) + end + end + end - expect(@analytics).to have_received(:track_event). - with('SAML Auth', analytics_hash) + context 'when the NameID format is missing' do + let(:name_identifier_format) { nil } + + context 'when the service provider is not configured with use_legacy_name_id_behavior' do + let(:use_legacy_name_id_behavior) { false } + + it_behaves_like 'sends the UUID' + end + context 'when the service provider is configured with use_legacy_name_id_behavior' do + let(:use_legacy_name_id_behavior) { true } + it_behaves_like 'sends the UUID' + end end - end - context 'service provider sends unsupported NameID format' do - let(:user) { create(:user, :fully_registered) } - let(:xmldoc) { SamlResponseDoc.new('controller', 'response_assertion', response) } - let(:subject) { xmldoc.subject_nodeset[0] } - let(:name_id) { subject.at('//ds:NameID', ds: Saml::XML::Namespaces::ASSERTION) } + context 'when the NameID format is "persistent"' do + let(:name_identifier_format) { Saml::Idp::Constants::NAME_ID_FORMAT_PERSISTENT } - before do - stub_analytics - allow(@analytics).to receive(:track_event) + it_behaves_like 'sends the UUID' end - it 'sends the appropriate identifier for non-email NameID SPs' do - auth_settings = saml_settings(overrides: { name_identifier_format: nil }) - auth_settings.name_identifier_format = - 'urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified' - service_provider = build(:service_provider, issuer: auth_settings.issuer) - IdentityLinker.new(user, service_provider).link_identity - user.identities.last.update!(verified_attributes: ['email']) - generate_saml_response(user, auth_settings) + context 'when the NameID format is "email"' do + let(:name_identifier_format) { Saml::Idp::Constants::NAME_ID_FORMAT_EMAIL } - expect(response.status).to eq(200) + context 'when the service provider is not allowed to use email' do + let(:email_allowed) { false } - analytics_hash = { - success: true, - errors: {}, - error_details: nil, - nameid_format: Saml::Idp::Constants::NAME_ID_FORMAT_PERSISTENT, - authn_context: request_authn_contexts, - authn_context_comparison: 'exact', - requested_ial: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, - service_provider: 'http://localhost:3000', - endpoint: "/api/saml/auth#{path_year}", - idv: false, - finish_profile: false, - request_signed: true, - matching_cert_serial: saml_test_sp_cert_serial, - } + it_behaves_like 'returns an unauthorized nameid error', + Saml::Idp::Constants::NAME_ID_FORMAT_EMAIL + end - expect(name_id.children.first.to_s).to eq(user.agency_identities.last.uuid) - expect(@analytics).to have_received(:track_event). - with('SAML Auth', analytics_hash) + context 'when the service provider is allowed to use email' do + let(:email_allowed) { true } + it_behaves_like 'sends the email' + end end - it 'sends the appropriate identifier for email NameID SPs' do - auth_settings = saml_settings(overrides: { name_identifier_format: nil }) - auth_settings.name_identifier_format = - 'urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified' - service_provider = ServiceProvider.find_by(issuer: auth_settings.issuer) - service_provider.update!(email_nameid_format_allowed: true) - IdentityLinker.new(user, service_provider).link_identity - user.identities.last.update!(verified_attributes: ['email']) - generate_saml_response(user, auth_settings) + context 'when the NameID format is an unsupported value' do + let(:name_identifier_format) { 'urn:oasis:names:tc:SAML:1.1:nameid-format:transient' } + let(:use_legacy_name_id_behavior) { nil } - expect(response.status).to eq(200) + context 'when the service provider is not configured with use_legacy_name_id_behavior' do + # This should always return an error. An aborted attempt was made to fix this + # with 30cb0f374 + let(:use_legacy_name_id_behavior) { false } - analytics_hash = { - success: true, - errors: {}, - error_details: nil, - nameid_format: Saml::Idp::Constants::NAME_ID_FORMAT_EMAIL, - authn_context: request_authn_contexts, - authn_context_comparison: 'exact', - requested_ial: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, - service_provider: auth_settings.issuer, - endpoint: "/api/saml/auth#{path_year}", - idv: false, - finish_profile: false, - request_signed: true, - matching_cert_serial: saml_test_sp_cert_serial, - } + context 'when the service provider is not allowed to use email' do + let(:email_allowed) { false } - expect(name_id.children.first.to_s).to eq(user.email_addresses.first.email) - expect(@analytics).to have_received(:track_event). - with('SAML Auth', analytics_hash) - end + it_behaves_like 'sends the UUID' + end - it 'sends the old user ID for legacy SPS' do - auth_settings = saml_settings(overrides: { name_identifier_format: nil }) - auth_settings.name_identifier_format = - 'urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified' - service_provider = ServiceProvider.find_by(issuer: auth_settings.issuer) - service_provider.update!(use_legacy_name_id_behavior: true) - IdentityLinker.new(user, service_provider).link_identity - user.identities.last.update!(verified_attributes: ['email']) - generate_saml_response(user, auth_settings) + context 'when the service provider is allowed to use email' do + let(:email_allowed) { true } - expect(response.status).to eq(200) + it_behaves_like 'sends the email' + end + end - analytics_hash = { - success: true, - errors: {}, - error_details: nil, - nameid_format: 'urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified', - authn_context: request_authn_contexts, - authn_context_comparison: 'exact', - requested_ial: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, - service_provider: 'http://localhost:3000', - endpoint: "/api/saml/auth#{path_year}", - idv: false, - finish_profile: false, - request_signed: true, - matching_cert_serial: saml_test_sp_cert_serial, - } + context 'when the service provider is configured with use_legacy_name_id_behavior' do + let(:use_legacy_name_id_behavior) { true } - expect(name_id.children.first.to_s).to eq(user.id.to_s) - expect(@analytics).to have_received(:track_event). - with('SAML Auth', analytics_hash) + it 'sends the id, not the UUID' do + generate_saml_response(user, auth_settings) + + expect(response.status).to eq(200) + + expect(name_id.attributes['Format'].value). + to eq(Saml::Idp::Constants::NAME_ID_FORMAT_PERSISTENT) + + expect(name_id.children.first.to_s).to eq(user.id.to_s) + end + end end end