SLING-11243 modifyace post a second time reverses the entry order (#11)
The entries for the principal get reversed on the second update which can make a more specific ACE not get used
diff --git a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/AbstractAccessGetServlet.java b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/AbstractAccessGetServlet.java
index a78a38c..752baff 100644
--- a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/AbstractAccessGetServlet.java
+++ b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/AbstractAccessGetServlet.java
@@ -19,15 +19,25 @@
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.security.Principal;
+import java.util.ArrayList;
import java.util.Arrays;
+import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
+import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.Set;
+import java.util.TreeMap;
+import java.util.function.Predicate;
+import java.util.stream.Stream;
import javax.jcr.AccessDeniedException;
import javax.jcr.RepositoryException;
import javax.jcr.Session;
+import javax.jcr.security.AccessControlEntry;
+import javax.jcr.security.AccessControlList;
+import javax.jcr.security.AccessControlPolicy;
import javax.jcr.security.Privilege;
import javax.json.Json;
import javax.json.JsonObject;
@@ -36,6 +46,8 @@
import javax.servlet.http.HttpServletResponse;
import org.apache.jackrabbit.api.security.JackrabbitAccessControlEntry;
+import org.apache.jackrabbit.api.security.JackrabbitAccessControlList;
+import org.apache.jackrabbit.api.security.authorization.PrincipalAccessControlList;
import org.apache.jackrabbit.api.security.principal.PrincipalManager;
import org.apache.jackrabbit.oak.spi.security.authorization.restriction.RestrictionDefinition;
import org.apache.jackrabbit.oak.spi.security.authorization.restriction.RestrictionProvider;
@@ -46,6 +58,7 @@
import org.apache.sling.jcr.base.util.AccessControlUtil;
import org.apache.sling.jcr.jackrabbit.accessmanager.LocalPrivilege;
import org.apache.sling.jcr.jackrabbit.accessmanager.LocalRestriction;
+import org.apache.sling.jcr.jackrabbit.accessmanager.impl.PrincipalAceHelper;
import org.apache.sling.jcr.jackrabbit.accessmanager.impl.PrivilegesHelper;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
@@ -199,4 +212,48 @@
}
}
+ /**
+ * Builds a map by merging all the entries for the supplied
+ * policies and ordering them by the effective path
+ *
+ * @param policies the policies to process
+ * @param accessControlEntryFilter a filter to find entries to include
+ * @param declaredAtPaths populated with details about where privileges are defined for the principal.
+ * In the map the key is the principal and the value is a map of paths the set of defined ACE
+ * types at that path.
+ * @return map of sorted entries, key is the effectivePath and value is the list of entries for that path
+ */
+ protected @NotNull Map<String, List<AccessControlEntry>> entriesSortedByEffectivePath(@NotNull AccessControlPolicy[] policies,
+ @NotNull Predicate<? super AccessControlEntry> accessControlEntryFilter) throws RepositoryException {
+ Comparator<? super String> effectivePathComparator = (k1, k2) -> Objects.compare(k1, k2, Comparator.nullsFirst(String::compareTo));
+ Map<String, List<AccessControlEntry>> effectivePathToEntriesMap = new TreeMap<>(effectivePathComparator);
+
+ // map the effectivePaths to the entries for that path
+ for (AccessControlPolicy accessControlPolicy : policies) {
+ AccessControlEntry[] accessControlEntries = ((AccessControlList)accessControlPolicy).getAccessControlEntries();
+ if (accessControlPolicy instanceof AccessControlList) {
+ Stream.of(accessControlEntries)
+ .filter(accessControlEntryFilter)
+ .forEach(entry -> {
+ String effectivePath = null;
+ if (entry instanceof PrincipalAccessControlList.Entry) {
+ // for principal-based ACE, the effectivePath comes from the entry
+ effectivePath = ((PrincipalAccessControlList.Entry)entry).getEffectivePath();
+ if (effectivePath == null) {
+ // special case
+ effectivePath = PrincipalAceHelper.RESOURCE_PATH_REPOSITORY;
+ }
+ } else if (accessControlPolicy instanceof JackrabbitAccessControlList) {
+ // for basic ACE, the effectivePath comes from the ACL path
+ effectivePath = ((JackrabbitAccessControlList)accessControlPolicy).getPath();
+ }
+ List<AccessControlEntry> entriesForPath = effectivePathToEntriesMap.computeIfAbsent(effectivePath, key -> new ArrayList<>());
+ entriesForPath.add(entry);
+ });
+ }
+ }
+
+ return effectivePathToEntriesMap;
+ }
+
}
diff --git a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/AbstractGetAceServlet.java b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/AbstractGetAceServlet.java
index a633124..4d56cad 100644
--- a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/AbstractGetAceServlet.java
+++ b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/AbstractGetAceServlet.java
@@ -18,6 +18,7 @@
import java.security.Principal;
import java.util.HashMap;
+import java.util.List;
import java.util.Map;
import java.util.Set;
@@ -49,8 +50,8 @@
protected JsonObject internalGetAce(Session jcrSession, String resourcePath, String principalId) throws RepositoryException {
Principal principal = validateArgs(jcrSession, resourcePath, principalId);
- AccessControlEntry[] accessControlEntries = getAccessControlEntries(jcrSession, resourcePath, principal);
- if (accessControlEntries == null || accessControlEntries.length == 0) {
+ Map<String, List<AccessControlEntry>> effectivePathToEntriesMap = getAccessControlEntriesMap(jcrSession, resourcePath, principal);
+ if (effectivePathToEntriesMap == null || effectivePathToEntriesMap.isEmpty()) {
throw new ResourceNotFoundException(resourcePath, "No access control entries were found");
}
@@ -62,14 +63,14 @@
}
Map<Privilege, LocalPrivilege> privilegeToLocalPrivilegesMap = new HashMap<>();
- //evaluate these in reverse order so the entries with highest specificity are processed last
- for (int i = accessControlEntries.length - 1; i >= 0; i--) {
- AccessControlEntry accessControlEntry = accessControlEntries[i];
- if (accessControlEntry instanceof JackrabbitAccessControlEntry) {
- JackrabbitAccessControlEntry jrAccessControlEntry = (JackrabbitAccessControlEntry)accessControlEntry;
- Privilege[] privileges = jrAccessControlEntry.getPrivileges();
- if (privileges != null) {
- processACE(srMap, jrAccessControlEntry, privileges, privilegeToLocalPrivilegesMap);
+ for (List<AccessControlEntry> accessControlEntries : effectivePathToEntriesMap.values()) {
+ for (AccessControlEntry accessControlEntry : accessControlEntries) {
+ if (accessControlEntry instanceof JackrabbitAccessControlEntry) {
+ JackrabbitAccessControlEntry jrAccessControlEntry = (JackrabbitAccessControlEntry)accessControlEntry;
+ Privilege[] privileges = jrAccessControlEntry.getPrivileges();
+ if (privileges != null) {
+ processACE(srMap, jrAccessControlEntry, privileges, privilegeToLocalPrivilegesMap);
+ }
}
}
}
@@ -84,6 +85,15 @@
return jsonObj.build();
}
- protected abstract AccessControlEntry[] getAccessControlEntries(Session session, String absPath, Principal principal) throws RepositoryException;
+ protected abstract Map<String, List<AccessControlEntry>> getAccessControlEntriesMap(Session session, String absPath, Principal principal) throws RepositoryException;
+
+ /**
+ * @deprecated use {@link #getAccessControlEntriesMap(Session, String, Principal, Map)} instead
+ */
+ @Deprecated
+ protected AccessControlEntry[] getAccessControlEntries(Session session, String absPath, Principal principal) throws RepositoryException {
+ return getAccessControlEntriesMap(session, absPath, principal).values().stream()
+ .toArray(size -> new AccessControlEntry[size]);
+ }
}
diff --git a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/AbstractGetAclServlet.java b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/AbstractGetAclServlet.java
index 449be89..a51902f 100644
--- a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/AbstractGetAclServlet.java
+++ b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/AbstractGetAclServlet.java
@@ -79,21 +79,24 @@
srMap.put(restrictionDefinition.getName(), restrictionDefinition);
}
- AccessControlEntry[] accessControlEntries = getAccessControlEntries(jcrSession, resourcePath);
+ Map<String, List<AccessControlEntry>> effectivePathToEntriesMap = getAccessControlEntriesMap(jcrSession, resourcePath);
Map<Principal, Integer> principalToOrderMap = new HashMap<>();
Map<Principal, Map<Privilege, LocalPrivilege>> principalToPrivilegesMap = new HashMap<>();
- //evaluate these in reverse order so the entries with highest specificity are processed last
- for (int i = accessControlEntries.length - 1; i >= 0; i--) {
- AccessControlEntry accessControlEntry = accessControlEntries[i];
- if (accessControlEntry instanceof JackrabbitAccessControlEntry) {
- JackrabbitAccessControlEntry jrAccessControlEntry = (JackrabbitAccessControlEntry)accessControlEntry;
- Privilege[] privileges = jrAccessControlEntry.getPrivileges();
- if (privileges != null) {
- Principal principal = accessControlEntry.getPrincipal();
- principalToOrderMap.put(principal, i);
- Map<Privilege, LocalPrivilege> map = principalToPrivilegesMap.computeIfAbsent(principal, k -> new HashMap<>());
+ for (Entry<String, List<AccessControlEntry>> entry : effectivePathToEntriesMap.entrySet()) {
+ List<AccessControlEntry> accessControlEntries = entry.getValue();
+ for (AccessControlEntry accessControlEntry : accessControlEntries) {
+ if (accessControlEntry instanceof JackrabbitAccessControlEntry) {
+ JackrabbitAccessControlEntry jrAccessControlEntry = (JackrabbitAccessControlEntry)accessControlEntry;
+ Privilege[] privileges = jrAccessControlEntry.getPrivileges();
+ if (privileges != null) {
+ Principal principal = accessControlEntry.getPrincipal();
+ if (!principalToPrivilegesMap.containsKey(principal)) {
+ principalToOrderMap.put(principal, principalToPrivilegesMap.size());
+ }
+ Map<Privilege, LocalPrivilege> map = principalToPrivilegesMap.computeIfAbsent(principal, k -> new HashMap<>());
- processACE(srMap, jrAccessControlEntry, privileges, map);
+ processACE(srMap, jrAccessControlEntry, privileges, map);
+ }
}
}
}
@@ -151,6 +154,15 @@
return JsonConvert.addTo(builder, value);
}
- protected abstract AccessControlEntry[] getAccessControlEntries(Session session, String absPath) throws RepositoryException;
+ protected abstract Map<String, List<AccessControlEntry>> getAccessControlEntriesMap(Session session, String absPath) throws RepositoryException;
+
+ /**
+ * @deprecated use {@link #getAccessControlEntriesMap(Session, String, Map)} instead
+ */
+ @Deprecated
+ protected AccessControlEntry[] getAccessControlEntries(Session session, String absPath) throws RepositoryException {
+ return getAccessControlEntriesMap(session, absPath).values().stream()
+ .toArray(size -> new AccessControlEntry[size]);
+ }
}
diff --git a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/GetAceServlet.java b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/GetAceServlet.java
index 27e1547..19edf38 100644
--- a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/GetAceServlet.java
+++ b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/GetAceServlet.java
@@ -19,21 +19,18 @@
package org.apache.sling.jcr.jackrabbit.accessmanager.post;
import java.security.Principal;
-import java.util.ArrayList;
import java.util.List;
-import java.util.stream.Stream;
+import java.util.Map;
import javax.jcr.RepositoryException;
import javax.jcr.Session;
import javax.jcr.security.AccessControlEntry;
-import javax.jcr.security.AccessControlList;
import javax.jcr.security.AccessControlManager;
import javax.jcr.security.AccessControlPolicy;
import javax.json.JsonObject;
import javax.servlet.Servlet;
import org.apache.jackrabbit.oak.spi.security.authorization.restriction.RestrictionProvider;
-import org.apache.sling.jcr.base.util.AccessControlUtil;
import org.apache.sling.jcr.jackrabbit.accessmanager.GetAce;
import org.osgi.service.component.annotations.Component;
import org.osgi.service.component.annotations.Reference;
@@ -95,19 +92,11 @@
}
@Override
- protected AccessControlEntry[] getAccessControlEntries(Session session, String absPath, Principal principal) throws RepositoryException {
- AccessControlManager acMgr = AccessControlUtil.getAccessControlManager(session);
+ protected Map<String, List<AccessControlEntry>> getAccessControlEntriesMap(Session session, String absPath,
+ Principal principal) throws RepositoryException {
+ AccessControlManager acMgr = session.getAccessControlManager();
AccessControlPolicy[] policies = acMgr.getPolicies(absPath);
- List<AccessControlEntry> allEntries = new ArrayList<>();
- for (AccessControlPolicy accessControlPolicy : policies) {
- if (accessControlPolicy instanceof AccessControlList) {
- AccessControlEntry[] accessControlEntries = ((AccessControlList)accessControlPolicy).getAccessControlEntries();
- Stream.of(accessControlEntries)
- .filter(entry -> principal.equals(entry.getPrincipal()))
- .forEach(allEntries::add);
- }
- }
- return allEntries.toArray(new AccessControlEntry[allEntries.size()]);
+ return entriesSortedByEffectivePath(policies, ace -> principal.equals(ace.getPrincipal()));
}
}
diff --git a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/GetAclServlet.java b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/GetAclServlet.java
index d731fe0..0017e1c 100644
--- a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/GetAclServlet.java
+++ b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/GetAclServlet.java
@@ -16,21 +16,18 @@
*/
package org.apache.sling.jcr.jackrabbit.accessmanager.post;
-import java.util.ArrayList;
-import java.util.Arrays;
import java.util.List;
+import java.util.Map;
import javax.jcr.RepositoryException;
import javax.jcr.Session;
import javax.jcr.security.AccessControlEntry;
-import javax.jcr.security.AccessControlList;
import javax.jcr.security.AccessControlManager;
import javax.jcr.security.AccessControlPolicy;
import javax.json.JsonObject;
import javax.servlet.Servlet;
import org.apache.jackrabbit.oak.spi.security.authorization.restriction.RestrictionProvider;
-import org.apache.sling.jcr.base.util.AccessControlUtil;
import org.apache.sling.jcr.jackrabbit.accessmanager.GetAcl;
import org.osgi.service.component.annotations.Component;
import org.osgi.service.component.annotations.Reference;
@@ -124,17 +121,10 @@
}
@Override
- protected AccessControlEntry[] getAccessControlEntries(Session session, String absPath) throws RepositoryException {
- AccessControlManager accessControlManager = AccessControlUtil.getAccessControlManager(session);
+ protected Map<String, List<AccessControlEntry>> getAccessControlEntriesMap(Session session, String absPath) throws RepositoryException {
+ AccessControlManager accessControlManager = session.getAccessControlManager();
AccessControlPolicy[] policies = accessControlManager.getPolicies(absPath);
- List<AccessControlEntry> allEntries = new ArrayList<>();
- for (AccessControlPolicy accessControlPolicy : policies) {
- if (accessControlPolicy instanceof AccessControlList) {
- AccessControlEntry[] accessControlEntries = ((AccessControlList)accessControlPolicy).getAccessControlEntries();
- allEntries.addAll(Arrays.asList(accessControlEntries));
- }
- }
- return allEntries.toArray(new AccessControlEntry[allEntries.size()]);
+ return entriesSortedByEffectivePath(policies, ace -> true);
}
}
diff --git a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/GetEffectiveAceServlet.java b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/GetEffectiveAceServlet.java
index 3cb9f63..d001350 100644
--- a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/GetEffectiveAceServlet.java
+++ b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/GetEffectiveAceServlet.java
@@ -19,14 +19,12 @@
package org.apache.sling.jcr.jackrabbit.accessmanager.post;
import java.security.Principal;
-import java.util.ArrayList;
import java.util.List;
-import java.util.stream.Stream;
+import java.util.Map;
import javax.jcr.RepositoryException;
import javax.jcr.Session;
import javax.jcr.security.AccessControlEntry;
-import javax.jcr.security.AccessControlList;
import javax.jcr.security.AccessControlManager;
import javax.jcr.security.AccessControlPolicy;
import javax.json.JsonObject;
@@ -95,19 +93,11 @@
}
@Override
- protected AccessControlEntry[] getAccessControlEntries(Session session, String absPath, Principal principal) throws RepositoryException {
+ protected Map<String, List<AccessControlEntry>> getAccessControlEntriesMap(Session session, String absPath,
+ Principal principal) throws RepositoryException {
AccessControlManager acMgr = AccessControlUtil.getAccessControlManager(session);
AccessControlPolicy[] policies = acMgr.getEffectivePolicies(absPath);
- List<AccessControlEntry> allEntries = new ArrayList<>();
- for (AccessControlPolicy policy : policies) {
- if (policy instanceof AccessControlList) {
- AccessControlEntry[] accessControlEntries = ((AccessControlList)policy).getAccessControlEntries();
- Stream.of(accessControlEntries)
- .filter(entry -> principal.equals(entry.getPrincipal()))
- .forEach(allEntries::add);
- }
- }
- return allEntries.toArray(new AccessControlEntry[allEntries.size()]);
+ return entriesSortedByEffectivePath(policies, ace -> principal.equals(ace.getPrincipal()));
}
}
diff --git a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/GetEffectiveAclServlet.java b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/GetEffectiveAclServlet.java
index 98f1119..f917e75 100644
--- a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/GetEffectiveAclServlet.java
+++ b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/GetEffectiveAclServlet.java
@@ -16,21 +16,18 @@
*/
package org.apache.sling.jcr.jackrabbit.accessmanager.post;
-import java.util.ArrayList;
-import java.util.Arrays;
import java.util.List;
+import java.util.Map;
import javax.jcr.RepositoryException;
import javax.jcr.Session;
import javax.jcr.security.AccessControlEntry;
-import javax.jcr.security.AccessControlList;
import javax.jcr.security.AccessControlManager;
import javax.jcr.security.AccessControlPolicy;
import javax.json.JsonObject;
import javax.servlet.Servlet;
import org.apache.jackrabbit.oak.spi.security.authorization.restriction.RestrictionProvider;
-import org.apache.sling.jcr.base.util.AccessControlUtil;
import org.apache.sling.jcr.jackrabbit.accessmanager.GetEffectiveAcl;
import org.osgi.service.component.annotations.Component;
import org.osgi.service.component.annotations.Reference;
@@ -124,17 +121,10 @@
}
@Override
- protected AccessControlEntry[] getAccessControlEntries(Session session, String absPath) throws RepositoryException {
- AccessControlManager accessControlManager = AccessControlUtil.getAccessControlManager(session);
+ protected Map<String, List<AccessControlEntry>> getAccessControlEntriesMap(Session session, String absPath) throws RepositoryException {
+ AccessControlManager accessControlManager = session.getAccessControlManager();
AccessControlPolicy[] policies = accessControlManager.getEffectivePolicies(absPath);
- List<AccessControlEntry> allEntries = new ArrayList<>();
- for (AccessControlPolicy accessControlPolicy : policies) {
- if (accessControlPolicy instanceof AccessControlList) {
- AccessControlEntry[] accessControlEntries = ((AccessControlList)accessControlPolicy).getAccessControlEntries();
- allEntries.addAll(Arrays.asList(accessControlEntries));
- }
- }
- return allEntries.toArray(new AccessControlEntry[allEntries.size()]);
+ return entriesSortedByEffectivePath(policies, ace -> true);
}
}
diff --git a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/GetPrincipalAceServlet.java b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/GetPrincipalAceServlet.java
index 2abb1b1..6dba636 100644
--- a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/GetPrincipalAceServlet.java
+++ b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/GetPrincipalAceServlet.java
@@ -19,9 +19,9 @@
package org.apache.sling.jcr.jackrabbit.accessmanager.post;
import java.security.Principal;
-import java.util.ArrayList;
+import java.util.Collections;
import java.util.List;
-import java.util.stream.Stream;
+import java.util.Map;
import javax.jcr.RepositoryException;
import javax.jcr.Session;
@@ -36,7 +36,6 @@
import org.apache.jackrabbit.api.security.authorization.PrincipalAccessControlList;
import org.apache.jackrabbit.oak.spi.security.authorization.restriction.RestrictionProvider;
import org.apache.sling.api.SlingHttpServletRequest;
-import org.apache.sling.jcr.base.util.AccessControlUtil;
import org.apache.sling.jcr.jackrabbit.accessmanager.GetPrincipalAce;
import org.apache.sling.jcr.jackrabbit.accessmanager.impl.PrincipalAceHelper;
import org.jetbrains.annotations.NotNull;
@@ -111,23 +110,16 @@
}
@Override
- protected AccessControlEntry[] getAccessControlEntries(Session session, String absPath, Principal principal) throws RepositoryException {
- List<AccessControlEntry> allEntries = new ArrayList<>();
-
- AccessControlManager acMgr = AccessControlUtil.getAccessControlManager(session);
+ protected Map<String, List<AccessControlEntry>> getAccessControlEntriesMap(Session session, String absPath,
+ Principal principal) throws RepositoryException {
+ AccessControlManager acMgr = session.getAccessControlManager();
if (acMgr instanceof JackrabbitAccessControlManager) {
JackrabbitAccessControlManager jacMgr = (JackrabbitAccessControlManager)acMgr;
- for (JackrabbitAccessControlPolicy policy : jacMgr.getPolicies(principal)) {
- if (policy instanceof PrincipalAccessControlList) {
- PrincipalAccessControlList pacl = (PrincipalAccessControlList)policy;
- Stream.of(pacl.getAccessControlEntries())
- .filter(entry -> matchesPrincipalAccessControlEntry(entry, absPath, principal))
- .forEach(allEntries::add);
- }
- }
+ JackrabbitAccessControlPolicy[] policies = jacMgr.getPolicies(principal);
+ return entriesSortedByEffectivePath(policies, ace -> matchesPrincipalAccessControlEntry(ace, absPath, principal));
+ } else {
+ return Collections.emptyMap();
}
-
- return allEntries.toArray(new AccessControlEntry[allEntries.size()]);
}
/**
diff --git a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/ModifyAceServlet.java b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/ModifyAceServlet.java
index 4989326..13abd0a 100644
--- a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/ModifyAceServlet.java
+++ b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/ModifyAceServlet.java
@@ -334,9 +334,7 @@
Map<Privilege, LocalPrivilege> privilegeToLocalPrivilegesMap = new HashMap<>();
JackrabbitAccessControlList acl = getAcl(acm, resourcePath, forPrincipal);
AccessControlEntry[] accessControlEntries = acl.getAccessControlEntries();
- //evaluate these in reverse order so the entries with highest specificity are processed last
- for (int i = accessControlEntries.length - 1; i >= 0; i--) {
- AccessControlEntry accessControlEntry = accessControlEntries[i];
+ for (AccessControlEntry accessControlEntry : accessControlEntries) {
JackrabbitAccessControlEntry jrAccessControlEntry = getJackrabbitAccessControlEntry(accessControlEntry, resourcePath, forPrincipal);
if (jrAccessControlEntry != null) {
Privilege[] privileges = jrAccessControlEntry.getPrivileges();
@@ -713,14 +711,25 @@
protected String removeAces(@NotNull String resourcePath, @Nullable String order, @NotNull Principal principal, @NotNull JackrabbitAccessControlList acl) // NOSONAR
throws RepositoryException {
AccessControlEntry[] existingAccessControlEntries = acl.getAccessControlEntries();
+
+ if (order == null || order.length() == 0) {
+ //order not specified, so keep track of the original ACE position.
+ Set<Principal> processedPrincipals = new HashSet<>();
+ for (int j = 0; j < existingAccessControlEntries.length; j++) {
+ AccessControlEntry ace = existingAccessControlEntries[j];
+ Principal principal2 = ace.getPrincipal();
+ if (principal2.equals(principal)) {
+ order = String.valueOf(processedPrincipals.size());
+ break;
+ } else {
+ processedPrincipals.add(principal2);
+ }
+ }
+ }
+
for (int j = 0; j < existingAccessControlEntries.length; j++) {
AccessControlEntry ace = existingAccessControlEntries[j];
if (ace.getPrincipal().equals(principal)) {
- if (order == null || order.length() == 0) {
- //order not specified, so keep track of the original ACE position.
- order = String.valueOf(j);
- }
-
acl.removeAccessControlEntry(ace);
}
}
@@ -740,11 +749,28 @@
protected void addAces(@NotNull String resourcePath, @NotNull Principal principal,
@NotNull Map<Set<LocalRestriction>, List<LocalPrivilege>> restrictionsToLocalPrivilegesMap,
boolean isAllow,
- @NotNull JackrabbitAccessControlList acl) throws RepositoryException {
+ @NotNull JackrabbitAccessControlList acl,
+ Map<Privilege, Integer> privilegeLongestDepthMap) throws RepositoryException {
List<Entry<Set<LocalRestriction>, List<LocalPrivilege>>> sortedEntries = new ArrayList<>(restrictionsToLocalPrivilegesMap.entrySet());
- // sort the entries to so the ACE without restrictions is last
- Collections.sort(sortedEntries, (e1, e2) -> Integer.compare(e2.getKey().size(), e1.getKey().size()));
+ // sort the entries by the most shallow depth of the contained privileges
+ Collections.sort(sortedEntries, (e1, e2) -> {
+ int shallowestDepth1 = Integer.MAX_VALUE;
+ for (LocalPrivilege lp : e1.getValue()) {
+ Integer depth = privilegeLongestDepthMap.get(lp.getPrivilege());
+ if (depth != null && depth.intValue() < shallowestDepth1) {
+ shallowestDepth1 = depth.intValue();
+ }
+ }
+ int shallowestDepth2 = Integer.MAX_VALUE;
+ for (LocalPrivilege lp : e2.getValue()) {
+ Integer depth = privilegeLongestDepthMap.get(lp.getPrivilege());
+ if (depth != null && depth.intValue() < shallowestDepth2) {
+ shallowestDepth2 = depth.intValue();
+ }
+ }
+ return Integer.compare(shallowestDepth1, shallowestDepth2);
+ });
for (Entry<Set<LocalRestriction>, List<LocalPrivilege>> entry: sortedEntries) {
Set<Privilege> privilegesSet = new HashSet<>();
@@ -852,44 +878,43 @@
throw new IllegalArgumentException("No ACE was found for the specified principal: " + afterPrincipalName);
}
} else {
+ int index = -1;
try {
- int index = Integer.parseInt(order);
- if (index > accessControlEntries.length) {
- //invalid index
- throw new IndexOutOfBoundsException("Index value is too large: " + index);
- }
-
- if (index == 0) {
- beforeEntry = accessControlEntries[0];
- } else {
- //the index value is the index of the principal. A principal may have more
- // than one ACEs (deny + grant), so we need to compensate.
- Set<Principal> processedPrincipals = new HashSet<>();
- for (int i = 0; i < accessControlEntries.length; i++) {
- Principal principal2 = accessControlEntries[i].getPrincipal();
- if (processedPrincipals.size() == index &&
- !processedPrincipals.contains(principal2)) {
- //we are now at the correct position in the list
- beforeEntry = accessControlEntries[i];
- break;
- }
-
- processedPrincipals.add(principal2);
- }
- }
+ index = Integer.parseInt(order);
} catch (NumberFormatException nfe) {
//not a number.
throw new IllegalArgumentException("Illegal value for the order parameter: " + order);
}
+ if (index > accessControlEntries.length) {
+ //invalid index
+ throw new IndexOutOfBoundsException("Index value is too large: " + index);
+ }
+
+ //the index value is the index of the principal. A principal may have more
+ // than one ACEs (deny + grant), so we need to compensate.
+ Map<Principal, Integer> principalToIndex = new HashMap<>();
+ for (int i = 0; i < accessControlEntries.length; i++) {
+ Principal principal2 = accessControlEntries[i].getPrincipal();
+ Integer idx = i;
+ principalToIndex.computeIfAbsent(principal2, key -> idx);
+ }
+ Integer[] sortedIndexes = principalToIndex.values().stream()
+ .sorted()
+ .toArray(size -> new Integer[size]);
+ if (index >= 0 && index < sortedIndexes.length - 1) {
+ int idx = sortedIndexes[index];
+ beforeEntry = accessControlEntries[idx];
+ }
}
- //now loop through the entries to move the affected ACEs to the specified
- // position.
- for (int i = accessControlEntries.length - 1; i >= 0; i--) {
- AccessControlEntry ace = accessControlEntries[i];
- if (principal.equals(ace.getPrincipal())) {
- //this ACE is for the specified principal.
- jacl.orderBefore(ace, beforeEntry);
+ if (beforeEntry != null) {
+ //now loop through the entries to move the affected ACEs to the specified
+ // position.
+ for (AccessControlEntry ace : accessControlEntries) {
+ if (principal.equals(ace.getPrincipal())) {
+ //this ACE is for the specified principal.
+ jacl.orderBefore(ace, beforeEntry);
+ }
}
}
} else {
@@ -1071,8 +1096,9 @@
order = removeAces(resourcePath, order, principal, acl);
// now add all the new aces that we have collected
- addAces(resourcePath, principal, denyRestrictionsToLocalPrivilegesMap, false, acl);
- addAces(resourcePath, principal, allowRestrictionsToLocalPrivilegesMap, true, acl);
+ Map<Privilege, Integer> privilegeLongestDepthMap = PrivilegesHelper.buildPrivilegeLongestDepthMap(acm.privilegeFromName(PrivilegeConstants.JCR_ALL));
+ addAces(resourcePath, principal, denyRestrictionsToLocalPrivilegesMap, false, acl, privilegeLongestDepthMap);
+ addAces(resourcePath, principal, allowRestrictionsToLocalPrivilegesMap, true, acl, privilegeLongestDepthMap);
// reorder the aces
reorderAccessControlEntries(acl, principal, order);
diff --git a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/ModifyPrincipalAceServlet.java b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/ModifyPrincipalAceServlet.java
index f364a84..8799ddb 100644
--- a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/ModifyPrincipalAceServlet.java
+++ b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/ModifyPrincipalAceServlet.java
@@ -28,6 +28,7 @@
import javax.jcr.security.AccessControlEntry;
import javax.jcr.security.AccessControlManager;
import javax.jcr.security.AccessControlPolicy;
+import javax.jcr.security.Privilege;
import javax.servlet.Servlet;
import org.apache.jackrabbit.api.security.JackrabbitAccessControlEntry;
@@ -211,12 +212,14 @@
@Override
protected void addAces(@NotNull String resourcePath, @NotNull Principal principal,
@NotNull Map<Set<LocalRestriction>, List<LocalPrivilege>> restrictionsToLocalPrivilegesMap, boolean isAllow,
- @NotNull JackrabbitAccessControlList acl) throws RepositoryException {
- if (!isAllow && !restrictionsToLocalPrivilegesMap.isEmpty()) {
+ @NotNull JackrabbitAccessControlList acl, Map<Privilege, Integer> privilegeLongestDepthMap)
+ throws RepositoryException {
+ if (isAllow) {
+ super.addAces(resourcePath, principal, restrictionsToLocalPrivilegesMap, isAllow, acl, privilegeLongestDepthMap);
+ } else if (!restrictionsToLocalPrivilegesMap.isEmpty()) {
// deny privileges not allowed in a principal ACE
throw new IllegalArgumentException("Deny privileges are not allowed in a principal ACE");
}
- super.addAces(resourcePath, principal, restrictionsToLocalPrivilegesMap, isAllow, acl);
}
/**
diff --git a/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/GetAceIT.java b/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/GetAceIT.java
index a3df761..0ab62a4 100644
--- a/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/GetAceIT.java
+++ b/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/GetAceIT.java
@@ -147,6 +147,15 @@
commonDeclaredAceWithLeafRestrictionForUser(1);
}
+ /**
+ * ACE servlet returns restriction details for leaf of also allowed aggregate after a second
+ * update to verify that the ordering doesn't get broken during update
+ */
+ @Test
+ public void testDeclaredAceWithLeafRestrictionForUserAfterSecondUpdate() throws IOException, JsonException {
+ commonDeclaredAceWithLeafRestrictionForUser(2);
+ }
+
protected void commonDeclaredAceWithLeafRestrictionForUser(int numberOfUpdateAceCalls) throws IOException {
testUserId = createTestUser();
testFolderUrl = createTestFolder(null, "sling-tests",
diff --git a/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/GetEaceIT.java b/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/GetEaceIT.java
index 848f2e5..9812d13 100644
--- a/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/GetEaceIT.java
+++ b/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/GetEaceIT.java
@@ -147,6 +147,15 @@
commonEffectiveAceWithLeafRestrictionForUser(1);
}
+ /**
+ * ACE servlet returns restriction details for leaf of also allowed aggregate after a second
+ * update to verify that the ordering doesn't get broken during update
+ */
+ @Test
+ public void testEffectiveAceWithLeafRestrictionForUserAfterSecondUpdate() throws IOException, JsonException {
+ commonEffectiveAceWithLeafRestrictionForUser(2);
+ }
+
protected void commonEffectiveAceWithLeafRestrictionForUser(int numberOfUpdateAceCalls) throws IOException {
testUserId = createTestUser();
testFolderUrl = createTestFolder(null, "sling-tests2",
diff --git a/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/GetPaceIT.java b/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/GetPaceIT.java
index efd6f5a..5b80ed0 100644
--- a/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/GetPaceIT.java
+++ b/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/GetPaceIT.java
@@ -141,6 +141,15 @@
commonPrivilegeAceWithLeafRestrictionForUser(1);
}
+ /**
+ * ACE servlet returns restriction details for leaf of also allowed aggregate after a second
+ * update to verify that the ordering doesn't get broken during update
+ */
+ @Test
+ public void testPrivilegeAceWithLeafRestrictionForUserAfterSecondUpdate() throws IOException, JsonException {
+ commonPrivilegeAceWithLeafRestrictionForUser(2);
+ }
+
protected void commonPrivilegeAceWithLeafRestrictionForUser(int numberOfUpdateAceCalls) throws IOException {
testFolderUrl = createTestFolder(null, "sling-tests",
"{ \"jcr:primaryType\": \"nt:unstructured\" }");