Spring Security InMemoryClientRegistrationRepository should accept an empty set of registrations

InMemoryClientRegistrationRepository can be constructed with either a List of ClientRegistrations or a Map of same. However, passing an empty list is a runtime error while passing an empty map is not.

Commit e554547593784f431723bb9c9f6a4ef9b77099c5 changed the assertion in the list case from notNull to notEmpty.

IMO, an empty repository is a perfectly valid use case, so the list constructor should also accept an empty list.

Comment From: jzheaux

@espenhw, thanks for the suggestion; however https://github.com/spring-projects/spring-security/commit/e554547593784f431723bb9c9f6a4ef9b77099c5 brings the var args and list constructors into alignment and both require the list to be non-empty. This is also consistent across the servlet and reactive versions.

Can you better explain your use case and why you need to be able to construct a repository with no values?

Comment From: espenhw

Once again: There is a constructor that takes a Map (https://docs.spring.io/spring-security/site/docs/current/api/org/springframework/security/oauth2/client/registration/InMemoryClientRegistrationRepository.html#%3Cinit%3E(java.util.Map)). That constructor accepts empty input, while the list- and varargs-taking constructors do not. It should be consistent.

As for why you'd want an empty registry: Imagine you have support for a configurable set of OIDC providers in addition to other authentication sources. Removing all the OIDC-related wiring just because there happens to be no OIDC providers configured isn't feasible.

Comment From: retroandchill

I'm going to echo that I just ran into this issue on our project, so this is absolutely a valid use case to have no OIDC providers. It should probably be changed from a program-killing assert to a warning log that can be used to help developers make sure the configs are getting loaded.

Comment From: jzheaux

That constructor accepts empty input, while the list- and varargs-taking constructors do not. It should be consistent.

I think you can look at this both ways. Consistency can be achieved by requiring that the map be non-empty. I'm not saying that you are wrong, just that I don't follow this argument.

Imagine you have support for a configurable set of OIDC providers in addition to other authentication sources. Removing all the OIDC-related wiring just because there happens to be no OIDC providers configured isn't feasible.

I think this is a reasonable use case; however, you cannot add and remove from InMemoryClientRegistrationRepository, so to achieve this use case, you'd need a custom implementation anyway.

It should probably be changed from a program-killing assert to a warning log that can be used to help developers make sure the configs are getting loaded.

I'm sorry this caused you trouble and also I appreciate the sentiment. That said, I feel that the earlier a developer can be advised of a problem, the better. If I could tell you at compile time, I would! :) Not allowing startup is an earlier alert than waiting for someone to notice the warning in the logs.

I'm going to close this for the time being. Creating an implementation where you can have zero providers is already only a few lines of code:

public class ZeroClientRegistrationRepositiory implements ClientRegistrationRepository, Iterable<ClientRegistration> {
    private final Map<String, ClientRegistration> registrations;

    public ZeroClientRegistrationRepository(List<ClientRegistrations> registrations) {
        this.registrations = new ArrayList<>(registrations);
    }

    @Override 
    public ClientRegistration findByRegistrationId(String registrationId) {
        return this.registrations.get(registrationId);
    }

    @Override
    public Iterator<ClientRegistration> iterator() {
        return this.registrations.iterator();
    }
}