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

Consider adding RelyingPartyRegistrationResolver #8887

Closed
jzheaux opened this issue Jul 30, 2020 · 3 comments
Closed

Consider adding RelyingPartyRegistrationResolver #8887

jzheaux opened this issue Jul 30, 2020 · 3 comments
Assignees
Labels
in: saml2 An issue in SAML2 modules type: enhancement A general enhancement
Milestone

Comments

@jzheaux
Copy link
Contributor

jzheaux commented Jul 30, 2020

Saml2WebSsoAuthenticationFilter, Saml2WebSsoAuthenticationRequestFilter, and upcoming filters like Saml2MetadataFilter and any request-based logout support will all need to look up the RelyingPartyRegistration.

Currently, the way that Saml2WebSsoAuthenticationFilter and Saml2WebSsoAuthenticationRequestFilter look up the registration is by using a pattern in the URL path to discover the the registration id and then querying the RelyingPartyRegistrationRepository.

As was noted in a separate ticket, there's value in being able to derive the registration in ways other than by a path placeholder. For example, some may derive it via some other request aspect like the host name. Using other strategies to derive the registration can also simplify IdP configuration by making the IdP only need to know about one set of paths for the SP.

Since this strategy would need to be configured on each filter, it's reasonable to consider a shared strategy like Converter<HttpServletRequest, RelyingPartyRegistration>. The default implementation would discover the registration id by path and look up the RelyingPartyRegistration by that registration id.

An additional benefit to consolidating this functionality is in resolving templates in RelyingPartyRegistration instances. The entityId and assertionConsumerServiceLocation are defined to allow placeholders, and these placeholders can only be resolved using the HttpServletRequest. This means that these values are not as useful at the service layer unless they are first resolved at the web layer. This default resolver, then, could also resolve the templates in the RelyingPartyRegistration so that they can have meaningful values at the service layer.

If added, this would either be valuable as a component of each filter, thereafter passing the RelyingPartyRegistration to the appropriate collaborators, or those collaborators themselves could instead use this component, simplifying their contract.

For example, Saml2AuthenticationRequestContextResolver, currently has a resolve method:

public Saml2AuthenticationRequestContext resolve(
        HttpServletRequest request, RelyingPartyRegistration registration)

Given this new component, the contract could be simplified to:

public Saml2AuthenticationRequestContext resolve(HttpServletRequest request)

and Saml2AuthenticationRequestContextResolver would simply delegate to this new component to retrieve the registration.

One benefit to changing the contract in this way is it makes it easier to consider things like a generic resolver interface since such an interface would likely take only one parameter.

@jzheaux jzheaux added type: enhancement A general enhancement in: saml2 An issue in SAML2 modules labels Jul 30, 2020
@jzheaux jzheaux self-assigned this Jul 30, 2020
@evgeniycheban
Copy link
Contributor

@jzheaux I'd like to work on it 👍

@fpagliar
Copy link

fpagliar commented Aug 3, 2020

Yes this is a big one in order to support an upgrade path for projects using spring-security-saml.

Right now, it is not possible to do so because it requires you to rewrite all your destination URLs to contain the ids, which would mean asking every customer to re-configure their SAML apps.

In the old schema, we had SP resolution using different structures:

  • local entity id as querystring argument
  • alias id
  • a default one

And for IdP it was mostly done based on the entityID of the SAML Message that was received. Which is problematic in the current structure because we determine the RelyingPartyConfiguration before parsing the message.

@jzheaux
Copy link
Contributor Author

jzheaux commented Aug 3, 2020

Actually, @evgeniycheban, I think I've got this one just about ready since I'm hoping to get this merged before RC1, which is scheduled for Wednesday morning.

I could use yours and @fpagliar's help on reviewing the PR, though.

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: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants