ProviderManager should have a varargs constructor

[jzheaux]

oauth2ResourceServer() introduces a couple of authenticationManager() methods for complete control over the authentication mechanism:

http
    .oauth2ResourceServer()
        .jwt()
            .authenticationManager(...)

This can be leveraged to construct a custom JwtAuthenticationProvider:

AuthenticationProvider jwt = new JwtAuthenticationProvider(... ;

http
    .oauth2ResourceServer()
        .jwt()
            .authenticationManager(jwt::authenticate)

However, the method reference shortcut may seem a bit terse for some tastes.

As an alternative, code can construct an instance of ProviderManager:

AuthenticationProvider jwt = new JwtAuthenticationProvider(... ;

http
    .oauth2ResourceServer()
        .jwt()
            .authenticationManager(new ProviderManager(Arrays.asList(jwt)))

This could be improved by adding a varargs constructor to ProviderManager:

AuthenticationProvider jwt = new JwtAuthenticationProvider(... ;

http
    .oauth2ResourceServer()
        .jwt()
            .authenticationManager(new ProviderManager(jwt))

[ThomasVitale]

Hi @jzheaux, I would like to work on this issue.

[jzheaux]

@ThomasVitale it's yours

[ThomasVitale]

@jzheaux do you have any recommendation to handle the ambiguity when passing null to the constructor? Would it be ok to force a Collections.emptyList() argument instead of null in case of no AuthenticationProviders passed to the ProviderManager? What about backward compatibility?

[jzheaux]

@ThomasVitale good question

new ProviderManager(null)

is not a supported use of that class, so I don't think there's much to worry about in the way of disambiguation. That is, if folks are doing that for some reason, they are on their own to make it work as they upgrade.

That said, we ought to continue to correctly error when null values are provided, so you might consider adding something like:

if (providers.contains(null)) {
	throw new IllegalArgumentException(
			"providers cannot contain null values");
}

To the checkState() method.

Would it be ok to force a Collections.emptyList()

Probably not. Spring Security prefers to throw an exception in misconfiguration scenarios instead of trying to leniently interpret the code's intent.

[ThomasVitale]

@jzheaux thanks for your answer, it solved my doubts.

I have just created a PR. I have added the varargs constructor and updated the tests to use it, both to actually test the constructor and also to improve their readability.

The test checking the exception thrown when a null value was passed to the constructor is now using an empty collection to trigger the validation, making sure the exception is correctly thrown. I haven't added extra validation since it was already covered.