From cad32ffe399025892b18cc67c35b50c4a6146cbf Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Sun, 13 Dec 2009 17:37:24 +0000 Subject: [PATCH] SEC-1325: Tighten up Authentication interface contract to disallow null authorities. Modified internals of AbstractAuthenticationToken to use an empty list instead of null. Clarified Javadoc. removed unnecessary null checks in classes which use the interface. --- .../MethodInvocationPrivilegeEvaluator.java | 3 +- .../AbstractAuthenticationToken.java | 50 ++++++++----------- .../jaas/JaasAuthenticationProvider.java | 5 +- .../security/core/Authentication.java | 10 ++-- .../security/core/userdetails/User.java | 6 +-- .../ldap/userdetails/LdapUserDetailsImpl.java | 9 ++-- .../taglibs/authz/LegacyAuthorizeTag.java | 4 -- ...efaultWebInvocationPrivilegeEvaluator.java | 3 +- ...AuthenticatedAuthenticationTokenTests.java | 4 +- 9 files changed, 41 insertions(+), 53 deletions(-) diff --git a/core/src/main/java/org/springframework/security/access/intercept/MethodInvocationPrivilegeEvaluator.java b/core/src/main/java/org/springframework/security/access/intercept/MethodInvocationPrivilegeEvaluator.java index 33050df604..acf2a815f5 100644 --- a/core/src/main/java/org/springframework/security/access/intercept/MethodInvocationPrivilegeEvaluator.java +++ b/core/src/main/java/org/springframework/security/access/intercept/MethodInvocationPrivilegeEvaluator.java @@ -68,8 +68,7 @@ public class MethodInvocationPrivilegeEvaluator implements InitializingBean { return true; } - if ((authentication == null) || (authentication.getAuthorities() == null) - || (authentication.getAuthorities().isEmpty())) { + if (authentication == null || authentication.getAuthorities().isEmpty()) { return false; } diff --git a/core/src/main/java/org/springframework/security/authentication/AbstractAuthenticationToken.java b/core/src/main/java/org/springframework/security/authentication/AbstractAuthenticationToken.java index 6dbd9f95cd..54cdfa1dd8 100644 --- a/core/src/main/java/org/springframework/security/authentication/AbstractAuthenticationToken.java +++ b/core/src/main/java/org/springframework/security/authentication/AbstractAuthenticationToken.java @@ -22,6 +22,7 @@ import java.util.Collections; import org.springframework.security.core.Authentication; import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.authority.AuthorityUtils; import org.springframework.security.core.userdetails.UserDetails; @@ -46,26 +47,23 @@ public abstract class AbstractAuthenticationToken implements Authentication { /** * Creates a token with the supplied array of authorities. * - * @param authorities the list of GrantedAuthoritys for the - * principal represented by this authentication object. A - * null value indicates that no authorities have been - * granted (pursuant to the interface contract specified by {@link - * Authentication#getAuthorities()}null should only be - * presented if the principal has not been authenticated). + * @param authorities the collection of GrantedAuthoritys for the + * principal represented by this authentication object. */ public AbstractAuthenticationToken(Collection authorities) { if (authorities == null) { - this.authorities = null; - } else { - for (GrantedAuthority a: authorities) { - if(a == null) { - throw new IllegalArgumentException("Authorities collection cannot contain any null elements"); - } - } - ArrayList temp = new ArrayList(authorities.size()); - temp.addAll(authorities); - this.authorities = Collections.unmodifiableList(temp); + this.authorities = AuthorityUtils.NO_AUTHORITIES; + return; } + + for (GrantedAuthority a: authorities) { + if (a == null) { + throw new IllegalArgumentException("Authorities collection cannot contain any null elements"); + } + } + ArrayList temp = new ArrayList(authorities.size()); + temp.addAll(authorities); + this.authorities = Collections.unmodifiableList(temp); } //~ Methods ======================================================================================================== @@ -77,14 +75,8 @@ public abstract class AbstractAuthenticationToken implements Authentication { AbstractAuthenticationToken test = (AbstractAuthenticationToken) obj; - if (!(authorities == null && test.authorities == null)) { - // Not both null - if (authorities == null || test.authorities == null) { - return false; - } - if(!authorities.equals(test.authorities)) { - return false; - } + if (!authorities.equals(test.authorities)) { + return false; } if ((this.details == null) && (test.getDetails() != null)) { @@ -141,10 +133,8 @@ public abstract class AbstractAuthenticationToken implements Authentication { public int hashCode() { int code = 31; - if (authorities != null) { - for (GrantedAuthority authority : authorities) { - code ^= authority.hashCode(); - } + for (GrantedAuthority authority : authorities) { + code ^= authority.hashCode(); } if (this.getPrincipal() != null) { @@ -179,14 +169,14 @@ public abstract class AbstractAuthenticationToken implements Authentication { } public String toString() { - StringBuffer sb = new StringBuffer(); + StringBuilder sb = new StringBuilder(); sb.append(super.toString()).append(": "); sb.append("Principal: ").append(this.getPrincipal()).append("; "); sb.append("Password: [PROTECTED]; "); sb.append("Authenticated: ").append(this.isAuthenticated()).append("; "); sb.append("Details: ").append(this.getDetails()).append("; "); - if (authorities != null) { + if (!authorities.isEmpty()) { sb.append("Granted Authorities: "); int i = 0; diff --git a/core/src/main/java/org/springframework/security/authentication/jaas/JaasAuthenticationProvider.java b/core/src/main/java/org/springframework/security/authentication/jaas/JaasAuthenticationProvider.java index c518dec5f0..be51530e9b 100644 --- a/core/src/main/java/org/springframework/security/authentication/jaas/JaasAuthenticationProvider.java +++ b/core/src/main/java/org/springframework/security/authentication/jaas/JaasAuthenticationProvider.java @@ -185,10 +185,7 @@ public class JaasAuthenticationProvider implements AuthenticationProvider, Appli // Create a set to hold the authorities, and add any that have already been applied. authorities = new HashSet(); - - if (request.getAuthorities() != null) { - authorities.addAll(request.getAuthorities()); - } + authorities.addAll(request.getAuthorities()); // Get the subject principals and pass them to each of the AuthorityGranters Set principals = loginContext.getSubject().getPrincipals(); diff --git a/core/src/main/java/org/springframework/security/core/Authentication.java b/core/src/main/java/org/springframework/security/core/Authentication.java index bc7e320d2b..23caff103f 100644 --- a/core/src/main/java/org/springframework/security/core/Authentication.java +++ b/core/src/main/java/org/springframework/security/core/Authentication.java @@ -51,10 +51,14 @@ public interface Authentication extends Principal, Serializable { /** * Set by an AuthenticationManager to indicate the authorities that the principal has been * granted. Note that classes should not rely on this value as being valid unless it has been set by a trusted - * AuthenticationManager.

Implementations should ensure that modifications to the returned - * array do not affect the state of the Authentication object (e.g. by returning an array copy).

+ * AuthenticationManager. + *

+ * Implementations should ensure that modifications to the returned collection + * array do not affect the state of the Authentication object, or use an unmodifiable instance. + *

* - * @return the authorities granted to the principal, or null if authentication has not been completed + * @return the authorities granted to the principal, or an empty collection if the token has not been authenticated. + * Never null. */ Collection getAuthorities(); diff --git a/core/src/main/java/org/springframework/security/core/userdetails/User.java b/core/src/main/java/org/springframework/security/core/userdetails/User.java index e8d4eaf721..4b9d110ddd 100644 --- a/core/src/main/java/org/springframework/security/core/userdetails/User.java +++ b/core/src/main/java/org/springframework/security/core/userdetails/User.java @@ -74,7 +74,7 @@ public class User implements UserDetails { * locked * @param authorities the authorities that should be granted to the caller * if they presented the correct username and password and the user - * is enabled + * is enabled. Not null. * * @throws IllegalArgumentException if a null value was passed * either as a parameter or as an element in the @@ -210,7 +210,7 @@ public class User implements UserDetails { } public String toString() { - StringBuffer sb = new StringBuffer(); + StringBuilder sb = new StringBuilder(); sb.append(super.toString()).append(": "); sb.append("Username: ").append(this.username).append("; "); sb.append("Password: [PROTECTED]; "); @@ -219,7 +219,7 @@ public class User implements UserDetails { sb.append("credentialsNonExpired: ").append(this.credentialsNonExpired).append("; "); sb.append("AccountNonLocked: ").append(this.accountNonLocked).append("; "); - if (this.getAuthorities() != null) { + if (!authorities.isEmpty()) { sb.append("Granted Authorities: "); boolean first = true; diff --git a/ldap/src/main/java/org/springframework/security/ldap/userdetails/LdapUserDetailsImpl.java b/ldap/src/main/java/org/springframework/security/ldap/userdetails/LdapUserDetailsImpl.java index 13cb273f5a..d4c8047b48 100644 --- a/ldap/src/main/java/org/springframework/security/ldap/userdetails/LdapUserDetailsImpl.java +++ b/ldap/src/main/java/org/springframework/security/ldap/userdetails/LdapUserDetailsImpl.java @@ -17,6 +17,8 @@ package org.springframework.security.ldap.userdetails; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; +import java.util.List; import javax.naming.Name; @@ -140,7 +142,7 @@ public class LdapUserDetailsImpl implements LdapUserDetails, PasswordPolicyData */ public static class Essence { protected LdapUserDetailsImpl instance = createTarget(); - private Collection mutableAuthorities = new ArrayList(); + private List mutableAuthorities = new ArrayList(); public Essence() { } @@ -184,7 +186,7 @@ public class LdapUserDetailsImpl implements LdapUserDetails, PasswordPolicyData Assert.notNull(instance.username, "username must not be null"); Assert.notNull(instance.getDn(), "Distinguished name must not be null"); - instance.authorities = getGrantedAuthorities(); + instance.authorities = Collections.unmodifiableList(mutableAuthorities); LdapUserDetails newInstance = instance; @@ -206,7 +208,8 @@ public class LdapUserDetailsImpl implements LdapUserDetails, PasswordPolicyData } public void setAuthorities(Collection authorities) { - mutableAuthorities = authorities; + mutableAuthorities = new ArrayList(); + mutableAuthorities.addAll(authorities); } public void setCredentialsNonExpired(boolean credentialsNonExpired) { diff --git a/taglibs/src/main/java/org/springframework/security/taglibs/authz/LegacyAuthorizeTag.java b/taglibs/src/main/java/org/springframework/security/taglibs/authz/LegacyAuthorizeTag.java index 725aa259f7..edd6810cb5 100644 --- a/taglibs/src/main/java/org/springframework/security/taglibs/authz/LegacyAuthorizeTag.java +++ b/taglibs/src/main/java/org/springframework/security/taglibs/authz/LegacyAuthorizeTag.java @@ -125,10 +125,6 @@ public class LegacyAuthorizeTag extends TagSupport { return Collections.emptyList(); } - if ((null == currentUser.getAuthorities())) { - return Collections.emptyList(); - } - return currentUser.getAuthorities(); } diff --git a/web/src/main/java/org/springframework/security/web/access/DefaultWebInvocationPrivilegeEvaluator.java b/web/src/main/java/org/springframework/security/web/access/DefaultWebInvocationPrivilegeEvaluator.java index 671a41966b..b2919908b0 100644 --- a/web/src/main/java/org/springframework/security/web/access/DefaultWebInvocationPrivilegeEvaluator.java +++ b/web/src/main/java/org/springframework/security/web/access/DefaultWebInvocationPrivilegeEvaluator.java @@ -130,8 +130,7 @@ public class DefaultWebInvocationPrivilegeEvaluator implements WebInvocationPriv return true; } - if ((authentication == null) || (authentication.getAuthorities() == null) - || authentication.getAuthorities().isEmpty()) { + if (authentication == null || authentication.getAuthorities().isEmpty()) { return false; } diff --git a/web/src/test/java/org/springframework/security/web/authentication/preauth/PreAuthenticatedAuthenticationTokenTests.java b/web/src/test/java/org/springframework/security/web/authentication/preauth/PreAuthenticatedAuthenticationTokenTests.java index 56e7bf43cd..1deebc46e3 100755 --- a/web/src/test/java/org/springframework/security/web/authentication/preauth/PreAuthenticatedAuthenticationTokenTests.java +++ b/web/src/test/java/org/springframework/security/web/authentication/preauth/PreAuthenticatedAuthenticationTokenTests.java @@ -25,7 +25,7 @@ public class PreAuthenticatedAuthenticationTokenTests extends TestCase { assertEquals(principal, token.getPrincipal()); assertEquals(credentials, token.getCredentials()); assertEquals(details, token.getDetails()); - assertNull(token.getAuthorities()); + assertTrue(token.getAuthorities().isEmpty()); } public void testPreAuthenticatedAuthenticationTokenRequestWithoutDetails() { @@ -35,7 +35,7 @@ public class PreAuthenticatedAuthenticationTokenTests extends TestCase { assertEquals(principal, token.getPrincipal()); assertEquals(credentials, token.getCredentials()); assertNull(token.getDetails()); - assertNull(token.getAuthorities()); + assertTrue(token.getAuthorities().isEmpty()); } public void testPreAuthenticatedAuthenticationTokenResponse() {