You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by an...@apache.org on 2013/06/04 19:01:11 UTC

svn commit: r1489515 - in /jackrabbit/oak/trunk/oak-core/src: main/java/org/apache/jackrabbit/oak/security/authorization/permission/ main/resources/org/apache/jackrabbit/oak/plugins/nodetype/write/ test/java/org/apache/jackrabbit/oak/security/authoriza...

Author: angela
Date: Tue Jun  4 17:00:55 2013
New Revision: 1489515

URL: http://svn.apache.org/r1489515
Log:
OAK-527: permissions (wip)

change permission store to contain a hierarchical view of the entries for a single principal instead of
keeping a flat, unstructured list. this allow to read permission entries lazily or partially depending on
the number and structure of policy nodes and entries in the content.

Modified:
    jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/permission/CompiledPermissionImpl.java
    jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/permission/PermissionConstants.java
    jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/permission/PermissionHook.java
    jackrabbit/oak/trunk/oak-core/src/main/resources/org/apache/jackrabbit/oak/plugins/nodetype/write/builtin_nodetypes.cnd
    jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/permission/CompiledPermissionImplTest.java
    jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/permission/PermissionHookTest.java

Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/permission/CompiledPermissionImpl.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/permission/CompiledPermissionImpl.java?rev=1489515&r1=1489514&r2=1489515&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/permission/CompiledPermissionImpl.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/permission/CompiledPermissionImpl.java Tue Jun  4 17:00:55 2013
@@ -16,9 +16,6 @@
  */
 package org.apache.jackrabbit.oak.security.authorization.permission;
 
-import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.base.Preconditions.checkNotNull;
-
 import java.security.Principal;
 import java.security.acl.Group;
 import java.util.Collections;
@@ -27,7 +24,6 @@ import java.util.HashSet;
 import java.util.Iterator;
 import java.util.Map;
 import java.util.Set;
-
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 
@@ -53,6 +49,9 @@ import org.apache.jackrabbit.oak.spi.sec
 import org.apache.jackrabbit.oak.util.TreeUtil;
 import org.apache.jackrabbit.util.Text;
 
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
+
 /**
  * TODO: WIP
  */
@@ -81,6 +80,8 @@ class CompiledPermissionImpl implements 
         this.bitsProvider = bitsProvider;
         this.readPaths = readPaths;
         this.trees = new HashMap<String, ImmutableTree>(principals.size());
+
+        // FIXME: load entries lazily or partially
         buildEntries(permissionsTree);
     }
 
@@ -374,7 +375,7 @@ class CompiledPermissionImpl implements 
         private PermissionEntry next;
 
         private PermissionEntry(String accessControlledPath, Tree entryTree, RestrictionProvider restrictionsProvider) {
-            isAllow = (PREFIX_ALLOW == entryTree.getName().charAt(0));
+            isAllow = entryTree.getProperty(REP_IS_ALLOW).getValue(Type.BOOLEAN);
             privilegeBits = PrivilegeBits.getInstance(entryTree.getProperty(REP_PRIVILEGE_BITS));
             path = accessControlledPath;
             restriction = restrictionsProvider.getPattern(accessControlledPath, entryTree);
@@ -411,21 +412,32 @@ class CompiledPermissionImpl implements 
         private void addEntries(@Nonnull Principal principal,
                                 @Nonnull Tree principalRoot,
                                 @Nonnull RestrictionProvider restrictionProvider) {
+            boolean isGroup = (principal instanceof Group);
             for (Tree entryTree : principalRoot.getChildren()) {
-                Key key = new Key(entryTree);
-                PermissionEntry entry = new PermissionEntry(key.path, entryTree, restrictionProvider);
-                if (!entry.privilegeBits.isEmpty()) {
-                    if (key.path == null) {
-                        repoEntries.put(key, entry);
-                    } else if (principal instanceof Group) {
-                        groupEntries.put(key, entry);
-                    } else {
-                        userEntries.put(key, entry);
-                    }
+                traverse(entryTree, isGroup, restrictionProvider);
+            }
+        }
+
+        private void traverse(@Nonnull Tree entryTree, boolean isGroup, @Nonnull RestrictionProvider restrictionProvider) {
+
+            Key key = new Key(entryTree);
+            PermissionEntry entry = new PermissionEntry(key.path, entryTree, restrictionProvider);
+            if (!entry.privilegeBits.isEmpty()) {
+                if (key.path == null) {
+                    repoEntries.put(key, entry);
+                } else if (isGroup) {
+                    groupEntries.put(key, entry);
+                } else {
+                    userEntries.put(key, entry);
                 }
             }
+
+            for (Tree child : entryTree.getChildren()) {
+                traverse(child, isGroup, restrictionProvider);
+            }
         }
 
+
         private Map<Key, PermissionEntry> getRepoEntries() {
             return repoEntries.build();
         }

Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/permission/PermissionConstants.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/permission/PermissionConstants.java?rev=1489515&r1=1489514&r2=1489515&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/permission/PermissionConstants.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/permission/PermissionConstants.java Tue Jun  4 17:00:55 2013
@@ -34,13 +34,11 @@ public interface PermissionConstants {
     String PERMISSIONS_STORE_PATH = '/' + JcrConstants.JCR_SYSTEM + '/' + REP_PERMISSION_STORE;
 
     String REP_ACCESS_CONTROLLED_PATH = "rep:accessControlledPath";
+	String REP_IS_ALLOW = "rep:isAllow";
     String REP_PRIVILEGE_BITS = "rep:privileges";
     String REP_INDEX = "rep:index";
 
-    char PREFIX_ALLOW = 'a';
-    char PREFIX_DENY = 'd';
-
     Set<String> PERMISSION_NODETYPE_NAMES = ImmutableSet.of(NT_REP_PERMISSIONS, NT_REP_PERMISSION_STORE);
     Set<String> PERMISSION_NODE_NAMES = ImmutableSet.of(REP_PERMISSION_STORE);
     Set<String> PERMISSION_PROPERTY_NAMES = ImmutableSet.of(REP_ACCESS_CONTROLLED_PATH, REP_PRIVILEGE_BITS, REP_INDEX);
-}
\ No newline at end of file
+}

Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/permission/PermissionHook.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/permission/PermissionHook.java?rev=1489515&r1=1489514&r2=1489515&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/permission/PermissionHook.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/permission/PermissionHook.java Tue Jun  4 17:00:55 2013
@@ -91,7 +91,7 @@ public class PermissionHook implements P
     @Nonnull
     private NodeBuilder getPermissionRoot(NodeBuilder rootBuilder) {
         // permission root has been created during workspace initialization
-        return rootBuilder.child(JCR_SYSTEM).child(REP_PERMISSION_STORE).child(workspaceName);
+        return rootBuilder.getChildNode(JCR_SYSTEM).getChildNode(REP_PERMISSION_STORE).getChildNode(workspaceName);
     }
 
     private static Tree getTree(String name, NodeState nodeState) {
@@ -116,8 +116,11 @@ public class PermissionHook implements P
                 List<String> reordered = new ChildOrderDiff(before, after).getReordered();
                 for (String name : reordered) {
                     NodeState beforeNode = parentBefore.getNodeState().getChildNode(name);
+                    removeEntry(name, beforeNode);
+                }
+                for (String name : reordered) {
                     NodeState afterNode = parentAfter.getNodeState().getChildNode(name);
-                    updateEntry(name, beforeNode, afterNode);
+                    addEntry(name, afterNode);
 
                     log.debug("Processed reordered child node " + name);
                     processed.add(name);
@@ -192,8 +195,12 @@ public class PermissionHook implements P
                 log.debug("ACE entry already processed -> skip updateEntry.");
                 return;
             }
-            removeEntry(name, before);
-            addEntry(name, after);
+            PermissionEntry beforeEntry = createPermissionEntry(name, before, parentBefore);
+            PermissionEntry afterEntry = createPermissionEntry(name, after, parentAfter);
+            if (!beforeEntry.equals(afterEntry)) {
+                beforeEntry.remove();
+                afterEntry.add();
+            }
         }
 
         @Nonnull
@@ -276,14 +283,14 @@ public class PermissionHook implements P
     private final class PermissionEntry {
 
         private final String accessControlledPath;
-        private final int index;
+        private final long index;
         private final String principalName;
         private final PrivilegeBits privilegeBits;
         private final boolean isAllow;
         private final Set<Restriction> restrictions;
         private final String nodeName;
 
-        private PermissionEntry(@Nonnull Tree aceTree, @Nonnull String accessControlledPath, int index) {
+        private PermissionEntry(@Nonnull Tree aceTree, @Nonnull String accessControlledPath, long index) {
             this.accessControlledPath = accessControlledPath;
             this.index = index;
 
@@ -293,9 +300,9 @@ public class PermissionHook implements P
             restrictions = restrictionProvider.readRestrictions(Strings.emptyToNull(accessControlledPath), aceTree);
 
             // create node name from ace definition
+            int depth = PathUtils.getDepth(accessControlledPath);
             StringBuilder name = new StringBuilder();
-            name.append((isAllow) ? PREFIX_ALLOW : PREFIX_DENY).append('-');
-            name.append(Objects.hashCode(accessControlledPath, principalName, index, privilegeBits, isAllow, restrictions));
+            name.append(depth).append('_').append(hashCode());
             nodeName = name.toString();
         }
 
@@ -304,30 +311,103 @@ public class PermissionHook implements P
             if (!principalRoot.hasProperty(JCR_PRIMARYTYPE)) {
                 principalRoot.setProperty(JCR_PRIMARYTYPE, NT_REP_PERMISSION_STORE, Type.NAME);
             }
-            NodeBuilder entry = principalRoot.child(nodeName)
+            NodeBuilder parent = getParent(principalRoot);
+            NodeBuilder entry = parent.child(nodeName)
                     .setProperty(JCR_PRIMARYTYPE, NT_REP_PERMISSIONS, Type.NAME)
                     .setProperty(REP_ACCESS_CONTROLLED_PATH, accessControlledPath)
+                    .setProperty(REP_IS_ALLOW, isAllow)
                     .setProperty(REP_INDEX, index)
                     .setProperty(privilegeBits.asPropertyState(REP_PRIVILEGE_BITS));
             for (Restriction restriction : restrictions) {
                 entry.setProperty(restriction.getProperty());
             }
+            for (String childName : parent.getChildNodeNames()) {
+                if (nodeName.equals(childName)) {
+                    continue;
+                }
+                NodeBuilder child = parent.getChildNode(childName);
+                String childPath = child.getProperty(REP_ACCESS_CONTROLLED_PATH).getValue(Type.STRING);
+                long childIndex = child.getProperty(REP_INDEX).getValue(Type.LONG);
+                boolean reconnect = false;
+                if (Text.isDescendant(accessControlledPath, childPath)) {
+                    reconnect = true;
+                } else if (childPath.equals(accessControlledPath)) {
+                    reconnect = index < childIndex;
+                }
+                if (reconnect) {
+                    entry.setChildNode(childName, child.getNodeState());
+                    child.remove();
+                }
+            }
         }
 
         private void remove() {
+            String msg = "Unable to remove permission entry";
             if (permissionRoot.hasChildNode(principalName)) {
-                NodeBuilder principalRoot = permissionRoot.child(principalName);
-                principalRoot.getChildNode(nodeName).remove();
+                NodeBuilder principalRoot = permissionRoot.getChildNode(principalName);
+                NodeBuilder parent = getParent(principalRoot);
+                if (parent.hasChildNode(nodeName)) {
+                    NodeBuilder target = parent.getChildNode(nodeName);
+                    for (String childName : target.getChildNodeNames()) {
+                        NodeBuilder child = target.getChildNode(childName);
+                        parent.setChildNode(childName, child.getNodeState());
+                        child.remove();
+                    }
+                    target.remove();
+                } else {
+                    log.error("{} {}. No corresponding node found in permission store.", msg, this);
+                }
+            } else {
+                log.error("{} {}: Principal root missing.", msg, this);
             }
         }
 
+        private NodeBuilder getParent(NodeBuilder parent) {
+            if (parent.hasChildNode(nodeName)) {
+                return parent;
+            }
+            for (String childName : parent.getChildNodeNames()) {
+                NodeBuilder child = parent.getChildNode(childName);
+                String childPath = child.getProperty(REP_ACCESS_CONTROLLED_PATH).getValue(Type.STRING);
+                long childIndex = child.getProperty(REP_INDEX).getValue(Type.LONG);
+                if (Text.isDescendant(childPath, accessControlledPath)) {
+                    return getParent(child);
+                } else if (childPath.equals(accessControlledPath) && index > childIndex) {
+                    return getParent(child);
+                }
+            }
+            return parent;
+        }
+
+        @Override
+        public int hashCode() {
+            return Objects.hashCode(accessControlledPath, principalName, index, privilegeBits, isAllow, restrictions);
+        }
+
+        @Override
+        public boolean equals(Object o) {
+            if (o == this) {
+                return true;
+            }
+            if (o instanceof PermissionEntry) {
+                PermissionEntry other = (PermissionEntry) o;
+                return index == other.index && isAllow == other.isAllow
+                        && privilegeBits.equals(other.privilegeBits)
+                        && principalName.equals(other.principalName)
+                        && accessControlledPath.equals(other.accessControlledPath)
+                        && restrictions.equals(other.restrictions);
+            }
+            return false;
+        }
+
+        @Override
         public String toString() {
             StringBuilder sb = new StringBuilder();
             sb.append("permission entry: ").append(accessControlledPath);
             sb.append(';').append(index);
             sb.append(';').append(principalName);
             sb.append(';').append(isAllow ? "allow" : "deny");
-            sb.append(';').append(privilegeBits);
+            sb.append(';').append(bitsProvider.getPrivilegeNames(privilegeBits));
             sb.append(';').append(restrictions);
             return sb.toString();
         }

Modified: jackrabbit/oak/trunk/oak-core/src/main/resources/org/apache/jackrabbit/oak/plugins/nodetype/write/builtin_nodetypes.cnd
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/resources/org/apache/jackrabbit/oak/plugins/nodetype/write/builtin_nodetypes.cnd?rev=1489515&r1=1489514&r2=1489515&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/resources/org/apache/jackrabbit/oak/plugins/nodetype/write/builtin_nodetypes.cnd (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/resources/org/apache/jackrabbit/oak/plugins/nodetype/write/builtin_nodetypes.cnd Tue Jun  4 17:00:55 2013
@@ -642,7 +642,7 @@
 [rep:PermissionStore]
   orderable
   + * (rep:PermissionStore) = rep:PermissionStore protected IGNORE
-  + * (rep:Permissions) = rep:Permissions protected  IGNORE
+  + * (rep:Permissions) = rep:Permissions protected IGNORE
 
 /**
  * @since oak 1.0
@@ -650,6 +650,7 @@
 [rep:Permissions]
   - * (UNDEFINED) protected
   - * (UNDEFINED) protected multiple
+  + * (rep:Permissions) = rep:Permissions protected IGNORE
 
 // -----------------------------------------------------------------------------
 // Principal based AC

Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/permission/CompiledPermissionImplTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/permission/CompiledPermissionImplTest.java?rev=1489515&r1=1489514&r2=1489515&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/permission/CompiledPermissionImplTest.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/permission/CompiledPermissionImplTest.java Tue Jun  4 17:00:55 2013
@@ -31,17 +31,18 @@ import org.apache.jackrabbit.JcrConstant
 import org.apache.jackrabbit.oak.AbstractSecurityTest;
 import org.apache.jackrabbit.oak.api.CommitFailedException;
 import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.commons.PathUtils;
 import org.apache.jackrabbit.oak.core.ImmutableRoot;
 import org.apache.jackrabbit.oak.core.ImmutableTree;
 import org.apache.jackrabbit.oak.core.TreeTypeProvider;
 import org.apache.jackrabbit.oak.security.SecurityProviderImpl;
-import org.apache.jackrabbit.oak.spi.security.SecurityConfiguration;
-import org.apache.jackrabbit.oak.spi.security.authorization.AccessControlConstants;
 import org.apache.jackrabbit.oak.security.authorization.restriction.RestrictionProviderImpl;
 import org.apache.jackrabbit.oak.security.privilege.PrivilegeBits;
 import org.apache.jackrabbit.oak.security.privilege.PrivilegeBitsProvider;
+import org.apache.jackrabbit.oak.spi.security.SecurityConfiguration;
 import org.apache.jackrabbit.oak.spi.security.SecurityProvider;
 import org.apache.jackrabbit.oak.spi.security.authorization.AccessControlConfiguration;
+import org.apache.jackrabbit.oak.spi.security.authorization.AccessControlConstants;
 import org.apache.jackrabbit.oak.spi.security.authorization.OpenAccessControlConfiguration;
 import org.apache.jackrabbit.oak.spi.security.authorization.permission.Permissions;
 import org.apache.jackrabbit.oak.spi.security.authorization.permission.ReadStatus;
@@ -524,11 +525,12 @@ public class CompiledPermissionImplTest 
     private void setupPermission(Principal principal, String path, boolean isAllow,
                                  int index, String[] privilegeName, Set<Restriction> restrictions) throws CommitFailedException {
         PrivilegeBits pb = pbp.getBits(privilegeName);
-        String name = ((isAllow) ? PREFIX_ALLOW : PREFIX_DENY) + "-" + Objects.hashCode(path, principal, index, pb, isAllow, restrictions);
+        String name = PathUtils.getDepth(path) + "_" + Objects.hashCode(path, principal, index, pb, isAllow, restrictions);
         Tree principalRoot = root.getTree(PERMISSIONS_STORE_PATH + '/' + principal.getName());
         Tree entry = principalRoot.addChild(name);
         entry.setProperty(JCR_PRIMARYTYPE, NT_REP_PERMISSIONS);
         entry.setProperty(REP_ACCESS_CONTROLLED_PATH, path);
+        entry.setProperty(REP_IS_ALLOW, isAllow);
         entry.setProperty(REP_INDEX, index);
         entry.setProperty(pb.asPropertyState(REP_PRIVILEGE_BITS));
         for (Restriction restriction : restrictions) {
@@ -541,7 +543,7 @@ public class CompiledPermissionImplTest 
                                   ReadStatus expectedProperties,
                                   CompiledPermissions cp,
                                   String treePath) {
-        assertReadStatus(expectedTrees, expectedTrees, cp, Collections.singleton(treePath));
+        assertReadStatus(expectedTrees, expectedProperties, cp, Collections.singleton(treePath));
     }
 
     private void assertReadStatus(ReadStatus expectedTrees,

Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/permission/PermissionHookTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/permission/PermissionHookTest.java?rev=1489515&r1=1489514&r2=1489515&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/permission/PermissionHookTest.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/permission/PermissionHookTest.java Tue Jun  4 17:00:55 2013
@@ -16,24 +16,16 @@
  */
 package org.apache.jackrabbit.oak.security.authorization.permission;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
-
 import java.security.Principal;
 import java.util.ArrayList;
-import java.util.HashSet;
+import java.util.Arrays;
+import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
-import java.util.Set;
-
 import javax.jcr.RepositoryException;
 import javax.jcr.security.AccessControlEntry;
 import javax.jcr.security.AccessControlManager;
 
-import com.google.common.collect.ImmutableSet;
 import org.apache.jackrabbit.JcrConstants;
 import org.apache.jackrabbit.api.security.JackrabbitAccessControlList;
 import org.apache.jackrabbit.api.security.user.Group;
@@ -42,6 +34,7 @@ import org.apache.jackrabbit.oak.api.Con
 import org.apache.jackrabbit.oak.api.Root;
 import org.apache.jackrabbit.oak.api.Tree;
 import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.commons.PathUtils;
 import org.apache.jackrabbit.oak.security.privilege.PrivilegeBits;
 import org.apache.jackrabbit.oak.security.privilege.PrivilegeBitsProvider;
 import org.apache.jackrabbit.oak.spi.security.authorization.AbstractAccessControlTest;
@@ -49,14 +42,21 @@ import org.apache.jackrabbit.oak.spi.sec
 import org.apache.jackrabbit.oak.spi.security.principal.EveryonePrincipal;
 import org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeConstants;
 import org.apache.jackrabbit.oak.util.NodeUtil;
+import org.apache.jackrabbit.util.Text;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
 /**
  * Testing the {@code PermissionHook}
  */
-public class PermissionHookTest extends AbstractAccessControlTest implements AccessControlConstants, PermissionConstants {
+public class PermissionHookTest extends AbstractAccessControlTest implements AccessControlConstants, PermissionConstants, PrivilegeConstants {
 
     private String testPath = "/testPath";
     private String childPath = "/testPath/childNode";
@@ -77,8 +77,8 @@ public class PermissionHookTest extends 
 
         AccessControlManager acMgr = getAccessControlManager(root);
         JackrabbitAccessControlList acl = AccessControlUtils.getAccessControlList(acMgr, testPath);
-        acl.addAccessControlEntry(testPrincipal, privilegesFromNames(PrivilegeConstants.JCR_ADD_CHILD_NODES));
-        acl.addAccessControlEntry(EveryonePrincipal.getInstance(), privilegesFromNames(PrivilegeConstants.JCR_READ));
+        acl.addAccessControlEntry(testPrincipal, privilegesFromNames(JCR_ADD_CHILD_NODES));
+        acl.addAccessControlEntry(EveryonePrincipal.getInstance(), privilegesFromNames(JCR_READ));
         acMgr.setPolicy(testPath, acl);
         root.commit();
 
@@ -109,15 +109,35 @@ public class PermissionHookTest extends 
         return root.getTree(PERMISSIONS_STORE_PATH).getChild(adminSession.getWorkspaceName()).getChild(principalName);
     }
 
-    private Tree getEntry(String principalName, String accessControlledPath) throws Exception {
+    private Tree getEntry(String principalName, String accessControlledPath, long index) throws Exception {
         Tree principalRoot = getPrincipalRoot(principalName);
-        for (Tree entry : principalRoot.getChildren()) {
-            if (accessControlledPath.equals(entry.getProperty(REP_ACCESS_CONTROLLED_PATH).getValue(Type.STRING))) {
-                return entry;
-            }
-        }
-        throw new RepositoryException("no such entry");
+		return traverse(principalRoot, accessControlledPath, index);
     }
+	
+	private Tree traverse(Tree parent, String accessControlledPath, long index) throws Exception {
+		for (Tree entry : parent.getChildren()) {
+		    String path = entry.getProperty(REP_ACCESS_CONTROLLED_PATH).getValue(Type.STRING);
+            long entryIndex = entry.getProperty(REP_INDEX).getValue(Type.LONG);
+            if (accessControlledPath.equals(path)) {
+                if (index == entryIndex) {
+                    return entry;
+                } else if (index > entryIndex) {
+                    return traverse(entry, accessControlledPath, index);
+                }
+            } else if (Text.isDescendant(path, accessControlledPath)) {
+				return traverse(entry, accessControlledPath, index);
+			}
+        }
+	    throw new RepositoryException("no such entry");
+	}
+	
+	private long cntEntries(Tree parent) {
+        long cnt = parent.getChildrenCount();
+		for (Tree child : parent.getChildren()) {
+			cnt += cntEntries(child);
+		}
+		return cnt;	
+	}
 
     private void createPrincipals() throws Exception {
         if (principals.isEmpty()) {
@@ -135,25 +155,29 @@ public class PermissionHookTest extends 
         NodeUtil policy = new NodeUtil(root.getTree(testPath + "/rep:policy"));
         NodeUtil ace = policy.addChild("duplicateAce", NT_REP_GRANT_ACE);
         ace.setString(REP_PRINCIPAL_NAME, testPrincipalName);
-        ace.setStrings(REP_PRIVILEGES, PrivilegeConstants.JCR_ADD_CHILD_NODES);
+        ace.setStrings(AccessControlConstants.REP_PRIVILEGES, JCR_ADD_CHILD_NODES);
         root.commit();
 
         Tree principalRoot = getPrincipalRoot(testPrincipalName);
-        assertEquals(2, principalRoot.getChildrenCount());
+        assertEquals(2, cntEntries(principalRoot));
 
-        Set<Integer> index = new HashSet<Integer>(2);
-        for (Tree entry : principalRoot.getChildren()) {
-            assertEquals(bitsProvider.getBits(PrivilegeConstants.JCR_ADD_CHILD_NODES), PrivilegeBits.getInstance(entry.getProperty(REP_PRIVILEGE_BITS)));
-            assertEquals(testPath, entry.getProperty(REP_ACCESS_CONTROLLED_PATH).getValue(Type.STRING));
-            index.add(entry.getProperty(REP_INDEX).getValue(Type.LONG).intValue());
-        }
-        assertEquals(ImmutableSet.of(0, 2), index);
+        assertEquals(1, principalRoot.getChildrenCount());
+		Tree entry1 = principalRoot.getChildren().iterator().next();
+		assertEquals(bitsProvider.getBits(JCR_ADD_CHILD_NODES), PrivilegeBits.getInstance(entry1.getProperty(REP_PRIVILEGE_BITS)));
+		assertEquals(testPath, entry1.getProperty(REP_ACCESS_CONTROLLED_PATH).getValue(Type.STRING));
+        assertEquals(0, entry1.getProperty(REP_INDEX).getValue(Type.LONG).intValue());
+		
+	    assertEquals(1, entry1.getChildrenCount());
+		Tree entry2 = entry1.getChildren().iterator().next();
+		assertEquals(bitsProvider.getBits(JCR_ADD_CHILD_NODES), PrivilegeBits.getInstance(entry2.getProperty(REP_PRIVILEGE_BITS)));
+		assertEquals(testPath, entry2.getProperty(REP_ACCESS_CONTROLLED_PATH).getValue(Type.STRING));
+        assertEquals(2, entry2.getProperty(REP_INDEX).getValue(Type.LONG).intValue());
 
         // remove duplicate policy entry again
         root.getTree(testPath + "/rep:policy/duplicateAce").remove();
         root.commit();
 
-        assertEquals(1, getPrincipalRoot(testPrincipalName).getChildrenCount());
+        assertEquals(1, cntEntries(getPrincipalRoot(testPrincipalName)));
     }
 
     @Test
@@ -169,7 +193,7 @@ public class PermissionHookTest extends 
         root.commit();
 
         Tree principalRoot = getPrincipalRoot(testPrincipalName);
-        assertEquals(1, principalRoot.getChildrenCount());
+        assertEquals(1, cntEntries(principalRoot));
         assertEquals("*", principalRoot.getChildren().iterator().next().getProperty(REP_GLOB).getValue(Type.STRING));
 
         // modify the restrictions node
@@ -178,7 +202,7 @@ public class PermissionHookTest extends 
         root.commit();
 
         principalRoot = getPrincipalRoot(testPrincipalName);
-        assertEquals(1, principalRoot.getChildrenCount());
+        assertEquals(1, cntEntries(principalRoot));
         assertEquals("/*/jcr:content/*", principalRoot.getChildren().iterator().next().getProperty(REP_GLOB).getValue(Type.STRING));
 
         // remove the restriction again
@@ -186,14 +210,13 @@ public class PermissionHookTest extends 
         root.commit();
 
         principalRoot = getPrincipalRoot(testPrincipalName);
-        assertEquals(1, principalRoot.getChildrenCount());
+        assertEquals(1, cntEntries(principalRoot));
         assertNull(principalRoot.getChildren().iterator().next().getProperty(REP_GLOB));
-
     }
 
     @Test
     public void testReorderAce() throws Exception {
-        Tree entry = getEntry(testPrincipalName, testPath);
+        Tree entry = getEntry(testPrincipalName, testPath, 0);
         assertEquals(0, entry.getProperty(REP_INDEX).getValue(Type.LONG).longValue());
 
         Tree aclTree = root.getTree(testPath + "/rep:policy");
@@ -201,13 +224,13 @@ public class PermissionHookTest extends 
 
         root.commit();
 
-        entry = getEntry(testPrincipalName, testPath);
+        entry = getEntry(testPrincipalName, testPath, 1);
         assertEquals(1, entry.getProperty(REP_INDEX).getValue(Type.LONG).longValue());
     }
 
     @Test
     public void testReorderAndAddAce() throws Exception {
-        Tree entry = getEntry(testPrincipalName, testPath);
+        Tree entry = getEntry(testPrincipalName, testPath, 0);
         assertEquals(0, entry.getProperty(REP_INDEX).getValue(Type.LONG).longValue());
 
         Tree aclTree = root.getTree(testPath + "/rep:policy");
@@ -217,16 +240,16 @@ public class PermissionHookTest extends 
         // add a new entry
         NodeUtil ace = new NodeUtil(aclTree).addChild("denyEveryoneLockMgt", NT_REP_DENY_ACE);
         ace.setString(REP_PRINCIPAL_NAME, EveryonePrincipal.NAME);
-        ace.setStrings(REP_PRIVILEGES, PrivilegeConstants.JCR_LOCK_MANAGEMENT);
+        ace.setStrings(AccessControlConstants.REP_PRIVILEGES, JCR_LOCK_MANAGEMENT);
         root.commit();
 
-        entry = getEntry(testPrincipalName, testPath);
+        entry = getEntry(testPrincipalName, testPath, 1);
         assertEquals(1, entry.getProperty(REP_INDEX).getValue(Type.LONG).longValue());
     }
 
     @Test
     public void testReorderAddAndRemoveAces() throws Exception {
-        Tree entry = getEntry(testPrincipalName, testPath);
+        Tree entry = getEntry(testPrincipalName, testPath, 0);
         assertEquals(0, entry.getProperty(REP_INDEX).getValue(Type.LONG).longValue());
 
         Tree aclTree = root.getTree(testPath + "/rep:policy");
@@ -243,14 +266,14 @@ public class PermissionHookTest extends 
         // add a new entry
         NodeUtil ace = new NodeUtil(aclTree).addChild("denyEveryoneLockMgt", NT_REP_DENY_ACE);
         ace.setString(REP_PRINCIPAL_NAME, EveryonePrincipal.NAME);
-        ace.setStrings(REP_PRIVILEGES, PrivilegeConstants.JCR_LOCK_MANAGEMENT);
+        ace.setStrings(AccessControlConstants.REP_PRIVILEGES, JCR_LOCK_MANAGEMENT);
 
         // reorder the new entry before the remaining existing entry
         ace.getTree().orderBefore(name);
 
         root.commit();
 
-        entry = getEntry(testPrincipalName, testPath);
+        entry = getEntry(testPrincipalName, testPath, 1);
         assertEquals(1, entry.getProperty(REP_INDEX).getValue(Type.LONG).longValue());
     }
 
@@ -266,7 +289,7 @@ public class PermissionHookTest extends 
         AccessControlManager acMgr = getAccessControlManager(root);
         JackrabbitAccessControlList acl = AccessControlUtils.getAccessControlList(acMgr, testPath);
         for (int i = 0; i < 4; i++) {
-            acl.addAccessControlEntry(principals.get(i), privilegesFromNames(PrivilegeConstants.JCR_READ));
+            acl.addAccessControlEntry(principals.get(i), privilegesFromNames(JCR_READ));
         }
         acMgr.setPolicy(testPath, acl);
         root.commit();
@@ -275,15 +298,15 @@ public class PermissionHookTest extends 
         acl.removeAccessControlEntry(aces[0]);
         acl.removeAccessControlEntry(aces[2]);
         acl.orderBefore(aces[4], aces[3]);
-        acl.addAccessControlEntry(principals.get(4), privilegesFromNames(PrivilegeConstants.JCR_READ));
-        acl.addAccessControlEntry(principals.get(5), privilegesFromNames(PrivilegeConstants.JCR_READ));
+        acl.addAccessControlEntry(principals.get(4), privilegesFromNames(JCR_READ));
+        acl.addAccessControlEntry(principals.get(5), privilegesFromNames(JCR_READ));
         acMgr.setPolicy(testPath, acl);
         root.commit();
 
-        Tree entry = getEntry(principals.get(2).getName(), testPath);
+        Tree entry = getEntry(principals.get(2).getName(), testPath, 1);
         assertEquals(1, entry.getProperty(REP_INDEX).getValue(Type.LONG).longValue());
 
-        entry = getEntry(principals.get(1).getName(), testPath);
+        entry = getEntry(principals.get(1).getName(), testPath, 2);
         assertEquals(2, entry.getProperty(REP_INDEX).getValue(Type.LONG).longValue());
     }
 
@@ -299,7 +322,7 @@ public class PermissionHookTest extends 
         AccessControlManager acMgr = getAccessControlManager(root);
         JackrabbitAccessControlList acl = AccessControlUtils.getAccessControlList(acMgr, testPath);
         for (int i = 0; i < 4; i++) {
-            acl.addAccessControlEntry(principals.get(i), privilegesFromNames(PrivilegeConstants.JCR_READ));
+            acl.addAccessControlEntry(principals.get(i), privilegesFromNames(JCR_READ));
         }
         acMgr.setPolicy(testPath, acl);
         root.commit();
@@ -312,15 +335,15 @@ public class PermissionHookTest extends 
         acMgr.setPolicy(testPath, acl);
         root.commit();
 
-        Tree entry = getEntry(EveryonePrincipal.NAME, testPath);
+        Tree entry = getEntry(EveryonePrincipal.NAME, testPath, 1);
         assertEquals(1, entry.getProperty(REP_INDEX).getValue(Type.LONG).longValue());
 
-        entry = getEntry(principals.get(2).getName(), testPath);
+        entry = getEntry(principals.get(2).getName(), testPath, 3);
         assertEquals(3, entry.getProperty(REP_INDEX).getValue(Type.LONG).longValue());
 
         for (String pName : new String[] {testPrincipalName, principals.get(0).getName()}) {
             try {
-                getEntry(pName, testPath);
+                getEntry(pName, testPath, 0);
                 fail();
             } catch (RepositoryException e) {
                 // success
@@ -332,18 +355,18 @@ public class PermissionHookTest extends 
     public void testImplicitAceRemoval() throws Exception {
         AccessControlManager acMgr = getAccessControlManager(root);
         JackrabbitAccessControlList acl = AccessControlUtils.getAccessControlList(acMgr, testPath);
-        acl.addAccessControlEntry(getTestPrincipal(), privilegesFromNames(PrivilegeConstants.JCR_READ, PrivilegeConstants.REP_WRITE));
+        acl.addAccessControlEntry(getTestPrincipal(), privilegesFromNames(JCR_READ, REP_WRITE));
         acMgr.setPolicy(testPath, acl);
 
         acl = AccessControlUtils.getAccessControlList(acMgr, childPath);
-        acl.addAccessControlEntry(EveryonePrincipal.getInstance(), privilegesFromNames(PrivilegeConstants.JCR_READ));
+        acl.addAccessControlEntry(EveryonePrincipal.getInstance(), privilegesFromNames(JCR_READ));
         acMgr.setPolicy(childPath, acl);
         root.commit();
 
         assertTrue(root.getTree(childPath+"/rep:policy").exists());
 
         Tree principalRoot = getPrincipalRoot(EveryonePrincipal.NAME);
-        assertEquals(2, principalRoot.getChildrenCount());
+        assertEquals(2, cntEntries(principalRoot));
 
         ContentSession testSession = createTestSession();
         Root testRoot = testSession.getLatestRoot();
@@ -361,6 +384,136 @@ public class PermissionHookTest extends 
         // aces must be removed in the permission store even if the editing
         // session wasn't able to access them.
         principalRoot = getPrincipalRoot(EveryonePrincipal.NAME);
-        assertEquals(1, principalRoot.getChildrenCount());
+        assertEquals(1, cntEntries(principalRoot));
+    }
+
+    @Test
+    public void testReorderForSinglePrincipal() throws Exception {
+        Principal testPrincipal = getTestPrincipal();
+        AccessControlManager acMgr = getAccessControlManager(root);
+        JackrabbitAccessControlList acl = AccessControlUtils.getAccessControlList(acMgr, testPath);
+        acl.addEntry(testPrincipal, privilegesFromNames(JCR_MODIFY_ACCESS_CONTROL), false);
+        acl.addEntry(testPrincipal, privilegesFromNames(JCR_READ_ACCESS_CONTROL), true, Collections.singletonMap(REP_GLOB, getValueFactory().createValue("/*")));
+        acMgr.setPolicy(testPath, acl);
+        root.commit();
+
+        /*
+        Original setup with 3 access control entries for testPrincipal @ testPath
+        Expected result:
+        0 - testuser - allow - JCR_ADD_CHILD_NODES       - NA
+        1 - everyone - allow - READ                      - NA
+        2 - testuser - deny  - JCR_MODIFY_ACCESS_CONTROL - NA
+        3 - testuser - allow - JCR_READ_ACCESS_CONTROL   - /*
+        */
+        long rootDepths = PathUtils.getDepth(getPrincipalRoot(testPrincipalName).getPath());
+        assertEntry(getEntry(testPrincipalName, testPath, 0), bitsProvider.getBits(JCR_ADD_CHILD_NODES), true, ++rootDepths);
+        assertEntry(getEntry(testPrincipalName, testPath, 2), bitsProvider.getBits(JCR_MODIFY_ACCESS_CONTROL), false, ++rootDepths);
+        assertEntry(getEntry(testPrincipalName, testPath, 3), bitsProvider.getBits(JCR_READ_ACCESS_CONTROL), true, ++rootDepths);
+
+        /*
+        Reorder entries
+        Expected result:
+        0 - allow - JCR_ADD_CHILD_NODES       - NA
+        2 - allow - JCR_READ_ACCESS_CONTROL   - /*
+        3 - deny  - JCR_MODIFY_ACCESS_CONTROL - NA
+        */
+        acl = AccessControlUtils.getAccessControlList(acMgr, testPath);
+        for (AccessControlEntry ace : acl.getAccessControlEntries()) {
+            if (testPrincipal.equals(ace.getPrincipal()) && Arrays.equals(privilegesFromNames(JCR_MODIFY_ACCESS_CONTROL), ace.getPrivileges())) {
+                acl.orderBefore(ace, null);
+            }
+        }
+        acMgr.setPolicy(testPath, acl);
+        root.commit();
+
+        rootDepths = PathUtils.getDepth(getPrincipalRoot(testPrincipalName).getPath());
+        assertEntry(getEntry(testPrincipalName, testPath, 0), bitsProvider.getBits(JCR_ADD_CHILD_NODES), true, ++rootDepths);
+        assertEntry(getEntry(testPrincipalName, testPath, 2), bitsProvider.getBits(JCR_READ_ACCESS_CONTROL), true, ++rootDepths);
+        assertEntry(getEntry(testPrincipalName, testPath, 3), bitsProvider.getBits(JCR_MODIFY_ACCESS_CONTROL), false, ++rootDepths);
+
+        /*
+        Remove all entries
+        */
+        acl = AccessControlUtils.getAccessControlList(acMgr, testPath);
+        for (AccessControlEntry ace : acl.getAccessControlEntries()) {
+            if (testPrincipal.equals(ace.getPrincipal())) {
+                acl.removeAccessControlEntry(ace);
+            }
+        }
+        acMgr.setPolicy(testPath, acl);
+        root.commit();
+
+        assertEquals(0, cntEntries(getPrincipalRoot(testPrincipalName)));
+    }
+
+    @Test
+    public void testReorderForSinglePrincipal2() throws Exception {
+        Principal testPrincipal = getTestPrincipal();
+        AccessControlManager acMgr = getAccessControlManager(root);
+        JackrabbitAccessControlList acl = AccessControlUtils.getAccessControlList(acMgr, testPath);
+        acl.addEntry(testPrincipal, privilegesFromNames(JCR_MODIFY_ACCESS_CONTROL), false);
+        acl.addEntry(testPrincipal, privilegesFromNames(JCR_READ_ACCESS_CONTROL), true, Collections.singletonMap(REP_GLOB, getValueFactory().createValue("/*")));
+        acMgr.setPolicy(testPath, acl);
+        root.commit();
+
+        /*
+        Original setup with 3 access control entries for testPrincipal @ testPath
+        Expected result:
+        0 - testuser - allow - JCR_ADD_CHILD_NODES       - NA
+        1 - everyone - allow - READ                      - NA
+        2 - testuser - deny  - JCR_MODIFY_ACCESS_CONTROL - NA
+        3 - testuser - allow - JCR_READ_ACCESS_CONTROL   - /*
+        */
+        long rootDepths = PathUtils.getDepth(getPrincipalRoot(testPrincipalName).getPath());
+        assertEntry(getEntry(testPrincipalName, testPath, 0), bitsProvider.getBits(JCR_ADD_CHILD_NODES), true, ++rootDepths);
+        assertEntry(getEntry(testPrincipalName, testPath, 2), bitsProvider.getBits(JCR_MODIFY_ACCESS_CONTROL), false, ++rootDepths);
+        assertEntry(getEntry(testPrincipalName, testPath, 3), bitsProvider.getBits(JCR_READ_ACCESS_CONTROL), true, ++rootDepths);
+
+        /*
+        Reorder entries
+        Expected result:
+        0 - allow - JCR_READ_ACCESS_CONTROL   - /*
+        1 - allow - JCR_ADD_CHILD_NODES       - NA
+        3 - deny  - JCR_MODIFY_ACCESS_CONTROL - NA
+        */
+        acl = AccessControlUtils.getAccessControlList(acMgr, testPath);
+        AccessControlEntry first = null;
+        for (AccessControlEntry ace : acl.getAccessControlEntries()) {
+            if (testPrincipal.equals(ace.getPrincipal())) {
+                if (first == null) {
+                    first = ace;
+                }
+                if (Arrays.equals(privilegesFromNames(JCR_READ_ACCESS_CONTROL), ace.getPrivileges())) {
+                    acl.orderBefore(ace, first);
+                }
+            }
+        }
+        acMgr.setPolicy(testPath, acl);
+        root.commit();
+
+        rootDepths = PathUtils.getDepth(getPrincipalRoot(testPrincipalName).getPath());
+        assertEntry(getEntry(testPrincipalName, testPath, 0), bitsProvider.getBits(JCR_READ_ACCESS_CONTROL), true, ++rootDepths);
+        assertEntry(getEntry(testPrincipalName, testPath, 1), bitsProvider.getBits(JCR_ADD_CHILD_NODES), true, ++rootDepths);
+        assertEntry(getEntry(testPrincipalName, testPath, 3), bitsProvider.getBits(JCR_MODIFY_ACCESS_CONTROL), false, ++rootDepths);
+
+        /*
+        Remove all entries
+        */
+        acl = AccessControlUtils.getAccessControlList(acMgr, testPath);
+        for (AccessControlEntry ace : acl.getAccessControlEntries()) {
+            if (testPrincipal.equals(ace.getPrincipal())) {
+                acl.removeAccessControlEntry(ace);
+            }
+        }
+        acMgr.setPolicy(testPath, acl);
+        root.commit();
+
+        assertEquals(0, cntEntries(getPrincipalRoot(testPrincipalName)));
+    }
+
+    private static void assertEntry(Tree entry, PrivilegeBits expectedBits, boolean isAllow, long expectedDepth) {
+        assertEquals(expectedBits, PrivilegeBits.getInstance(entry.getProperty(REP_PRIVILEGE_BITS)));
+        assertEquals(isAllow, entry.getProperty(REP_IS_ALLOW).getValue(Type.BOOLEAN));
+        assertEquals(expectedDepth, PathUtils.getDepth(entry.getPath()));
     }
 }
\ No newline at end of file