Skip to content
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.

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

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

Closed

Conversation

lasselindqvist
Copy link

@lasselindqvist lasselindqvist commented Jun 15, 2023

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

@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

@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 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
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 philwebb closed this in 094cc55 Jul 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: merge-with-amendments Needs some changes when we merge type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants