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:35 UTC

[sling-org-apache-sling-jcr-repoinit] branch SLING-11160 created (now bc1c715)

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

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


      at bc1c715  SLING-11160 : Repoinit does not allow to remove individual ACEs (jcr impl)

This branch includes the following new commits:

     new bc1c715  SLING-11160 : Repoinit does not allow to remove individual ACEs (jcr impl)

The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.


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

Posted by an...@apache.org.
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