SLING-11245 : AclUtil.removePolicies(Session, List<String> paths) will fail if no policy exists
SLING-11246 : Avoid repeated calls to Session.getAccessControlManager in AclUtil
SLING-11249 : Missing test for removing entries at non-existing path
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 407c8a7..cc8dd9f 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
@@ -129,10 +129,12 @@
private static void setAcl(Session session, List<String> principals, String jcrPath, List<String> privileges, boolean isAllow, List<RestrictionClause> restrictionClauses)
throws RepositoryException {
- final String [] privArray = privileges.toArray(new String[privileges.size()]);
- final Privilege[] jcrPriv = AccessControlUtils.privilegesFromNames(session, privArray);
+ AccessControlManager acMgr = session.getAccessControlManager();
- JackrabbitAccessControlList acl = AccessControlUtils.getAccessControlList(session, jcrPath);
+ final String [] privArray = privileges.toArray(new String[privileges.size()]);
+ final Privilege[] jcrPriv = AccessControlUtils.privilegesFromNames(acMgr, privArray);
+
+ JackrabbitAccessControlList acl = getAccessControlList(acMgr, jcrPath, true);
checkState(acl != null, "No JackrabbitAccessControlList available for path " + jcrPath);
LocalRestrictions localRestrictions = createLocalRestrictions(restrictionClauses, acl, session);
@@ -159,7 +161,7 @@
changed = true;
}
if ( changed ) {
- session.getAccessControlManager().setPolicy(jcrPath, acl);
+ acMgr.setPolicy(jcrPath, acl);
}
}
@@ -202,24 +204,30 @@
* @throws RepositoryException
*/
public static void removePolicies(@NotNull Session session, @NotNull List<String> paths) throws RepositoryException {
+ AccessControlManager acMgr = session.getAccessControlManager();
for (String jcrPath : getJcrPaths(session, paths)) {
- LOG.info("Removing access control policy on {}", jcrPath);
- JackrabbitAccessControlList acl = AccessControlUtils.getAccessControlList(session, jcrPath);
+ if (!isValidPath(session, jcrPath)) {
+ LOG.info("Cannot remove ACL; no node at {} ", jcrPath);
+ continue;
+ }
+ LOG.info("Removing access control policy at {}", jcrPath);
+ JackrabbitAccessControlList acl = getAccessControlList(acMgr, jcrPath, false);
if (acl == null) {
LOG.info("No ACL to remove at path {}", jcrPath);
} else {
- session.getAccessControlManager().removePolicy(jcrPath, acl);
+ acMgr.removePolicy(jcrPath, acl);
}
}
}
public static void removeEntries(@NotNull Session session, @NotNull List<String> principals, @NotNull List<String> paths) throws RepositoryException {
Set<String> principalNames = new HashSet<>(principals);
+ AccessControlManager acMgr = session.getAccessControlManager();
for (String jcrPath : getJcrPaths(session, paths)) {
- if (jcrPath != null && !session.nodeExists(jcrPath)) {
+ if (!isValidPath(session, jcrPath)) {
LOG.info("Cannot remove access control entries on non-existent path {}", jcrPath);
} else {
- JackrabbitAccessControlList acl = AccessControlUtils.getAccessControlList(session, jcrPath);
+ JackrabbitAccessControlList acl = getAccessControlList(acMgr, jcrPath, false);
if (acl != null) {
boolean modified = false;
for (AccessControlEntry ace : acl.getAccessControlEntries()) {
@@ -229,7 +237,7 @@
}
}
if (modified) {
- session.getAccessControlManager().setPolicy(jcrPath, acl);
+ acMgr.setPolicy(jcrPath, acl);
}
} else {
LOG.info("Cannot remove access control entries for principal(s) {}. No ACL at {}", principalNames, jcrPath);
@@ -240,16 +248,17 @@
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);
+ AccessControlManager acMgr = session.getAccessControlManager();
for (String jcrPath : getJcrPaths(session, paths)) {
- if (jcrPath != null && !session.nodeExists(jcrPath)) {
+ if (!isValidPath(session, jcrPath)) {
LOG.info("Cannot remove access control entries on non-existent path {}", jcrPath);
} else {
- JackrabbitAccessControlList acl = AccessControlUtils.getAccessControlList(session, jcrPath);
+ JackrabbitAccessControlList acl = getAccessControlList(acMgr, jcrPath, false);
if (acl != null) {
boolean modified = false;
LocalRestrictions restr = createLocalRestrictions(restrictionClauses, acl, session);
- Privilege[] privs = AccessControlUtils.privilegesFromNames(session, privileges.toArray(new String[0]));
+ Privilege[] privs = AccessControlUtils.privilegesFromNames(acMgr, privileges.toArray(new String[0]));
for (AccessControlEntry ace : acl.getAccessControlEntries()) {
Principal principal = ace.getPrincipal();
@@ -263,7 +272,7 @@
}
}
if (modified) {
- session.getAccessControlManager().setPolicy(jcrPath, acl);
+ acMgr.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);
}
@@ -299,7 +308,7 @@
modified = true;
}
} else if (action == AclLine.Action.ALLOW) {
- final Privilege[] privileges = AccessControlUtils.privilegesFromNames(session, line.getProperty(PROP_PRIVILEGES).toArray(new String[0]));
+ final Privilege[] privileges = AccessControlUtils.privilegesFromNames(acMgr, line.getProperty(PROP_PRIVILEGES).toArray(new String[0]));
for (String effectivePath : jcrPaths) {
if (acl == null) {
// no PrincipalAccessControlList available: don't fail if an equivalent path-based entry with the same definition exists
@@ -345,7 +354,7 @@
List<String> jcrPaths = getJcrPaths(session, line.getProperty(PROP_PATHS));
LocalRestrictions restr = createLocalRestrictions(line.getRestrictions(), acl, session);
List<String> privNames = line.getProperty(PROP_PRIVILEGES);
- Privilege[] privs = AccessControlUtils.privilegesFromNames(session, privNames.toArray(new String[0]));
+ Privilege[] privs = AccessControlUtils.privilegesFromNames(acMgr, privNames.toArray(new String[0]));
Predicate<PrincipalAccessControlList.Entry> predicate = entry -> {
if (!jcrPaths.contains(entry.getEffectivePath())) {
return false;
@@ -386,6 +395,25 @@
acMgr.removePolicy(acl.getPath(), acl);
}
}
+
+ private static boolean isValidPath(@NotNull Session session, @Nullable String jcrPath) throws RepositoryException {
+ return jcrPath == null || session.nodeExists(jcrPath);
+ }
+
+ @Nullable
+ private static JackrabbitAccessControlList getAccessControlList(@NotNull AccessControlManager acMgr,
+ @Nullable String path, boolean includeApplicable) throws RepositoryException {
+ if (includeApplicable) {
+ return AccessControlUtils.getAccessControlList(acMgr, path);
+ } else {
+ for (AccessControlPolicy policy : acMgr.getPolicies(path)) {
+ if (policy instanceof JackrabbitAccessControlList) {
+ return (JackrabbitAccessControlList) policy;
+ }
+ }
+ return null;
+ }
+ }
@Nullable
private static PrincipalAccessControlList getPrincipalAccessControlList(@NotNull JackrabbitAccessControlManager acMgr,
diff --git a/src/test/java/org/apache/sling/jcr/repoinit/DeleteAclTest.java b/src/test/java/org/apache/sling/jcr/repoinit/DeleteAclTest.java
index 306e63f..ed19535 100644
--- a/src/test/java/org/apache/sling/jcr/repoinit/DeleteAclTest.java
+++ b/src/test/java/org/apache/sling/jcr/repoinit/DeleteAclTest.java
@@ -50,7 +50,9 @@
import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
@RunWith(Parameterized.class)
public class DeleteAclTest {
@@ -94,6 +96,7 @@
U.parseAndExecute(""
+ "create path (nt:unstructured) /content\n"
+ "create path (nt:unstructured) /var\n"
+ + "create path (nt:unstructured) /no/policy\n"
+ "set ACL for " + U.username + "\n"
+ "allow jcr:read on /content, /var, home("+userA+")\n"
+ "allow jcr:namespaceManagement on :repository\n"
@@ -131,9 +134,22 @@
assertArrayEquals(new AccessControlPolicy[0], acMgr.getPolicies((String) null));
}
- @Test(expected = RuntimeException.class)
+ @Test
public void testDeleteAclNonExistingPath() throws Exception {
+ assertFalse(adminSession.nodeExists("/nonExisting"));
U.parseAndExecute("delete ACL on /nonExisting\n");
+ assertFalse(adminSession.nodeExists("/nonExisting"));
+ }
+
+ @Test
+ public void testDeleteNonExistingAcl() throws Exception {
+ assertTrue(adminSession.nodeExists("/no/policy"));
+ assertArrayEquals(new AccessControlPolicy[0], acMgr.getPolicies("/no/policy"));
+
+ U.parseAndExecute("delete ACL on /no/policy\n");
+
+ assertTrue(adminSession.nodeExists("/no/policy"));
+ assertArrayEquals(new AccessControlPolicy[0], acMgr.getPolicies("/no/policy"));
}
@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 aa13145..cdce414 100644
--- a/src/test/java/org/apache/sling/jcr/repoinit/RemoveTest.java
+++ b/src/test/java/org/apache/sling/jcr/repoinit/RemoveTest.java
@@ -41,6 +41,7 @@
import java.util.UUID;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
import static org.junit.Assert.fail;
public class RemoveTest {
@@ -170,6 +171,28 @@
"end";
U.parseAndExecute(setup);
}
+
+ @Test
+ public void testRemoveByNonExistingPath() throws Exception {
+ String path = "/non/existing";
+ String setup = "remove ACE for " + U.username + "\n"
+ + "deny jcr:read on "+path+"\n"
+ + "end";
+ U.parseAndExecute(setup);
+ assertFalse(U.adminSession.nodeExists(path));
+
+ setup = "remove ACE on "+path+"\n"
+ + "deny jcr:read for " + U.username + "\n"
+ + "end";
+ U.parseAndExecute(setup);
+ assertFalse(U.adminSession.nodeExists(path));
+
+ setup = "set ACL on "+path+"\n" +
+ "remove * for "+U.username+"\n" +
+ "end";
+ U.parseAndExecute(setup);
+ assertFalse(U.adminSession.nodeExists(path));
+ }
@Test
public void testRemoveByPath() throws RepoInitParsingException, RepositoryException {
