Tiding up code in acl package (formatting, reduction onf nesting etc).

core/src/main/java/org/springframework/security/acls/AclFormattingUtils.java

@@ -91,7 +91,9 @@ public final class AclFormattingUtils {
 
     /**
      * Returns a representation of the active bits in the presented mask, with each active bit being denoted by
-     * the passed character.<p>Inactive bits will be denoted by character {@link Permission#RESERVED_OFF}.</p>
+     * the passed character.
+     * <p>
+     * Inactive bits will be denoted by character {@link Permission#RESERVED_OFF}.
      *
      * @param mask the integer bit mask to print the active bits for
      * @param code the character to print when an active bit is detected
@@ -99,12 +101,11 @@ public final class AclFormattingUtils {
      * @return a 32-character representation of the bit mask
      */
     public static String printBinary(int mask, char code) {
-        Assert.doesNotContain(new Character(code).toString(), new Character(Permission.RESERVED_ON).toString(),
+        Assert.doesNotContain(Character.toString(code), Character.toString(Permission.RESERVED_ON),
             Permission.RESERVED_ON + " is a reserved character code");
-        Assert.doesNotContain(new Character(code).toString(), new Character(Permission.RESERVED_OFF).toString(),
+        Assert.doesNotContain(Character.toString(code), Character.toString(Permission.RESERVED_OFF),
             Permission.RESERVED_OFF + " is a reserved character code");
 
-        return printBinary(mask, Permission.RESERVED_ON, Permission.RESERVED_OFF)
+        return printBinary(mask, Permission.RESERVED_ON, Permission.RESERVED_OFF).replace(Permission.RESERVED_ON, code);
-                                 .replace(Permission.RESERVED_ON, code);
     }
 }

core/src/main/java/org/springframework/security/acls/domain/BasePermission.java

@@ -22,9 +22,7 @@ import org.springframework.util.Assert;
 import java.lang.reflect.Field;
 
 import java.util.HashMap;
-import java.util.List;
 import java.util.Map;
-import java.util.Vector;
 
 
 /**
@@ -108,16 +106,16 @@ public final class BasePermission implements Permission {
 
     public static Permission[] buildFromMask(int[] masks) {
         if ((masks == null) || (masks.length == 0)) {
-            return new Permission[] {};
+            return new Permission[0];
         }
 
-        List list = new Vector();
+        Permission[] permissions = new Permission[masks.length];
 
         for (int i = 0; i < masks.length; i++) {
-            list.add(BasePermission.buildFromMask(masks[i]));
+            permissions[i] = buildFromMask(masks[i]);
         }
 
-        return (Permission[]) list.toArray(new Permission[] {});
+        return permissions;
     }
 
     public static Permission buildFromName(String name) {
@@ -128,16 +126,16 @@ public final class BasePermission implements Permission {
 
     public static Permission[] buildFromName(String[] names) {
         if ((names == null) || (names.length == 0)) {
-            return new Permission[] {};
+            return new Permission[0];
         }
 
-        List list = new Vector();
+        Permission[] permissions = new Permission[names.length];
 
         for (int i = 0; i < names.length; i++) {
-            list.add(BasePermission.buildFromName(names[i]));
+            permissions[i] = buildFromName(names[i]);
         }
 
-        return (Permission[]) list.toArray(new Permission[] {});
+        return permissions;
     }
 
     public boolean equals(Object arg0) {

core/src/main/java/org/springframework/security/acls/jdbc/JdbcAclService.java

@@ -39,9 +39,11 @@ import javax.sql.DataSource;
 
 
 /**
- * Simple JDBC-based implementation of <code>AclService</code>.<p>Requires the "dirty" flags in {@link
+ * Simple JDBC-based implementation of <code>AclService</code>.
- * org.springframework.security.acls.domain.AclImpl} and {@link org.springframework.security.acls.domain.AccessControlEntryImpl} to be set,
+ * <p>
- * so that the implementation can detect changed parameters easily.</p>
+ * Requires the "dirty" flags in {@link org.springframework.security.acls.domain.AclImpl} and 
+ * {@link org.springframework.security.acls.domain.AccessControlEntryImpl} to be set, so that the implementation can
+ * detect changed parameters easily.
  *
  * @author Ben Alex
  * @version $Id$
@@ -88,15 +90,14 @@ public class JdbcAclService implements AclService {
         return (ObjectIdentityImpl[]) objects.toArray(new ObjectIdentityImpl[] {});
     }
 
-    public Acl readAclById(ObjectIdentity object, Sid[] sids)
+    public Acl readAclById(ObjectIdentity object, Sid[] sids) throws NotFoundException {
-        throws NotFoundException {
         Map map = readAclsById(new ObjectIdentity[] {object}, sids);
 
         if (map.size() == 0) {
             throw new NotFoundException("Could not find ACL");
-        } else {
-            return (Acl) map.get(object);
         }
+
+        return (Acl) map.get(object);
     }
 
     public Acl readAclById(ObjectIdentity object) throws NotFoundException {
@@ -107,8 +108,7 @@ public class JdbcAclService implements AclService {
         return readAclsById(objects, null);
     }
 
-    public Map readAclsById(ObjectIdentity[] objects, Sid[] sids)
+    public Map readAclsById(ObjectIdentity[] objects, Sid[] sids) throws NotFoundException {
-        throws NotFoundException {
         return lookupStrategy.readAclsById(objects, sids);
     }
 }

core/src/main/java/org/springframework/security/acls/jdbc/LookupStrategy.java

@@ -21,7 +21,7 @@ import java.util.Map;
 
 
 /**
- * Performs optimised lookups for {@link JdbcAclService}.
+ * Performs lookups for {@link org.springframework.security.acls.AclService}.
  *
  * @author Ben Alex
  * @version $Id$

core/src/main/java/org/springframework/security/acls/objectidentity/ObjectIdentityImpl.java

@@ -25,11 +25,13 @@ import java.lang.reflect.Method;
 
 
 /**
- * Simple implementation of {@link org.springframework.security.acl.basic.AclObjectIdentity AclObjectIdentity}.
+ * Simple implementation of {@link ObjectIdentity}.
  * <p>
  * Uses <code>String</code>s to store the identity of the domain object instance. Also offers a constructor that uses
  * reflection to build the identity information.
- * </p>
+ *
+ * @author Ben Alex
+ * @version $Id$
  */
 public class ObjectIdentityImpl implements ObjectIdentity {
     //~ Instance fields ================================================================================================

core/src/main/java/org/springframework/security/acls/sid/PrincipalSid.java

@@ -44,10 +44,11 @@ public class PrincipalSid implements Sid {
     public PrincipalSid(Authentication authentication) {
         Assert.notNull(authentication, "Authentication required");
         Assert.notNull(authentication.getPrincipal(), "Principal required");
-        this.principal = authentication.getPrincipal().toString();
 
         if (authentication.getPrincipal() instanceof UserDetails) {
             this.principal = ((UserDetails) authentication.getPrincipal()).getUsername();
+        } else {
+            this.principal = authentication.getPrincipal().toString();
         }
     }
 

core/src/main/java/org/springframework/security/acls/sid/SidRetrievalStrategyImpl.java

@@ -18,14 +18,11 @@ package org.springframework.security.acls.sid;
 import org.springframework.security.Authentication;
 import org.springframework.security.GrantedAuthority;
 
-import java.util.List;
-import java.util.Vector;
-
-
 /**
  * Basic implementation of {@link SidRetrievalStrategy} that creates a {@link Sid} for the principal, as well as
- * every granted authority the principal holds.<p>The returned array will always contain the {@link PrincipalSid}
+ * every granted authority the principal holds.
- * before any {@link GrantedAuthoritySid} elements.</p>
+ * <p>
+ * The returned array will always contain the {@link PrincipalSid} before any {@link GrantedAuthoritySid} elements.
  *
  * @author Ben Alex
  * @version $Id$
@@ -34,15 +31,15 @@ public class SidRetrievalStrategyImpl implements SidRetrievalStrategy {
     //~ Methods ========================================================================================================
 
     public Sid[] getSids(Authentication authentication) {
-        List list = new Vector();
-        list.add(new PrincipalSid(authentication));
-
         GrantedAuthority[] authorities = authentication.getAuthorities();
+        Sid[] sids = new Sid[authorities.length + 1];
 
-        for (int i = 0; i < authorities.length; i++) {
+        sids[0] = new PrincipalSid(authentication);
-            list.add(new GrantedAuthoritySid(authorities[i]));
+
+        for (int i = 1; i <= authorities.length; i++) {
+            sids[i] = new GrantedAuthoritySid(authorities[i - 1]);
         }
 
-        return (Sid[]) list.toArray(new Sid[] {});
+        return sids;
     }
 }

core/src/main/java/org/springframework/security/afterinvocation/AbstractAclProvider.java

@@ -110,11 +110,7 @@ public abstract class AbstractAclProvider implements AfterInvocationProvider {
     }
 
     public boolean supports(ConfigAttribute attribute) {
-        if ((attribute.getAttribute() != null) && attribute.getAttribute().equals(this.processConfigAttribute)) {
+        return processConfigAttribute.equals(attribute.getAttribute());
-            return true;
-        } else {
-            return false;
-        }
     }
 
     /**

core/src/main/java/org/springframework/security/afterinvocation/AclEntryAfterInvocationCollectionFilteringProvider.java

@@ -31,26 +31,32 @@ import java.util.Iterator;
 
 
 /**
- * <p>Given a <code>Collection</code> of domain object instances returned from a secure object invocation, remove
+ * <p>
+ * Given a <code>Collection</code> of domain object instances returned from a secure object invocation, remove
  * any <code>Collection</code> elements the principal does not have appropriate permission to access as defined by the
- * {@link AclService}.</p>
+ * {@link AclService}.
- *  <p>The <code>AclService</code> is used to retrieve the access control list (ACL) permissions associated with
+ * <p>
- * each <code>Collection</code> domain object instance element for the current <code>Authentication</code> object.</p>
+ * The <code>AclService</code> is used to retrieve the access control list (ACL) permissions associated with
- *  <p>This after invocation provider will fire if any {@link ConfigAttribute#getAttribute()} matches the {@link
+ * each <code>Collection</code> domain object instance element for the current <code>Authentication</code> object.
+ * <p>
+ * This after invocation provider will fire if any {@link ConfigAttribute#getAttribute()} matches the {@link
  * #processConfigAttribute}. The provider will then lookup the ACLs from the <code>AclService</code> and ensure the
- * principal is
+ * principal is {@link org.springframework.security.acls.Acl#isGranted(org.springframework.security.acls.Permission[],
- * {@link org.springframework.security.acls.Acl#isGranted(org.springframework.security.acls.Permission[],
  * org.springframework.security.acls.sid.Sid[], boolean) Acl.isGranted(Permission[], Sid[], boolean)}
- * when presenting the {@link #requirePermission} array to that method.</p>
+ * when presenting the {@link #requirePermission} array to that method.
- *  <p>If the principal does not have permission, that element will not be included in the returned
+ * <p>
- * <code>Collection</code>.</p>
+ * If the principal does not have permission, that element will not be included in the returned
- *  <p>Often users will setup a <code>BasicAclEntryAfterInvocationProvider</code> with a {@link
+ * <code>Collection</code>.
+ * <p>
+ * Often users will setup a <code>BasicAclEntryAfterInvocationProvider</code> with a {@link
  * #processConfigAttribute} of <code>AFTER_ACL_COLLECTION_READ</code> and a {@link #requirePermission} of
- * <code>BasePermission.READ</code>. These are also the defaults.</p>
+ * <code>BasePermission.READ</code>. These are also the defaults.
- *  <p>If the provided <code>returnObject</code> is <code>null</code>, a <code>null</code><code>Collection</code>
+ * <p>
+ * If the provided <code>returnObject</code> is <code>null</code>, a <code>null</code><code>Collection</code>
  * will be returned. If the provided <code>returnObject</code> is not a <code>Collection</code>, an {@link
- * AuthorizationServiceException} will be thrown.</p>
+ * AuthorizationServiceException} will be thrown.
- *  <p>All comparisons and prefixes are case sensitive.</p>
+ * <p>
+ * All comparisons and prefixes are case sensitive.
  *
  * @author Ben Alex
  * @author Paulo Neves
@@ -70,62 +76,58 @@ public class AclEntryAfterInvocationCollectionFilteringProvider extends Abstract
     //~ Methods ========================================================================================================
 
     public Object decide(Authentication authentication, Object object, ConfigAttributeDefinition config,
-        Object returnedObject) throws AccessDeniedException {
+            Object returnedObject) throws AccessDeniedException {
+
+        if (returnedObject == null) {
+            if (logger.isDebugEnabled()) {
+                logger.debug("Return object is null, skipping");
+            }
+
+            return null;
+        }
+
         Iterator iter = config.getConfigAttributes().iterator();
 
         while (iter.hasNext()) {
             ConfigAttribute attr = (ConfigAttribute) iter.next();
 
-            if (this.supports(attr)) {
+            if (!this.supports(attr)) {
-                // Need to process the Collection for this invocation
+                continue;
-                if (returnedObject == null) {
-                    if (logger.isDebugEnabled()) {
-                        logger.debug("Return object is null, skipping");
-                    }
-
-                    return null;
-                }
-
-                Filterer filterer = null;
-
-                if (returnedObject instanceof Collection) {
-                    Collection collection = (Collection) returnedObject;
-                    filterer = new CollectionFilterer(collection);
-                } else if (returnedObject.getClass().isArray()) {
-                    Object[] array = (Object[]) returnedObject;
-                    filterer = new ArrayFilterer(array);
-                } else {
-                    throw new AuthorizationServiceException("A Collection or an array (or null) was required as the "
-                            + "returnedObject, but the returnedObject was: " + returnedObject);
-                }
-
-                // Locate unauthorised Collection elements
-                Iterator collectionIter = filterer.iterator();
-
-                while (collectionIter.hasNext()) {
-                    Object domainObject = collectionIter.next();
-
-                    boolean hasPermission = false;
-
-                    if (domainObject == null) {
-                        hasPermission = true;
-                    } else if (!getProcessDomainObjectClass().isAssignableFrom(domainObject.getClass())) {
-                        hasPermission = true;
-                    } else {
-                        hasPermission = hasPermission(authentication, domainObject);
-
-                        if (!hasPermission) {
-                            filterer.remove(domainObject);
-
-                            if (logger.isDebugEnabled()) {
-                                logger.debug("Principal is NOT authorised for element: " + domainObject);
-                            }
-                        }
-                    }
-                }
-
-                return filterer.getFilteredObject();
             }
+
+            // Need to process the Collection for this invocation
+            Filterer filterer;
+
+            if (returnedObject instanceof Collection) {
+                filterer = new CollectionFilterer((Collection) returnedObject);
+            } else if (returnedObject.getClass().isArray()) {
+                filterer = new ArrayFilterer((Object[]) returnedObject);
+            } else {
+                throw new AuthorizationServiceException("A Collection or an array (or null) was required as the "
+                        + "returnedObject, but the returnedObject was: " + returnedObject);
+            }
+
+            // Locate unauthorised Collection elements
+            Iterator collectionIter = filterer.iterator();
+
+            while (collectionIter.hasNext()) {
+                Object domainObject = collectionIter.next();
+
+                // Ignore nulls or entries which aren't instances of the configured domain object class
+                if (domainObject == null || !getProcessDomainObjectClass().isAssignableFrom(domainObject.getClass())) {
+                    continue;
+                }
+
+                if(!hasPermission(authentication, domainObject)) {
+                    filterer.remove(domainObject);
+
+                    if (logger.isDebugEnabled()) {
+                        logger.debug("Principal is NOT authorised for element: " + domainObject);
+                    }
+                }
+            }
+
+            return filterer.getFilteredObject();
         }
 
         return returnedObject;

core/src/main/java/org/springframework/security/afterinvocation/AclEntryAfterInvocationProvider.java

@@ -34,22 +34,28 @@ import java.util.Iterator;
 
 
 /**
- * <p>Given a domain object instance returned from a secure object invocation, ensures the principal has
+ * Given a domain object instance returned from a secure object invocation, ensures the principal has
- * appropriate permission as defined by the {@link AclService}.</p>
+ * appropriate permission as defined by the {@link AclService}.
- * <p>The <code>AclService</code> is used to retrieve the access control list (ACL) permissions associated with a
+ * <p>
- * domain object instance for the current <code>Authentication</code> object.</p>
+ * The <code>AclService</code> is used to retrieve the access control list (ACL) permissions associated with a
- * <p>This after invocation provider will fire if any  {@link ConfigAttribute#getAttribute()} matches the {@link
+ * domain object instance for the current <code>Authentication</code> object.
- * #processConfigAttribute}. The provider will then lookup the ACLs from the <code>AclService</code> and ensure the
+ * <p>
+ * This after invocation provider will fire if any  {@link ConfigAttribute#getAttribute()} matches the {@link
+ * #processConfigAttribute}. The provider will then lookup the ACLs from the <tt>AclService</tt> and ensure the
  * principal is {@link org.springframework.security.acls.Acl#isGranted(org.springframework.security.acls.Permission[],
    org.springframework.security.acls.sid.Sid[], boolean) Acl.isGranted(Permission[], Sid[], boolean)}
- * when presenting the {@link #requirePermission} array to that method.</p>
+ * when presenting the {@link #requirePermission} array to that method.
- * <p>Often users will setup an <code>AclEntryAfterInvocationProvider</code> with a {@link
+ * <p>
+ * Often users will setup an <code>AclEntryAfterInvocationProvider</code> with a {@link
  * #processConfigAttribute} of <code>AFTER_ACL_READ</code> and a {@link #requirePermission} of
- * <code>BasePermission.READ</code>. These are also the defaults.</p>
+ * <code>BasePermission.READ</code>. These are also the defaults.
- * <p>If the principal does not have sufficient permissions, an <code>AccessDeniedException</code> will be thrown.</p>
+ * <p>
- * <p>If the provided <code>returnObject</code> is <code>null</code>, permission will always be granted and
+ * If the principal does not have sufficient permissions, an <code>AccessDeniedException</code> will be thrown.
- * <code>null</code> will be returned.</p>
+ * <p>
- * <p>All comparisons and prefixes are case sensitive.</p>
+ * If the provided <tt>returnedObject</tt> is <code>null</code>, permission will always be granted and
+ * <code>null</code> will be returned.
+ * <p>
+ * All comparisons and prefixes are case sensitive.
  */
 public class AclEntryAfterInvocationProvider extends AbstractAclProvider implements MessageSourceAware {
     //~ Static fields/initializers =====================================================================================
@@ -69,45 +75,45 @@ public class AclEntryAfterInvocationProvider extends AbstractAclProvider impleme
     //~ Methods ========================================================================================================
 
     public Object decide(Authentication authentication, Object object, ConfigAttributeDefinition config,
-        Object returnedObject) throws AccessDeniedException {
+            Object returnedObject) throws AccessDeniedException {
+
         Iterator iter = config.getConfigAttributes().iterator();
 
+        if (returnedObject == null) {
+            // AclManager interface contract prohibits nulls
+            // As they have permission to null/nothing, grant access
+            if (logger.isDebugEnabled()) {
+                logger.debug("Return object is null, skipping");
+            }
+
+            return null;
+        }
+
+        if (!getProcessDomainObjectClass().isAssignableFrom(returnedObject.getClass())) {
+            if (logger.isDebugEnabled()) {
+                logger.debug("Return object is not applicable for this provider, skipping");
+            }
+
+            return returnedObject;
+        }        
+
         while (iter.hasNext()) {
             ConfigAttribute attr = (ConfigAttribute) iter.next();
 
-            if (this.supports(attr)) {
+            if (!this.supports(attr)) {
-                // Need to make an access decision on this invocation
+                continue;
-                if (returnedObject == null) {
-                    // AclManager interface contract prohibits nulls
-                    // As they have permission to null/nothing, grant access
-                    if (logger.isDebugEnabled()) {
-                        logger.debug("Return object is null, skipping");
-                    }
-
-                    return null;
-                }
-
-                if (!getProcessDomainObjectClass().isAssignableFrom(returnedObject.getClass())) {
-                    if (logger.isDebugEnabled()) {
-                        logger.debug("Return object is not applicable for this provider, skipping");
-                    }
-
-                    return returnedObject;
-                }
-
-                if (hasPermission(authentication, returnedObject)) {
-                    return returnedObject;
-                } else {
-                    if (logger.isDebugEnabled()) {
-                        logger.debug("Denying access");
-                    }
-
-                    throw new AccessDeniedException(messages.getMessage(
-                            "BasicAclEntryAfterInvocationProvider.noPermission",
-                            new Object[] {authentication.getName(), returnedObject},
-                            "Authentication {0} has NO permissions to the domain object {1}"));
-                }
             }
+            // Need to make an access decision on this invocation
+
+            if (hasPermission(authentication, returnedObject)) {
+                return returnedObject;
+            }
+
+            logger.debug("Denying access");
+
+            throw new AccessDeniedException(messages.getMessage("BasicAclEntryAfterInvocationProvider.noPermission",
+                    new Object[] {authentication.getName(), returnedObject},
+                    "Authentication {0} has NO permissions to the domain object {1}"));
         }
 
         return returnedObject;

core/src/main/java/org/springframework/security/afterinvocation/AfterInvocationProvider.java

@@ -22,8 +22,7 @@ import org.springframework.security.ConfigAttributeDefinition;
 
 
 /**
- * Indicates a class is responsible for participating in an {@link
+ * Indicates a class is responsible for participating in an {@link AfterInvocationProviderManager} decision.
- * AfterInvocationProviderManager} decision.
  *
  * @author Ben Alex
  * @version $Id$

core/src/main/java/org/springframework/security/afterinvocation/AfterInvocationProviderManager.java

@@ -25,6 +25,7 @@ import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
 import org.springframework.beans.factory.InitializingBean;
+import org.springframework.util.Assert;
 
 import java.util.Iterator;
 import java.util.List;
@@ -87,16 +88,10 @@ public class AfterInvocationProviderManager implements AfterInvocationManager, I
         Iterator iter = newList.iterator();
 
         while (iter.hasNext()) {
-            Object currentObject = null;
+            Object currentObject = iter.next();
 
-            try {
+            Assert.isInstanceOf(AfterInvocationProvider.class, currentObject, "AfterInvocationProvider " +
-                currentObject = iter.next();
+                    currentObject.getClass().getName() + " must implement AfterInvocationProvider");
-
-                AfterInvocationProvider attemptToCast = (AfterInvocationProvider) currentObject;
-            } catch (ClassCastException cce) {
-                throw new IllegalArgumentException("AfterInvocationProvider " + currentObject.getClass().getName()
-                    + " must implement AfterInvocationProvider");
-            }
         }
 
         this.providers = newList;

core/src/main/java/org/springframework/security/vote/AclEntryVoter.java

@@ -35,29 +35,36 @@ import org.springframework.security.acls.sid.SidRetrievalStrategyImpl;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.springframework.util.Assert;
+import org.springframework.util.StringUtils;
 
 
 /**
- * <p>Given a domain object instance passed as a method argument, ensures the principal has appropriate permission
+ * <p>
- * as indicated by the {@link AclService}.</p>
+ * Given a domain object instance passed as a method argument, ensures the principal has appropriate permission
- *  <p>The <code>AclService</code> is used to retrieve the access control list (ACL) permissions associated with a
+ * as indicated by the {@link AclService}.
- * domain object instance for the current <code>Authentication</code> object.</p>
+ * <p>
- *  <p>The voter will vote if any  {@link ConfigAttribute#getAttribute()} matches the {@link
+ * The <tt>AclService</tt> is used to retrieve the access control list (ACL) permissions associated with a
- * #processConfigAttribute}. The provider will then locate the first method argument of type {@link
+ * domain object instance for the current <tt>Authentication</tt> object.
- * #processDomainObjectClass}. Assuming that method argument is non-null, the provider will then lookup the ACLs from
+ * <p>
- * the <code>AclManager</code> and ensure the principal is  {@link Acl#isGranted(org.springframework.security.acls.Permission[],
+ * The voter will vote if any  {@link ConfigAttribute#getAttribute()} matches the {@link #processConfigAttribute}.
- * org.springframework.security.acls.sid.Sid[], boolean)}  when presenting the {@link #requirePermission} array to that method.</p>
+ * The provider will then locate the first method argument of type {@link #processDomainObjectClass}. Assuming that
- *  <p>If the method argument is <code>null</code>, the voter will abstain from voting. If the method argument
+ * method argument is non-null, the provider will then lookup the ACLs from the <code>AclManager</code> and ensure the
- * could not be found, an {@link org.springframework.security.AuthorizationServiceException} will be thrown.</p>
+ * principal is {@link Acl#isGranted(org.springframework.security.acls.Permission[],
- *  <p>In practical terms users will typically setup a number of <code>AclEntryVoter</code>s. Each will have a
+ * org.springframework.security.acls.sid.Sid[], boolean)} when presenting the {@link #requirePermission} array to that
+ * method.
+ * <p>
+ * If the method argument is <tt>null</tt>, the voter will abstain from voting. If the method argument
+ * could not be found, an {@link org.springframework.security.AuthorizationServiceException} will be thrown.
+ * <p>
+ * In practical terms users will typically setup a number of <tt>AclEntryVoter</tt>s. Each will have a
  * different {@link #processDomainObjectClass}, {@link #processConfigAttribute} and {@link #requirePermission}
- * combination. For example, a small application might employ the following instances of <code>AclEntryVoter</code>:
+ * combination. For example, a small application might employ the following instances of <tt>AclEntryVoter</tt>:
  *  <ul>
  *      <li>Process domain object class <code>BankAccount</code>, configuration attribute
  *      <code>VOTE_ACL_BANK_ACCONT_READ</code>, require permission <code>BasePermission.READ</code></li>
  *      <li>Process domain object class <code>BankAccount</code>, configuration attribute
  *      <code>VOTE_ACL_BANK_ACCOUNT_WRITE</code>, require permission list <code>BasePermission.WRITE</code> and
- *      <code>BasePermission.CREATE</code> (allowing the principal to have <b>either</b> of these two permissions</li>
+ *      <code>BasePermission.CREATE</code> (allowing the principal to have <b>either</b> of these two permissions)</li>
  *      <li>Process domain object class <code>Customer</code>, configuration attribute
  *      <code>VOTE_ACL_CUSTOMER_READ</code>, require permission <code>BasePermission.READ</code></li>
  *      <li>Process domain object class <code>Customer</code>, configuration attribute
@@ -113,18 +120,18 @@ public class AclEntryVoter extends AbstractAclVoter {
      *         should be invoked to obtain an <code>Object</code> which will be the domain object used for ACL
      *         evaluation
      */
-    public String getInternalMethod() {
+    protected String getInternalMethod() {
         return internalMethod;
     }
 
-    public String getProcessConfigAttribute() {
-        return processConfigAttribute;
-    }
-
     public void setInternalMethod(String internalMethod) {
         this.internalMethod = internalMethod;
     }
 
+    protected String getProcessConfigAttribute() {
+        return processConfigAttribute;
+    }
+
     public void setObjectIdentityRetrievalStrategy(ObjectIdentityRetrievalStrategy objectIdentityRetrievalStrategy) {
         Assert.notNull(objectIdentityRetrievalStrategy, "ObjectIdentityRetrievalStrategy required");
         this.objectIdentityRetrievalStrategy = objectIdentityRetrievalStrategy;
@@ -149,95 +156,84 @@ public class AclEntryVoter extends AbstractAclVoter {
         while (iter.hasNext()) {
             ConfigAttribute attr = (ConfigAttribute) iter.next();
 
-            if (this.supports(attr)) {
+            if (!this.supports(attr)) {
-                // Need to make an access decision on this invocation
+                continue;
-                // Attempt to locate the domain object instance to process
+            }
-                Object domainObject = getDomainObjectInstance(object);
+            // Need to make an access decision on this invocation
+            // Attempt to locate the domain object instance to process
+            Object domainObject = getDomainObjectInstance(object);
 
-                // Evaluate if we are required to use an inner domain object
+            // If domain object is null, vote to abstain
-                if (domainObject != null && internalMethod != null && (!"".equals(internalMethod))) {
+            if (domainObject == null) {
-                    try {
+                if (logger.isDebugEnabled()) {
-                        Class clazz = domainObject.getClass();
+                    logger.debug("Voting to abstain - domainObject is null");
-                        Method method = clazz.getMethod(internalMethod, new Class[] {});
-                        domainObject = method.invoke(domainObject, new Object[] {});
-                    } catch (NoSuchMethodException nsme) {
-                        throw new AuthorizationServiceException("Object of class '" + domainObject.getClass()
-                            + "' does not provide the requested internalMethod: " + internalMethod);
-                    } catch (IllegalAccessException iae) {
-                        if (logger.isDebugEnabled()) {
-                            logger.debug("IllegalAccessException", iae);
-
-                            if (iae.getCause() != null) {
-                                logger.debug("Cause: " + iae.getCause().getMessage(), iae.getCause());
-                            }
-                        }
-
-                        throw new AuthorizationServiceException("Problem invoking internalMethod: " + internalMethod
-                            + " for object: " + domainObject);
-                    } catch (InvocationTargetException ite) {
-                        if (logger.isDebugEnabled()) {
-                            logger.debug("InvocationTargetException", ite);
-
-                            if (ite.getCause() != null) {
-                                logger.debug("Cause: " + ite.getCause().getMessage(), ite.getCause());
-                            }
-                        }
-
-                        throw new AuthorizationServiceException("Problem invoking internalMethod: " + internalMethod
-                            + " for object: " + domainObject);
-                    }
                 }
 
-                // If domain object is null, vote to abstain
+                return AccessDecisionVoter.ACCESS_ABSTAIN;
-                if (domainObject == null) {
+            }
-                    if (logger.isDebugEnabled()) {
-                        logger.debug("Voting to abstain - domainObject is null");
-                    }
-
-                    return AccessDecisionVoter.ACCESS_ABSTAIN;
-                }
-
-                // Obtain the OID applicable to the domain object
-                ObjectIdentity objectIdentity = objectIdentityRetrievalStrategy.getObjectIdentity(domainObject);
-
-                // Obtain the SIDs applicable to the principal
-                Sid[] sids = sidRetrievalStrategy.getSids(authentication);
-
-                Acl acl;
 
+            // Evaluate if we are required to use an inner domain object
+            if (StringUtils.hasText(internalMethod)) {
                 try {
-                    // Lookup only ACLs for SIDs we're interested in
+                    Class clazz = domainObject.getClass();
-                    acl = aclService.readAclById(objectIdentity, sids);
+                    Method method = clazz.getMethod(internalMethod, new Class[0]);
-                } catch (NotFoundException nfe) {
+                    domainObject = method.invoke(domainObject, new Object[0]);
+                } catch (NoSuchMethodException nsme) {
+                    throw new AuthorizationServiceException("Object of class '" + domainObject.getClass()
+                        + "' does not provide the requested internalMethod: " + internalMethod);
+                } catch (IllegalAccessException iae) {
+                    logger.debug("IllegalAccessException", iae);
+
+                    throw new AuthorizationServiceException("Problem invoking internalMethod: " + internalMethod
+                        + " for object: " + domainObject);
+                } catch (InvocationTargetException ite) {
+                    logger.debug("InvocationTargetException", ite);
+
+                    throw new AuthorizationServiceException("Problem invoking internalMethod: " + internalMethod
+                        + " for object: " + domainObject);
+                }
+            }
+
+            // Obtain the OID applicable to the domain object
+            ObjectIdentity objectIdentity = objectIdentityRetrievalStrategy.getObjectIdentity(domainObject);
+
+            // Obtain the SIDs applicable to the principal
+            Sid[] sids = sidRetrievalStrategy.getSids(authentication);
+
+            Acl acl;
+
+            try {
+                // Lookup only ACLs for SIDs we're interested in
+                acl = aclService.readAclById(objectIdentity, sids);
+            } catch (NotFoundException nfe) {
+                if (logger.isDebugEnabled()) {
+                    logger.debug("Voting to deny access - no ACLs apply for this principal");
+                }
+
+                return AccessDecisionVoter.ACCESS_DENIED;
+            }
+
+            try {
+                if (acl.isGranted(requirePermission, sids, false)) {
                     if (logger.isDebugEnabled()) {
-                        logger.debug("Voting to deny access - no ACLs apply for this principal");
+                        logger.debug("Voting to grant access");
+                    }
+
+                    return AccessDecisionVoter.ACCESS_GRANTED;
+                } else {
+                    if (logger.isDebugEnabled()) {
+                        logger.debug(
+                            "Voting to deny access - ACLs returned, but insufficient permissions for this principal");
                     }
 
                     return AccessDecisionVoter.ACCESS_DENIED;
                 }
-
+            } catch (NotFoundException nfe) {
-                try {
+                if (logger.isDebugEnabled()) {
-                    if (acl.isGranted(requirePermission, sids, false)) {
+                    logger.debug("Voting to deny access - no ACLs apply for this principal");
-                        if (logger.isDebugEnabled()) {
-                            logger.debug("Voting to grant access");
-                        }
-
-                        return AccessDecisionVoter.ACCESS_GRANTED;
-                    } else {
-                        if (logger.isDebugEnabled()) {
-                            logger.debug(
-                                "Voting to deny access - ACLs returned, but insufficient permissions for this principal");
-                        }
-
-                        return AccessDecisionVoter.ACCESS_DENIED;
-                    }
-                } catch (NotFoundException nfe) {
-                    if (logger.isDebugEnabled()) {
-                        logger.debug("Voting to deny access - no ACLs apply for this principal");
-                    }
-
-                    return AccessDecisionVoter.ACCESS_DENIED;
                 }
+
+                return AccessDecisionVoter.ACCESS_DENIED;
             }
         }
 

core/src/test/java/org/springframework/security/acls/sid/SidRetrievalStrategyTests.java

@@ -23,14 +23,13 @@ public class SidRetrievalStrategyTests extends TestCase {
         SidRetrievalStrategy retrStrategy = new SidRetrievalStrategyImpl();
         Sid[] sids = retrStrategy.getSids(authentication);
 
-        Assert.assertNotNull(sids);
+        assertNotNull(sids);
-        Assert.assertEquals(4, sids.length);
+        assertEquals(4, sids.length);
-        Assert.assertNotNull(sids[0]);
+        assertNotNull(sids[0]);
+        assertTrue(sids[0] instanceof PrincipalSid);
 
-        Assert.assertTrue(PrincipalSid.class.isAssignableFrom(sids[0].getClass()));
         for (int i = 1; i < sids.length; i++) {
-            Sid sid = sids[i];
+            assertTrue(sids[i] instanceof GrantedAuthoritySid);
-            Assert.assertTrue(GrantedAuthoritySid.class.isAssignableFrom(sid.getClass()));
         }
 
         Assert.assertEquals("scott", ((PrincipalSid) sids[0]).getPrincipal());