Skip to content

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

Merged
Sgtpluck merged 4 commits intomainfrom
dmm/validation-nil-error
Mar 5, 2025
Merged

Ensure validations work when there is no valid service provider#11946
Sgtpluck merged 4 commits intomainfrom
dmm/validation-nil-error

Conversation

@Sgtpluck
Copy link
Contributor

@Sgtpluck Sgtpluck commented Mar 5, 2025

🎫 Ticket

Link to the relevant ticket:
Validation nil error

🛠 Summary of changes

This change adds a guard for the authorized_authn_context validation. Before, if the service provider did not exist but the authn_context was requesting IdV, IALMax, or IAL2, the validation would throw a no method error.

@Sgtpluck Sgtpluck requested a review from n1zyy March 5, 2025 15:17
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
Contributor

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
Contributor Author

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

@Sgtpluck Sgtpluck merged commit 4fa77ac into main Mar 5, 2025
2 checks passed
@Sgtpluck Sgtpluck deleted the dmm/validation-nil-error branch March 5, 2025 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants