From c5e6a4cdfd642198651379403984d9abeee6f739 Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Tue, 8 Jan 2008 13:33:20 +0000 Subject: [PATCH] SEC-546: Added AccountStatusException as base class for dibled, locked etc. Modified ProviderManager to prevent it querying further providers if either this exception or a ConcurrentLoginException is thrown. --- .../security/AccountExpiredException.java | 6 +- .../security/AccountStatusException.java | 18 + .../security/CredentialsExpiredException.java | 6 +- .../security/DisabledException.java | 6 +- .../security/LockedException.java | 6 +- .../security/providers/ProviderManager.java | 79 ++-- .../providers/ProviderManagerTests.java | 341 +++++++++--------- 7 files changed, 256 insertions(+), 206 deletions(-) create mode 100644 core/src/main/java/org/springframework/security/AccountStatusException.java diff --git a/core/src/main/java/org/springframework/security/AccountExpiredException.java b/core/src/main/java/org/springframework/security/AccountExpiredException.java index 3b3f135f5e..9b0eb3fab3 100644 --- a/core/src/main/java/org/springframework/security/AccountExpiredException.java +++ b/core/src/main/java/org/springframework/security/AccountExpiredException.java @@ -22,10 +22,10 @@ package org.springframework.security; * @author Ben Alex * @version $Id$ */ -public class AccountExpiredException extends AuthenticationException { +public class AccountExpiredException extends AccountStatusException { //~ Constructors =================================================================================================== -/** + /** * Constructs a AccountExpiredException with the specified * message. * @@ -35,7 +35,7 @@ public class AccountExpiredException extends AuthenticationException { super(msg); } -/** + /** * Constructs a AccountExpiredException with the specified * message and root cause. * diff --git a/core/src/main/java/org/springframework/security/AccountStatusException.java b/core/src/main/java/org/springframework/security/AccountStatusException.java new file mode 100644 index 0000000000..6d820bf548 --- /dev/null +++ b/core/src/main/java/org/springframework/security/AccountStatusException.java @@ -0,0 +1,18 @@ +package org.springframework.security; + +/** + * Base class for authentication exceptions which are caused by a particular + * user account status (locked, disabled etc). + * + * @author Luke Taylor + * @version $Id$ + */ +public abstract class AccountStatusException extends AuthenticationException { + public AccountStatusException(String msg) { + super(msg); + } + + public AccountStatusException(String msg, Throwable t) { + super(msg, t); + } +} diff --git a/core/src/main/java/org/springframework/security/CredentialsExpiredException.java b/core/src/main/java/org/springframework/security/CredentialsExpiredException.java index f6ddd3d4f3..a65396886a 100644 --- a/core/src/main/java/org/springframework/security/CredentialsExpiredException.java +++ b/core/src/main/java/org/springframework/security/CredentialsExpiredException.java @@ -22,10 +22,10 @@ package org.springframework.security; * @author Ben Alex * @version $Id$ */ -public class CredentialsExpiredException extends AuthenticationException { +public class CredentialsExpiredException extends AccountStatusException { //~ Constructors =================================================================================================== -/** + /** * Constructs a CredentialsExpiredException with the specified * message. * @@ -35,7 +35,7 @@ public class CredentialsExpiredException extends AuthenticationException { super(msg); } -/** + /** * Constructs a CredentialsExpiredException with the specified * message and root cause. * diff --git a/core/src/main/java/org/springframework/security/DisabledException.java b/core/src/main/java/org/springframework/security/DisabledException.java index 3c9ec0e7e8..d34a80fec7 100644 --- a/core/src/main/java/org/springframework/security/DisabledException.java +++ b/core/src/main/java/org/springframework/security/DisabledException.java @@ -22,10 +22,10 @@ package org.springframework.security; * @author Ben Alex * @version $Id$ */ -public class DisabledException extends AuthenticationException { +public class DisabledException extends AccountStatusException { //~ Constructors =================================================================================================== -/** + /** * Constructs a DisabledException with the specified message. * * @param msg the detail message @@ -34,7 +34,7 @@ public class DisabledException extends AuthenticationException { super(msg); } -/** + /** * Constructs a DisabledException with the specified message * and root cause. * diff --git a/core/src/main/java/org/springframework/security/LockedException.java b/core/src/main/java/org/springframework/security/LockedException.java index b41de14894..889a48b48c 100644 --- a/core/src/main/java/org/springframework/security/LockedException.java +++ b/core/src/main/java/org/springframework/security/LockedException.java @@ -22,10 +22,10 @@ package org.springframework.security; * @author Ben Alex * @version $Id$ */ -public class LockedException extends AuthenticationException { +public class LockedException extends AccountStatusException { //~ Constructors =================================================================================================== -/** + /** * Constructs a LockedException with the specified message. * * @param msg the detail message. @@ -34,7 +34,7 @@ public class LockedException extends AuthenticationException { super(msg); } -/** + /** * Constructs a LockedException with the specified message and * root cause. * diff --git a/core/src/main/java/org/springframework/security/providers/ProviderManager.java b/core/src/main/java/org/springframework/security/providers/ProviderManager.java index 3cc94e2782..e8aa1d2d98 100644 --- a/core/src/main/java/org/springframework/security/providers/ProviderManager.java +++ b/core/src/main/java/org/springframework/security/providers/ProviderManager.java @@ -25,6 +25,7 @@ import org.springframework.security.BadCredentialsException; import org.springframework.security.CredentialsExpiredException; import org.springframework.security.DisabledException; import org.springframework.security.LockedException; +import org.springframework.security.AccountStatusException; import org.springframework.security.concurrent.ConcurrentLoginException; import org.springframework.security.concurrent.ConcurrentSessionController; @@ -74,15 +75,18 @@ import java.util.Properties; * Can optionally be configured with a {@link ConcurrentSessionController} to limit the number of sessions a user can * have. *

- * AuthenticationProviders are tried in order until one provides a non-null response. + * AuthenticationProviders are usually tried in order until one provides a non-null response. * A non-null response indicates the provider had authority to decide on the authentication request and no further - * providers are tried. If an AuthenticationException is thrown by a provider, it is retained until - * subsequent providers are tried. If a subsequent provider successfully authenticates the request, the earlier - * authentication exception is disregarded and the successful authentication will be used. If no subsequent provider - * provides a non-null response, or a new AuthenticationException, the last - * AuthenticationException received will be used. If no provider returns a non-null response, or indicates - * it can even process an Authentication, the ProviderManager will throw a - * ProviderNotFoundException. + * providers are tried. + * If a subsequent provider successfully authenticates the request, the earlier authentication exception is disregarded + * and the successful authentication will be used. If no subsequent provider provides a non-null response, or a new + * AuthenticationException, the last AuthenticationException received will be used. + * If no provider returns a non-null response, or indicates it can even process an Authentication, + * the ProviderManager will throw a ProviderNotFoundException. + *

+ * The exception to this process is when a provider throws an {@link AccountStatusException} or if the configured + * concurrent session controller throws a {@link ConcurrentLoginException}. In both these cases, no further providers + * in the list will be queried. * *

* If a valid Authentication is returned by an AuthenticationProvider, the @@ -101,8 +105,8 @@ import java.util.Properties; * @version $Id$ * @see ConcurrentSessionController */ -public class ProviderManager extends AbstractAuthenticationManager implements InitializingBean, - ApplicationEventPublisherAware, MessageSourceAware { +public class ProviderManager extends AbstractAuthenticationManager implements InitializingBean, MessageSourceAware, + ApplicationEventPublisherAware { //~ Static fields/initializers ===================================================================================== private static final Log logger = LogFactory.getLog(ProviderManager.class); @@ -193,26 +197,34 @@ public class ProviderManager extends AbstractAuthenticationManager implements In while (iter.hasNext()) { AuthenticationProvider provider = (AuthenticationProvider) iter.next(); - if (provider.supports(toTest)) { - logger.debug("Authentication attempt using " + provider.getClass().getName()); + if (!provider.supports(toTest)) { + continue; + } - Authentication result; + logger.debug("Authentication attempt using " + provider.getClass().getName()); - try { - result = provider.authenticate(authentication); - copyDetails(authentication, result); - sessionController.checkAuthenticationAllowed(result); - } catch (AuthenticationException ae) { - lastException = ae; - result = null; - } + Authentication result; - if (result != null) { - sessionController.registerSuccessfulAuthentication(result); - publishEvent(new AuthenticationSuccessEvent(result)); + try { + result = provider.authenticate(authentication); + copyDetails(authentication, result); + sessionController.checkAuthenticationAllowed(result); + } catch (AuthenticationException ae) { + lastException = ae; + result = null; + } - return result; - } + // SEC-546: Avoid polling additional providers if auth failure is due to invalid account status or + // disallowed concurrent login. + if (lastException instanceof AccountStatusException || lastException instanceof ConcurrentLoginException) { + break; + } + + if (result != null) { + sessionController.registerSuccessfulAuthentication(result); + publishEvent(new AuthenticationSuccessEvent(result)); + + return result; } } @@ -221,8 +233,13 @@ public class ProviderManager extends AbstractAuthenticationManager implements In new Object[] {toTest.getName()}, "No AuthenticationProvider found for {0}")); } - // Publish the event - String className = exceptionMappings.getProperty(lastException.getClass().getName()); + publishAuthenticationFailure(lastException, authentication); + + throw lastException; + } + + private void publishAuthenticationFailure(AuthenticationException exception, Authentication authentication) { + String className = exceptionMappings.getProperty(exception.getClass().getName()); AbstractAuthenticationEvent event = null; if (className != null) { @@ -231,7 +248,7 @@ public class ProviderManager extends AbstractAuthenticationManager implements In Constructor constructor = clazz.getConstructor(new Class[] { Authentication.class, AuthenticationException.class }); - Object obj = constructor.newInstance(new Object[] {authentication, lastException}); + Object obj = constructor.newInstance(new Object[] {authentication, exception}); Assert.isInstanceOf(AbstractAuthenticationEvent.class, obj, "Must be an AbstractAuthenticationEvent"); event = (AbstractAuthenticationEvent) obj; } catch (ClassNotFoundException ignored) {} @@ -245,12 +262,10 @@ public class ProviderManager extends AbstractAuthenticationManager implements In publishEvent(event); } else { if (logger.isDebugEnabled()) { - logger.debug("No event was found for the exception " + lastException.getClass().getName()); + logger.debug("No event was found for the exception " + exception.getClass().getName()); } } - // Throw the exception - throw lastException; } /** diff --git a/core/src/test/java/org/springframework/security/providers/ProviderManagerTests.java b/core/src/test/java/org/springframework/security/providers/ProviderManagerTests.java index 2a21fe5c63..c7b93f56c0 100644 --- a/core/src/test/java/org/springframework/security/providers/ProviderManagerTests.java +++ b/core/src/test/java/org/springframework/security/providers/ProviderManagerTests.java @@ -21,15 +21,17 @@ import org.springframework.security.AuthenticationServiceException; import org.springframework.security.GrantedAuthority; import org.springframework.security.GrantedAuthorityImpl; import org.springframework.security.MockApplicationEventPublisher; +import org.springframework.security.AccountStatusException; import org.springframework.security.concurrent.ConcurrentSessionControllerImpl; import org.springframework.security.concurrent.NullConcurrentSessionController; - -import junit.framework.TestCase; +import org.springframework.security.concurrent.ConcurrentLoginException; import java.util.Arrays; import java.util.List; import java.util.Vector; +import org.junit.Test; +import static org.junit.Assert.*; /** * Tests {@link ProviderManager}. @@ -37,18 +39,162 @@ import java.util.Vector; * @author Ben Alex * @version $Id$ */ -public class ProviderManagerTests extends TestCase { - //~ Constructors =================================================================================================== - - public ProviderManagerTests() { - } - - public ProviderManagerTests(String arg0) { - super(arg0); - } +public class ProviderManagerTests { //~ Methods ======================================================================================================== + @Test(expected=ProviderNotFoundException.class) + public void authenticationFailsWithUnsupportedToken() throws Exception { + UsernamePasswordAuthenticationToken token = new UsernamePasswordAuthenticationToken("Test", "Password", + new GrantedAuthority[] {new GrantedAuthorityImpl("ROLE_ONE"), new GrantedAuthorityImpl("ROLE_TWO")}); + + ProviderManager mgr = makeProviderManager(); + mgr.setApplicationEventPublisher(new MockApplicationEventPublisher(true)); + mgr.authenticate(token); + } + + @Test + public void authenticationSucceedsWithSupportedTokenAndReturnsExpectedObject() throws Exception { + TestingAuthenticationToken token = new TestingAuthenticationToken("Test", "Password", + new GrantedAuthority[] {new GrantedAuthorityImpl("ROLE_ONE"), new GrantedAuthorityImpl("ROLE_TWO")}); + + ProviderManager mgr = makeProviderManager(); + mgr.setApplicationEventPublisher(new MockApplicationEventPublisher(true)); + + Authentication result = mgr.authenticate(token); + + if (!(result instanceof TestingAuthenticationToken)) { + fail("Should have returned instance of TestingAuthenticationToken"); + } + + TestingAuthenticationToken castResult = (TestingAuthenticationToken) result; + assertEquals("Test", castResult.getPrincipal()); + assertEquals("Password", castResult.getCredentials()); + assertEquals("ROLE_ONE", castResult.getAuthorities()[0].getAuthority()); + assertEquals("ROLE_TWO", castResult.getAuthorities()[1].getAuthority()); + } + + @Test + public void authenticationSuccessWhenFirstProviderReturnsNullButSecondAuthenticates() { + TestingAuthenticationToken token = new TestingAuthenticationToken("Test", "Password", + new GrantedAuthority[] {new GrantedAuthorityImpl("ROLE_ONE"), new GrantedAuthorityImpl("ROLE_TWO")}); + + ProviderManager mgr = makeProviderManagerWithMockProviderWhichReturnsNullInList(); + mgr.setApplicationEventPublisher(new MockApplicationEventPublisher(true)); + + Authentication result = mgr.authenticate(token); + + if (!(result instanceof TestingAuthenticationToken)) { + fail("Should have returned instance of TestingAuthenticationToken"); + } + + TestingAuthenticationToken castResult = (TestingAuthenticationToken) result; + assertEquals("Test", castResult.getPrincipal()); + assertEquals("Password", castResult.getCredentials()); + assertEquals("ROLE_ONE", castResult.getAuthorities()[0].getAuthority()); + assertEquals("ROLE_TWO", castResult.getAuthorities()[1].getAuthority()); + } + + @Test + public void concurrentSessionControllerConfiguration() throws Exception { + ProviderManager target = new ProviderManager(); + + //The NullConcurrentSessionController should be the default + assertNotNull(target.getSessionController()); + assertTrue(target.getSessionController() instanceof NullConcurrentSessionController); + + ConcurrentSessionControllerImpl impl = new ConcurrentSessionControllerImpl(); + target.setSessionController(impl); + assertEquals(impl, target.getSessionController()); + } + + @Test(expected=IllegalArgumentException.class) + public void startupFailsIfProviderListDoesNotContainProviders() throws Exception { + List providers = new Vector(); + providers.add("THIS_IS_NOT_A_PROVIDER"); + + ProviderManager mgr = new ProviderManager(); + + mgr.setProviders(providers); + } + + @Test(expected=IllegalArgumentException.class) + public void startupFailsIfProviderListNotSet() throws Exception { + ProviderManager mgr = new ProviderManager(); + + mgr.afterPropertiesSet(); + } + + @Test(expected=IllegalArgumentException.class) + public void testStartupFailsIfProviderListNull() throws Exception { + ProviderManager mgr = new ProviderManager(); + + mgr.setProviders(null); + } + + @Test + public void detailsAreNotSetOnAuthenticationTokenIfAlreadySetByProvider() throws Exception { + Object requestDetails = "(Request Details)"; + final Object resultDetails = "(Result Details)"; + ProviderManager authMgr = makeProviderManager(); + + AuthenticationProvider provider = new AuthenticationProvider() { + public Authentication authenticate(Authentication authentication) throws AuthenticationException { + ((TestingAuthenticationToken)authentication).setDetails(resultDetails); + return authentication; + } + + public boolean supports(Class authentication) { + return true; + } + }; + + authMgr.setProviders(Arrays.asList(provider)); + + TestingAuthenticationToken request = createAuthenticationToken(); + request.setDetails(requestDetails); + + Authentication result = authMgr.authenticate(request); + assertEquals(resultDetails, result.getDetails()); + } + + @Test + public void detailsAreSetOnAuthenticationTokenIfNotAlreadySetByProvider() throws Exception { + Object details = new Object(); + ProviderManager authMgr = makeProviderManager(); + + TestingAuthenticationToken request = createAuthenticationToken(); + request.setDetails(details); + + Authentication result = authMgr.authenticate(request); + assertEquals(details, result.getDetails()); + } + + // SEC-546 + @Test(expected=AccountStatusException.class) + public void accountStatusExceptionPreventsCallsToSubsequentProviders() throws Exception { + ProviderManager authMgr = makeProviderManager(); + + authMgr.setProviders(Arrays.asList(new MockProviderWhichThrowsAccountStatusException(), + new MockProviderWhichThrowsConcurrentLoginException()) ); + + authMgr.authenticate(createAuthenticationToken()); + } + + @Test(expected=ConcurrentLoginException.class) + public void concurrentLoginExceptionPreventsCallsToSubsequentProviders() throws Exception { + ProviderManager authMgr = makeProviderManager(); + + authMgr.setProviders(Arrays.asList(new MockProviderWhichThrowsConcurrentLoginException(), + new MockProviderWhichThrowsAccountStatusException()) ); + + authMgr.authenticate(createAuthenticationToken()); + } + + private TestingAuthenticationToken createAuthenticationToken() { + return new TestingAuthenticationToken("name", "password", new GrantedAuthorityImpl[0]); + } + private ProviderManager makeProviderManager() throws Exception { MockProvider provider1 = new MockProvider(); List providers = new Vector(); @@ -74,157 +220,7 @@ public class ProviderManagerTests extends TestCase { return mgr; } - - public void testAuthenticationFails() throws Exception { - UsernamePasswordAuthenticationToken token = new UsernamePasswordAuthenticationToken("Test", "Password", - new GrantedAuthority[] {new GrantedAuthorityImpl("ROLE_ONE"), new GrantedAuthorityImpl("ROLE_TWO")}); - - ProviderManager mgr = makeProviderManager(); - mgr.setApplicationEventPublisher(new MockApplicationEventPublisher(true)); - - try { - mgr.authenticate(token); - fail("Should have thrown ProviderNotFoundException"); - } catch (ProviderNotFoundException expected) { - assertTrue(true); - } - } - - public void testAuthenticationSuccess() throws Exception { - TestingAuthenticationToken token = new TestingAuthenticationToken("Test", "Password", - new GrantedAuthority[] {new GrantedAuthorityImpl("ROLE_ONE"), new GrantedAuthorityImpl("ROLE_TWO")}); - - ProviderManager mgr = makeProviderManager(); - mgr.setApplicationEventPublisher(new MockApplicationEventPublisher(true)); - - Authentication result = mgr.authenticate(token); - - if (!(result instanceof TestingAuthenticationToken)) { - fail("Should have returned instance of TestingAuthenticationToken"); - } - - TestingAuthenticationToken castResult = (TestingAuthenticationToken) result; - assertEquals("Test", castResult.getPrincipal()); - assertEquals("Password", castResult.getCredentials()); - assertEquals("ROLE_ONE", castResult.getAuthorities()[0].getAuthority()); - assertEquals("ROLE_TWO", castResult.getAuthorities()[1].getAuthority()); - } - - public void testAuthenticationSuccessWhenFirstProviderReturnsNullButSecondAuthenticates() { - TestingAuthenticationToken token = new TestingAuthenticationToken("Test", "Password", - new GrantedAuthority[] {new GrantedAuthorityImpl("ROLE_ONE"), new GrantedAuthorityImpl("ROLE_TWO")}); - - ProviderManager mgr = makeProviderManagerWithMockProviderWhichReturnsNullInList(); - mgr.setApplicationEventPublisher(new MockApplicationEventPublisher(true)); - - Authentication result = mgr.authenticate(token); - - if (!(result instanceof TestingAuthenticationToken)) { - fail("Should have returned instance of TestingAuthenticationToken"); - } - - TestingAuthenticationToken castResult = (TestingAuthenticationToken) result; - assertEquals("Test", castResult.getPrincipal()); - assertEquals("Password", castResult.getCredentials()); - assertEquals("ROLE_ONE", castResult.getAuthorities()[0].getAuthority()); - assertEquals("ROLE_TWO", castResult.getAuthorities()[1].getAuthority()); - } - - public void testConcurrentSessionControllerConfiguration() throws Exception { - ProviderManager target = new ProviderManager(); - - //The NullConcurrentSessionController should be the default - assertNotNull(target.getSessionController()); - assertTrue(target.getSessionController() instanceof NullConcurrentSessionController); - - ConcurrentSessionControllerImpl impl = new ConcurrentSessionControllerImpl(); - target.setSessionController(impl); - assertEquals(impl, target.getSessionController()); - } - - public void testStartupFailsIfProviderListDoesNotContainingProviders() throws Exception { - List providers = new Vector(); - providers.add("THIS_IS_NOT_A_PROVIDER"); - - ProviderManager mgr = new ProviderManager(); - - try { - mgr.setProviders(providers); - fail("Should have thrown IllegalArgumentException"); - } catch (IllegalArgumentException expected) { - assertTrue(true); - } - } - - public void testStartupFailsIfProviderListNotSet() throws Exception { - ProviderManager mgr = new ProviderManager(); - - try { - mgr.afterPropertiesSet(); - fail("Should have thrown IllegalArgumentException"); - } catch (IllegalArgumentException expected) { - assertTrue(true); - } - } - - public void testStartupFailsIfProviderListNull() throws Exception { - ProviderManager mgr = new ProviderManager(); - - try { - mgr.setProviders(null); - fail("Should have thrown IllegalArgumentException"); - } catch (IllegalArgumentException expected) { - assertTrue(true); - } - } - - public void testSuccessfulStartup() throws Exception { - ProviderManager mgr = makeProviderManager(); - mgr.afterPropertiesSet(); - assertTrue(true); - assertEquals(1, mgr.getProviders().size()); - } - - public void testDetailsAreNotSetOnAuthenticationTokenIfAlreadySetByProvider() throws Exception { - Object requestDetails = new String("(Request Details)"); - final Object resultDetails = new String("(Result Details)"); - ProviderManager authMgr = makeProviderManager(); - - AuthenticationProvider provider = new AuthenticationProvider() { - public Authentication authenticate(Authentication authentication) throws AuthenticationException { - ((TestingAuthenticationToken)authentication).setDetails(resultDetails); - return authentication; - } - - public boolean supports(Class authentication) { - return true; - } - }; - - authMgr.setProviders(Arrays.asList(new AuthenticationProvider[] {provider})); - - TestingAuthenticationToken request = createAuthenticationToken(); - request.setDetails(requestDetails); - - Authentication result = authMgr.authenticate(request); - assertEquals(resultDetails, result.getDetails()); - } - - public void testDetailsAreSetOnAuthenticationTokenIfNotAlreadySetByProvider() throws Exception { - Object details = new Object(); - ProviderManager authMgr = makeProviderManager(); - - TestingAuthenticationToken request = createAuthenticationToken(); - request.setDetails(details); - - Authentication result = authMgr.authenticate(request); - assertEquals(details, result.getDetails()); - } - - private TestingAuthenticationToken createAuthenticationToken() { - return new TestingAuthenticationToken("name", "password", new GrantedAuthorityImpl[0]); - } - + //~ Inner Classes ================================================================================================== private class MockProvider implements AuthenticationProvider { @@ -262,4 +258,25 @@ public class ProviderManagerTests extends TestCase { } } } + + private class MockProviderWhichThrowsAccountStatusException implements AuthenticationProvider { + public Authentication authenticate(Authentication authentication) throws AuthenticationException { + throw new AccountStatusException("xxx") {}; + } + + public boolean supports(Class authentication) { + return true; + } + } + + private class MockProviderWhichThrowsConcurrentLoginException implements AuthenticationProvider { + public Authentication authenticate(Authentication authentication) throws AuthenticationException { + throw new ConcurrentLoginException("xxx") {}; + } + + public boolean supports(Class authentication) { + return true; + } + } + }