SEC-536: Introduced UserDetailsChecker strategy to extract code for checking status of accounts and allowing variation in pre/post authentication checks made by AbstractUserDetailsAuthenticationProvider

core/src/main/java/org/springframework/security/config/X509BeanDefinitionParser.java

@@ -5,7 +5,6 @@ import org.springframework.security.ui.preauth.x509.X509PreAuthenticatedProcessi
 import org.springframework.security.ui.preauth.x509.SubjectDnX509PrincipalExtractor;
 import org.springframework.security.providers.preauth.PreAuthenticatedAuthenticationProvider;
 import org.springframework.security.providers.preauth.UserDetailsByNameServiceWrapper;
-import org.springframework.security.userdetails.decorator.StatusCheckingUserDetailsService;
 import org.springframework.beans.factory.xml.BeanDefinitionParser;
 import org.springframework.beans.factory.xml.ParserContext;
 import org.springframework.beans.factory.config.BeanDefinition;
@@ -52,12 +51,9 @@ public class X509BeanDefinitionParser implements BeanDefinitionParser {
         String userServiceRef = element.getAttribute(ATT_USER_SERVICE_REF);
 
         if (StringUtils.hasText(userServiceRef)) {
-            RootBeanDefinition statusCheckingUserService = new RootBeanDefinition(StatusCheckingUserDetailsService.class);
-            statusCheckingUserService.setSource(source);
-            statusCheckingUserService.getConstructorArgumentValues().addIndexedArgumentValue(0, new RuntimeBeanReference(userServiceRef));
             RootBeanDefinition preAuthUserService = new RootBeanDefinition(UserDetailsByNameServiceWrapper.class);
             preAuthUserService.setSource(source);
-            preAuthUserService.getPropertyValues().addPropertyValue("userDetailsService", statusCheckingUserService);
+            preAuthUserService.getPropertyValues().addPropertyValue("userDetailsService", new RuntimeBeanReference(userServiceRef));
             provider.getPropertyValues().addPropertyValue("preAuthenticatedUserDetailsService", preAuthUserService);
         }
 

core/src/main/java/org/springframework/security/providers/dao/AbstractUserDetailsAuthenticationProvider.java

@@ -31,6 +31,7 @@ import org.springframework.security.providers.dao.cache.NullUserCache;
 import org.springframework.security.userdetails.UserDetails;
 import org.springframework.security.userdetails.UserDetailsService;
 import org.springframework.security.userdetails.UsernameNotFoundException;
+import org.springframework.security.userdetails.UserDetailsChecker;
 
 import org.springframework.beans.factory.InitializingBean;
 
@@ -56,8 +57,8 @@ import org.springframework.util.Assert;
  * and <code>UserDetails</code> implementations provide additional flexibility, by default a <code>UserDetails</code>
  * is returned. To override this
  * default, set the {@link #setForcePrincipalAsString} to <code>true</code>.
- * </p>
- *  <p>Caching is handled via the <code>UserDetails</code> object being placed in the {@link UserCache}. This
+ * <p>
+ * Caching is handled via the <code>UserDetails</code> object being placed in the {@link UserCache}. This
  * ensures that subsequent requests with the same username can be validated without needing to query the {@link
  * UserDetailsService}. It should be noted that if a user appears to present an incorrect password, the {@link
  * UserDetailsService} will be queried to confirm the most up-to-date password was used for comparison.</p>
@@ -66,13 +67,15 @@ import org.springframework.util.Assert;
  * @version $Id$
  */
 public abstract class AbstractUserDetailsAuthenticationProvider implements AuthenticationProvider, InitializingBean,
-    MessageSourceAware {
+        MessageSourceAware {
     //~ Instance fields ================================================================================================
 
     protected MessageSourceAccessor messages = SpringSecurityMessageSource.getAccessor();
     private UserCache userCache = new NullUserCache();
     private boolean forcePrincipalAsString = false;
     protected boolean hideUserNotFoundExceptions = true;
+    private UserDetailsChecker preAuthenticationChecks = new DefaultPreAuthenticationChecks();
+    private UserDetailsChecker postAuthenticationChecks = new DefaultPostAuthenticationChecks();
 
     //~ Methods ========================================================================================================
 
@@ -100,8 +103,7 @@ public abstract class AbstractUserDetailsAuthenticationProvider implements Authe
         doAfterPropertiesSet();
     }
 
-    public Authentication authenticate(Authentication authentication)
-        throws AuthenticationException {
+    public Authentication authenticate(Authentication authentication) throws AuthenticationException {
         Assert.isInstanceOf(UsernamePasswordAuthenticationToken.class, authentication,
             messages.getMessage("AbstractUserDetailsAuthenticationProvider.onlySupports",
                 "Only UsernamePasswordAuthenticationToken is supported"));
@@ -129,21 +131,8 @@ public abstract class AbstractUserDetailsAuthenticationProvider implements Authe
             Assert.notNull(user, "retrieveUser returned null - a violation of the interface contract");
         }
 
-        if (!user.isAccountNonLocked()) {
-            throw new LockedException(messages.getMessage("AbstractUserDetailsAuthenticationProvider.locked",
-                    "User account is locked"));
-        }
-
-        if (!user.isEnabled()) {
-            throw new DisabledException(messages.getMessage("AbstractUserDetailsAuthenticationProvider.disabled",
-                    "User is disabled"));
-        }
-
-        if (!user.isAccountNonExpired()) {
-            throw new AccountExpiredException(messages.getMessage("AbstractUserDetailsAuthenticationProvider.expired",
-                    "User account has expired"));
-        }
-
+        preAuthenticationChecks.check(user);
+        
         // This check must come here, as we don't want to tell users
         // about account status unless they presented the correct credentials
         try {
@@ -160,10 +149,7 @@ public abstract class AbstractUserDetailsAuthenticationProvider implements Authe
             }
         }
 
-        if (!user.isCredentialsNonExpired()) {
-            throw new CredentialsExpiredException(messages.getMessage(
-                    "AbstractUserDetailsAuthenticationProvider.credentialsExpired", "User credentials have expired"));
-        }
+        postAuthenticationChecks.check(user);
 
         if (!cacheWasUsed) {
             this.userCache.putUserInCache(user);
@@ -278,4 +264,50 @@ public abstract class AbstractUserDetailsAuthenticationProvider implements Authe
     public boolean supports(Class authentication) {
         return (UsernamePasswordAuthenticationToken.class.isAssignableFrom(authentication));
     }
+
+    protected UserDetailsChecker getPreAuthenticationChecks() {
+        return preAuthenticationChecks;
+    }
+
+    public void setPreAuthenticationChecks(UserDetailsChecker preAuthenticationChecks) {
+        this.preAuthenticationChecks = preAuthenticationChecks;
+    }
+
+    protected UserDetailsChecker getPostAuthenticationChecks() {
+        return postAuthenticationChecks;
+    }
+
+    public void setPostAuthenticationChecks(UserDetailsChecker postAuthenticationChecks) {
+        this.postAuthenticationChecks = postAuthenticationChecks;
+    }
+
+    private class DefaultPreAuthenticationChecks implements UserDetailsChecker {
+        public void check(UserDetails user) {
+            if (!user.isAccountNonLocked()) {
+                throw new LockedException(messages.getMessage("AbstractUserDetailsAuthenticationProvider.locked",
+                        "User account is locked"));
+            }
+
+            if (!user.isEnabled()) {
+                throw new DisabledException(messages.getMessage("AbstractUserDetailsAuthenticationProvider.disabled",
+                        "User is disabled"));
+            }
+
+            if (!user.isAccountNonExpired()) {
+                throw new AccountExpiredException(messages.getMessage("AbstractUserDetailsAuthenticationProvider.expired",
+                        "User account has expired"));
+            }
+
+        }
+    }
+
+    private class DefaultPostAuthenticationChecks implements UserDetailsChecker {
+        public void check(UserDetails user) {
+            if (!user.isCredentialsNonExpired()) {
+                throw new CredentialsExpiredException(messages.getMessage(
+                        "AbstractUserDetailsAuthenticationProvider.credentialsExpired", "User credentials have expired"));
+            }
+
+        }
+    }
 }

core/src/main/java/org/springframework/security/ui/rememberme/AbstractRememberMeServices.java

@@ -15,7 +15,8 @@ import org.springframework.security.ui.logout.LogoutHandler;
 import org.springframework.security.userdetails.UserDetails;
 import org.springframework.security.userdetails.UserDetailsService;
 import org.springframework.security.userdetails.UsernameNotFoundException;
-import org.springframework.security.userdetails.decorator.StatusCheckingUserDetailsService;
+import org.springframework.security.userdetails.UserDetailsChecker;
+import org.springframework.security.userdetails.checker.AccountStatusUserDetailsChecker;
 import org.springframework.util.Assert;
 import org.springframework.util.StringUtils;
 import org.springframework.web.bind.ServletRequestUtils;
@@ -44,8 +45,8 @@ public abstract class AbstractRememberMeServices implements RememberMeServices,
 
     protected MessageSourceAccessor messages = SpringSecurityMessageSource.getAccessor();
 
-
     private UserDetailsService userDetailsService;
+    private UserDetailsChecker userDetailsChecker = new AccountStatusUserDetailsChecker();
     private AuthenticationDetailsSource authenticationDetailsSource = new AuthenticationDetailsSourceImpl();
 
     private String cookieName = SPRING_SECURITY_REMEMBER_ME_COOKIE_KEY;
@@ -83,6 +84,7 @@ public abstract class AbstractRememberMeServices implements RememberMeServices,
         try {
             String[] cookieTokens = decodeCookie(rememberMeCookie);
             user = processAutoLoginCookie(cookieTokens, request, response);
+            userDetailsChecker.check(user);
         } catch (CookieTheftException cte) {
             cancelCookie(request, response);
             throw cte;
@@ -319,7 +321,7 @@ public abstract class AbstractRememberMeServices implements RememberMeServices,
     }
 
     public void setUserDetailsService(UserDetailsService userDetailsService) {
-        this.userDetailsService = new StatusCheckingUserDetailsService(userDetailsService);
+        this.userDetailsService = userDetailsService;
     }
 
     public void setKey(String key) {

core/src/main/java/org/springframework/security/ui/switchuser/SwitchUserProcessingFilter.java

@@ -35,8 +35,9 @@ import org.springframework.security.ui.FilterChainOrder;
 import org.springframework.security.ui.AbstractProcessingFilter;
 import org.springframework.security.userdetails.UserDetails;
 import org.springframework.security.userdetails.UserDetailsService;
-import org.springframework.security.userdetails.decorator.StatusCheckingUserDetailsService;
 import org.springframework.security.userdetails.UsernameNotFoundException;
+import org.springframework.security.userdetails.UserDetailsChecker;
+import org.springframework.security.userdetails.checker.AccountStatusUserDetailsChecker;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
@@ -120,6 +121,7 @@ public class SwitchUserProcessingFilter extends SpringSecurityFilter implements
     private String switchFailureUrl;
     private SwitchUserAuthorityChanger switchUserAuthorityChanger;
     private UserDetailsService userDetailsService;
+    private UserDetailsChecker userDetailsChecker = new AccountStatusUserDetailsChecker();
     private boolean useRelativeContext;
 
     //~ Methods ========================================================================================================
@@ -204,8 +206,8 @@ public class SwitchUserProcessingFilter extends SpringSecurityFilter implements
             logger.debug("Attempt to switch to user [" + username + "]");
         }
 
-        // load the user by name
-        UserDetails targetUser = this.userDetailsService.loadUserByUsername(username);
+        UserDetails targetUser = userDetailsService.loadUserByUsername(username);
+        userDetailsChecker.check(targetUser);
 
         // ok, create the switch user token
         targetUserRequest = createSwitchUserToken(request, targetUser);
@@ -426,7 +428,7 @@ public class SwitchUserProcessingFilter extends SpringSecurityFilter implements
      * @param userDetailsService The authentication dao
      */
     public void setUserDetailsService(UserDetailsService userDetailsService) {
-        this.userDetailsService = new StatusCheckingUserDetailsService(userDetailsService);
+        this.userDetailsService = userDetailsService;
     }
 
     /**

core/src/main/java/org/springframework/security/userdetails/UserDetailsChecker.java

@@ -0,0 +1,10 @@
+package org.springframework.security.userdetails;
+
+/**
+ * @author Luke Taylor
+ * @version $Id$
+ * @since 2.0
+ */
+public interface UserDetailsChecker {
+    void check(UserDetails toCheck);
+}

core/src/main/java/org/springframework/security/userdetails/decorator/StatusCheckingUserDetailsService.java → core/src/main/java/org/springframework/security/userdetails/checker/AccountStatusUserDetailsChecker.java

@@ -1,39 +1,23 @@
-package org.springframework.security.userdetails.decorator;
+package org.springframework.security.userdetails.checker;
 
-import org.springframework.security.userdetails.UserDetailsService;
+import org.springframework.security.userdetails.UserDetailsChecker;
 import org.springframework.security.userdetails.UserDetails;
 import org.springframework.security.LockedException;
 import org.springframework.security.DisabledException;
 import org.springframework.security.AccountExpiredException;
 import org.springframework.security.CredentialsExpiredException;
 import org.springframework.security.SpringSecurityMessageSource;
-import org.springframework.security.AuthenticationException;
-import org.springframework.dao.DataAccessException;
 import org.springframework.context.support.MessageSourceAccessor;
-import org.springframework.util.Assert;
 
 /**
- * Decorates a {@link UserDetailsService}, making it throw an exception if the account is locked, disabled etc. This
- * removes the need for separate account status checks in classes which make use of a <tt>UserDetailsService</tt>. 
- *
  * @author Luke Taylor
  * @version $Id$
  */
-public class StatusCheckingUserDetailsService implements UserDetailsService {
-    private UserDetailsService delegate;
+public class AccountStatusUserDetailsChecker implements UserDetailsChecker {
 
     protected MessageSourceAccessor messages = SpringSecurityMessageSource.getAccessor();
 
-    public StatusCheckingUserDetailsService(UserDetailsService userDetailsService) {
-        this.delegate = userDetailsService;
-    }
-
-    public UserDetails loadUserByUsername(String username) throws AuthenticationException, DataAccessException {
-
-        UserDetails user = delegate.loadUserByUsername(username);
-
-        Assert.notNull(user, "UserDetailsService returned null user, an interface violation.");
-
+    public void check(UserDetails user) {
         if (!user.isAccountNonLocked()) {
             throw new LockedException(messages.getMessage("UserDetailsService.locked", "User account is locked"));
         }
@@ -51,7 +35,5 @@ public class StatusCheckingUserDetailsService implements UserDetailsService {
             throw new CredentialsExpiredException(messages.getMessage("UserDetailsService.credentialsExpired",
                     "User credentials have expired"));
         }
-
-        return user;
     }
 }

core/src/test/java/org/springframework/security/providers/siteminder/SiteminderAuthenticationProviderTests.java

@@ -50,14 +50,6 @@ import org.springframework.dao.DataRetrievalFailureException;
 public class SiteminderAuthenticationProviderTests extends TestCase {
     //~ Methods ========================================================================================================
 
-    public static void main(String[] args) {
-        junit.textui.TestRunner.run(SiteminderAuthenticationProviderTests.class);
-    }
-
-    public final void setUp() throws Exception {
-        super.setUp();
-    }
-
     public void testAuthenticateFailsIfAccountExpired() {
         UsernamePasswordAuthenticationToken token = new UsernamePasswordAuthenticationToken("peter", "opal");
 

core/src/test/java/org/springframework/security/ui/AbstractProcessingFilterTests.java

@@ -478,11 +478,11 @@ public class AbstractProcessingFilterTests extends TestCase {
 
         // Setup our test object, to grant access
         MockAbstractProcessingFilter filter = new MockAbstractProcessingFilter(true);
-        filter.setDefaultTargetUrl("http://monkeymachine.co.uk/");
+        filter.setDefaultTargetUrl("https://monkeymachine.co.uk/");
         filter.setAlwaysUseDefaultTargetUrl(true);
 
         executeFilterInContainerSimulator(config, filter, request, response, chain);
-        assertEquals("http://monkeymachine.co.uk/", response.getRedirectedUrl());
+        assertEquals("https://monkeymachine.co.uk/", response.getRedirectedUrl());
         assertNotNull(SecurityContextHolder.getContext().getAuthentication());
     }
 

core/src/test/java/org/springframework/security/userdetails/decorator/StatusCheckingUserDetailsServiceTests.java

@@ -1,43 +0,0 @@
-package org.springframework.security.userdetails.decorator;
-
-import org.springframework.security.userdetails.MockUserDetailsService;
-import org.springframework.security.LockedException;
-import org.springframework.security.DisabledException;
-import org.springframework.security.CredentialsExpiredException;
-import org.springframework.security.AccountExpiredException;
-
-import org.junit.Test;
-
-/**
- * @author Luke Taylor
- * @version $Id$
- */
-public class StatusCheckingUserDetailsServiceTests {
-    private StatusCheckingUserDetailsService us = new StatusCheckingUserDetailsService(new MockUserDetailsService());
-
-    @Test
-    public void validAccountIsSuccessfullyLoaded() throws Exception {
-        us.loadUserByUsername("valid");
-    }
-
-    @Test(expected = LockedException.class)
-    public void lockedAccountThrowsLockedException() throws Exception {
-        us.loadUserByUsername("locked");
-    }
-
-    @Test(expected = DisabledException.class)
-    public void disabledAccountThrowsDisabledException() throws Exception {
-        us.loadUserByUsername("disabled");
-    }
-
-    @Test(expected = CredentialsExpiredException.class)
-    public void credentialsExpiredAccountThrowsCredentialsExpiredException() throws Exception {
-        us.loadUserByUsername("credentialsExpired");
-    }
-
-    @Test(expected = AccountExpiredException.class)
-    public void expiredAccountThrowsAccountExpiredException() throws Exception {
-        us.loadUserByUsername("expired");
-    }
-
-}

openid/src/main/java/org/springframework/security/providers/openid/OpenIDAuthenticationProvider.java

@@ -34,9 +34,6 @@ import org.springframework.util.Assert;
  * enabled/disabled status of the <code>UserDetails</code> because this is
  * authentication-related and should have been enforced by another provider server.
  * <p>
- * You can optionally have these checked by configuring wrapping the <tt>UserDetailsService</tt> in a
- * {@link org.springframework.security.userdetails.decorator.StatusCheckingUserDetailsService} decorator.
- * <p>
  * The <code>UserDetails</code> returned by implementations is stored in the generated <code>AuthenticationToken</code>,
  * so additional properties such as email addresses, telephone numbers etc can easily be stored.
  *