Saml2WebSsoAuthenticationFilter does not follow standard patterns for request matching.

[fpagliar]

Describe the bug

The common pattern for filters is to provide the ability to set a custom RequestMatcher for them to execute.
This is the case for Saml2WebSsoAuthenticationRequestFilter and AbstractAuthenticationProcessingFilter.

Now Saml2WebSsoAuthenticationFilter extends AbstractAuthenticationProcessingFilter but creates its own custom private RequestMatcher from a String.
This is unnecessarily restrictive for the developer, and goes against the common pattern.

Not only that, but since AbstractAuthenticationProcessingFilter provides setRequiresAuthenticationRequestMatcher()
A developer can totally unknowingly set a different matcher on the parent.

Example

The following line compiles and looks valid to a developer using SpringSecurity, but creates an instance of the filter that is completely inconsistent.

final Saml2WebSsoAuthenticationFilter myFilter = new Saml2WebSsoAuthenticationFilter(getRelyingPartyRegistrationRepository());
myFilter.setRequiresAuthenticationRequestMatcher(new AntPathRequestMatcher("/someother"));

The method requiresAuthentication will match based on the parent matcher, but attemptAuthentication will match based on the local one.

[fpagliar]

I was looking and the old infrastructure (spring-security-saml) uses the same URL endpoint for authentication for every IdP and picks up the appropriate configuration info from the cache by matching based on the issuer ID.

That allows to have one SP config and multiple IdPs pointing with the exact same config. Is there a benefit for the current alternative of requiring a different endpoint per IdP config?

[jzheaux]

@fpagliar, it allows looking up the configuration without needing to first parse the assertion. I believe the other approach is valid, too, but perhaps discussing how to best address that is for another ticket.

For this ticket, it appears the private matcher is there simply because the parent class keeps the requiresAuthenticationRequestMatcher private. I believe it would resolve the issue to override setRequiresAuthenticationRequestMatcher to set both the local matcher and the parent's matcher.

Would you be able to submit a PR to bring those to member variables into alignment?

[fpagliar]

I can. I was wondering about the implementation though.
AbstractAuthenticationProcessingFilter.setRequiresAuthenticationRequestMatcher() is a final method, and that makes sense given that it is called from the constructor so it shouldn't be overrideable.

Any alternative I'm thinking of right now requires either redesigning that bit on the AbstractAuthenticationProcessingFilter or accepting/documenting some part of inconsistency. I'll sit on it for a bit and see what I come up with.