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

Reloading Saml2RelyingPartyRegistrations with help of Saml2RelyingPartyRegistrationConfiguration #23918

Open
dawi opened this issue Oct 27, 2020 · 8 comments
Labels
type: enhancement A general enhancement
Milestone

Comments

@dawi
Copy link

dawi commented Oct 27, 2020

The spring-security-saml extension provides a HTTPMetadataProvider which is able to automatically refresh SAML metadata in configurable intervals. It would be nice if this feature would also be supported by spring security, but I'm afraid that this feature is out of scope of this library (spring-projects/spring-security#9134).

This is ok, because this feature can be realized fairly simple by implementing a custom RelyingPartyRegistrationRepository.

However the user has to write a lot of code that is already part of Spring Boot and Spring Security internally (e.g. reading certificates, thinking about configuration properties, ...).

It would be nice if a user could just use the existing Spring Boot SAML configuration like this (example on github):

public ReloadingRelyingPartyRegistrationRepository(Saml2RelyingPartyProperties properties) {
    this.properties = properties;
    refreshRelyingPartyRegistrations();
}

@Scheduled(fixedDelayString = "${metadata-refresh-interval}", initialDelay = 10_000)
private void refreshRelyingPartyRegistrations() {

    LOGGER.debug("refreshRelyingPartyRegistrations");

    properties.getRegistration().forEach((registrationId, registrationProperties) -> {

        try {
            registrations.put(registrationId, asRegistration(registrationId, registrationProperties));
        }
        catch (Exception e) {
            LOGGER.warn("Could not refresh RelyingPartyRegistration configuration.", e);
        }
    });
}

My question is, would you consider making the private asRegistration(...) method from Saml2RelyingPartyRegistrationConfiguration in some form accessible to users, so that they don't have to repeat that code? Maybe not in Saml2RelyingPartyRegistrationConfiguration but in a separate utility class?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 27, 2020
@philwebb philwebb added for: team-attention An issue we'd like other members of the team to review type: enhancement A general enhancement and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Oct 27, 2020
@philwebb philwebb added this to the 2.x milestone Oct 28, 2020
@dawi
Copy link
Author

dawi commented Dec 16, 2020

@philwebb Just curious, are there any news regarding this issue? :)

@philwebb
Copy link
Member

Not much I'm afraid. We targeted it to 2.x which means it's a valid issue and something we'd like to fix, we just haven't had the time yet.

@dawi
Copy link
Author

dawi commented Jan 4, 2022

@philwebb How likely is it that this will be included in a 2.x release?
With guidance I can maybe help with a pull request.

@philwebb
Copy link
Member

A pull-request would be most welcome. There a few options for fixing this, and I'm not sure which would be best.

We could create a RelyingPartyRegistrationProperyMapper class that contains the existing asRegistration method. This would allow us to do something like:

properties.getRegistration().entrySet().stream().map(RelyingPartyRegistrationProperyMapper::asRegistration)

Another option might be to introduce a RelyingPartyRegistrationRepositoryFactory interface that is given a List<RelyingPartyRegistration> and returns a RelyingPartyRegistrationRepository instance. That would allow us to keep the auto-configuration logic hidden but still allow users to plugin a different RelyingPartyRegistrationRepository implementation.

Flagging for team attention to see if there's a preference.

@philwebb philwebb added the for: team-attention An issue we'd like other members of the team to review label Jan 10, 2022
@mbhave
Copy link
Contributor

mbhave commented Jun 27, 2022

Adding a RelyingPartyRegistrationRepositoryFactory could get confusing because then there would be two ways to supply a custom RelyingPartyRegistrationRepository . The auto-configuration would need to use the RelyingPartyRegistrationRepositoryFactory bean to create a RelyingPartyRegistrationRepository but back off in the presence of a user-provided RelyingPartyRegistrationRepository .

Given that it's cumbersome to get a list of RelyingPartyRegistrations from properties, moving asRegistration to a public util class would make sense.

@philwebb philwebb modified the milestones: 2.x, 3.x Aug 19, 2022
@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Aug 24, 2022
@tommai78101
Copy link

tommai78101 commented Jun 29, 2023

Just wondering, does the list of RelyingPartyRegistration really need to be loaded in from properties? Wouldn't it be nice if there is an alternate way of loading them in, such as loading from a database and adding the registration information iteratively?

@wilkinsona
Copy link
Member

@tommai78101 the list of RelyingPartyRegistration can be loaded from anywhere and RelyingPartyRegistrationRepository is the plug point that enables that. If you think that a JDBC-based implementation would be useful in addition to the InMemoryRelyingPartyRegistrationRepository that Spring Security provides, please open a Spring Security issue. There's nothing Spring Boot specific about a JDBC-based implementation so I don't think it belongs here.

@tommai78101
Copy link

the list of RelyingPartyRegistration can be loaded from anywhere and RelyingPartyRegistrationRepository is the plug point that enables that.

Oh, this is all I needed to know, the ability to load a collection of RelyingPartyRegistration externally. JDBC-based implementations aren't what I'm thinking of, as I was just making an example of loading data externally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

6 participants