From 84c7ac5e57dd2655fe2d2ec09c075408b7160626 Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Mon, 4 Feb 2008 21:26:07 +0000 Subject: [PATCH] SEC-664: Removed validateUserDetails method from AbstractRememberMeServices, wrapped the UserDetailsService in a status-checking one and added a catch block for AccountStatusExceptions. Also some minor tidying up of other remember-me classes. --- .../AbstractRememberMeServices.java | 24 +++++++------------ .../rememberme/JdbcTokenRepositoryImpl.java | 1 + ...ersistentTokenBasedRememberMeServices.java | 3 +-- .../rememberme/PersistentTokenRepository.java | 3 ++- .../TokenBasedRememberMeServices.java | 17 ++++--------- .../AbstractRememberMeServicesTests.java | 1 - .../TokenBasedRememberMeServicesTests.java | 15 ++++-------- .../applicationContext-security-ns.xml | 6 ++--- 8 files changed, 25 insertions(+), 45 deletions(-) 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 5554e80661..20b6c1a021 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 @@ -7,6 +7,7 @@ import org.springframework.beans.factory.InitializingBean; import org.springframework.context.support.MessageSourceAccessor; import org.springframework.security.Authentication; import org.springframework.security.SpringSecurityMessageSource; +import org.springframework.security.AccountStatusException; import org.springframework.security.providers.rememberme.RememberMeAuthenticationToken; import org.springframework.security.ui.AuthenticationDetailsSource; import org.springframework.security.ui.AuthenticationDetailsSourceImpl; @@ -14,6 +15,7 @@ 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.util.Assert; import org.springframework.util.StringUtils; import org.springframework.web.bind.ServletRequestUtils; @@ -27,6 +29,7 @@ import javax.servlet.http.HttpServletResponse; * * @author Luke Taylor * @version $Id$ + * @since 2.0 */ public abstract class AbstractRememberMeServices implements RememberMeServices, InitializingBean, LogoutHandler { //~ Static fields/initializers ===================================================================================== @@ -91,6 +94,10 @@ public abstract class AbstractRememberMeServices implements RememberMeServices, cancelCookie(request, response); logger.debug("Invalid remember-me cookie: " + invalidCookie.getMessage()); return null; + } catch (AccountStatusException statusInvalid) { + cancelCookie(request, response); + logger.debug("Invalid UserDetails: " + statusInvalid.getMessage()); + return null; } catch (RememberMeAuthenticationException e) { cancelCookie(request, response); logger.debug(e.getMessage()); @@ -175,21 +182,6 @@ public abstract class AbstractRememberMeServices implements RememberMeServices, return sb.toString(); } - /** - * Provided for subclass convenience to check the account status of a loaded user. - * - * @throws UsernameNotFoundException if the username could not be located by the configured UserDetailsService. - * @throws RememberMeAuthenticationException if the account is locked or disabled. - */ - protected void validateUserDetails(UserDetails user) throws UsernameNotFoundException, - RememberMeAuthenticationException { - - if (!user.isAccountNonExpired() || !user.isCredentialsNonExpired() || !user.isEnabled()) { - throw new RememberMeAuthenticationException("Remember-me login was valid for user " + - user.getUsername() + ", but account is expired, has expired credentials or is disabled"); - } - } - public final void loginFail(HttpServletRequest request, HttpServletResponse response) { logger.debug("Interactive login attempt was unsuccessful."); cancelCookie(request, response); @@ -327,7 +319,7 @@ public abstract class AbstractRememberMeServices implements RememberMeServices, } public void setUserDetailsService(UserDetailsService userDetailsService) { - this.userDetailsService = userDetailsService; + this.userDetailsService = new StatusCheckingUserDetailsService(userDetailsService); } public void setKey(String key) { diff --git a/core/src/main/java/org/springframework/security/ui/rememberme/JdbcTokenRepositoryImpl.java b/core/src/main/java/org/springframework/security/ui/rememberme/JdbcTokenRepositoryImpl.java index a80fc5e01a..11b3f43b57 100644 --- a/core/src/main/java/org/springframework/security/ui/rememberme/JdbcTokenRepositoryImpl.java +++ b/core/src/main/java/org/springframework/security/ui/rememberme/JdbcTokenRepositoryImpl.java @@ -18,6 +18,7 @@ import java.util.Date; * * @author Luke Taylor * @version $Id$ + * @since 2.0 */ public class JdbcTokenRepositoryImpl extends JdbcDaoSupport implements PersistentTokenRepository { //~ Static fields/initializers ===================================================================================== diff --git a/core/src/main/java/org/springframework/security/ui/rememberme/PersistentTokenBasedRememberMeServices.java b/core/src/main/java/org/springframework/security/ui/rememberme/PersistentTokenBasedRememberMeServices.java index 88ee6b5b8b..986d74e300 100644 --- a/core/src/main/java/org/springframework/security/ui/rememberme/PersistentTokenBasedRememberMeServices.java +++ b/core/src/main/java/org/springframework/security/ui/rememberme/PersistentTokenBasedRememberMeServices.java @@ -34,6 +34,7 @@ import java.util.Date; * * @author Luke Taylor * @version $Id$ + * @since 2.0 */ public class PersistentTokenBasedRememberMeServices extends AbstractRememberMeServices { @@ -112,8 +113,6 @@ public class PersistentTokenBasedRememberMeServices extends AbstractRememberMeSe UserDetails user = getUserDetailsService().loadUserByUsername(token.getUsername()); - validateUserDetails(user); - return user; } diff --git a/core/src/main/java/org/springframework/security/ui/rememberme/PersistentTokenRepository.java b/core/src/main/java/org/springframework/security/ui/rememberme/PersistentTokenRepository.java index f5e051a347..4dbcca20f2 100644 --- a/core/src/main/java/org/springframework/security/ui/rememberme/PersistentTokenRepository.java +++ b/core/src/main/java/org/springframework/security/ui/rememberme/PersistentTokenRepository.java @@ -7,10 +7,11 @@ import java.util.Date; * login tokens for a user. * * @see JdbcTokenRepositoryImpl - * @see InMemoryTokenRepositoryImpl + * @see InMemoryTokenRepositoryImpl * * @author Luke Taylor * @version $Id$ + * @since 2.0 */ public interface PersistentTokenRepository { diff --git a/core/src/main/java/org/springframework/security/ui/rememberme/TokenBasedRememberMeServices.java b/core/src/main/java/org/springframework/security/ui/rememberme/TokenBasedRememberMeServices.java index 3ea087b93c..0c2f6dac6f 100644 --- a/core/src/main/java/org/springframework/security/ui/rememberme/TokenBasedRememberMeServices.java +++ b/core/src/main/java/org/springframework/security/ui/rememberme/TokenBasedRememberMeServices.java @@ -39,16 +39,12 @@ import java.util.Date; * credentials - not the time period they last logged in via remember-me. The * implementation will only send a remember-me token if the parameter defined by * {@link #setParameter(String)} is present. - *

* *

* An {@link org.springframework.security.userdetails.UserDetailsService} is required by * this implementation, so that it can construct a valid - * Authentication from the returned {@link - * org.springframework.security.userdetails.UserDetails}. This is also necessary so that - * the user's password is available and can be checked as part of the encoded - * cookie. - *

+ * Authentication from the returned {@link org.springframework.security.userdetails.UserDetails}. + * This is also necessary so that the user's password is available and can be checked as part of the encoded cookie. * *

* The cookie encoded by this implementation adopts the following form: @@ -57,7 +53,6 @@ import java.util.Date; * username + ":" + expiryTime + ":" + Md5Hex(username + ":" + expiryTime + ":" + password + ":" + key) * * - *

*

* As such, if the user changes their password, any remember-me token will be * invalidated. Equally, the system administrator may invalidate every @@ -69,17 +64,17 @@ import java.util.Date; * implementation (as we do not want to rely on a database for remember-me * services) and as such high security applications should be aware of this * occasionally undesired disclosure of a valid username. - *

+ * *

* This is a basic remember-me implementation which is suitable for many * applications. However, we recommend a database-based implementation if you * require a more secure remember-me approach (see {@link PersistentTokenBasedRememberMeServices}). - *

+ * *

* By default the tokens will be valid for 14 days from the last successful * authentication attempt. This can be changed using * {@link #setTokenValiditySeconds(int)}. - *

+ * * * @author Ben Alex * @version $Id$ @@ -116,8 +111,6 @@ public class TokenBasedRememberMeServices extends AbstractRememberMeServices { UserDetails userDetails = getUserDetailsService().loadUserByUsername(cookieTokens[0]); - validateUserDetails(userDetails); - // Check signature of token matches remaining details. // Must do this after user lookup, as we need the DAO-derived password. // If efficiency was a major issue, just add in a UserCache implementation, diff --git a/core/src/test/java/org/springframework/security/ui/rememberme/AbstractRememberMeServicesTests.java b/core/src/test/java/org/springframework/security/ui/rememberme/AbstractRememberMeServicesTests.java index 5ea623f2f7..57c8317996 100644 --- a/core/src/test/java/org/springframework/security/ui/rememberme/AbstractRememberMeServicesTests.java +++ b/core/src/test/java/org/springframework/security/ui/rememberme/AbstractRememberMeServicesTests.java @@ -259,7 +259,6 @@ public class AbstractRememberMeServicesTests { } UserDetails user = getUserDetailsService().loadUserByUsername("joe"); - validateUserDetails(user); return user; } diff --git a/core/src/test/java/org/springframework/security/ui/rememberme/TokenBasedRememberMeServicesTests.java b/core/src/test/java/org/springframework/security/ui/rememberme/TokenBasedRememberMeServicesTests.java index 1f9d04e5ce..33b1d71588 100644 --- a/core/src/test/java/org/springframework/security/ui/rememberme/TokenBasedRememberMeServicesTests.java +++ b/core/src/test/java/org/springframework/security/ui/rememberme/TokenBasedRememberMeServicesTests.java @@ -84,8 +84,7 @@ public class TokenBasedRememberMeServicesTests extends TestCase { return tokenValueBase64; } - public void testAutoLoginIfDoesNotPresentAnyCookies() - throws Exception { + public void testAutoLoginIfDoesNotPresentAnyCookies() throws Exception { TokenBasedRememberMeServices services = new TokenBasedRememberMeServices(); services.setKey("key"); services.setUserDetailsService(new MockAuthenticationDao(null, true)); @@ -104,8 +103,7 @@ public class TokenBasedRememberMeServicesTests extends TestCase { assertNull(returnedCookie); // shouldn't try to invalidate our cookie } - public void testAutoLoginIfDoesNotPresentRequiredCookie() - throws Exception { + public void testAutoLoginIfDoesNotPresentRequiredCookie() throws Exception { TokenBasedRememberMeServices services = new TokenBasedRememberMeServices(); services.setKey("key"); services.setUserDetailsService(new MockAuthenticationDao(null, true)); @@ -150,8 +148,7 @@ public class TokenBasedRememberMeServicesTests extends TestCase { assertEquals(0, returnedCookie.getMaxAge()); } - public void testAutoLoginIfMissingThreeTokensInCookieValue() - throws Exception { + public void testAutoLoginIfMissingThreeTokensInCookieValue() throws Exception { UserDetails user = new User("someone", "password", true, true, true, true, new GrantedAuthority[] {new GrantedAuthorityImpl("ROLE_ABC")}); @@ -201,8 +198,7 @@ public class TokenBasedRememberMeServicesTests extends TestCase { assertEquals(0, returnedCookie.getMaxAge()); } - public void testAutoLoginIfSignatureBlocksDoesNotMatchExpectedValue() - throws Exception { + public void testAutoLoginIfSignatureBlocksDoesNotMatchExpectedValue() throws Exception { UserDetails user = new User("someone", "password", true, true, true, true, new GrantedAuthority[] {new GrantedAuthorityImpl("ROLE_ABC")}); @@ -228,8 +224,7 @@ public class TokenBasedRememberMeServicesTests extends TestCase { assertEquals(0, returnedCookie.getMaxAge()); } - public void testAutoLoginIfTokenDoesNotContainANumberInCookieValue() - throws Exception { + public void testAutoLoginIfTokenDoesNotContainANumberInCookieValue() throws Exception { UserDetails user = new User("someone", "password", true, true, true, true, new GrantedAuthority[] {new GrantedAuthorityImpl("ROLE_ABC")}); diff --git a/samples/tutorial/src/main/webapp/WEB-INF/applicationContext-security-ns.xml b/samples/tutorial/src/main/webapp/WEB-INF/applicationContext-security-ns.xml index 93d97696b0..81636d2a4e 100644 --- a/samples/tutorial/src/main/webapp/WEB-INF/applicationContext-security-ns.xml +++ b/samples/tutorial/src/main/webapp/WEB-INF/applicationContext-security-ns.xml @@ -23,9 +23,9 @@ --> ---> + @@ -68,7 +68,7 @@ Uncomment to authenticate against an embedded LDAP server. - +