From 677607bcadf33d85274645431aa1256c77016446 Mon Sep 17 00:00:00 2001 From: Ben Alex Date: Sat, 5 Apr 2008 20:43:10 +0000 Subject: [PATCH] SEC-530: Refactor ACL module so ACE manipulation is index-based as opposed to AccessControlEntry.getId() based. --- .../springframework/security/acls/Acl.java | 15 ++-- .../security/acls/AuditableAcl.java | 3 +- .../security/acls/MutableAcl.java | 21 ++--- .../security/acls/domain/AclImpl.java | 83 +++++++------------ .../security/acls/domain/AclImplTests.java | 80 +++++++++--------- .../AclImplementationSecurityCheckTests.java | 12 +-- .../jdbc/AclPermissionInheritanceTests.java | 8 +- .../acls/jdbc/JdbcAclServiceTests.java | 20 ++--- .../sample/contact/ContactManagerBackend.java | 4 +- .../sample/contact/DataSourcePopulator.java | 2 +- .../secured/SecureDataSourcePopulator.java | 4 +- .../dms/secured/SecureDocumentDaoImpl.java | 2 +- 12 files changed, 107 insertions(+), 147 deletions(-) diff --git a/acl/src/main/java/org/springframework/security/acls/Acl.java b/acl/src/main/java/org/springframework/security/acls/Acl.java index a5dbc97fe3..b8dafaa7e5 100644 --- a/acl/src/main/java/org/springframework/security/acls/Acl.java +++ b/acl/src/main/java/org/springframework/security/acls/Acl.java @@ -32,10 +32,11 @@ import java.io.Serializable; *

* *

- * TODO: Clarify this paragraph - * An implementation represents the {@link org.springframework.security.acls.Permission} - * list applicable for some or all {@link org.springframework.security.acls.sid.Sid} - * instances. + * Implementing classes may elect to return instances that represent + * {@link org.springframework.security.acls.Permission} information for either + * some OR all {@link org.springframework.security.acls.sid.Sid} + * instances. Therefore, an instance may NOT necessarily contain ALL Sids + * for a given domain object. *

* * @author Ben Alex @@ -49,9 +50,9 @@ public interface Acl extends Serializable { * *

This method is typically used for administrative purposes.

* - *

The order that entries appear in the array is unspecified. However, if implementations use - * particular ordering logic in authorization decisions, the entries returned by this method - * MUST be ordered in that manner.

+ *

The order that entries appear in the array is important for methods declared in the + * {@link MutableAcl} interface. Furthermore, some implementations MAY use ordering as + * part of advanced permission checking.

* *

Do NOT use this method for making authorization decisions. Instead use {@link * #isGranted(Permission[], Sid[], boolean)}.

diff --git a/acl/src/main/java/org/springframework/security/acls/AuditableAcl.java b/acl/src/main/java/org/springframework/security/acls/AuditableAcl.java index 23a04b6e26..d102cb45aa 100644 --- a/acl/src/main/java/org/springframework/security/acls/AuditableAcl.java +++ b/acl/src/main/java/org/springframework/security/acls/AuditableAcl.java @@ -14,7 +14,6 @@ */ package org.springframework.security.acls; -import java.io.Serializable; /** @@ -27,5 +26,5 @@ import java.io.Serializable; public interface AuditableAcl extends MutableAcl { //~ Methods ======================================================================================================== - void updateAuditing(Serializable aceId, boolean auditSuccess, boolean auditFailure); + void updateAuditing(int aceIndex, boolean auditSuccess, boolean auditFailure); } diff --git a/acl/src/main/java/org/springframework/security/acls/MutableAcl.java b/acl/src/main/java/org/springframework/security/acls/MutableAcl.java index de5f7d6603..0d51d1db43 100644 --- a/acl/src/main/java/org/springframework/security/acls/MutableAcl.java +++ b/acl/src/main/java/org/springframework/security/acls/MutableAcl.java @@ -14,10 +14,10 @@ */ package org.springframework.security.acls; -import org.springframework.security.acls.sid.Sid; - import java.io.Serializable; +import org.springframework.security.acls.sid.Sid; + /** * A mutable Acl. @@ -33,18 +33,7 @@ import java.io.Serializable; public interface MutableAcl extends Acl { //~ Methods ======================================================================================================== - void deleteAce(Serializable aceId) throws NotFoundException; - - /** - * Retrieves all of the non-deleted {@link AccessControlEntry} instances currently stored by the - * MutableAcl. The returned objects should be immutable outside the package, and therefore it is safe - * to return them to the caller for informational purposes. The AccessControlEntry information is - * needed so that invocations of update and delete methods on the MutableAcl can refer to a valid - * {@link AccessControlEntry#getId()}. - * - * @return DOCUMENT ME! - */ - AccessControlEntry[] getEntries(); + void deleteAce(int aceIndex) throws NotFoundException; /** * Obtains an identifier that represents this MutableAcl. @@ -53,7 +42,7 @@ public interface MutableAcl extends Acl { */ Serializable getId(); - void insertAce(Serializable afterAceId, Permission permission, Sid sid, boolean granting) + void insertAce(int atIndexLocation, Permission permission, Sid sid, boolean granting) throws NotFoundException; /** @@ -77,6 +66,6 @@ public interface MutableAcl extends Acl { */ void setParent(Acl newParent); - void updateAce(Serializable aceId, Permission permission) + void updateAce(int aceIndex, Permission permission) throws NotFoundException; } 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 7666576bc2..ac796a500c 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 @@ -114,34 +114,22 @@ public class AclImpl implements Acl, MutableAcl, AuditableAcl, OwnershipAcl { //~ Methods ======================================================================================================== - public void deleteAce(Serializable aceId) throws NotFoundException { - aclAuthorizationStrategy.securityCheck(this, AclAuthorizationStrategy.CHANGE_GENERAL); - - synchronized (aces) { - int offset = findAceOffset(aceId); - - if (offset == -1) { - throw new NotFoundException("Requested ACE ID not found"); - } - - this.aces.remove(offset); + private void verifyAceIndexExists(int aceIndex) { + if (aceIndex < 0) { + throw new NotFoundException("aceIndex must be greater than or equal to zero"); + } + if (aceIndex > this.aces.size()) { + throw new NotFoundException("aceIndex must correctly refer to an index of the AccessControlEntry collection"); } } - - private int findAceOffset(Serializable aceId) { - Assert.notNull(aceId, "ACE ID is required"); - + + public void deleteAce(int aceIndex) throws NotFoundException { + aclAuthorizationStrategy.securityCheck(this, AclAuthorizationStrategy.CHANGE_GENERAL); + verifyAceIndexExists(aceIndex); + synchronized (aces) { - for (int i = 0; i < aces.size(); i++) { - AccessControlEntry ace = (AccessControlEntry) aces.get(i); - - if (ace.getId().equals(aceId)) { - return i; - } - } + this.aces.remove(aceIndex); } - - return -1; } public AccessControlEntry[] getEntries() { @@ -165,26 +153,22 @@ public class AclImpl implements Acl, MutableAcl, AuditableAcl, OwnershipAcl { return parentAcl; } - public void insertAce(Serializable afterAceId, Permission permission, Sid sid, boolean granting) + public void insertAce(int atIndexLocation, Permission permission, Sid sid, boolean granting) throws NotFoundException { aclAuthorizationStrategy.securityCheck(this, AclAuthorizationStrategy.CHANGE_GENERAL); Assert.notNull(permission, "Permission required"); Assert.notNull(sid, "Sid required"); + if (atIndexLocation < 0) { + throw new NotFoundException("atIndexLocation must be greater than or equal to zero"); + } + if (atIndexLocation > this.aces.size()) { + throw new NotFoundException("atIndexLocation must be less than or equal to the size of the AccessControlEntry collection"); + } AccessControlEntryImpl ace = new AccessControlEntryImpl(null, this, sid, permission, granting, false, false); synchronized (aces) { - if (afterAceId != null) { - int offset = findAceOffset(afterAceId); - - if (offset == -1) { - throw new NotFoundException("Requested ACE ID not found"); - } - - this.aces.add(offset + 1, ace); - } else { - this.aces.add(ace); - } + this.aces.add(atIndexLocation, ace); } } @@ -372,33 +356,23 @@ public class AclImpl implements Acl, MutableAcl, AuditableAcl, OwnershipAcl { return sb.toString(); } - public void updateAce(Serializable aceId, Permission permission) + public void updateAce(int aceIndex, Permission permission) throws NotFoundException { aclAuthorizationStrategy.securityCheck(this, AclAuthorizationStrategy.CHANGE_GENERAL); - + verifyAceIndexExists(aceIndex); + synchronized (aces) { - int offset = findAceOffset(aceId); - - if (offset == -1) { - throw new NotFoundException("Requested ACE ID not found"); - } - - AccessControlEntryImpl ace = (AccessControlEntryImpl) aces.get(offset); + AccessControlEntryImpl ace = (AccessControlEntryImpl) aces.get(aceIndex); ace.setPermission(permission); } } - public void updateAuditing(Serializable aceId, boolean auditSuccess, boolean auditFailure) { + public void updateAuditing(int aceIndex, boolean auditSuccess, boolean auditFailure) { aclAuthorizationStrategy.securityCheck(this, AclAuthorizationStrategy.CHANGE_AUDITING); - + verifyAceIndexExists(aceIndex); + synchronized (aces) { - int offset = findAceOffset(aceId); - - if (offset == -1) { - throw new NotFoundException("Requested ACE ID not found"); - } - - AccessControlEntryImpl ace = (AccessControlEntryImpl) aces.get(offset); + AccessControlEntryImpl ace = (AccessControlEntryImpl) aces.get(aceIndex); ace.setAuditSuccess(auditSuccess); ace.setAuditFailure(auditFailure); } @@ -406,7 +380,6 @@ public class AclImpl implements Acl, MutableAcl, AuditableAcl, OwnershipAcl { 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))) { diff --git a/acl/src/test/java/org/springframework/security/acls/domain/AclImplTests.java b/acl/src/test/java/org/springframework/security/acls/domain/AclImplTests.java index caf71ddaa9..d80c53e557 100644 --- a/acl/src/test/java/org/springframework/security/acls/domain/AclImplTests.java +++ b/acl/src/test/java/org/springframework/security/acls/domain/AclImplTests.java @@ -138,14 +138,14 @@ public class AclImplTests extends TestCase { MutableAcl acl = new AclImpl(identity, new Long(1), strategy, auditLogger, null, null, true, new PrincipalSid( "johndoe")); try { - acl.insertAce(new Long(1), null, new GrantedAuthoritySid("ROLE_IGNORED"), true); + acl.insertAce(0, null, new GrantedAuthoritySid("ROLE_IGNORED"), true); fail("It should have thrown IllegalArgumentException"); } catch (IllegalArgumentException expected) { assertTrue(true); } try { - acl.insertAce(new Long(1), BasePermission.READ, null, true); + acl.insertAce(0, BasePermission.READ, null, true); fail("It should have thrown IllegalArgumentException"); } catch (IllegalArgumentException expected) { @@ -168,7 +168,7 @@ public class AclImplTests extends TestCase { MockAclService service = new MockAclService(); // Insert one permission - acl.insertAce(null, BasePermission.READ, new GrantedAuthoritySid("ROLE_TEST1"), true); + acl.insertAce(0, BasePermission.READ, new GrantedAuthoritySid("ROLE_TEST1"), true); service.updateAcl(acl); // Check it was successfully added assertEquals(1, acl.getEntries().length); @@ -177,7 +177,7 @@ public class AclImplTests extends TestCase { assertEquals(acl.getEntries()[0].getSid(), new GrantedAuthoritySid("ROLE_TEST1")); // Add a second permission - acl.insertAce(null, BasePermission.READ, new GrantedAuthoritySid("ROLE_TEST2"), true); + acl.insertAce(1, BasePermission.READ, new GrantedAuthoritySid("ROLE_TEST2"), true); service.updateAcl(acl); // Check it was added on the last position assertEquals(2, acl.getEntries().length); @@ -186,7 +186,7 @@ public class AclImplTests extends TestCase { assertEquals(acl.getEntries()[1].getSid(), new GrantedAuthoritySid("ROLE_TEST2")); // Add a third permission, after the first one - acl.insertAce(acl.getEntries()[0].getId(), BasePermission.WRITE, new GrantedAuthoritySid("ROLE_TEST3"), false); + acl.insertAce(1, BasePermission.WRITE, new GrantedAuthoritySid("ROLE_TEST3"), false); service.updateAcl(acl); assertEquals(3, acl.getEntries().length); // Check the third entry was added between the two existent ones @@ -213,11 +213,11 @@ public class AclImplTests extends TestCase { MockAclService service = new MockAclService(); // Insert one permission - acl.insertAce(null, BasePermission.READ, new GrantedAuthoritySid("ROLE_TEST1"), true); + acl.insertAce(0, BasePermission.READ, new GrantedAuthoritySid("ROLE_TEST1"), true); service.updateAcl(acl); try { - acl.insertAce(new Long(5), BasePermission.READ, new GrantedAuthoritySid("ROLE_TEST2"), true); + acl.insertAce(55, BasePermission.READ, new GrantedAuthoritySid("ROLE_TEST2"), true); fail("It should have thrown NotFoundException"); } catch (NotFoundException expected) { @@ -240,28 +240,28 @@ public class AclImplTests extends TestCase { MockAclService service = new MockAclService(); // Add several permissions - acl.insertAce(null, BasePermission.READ, new GrantedAuthoritySid("ROLE_TEST1"), true); - acl.insertAce(null, BasePermission.READ, new GrantedAuthoritySid("ROLE_TEST2"), true); - acl.insertAce(null, BasePermission.READ, new GrantedAuthoritySid("ROLE_TEST3"), true); + acl.insertAce(0, BasePermission.READ, new GrantedAuthoritySid("ROLE_TEST1"), true); + acl.insertAce(1, BasePermission.READ, new GrantedAuthoritySid("ROLE_TEST2"), true); + acl.insertAce(2, BasePermission.READ, new GrantedAuthoritySid("ROLE_TEST3"), true); service.updateAcl(acl); // Delete first permission and check the order of the remaining permissions is kept - acl.deleteAce(new Long(1)); + acl.deleteAce(0); assertEquals(2, acl.getEntries().length); assertEquals(acl.getEntries()[0].getSid(), new GrantedAuthoritySid("ROLE_TEST2")); assertEquals(acl.getEntries()[1].getSid(), new GrantedAuthoritySid("ROLE_TEST3")); // Add one more permission and remove the permission in the middle - acl.insertAce(null, BasePermission.READ, new GrantedAuthoritySid("ROLE_TEST4"), true); + acl.insertAce(2, BasePermission.READ, new GrantedAuthoritySid("ROLE_TEST4"), true); service.updateAcl(acl); - acl.deleteAce(new Long(2)); + acl.deleteAce(1); assertEquals(2, acl.getEntries().length); assertEquals(acl.getEntries()[0].getSid(), new GrantedAuthoritySid("ROLE_TEST2")); assertEquals(acl.getEntries()[1].getSid(), new GrantedAuthoritySid("ROLE_TEST4")); // Remove remaining permissions - acl.deleteAce(new Long(1)); - acl.deleteAce(new Long(3)); + acl.deleteAce(1); + acl.deleteAce(0); assertEquals(0, acl.getEntries().length); } @@ -278,7 +278,7 @@ public class AclImplTests extends TestCase { MutableAcl acl = new AclImpl(identity, new Long(1), strategy, auditLogger, null, null, true, new PrincipalSid( "johndoe")); try { - acl.deleteAce(new Long(1)); + acl.deleteAce(99); fail("It should have thrown NotFoundException"); } catch (NotFoundException expected) { @@ -327,10 +327,10 @@ public class AclImplTests extends TestCase { "johndoe")); // Grant some permissions - rootAcl.insertAce(null, BasePermission.READ, new PrincipalSid("ben"), false); - rootAcl.insertAce(null, BasePermission.WRITE, new PrincipalSid("scott"), true); - rootAcl.insertAce(null, BasePermission.WRITE, new PrincipalSid("rod"), false); - rootAcl.insertAce(null, BasePermission.WRITE, new GrantedAuthoritySid("WRITE_ACCESS_ROLE"), true); + rootAcl.insertAce(0, BasePermission.READ, new PrincipalSid("ben"), false); + rootAcl.insertAce(1, BasePermission.WRITE, new PrincipalSid("scott"), true); + rootAcl.insertAce(2, BasePermission.WRITE, new PrincipalSid("rod"), false); + rootAcl.insertAce(3, BasePermission.WRITE, new GrantedAuthoritySid("WRITE_ACCESS_ROLE"), true); // Check permissions granting Permission[] permissions = new Permission[] { BasePermission.READ, BasePermission.CREATE }; @@ -394,14 +394,14 @@ public class AclImplTests extends TestCase { parentAcl1.setParent(grandParentAcl); // Add some permissions - grandParentAcl.insertAce(null, BasePermission.READ, new GrantedAuthoritySid("ROLE_USER_READ"), true); - grandParentAcl.insertAce(null, BasePermission.WRITE, new PrincipalSid("ben"), true); - grandParentAcl.insertAce(null, BasePermission.DELETE, new PrincipalSid("ben"), false); - grandParentAcl.insertAce(null, BasePermission.DELETE, new PrincipalSid("scott"), true); - parentAcl1.insertAce(null, BasePermission.READ, new PrincipalSid("scott"), true); - parentAcl1.insertAce(null, BasePermission.DELETE, new PrincipalSid("scott"), false); - parentAcl2.insertAce(null, BasePermission.CREATE, new PrincipalSid("ben"), true); - childAcl1.insertAce(null, BasePermission.CREATE, new PrincipalSid("scott"), true); + grandParentAcl.insertAce(0, BasePermission.READ, new GrantedAuthoritySid("ROLE_USER_READ"), true); + grandParentAcl.insertAce(1, BasePermission.WRITE, new PrincipalSid("ben"), true); + grandParentAcl.insertAce(2, BasePermission.DELETE, new PrincipalSid("ben"), false); + grandParentAcl.insertAce(3, BasePermission.DELETE, new PrincipalSid("scott"), true); + parentAcl1.insertAce(0, BasePermission.READ, new PrincipalSid("scott"), true); + parentAcl1.insertAce(1, BasePermission.DELETE, new PrincipalSid("scott"), false); + parentAcl2.insertAce(0, BasePermission.CREATE, new PrincipalSid("ben"), true); + childAcl1.insertAce(0, BasePermission.CREATE, new PrincipalSid("scott"), true); // Check granting process for parent1 assertTrue(parentAcl1.isGranted(new Permission[] { BasePermission.READ }, new Sid[] { new PrincipalSid("scott") }, @@ -464,9 +464,9 @@ public class AclImplTests extends TestCase { "johndoe")); MockAclService service = new MockAclService(); - acl.insertAce(null, BasePermission.READ, new GrantedAuthoritySid("ROLE_USER_READ"), true); - acl.insertAce(null, BasePermission.WRITE, new GrantedAuthoritySid("ROLE_USER_READ"), true); - acl.insertAce(null, BasePermission.CREATE, new PrincipalSid("ben"), true); + acl.insertAce(0, BasePermission.READ, new GrantedAuthoritySid("ROLE_USER_READ"), true); + acl.insertAce(1, BasePermission.WRITE, new GrantedAuthoritySid("ROLE_USER_READ"), true); + acl.insertAce(2, BasePermission.CREATE, new PrincipalSid("ben"), true); service.updateAcl(acl); assertEquals(acl.getEntries()[0].getPermission(), BasePermission.READ); @@ -474,9 +474,9 @@ public class AclImplTests extends TestCase { assertEquals(acl.getEntries()[2].getPermission(), BasePermission.CREATE); // Change each permission - acl.updateAce(new Long(1), BasePermission.CREATE); - acl.updateAce(new Long(2), BasePermission.DELETE); - acl.updateAce(new Long(3), BasePermission.READ); + acl.updateAce(0, BasePermission.CREATE); + acl.updateAce(1, BasePermission.DELETE); + acl.updateAce(2, BasePermission.READ); // Check the change was successfuly made assertEquals(acl.getEntries()[0].getPermission(), BasePermission.CREATE); @@ -498,8 +498,8 @@ public class AclImplTests extends TestCase { "johndoe")); MockAclService service = new MockAclService(); - acl.insertAce(null, BasePermission.READ, new GrantedAuthoritySid("ROLE_USER_READ"), true); - acl.insertAce(null, BasePermission.WRITE, new GrantedAuthoritySid("ROLE_USER_READ"), true); + acl.insertAce(0, BasePermission.READ, new GrantedAuthoritySid("ROLE_USER_READ"), true); + acl.insertAce(1, BasePermission.WRITE, new GrantedAuthoritySid("ROLE_USER_READ"), true); service.updateAcl(acl); assertFalse(((AuditableAccessControlEntry) acl.getEntries()[0]).isAuditFailure()); @@ -508,8 +508,8 @@ public class AclImplTests extends TestCase { assertFalse(((AuditableAccessControlEntry) acl.getEntries()[1]).isAuditSuccess()); // Change each permission - ((AuditableAcl) acl).updateAuditing(new Long(1), true, true); - ((AuditableAcl) acl).updateAuditing(new Long(2), true, true); + ((AuditableAcl) acl).updateAuditing(0, true, true); + ((AuditableAcl) acl).updateAuditing(1, true, true); // Check the change was successfuly made assertTrue(((AuditableAccessControlEntry) acl.getEntries()[0]).isAuditFailure()); @@ -534,8 +534,8 @@ public class AclImplTests extends TestCase { MutableAcl parentAcl = new AclImpl(identity2, new Long(2), strategy, auditLogger, null, null, true, new PrincipalSid( "johndoe")); MockAclService service = new MockAclService(); - acl.insertAce(null, BasePermission.READ, new GrantedAuthoritySid("ROLE_USER_READ"), true); - acl.insertAce(null, BasePermission.WRITE, new GrantedAuthoritySid("ROLE_USER_READ"), true); + acl.insertAce(0, BasePermission.READ, new GrantedAuthoritySid("ROLE_USER_READ"), true); + acl.insertAce(1, BasePermission.WRITE, new GrantedAuthoritySid("ROLE_USER_READ"), true); service.updateAcl(acl); assertEquals(acl.getId(), new Long(1)); 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 dbc25ccf60..32686b5632 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 @@ -114,7 +114,7 @@ public class AclImplementationSecurityCheckTests extends TestCase { // Let's give the principal the ADMINISTRATION permission, without // granting access MutableAcl aclFirstDeny = new AclImpl(identity, new Long(1), aclAuthorizationStrategy, new ConsoleAuditLogger()); - aclFirstDeny.insertAce(null, BasePermission.ADMINISTRATION, new PrincipalSid(auth), false); + aclFirstDeny.insertAce(0, BasePermission.ADMINISTRATION, new PrincipalSid(auth), false); // The CHANGE_GENERAL test should pass as the principal has ROLE_GENERAL try { @@ -143,7 +143,7 @@ public class AclImplementationSecurityCheckTests extends TestCase { } // Add granting access to this principal - aclFirstDeny.insertAce(null, BasePermission.ADMINISTRATION, new PrincipalSid(auth), true); + aclFirstDeny.insertAce(1, BasePermission.ADMINISTRATION, new PrincipalSid(auth), true); // and try again for CHANGE_AUDITING - the first ACE's granting flag // (false) will deny this access try { @@ -158,7 +158,7 @@ public class AclImplementationSecurityCheckTests extends TestCase { // permission, with granting access MutableAcl aclFirstAllow = new AclImpl(identity, new Long(1), aclAuthorizationStrategy, new ConsoleAuditLogger()); - aclFirstAllow.insertAce(null, BasePermission.ADMINISTRATION, new PrincipalSid(auth), true); + aclFirstAllow.insertAce(0, BasePermission.ADMINISTRATION, new PrincipalSid(auth), true); // The CHANGE_AUDITING test should pass as there is one ACE with // granting access @@ -171,7 +171,7 @@ public class AclImplementationSecurityCheckTests extends TestCase { } // Add a deny ACE and test again for CHANGE_AUDITING - aclFirstAllow.insertAce(null, BasePermission.ADMINISTRATION, new PrincipalSid(auth), false); + aclFirstAllow.insertAce(1, BasePermission.ADMINISTRATION, new PrincipalSid(auth), false); try { aclAuthorizationStrategy.securityCheck(aclFirstAllow, AclAuthorizationStrategy.CHANGE_AUDITING); Assert.assertTrue(true); @@ -215,7 +215,7 @@ public class AclImplementationSecurityCheckTests extends TestCase { // Let's give the principal an ADMINISTRATION permission, with granting // access MutableAcl parentAcl = new AclImpl(identity, new Long(1), aclAuthorizationStrategy, new ConsoleAuditLogger()); - parentAcl.insertAce(null, BasePermission.ADMINISTRATION, new PrincipalSid(auth), true); + parentAcl.insertAce(0, BasePermission.ADMINISTRATION, new PrincipalSid(auth), true); MutableAcl childAcl = new AclImpl(identity, new Long(2), aclAuthorizationStrategy, new ConsoleAuditLogger()); // Check against the 'child' acl, which doesn't offer any authorization @@ -244,7 +244,7 @@ public class AclImplementationSecurityCheckTests extends TestCase { MutableAcl rootParentAcl = new AclImpl(identity, new Long(1), aclAuthorizationStrategy, new ConsoleAuditLogger()); parentAcl = new AclImpl(identity, new Long(1), aclAuthorizationStrategy, new ConsoleAuditLogger()); - rootParentAcl.insertAce(null, BasePermission.ADMINISTRATION, new PrincipalSid(auth), true); + rootParentAcl.insertAce(0, BasePermission.ADMINISTRATION, new PrincipalSid(auth), true); parentAcl.setEntriesInheriting(true); parentAcl.setParent(rootParentAcl); childAcl.setParent(parentAcl); diff --git a/acl/src/test/java/org/springframework/security/acls/jdbc/AclPermissionInheritanceTests.java b/acl/src/test/java/org/springframework/security/acls/jdbc/AclPermissionInheritanceTests.java index 0b36a925f1..188afe66ca 100644 --- a/acl/src/test/java/org/springframework/security/acls/jdbc/AclPermissionInheritanceTests.java +++ b/acl/src/test/java/org/springframework/security/acls/jdbc/AclPermissionInheritanceTests.java @@ -75,12 +75,12 @@ public class AclPermissionInheritanceTests extends TestCase { aclService.updateAcl(child); parent = (AclImpl) aclService.readAclById(rootObject); - parent.insertAce(null, BasePermission.READ, + parent.insertAce(0, BasePermission.READ, new PrincipalSid("john"), true); aclService.updateAcl(parent); parent = (AclImpl) aclService.readAclById(rootObject); - parent.insertAce(null, BasePermission.READ, + parent.insertAce(1, BasePermission.READ, new PrincipalSid("joe"), true); aclService.updateAcl(parent); @@ -109,11 +109,11 @@ public class AclPermissionInheritanceTests extends TestCase { child.setParent(parent); aclService.updateAcl(child); - parent.insertAce(null, BasePermission.ADMINISTRATION, + parent.insertAce(0, BasePermission.ADMINISTRATION, new GrantedAuthoritySid("ROLE_ADMINISTRATOR"), true); aclService.updateAcl(parent); - parent.insertAce(null, BasePermission.DELETE, new PrincipalSid("terry"), true); + parent.insertAce(1, BasePermission.DELETE, new PrincipalSid("terry"), true); aclService.updateAcl(parent); child = (MutableAcl) aclService.readAclById( diff --git a/acl/src/test/java/org/springframework/security/acls/jdbc/JdbcAclServiceTests.java b/acl/src/test/java/org/springframework/security/acls/jdbc/JdbcAclServiceTests.java index 5b767d04eb..fbd8144676 100644 --- a/acl/src/test/java/org/springframework/security/acls/jdbc/JdbcAclServiceTests.java +++ b/acl/src/test/java/org/springframework/security/acls/jdbc/JdbcAclServiceTests.java @@ -108,10 +108,10 @@ public class JdbcAclServiceTests extends AbstractTransactionalDataSourceSpringCo child.setParent(middleParent); // Now let's add a couple of permissions - topParent.insertAce(null, BasePermission.READ, new PrincipalSid(auth), true); - topParent.insertAce(null, BasePermission.WRITE, new PrincipalSid(auth), false); - middleParent.insertAce(null, BasePermission.DELETE, new PrincipalSid(auth), true); - child.insertAce(null, BasePermission.DELETE, new PrincipalSid(auth), false); + topParent.insertAce(0, BasePermission.READ, new PrincipalSid(auth), true); + topParent.insertAce(1, BasePermission.WRITE, new PrincipalSid(auth), false); + middleParent.insertAce(0, BasePermission.DELETE, new PrincipalSid(auth), true); + child.insertAce(0, BasePermission.DELETE, new PrincipalSid(auth), false); // Explictly save the changed ACL jdbcMutableAclService.updateAcl(topParent); @@ -144,10 +144,8 @@ public class JdbcAclServiceTests extends AbstractTransactionalDataSourceSpringCo // Check the retrieved rights are correct assertTrue(topParent.isGranted(new Permission[] {BasePermission.READ}, new Sid[] {new PrincipalSid(auth)}, false)); - assertFalse(topParent.isGranted(new Permission[] {BasePermission.WRITE}, new Sid[] {new PrincipalSid(auth)}, - false)); - assertTrue(middleParent.isGranted(new Permission[] {BasePermission.DELETE}, new Sid[] {new PrincipalSid(auth)}, - false)); + assertFalse(topParent.isGranted(new Permission[] {BasePermission.WRITE}, new Sid[] {new PrincipalSid(auth)}, false)); + assertTrue(middleParent.isGranted(new Permission[] {BasePermission.DELETE}, new Sid[] {new PrincipalSid(auth)}, false)); assertFalse(child.isGranted(new Permission[] {BasePermission.DELETE}, new Sid[] {new PrincipalSid(auth)}, false)); try { @@ -186,10 +184,10 @@ public class JdbcAclServiceTests extends AbstractTransactionalDataSourceSpringCo } // Let's add an identical permission to the child, but it'll appear AFTER the current permission, so has no impact - child.insertAce(null, BasePermission.DELETE, new PrincipalSid(auth), true); + child.insertAce(1, BasePermission.DELETE, new PrincipalSid(auth), true); // Let's also add another permission to the child - child.insertAce(null, BasePermission.CREATE, new PrincipalSid(auth), true); + child.insertAce(2, BasePermission.CREATE, new PrincipalSid(auth), true); // Save the changed child jdbcMutableAclService.updateAcl(child); @@ -213,7 +211,7 @@ public class JdbcAclServiceTests extends AbstractTransactionalDataSourceSpringCo assertNotNull(entry.getId()); // Now delete that first ACE - child.deleteAce(entry.getId()); + child.deleteAce(0); // Save and check it worked child = jdbcMutableAclService.updateAcl(child); diff --git a/samples/contacts/src/main/java/sample/contact/ContactManagerBackend.java b/samples/contacts/src/main/java/sample/contact/ContactManagerBackend.java index 947770d76d..df81964cc4 100644 --- a/samples/contacts/src/main/java/sample/contact/ContactManagerBackend.java +++ b/samples/contacts/src/main/java/sample/contact/ContactManagerBackend.java @@ -66,7 +66,7 @@ public class ContactManagerBackend extends ApplicationObjectSupport implements C acl = mutableAclService.createAcl(oid); } - acl.insertAce(null, permission, recipient, true); + acl.insertAce(acl.getEntries().length, permission, recipient, true); mutableAclService.updateAcl(acl); if (logger.isDebugEnabled()) { @@ -113,7 +113,7 @@ public class ContactManagerBackend extends ApplicationObjectSupport implements C for (int i = 0; i < entries.length; i++) { if (entries[i].getSid().equals(recipient) && entries[i].getPermission().equals(permission)) { - acl.deleteAce(entries[i].getId()); + acl.deleteAce(i); } } diff --git a/samples/contacts/src/main/java/sample/contact/DataSourcePopulator.java b/samples/contacts/src/main/java/sample/contact/DataSourcePopulator.java index e01ecc183b..96d213674c 100644 --- a/samples/contacts/src/main/java/sample/contact/DataSourcePopulator.java +++ b/samples/contacts/src/main/java/sample/contact/DataSourcePopulator.java @@ -243,7 +243,7 @@ public class DataSourcePopulator implements InitializingBean { private void grantPermissions(int contactNumber, String recipientUsername, Permission permission) { AclImpl acl = (AclImpl) mutableAclService.readAclById(new ObjectIdentityImpl(Contact.class, new Long(contactNumber))); - acl.insertAce(null, permission, new PrincipalSid(recipientUsername), true); + acl.insertAce(acl.getEntries().length, permission, new PrincipalSid(recipientUsername), true); updateAclInTransaction(acl); } diff --git a/samples/dms/src/main/java/sample/dms/secured/SecureDataSourcePopulator.java b/samples/dms/src/main/java/sample/dms/secured/SecureDataSourcePopulator.java index 98b6c97cf3..7a7149da86 100755 --- a/samples/dms/src/main/java/sample/dms/secured/SecureDataSourcePopulator.java +++ b/samples/dms/src/main/java/sample/dms/secured/SecureDataSourcePopulator.java @@ -76,9 +76,9 @@ public class SecureDataSourcePopulator extends DataSourcePopulator { // Now we have an ACL, add another ACE to it if (level == LEVEL_NEGATE_READ) { - acl.insertAce(null, permission, sid, false); // not granting + acl.insertAce(acl.getEntries().length, permission, sid, false); // not granting } else { - acl.insertAce(null, permission, sid, true); // granting + acl.insertAce(acl.getEntries().length, permission, sid, true); // granting } // Finally, persist the modified ACL diff --git a/samples/dms/src/main/java/sample/dms/secured/SecureDocumentDaoImpl.java b/samples/dms/src/main/java/sample/dms/secured/SecureDocumentDaoImpl.java index 1d33b0072e..f79b95e6c9 100755 --- a/samples/dms/src/main/java/sample/dms/secured/SecureDocumentDaoImpl.java +++ b/samples/dms/src/main/java/sample/dms/secured/SecureDocumentDaoImpl.java @@ -54,7 +54,7 @@ public class SecureDocumentDaoImpl extends DocumentDaoImpl implements SecureDocu MutableAcl aclParent = (MutableAcl) mutableAclService.readAclById(parentIdentity); acl.setParent(aclParent); } - acl.insertAce(null, BasePermission.ADMINISTRATION, new PrincipalSid(SecurityContextHolder.getContext().getAuthentication()), true); + acl.insertAce(acl.getEntries().length, BasePermission.ADMINISTRATION, new PrincipalSid(SecurityContextHolder.getContext().getAuthentication()), true); mutableAclService.updateAcl(acl); }