Skip to content

ProviderManager should have a varargs constructor #7806

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 30, 2020
Merged

ProviderManager should have a varargs constructor #7806

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26caa01
ProviderManager should have a varargs constructor
ThomasVitale Jan 30, 2020
fab4af2
Make code cleaner in ProviderManagerTests
ThomasVitale Jan 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions core/src/main/java/org/springframework/security/authentication/ProviderManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package org.springframework.security.authentication;

import java.util.Arrays;
import java.util.Collections;
import java.util.List;

Expand Down Expand Up @@ -104,6 +105,10 @@ public ProviderManager(List<AuthenticationProvider> providers) {
this(providers, null);
}

public ProviderManager(AuthenticationProvider... providers) {
this(Arrays.asList(providers), null);
}

public ProviderManager(List<AuthenticationProvider> providers,
AuthenticationManager parent) {
Assert.notNull(providers, "providers list cannot be null");
Expand All @@ -124,6 +129,9 @@ private void checkState() {
throw new IllegalArgumentException(
"A parent AuthenticationManager or a list "
+ "of AuthenticationProviders is required");
} else if (providers.contains(null)) {
throw new IllegalArgumentException(
"providers list cannot contain null values");
}
}

Expand Down
57 changes: 27 additions & 30 deletions core/src/test/java/org/springframework/security/authentication/ProviderManagerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,10 @@
*
* @author Ben Alex
*/
@SuppressWarnings("unchecked")
public class ProviderManagerTests {

@Test(expected = ProviderNotFoundException.class)
public void authenticationFailsWithUnsupportedToken() throws Exception {
public void authenticationFailsWithUnsupportedToken() {
Authentication token = new AbstractAuthenticationToken(null) {
public Object getCredentials() {
return "";
Expand All @@ -55,7 +54,7 @@ public Object getPrincipal() {
}

@Test
public void credentialsAreClearedByDefault() throws Exception {
public void credentialsAreClearedByDefault() {
UsernamePasswordAuthenticationToken token = new UsernamePasswordAuthenticationToken(
"Test", "Password");
ProviderManager mgr = makeProviderManager();
Expand All @@ -71,8 +70,7 @@ public void credentialsAreClearedByDefault() throws Exception {
@Test
public void authenticationSucceedsWithSupportedTokenAndReturnsExpectedObject() {
final Authentication a = mock(Authentication.class);
ProviderManager mgr = new ProviderManager(
Arrays.asList(createProviderWhichReturns(a)));
ProviderManager mgr = new ProviderManager(createProviderWhichReturns(a));
AuthenticationEventPublisher publisher = mock(AuthenticationEventPublisher.class);
mgr.setAuthenticationEventPublisher(publisher);

Expand All @@ -95,8 +93,13 @@ public void authenticationSucceedsWhenFirstProviderReturnsNullButSecondAuthentic
}

@Test(expected = IllegalArgumentException.class)
public void testStartupFailsIfProvidersNotSet() {
new ProviderManager(null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should still have a test for new ProviderManager((AuthenticationProvider) null) and for new ProviderManager((List<AuthenticationProvider>) null).

The reason is that an application might do:

AuthenticationProvider provider = constructProvider();
return new ProviderManager(provider);

but constructProvider returns null.

I believe this also implies that the extra validation logic in checkState() will turn out to be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks for the clarification. I have added the extra validation login in checkState() and tests for both null scenarios.

public void testStartupFailsIfProvidersNotSetAsList() {
new ProviderManager((List<AuthenticationProvider>) null);
}

@Test(expected = IllegalArgumentException.class)
public void testStartupFailsIfProvidersNotSetAsVarargs() {
new ProviderManager((AuthenticationProvider) null);
}

@Test
Expand All @@ -117,7 +120,7 @@ public boolean supports(Class<?> authentication) {
}
};

ProviderManager authMgr = new ProviderManager(Arrays.asList(provider));
ProviderManager authMgr = new ProviderManager(provider);

TestingAuthenticationToken request = createAuthenticationToken();
request.setDetails(requestDetails);
Expand All @@ -127,8 +130,7 @@ public boolean supports(Class<?> authentication) {
}

@Test
public void detailsAreSetOnAuthenticationTokenIfNotAlreadySetByProvider()
throws Exception {
public void detailsAreSetOnAuthenticationTokenIfNotAlreadySetByProvider() {
Object details = new Object();
ProviderManager authMgr = makeProviderManager();

Expand All @@ -144,8 +146,8 @@ public void detailsAreSetOnAuthenticationTokenIfNotAlreadySetByProvider()
public void authenticationExceptionIsIgnoredIfLaterProviderAuthenticates() {
final Authentication authReq = mock(Authentication.class);
ProviderManager mgr = new ProviderManager(
Arrays.asList(createProviderWhichThrows(new BadCredentialsException("",
new Throwable())), createProviderWhichReturns(authReq)));
createProviderWhichThrows(new BadCredentialsException("",
new Throwable())), createProviderWhichReturns(authReq));
assertThat(mgr.authenticate(mock(Authentication.class))).isSameAs(authReq);
}

Expand Down Expand Up @@ -180,7 +182,7 @@ public void accountStatusExceptionPreventsCallsToSubsequentProviders() {
}
catch (AccountStatusException expected) {
}
verifyZeroInteractions(otherProvider);
verifyNoInteractions(otherProvider);
}

@Test
Expand All @@ -189,7 +191,7 @@ public void parentAuthenticationIsUsedIfProvidersDontAuthenticate() {
Authentication authReq = mock(Authentication.class);
when(parent.authenticate(authReq)).thenReturn(authReq);
ProviderManager mgr = new ProviderManager(
Arrays.asList(mock(AuthenticationProvider.class)), parent);
Collections.singletonList(mock(AuthenticationProvider.class)), parent);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these cleanup items, too. Will you please put them in a separate commit (same PR)? That is, any code changes that aren't directly related to the new constructor, let's separate them into their own commit. This simplifies maintenance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, now they are in their own commit.

assertThat(mgr.authenticate(authReq)).isSameAs(authReq);
}

Expand All @@ -200,14 +202,14 @@ public void parentIsNotCalledIfAccountStatusExceptionIsThrown() {
});
AuthenticationManager parent = mock(AuthenticationManager.class);
ProviderManager mgr = new ProviderManager(
Arrays.asList(iThrowAccountStatusException), parent);
Collections.singletonList(iThrowAccountStatusException), parent);
try {
mgr.authenticate(mock(Authentication.class));
fail("Expected exception");
}
catch (AccountStatusException expected) {
}
verifyZeroInteractions(parent);
verifyNoInteractions(parent);
}

@Test
Expand All @@ -220,7 +222,7 @@ public void providerNotFoundFromParentIsIgnored() {
// Set a provider that throws an exception - this is the exception we expect to be
// propagated
ProviderManager mgr = new ProviderManager(
Arrays.asList(createProviderWhichThrows(new BadCredentialsException(""))),
Collections.singletonList(createProviderWhichThrows(new BadCredentialsException(""))),
parent);
mgr.setAuthenticationEventPublisher(publisher);

Expand All @@ -237,7 +239,7 @@ public void providerNotFoundFromParentIsIgnored() {
public void authenticationExceptionFromParentOverridesPreviousOnes() {
AuthenticationManager parent = mock(AuthenticationManager.class);
ProviderManager mgr = new ProviderManager(
Arrays.asList(createProviderWhichThrows(new BadCredentialsException(""))),
Collections.singletonList(createProviderWhichThrows(new BadCredentialsException(""))),
parent);
final Authentication authReq = mock(Authentication.class);
AuthenticationEventPublisher publisher = mock(AuthenticationEventPublisher.class);
Expand All @@ -257,12 +259,11 @@ public void authenticationExceptionFromParentOverridesPreviousOnes() {
}

@Test
@SuppressWarnings("deprecation")
public void statusExceptionIsPublished() {
AuthenticationManager parent = mock(AuthenticationManager.class);
final LockedException expected = new LockedException("");
ProviderManager mgr = new ProviderManager(
Arrays.asList(createProviderWhichThrows(expected)), parent);
Collections.singletonList(createProviderWhichThrows(expected)), parent);
final Authentication authReq = mock(Authentication.class);
AuthenticationEventPublisher publisher = mock(AuthenticationEventPublisher.class);
mgr.setAuthenticationEventPublisher(publisher);
Expand Down Expand Up @@ -298,10 +299,9 @@ public void providerThrowsInternalAuthenticationServiceException() {
@Test
public void authenticateWhenFailsInParentAndPublishesThenChildDoesNotPublish() {
BadCredentialsException badCredentialsExParent = new BadCredentialsException("Bad Credentials in parent");
ProviderManager parentMgr = new ProviderManager(
Collections.singletonList(createProviderWhichThrows(badCredentialsExParent)));
ProviderManager parentMgr = new ProviderManager(createProviderWhichThrows(badCredentialsExParent));
ProviderManager childMgr = new ProviderManager(Collections.singletonList(createProviderWhichThrows(
new BadCredentialsException("Bad Credentials in child"))), parentMgr);
new BadCredentialsException("Bad Credentials in child"))), parentMgr);

AuthenticationEventPublisher publisher = mock(AuthenticationEventPublisher.class);
parentMgr.setAuthenticationEventPublisher(publisher);
Expand Down Expand Up @@ -343,17 +343,14 @@ private TestingAuthenticationToken createAuthenticationToken() {
}

private ProviderManager makeProviderManager() {
MockProvider provider1 = new MockProvider();
List<AuthenticationProvider> providers = new ArrayList<>();
providers.add(provider1);

return new ProviderManager(providers);
MockProvider provider = new MockProvider();
return new ProviderManager(provider);
}

// ~ Inner Classes
// ==================================================================================================

private class MockProvider implements AuthenticationProvider {
private static class MockProvider implements AuthenticationProvider {
public Authentication authenticate(Authentication authentication)
throws AuthenticationException {
if (supports(authentication.getClass())) {
Expand All @@ -367,7 +364,7 @@ public Authentication authenticate(Authentication authentication)
public boolean supports(Class<?> authentication) {
return TestingAuthenticationToken.class.isAssignableFrom(authentication)
|| UsernamePasswordAuthenticationToken.class
.isAssignableFrom(authentication);
.isAssignableFrom(authentication);
}
}
}