From acd7dc1f2d6040d90a5d769493a539f5fe9eb5a9 Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Fri, 8 May 2009 03:10:25 +0000 Subject: [PATCH] SEC-1151: Fixed check on ACE list bounds in AclImpl and updated tests --- .../security/acls/domain/AclImpl.java | 5 +- .../security/acls/domain/AclImplTests.java | 185 ++++++++++-------- 2 files changed, 111 insertions(+), 79 deletions(-) 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 0c263b17f3..de336e841c 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 @@ -127,8 +127,9 @@ public class AclImpl implements Acl, MutableAcl, AuditableAcl, OwnershipAcl { 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"); + if (aceIndex >= this.aces.size()) { + throw new NotFoundException("aceIndex must refer to an index of the AccessControlEntry list. " + + "List size is " + aces.size() + ", index was " + aceIndex); } } 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 fe07e06cd2..de2abada4f 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 @@ -1,6 +1,7 @@ package org.springframework.security.acls.domain; import static org.junit.Assert.*; +import static org.mockito.Mockito.mock; import java.lang.reflect.Field; import java.util.ArrayList; @@ -8,7 +9,6 @@ import java.util.Arrays; import java.util.List; import java.util.Map; -import org.jmock.Expectations; import org.jmock.Mockery; import org.junit.After; import org.junit.Before; @@ -51,23 +51,19 @@ public class AclImplTests { private static final List SCOTT = Arrays.asList((Sid)new PrincipalSid("scott")); private static final List BEN = Arrays.asList((Sid)new PrincipalSid("ben")); - Authentication auth = new TestingAuthenticationToken("johndoe", "ignored", "ROLE_ADMINISTRATOR"); + Authentication auth = new TestingAuthenticationToken("joe", "ignored", "ROLE_ADMINISTRATOR"); Mockery jmockCtx = new Mockery(); AclAuthorizationStrategy mockAuthzStrategy; AuditLogger mockAuditLogger; - ObjectIdentity objectIdentity = new ObjectIdentityImpl(TARGET_CLASS, new Long(100)); + ObjectIdentity objectIdentity = new ObjectIdentityImpl(TARGET_CLASS, 100); // ~ Methods ======================================================================================================== @Before public void setUp() throws Exception { SecurityContextHolder.getContext().setAuthentication(auth); - mockAuthzStrategy = jmockCtx.mock(AclAuthorizationStrategy.class); - mockAuditLogger = jmockCtx.mock(AuditLogger.class);; - jmockCtx.checking(new Expectations() {{ - ignoring(mockAuthzStrategy); - ignoring(mockAuditLogger); - }}); + mockAuthzStrategy = mock(AclAuthorizationStrategy.class); + mockAuditLogger = mock(AuditLogger.class);; auth.setAuthenticated(true); } @@ -77,20 +73,20 @@ public class AclImplTests { } @Test(expected=IllegalArgumentException.class) - public void testConstructorsRejectNullObjectIdentity() throws Exception { + public void constructorsRejectNullObjectIdentity() throws Exception { try { - new AclImpl(null, new Long(1), mockAuthzStrategy, mockAuditLogger, null, null, true, new PrincipalSid("johndoe")); + new AclImpl(null, 1, mockAuthzStrategy, mockAuditLogger, null, null, true, new PrincipalSid("joe")); fail("Should have thrown IllegalArgumentException"); } catch (IllegalArgumentException expected) { } - new AclImpl(null, new Long(1), mockAuthzStrategy, mockAuditLogger); + new AclImpl(null, 1, mockAuthzStrategy, mockAuditLogger); } @Test(expected=IllegalArgumentException.class) - public void testConstructorsRejectNullId() throws Exception { + public void constructorsRejectNullId() throws Exception { try { - new AclImpl(objectIdentity, null, mockAuthzStrategy, mockAuditLogger, null, null, true, new PrincipalSid("johndoe")); + new AclImpl(objectIdentity, null, mockAuthzStrategy, mockAuditLogger, null, null, true, new PrincipalSid("joe")); fail("Should have thrown IllegalArgumentException"); } catch (IllegalArgumentException expected) { @@ -99,31 +95,31 @@ public class AclImplTests { } @Test(expected=IllegalArgumentException.class) - public void testConstructorsRejectNullAclAuthzStrategy() throws Exception { + public void constructorsRejectNullAclAuthzStrategy() throws Exception { try { - new AclImpl(objectIdentity, new Long(1), null, mockAuditLogger, null, null, true, new PrincipalSid("johndoe")); + new AclImpl(objectIdentity, 1, null, mockAuditLogger, null, null, true, new PrincipalSid("joe")); fail("It should have thrown IllegalArgumentException"); } catch (IllegalArgumentException expected) { } - new AclImpl(objectIdentity, new Long(1), null, mockAuditLogger); + new AclImpl(objectIdentity, 1, null, mockAuditLogger); } @Test(expected=IllegalArgumentException.class) - public void testConstructorsRejectNullAuditLogger() throws Exception { + public void constructorsRejectNullAuditLogger() throws Exception { try { - new AclImpl(objectIdentity, new Long(1), mockAuthzStrategy, null, null, null, true, new PrincipalSid("johndoe")); + new AclImpl(objectIdentity, 1, mockAuthzStrategy, null, null, null, true, new PrincipalSid("joe")); fail("It should have thrown IllegalArgumentException"); } catch (IllegalArgumentException expected) { } - new AclImpl(objectIdentity, new Long(1), mockAuthzStrategy, null); + new AclImpl(objectIdentity, 1, mockAuthzStrategy, null); } @Test - public void testInsertAceRejectsNullParameters() throws Exception { - MutableAcl acl = new AclImpl(objectIdentity, new Long(1), mockAuthzStrategy, mockAuditLogger, null, null, true, new PrincipalSid( - "johndoe")); + public void insertAceRejectsNullParameters() throws Exception { + MutableAcl acl = new AclImpl(objectIdentity, 1, mockAuthzStrategy, mockAuditLogger, null, null, true, new PrincipalSid( + "joe")); try { acl.insertAce(0, null, new GrantedAuthoritySid("ROLE_IGNORED"), true); fail("It should have thrown IllegalArgumentException"); @@ -139,8 +135,8 @@ public class AclImplTests { } @Test - public void testInsertAceAddsElementAtCorrectIndex() throws Exception { - MutableAcl acl = new AclImpl(objectIdentity, new Long(1), mockAuthzStrategy, mockAuditLogger, null, null, true, new PrincipalSid("johndoe")); + public void insertAceAddsElementAtCorrectIndex() throws Exception { + MutableAcl acl = new AclImpl(objectIdentity, 1, mockAuthzStrategy, mockAuditLogger, null, null, true, new PrincipalSid("joe")); MockAclService service = new MockAclService(); // Insert one permission @@ -175,9 +171,9 @@ public class AclImplTests { } @Test(expected=NotFoundException.class) - public void testInsertAceFailsForInexistentElement() throws Exception { - MutableAcl acl = new AclImpl(objectIdentity, new Long(1), mockAuthzStrategy, mockAuditLogger, null, null, true, new PrincipalSid( - "johndoe")); + public void insertAceFailsForNonExistentElement() throws Exception { + MutableAcl acl = new AclImpl(objectIdentity,1, mockAuthzStrategy, mockAuditLogger, null, null, true, new PrincipalSid( + "joe")); MockAclService service = new MockAclService(); // Insert one permission @@ -188,9 +184,9 @@ public class AclImplTests { } @Test - public void testDeleteAceKeepsInitialOrdering() throws Exception { - MutableAcl acl = new AclImpl(objectIdentity, new Long(1), mockAuthzStrategy, mockAuditLogger, null, null, true, new PrincipalSid( - "johndoe")); + public void deleteAceKeepsInitialOrdering() throws Exception { + MutableAcl acl = new AclImpl(objectIdentity,1, mockAuthzStrategy, mockAuditLogger, null, null, true, new PrincipalSid( + "joe")); MockAclService service = new MockAclService(); // Add several permissions @@ -220,13 +216,13 @@ public class AclImplTests { } @Test - public void testDeleteAceFailsForInexistentElement() throws Exception { + public void deleteAceFailsForNonExistentElement() throws Exception { AclAuthorizationStrategyImpl strategy = new AclAuthorizationStrategyImpl(new GrantedAuthority[] { new GrantedAuthorityImpl("ROLE_OWNERSHIP"), new GrantedAuthorityImpl("ROLE_AUDITING"), new GrantedAuthorityImpl("ROLE_GENERAL") }); AuditLogger auditLogger = new ConsoleAuditLogger(); - MutableAcl acl = new AclImpl(objectIdentity, new Long(1), strategy, auditLogger, null, null, true, new PrincipalSid( - "johndoe")); + MutableAcl acl = new AclImpl(objectIdentity, (1), strategy, auditLogger, null, null, true, new PrincipalSid( + "joe")); try { acl.deleteAce(99); fail("It should have thrown NotFoundException"); @@ -236,9 +232,9 @@ public class AclImplTests { } @Test - public void testIsGrantingRejectsEmptyParameters() throws Exception { - MutableAcl acl = new AclImpl(objectIdentity, new Long(1), mockAuthzStrategy, mockAuditLogger, null, null, true, new PrincipalSid( - "johndoe")); + public void isGrantingRejectsEmptyParameters() throws Exception { + MutableAcl acl = new AclImpl(objectIdentity, 1, mockAuthzStrategy, mockAuditLogger, null, null, true, new PrincipalSid( + "joe")); Sid ben = new PrincipalSid("ben"); try { acl.isGranted(new ArrayList(0), Arrays.asList(ben) , false); @@ -255,15 +251,15 @@ public class AclImplTests { } @Test - public void testIsGrantingGrantsAccessForAclWithNoParent() throws Exception { + public void isGrantingGrantsAccessForAclWithNoParent() throws Exception { Authentication auth = new TestingAuthenticationToken("ben", "ignored", "ROLE_GENERAL","ROLE_GUEST"); auth.setAuthenticated(true); SecurityContextHolder.getContext().setAuthentication(auth); - ObjectIdentity rootOid = new ObjectIdentityImpl(TARGET_CLASS, new Long(100)); + ObjectIdentity rootOid = new ObjectIdentityImpl(TARGET_CLASS, 100); // Create an ACL which owner is not the authenticated principal - MutableAcl rootAcl = new AclImpl(rootOid, new Long(1), mockAuthzStrategy, mockAuditLogger, null, null, false, new PrincipalSid( - "johndoe")); + MutableAcl rootAcl = new AclImpl(rootOid, 1, mockAuthzStrategy, mockAuditLogger, null, null, false, new PrincipalSid( + "joe")); // Grant some permissions rootAcl.insertAce(0, BasePermission.READ, new PrincipalSid("ben"), false); @@ -295,27 +291,27 @@ public class AclImplTests { } @Test - public void testIsGrantingGrantsAccessForInheritableAcls() throws Exception { + public void isGrantingGrantsAccessForInheritableAcls() throws Exception { Authentication auth = new TestingAuthenticationToken("ben", "ignored","ROLE_GENERAL"); auth.setAuthenticated(true); SecurityContextHolder.getContext().setAuthentication(auth); - ObjectIdentity grandParentOid = new ObjectIdentityImpl(TARGET_CLASS, new Long(100)); - ObjectIdentity parentOid1 = new ObjectIdentityImpl(TARGET_CLASS, new Long(101)); - ObjectIdentity parentOid2 = new ObjectIdentityImpl(TARGET_CLASS, new Long(102)); - ObjectIdentity childOid1 = new ObjectIdentityImpl(TARGET_CLASS, new Long(103)); - ObjectIdentity childOid2 = new ObjectIdentityImpl(TARGET_CLASS, new Long(104)); + ObjectIdentity grandParentOid = new ObjectIdentityImpl(TARGET_CLASS, 100); + ObjectIdentity parentOid1 = new ObjectIdentityImpl(TARGET_CLASS, 101); + ObjectIdentity parentOid2 = new ObjectIdentityImpl(TARGET_CLASS, 102); + ObjectIdentity childOid1 = new ObjectIdentityImpl(TARGET_CLASS, 103); + ObjectIdentity childOid2 = new ObjectIdentityImpl(TARGET_CLASS, 104); // Create ACLs - MutableAcl grandParentAcl = new AclImpl(grandParentOid, new Long(1), mockAuthzStrategy, mockAuditLogger, null, null, false, - new PrincipalSid("johndoe")); - MutableAcl parentAcl1 = new AclImpl(parentOid1, new Long(2), mockAuthzStrategy, mockAuditLogger, null, null, true, - new PrincipalSid("johndoe")); - MutableAcl parentAcl2 = new AclImpl(parentOid2, new Long(3), mockAuthzStrategy, mockAuditLogger, null, null, true, - new PrincipalSid("johndoe")); - MutableAcl childAcl1 = new AclImpl(childOid1, new Long(4), mockAuthzStrategy, mockAuditLogger, null, null, true, - new PrincipalSid("johndoe")); - MutableAcl childAcl2 = new AclImpl(childOid2, new Long(4), mockAuthzStrategy, mockAuditLogger, null, null, false, - new PrincipalSid("johndoe")); + MutableAcl grandParentAcl = new AclImpl(grandParentOid, 1, mockAuthzStrategy, mockAuditLogger, null, null, false, + new PrincipalSid("joe")); + MutableAcl parentAcl1 = new AclImpl(parentOid1, 2, mockAuthzStrategy, mockAuditLogger, null, null, true, + new PrincipalSid("joe")); + MutableAcl parentAcl2 = new AclImpl(parentOid2, 3, mockAuthzStrategy, mockAuditLogger, null, null, true, + new PrincipalSid("joe")); + MutableAcl childAcl1 = new AclImpl(childOid1, 4, mockAuthzStrategy, mockAuditLogger, null, null, true, + new PrincipalSid("joe")); + MutableAcl childAcl2 = new AclImpl(childOid2, 4, mockAuthzStrategy, mockAuditLogger, null, null, false, + new PrincipalSid("joe")); // Create hierarchies childAcl2.setParent(childAcl1); @@ -360,7 +356,7 @@ public class AclImplTests { assertTrue(true); } try { - assertTrue(childAcl2.isGranted(CREATE, Arrays.asList((Sid)new PrincipalSid("johndoe")), false)); + assertTrue(childAcl2.isGranted(CREATE, Arrays.asList((Sid)new PrincipalSid("joe")), false)); fail("It should have thrown NotFoundException"); } catch (NotFoundException expected) { @@ -369,12 +365,12 @@ public class AclImplTests { } @Test - public void testUpdateAce() throws Exception { + public void updatedAceValuesAreCorrectlyReflectedInAcl() throws Exception { Authentication auth = new TestingAuthenticationToken("ben", "ignored","ROLE_GENERAL"); auth.setAuthenticated(true); SecurityContextHolder.getContext().setAuthentication(auth); - MutableAcl acl = new AclImpl(objectIdentity, new Long(1), mockAuthzStrategy, mockAuditLogger, null, null, false, new PrincipalSid( - "johndoe")); + MutableAcl acl = new AclImpl(objectIdentity, 1, mockAuthzStrategy, mockAuditLogger, null, null, false, new PrincipalSid( + "joe")); MockAclService service = new MockAclService(); acl.insertAce(0, BasePermission.READ, new GrantedAuthoritySid("ROLE_USER_READ"), true); @@ -398,12 +394,12 @@ public class AclImplTests { } @Test - public void testUpdateAuditing() throws Exception { + public void auditableEntryFlagsAreUpdatedCorrectly() throws Exception { Authentication auth = new TestingAuthenticationToken("ben", "ignored", "ROLE_AUDITING", "ROLE_GENERAL"); auth.setAuthenticated(true); SecurityContextHolder.getContext().setAuthentication(auth); - MutableAcl acl = new AclImpl(objectIdentity, new Long(1), mockAuthzStrategy, mockAuditLogger, null, null, false, new PrincipalSid( - "johndoe")); + MutableAcl acl = new AclImpl(objectIdentity, 1, mockAuthzStrategy, mockAuditLogger, null, null, false, new PrincipalSid( + "joe")); MockAclService service = new MockAclService(); acl.insertAce(0, BasePermission.READ, new GrantedAuthoritySid("ROLE_USER_READ"), true); @@ -427,25 +423,25 @@ public class AclImplTests { } @Test - public void testGettersSetters() throws Exception { + public void gettersAndSettersAreConsistent() throws Exception { Authentication auth = new TestingAuthenticationToken("ben", "ignored", new GrantedAuthority[] { new GrantedAuthorityImpl("ROLE_GENERAL") }); auth.setAuthenticated(true); SecurityContextHolder.getContext().setAuthentication(auth); - ObjectIdentity identity = new ObjectIdentityImpl(TARGET_CLASS, new Long(100)); - ObjectIdentity identity2 = new ObjectIdentityImpl(TARGET_CLASS, new Long(101)); - MutableAcl acl = new AclImpl(identity, new Long(1), mockAuthzStrategy, mockAuditLogger, null, null, true, new PrincipalSid( - "johndoe")); - MutableAcl parentAcl = new AclImpl(identity2, new Long(2), mockAuthzStrategy, mockAuditLogger, null, null, true, new PrincipalSid( - "johndoe")); + ObjectIdentity identity = new ObjectIdentityImpl(TARGET_CLASS, (100)); + ObjectIdentity identity2 = new ObjectIdentityImpl(TARGET_CLASS, (101)); + MutableAcl acl = new AclImpl(identity, 1, mockAuthzStrategy, mockAuditLogger, null, null, true, + new PrincipalSid("joe")); + MutableAcl parentAcl = new AclImpl(identity2, 2, mockAuthzStrategy, mockAuditLogger, null, null, true, + new PrincipalSid("joe")); MockAclService service = new MockAclService(); 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)); + assertEquals(acl.getId(), 1); assertEquals(acl.getObjectIdentity(), identity); - assertEquals(acl.getOwner(), new PrincipalSid("johndoe")); + assertEquals(acl.getOwner(), new PrincipalSid("joe")); assertNull(acl.getParentAcl()); assertTrue(acl.isEntriesInheriting()); assertEquals(2, acl.getEntries().size()); @@ -461,10 +457,10 @@ public class AclImplTests { } @Test - public void testIsSidLoaded() throws Exception { + public void isSidLoadedBehavesAsExpected() throws Exception { List loadedSids = Arrays.asList(new PrincipalSid("ben"), new GrantedAuthoritySid("ROLE_IGNORED")); - MutableAcl acl = new AclImpl(objectIdentity, new Long(1), mockAuthzStrategy, mockAuditLogger, null, loadedSids, true, new PrincipalSid( - "johndoe")); + MutableAcl acl = new AclImpl(objectIdentity, 1, mockAuthzStrategy, mockAuditLogger, null, loadedSids, true, + new PrincipalSid("joe")); assertTrue(acl.isSidLoaded(loadedSids)); assertTrue(acl.isSidLoaded(Arrays.asList(new GrantedAuthoritySid("ROLE_IGNORED"), new PrincipalSid("ben")))); @@ -477,6 +473,41 @@ public class AclImplTests { assertFalse(acl.isSidLoaded(Arrays.asList((Sid)new GrantedAuthoritySid("ROLE_IGNORED"), new GrantedAuthoritySid("ROLE_GENERAL")))); } + @Test(expected=NotFoundException.class) + public void insertAceRaisesNotFoundExceptionForIndexLessThanZero() throws Exception { + AclImpl acl = new AclImpl(objectIdentity, 1, mockAuthzStrategy, mockAuditLogger, null, null, true, + new PrincipalSid("joe")); + acl.insertAce(-1, mock(Permission.class), mock(Sid.class), true); + } + + @Test(expected=NotFoundException.class) + public void deleteAceRaisesNotFoundExceptionForIndexLessThanZero() throws Exception { + AclImpl acl = new AclImpl(objectIdentity, 1, mockAuthzStrategy, mockAuditLogger, null, null, true, + new PrincipalSid("joe")); + acl.deleteAce(-1); + } + + @Test(expected=NotFoundException.class) + public void insertAceRaisesNotFoundExceptionForIndexGreaterThanSize() throws Exception { + AclImpl acl = new AclImpl(objectIdentity, 1, mockAuthzStrategy, mockAuditLogger, null, null, true, + new PrincipalSid("joe")); + // Insert at zero, OK. + acl.insertAce(0, mock(Permission.class), mock(Sid.class), true); + // Size is now 1 + acl.insertAce(2, mock(Permission.class), mock(Sid.class), true); + } + + // SEC-1151 + @Test(expected=NotFoundException.class) + public void deleteAceRaisesNotFoundExceptionForIndexEqualToSize() throws Exception { + AclImpl acl = new AclImpl(objectIdentity, 1, mockAuthzStrategy, mockAuditLogger, null, null, true, + new PrincipalSid("joe")); + acl.insertAce(0, mock(Permission.class), mock(Sid.class), true); + // Size is now 1 + acl.deleteAce(1); + } + + //~ Inner Classes ================================================================================================== private class MockAclService implements MutableAclService { @@ -504,7 +535,7 @@ public class AclImplTests { for (int i = 0; i < oldAces.size(); i++) { AccessControlEntry ac = oldAces.get(i); // Just give an ID to all this acl's aces, rest of the fields are just copied - newAces.add(new AccessControlEntryImpl(new Long(i + 1), ac.getAcl(), ac.getSid(), ac.getPermission(), ac + newAces.add(new AccessControlEntryImpl((i + 1), ac.getAcl(), ac.getSid(), ac.getPermission(), ac .isGranting(), ((AuditableAccessControlEntry) ac).isAuditSuccess(), ((AuditableAccessControlEntry) ac).isAuditFailure())); }