Skip to content

RelyingPartyRegistrationResolvers should allow for the registration id to be specified #9486

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

Closed
jzheaux opened this issue Mar 4, 2021 · 3 comments
Assignees
Labels
in: saml2 An issue in SAML2 modules type: breaks-passivity A change that breaks passivity with the previous release type: enhancement A general enhancement
Milestone

Comments

@jzheaux
Copy link
Contributor

jzheaux commented Mar 4, 2021

There are are number of higher-level SAML 2.0 components that derive the registration id from the request. Examples include Saml2MetadataFilter, Saml2WebSsoAuthenticationFilter, and Saml2WebSsoAuthenticationRequestFilter, with more to come in the future.

On each request, RelyingPartyRegistrationResolver re-derives the registration id and usually needs to use an equivalent strategy. This complexifies some use cases since it then requires the derivation strategy in the resolver to sync up with the strategy in the higher-level component.

For example, by default the SAML 2.0 Response endpoint is configured to be /login/saml2/sso/{registrationId} in Saml2WebSsoAuthenticationFilter. Since the default resolver only takes an HttpServletRequest as a parameter, the resolver must re-perform the same request-matching logic in order to derive the registrationId for its purposes. Aside from the duplication of effort, this also means that if the application wants to customize the URL to be /{registrationId}/login/saml2/sso, for example, then it needs to be configured in both places.

Like OAuth2AuthorizationRequestResolver, these resolvers should allow higher-level components to specify the registration id when they have it, alleviating this duplication of effort.

Likely this will require introducing an interface, which should be named RelyingPartyRegistrationResolver. Components that use the Converter<HttpServletRequest, RelyingPartyRegistration> should adapt to this interface and should send any registration id that they have.

@jzheaux jzheaux added type: enhancement A general enhancement in: saml2 An issue in SAML2 modules labels Mar 4, 2021
@jzheaux jzheaux added this to the 5.5.0-M3 milestone Mar 4, 2021
@jzheaux jzheaux self-assigned this Mar 4, 2021
@jzheaux
Copy link
Contributor Author

jzheaux commented Mar 4, 2021

Related to #9133

jzheaux added a commit to jzheaux/spring-security that referenced this issue Mar 5, 2021
@eleftherias eleftherias modified the milestones: 5.5.0-M3, 5.5.0-RC1 Mar 15, 2021
@jzheaux
Copy link
Contributor Author

jzheaux commented Apr 12, 2021

The original impetus for this ticket is #8731 and since it has been reopened for additional research, we'll reopen this one as well, just in case changes there require changes here.

@jzheaux jzheaux reopened this Apr 12, 2021
@jzheaux jzheaux modified the milestones: 5.5.0-RC1, 5.6.0-M1 Apr 12, 2021
jzheaux added a commit to jzheaux/spring-security that referenced this issue Apr 15, 2021
@eleftherias eleftherias modified the milestones: 5.6.0-M1, 5.6.0-M2 Jul 19, 2021
@rwinch rwinch modified the milestones: 5.6.0-M2, 5.6.0-M3 Aug 16, 2021
akohli96 pushed a commit to akohli96/spring-security that referenced this issue Aug 25, 2021
jzheaux added a commit to jzheaux/spring-security that referenced this issue Sep 13, 2021
@jzheaux
Copy link
Contributor Author

jzheaux commented Sep 13, 2021

The introduction of this interface breaks passivity in a fairly isolated way.

If an application constructs a Saml2MetadataFilter like so:

Converter<HttpServletRequest, RelyingPartyRegistration> registrationResolver = 
        new DefaultRelyingPartyRegistrationResolver(registrations);
Saml2MetadataResolver metadataResolver = myMetadataResolver();
Saml2MetadataFilter filter = new Saml2MetadataFilter(registrationResolver, metadataResolver);

Then there will be no issue when updating to the latest.

However, if the construction of DefaultRelyingPartyRegistrationResolver is inlined:

Saml2MetadataResolver metadataResolver = myMetadataResolver();
Saml2MetadataFilter filter = new Saml2MetadataFilter(
        new DefaultRelyingPartyRegistrationResolver(registrations), 
        metadataResolver);

Then which constructor to be used is ambiguous since DefaultRelyingPartyRegistrationResolver implements both Converter<HttpServletRequest, RelyingPartyRegistration> and RelyingPartyRegistrationResolver.

The resolution is to place the construction of DefaultRelyingPartyRegistrationResolver onto its own line like so:

RelyingPartyRegistrationResolver registrationResolver = new DefaultRelyingPartyRegistrationResolver(registrations)
Saml2MetadataResolver metadataResolver = myMetadataResolver();
Saml2MetadataFilter filter = new Saml2MetadataFilter(registrationResolver, metadataResolver);

@jzheaux jzheaux added the type: breaks-passivity A change that breaks passivity with the previous release label Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: saml2 An issue in SAML2 modules type: breaks-passivity A change that breaks passivity with the previous release type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants