Skip to content

Saml2RelyingPartyRegistrationConfiguration can choose the wrong RelyingPartyRegistration.Builder when using a metadata file with multiple providers #35902

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

Jump to bottom
Closed
lasselindqvist wants to merge 1 commit into from
Closed

Saml2RelyingPartyRegistrationConfiguration can choose the wrong RelyingPartyRegistration.Builder when using a metadata file with multiple providers #35902

lasselindqvist wants to merge 1 commit into from

Conversation

@lasselindqvist
Copy link

@lasselindqvist lasselindqvist commented Jun 15, 2023 •
edited by philwebb
Loading

Saml2RelyingPartyRegistrationConfiguration chooses the wrong RelyingPartyRegistration.Builder when using a metadata file with multiple providers. It always just chooses the first one it finds, when it actually should chooses the one with the correct entity-id.

The method now is:

	private RelyingPartyRegistration asRegistration(String id, Registration properties) {
		AssertingPartyProperties assertingParty = new AssertingPartyProperties(properties, id);
		boolean usingMetadata = StringUtils.hasText(assertingParty.getMetadataUri());
		Builder builder = (usingMetadata)
				? RelyingPartyRegistrations.fromMetadataLocation(assertingParty.getMetadataUri()).registrationId(id)
				: RelyingPartyRegistration.withRegistrationId(id);
....

when it could be for example:

	private RelyingPartyRegistration asRegistration(String id, Registration properties) {
		AssertingPartyProperties assertingParty = new AssertingPartyProperties(properties, id);
		boolean usingMetadata = StringUtils.hasText(assertingParty.getMetadataUri());
		Builder builder = (usingMetadata)
				? RelyingPartyRegistrations.collectionFromMetadataLocation(assertingParty.getMetadataUri())
				.stream().filter(builder -> properties.getEntityId().equals(builder.build().getEntityId()))
				.registrationId(id)
				: RelyingPartyRegistration.withRegistrationId(id);
....

This can be tested with metadata files that have multiple providers, such as https://virtu-ds.csc.fi/fed/virtu/virtu-metadata-v7.xml

@pivotal-cla
Copy link

pivotal-cla commented Jun 15, 2023

@lasselindqvist Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

pivotal-cla commented Jun 15, 2023

@lasselindqvist Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 15, 2023
@philwebb philwebb changed the title Issue: #35901: Choose provider based on entity-id, not just the first from the metadata Saml2RelyingPartyRegistrationConfiguration can choose the wrong RelyingPartyRegistration.Builder when using a metadata file with multiple providers Jun 15, 2023
@philwebb philwebb added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 15, 2023
@philwebb philwebb added this to the 2.7.x milestone Jun 15, 2023
@philwebb philwebb added the for: merge-with-amendments Needs some changes when we merge label Jun 15, 2023
@philwebb philwebb self-assigned this Jun 15, 2023
@philwebb philwebb mentioned this pull request Jun 15, 2023
Closed
@philwebb philwebb mentioned this pull request Jul 2, 2023
Closed
@philwebb philwebb modified the milestones: 2.7.x, 2.7.14 Jul 2, 2023
@philwebb
Copy link
Member

philwebb commented Jul 2, 2023

Thanks very much @lasselindqvist, this is now in 2.7.x and merged forward to 3.0.x, 3.1.x and 3.2.x,

philwebb pushed a commit that referenced this pull request Jul 2, 2023
@philwebb
Choose SAML party based on entity ID rather than always using first
864af59 
Update `Saml2RelyingPartyRegistrationConfiguration` so that
`RelyingPartyRegistrations` uses `collectionFromMetadataLocation`
rather than `fromMetadataLocation` and searches candidates for a
matching entity ID.

Prior to this commit, it was possible for the wrong provider to be
used if multiple candidates existed in the returned metadata.

See gh-35902
philwebb added a commit that referenced this pull request Jul 2, 2023
@philwebb
Polish 'Choose SAML party based on entity ID rather than always using…
b699094 
… first'

See gh-35902
@philwebb philwebb closed this in 094cc55 Jul 2, 2023
@Netyyyy Netyyyy mentioned this pull request Aug 10, 2023
Closed
@Netyyyy Netyyyy mentioned this pull request Aug 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@philwebb philwebb

Labels

for: merge-with-amendments Needs some changes when we merge type: bug A general bug

Projects

None yet

Milestone

2.7.14

Development

Successfully merging this pull request may close these issues.

4 participants

@lasselindqvist @pivotal-cla @philwebb @spring-projects-issues