From 5cf5140029bc42ed0edeca31055be4fb76ea8171 Mon Sep 17 00:00:00 2001 From: Ben Alex Date: Sat, 5 Apr 2008 05:58:11 +0000 Subject: [PATCH] SEC-527: Correct serialization issues with EH-CACHE. --- .../security/acls/AccessControlEntry.java | 2 +- .../security/acls/Permission.java | 4 +- .../acls/domain/AccessControlEntryImpl.java | 44 +++++++++++- .../security/acls/domain/AclImpl.java | 52 +++++++++++--- .../acls/jdbc/EhCacheBasedAclCache.java | 39 +++++++++-- .../security/acls/sid/Sid.java | 4 +- .../acls/domain/AccessControlEntryTests.java | 2 - .../AclImplementationSecurityCheckTests.java | 2 +- .../acls/jdbc/EhCacheBasedAclCacheTests.java | 69 ++++++++++++++++--- 9 files changed, 183 insertions(+), 35 deletions(-) diff --git a/acl/src/main/java/org/springframework/security/acls/AccessControlEntry.java b/acl/src/main/java/org/springframework/security/acls/AccessControlEntry.java index af0e7eed07..e64c28e89b 100644 --- a/acl/src/main/java/org/springframework/security/acls/AccessControlEntry.java +++ b/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(); diff --git a/acl/src/main/java/org/springframework/security/acls/Permission.java b/acl/src/main/java/org/springframework/security/acls/Permission.java index 36aba2c7cb..53e6bb5dbe 100644 --- a/acl/src/main/java/org/springframework/security/acls/Permission.java +++ b/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 = '~'; diff --git a/acl/src/main/java/org/springframework/security/acls/domain/AccessControlEntryImpl.java b/acl/src/main/java/org/springframework/security/acls/domain/AccessControlEntryImpl.java index 9adb58dde3..23ec5a8fe3 100644 --- a/acl/src/main/java/org/springframework/security/acls/domain/AccessControlEntryImpl.java +++ b/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) { - return false; + if (this.acl == null) { + 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; } diff --git a/acl/src/main/java/org/springframework/security/acls/domain/AclImpl.java b/acl/src/main/java/org/springframework/security/acls/domain/AclImpl.java index 450fc8d58d..4f44831380 100644 --- a/acl/src/main/java/org/springframework/security/acls/domain/AclImpl.java +++ b/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 Acl. @@ -42,10 +40,10 @@ import java.util.Vector; */ public class AclImpl implements Acl, MutableAcl, AuditableAcl, OwnershipAcl { //~ Instance fields ================================================================================================ - + private Acl parentAcl; - private AclAuthorizationStrategy aclAuthorizationStrategy; - private AuditLogger auditLogger; + private transient AclAuthorizationStrategy aclAuthorizationStrategy; + 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; + } + } diff --git a/acl/src/main/java/org/springframework/security/acls/jdbc/EhCacheBasedAclCache.java b/acl/src/main/java/org/springframework/security/acls/jdbc/EhCacheBasedAclCache.java index 4a217410a8..aa2508b65a 100644 --- a/acl/src/main/java/org/springframework/security/acls/jdbc/EhCacheBasedAclCache.java +++ b/acl/src/main/java/org/springframework/security/acls/jdbc/EhCacheBasedAclCache.java @@ -14,20 +14,28 @@ */ 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. + * + *

+ * 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. + *

* * @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; + } } diff --git a/acl/src/main/java/org/springframework/security/acls/sid/Sid.java b/acl/src/main/java/org/springframework/security/acls/sid/Sid.java index 725551a182..ecb06167eb 100644 --- a/acl/src/main/java/org/springframework/security/acls/sid/Sid.java +++ b/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 ======================================================================================================== /** diff --git a/acl/src/test/java/org/springframework/security/acls/domain/AccessControlEntryTests.java b/acl/src/test/java/org/springframework/security/acls/domain/AccessControlEntryTests.java index 7c0700c704..8dd8ad3797 100644 --- a/acl/src/test/java/org/springframework/security/acls/domain/AccessControlEntryTests.java +++ b/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, diff --git a/acl/src/test/java/org/springframework/security/acls/domain/AclImplementationSecurityCheckTests.java b/acl/src/test/java/org/springframework/security/acls/domain/AclImplementationSecurityCheckTests.java index 3740239423..dbc25ccf60 100644 --- a/acl/src/test/java/org/springframework/security/acls/domain/AclImplementationSecurityCheckTests.java +++ b/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 diff --git a/acl/src/test/java/org/springframework/security/acls/jdbc/EhCacheBasedAclCacheTests.java b/acl/src/test/java/org/springframework/security/acls/jdbc/EhCacheBasedAclCacheTests.java index e518c01ce4..4d227b2774 100644 --- a/acl/src/test/java/org/springframework/security/acls/jdbc/EhCacheBasedAclCacheTests.java +++ b/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.junit.BeforeClass; -import org.junit.AfterClass; -import org.junit.After; -import org.junit.Test; -import static org.junit.Assert.*; +import org.springframework.security.util.FieldUtils; /** * 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 @@ -115,6 +125,36 @@ public class EhCacheBasedAclCacheTests { assertTrue(true); } } + + // 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 { @@ -127,9 +167,13 @@ 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); assertEquals(myCache.getFromCache(identity), 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);