Skip to content

Ensure validations work when there is no valid service provider #11946

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.

Sign up for GitHub

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

Merged
Sgtpluck merged 4 commits into from
Mar 5, 2025
Merged

Ensure validations work when there is no valid service provider #11946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2e255b1
changelog: Bug Fixes, SAML, Ensure validations work when there is no …
Sgtpluck Mar 5, 2025
1545660
Updates test to be clearer
Sgtpluck Mar 5, 2025
9a20fad
Fixes other test
Sgtpluck Mar 5, 2025
e96d60b
Fixes one more test that snuck through
Sgtpluck Mar 5, 2025
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
5 changes: 4 additions & 1 deletion app/services/saml_request_validator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ def authorized_service_provider
end

def authorized_authn_context
# if there is no service provider, an error has already been added
return unless service_provider.present?

if !valid_authn_context? ||
(identity_proofing_requested? && !service_provider.identity_proofing_allowed?) ||
(ial_max_requested? && !service_provider.ialmax_allowed?) ||
Expand All @@ -74,7 +77,7 @@ def parsable_vtr
end

def registered_cert_exists
# if there is no service provider, this error has already been added
# if there is no service provider, an error has already been added
return if service_provider.blank?
return if service_provider.certs.present?
return unless service_provider.encrypt_responses?
Expand Down
4 changes: 1 addition & 3 deletions spec/controllers/saml_idp_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1418,14 +1418,12 @@ def name_id_version(format_urn)

expect(controller).to render_template('saml_idp/auth/error')
expect(response.status).to eq(400)
expect(response.body).to include(t('errors.messages.unauthorized_authn_context'))
expect(response.body).to include(t('errors.messages.unauthorized_service_provider'))
expect(@analytics).to have_logged_event(
'SAML Auth',
hash_including(
success: false,
error_details: {
authn_context: { unauthorized_authn_context: true },
service_provider: { unauthorized_service_provider: true },
},
nameid_format: Saml::Idp::Constants::NAME_ID_FORMAT_PERSISTENT,
Expand All @@ -1440,7 +1438,7 @@ def name_id_version(format_urn)
)
expect(@analytics).to have_logged_event(
:sp_integration_errors_present,
error_details: ['Unauthorized Service Provider', 'Unauthorized authentication context'],
error_details: ['Unauthorized Service Provider'],
error_types: { saml_request_errors: true },
event: :saml_auth_request,
integration_exists: false,
Expand Down
39 changes: 37 additions & 2 deletions spec/services/saml_request_validator_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@
end

context 'valid authn context and invalid sp and authorized nameID format' do
let(:sp) { ServiceProvider.find_by(issuer: 'foo') }
let(:sp) { nil }

it 'returns FormResponse with success: false' do
expect(response.to_h).to eq(
Expand All @@ -96,6 +96,42 @@
**extra,
)
end

context 'when identity proofing is requested' do
let(:authn_context) { [Saml::Idp::Constants::IAL_VERIFIED_ACR] }

it 'returns FormResponse with success: false' do
expect(response.to_h).to eq(
success: false,
error_details: { service_provider: { unauthorized_service_provider: true } },
Copy link
Copy Markdown
Contributor

@n1zyy n1zyy Mar 5, 2025

Choose a reason for hiding this comment

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

I am diving into the weeds and think this is out of scope, but it took me a bit to realize that this bit on line 90:

let(:sp) { ServiceProvider.find_by(issuer: 'foo') }

returns nil because it finds nothing. Not knowing this section of code well, I guessed it was a ServiceProvider which existed, but hadn't been "authorized", whatever that might mean.

Would it be more expressive if it was just let(:sp) { nil }? Or I guess the point is that something was passed for an issuer, but it didn't match a real SP?

This PR is a simple bugfix + adding a regression test, so please don't let me derail it unless this sparks something on your end.

Copy link
Copy Markdown
Contributor Author

@Sgtpluck Sgtpluck Mar 5, 2025

Choose a reason for hiding this comment

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

updated in 1545660 -- i think there is some value to having it mirror the actual thing being passed in, but not at the expense of being clear to the folks reading the tests

**extra,
)
end
end

context 'when IALmax is requested' do
let(:authn_context) { [Saml::Idp::Constants::IALMAX_AUTHN_CONTEXT_CLASSREF] }

it 'returns FormResponse with success: false' do
expect(response.to_h).to eq(
success: false,
error_details: { service_provider: { unauthorized_service_provider: true } },
**extra,
)
end
end

context 'when facial matching is requested' do
let(:authn_context) { [Saml::Idp::Constants::IAL_VERIFIED_FACIAL_MATCH_REQUIRED_ACR] }

it 'returns FormResponse with success: false' do
expect(response.to_h).to eq(
success: false,
error_details: { service_provider: { unauthorized_service_provider: true } },
**extra,
)
end
end
end

context 'valid authn context and unauthorized nameid format' do
Expand Down Expand Up @@ -302,7 +338,6 @@
expect(response.to_h).to eq(
success: false,
error_details: {
authn_context: { unauthorized_authn_context: true },
service_provider: { unauthorized_service_provider: true },
},
**extra,
Expand Down