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\" }");