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.

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 <code>AccountExpiredException</code> with the specified
      * message.
      *
@@ -35,7 +35,7 @@ public class AccountExpiredException extends AuthenticationException {
         super(msg);
     }
 
-/**
+    /**
      * Constructs a <code>AccountExpiredException</code> with the specified
      * message and root cause.
      *

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);
+    }
+}

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 <code>CredentialsExpiredException</code> with the specified
      * message.
      *
@@ -35,7 +35,7 @@ public class CredentialsExpiredException extends AuthenticationException {
         super(msg);
     }
 
-/**
+    /**
      * Constructs a <code>CredentialsExpiredException</code> with the specified
      * message and root cause.
      *

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 <code>DisabledException</code> with the specified message.
      *
      * @param msg the detail message
@@ -34,7 +34,7 @@ public class DisabledException extends AuthenticationException {
         super(msg);
     }
 
-/**
+    /**
      * Constructs a <code>DisabledException</code> with the specified message
      * and root cause.
      *

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 <code>LockedException</code> with the specified message.
      *
      * @param msg the detail message.
@@ -34,7 +34,7 @@ public class LockedException extends AuthenticationException {
         super(msg);
     }
 
-/**
+    /**
      * Constructs a <code>LockedException</code> with the specified message and
      * root cause.
      *

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.
  * <p>
- * <code>AuthenticationProvider</code>s are tried in order until one provides a non-null response.
+ * <tt>AuthenticationProvider</tt>s 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 <code>AuthenticationException</code> 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 <code>AuthenticationException</code>, the last
- * <code>AuthenticationException</code> received will be used. If no provider returns a non-null response, or indicates
- * it can even process an <code>Authentication</code>, the <code>ProviderManager</code> will throw a
- * <code>ProviderNotFoundException</code>.
+ * 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
+ * <code>AuthenticationException</code>, the last <code>AuthenticationException</code> received will be used.
+ * If no provider returns a non-null response, or indicates it can even process an <code>Authentication</code>,
+ * the <code>ProviderManager</code> will throw a <code>ProviderNotFoundException</code>.
+ * <p>
+ * 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. 
  *
  * <p>
  * If a valid <code>Authentication</code> is returned by an <code>AuthenticationProvider</code>, 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;
+            }
+
+            logger.debug("Authentication attempt using " + provider.getClass().getName());
 
-                Authentication result;
+            Authentication result;
+
+            try {
+                result = provider.authenticate(authentication);
+                copyDetails(authentication, result);
+                sessionController.checkAuthenticationAllowed(result);
+            } catch (AuthenticationException ae) {
+                lastException = ae;
+                result = null;
+            }
 
-                try {
-                    result = provider.authenticate(authentication);
-                    copyDetails(authentication, result);
-                    sessionController.checkAuthenticationAllowed(result);
-                } catch (AuthenticationException ae) {
-                    lastException = ae;
-                    result = null;
-                }
+            // 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));
+            if (result != null) {
+                sessionController.registerSuccessfulAuthentication(result);
+                publishEvent(new AuthenticationSuccessEvent(result));
 
-                    return 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;
     }
 
     /**

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,60 +39,22 @@ 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 ========================================================================================================
 
-    private ProviderManager makeProviderManager() throws Exception {
-        MockProvider provider1 = new MockProvider();
-        List providers = new Vector();
-        providers.add(provider1);
-
-        ProviderManager mgr = new ProviderManager();
-        mgr.setProviders(providers);
-
-        mgr.afterPropertiesSet();
-
-        return mgr;
-    }
-
-    private ProviderManager makeProviderManagerWithMockProviderWhichReturnsNullInList() {
-        MockProviderWhichReturnsNull provider1 = new MockProviderWhichReturnsNull();
-        MockProvider provider2 = new MockProvider();
-        List providers = new Vector();
-        providers.add(provider1);
-        providers.add(provider2);
-
-        ProviderManager mgr = new ProviderManager();
-        mgr.setProviders(providers);
-
-        return mgr;
-    }
-
-    public void testAuthenticationFails() throws Exception {
+    @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));
-
-        try {
-            mgr.authenticate(token);
-            fail("Should have thrown ProviderNotFoundException");
-        } catch (ProviderNotFoundException expected) {
-            assertTrue(true);
-        }
+        mgr.authenticate(token);
     }
 
-    public void testAuthenticationSuccess() throws Exception {
+    @Test
+    public void authenticationSucceedsWithSupportedTokenAndReturnsExpectedObject() throws Exception {
         TestingAuthenticationToken token = new TestingAuthenticationToken("Test", "Password",
                 new GrantedAuthority[] {new GrantedAuthorityImpl("ROLE_ONE"), new GrantedAuthorityImpl("ROLE_TWO")});
 
@@ -110,7 +74,8 @@ public class ProviderManagerTests extends TestCase {
         assertEquals("ROLE_TWO", castResult.getAuthorities()[1].getAuthority());
     }
 
-    public void testAuthenticationSuccessWhenFirstProviderReturnsNullButSecondAuthenticates() {
+    @Test
+    public void authenticationSuccessWhenFirstProviderReturnsNullButSecondAuthenticates() {
         TestingAuthenticationToken token = new TestingAuthenticationToken("Test", "Password",
                 new GrantedAuthority[] {new GrantedAuthorityImpl("ROLE_ONE"), new GrantedAuthorityImpl("ROLE_TWO")});
 
@@ -130,7 +95,8 @@ public class ProviderManagerTests extends TestCase {
         assertEquals("ROLE_TWO", castResult.getAuthorities()[1].getAuthority());
     }
 
-    public void testConcurrentSessionControllerConfiguration() throws Exception {
+    @Test
+    public void concurrentSessionControllerConfiguration() throws Exception {
         ProviderManager target = new ProviderManager();
 
         //The NullConcurrentSessionController should be the default
@@ -142,52 +108,34 @@ public class ProviderManagerTests extends TestCase {
         assertEquals(impl, target.getSessionController());
     }
 
-    public void testStartupFailsIfProviderListDoesNotContainingProviders() throws Exception {
+    @Test(expected=IllegalArgumentException.class)
+    public void startupFailsIfProviderListDoesNotContainProviders() 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);
-        }
+        mgr.setProviders(providers);
     }
 
-    public void testStartupFailsIfProviderListNotSet() throws Exception {
+    @Test(expected=IllegalArgumentException.class)
+    public void startupFailsIfProviderListNotSet() throws Exception {
         ProviderManager mgr = new ProviderManager();
 
-        try {
-            mgr.afterPropertiesSet();
-            fail("Should have thrown IllegalArgumentException");
-        } catch (IllegalArgumentException expected) {
-            assertTrue(true);
-        }
+        mgr.afterPropertiesSet();
     }
 
+    @Test(expected=IllegalArgumentException.class)
     public void testStartupFailsIfProviderListNull() throws Exception {
         ProviderManager mgr = new ProviderManager();
 
-        try {
-            mgr.setProviders(null);
-            fail("Should have thrown IllegalArgumentException");
-        } catch (IllegalArgumentException expected) {
-            assertTrue(true);
-        }
+        mgr.setProviders(null);
     }
 
-    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)");
+    @Test
+    public void detailsAreNotSetOnAuthenticationTokenIfAlreadySetByProvider() throws Exception {
+        Object requestDetails = "(Request Details)";
+        final Object resultDetails = "(Result Details)";
         ProviderManager authMgr = makeProviderManager();
 
         AuthenticationProvider provider = new AuthenticationProvider() {
@@ -201,7 +149,7 @@ public class ProviderManagerTests extends TestCase {
             }
         };
 
-        authMgr.setProviders(Arrays.asList(new AuthenticationProvider[] {provider}));
+        authMgr.setProviders(Arrays.asList(provider));
 
         TestingAuthenticationToken request = createAuthenticationToken();
         request.setDetails(requestDetails);
@@ -210,7 +158,8 @@ public class ProviderManagerTests extends TestCase {
         assertEquals(resultDetails, result.getDetails());
     }
 
-    public void testDetailsAreSetOnAuthenticationTokenIfNotAlreadySetByProvider() throws Exception {
+    @Test
+    public void detailsAreSetOnAuthenticationTokenIfNotAlreadySetByProvider() throws Exception {
         Object details = new Object();
         ProviderManager authMgr = makeProviderManager();
 
@@ -221,10 +170,57 @@ public class ProviderManagerTests extends TestCase {
         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();
+        providers.add(provider1);
+
+        ProviderManager mgr = new ProviderManager();
+        mgr.setProviders(providers);
+
+        mgr.afterPropertiesSet();
+
+        return mgr;
+    }
+
+    private ProviderManager makeProviderManagerWithMockProviderWhichReturnsNullInList() {
+        MockProviderWhichReturnsNull provider1 = new MockProviderWhichReturnsNull();
+        MockProvider provider2 = new MockProvider();
+        List providers = new Vector();
+        providers.add(provider1);
+        providers.add(provider2);
+
+        ProviderManager mgr = new ProviderManager();
+        mgr.setProviders(providers);
+
+        return mgr;
+    }
+    
     //~ 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;
+        }
+    }
+
 }