SEC-1022: Remove use of static methods/initializers in Acl Permissions. Converted PermissionFactory to a strategy which is used to convert integers and names to Permission instances.

acl/src/main/java/org/springframework/security/acls/AclPermissionEvaluator.java

@@ -7,8 +7,9 @@ import java.util.List;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.springframework.security.access.PermissionEvaluator;
-import org.springframework.security.acls.domain.BasePermission;
+import org.springframework.security.acls.domain.DefaultPermissionFactory;
 import org.springframework.security.acls.domain.ObjectIdentityRetrievalStrategyImpl;
+import org.springframework.security.acls.domain.PermissionFactory;
 import org.springframework.security.acls.domain.SidRetrievalStrategyImpl;
 import org.springframework.security.acls.model.Acl;
 import org.springframework.security.acls.model.AclService;
@@ -38,6 +39,7 @@ public class AclPermissionEvaluator implements PermissionEvaluator {
     private ObjectIdentityRetrievalStrategy objectIdentityRetrievalStrategy = new ObjectIdentityRetrievalStrategyImpl();
     private ObjectIdentityGenerator objectIdentityGenerator = new ObjectIdentityRetrievalStrategyImpl();
     private SidRetrievalStrategy sidRetrievalStrategy = new SidRetrievalStrategyImpl();
+    private PermissionFactory permissionFactory = new DefaultPermissionFactory();
 
     public AclPermissionEvaluator(AclService aclService) {
         this.aclService = aclService;
@@ -95,10 +97,9 @@ public class AclPermissionEvaluator implements PermissionEvaluator {
 
     }
 
-    // TODO: Add permission resolver/PermissionFactory rewrite
     List<Permission> resolvePermission(Object permission) {
         if (permission instanceof Integer) {
-            return Arrays.asList(BasePermission.buildFromMask(((Integer)permission).intValue()));
+            return Arrays.asList(permissionFactory.buildFromMask(((Integer)permission).intValue()));
         }
 
         if (permission instanceof Permission) {
@@ -114,9 +115,9 @@ public class AclPermissionEvaluator implements PermissionEvaluator {
             Permission p = null;
 
             try {
-                p = BasePermission.buildFromName(permString);
+                p = permissionFactory.buildFromName(permString);
             } catch(IllegalArgumentException notfound) {
-                p = BasePermission.buildFromName(permString.toUpperCase());
+                p = permissionFactory.buildFromName(permString.toUpperCase());
             }
 
             if (p != null) {

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

@@ -37,14 +37,6 @@ public class BasePermission extends AbstractPermission {
 
     protected static DefaultPermissionFactory defaultPermissionFactory = new DefaultPermissionFactory();
 
-    /**
-     * Registers the public static permissions defined on this class. This is mandatory so
-     * that the static methods will operate correctly.
-     */
-    static {
-        registerPermissionsFor(BasePermission.class);
-    }
-    
     protected BasePermission(int mask) {
        super(mask);
     }
@@ -53,24 +45,20 @@ public class BasePermission extends AbstractPermission {
         super(mask, code);
     }
 
-    protected final static void registerPermissionsFor(Class<? extends Permission> subClass) {
+//    public final static Permission buildFromMask(int mask) {
-        defaultPermissionFactory.registerPublicPermissions(subClass);
+//        return defaultPermissionFactory.buildFromMask(mask);
-    }
+//    }
-
+//
-    public final static Permission buildFromMask(int mask) {
+//    public final static Permission[] buildFromMask(int[] masks) {
-        return defaultPermissionFactory.buildFromMask(mask);
+//        return defaultPermissionFactory.buildFromMask(masks);
-    }
+//    }
-
+//
-    public final static Permission[] buildFromMask(int[] masks) {
+//    public final static Permission buildFromName(String name) {
-        return defaultPermissionFactory.buildFromMask(masks);
+//        return defaultPermissionFactory.buildFromName(name);
-    }
+//    }
-
+//
-    public final static Permission buildFromName(String name) {
+//    public final static Permission[] buildFromName(String[] names) {
-        return defaultPermissionFactory.buildFromName(name);
+//        return defaultPermissionFactory.buildFromName(names);
-    }
+//    }
-
-    public final static Permission[] buildFromName(String[] names) {
-        return defaultPermissionFactory.buildFromName(names);
-    }
 
 }

acl/src/main/java/org/springframework/security/acls/domain/DefaultPermissionFactory.java

@@ -1,7 +1,10 @@
 package org.springframework.security.acls.domain;
 
 import java.lang.reflect.Field;
+import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 
 import org.springframework.security.acls.jdbc.LookupStrategy;
@@ -23,6 +26,10 @@ public class DefaultPermissionFactory implements PermissionFactory {
     private final Map<Integer, Permission> registeredPermissionsByInteger = new HashMap<Integer, Permission>();
     private final Map<String, Permission> registeredPermissionsByName = new HashMap<String, Permission>();
 
+    public DefaultPermissionFactory() {
+        registerPublicPermissions(BasePermission.class);
+    }
+
     /**
      * Permit registration of a {@link DefaultPermissionFactory} class. The class must provide
      * public static fields of type {@link Permission} to represent the possible permissions.
@@ -106,15 +113,15 @@ public class DefaultPermissionFactory implements PermissionFactory {
         return (Permission) registeredPermissionsByName.get(name);
     }
 
-    public Permission[] buildFromName(String[] names) {
+    public List<Permission> buildFromNames(List<String> names) {
-        if ((names == null) || (names.length == 0)) {
+        if ((names == null) || (names.size() == 0)) {
-            return new Permission[0];
+            return Collections.emptyList();
         }
 
-        Permission[] permissions = new Permission[names.length];
+        List<Permission> permissions = new ArrayList<Permission>(names.size());
 
-        for (int i = 0; i < names.length; i++) {
+        for (String name : names) {
-            permissions[i] = buildFromName(names[i]);
+            permissions.add(buildFromName(name));
         }
 
         return permissions;

acl/src/main/java/org/springframework/security/acls/domain/PermissionFactory.java

@@ -1,5 +1,7 @@
 package org.springframework.security.acls.domain;
 
+import java.util.List;
+
 import org.springframework.security.acls.model.Permission;
 
 /**
@@ -19,6 +21,11 @@ public interface PermissionFactory {
      *
      * @return a Permission representing the requested object
      */
-    public abstract Permission buildFromMask(int mask);
+    Permission buildFromMask(int mask);
 
+
+    Permission buildFromName(String name);
+
+
+    List<Permission> buildFromNames(List<String> names);
 }

acl/src/main/java/org/springframework/security/acls/jdbc/BasicLookupStrategy.java

@@ -37,9 +37,10 @@ import org.springframework.security.acls.domain.AccessControlEntryImpl;
 import org.springframework.security.acls.domain.AclAuthorizationStrategy;
 import org.springframework.security.acls.domain.AclImpl;
 import org.springframework.security.acls.domain.AuditLogger;
-import org.springframework.security.acls.domain.BasePermission;
+import org.springframework.security.acls.domain.DefaultPermissionFactory;
 import org.springframework.security.acls.domain.GrantedAuthoritySid;
 import org.springframework.security.acls.domain.ObjectIdentityImpl;
+import org.springframework.security.acls.domain.PermissionFactory;
 import org.springframework.security.acls.domain.PrincipalSid;
 import org.springframework.security.acls.model.AccessControlEntry;
 import org.springframework.security.acls.model.Acl;
@@ -71,6 +72,7 @@ public final class BasicLookupStrategy implements LookupStrategy {
     //~ Instance fields ================================================================================================
 
     private AclAuthorizationStrategy aclAuthorizationStrategy;
+    private PermissionFactory permissionFactory = new DefaultPermissionFactory();
     private AclCache aclCache;
     private AuditLogger auditLogger;
     private JdbcTemplate jdbcTemplate;
@@ -225,11 +227,6 @@ public final class BasicLookupStrategy implements LookupStrategy {
         }
     }
 
-
-    protected Permission convertMaskIntoPermission(int mask) {
-        return BasePermission.buildFromMask(mask);
-    }
-
     /**
      * Locates the primary key IDs specified in "findNow", adding AclImpl instances with StubAclParents to the
      * "acls" Map.
@@ -519,7 +516,7 @@ public final class BasicLookupStrategy implements LookupStrategy {
                 }
 
                 int mask = rs.getInt("mask");
-                Permission permission = convertMaskIntoPermission(mask);
+                Permission permission = permissionFactory.buildFromMask(mask);
                 boolean granting = rs.getBoolean("granting");
                 boolean auditSuccess = rs.getBoolean("audit_success");
                 boolean auditFailure = rs.getBoolean("audit_failure");

acl/src/test/java/org/springframework/security/acls/domain/PermissionTests.java

@@ -16,6 +16,7 @@ package org.springframework.security.acls.domain;
 
 import static org.junit.Assert.*;
 
+import org.junit.Before;
 import org.junit.Test;
 import org.springframework.security.acls.model.Permission;
 
@@ -24,13 +25,20 @@ import org.springframework.security.acls.model.Permission;
  * Tests classes associated with Permission.
  *
  * @author Ben Alex
- * @version $Id${date}
+ * @version $Id$
  */
 public class PermissionTests {
 
+    private DefaultPermissionFactory permissionFactory;
+
+    @Before
+    public void createPermissionfactory() {
+        permissionFactory = new DefaultPermissionFactory();
+    }
+
     @Test
     public void basePermissionTest() {
-        Permission p = BasePermission.buildFromName("WRITE");
+        Permission p = permissionFactory.buildFromName("WRITE");
         assertNotNull(p);
     }
 
@@ -47,14 +55,16 @@ public class PermissionTests {
 
     @Test
     public void fromInteger() {
-        Permission permission = BasePermission.buildFromMask(7);
+        Permission permission = permissionFactory.buildFromMask(7);
         System.out.println("7 =  " + permission.toString());
-        permission = BasePermission.buildFromMask(4);
+        permission = permissionFactory.buildFromMask(4);
         System.out.println("4 =  " + permission.toString());
     }
 
     @Test
     public void stringConversion() {
+        permissionFactory.registerPublicPermissions(SpecialPermission.class);
+
         System.out.println("R =  " + BasePermission.READ.toString());
         assertEquals("BasePermission[...............................R=1]", BasePermission.READ.toString());
 

acl/src/test/java/org/springframework/security/acls/domain/SpecialPermission.java

@@ -26,14 +26,6 @@ import org.springframework.security.acls.model.Permission;
 public class SpecialPermission extends BasePermission {
     public static final Permission ENTER = new SpecialPermission(1 << 5, 'E'); // 32
 
-    /**
-     * Registers the public static permissions defined on this class. This is mandatory so
-     * that the static methods will operate correctly.
-     */
-    static {
-        registerPermissionsFor(SpecialPermission.class);
-    }
-
     protected SpecialPermission(int mask, char code) {
         super(mask, code);
     }

samples/contacts/src/main/java/sample/contact/AdminPermissionController.java

@@ -10,7 +10,9 @@ import org.springframework.context.MessageSourceAware;
 import org.springframework.context.support.MessageSourceAccessor;
 import org.springframework.dao.DataAccessException;
 import org.springframework.security.acls.domain.BasePermission;
+import org.springframework.security.acls.domain.DefaultPermissionFactory;
 import org.springframework.security.acls.domain.ObjectIdentityImpl;
+import org.springframework.security.acls.domain.PermissionFactory;
 import org.springframework.security.acls.domain.PrincipalSid;
 import org.springframework.security.acls.model.Acl;
 import org.springframework.security.acls.model.AclService;
@@ -45,6 +47,7 @@ public final class AdminPermissionController implements MessageSourceAware{
     private ContactManager contactManager;
     private MessageSourceAccessor messages;
     private Validator addPermissionValidator = new AddPermissionValidator();
+    private PermissionFactory permissionFactory = new DefaultPermissionFactory();
 
     /**
      * Displays the permission admin page for a particular contact.
@@ -99,7 +102,7 @@ public final class AdminPermissionController implements MessageSourceAware{
         }
 
         PrincipalSid sid = new PrincipalSid(addPermission.getRecipient());
-        Permission permission = BasePermission.buildFromMask(addPermission.getPermission().intValue());
+        Permission permission = permissionFactory.buildFromMask(addPermission.getPermission());
 
         try {
             contactManager.addPermission(addPermission.getContact(), sid, permission);
@@ -127,7 +130,7 @@ public final class AdminPermissionController implements MessageSourceAware{
         Contact contact = contactManager.getById(new Long(contactId));
 
         Sid sidObject = new PrincipalSid(sid);
-        Permission permission = BasePermission.buildFromMask(mask);
+        Permission permission = permissionFactory.buildFromMask(mask);
 
         contactManager.deletePermission(contact, sidObject, permission);
 

taglibs/src/main/java/org/springframework/security/taglibs/authz/AccessControlListTag.java

@@ -14,8 +14,25 @@
  */
 package org.springframework.security.taglibs.authz;
 
-import org.springframework.security.acls.domain.BasePermission;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.StringTokenizer;
+
+import javax.servlet.ServletContext;
+import javax.servlet.jsp.JspException;
+import javax.servlet.jsp.PageContext;
+import javax.servlet.jsp.tagext.Tag;
+import javax.servlet.jsp.tagext.TagSupport;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.springframework.context.ApplicationContext;
+import org.springframework.security.acls.domain.DefaultPermissionFactory;
 import org.springframework.security.acls.domain.ObjectIdentityRetrievalStrategyImpl;
+import org.springframework.security.acls.domain.PermissionFactory;
 import org.springframework.security.acls.domain.SidRetrievalStrategyImpl;
 import org.springframework.security.acls.model.Acl;
 import org.springframework.security.acls.model.AclService;
@@ -25,31 +42,10 @@ import org.springframework.security.acls.model.ObjectIdentityRetrievalStrategy;
 import org.springframework.security.acls.model.Permission;
 import org.springframework.security.acls.model.Sid;
 import org.springframework.security.acls.model.SidRetrievalStrategy;
-
 import org.springframework.security.core.context.SecurityContextHolder;
-
-import org.apache.commons.logging.Log;
-import org.apache.commons.logging.LogFactory;
-
-import org.springframework.context.ApplicationContext;
-
 import org.springframework.web.context.support.WebApplicationContextUtils;
 import org.springframework.web.util.ExpressionEvaluationUtils;
 
-import java.util.ArrayList;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
-import java.util.StringTokenizer;
-import java.util.HashMap;
-
-import javax.servlet.ServletContext;
-import javax.servlet.jsp.JspException;
-import javax.servlet.jsp.PageContext;
-import javax.servlet.jsp.tagext.Tag;
-import javax.servlet.jsp.tagext.TagSupport;
-
 
 /**
  * An implementation of {@link Tag} that allows its body through if some authorizations are granted to the request's
@@ -81,17 +77,18 @@ public class AccessControlListTag extends TagSupport {
     private Object domainObject;
     private ObjectIdentityRetrievalStrategy objectIdentityRetrievalStrategy;
     private SidRetrievalStrategy sidRetrievalStrategy;
+    private PermissionFactory permissionFactory;
     private String hasPermission = "";
 
     //~ Methods ========================================================================================================
 
     public int doStartTag() throws JspException {
-        initializeIfRequired();
-
         if ((null == hasPermission) || "".equals(hasPermission)) {
             return Tag.SKIP_BODY;
         }
 
+        initializeIfRequired();
+
         final String evaledPermissionsString = ExpressionEvaluationUtils.evaluateString("hasPermission", hasPermission,
                 pageContext);
 
@@ -169,7 +166,6 @@ public class AccessControlListTag extends TagSupport {
         return hasPermission;
     }
 
-    @SuppressWarnings("unchecked")
     private void initializeIfRequired() throws JspException {
         if (applicationContext != null) {
             return;
@@ -177,42 +173,43 @@ public class AccessControlListTag extends TagSupport {
 
         this.applicationContext = getContext(pageContext);
 
-        Map map = new HashMap();
+        aclService = getBeanOfType(AclService.class);
-        ApplicationContext context = applicationContext;
 
-        while (context != null) {
+        sidRetrievalStrategy = getBeanOfType(SidRetrievalStrategy.class);
-            map.putAll(context.getBeansOfType(AclService.class));
-            context = context.getParent();
-        }
 
-        if (map.size() != 1) {
+        if (sidRetrievalStrategy == null) {
-            throw new JspException(
-                "Found incorrect number of AclService instances in application context - you must have only have one!");
-        }
-
-        aclService = (AclService) map.values().iterator().next();
-
-        map = applicationContext.getBeansOfType(SidRetrievalStrategy.class);
-
-        if (map.size() == 0) {
             sidRetrievalStrategy = new SidRetrievalStrategyImpl();
-        } else if (map.size() == 1) {
-            sidRetrievalStrategy = (SidRetrievalStrategy) map.values().iterator().next();
-        } else {
-            throw new JspException("Found incorrect number of SidRetrievalStrategy instances in application "
-                    + "context - you must have only have one!");
         }
 
-        map = applicationContext.getBeansOfType(ObjectIdentityRetrievalStrategy.class);
+        objectIdentityRetrievalStrategy = getBeanOfType(ObjectIdentityRetrievalStrategy.class);
+
+        if (objectIdentityRetrievalStrategy == null) {
+            objectIdentityRetrievalStrategy = new ObjectIdentityRetrievalStrategyImpl();
+        }
+
+        permissionFactory = getBeanOfType(PermissionFactory.class);
+
+        if (permissionFactory == null) {
+            permissionFactory = new DefaultPermissionFactory();
+        }
+    }
+
+    private <T> T getBeanOfType(Class<T> type) throws JspException {
+        Map<String, T> map = applicationContext.getBeansOfType(type);
+
+        for (ApplicationContext context = applicationContext.getParent();
+            context != null; context = context.getParent()) {
+            map.putAll(context.getBeansOfType(type));
+        }
 
         if (map.size() == 0) {
-            objectIdentityRetrievalStrategy = new ObjectIdentityRetrievalStrategyImpl();
+            return null;
         } else if (map.size() == 1) {
-            objectIdentityRetrievalStrategy = (ObjectIdentityRetrievalStrategy) map.values().iterator().next();
+            return map.values().iterator().next();
-        } else {
-            throw new JspException("Found incorrect number of ObjectIdentityRetrievalStrategy instances in "
-                    + "application context - you must have only have one!");
         }
+
+        throw new JspException("Found incorrect number of " + type.getSimpleName() +" instances in "
+                    + "application context - you must have only have one!");
     }
 
     private List<Permission> parsePermissionsString(String integersString)
@@ -223,7 +220,7 @@ public class AccessControlListTag extends TagSupport {
 
         while (tokenizer.hasMoreTokens()) {
             String integer = tokenizer.nextToken();
-            permissions.add(BasePermission.buildFromMask(new Integer(integer).intValue()));
+            permissions.add(permissionFactory.buildFromMask(new Integer(integer)));
         }
 
         return new ArrayList<Permission>(permissions);