SEC-527: Correct serialization issues with EH-CACHE.

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

@@ -31,7 +31,7 @@ import java.io.Serializable;
  * @version $Id$
  *
  */
-public interface AccessControlEntry {
+public interface AccessControlEntry extends Serializable {
     //~ Methods ========================================================================================================
 
     Acl getAcl();

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

@@ -14,13 +14,15 @@
  */
 package org.springframework.security.acls;
 
+import java.io.Serializable;
+
 /**
  * Represents a permission granted to a {@link org.springframework.security.acls.sid.Sid Sid} for a given domain object.
  *
  * @author Ben Alex
  * @version $Id$
  */
-public interface Permission {
+public interface Permission extends Serializable {
     //~ Static fields/initializers =====================================================================================
 
     char RESERVED_ON = '~';

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

@@ -67,12 +67,50 @@ public class AccessControlEntryImpl implements AccessControlEntry, AuditableAcce
 
         AccessControlEntryImpl rhs = (AccessControlEntryImpl) arg0;
 
-        if (this.acl == null && rhs.getAcl() != null) {
+        if (this.acl == null) {
-        	return false;
+        	if (rhs.getAcl() != null) {
+        		return false;
+        	}
+        	// Both this.acl and rhs.acl are null and thus equal
+        } else {
+        	// this.acl is non-null
+        	if (rhs.getAcl() == null) {
+        		return false;
+        	}
+        	
+        	// Both this.acl and rhs.acl are non-null, so do a comparison
+        	if (this.acl.getObjectIdentity() == null) {
+        		if (rhs.acl.getObjectIdentity() != null) {
+        			return false;
+        		}
+        		// Both this.acl and rhs.acl are null and thus equal
+        	} else {
+        		// Both this.acl.objectIdentity and rhs.acl.objectIdentity are non-null
+            	if (!this.acl.getObjectIdentity().equals(rhs.getAcl().getObjectIdentity())) {
+            		return false;
+            	}
+        	}
+        }
+        
+        if (this.id == null) {
+        	if (rhs.id != null) {
+        		return false;
+        	}
+        	// Both this.id and rhs.id are null and thus equal
+        } else {
+        	// this.id is non-null
+        	if (rhs.id == null) {
+        		return false;
+        	}
+
+        	// Both this.id and rhs.id are non-null
+        	if (!this.id.equals(rhs.id)) {
+        		return false;
+        	}
         }
         
         if ((this.auditFailure != rhs.isAuditFailure()) || (this.auditSuccess != rhs.isAuditSuccess())
-            || (this.granting != rhs.isGranting()) || !this.acl.equals(rhs.getAcl()) || !this.id.equals(rhs.getId())
+            || (this.granting != rhs.isGranting())
             || !this.permission.equals(rhs.getPermission()) || !this.sid.equals(rhs.getSid())) {
             return false;
         }

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

@@ -14,6 +14,11 @@
  */
 package org.springframework.security.acls.domain;
 
+import java.io.Serializable;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Vector;
+
 import org.springframework.security.acls.AccessControlEntry;
 import org.springframework.security.acls.Acl;
 import org.springframework.security.acls.AuditableAcl;
@@ -24,15 +29,8 @@ import org.springframework.security.acls.Permission;
 import org.springframework.security.acls.UnloadedSidException;
 import org.springframework.security.acls.objectidentity.ObjectIdentity;
 import org.springframework.security.acls.sid.Sid;
-
 import org.springframework.util.Assert;
 
-import java.io.Serializable;
-
-import java.util.Iterator;
-import java.util.List;
-import java.util.Vector;
-
 
 /**
  * Base implementation of <code>Acl</code>.
@@ -44,8 +42,8 @@ public class AclImpl implements Acl, MutableAcl, AuditableAcl, OwnershipAcl {
     //~ Instance fields ================================================================================================
 	
     private Acl parentAcl;
-    private AclAuthorizationStrategy aclAuthorizationStrategy;
+    private transient AclAuthorizationStrategy aclAuthorizationStrategy;
-    private AuditLogger auditLogger;
+    private transient AuditLogger auditLogger;
     private List aces = new Vector();
     private ObjectIdentity objectIdentity;
     private Serializable id;
@@ -368,6 +366,8 @@ public class AclImpl implements Acl, MutableAcl, AuditableAcl, OwnershipAcl {
 
         sb.append("inheriting: ").append(this.entriesInheriting).append("; ");
         sb.append("parent: ").append((this.parentAcl == null) ? "Null" : this.parentAcl.getObjectIdentity().toString());
+        sb.append("aclAuthorizationStrategy: ").append(this.aclAuthorizationStrategy).append("; ");
+        sb.append("auditLogger: ").append(this.auditLogger);
         sb.append("]");
 
         return sb.toString();
@@ -404,4 +404,36 @@ public class AclImpl implements Acl, MutableAcl, AuditableAcl, OwnershipAcl {
             ace.setAuditFailure(auditFailure);
         }
     }
+    
+	public boolean equals(Object obj) {
+		if (obj instanceof AclImpl) {
+			
+			AclImpl rhs = (AclImpl) obj;
+			if (this.aces.equals(rhs.aces)) {
+				if ((this.parentAcl == null && rhs.parentAcl == null) || (this.parentAcl.equals(rhs.parentAcl))) {
+					if ((this.objectIdentity == null && rhs.objectIdentity == null) || (this.objectIdentity.equals(rhs.objectIdentity))) {
+						if ((this.id == null && rhs.id == null) || (this.id.equals(rhs.id))) {
+							if ((this.owner == null && rhs.owner == null) || this.owner.equals(rhs.owner)) {
+								if (this.entriesInheriting == rhs.entriesInheriting) {
+									if ((this.loadedSids == null && rhs.loadedSids == null)) {
+										return true;
+									}
+									if (this.loadedSids.length == rhs.loadedSids.length) {
+										for (int i = 0; i < this.loadedSids.length; i++) {
+											if (!this.loadedSids[i].equals(rhs.loadedSids[i])) {
+												return false;
+											}
+										}
+										return true;
+									}
+								}								
+							}
+						}
+					}
+				}
+			}
+		}
+		return false;
+	}
+    
 }

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

@@ -14,21 +14,29 @@
  */
 package org.springframework.security.acls.jdbc;
 
+import java.io.Serializable;
+
 import net.sf.ehcache.CacheException;
-import net.sf.ehcache.Element;
 import net.sf.ehcache.Ehcache;
+import net.sf.ehcache.Element;
 
 import org.springframework.security.acls.MutableAcl;
+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.objectidentity.ObjectIdentity;
-
+import org.springframework.security.util.FieldUtils;
 import org.springframework.util.Assert;
 
-import java.io.Serializable;
-
 
 /**
  * Simple implementation of {@link AclCache} that delegates to EH-CACHE.
  * 
+ * <p>
+ * Designed to handle the transient fields in {@link AclImpl}. Note that this implementation assumes all
+ * {@link AclImpl} instances share the same {@link AuditLogger} and {@link AclAuthorizationStrategy} instance.
+ * </p>
+ *
  * @author Ben Alex
  * @version $Id$
  */
@@ -36,6 +44,8 @@ public class EhCacheBasedAclCache implements AclCache {
     //~ Instance fields ================================================================================================
 
     private Ehcache cache;
+    private AuditLogger auditLogger;
+    private AclAuthorizationStrategy aclAuthorizationStrategy;
 
     //~ Constructors ===================================================================================================
 
@@ -81,10 +91,10 @@ public class EhCacheBasedAclCache implements AclCache {
             return null;
         }
 
-        return (MutableAcl) element.getValue();
+        return initializeTransientFields((MutableAcl)element.getValue());
     }
 
-    public MutableAcl getFromCache(Serializable pk) {
+	public MutableAcl getFromCache(Serializable pk) {
         Assert.notNull(pk, "Primary key (identifier) required");
 
         Element element = null;
@@ -97,7 +107,7 @@ public class EhCacheBasedAclCache implements AclCache {
             return null;
         }
 
-        return (MutableAcl) element.getValue();
+        return initializeTransientFields((MutableAcl) element.getValue());
     }
 
     public void putInCache(MutableAcl acl) {
@@ -105,6 +115,13 @@ public class EhCacheBasedAclCache implements AclCache {
         Assert.notNull(acl.getObjectIdentity(), "ObjectIdentity required");
         Assert.notNull(acl.getId(), "ID required");
 
+        if (this.aclAuthorizationStrategy == null) {
+            if (acl instanceof AclImpl) {
+            	this.aclAuthorizationStrategy = (AclAuthorizationStrategy) FieldUtils.getProtectedFieldValue("aclAuthorizationStrategy", acl);
+            	this.auditLogger = (AuditLogger) FieldUtils.getProtectedFieldValue("auditLogger", acl);
+            }
+        }
+        
         if ((acl.getParentAcl() != null) && (acl.getParentAcl() instanceof MutableAcl)) {
             putInCache((MutableAcl) acl.getParentAcl());
         }
@@ -112,4 +129,12 @@ public class EhCacheBasedAclCache implements AclCache {
         cache.put(new Element(acl.getObjectIdentity(), acl));
         cache.put(new Element(acl.getId(), acl));
     }
+
+    private MutableAcl initializeTransientFields(MutableAcl value) {
+    	if (value instanceof AclImpl) {
+    		FieldUtils.setProtectedFieldValue("aclAuthorizationStrategy", value, this.aclAuthorizationStrategy);
+    		FieldUtils.setProtectedFieldValue("auditLogger", value, this.auditLogger);
+    	}
+    	return value;
+	}
 }

acl/src/main/java/org/springframework/security/acls/sid/Sid.java

@@ -14,6 +14,8 @@
  */
 package org.springframework.security.acls.sid;
 
+import java.io.Serializable;
+
 /**
  * A security identity recognised by the ACL system.
  *
@@ -29,7 +31,7 @@ package org.springframework.security.acls.sid;
  * @author Ben Alex
  * @version $Id$
  */
-public interface Sid {
+public interface Sid extends Serializable {
     //~ Methods ========================================================================================================
 
     /**

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

@@ -86,8 +86,6 @@ public class AccessControlEntryTests extends TestCase {
 				BasePermission.ADMINISTRATION, true, true, true)));
 		Assert.assertFalse(ace.equals(new AccessControlEntryImpl(new Long(2), mockAcl, sid,
 				BasePermission.ADMINISTRATION, true, true, true)));
-		Assert.assertFalse(ace.equals(new AccessControlEntryImpl(new Long(1), new MockAcl(), sid,
-				BasePermission.ADMINISTRATION, true, true, true)));
 		Assert.assertFalse(ace.equals(new AccessControlEntryImpl(new Long(1), mockAcl, new PrincipalSid("scott"),
 				BasePermission.ADMINISTRATION, true, true, true)));
 		Assert.assertFalse(ace.equals(new AccessControlEntryImpl(new Long(1), mockAcl, sid, BasePermission.WRITE, true,

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

@@ -216,7 +216,7 @@ public class AclImplementationSecurityCheckTests extends TestCase {
 		// access
 		MutableAcl parentAcl = new AclImpl(identity, new Long(1), aclAuthorizationStrategy, new ConsoleAuditLogger());
 		parentAcl.insertAce(null, BasePermission.ADMINISTRATION, new PrincipalSid(auth), true);
-		MutableAcl childAcl = new AclImpl(identity, new Long(1), aclAuthorizationStrategy, new ConsoleAuditLogger());
+		MutableAcl childAcl = new AclImpl(identity, new Long(2), aclAuthorizationStrategy, new ConsoleAuditLogger());
 
 		// Check against the 'child' acl, which doesn't offer any authorization
 		// rights on CHANGE_OWNERSHIP

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

@@ -1,11 +1,25 @@
 package org.springframework.security.acls.jdbc;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
 import java.io.Serializable;
 
 import net.sf.ehcache.Cache;
-import net.sf.ehcache.Ehcache;
 import net.sf.ehcache.CacheManager;
+import net.sf.ehcache.Ehcache;
 
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
 import org.springframework.security.Authentication;
 import org.springframework.security.GrantedAuthority;
 import org.springframework.security.GrantedAuthorityImpl;
@@ -18,12 +32,7 @@ import org.springframework.security.acls.objectidentity.ObjectIdentity;
 import org.springframework.security.acls.objectidentity.ObjectIdentityImpl;
 import org.springframework.security.context.SecurityContextHolder;
 import org.springframework.security.providers.TestingAuthenticationToken;
-
+import org.springframework.security.util.FieldUtils;
-import org.junit.BeforeClass;
-import org.junit.AfterClass;
-import org.junit.After;
-import org.junit.Test;
-import static org.junit.Assert.*;
 
 /**
  * Tests {@link EhCacheBasedAclCache}
@@ -38,7 +47,8 @@ public class EhCacheBasedAclCacheTests {
     @BeforeClass
     public static void initCacheManaer() {
         cacheManager = new CacheManager();
-        cacheManager.addCache(new Cache("ehcachebasedacltests", 500, false, false, 30, 30));
+        // Use disk caching immediately (to test for serialization issue reported in SEC-527)
+        cacheManager.addCache(new Cache("ehcachebasedacltests", 0, true, false, 30, 30));
     }
 
     @AfterClass
@@ -116,6 +126,36 @@ public class EhCacheBasedAclCacheTests {
         }
     }
     
+    // SEC-527
+    @Test
+    public void testDiskSerializationOfMutableAclObjectInstance() throws Exception {
+        ObjectIdentity identity = new ObjectIdentityImpl("org.springframework.security.TargetObject", new Long(100));
+        AclAuthorizationStrategy aclAuthorizationStrategy = new AclAuthorizationStrategyImpl(new GrantedAuthority[] {
+                new GrantedAuthorityImpl("ROLE_OWNERSHIP"), new GrantedAuthorityImpl("ROLE_AUDITING"),
+                new GrantedAuthorityImpl("ROLE_GENERAL") });
+        MutableAcl acl = new AclImpl(identity, new Long(1), aclAuthorizationStrategy, new ConsoleAuditLogger());
+
+        // Serialization test
+        File file = File.createTempFile("SEC_TEST", ".object");
+        FileOutputStream fos = new FileOutputStream(file);
+    	ObjectOutputStream oos = new ObjectOutputStream(fos);
+    	oos.writeObject(acl);
+    	oos.close();
+        
+    	FileInputStream fis = new FileInputStream(file);
+    	ObjectInputStream ois = new ObjectInputStream(fis);
+    	MutableAcl retrieved = (MutableAcl) ois.readObject();
+    	ois.close();
+    	
+        assertEquals(acl, retrieved);
+        
+        Object retrieved1 = FieldUtils.getProtectedFieldValue("aclAuthorizationStrategy", retrieved);
+        assertEquals(null, retrieved1);
+        
+        Object retrieved2 = FieldUtils.getProtectedFieldValue("auditLogger", retrieved);
+        assertEquals(null, retrieved2);
+    }
+
     @Test
     public void cacheOperationsAclWithoutParent() throws Exception {
         Ehcache cache = getCache();
@@ -127,8 +167,12 @@ public class EhCacheBasedAclCacheTests {
                 new GrantedAuthorityImpl("ROLE_GENERAL") });
         MutableAcl acl = new AclImpl(identity, new Long(1), aclAuthorizationStrategy, new ConsoleAuditLogger());
 
+        assertEquals(0, cache.getDiskStoreSize());
         myCache.putInCache(acl);
         assertEquals(cache.getSize(), 2);
+        assertEquals(2, cache.getDiskStoreSize());
+        assertTrue(cache.isElementOnDisk(acl.getObjectIdentity()));
+        assertFalse(cache.isElementInMemory(acl.getObjectIdentity()));
         
         // Check we can get from cache the same objects we put in
         assertEquals(myCache.getFromCache(new Long(1)), acl);
@@ -140,14 +184,17 @@ public class EhCacheBasedAclCacheTests {
 
         myCache.putInCache(acl2);
         assertEquals(cache.getSize(), 4);
+        assertEquals(4, cache.getDiskStoreSize());
 
         // Try to evict an entry that doesn't exist
         myCache.evictFromCache(new Long(3));
         myCache.evictFromCache(new ObjectIdentityImpl("org.springframework.security.TargetObject", new Long(102)));
         assertEquals(cache.getSize(), 4);
+        assertEquals(4, cache.getDiskStoreSize());
 
         myCache.evictFromCache(new Long(1));
         assertEquals(cache.getSize(), 2);
+        assertEquals(2, cache.getDiskStoreSize());
 
         // Check the second object inserted
         assertEquals(myCache.getFromCache(new Long(2)), acl2);
@@ -177,8 +224,12 @@ public class EhCacheBasedAclCacheTests {
         
         acl.setParent(parentAcl);
 
+        assertEquals(0, cache.getDiskStoreSize());
         myCache.putInCache(acl);
         assertEquals(cache.getSize(), 4);
+        assertEquals(4, cache.getDiskStoreSize());
+        assertTrue(cache.isElementOnDisk(acl.getObjectIdentity()));
+        assertFalse(cache.isElementInMemory(acl.getObjectIdentity()));
 
         // Check we can get from cache the same objects we put in
         assertEquals(myCache.getFromCache(new Long(1)), acl);