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.

2026-08-05 09:47:05 +00:00 · 2009-12-13 17:37:24 +00:00
parent ef3d9c7877
commit cad32ffe39
9 changed files with 41 additions and 53 deletions
@@ -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;
        }

@@ -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 <tt>GrantedAuthority</tt>s for the
-     *                    principal represented by this authentication object. A
-     *                    <code>null</code> value indicates that no authorities have been
-     *                    granted (pursuant to the interface contract specified by {@link
-     *                    Authentication#getAuthorities()}<code>null</code> should only be
-     *                    presented if the principal has not been authenticated).
+     * @param authorities the collection of <tt>GrantedAuthority</tt>s for the
+     *                    principal represented by this authentication object.
     */
    public AbstractAuthenticationToken(Collection<GrantedAuthority> 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<GrantedAuthority> temp = new ArrayList<GrantedAuthority>(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<GrantedAuthority> temp = new ArrayList<GrantedAuthority>(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;
@@ -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<GrantedAuthority>();
-
-            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<Principal> principals = loginContext.getSubject().getPrincipals();
@@ -51,10 +51,14 @@ public interface Authentication extends Principal, Serializable {
    /**
     * Set by an <code>AuthenticationManager</code> 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
-     * <code>AuthenticationManager</code>.<p>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).</p>
+     * <code>AuthenticationManager</code>.
+     * <p>
+     * Implementations should ensure that modifications to the returned collection
+     * array do not affect the state of the Authentication object, or use an unmodifiable instance.
+     * </p>
     *
-     * @return the authorities granted to the principal, or <code>null</code> 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<GrantedAuthority> getAuthorities();

@@ -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 <code>null</code> 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;