SEC-590: Correct misuse of "continue" keyword so ACLs retrieved even when last element is already cached.

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

@@ -385,48 +385,53 @@ public final class BasicLookupStrategy implements LookupStrategy {
         Set currentBatchToLoad = new HashSet(); // contains ObjectIdentitys
 
         for (int i = 0; i < objects.length; i++) {
-            // Check we don't already have this ACL in the results
+        	boolean aclFound = false;
+
+        	// Check we don't already have this ACL in the results
             if (result.containsKey(objects[i])) {
-                continue; // already in results, so move to next element
+                aclFound = true;
             }
 
             // Check cache for the present ACL entry
-            Acl acl = aclCache.getFromCache(objects[i]);
-
-            // Ensure any cached element supports all the requested SIDs
-            // (they should always, as our base impl doesn't filter on SID)
-            if (acl != null) {
-                if (acl.isSidLoaded(sids)) {
-                    result.put(acl.getObjectIdentity(), acl);
-
-                    continue; // now in results, so move to next element
-                } else {
-                    throw new IllegalStateException(
-                        "Error: SID-filtered element detected when implementation does not perform SID filtering "
-                                + "- have you added something to the cache manually?");
+            if (!aclFound) {
+            	Acl acl = aclCache.getFromCache(objects[i]);
+            	
+                // Ensure any cached element supports all the requested SIDs
+                // (they should always, as our base impl doesn't filter on SID)
+                if (acl != null) {
+                    if (acl.isSidLoaded(sids)) {
+                        result.put(acl.getObjectIdentity(), acl);
+                        aclFound = true;
+                    } else {
+                        throw new IllegalStateException(
+                            "Error: SID-filtered element detected when implementation does not perform SID filtering "
+                                    + "- have you added something to the cache manually?");
+                    }
                 }
             }
-
-            // To get this far, we have no choice but to retrieve it via JDBC
-            // (although we don't do it until we get a batch of them to load)
-            currentBatchToLoad.add(objects[i]);
+            
+            // Load the ACL from the database
+            if (!aclFound) {
+                currentBatchToLoad.add(objects[i]);
+            }
 
             // Is it time to load from JDBC the currentBatchToLoad?
             if ((currentBatchToLoad.size() == this.batchSize) || ((i + 1) == objects.length)) {
-                Map loadedBatch = lookupObjectIdentities((ObjectIdentity[]) currentBatchToLoad.toArray(
-                            new ObjectIdentity[] {}), sids);
-
-                // Add loaded batch (all elements 100% initialized) to results
-                result.putAll(loadedBatch);
-
-                // Add the loaded batch to the cache
-                Iterator loadedAclIterator = loadedBatch.values().iterator();
-
-                while (loadedAclIterator.hasNext()) {
-                    aclCache.putInCache((AclImpl) loadedAclIterator.next());
-                }
-
-                currentBatchToLoad.clear();
+            	if (currentBatchToLoad.size() > 0) {
+            		Map loadedBatch = lookupObjectIdentities((ObjectIdentity[]) currentBatchToLoad.toArray(new ObjectIdentity[] {}), sids);
+
+	                // Add loaded batch (all elements 100% initialized) to results
+	                result.putAll(loadedBatch);
+	
+	                // Add the loaded batch to the cache
+	                Iterator loadedAclIterator = loadedBatch.values().iterator();
+	
+	                while (loadedAclIterator.hasNext()) {
+	                    aclCache.putInCache((AclImpl) loadedAclIterator.next());
+	                }
+	
+	                currentBatchToLoad.clear();
+            	}
             }
         }
 

acl/src/test/java/org/springframework/security/acls/jdbc/BasicLookupStrategyTests.java

@@ -3,9 +3,9 @@ package org.springframework.security.acls.jdbc;
 import java.util.Map;
 
 import junit.framework.Assert;
-import net.sf.ehcache.Ehcache;
-import net.sf.ehcache.CacheManager;
 import net.sf.ehcache.Cache;
+import net.sf.ehcache.CacheManager;
+import net.sf.ehcache.Ehcache;
 
 import org.junit.After;
 import org.junit.AfterClass;
@@ -21,6 +21,8 @@ import org.springframework.security.TestDataSource;
 import org.springframework.security.acls.Acl;
 import org.springframework.security.acls.AuditableAccessControlEntry;
 import org.springframework.security.acls.MutableAcl;
+import org.springframework.security.acls.NotFoundException;
+import org.springframework.security.acls.Permission;
 import org.springframework.security.acls.domain.AclAuthorizationStrategy;
 import org.springframework.security.acls.domain.AclAuthorizationStrategyImpl;
 import org.springframework.security.acls.domain.BasePermission;
@@ -247,7 +249,7 @@ public class BasicLookupStrategyTests {
     /**
      * Test created from SEC-590.
      */
-/*    @Test
+    @Test
     public void testReadAllObjectIdentitiesWhenLastElementIsAlreadyCached() throws Exception {
         String query = "INSERT INTO acl_object_identity(ID,OBJECT_ID_CLASS,OBJECT_ID_IDENTITY,PARENT_OBJECT,OWNER_SID,ENTRIES_INHERITING) VALUES (4,2,104,null,1,1);"
                 + "INSERT INTO acl_object_identity(ID,OBJECT_ID_CLASS,OBJECT_ID_IDENTITY,PARENT_OBJECT,OWNER_SID,ENTRIES_INHERITING) VALUES (5,2,105,4,1,1);"
@@ -286,7 +288,7 @@ public class BasicLookupStrategyTests {
         Acl foundParent2Acl = (Acl) foundAcls.get(parent2Oid);
         Assert.assertNotNull(foundParent2Acl);
         Assert.assertTrue(foundParent2Acl.isGranted(checkPermission, sids, false));
-    }*/
+    }
     
     @Test
     public void testAclsWithDifferentSerializableTypesAsObjectIdentities() throws Exception {