From 5e204e23f3975dcb7d1f727af177a3527b7f4132 Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Fri, 15 Feb 2008 18:05:12 +0000 Subject: [PATCH] SEC-536: Introduced UserDetailsChecker strategy to extract code for checking status of accounts and allowing variation in pre/post authentication checks made by AbstractUserDetailsAuthenticationProvider --- .../config/X509BeanDefinitionParser.java | 6 +- ...ractUserDetailsAuthenticationProvider.java | 80 +++++++++++++------ .../AbstractRememberMeServices.java | 8 +- .../SwitchUserProcessingFilter.java | 10 ++- .../userdetails/UserDetailsChecker.java | 10 +++ .../AccountStatusUserDetailsChecker.java} | 26 +----- ...SiteminderAuthenticationProviderTests.java | 8 -- .../ui/AbstractProcessingFilterTests.java | 4 +- ...StatusCheckingUserDetailsServiceTests.java | 43 ---------- .../openid/OpenIDAuthenticationProvider.java | 3 - 10 files changed, 84 insertions(+), 114 deletions(-) create mode 100644 core/src/main/java/org/springframework/security/userdetails/UserDetailsChecker.java rename core/src/main/java/org/springframework/security/userdetails/{decorator/StatusCheckingUserDetailsService.java => checker/AccountStatusUserDetailsChecker.java} (55%) delete mode 100644 core/src/test/java/org/springframework/security/userdetails/decorator/StatusCheckingUserDetailsServiceTests.java diff --git a/core/src/main/java/org/springframework/security/config/X509BeanDefinitionParser.java b/core/src/main/java/org/springframework/security/config/X509BeanDefinitionParser.java index 297b7ad11b..eba9b580f3 100644 --- a/core/src/main/java/org/springframework/security/config/X509BeanDefinitionParser.java +++ b/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); } diff --git a/core/src/main/java/org/springframework/security/providers/dao/AbstractUserDetailsAuthenticationProvider.java b/core/src/main/java/org/springframework/security/providers/dao/AbstractUserDetailsAuthenticationProvider.java index 01d9c72691..a699136ae3 100644 --- a/core/src/main/java/org/springframework/security/providers/dao/AbstractUserDetailsAuthenticationProvider.java +++ b/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 UserDetails implementations provide additional flexibility, by default a UserDetails * is returned. To override this * default, set the {@link #setForcePrincipalAsString} to true. - *

- *

Caching is handled via the UserDetails object being placed in the {@link UserCache}. This + *

+ * Caching is handled via the UserDetails 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.

@@ -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")); + } + + } + } } diff --git a/core/src/main/java/org/springframework/security/ui/rememberme/AbstractRememberMeServices.java b/core/src/main/java/org/springframework/security/ui/rememberme/AbstractRememberMeServices.java index 20b6c1a021..6d9f66531c 100644 --- a/core/src/main/java/org/springframework/security/ui/rememberme/AbstractRememberMeServices.java +++ b/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) { diff --git a/core/src/main/java/org/springframework/security/ui/switchuser/SwitchUserProcessingFilter.java b/core/src/main/java/org/springframework/security/ui/switchuser/SwitchUserProcessingFilter.java index b0dd5c50d1..234768f253 100644 --- a/core/src/main/java/org/springframework/security/ui/switchuser/SwitchUserProcessingFilter.java +++ b/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; } /** diff --git a/core/src/main/java/org/springframework/security/userdetails/UserDetailsChecker.java b/core/src/main/java/org/springframework/security/userdetails/UserDetailsChecker.java new file mode 100644 index 0000000000..46156a72b0 --- /dev/null +++ b/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); +} diff --git a/core/src/main/java/org/springframework/security/userdetails/decorator/StatusCheckingUserDetailsService.java b/core/src/main/java/org/springframework/security/userdetails/checker/AccountStatusUserDetailsChecker.java similarity index 55% rename from core/src/main/java/org/springframework/security/userdetails/decorator/StatusCheckingUserDetailsService.java rename to core/src/main/java/org/springframework/security/userdetails/checker/AccountStatusUserDetailsChecker.java index 33413877fa..ab08cbf628 100644 --- a/core/src/main/java/org/springframework/security/userdetails/decorator/StatusCheckingUserDetailsService.java +++ b/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 UserDetailsService. - * * @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; } } diff --git a/core/src/test/java/org/springframework/security/providers/siteminder/SiteminderAuthenticationProviderTests.java b/core/src/test/java/org/springframework/security/providers/siteminder/SiteminderAuthenticationProviderTests.java index f43ab7e81c..652078ff23 100644 --- a/core/src/test/java/org/springframework/security/providers/siteminder/SiteminderAuthenticationProviderTests.java +++ b/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"); diff --git a/core/src/test/java/org/springframework/security/ui/AbstractProcessingFilterTests.java b/core/src/test/java/org/springframework/security/ui/AbstractProcessingFilterTests.java index 5bb301caaa..dcc9842871 100644 --- a/core/src/test/java/org/springframework/security/ui/AbstractProcessingFilterTests.java +++ b/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()); } diff --git a/core/src/test/java/org/springframework/security/userdetails/decorator/StatusCheckingUserDetailsServiceTests.java b/core/src/test/java/org/springframework/security/userdetails/decorator/StatusCheckingUserDetailsServiceTests.java deleted file mode 100644 index 56825057ec..0000000000 --- a/core/src/test/java/org/springframework/security/userdetails/decorator/StatusCheckingUserDetailsServiceTests.java +++ /dev/null @@ -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"); - } - -} diff --git a/openid/src/main/java/org/springframework/security/providers/openid/OpenIDAuthenticationProvider.java b/openid/src/main/java/org/springframework/security/providers/openid/OpenIDAuthenticationProvider.java index 101089f7f7..d26894572b 100644 --- a/openid/src/main/java/org/springframework/security/providers/openid/OpenIDAuthenticationProvider.java +++ b/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 UserDetails because this is * authentication-related and should have been enforced by another provider server. *

- * You can optionally have these checked by configuring wrapping the UserDetailsService in a - * {@link org.springframework.security.userdetails.decorator.StatusCheckingUserDetailsService} decorator. - *

* The UserDetails returned by implementations is stored in the generated AuthenticationToken, * so additional properties such as email addresses, telephone numbers etc can easily be stored. *