RelyingPartyRegistrations typically produces unusable registrationId

[OrangeDog]

Describe the bug
Methods such as RelyingPartyRegistrations.collectionFromMetadataLocation use the entity of the asserting party as the registrationId.

SAML entity IDs are usually URIs (and often a URL to their metadata), and Spring's SAML support (incl. the default login page) requires the registrationId to be added to a URL.

This typically results in an error when trying to access e.g. https://relyingparty.com/saml2/authenticate/https://assertingparty.com/SAML/metadata.xml

To Reproduce
Construct a RelyingPartyRegistrationRepository using RelyingPartyRegistrations without changing the default registrationId.

Expected behavior
registrationId should always be generated as URL-safe, or should always be escaped when used in a URL.

Workaround

List<RelyingPartyRegistration> registrations = RelyingPartyRegistrations.collectionFromMetadataLocation(url)
        .stream()
        .map(builder -> builder
                .entityId("https://relyingparty.com/saml-metadata.xml")
                // etc.
                .build())
        .map(reg -> reg.mutate()
                .registrationId(reg.getAssertingPartyDetails().getEntityId().replaceAll("[:/?#]+", "_"))
                .build())
        .toList();

Caveat
Using URLEncoder.encode to make it safe is not sufficient, as you still get a 400 Bad Request from a default Spring Boot server.

[jzheaux]

Thanks, @OrangeDog, for the suggestion. Note that this is covered in the JavaDoc:

* Note that by default the registrationId is set to be the given metadata location,
* but this will most often not be sufficient. To complete the configuration, most
* applications will also need to provide a registrationId, like so:

Still, I think this would be good to investigate and improve. I wanted to add this context so it's clear why I'm changing the ticket to an enhancement.

Using URLEncoder.encode to make it safe is not sufficient, as you still get a 400 Bad Request from a default Spring Boot server.

Hex encoding might be worth considering.

[OrangeDog]

The SAML Discovery protocol (which the previous extension implemented) uses a query parameter rather than a path fragment so that entity ids can be transferred safely. Is it possible to do that with the current authenticationRequestUri template? A standard URI encoding would work there.

[jzheaux]

Nice idea. That's not yet supported in the DSL, but you could achieve that by publishing the Saml2WebSsoAuthenticationRequestFilter in the filter chain and setting the RequestMatcher accordingly.

That said, the Spring Security RequestMatcher implementations don't match the query string. So to simplify that, we could add some internal logic to the DSL so folks can do:

saml2Login((saml2) -> saml2
    .authenticationRequestUri("/saml2/authenticate?entityID={registrationId}")

or similar.

[OrangeDog]

publishing the Saml2WebSsoAuthenticationRequestFilter in the filter chain and setting the RequestMatcher accordingly

That doesn't seem right. Surely right now you have to build and supply a Saml2AuthenticationRequestResolver to the DSL?

[OrangeDog]

Noting that something like this works great, falling through to the login page nicely to choose a registration.

@Override
public MatchResult matcher(HttpServletRequest request) {
    String registrationId = request.getParameter("registrationId");
    if (new AntPathRequestMatcher("/login").matches(request) && StringUtils.hasText(registrationId)) {
        return MatchResult.match(Map.of("registrationId", registrationId));
    } else {
        return MatchResult.notMatch();
    }
}