You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by an...@apache.org on 2022/02/25 09:31:36 UTC

[sling-org-apache-sling-jcr-repoinit] 01/01: SLING-11160 : Repoinit does not allow to remove individual ACEs (jcr impl)

This is an automated email from the ASF dual-hosted git repository.

angela pushed a commit to branch SLING-11160
in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-jcr-repoinit.git

commit bc1c71585235c79fb5619a847d3d4060d54a9037
Author: angela <an...@adobe.com>
AuthorDate: Fri Feb 25 10:31:18 2022 +0100

    SLING-11160 : Repoinit does not allow to remove individual ACEs (jcr impl)
---
 pom.xml                                            |   2 +-
 .../apache/sling/jcr/repoinit/impl/AclUtil.java    | 133 ++++++++++++++++-----
 .../apache/sling/jcr/repoinit/impl/AclVisitor.java |   8 +-
 .../sling/jcr/repoinit/PrincipalBasedAclTest.java  | 120 +++++++++----------
 .../org/apache/sling/jcr/repoinit/RemoveTest.java  |  77 ++++++++++--
 5 files changed, 236 insertions(+), 104 deletions(-)

diff --git a/pom.xml b/pom.xml
index 1b7ea45..159c7f0 100644
--- a/pom.xml
+++ b/pom.xml
@@ -225,7 +225,7 @@
         <dependency>
             <groupId>org.apache.sling</groupId>
             <artifactId>org.apache.sling.repoinit.parser</artifactId>
-            <version>1.6.11-SNAPSHOT</version>
+            <version>1.6.13-SNAPSHOT</version>
             <scope>provided</scope>
         </dependency>
         <dependency>
diff --git a/src/main/java/org/apache/sling/jcr/repoinit/impl/AclUtil.java b/src/main/java/org/apache/sling/jcr/repoinit/impl/AclUtil.java
index 8ff754a..a0d0d12 100644
--- a/src/main/java/org/apache/sling/jcr/repoinit/impl/AclUtil.java
+++ b/src/main/java/org/apache/sling/jcr/repoinit/impl/AclUtil.java
@@ -16,27 +16,6 @@
  */
 package org.apache.sling.jcr.repoinit.impl;
 
-import java.security.Principal;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
-
-import javax.jcr.PathNotFoundException;
-import javax.jcr.RepositoryException;
-import javax.jcr.Session;
-import javax.jcr.Value;
-import javax.jcr.ValueFactory;
-import javax.jcr.security.AccessControlEntry;
-import javax.jcr.security.AccessControlException;
-import javax.jcr.security.AccessControlManager;
-import javax.jcr.security.AccessControlPolicy;
-import javax.jcr.security.Privilege;
-
 import org.apache.jackrabbit.api.security.JackrabbitAccessControlEntry;
 import org.apache.jackrabbit.api.security.JackrabbitAccessControlList;
 import org.apache.jackrabbit.api.security.JackrabbitAccessControlManager;
@@ -53,6 +32,27 @@ import org.jetbrains.annotations.Nullable;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import javax.jcr.PathNotFoundException;
+import javax.jcr.RepositoryException;
+import javax.jcr.Session;
+import javax.jcr.Value;
+import javax.jcr.ValueFactory;
+import javax.jcr.security.AccessControlEntry;
+import javax.jcr.security.AccessControlException;
+import javax.jcr.security.AccessControlManager;
+import javax.jcr.security.AccessControlPolicy;
+import javax.jcr.security.Privilege;
+import java.security.Principal;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.function.Predicate;
+
 import static org.apache.sling.repoinit.parser.operations.AclLine.ID_DELIMINATOR;
 import static org.apache.sling.repoinit.parser.operations.AclLine.PATH_HOME;
 import static org.apache.sling.repoinit.parser.operations.AclLine.PATH_REPOSITORY;
@@ -238,6 +238,42 @@ public class AclUtil {
         }
     }
 
+    public static void removeEntries(@NotNull Session session, @NotNull List<String> principals, @NotNull List<String> paths, List<String> privileges, boolean isAllow, List<RestrictionClause> restrictionClauses) throws RepositoryException {
+        Set<String> principalNames = new HashSet<>(principals);
+        for (String jcrPath : getJcrPaths(session, paths)) {
+            if (jcrPath != null && !session.nodeExists(jcrPath)) {
+                LOG.info("Cannot remove access control entries on non-existent path {}", jcrPath);
+            } else {
+                JackrabbitAccessControlList acl = AccessControlUtils.getAccessControlList(session, jcrPath);
+                if (acl != null) {
+                    boolean modified = false;
+
+                    LocalRestrictions restr = createLocalRestrictions(restrictionClauses, acl, session);
+                    Privilege[] privs = AccessControlUtils.privilegesFromNames(session, privileges.toArray(new String[0]));
+                    
+                    for (AccessControlEntry ace : acl.getAccessControlEntries()) {
+                        Principal principal = ace.getPrincipal();
+                        if (!principalNames.contains(principal.getName())) {
+                            continue;
+                        }
+                        LocalAccessControlEntry entry = new LocalAccessControlEntry(ace.getPrincipal(), privs, isAllow, restr);
+                        if (entry.isEqual(ace)) {
+                            acl.removeAccessControlEntry(ace);
+                            modified = true;
+                        }
+                    }
+                    if (modified) {
+                        session.getAccessControlManager().setPolicy(jcrPath, acl);
+                    } else {
+                        LOG.info("No matching access control entry found to remove for principals {} at {}. Expected entry with isAllow={}, privileges={}, restrictions={}", principalNames, jcrPath, isAllow, privileges, restrictionClauses);
+                    }
+                } else {
+                    LOG.info("Cannot remove access control entries for principal(s) {}. No ACL at {}", principalNames, jcrPath);
+                }
+            }
+        }
+    }
+
     public static void setPrincipalAcl(Session session, String principalName, Collection<AclLine> lines) throws RepositoryException {
         final JackrabbitAccessControlManager acMgr = getJACM(session);
         Principal principal = AccessControlUtils.getPrincipal(session, principalName);
@@ -253,17 +289,34 @@ public class AclUtil {
         boolean modified = false;
         for (AclLine line : lines) {
             AclLine.Action action = line.getAction();
-            if (action == AclLine.Action.DENY) {
+            if (!line.isAllow()) {
                 throw new AccessControlException("PrincipalAccessControlList doesn't support 'deny' entries.");
-            } else if (action == AclLine.Action.REMOVE) {
-                throw new RuntimeException(AclLine.Action.REMOVE + " is not supported");
+            } 
+            
+            List<String> jcrPaths = getJcrPaths(session, line.getProperty(PROP_PATHS));
+            if (action == AclLine.Action.REMOVE) {
+                LocalRestrictions restr = createLocalRestrictions(line.getRestrictions(), acl, session);
+                List<String> privNames = line.getProperty(PROP_PRIVILEGES);
+                Privilege[] privs = AccessControlUtils.privilegesFromNames(session, privNames.toArray(new String[0]));
+                Predicate<PrincipalAccessControlList.Entry> predicate = entry -> {
+                    if (!jcrPaths.contains(entry.getEffectivePath())) {
+                        return false;
+                    }
+                    LocalAccessControlEntry lace = new LocalAccessControlEntry(entry.getPrincipal(), privs, line.isAllow(), restr);
+                    return lace.isEqual(entry);
+                };
+                if (removePrincipalEntries(acl, principalName, predicate)) {
+                    modified = true;
+                } else {
+                    LOG.info("No matching access control entry found to remove for principal {} at {}. Expected entry with isAllow={}, privileges={}, restrictions={}", principalName, jcrPaths, line.isAllow(), privNames, line.getRestrictions());
+                }
             } else if (action == AclLine.Action.REMOVE_ALL) {
-                if(removePrincipalEntries(acl, principalName, getJcrPaths(session, line.getProperty(PROP_PATHS)))) {
+                if (removePrincipalEntries(acl, principalName, entry -> jcrPaths.contains(entry.getEffectivePath()))) {
                     modified = true;
                 }
             } else if (action == AclLine.Action.ALLOW) {
                 final Privilege[] privileges = AccessControlUtils.privilegesFromNames(session, line.getProperty(PROP_PRIVILEGES).toArray(new String[0]));
-                for (String effectivePath : getJcrPaths(session, line.getProperty(PROP_PATHS))) {
+                for (String effectivePath : jcrPaths) {
                     if (acl == null) {
                         // no PrincipalAccessControlList available: don't fail if an equivalent path-based entry with the same definition exists
                         // or if there exists no node at the effective path (unable to evaluate path-based entries).
@@ -335,15 +388,15 @@ public class AclUtil {
         return acl;
     }
 
-    private static boolean removePrincipalEntries(@Nullable PrincipalAccessControlList acl, @NotNull String principalName, @NotNull List<String> paths) throws RepositoryException {
+    private static boolean removePrincipalEntries(@Nullable PrincipalAccessControlList acl, @NotNull String principalName, @NotNull Predicate<PrincipalAccessControlList.Entry> predicate) throws RepositoryException {
         boolean modified = false;
         if (acl == null) {
-            LOG.info("Cannot remove entries for paths(s) {}. No principal-based ACL for {}", paths, principalName);
+            LOG.info("Cannot remove entries. No principal-based ACL for {}", principalName);
         } else {
             for (AccessControlEntry ace : acl.getAccessControlEntries()) {
                 if (ace instanceof PrincipalAccessControlList.Entry) {
                     PrincipalAccessControlList.Entry entry = (PrincipalAccessControlList.Entry) ace;
-                    if (paths.contains(entry.getEffectivePath())) {
+                    if (predicate.test(entry)) {
                         acl.removeAccessControlEntry(ace);
                         modified = true;
                     }
@@ -473,6 +526,22 @@ public class AclUtil {
                     other.isAllow() == isAllow &&
                     sameRestrictions(other);
         }
+
+        public boolean isEqual(AccessControlEntry other) {
+            if (!(other instanceof JackrabbitAccessControlEntry)) {
+                return false;
+            }
+            try {
+                JackrabbitAccessControlEntry otherAce = (JackrabbitAccessControlEntry) other;
+                return other.getPrincipal().equals(principal) &&
+                        equalPrivileges(other.getPrivileges(), privileges) &&
+                        otherAce.isAllow() == isAllow &&
+                        sameRestrictions(otherAce);
+            } catch (RepositoryException e) {
+                throw new RuntimeException("Cannot verify equivalence of access control entries", e);
+            }
+        }
+        
         private Set<Privilege> expandPrivileges(Privilege[] privileges){
             Set<Privilege> expandedSet = new HashSet<>();
 
@@ -526,6 +595,12 @@ public class AclUtil {
             return set1.containsAll(set2);
         }
 
+        private boolean equalPrivileges(Privilege[] first, Privilege[] second) {
+            Set<Privilege> set1 = expandPrivileges(first);
+            Set<Privilege> set2 = expandPrivileges(second);
+            return set1.equals(set2);
+        }
+
         @Override
         public String toString() {
             return "[" + getClass().getSimpleName() + "# principal " + principal+ ", privileges: " + Arrays.toString(privileges) + ", isAllow : " + isAllow + "]";
diff --git a/src/main/java/org/apache/sling/jcr/repoinit/impl/AclVisitor.java b/src/main/java/org/apache/sling/jcr/repoinit/impl/AclVisitor.java
index 25e24b4..bc62456 100644
--- a/src/main/java/org/apache/sling/jcr/repoinit/impl/AclVisitor.java
+++ b/src/main/java/org/apache/sling/jcr/repoinit/impl/AclVisitor.java
@@ -72,11 +72,11 @@ class AclVisitor extends DoNothingVisitor {
     private void setAcl(AclLine line, Session s, List<String> principals, List<String> paths, List<String> privileges, AclLine.Action action) {
         try {
             if (action == AclLine.Action.REMOVE) {
-                report("remove not supported");
+                AclUtil.removeEntries(s, principals, paths, privileges, line.isAllow(), line.getRestrictions());
             } else if (action == AclLine.Action.REMOVE_ALL) {
                 AclUtil.removeEntries(s, principals, paths);
             } else {
-                final boolean isAllow = line.getAction().equals(AclLine.Action.ALLOW);
+                final boolean isAllow = line.isAllow();
                 log.info("Adding ACL '{}' entry '{}' for {} on {}", isAllow ? "allow" : "deny", privileges, principals, paths);
                 List<RestrictionClause> restrictions = line.getRestrictions();
                 AclUtil.setAcl(s, principals, paths, privileges, isAllow, restrictions);
@@ -89,11 +89,11 @@ class AclVisitor extends DoNothingVisitor {
     private void setRepositoryAcl(AclLine line, Session s, List<String> principals, List<String> privileges, AclLine.Action action) {
         try {
             if (action == AclLine.Action.REMOVE) {
-                report("remove not supported");
+                AclUtil.removeEntries(s, principals, Collections.singletonList(null), privileges, line.isAllow(), line.getRestrictions());
             } else if (action == AclLine.Action.REMOVE_ALL) {
                 AclUtil.removeEntries(s, principals, Collections.singletonList(null));
             } else {
-                final boolean isAllow = line.getAction().equals(AclLine.Action.ALLOW);
+                final boolean isAllow = line.isAllow();
                 log.info("Adding repository level ACL '{}' entry '{}' for {}", isAllow ? "allow" : "deny", privileges, principals);
                 List<RestrictionClause> restrictions = line.getRestrictions();
                 AclUtil.setRepositoryAcl(s, principals, privileges, isAllow, restrictions);
diff --git a/src/test/java/org/apache/sling/jcr/repoinit/PrincipalBasedAclTest.java b/src/test/java/org/apache/sling/jcr/repoinit/PrincipalBasedAclTest.java
index b9aaab0..2205f75 100644
--- a/src/test/java/org/apache/sling/jcr/repoinit/PrincipalBasedAclTest.java
+++ b/src/test/java/org/apache/sling/jcr/repoinit/PrincipalBasedAclTest.java
@@ -39,6 +39,7 @@ import org.apache.jackrabbit.oak.spi.security.principal.SystemUserPrincipal;
 import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration;
 import org.apache.jackrabbit.oak.spi.security.user.UserConstants;
 import org.apache.sling.jcr.repoinit.impl.AclUtil;
+import org.apache.sling.jcr.repoinit.impl.RepoInitException;
 import org.apache.sling.jcr.repoinit.impl.TestUtil;
 import org.apache.sling.repoinit.parser.RepoInitParsingException;
 import org.apache.sling.repoinit.parser.operations.AclLine;
@@ -71,7 +72,6 @@ import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
 
 public class PrincipalBasedAclTest {
 
@@ -181,6 +181,14 @@ public class PrincipalBasedAclTest {
     private static void assertRegex(String regex, String shouldMatch) {
         assertTrue("Expecting '" +  shouldMatch + "'' to match " + regex, shouldMatch.matches(regex));
     }
+    
+    @NotNull
+    private static PrincipalAccessControlList assertPolicy(@NotNull Principal principal, @NotNull Session session,  int expectedSize) throws RepositoryException {
+        PrincipalAccessControlList acl = getAcl(principal, session);
+        assertNotNull(acl);
+        assertEquals(expectedSize, acl.size());
+        return acl;
+    }
 
     private Authorizable getServiceUser(@NotNull String uid) throws RepositoryException {
         UserManager uMgr = adminSession.getUserManager();
@@ -251,7 +259,7 @@ public class PrincipalBasedAclTest {
         assertPermission(testSession, path + "/newchild", permissions, true);
     }
 
-    @Test(expected = RuntimeException.class)
+    @Test(expected = RepoInitException.class)
     public void invalidPrivilege() throws Exception {
         String setup =
                 "set principal ACL for " + U.username + "\n"
@@ -260,7 +268,7 @@ public class PrincipalBasedAclTest {
         U.parseAndExecute(setup);
     }
 
-    @Test(expected = RuntimeException.class)
+    @Test(expected = RepoInitException.class)
     public void denyEntry() throws Exception  {
         String setup =
                 "set principal ACL for " + U.username + "\n"
@@ -357,7 +365,7 @@ public class PrincipalBasedAclTest {
         assertPermission(testSession, propPath, Session.ACTION_READ, false);
     }
 
-    @Test(expected = RuntimeException.class)
+    @Test(expected = RepoInitException.class)
     public void unsupportedRestriction() throws Exception {
         String setup =
                 "set principal ACL for " + U.username + "\n"
@@ -665,55 +673,64 @@ public class PrincipalBasedAclTest {
         }
     }
 
-    @Test(expected = RuntimeException.class)
+    @Test
     public void testRemoveNoExistingPolicy() throws Exception {
         String setup = "set principal ACL for " + U.username + "\n"
-                + "remove jcr:read on " + path + "\n"
+                + "remove allow jcr:read on " + path + "\n"
                 + "end";
         U.parseAndExecute(setup);
     }
 
     @Test
     public void testRemoveMatchingEntry() throws Exception {
+        Principal principal = getPrincipal(U.username);
         String setup = "set principal ACL for " + U.username + "\n"
                 + "allow jcr:write on "+path+"\n"
                 + "end";
         U.parseAndExecute(setup);
+        assertPolicy(principal, U.adminSession, 1);
 
+        // privilege mismatch
         setup = "set principal ACL for " + U.username + "\n"
-                + "remove jcr:write on " + path + "\n"
+                + "remove allow jcr:read,jcr:write on " + path + "\n"
                 + "end";
+        U.parseAndExecute(setup);
+        assertPolicy(principal, U.adminSession, 1);
 
-        try {
-            U.parseAndExecute(setup);
-            fail("Expecting REMOVE to fail");
-        } catch(RuntimeException rex) {
-            assertRegex(REMOVE_NOT_SUPPORTED_REGEX, rex.getMessage());
-        }
+        // path mismatch
+        setup = "set principal ACL for " + U.username + "\n"
+                + "remove allow jcr:write on " + path + "/mismatch\n"
+                + "end";
+        U.parseAndExecute(setup);
+        assertPolicy(principal, U.adminSession, 1);
+
+        // restriction mismatch
+        setup = "set principal ACL for " + U.username + "\n"
+                + "remove allow jcr:write on " + path + " restriction(rep:glob, /*/jcr:content/*)\n"
+                + "end";
+        U.parseAndExecute(setup);
+        assertPolicy(principal, U.adminSession, 1);
     }
 
     @Test
     public void testRemoveNoMatchingEntry() throws Exception {
+        Principal principal = getPrincipal(U.username);
         String setup = "set principal ACL for " + U.username + "\n"
                 + "allow jcr:write on "+path+"\n"
                 + "end";
         U.parseAndExecute(setup);
+        assertPolicy(principal, U.adminSession, 1);
 
         setup = "set principal ACL for " + U.username + "\n"
-                + "remove jcr:read on " + path + "\n"
+                + "remove allow jcr:read on " + path + "\n"
                 + "end";
-        try {
-            U.parseAndExecute(setup);
-            fail("Expecting REMOVE to fail");
-        } catch(RuntimeException rex) {
-            assertRegex(REMOVE_NOT_SUPPORTED_REGEX, rex.getMessage());
-        }      
+        assertPolicy(principal, U.adminSession, 1);
     }
 
-    @Test(expected = RuntimeException.class)
+    @Test(expected = RepoInitException.class)
     public void testRemoveNonExistingPrincipal() throws Exception {
         String setup = "set principal ACL for nonExistingPrincipal\n"
-                + "remove jcr:write on " + path + "\n"
+                + "remove deny jcr:write on " + path + "\n"
                 + "end";
         U.parseAndExecute(setup);
     }
@@ -725,16 +742,16 @@ public class PrincipalBasedAclTest {
                 + "end";
         U.parseAndExecute(setup);
         U.parseAndExecute("create service user otherSystemPrincipal");
+        assertPolicy(getPrincipal(U.username), U.adminSession, 1);
 
-        try {
-            setup = "set principal ACL for otherSystemPrincipal\n"
-            + "remove jcr:write on " + path + "\n"
-            + "end";
-            U.parseAndExecute(setup);
-            fail("Expecting REMOVE to fail");
-        } catch(RuntimeException rex) {
-            assertRegex(REMOVE_NOT_SUPPORTED_REGEX, rex.getMessage());
-        }
+        setup = "set principal ACL for otherSystemPrincipal\n"
+                + "remove allow jcr:write on " + path + "\n"
+                + "end";
+        U.parseAndExecute(setup);
+
+        // ace must not have been removed
+        assertPolicy(getPrincipal(U.username), U.adminSession, 1);
+        assertNull(getAcl(getPrincipal("otherSystemPrincipal"), U.adminSession));
     }
 
     @Test
@@ -744,10 +761,11 @@ public class PrincipalBasedAclTest {
                 + "end";
         U.parseAndExecute(setup);
 
+        // removal must be ignored and no policy must be created
         assertNull(getAcl(getPrincipal(U.username), U.adminSession));
     }
 
-    @Test(expected = RuntimeException.class)
+    @Test(expected = RepoInitException.class)
     public void testAllRemoveNonExistingPrincipal() throws Exception {
         String setup = "set principal ACL for nonExistingPrincipal\n"
                 + "remove * on " + path + "\n"
@@ -768,9 +786,7 @@ public class PrincipalBasedAclTest {
                 + "end";
         U.parseAndExecute(setup);
 
-        PrincipalAccessControlList acl = getAcl(getPrincipal(U.username), U.adminSession);
-        assertNotNull(acl);
-        assertTrue(acl.isEmpty());
+        assertPolicy(getPrincipal(U.username), U.adminSession, 0);
     }
 
     @Test
@@ -781,18 +797,14 @@ public class PrincipalBasedAclTest {
                 + "end";
         U.parseAndExecute(setup);
 
-        PrincipalAccessControlList acl = getAcl(getPrincipal(U.username), U.adminSession);
-        assertNotNull(acl);
-        assertEquals(2, acl.size());
+        assertPolicy(getPrincipal(U.username), U.adminSession, 2);
 
         setup = "set principal ACL for " + U.username + "\n"
                 + "remove * on :repository\n"
                 + "end";
         U.parseAndExecute(setup);
 
-        acl = getAcl(getPrincipal(U.username), U.adminSession);
-        assertNotNull(acl);
-        assertEquals(1, acl.size());
+        assertPolicy(getPrincipal(U.username), U.adminSession, 1);
     }
 
     @Test
@@ -803,18 +815,14 @@ public class PrincipalBasedAclTest {
                 + "end";
         U.parseAndExecute(setup);
 
-        PrincipalAccessControlList acl = getAcl(getPrincipal(U.username), U.adminSession);
-        assertNotNull(acl);
-        assertEquals(2, acl.size());
+        assertPolicy(getPrincipal(U.username), U.adminSession, 2);
 
         setup = "set principal ACL for " + U.username + "\n"
                 + "remove * on " + path + "\n"
                 + "end";
         U.parseAndExecute(setup);
 
-        acl = getAcl(getPrincipal(U.username), U.adminSession);
-        assertNotNull(acl);
-        assertEquals(1, acl.size());
+        assertPolicy(getPrincipal(U.username), U.adminSession, 1);
     }
 
     @Test
@@ -825,18 +833,14 @@ public class PrincipalBasedAclTest {
                 + "end";
         U.parseAndExecute(setup);
 
-        PrincipalAccessControlList acl = getAcl(getPrincipal(U.username), U.adminSession);
-        assertNotNull(acl);
-        assertEquals(2, acl.size());
+        assertPolicy(getPrincipal(U.username), U.adminSession, 2);
 
         setup = "set principal ACL for " + U.username + "\n"
                 + "remove * on " + path + ", home("+U.username+")\n"
                 + "end";
         U.parseAndExecute(setup);
 
-        acl = getAcl(getPrincipal(U.username), U.adminSession);
-        assertNotNull(acl);
-        assertTrue(acl.isEmpty());
+        assertPolicy(getPrincipal(U.username), U.adminSession, 0);
     }
 
     @Test
@@ -852,9 +856,7 @@ public class PrincipalBasedAclTest {
                 + "end";
         U.parseAndExecute(setup);
 
-        PrincipalAccessControlList acl = getAcl(getPrincipal(U.username), U.adminSession);
-        assertNotNull(acl);
-        assertEquals(2, acl.size());
+        assertPolicy(getPrincipal(U.username), U.adminSession, 2);
     }
 
     @Test
@@ -885,11 +887,7 @@ public class PrincipalBasedAclTest {
             + "end\n"
         );
 
-        {
-            PrincipalAccessControlList acl = getAcl(getPrincipal(U.username), U.adminSession);
-            assertNotNull(acl);
-            assertEquals(0, acl.size());
-        }
+        assertPolicy(getPrincipal(U.username), U.adminSession, 0);
     }
 
     @Test
diff --git a/src/test/java/org/apache/sling/jcr/repoinit/RemoveTest.java b/src/test/java/org/apache/sling/jcr/repoinit/RemoveTest.java
index e3c87af..a1940d4 100644
--- a/src/test/java/org/apache/sling/jcr/repoinit/RemoveTest.java
+++ b/src/test/java/org/apache/sling/jcr/repoinit/RemoveTest.java
@@ -76,7 +76,7 @@ public class RemoveTest {
                 + "end\n"
                 + "\n"
                 + "set ACL for " + groupPrincipalName + "\n"
-                + "allow jcr:read on "+path+"\n"
+                + "allow jcr:read on "+path+" restriction(rep:glob,/*/foo/*)\n"
                 + "allow jcr:namespaceManagement on :repository\n"
                 + "allow jcr:read on home(" + U.username + ")\n"
                 + "end";
@@ -147,27 +147,86 @@ public class RemoveTest {
         assertPolicy(userHomePath, U.adminSession, 2);
     }
 
-    @Test(expected = RuntimeException.class)
+    @Test
     public void testRemoveByPath() throws RepoInitParsingException, RepositoryException {
+        // non-matching ACE (path-mismatch) -> not removed (and no exception)
         String setup = "set ACL for " + U.username + "\n"
-                + "remove jcr:read on "+path+"\n"
+                + "remove deny jcr:read on /\n"
+                + "end";
+        U.parseAndExecute(setup);
+        assertPolicy(path, U.adminSession, 2);
+
+        // non-matching ACE (privilege-mismatch) -> not removed (and no exception)
+        setup = "set ACL for " + U.username + "\n"
+                + "remove deny jcr:read,jcr:write on "+path+"\n"
                 + "end";
         U.parseAndExecute(setup);
+        assertPolicy(path, U.adminSession, 2);
+
+        // matching ACE -> removed
+        setup = "set ACL for " + U.username + "\n"
+                + "remove deny jcr:read on "+path+"\n"
+                + "end";
+        U.parseAndExecute(setup);
+        assertPolicy(path, U.adminSession, 1);
     }
 
-    @Test(expected = RuntimeException.class)
-    public void testRemoveByPath2() throws RepoInitParsingException, RepositoryException {
+    @Test
+    public void testRemoveByRepository() throws RepoInitParsingException, RepositoryException {
+        // non-matching ACE (allow mismatch) -> not removed (and no exception)
+        String setup = "set repository ACL for " + groupPrincipalName + "\n"
+                + "remove deny jcr:namespaceManagement\n"
+                + "end";
+        U.parseAndExecute(setup);
+        assertPolicy(null, U.adminSession, 2);
+
+        // matching ACE -> removed
+        setup = "set repository ACL for " + groupPrincipalName + "\n"
+                + "remove allow jcr:namespaceManagement\n"
+                + "end";
+        U.parseAndExecute(setup);
+        assertPolicy(null, U.adminSession, 1);
+    }
+
+    @Test
+    public void testRemoveByPrincipalRepositoryPath() throws RepoInitParsingException, RepositoryException {
+        // non-matching ACE (privilege mismatch) -> not removed (and no exception)
         String setup = "set ACL for " + groupPrincipalName + "\n"
-                + "remove jcr:versionManagement on :repository\n"
+                + "remove allow jcr:versionManagement on :repository\n"
                 + "end";
         U.parseAndExecute(setup);
+        assertPolicy(null, U.adminSession, 2);
+
+        // matching ACE -> removed
+        setup = "set ACL for " + groupPrincipalName + "\n"
+                + "remove allow jcr:namespaceManagement on :repository\n"
+                + "end";
+        U.parseAndExecute(setup);
+        assertPolicy(null, U.adminSession, 1);
     }
 
-    @Test(expected = RuntimeException.class)
-    public void testRemoveByPrincipal() throws RepoInitParsingException, RepositoryException {
+    @Test
+    public void testRemoveByHomePath() throws RepoInitParsingException, RepositoryException {
+        // no-matching ACE (restriction mismatch) -> not removed
         String setup = "set ACL on home("+U.username+")\n"
-                + "remove jcr:all for "+U.username+"\n" +
+                + "remove allow jcr:read for "+U.username+" restriction(rep:itemNames, prop1)\n" +
+                "end";
+        U.parseAndExecute(setup);
+        assertPolicy(userHomePath, U.adminSession, 2);
+        
+        setup = "set ACL on home("+U.username+")\n"
+                + "remove allow jcr:read for "+U.username+"\n" +
                 "end";
         U.parseAndExecute(setup);
+        assertPolicy(userHomePath, U.adminSession, 1);
+    }
+    
+    @Test
+    public void testRemoveEntryWithRestriction() throws Exception {
+        String setup = "set ACL for " + groupPrincipalName + "\n"
+                + "remove allow jcr:read on "+path+" restriction(rep:glob, /*/foo/*)\n"
+                + "end";
+        U.parseAndExecute(setup);
+        assertPolicy(path, U.adminSession, 1);
     }
 }
\ No newline at end of file