Spring Security Customizing the metadata endpoint does not work

Describe the bug

The documentation states that the metadata endpoint can be changed by like this:

filter.setRequestMatcher(new AntPathRequestMatcher("/saml2/metadata", "GET"));

But it does not work as expected because the registration id resolver returns metadata as registration id due to the used ant matcher.

https://github.com/spring-projects/spring-security/blob/6714112961335adad004979f7e38f580dc5bd004/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/web/DefaultRelyingPartyRegistrationResolver.java#L61

Comment From: jzheaux

Thanks, @dawi, for the report. I think we should update the documentation.

The filter by default expects the registrationId to be part of the URL. For example, you can customize the URL like so:

filter.setRequestMatcher(new AntPathRequestMatcher("/saml2/metadata/{registrationId}", "GET"));

Would you be able to provide a PR updating the documentation?

Comment From: dawi

Requiring the registrationId as part of the metadata URL would prevent transparent upgrades from spring-security-saml for those who have a metadata URL without registrationId, which seems to be the default there.

I understand that registrationId is required as part of the metadata URL if there are multiple RelyingPartyRegistrations in an application. But if there is only one, wouldn't it make sense to allow to omit it?

Update: I have found a really hacky workaround: One could use "metadata" as registrationId and then use "/saml2/{registrationId}" as AntPathRequestMatcher pattern. But I don't like it and I'm sure nobody want's that. :)

Update 2: If a custom RelyingPartyRegistrationRepository only holds a single RelyingPartyRegistration and the findByRegistrationId(String registrationId) method always returns that instance, then it is also possible to omit the registrationId because the name does not matter in this case. So from a user perspective this would be the way to go, although it feels like it only works as a side effect.

Comment From: dawi

Another thing I don't understand is that currently the URL should contain a registrationId placeholder like this:

filter.setRequestMatcher(new AntPathRequestMatcher("/saml2/metadata/{registrationId}", "GET"));

but this pattern seems not to be used to parse the registrationId in Saml2MetadataFilter. Instead the registrationId is parsed via RegistrationIdResolver in DefaultRelyingPartyRegistrationResolver which uses the non configurable URL pattern /**/{registrationId}.

So if someone want's to set a metadata url like /saml2/{registrationId}/metadata it would not work.

All in all, it would be nice if it could be simpler to configure the metadata url.

If spring security could offer such a solution, then maybe spring boot would adopt this too, and the metadata URL could also be configured via application.yml.

Comment From: jzheaux

All in all, it would be nice if it could be simpler to configure the metadata URL.

Do you have a proposal for how to make things simpler?

If a custom RelyingPartyRegistrationRepository only holds a single RelyingPartyRegistration and the findByRegistrationId(String registrationId) method always returns that instance, then it is also possible to omit the registrationId because the name does not matter in this case. So from a user perspective this would be the way to go, although it feels like it only works as a side effect.

The repository's API is intended to be reduceable to a single-tenant application, it's not a "happy accident".

In your situation, I see two possibilities:

First, introduce a rewrite rule so that the old URL points to the new one.
Second, rely on the fact that RelyingPartyRegistrationRepository always returns the same registration

There may be ways to simplify this as well for the other scenarios you outlined, and I'm open to your thoughts. Most things are private in DefaultRelyingPartyRegistrationResolver which gives us a lot of flexibility to change it.

Comment From: jzheaux

For the single-tenant scenario, you can replace the default resolver with a lambda, like so:

@Bean 
Filter metadata(RelyingPartyRegistrationRepository relyingPartyRegistrationRepository) {
    Saml2MetadataFilter metadata = new Saml2MetadataFilter(
        (request) -> relyingPartyRegistrationRepository.findByRegistrationId("single"),
        new OpenSamlMetadataResolver());
    metadata.setRequestMatcher(new AntMathRequestMatcher("/saml2/metadata", "GET"));
    return metadata;
}

This is about the same level of simplicity as the default Saml2MetadataFilter arrangement:

@Bean 
Filter metadata(RelyingPartyRegistrationRepository relyingPartyRegistrationRepository) {
    Saml2MetadataFilter metadata = new Saml2MetadataFilter(
        new DefaultRelyingPartyRegistrationResolver(relyingPartyRegistrationRepository),
        new OpenSamlMetadataResolver());
    return metadata;
}

For multi-tenant scenarios, it's important to remember that a path matcher may be used to both identify endpoints as well as resolve registration ids, but it's not required. The way to figure out the registration id may be completely different (for example, by subdomain). Any application can use a custom Converter<HttpServletRequest, RelyingPartyRegistration> to address that.

As reports of more use cases come in, I think improvements to DefaultRelyingPartyRegistrationResolver as an alternative to customization will become clearer. Until then, I'm inclined to leave it as-is.

I think the best step forward right now is to clarify the documentation.

Comment From: dawi

I think you are right. In retrospect, it is quite easy to configure, and yet very flexible. For now, updating the docs would probably be the best thing to do. :)