Skip to content

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

Closed
lasselindqvist wants to merge 1 commit intospring-projects:mainfrom
lasselindqvist:issue/35901
Closed

Saml2RelyingPartyRegistrationConfiguration can choose the wrong RelyingPartyRegistration.Builder when using a metadata file with multiple providers#35902
lasselindqvist wants to merge 1 commit intospring-projects:mainfrom
lasselindqvist:issue/35901

Conversation

@lasselindqvist
Copy link
Copy Markdown

@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
Copy Markdown

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
Copy Markdown

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
Copy Markdown
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