OAK-8510 : FilterImpl.isValidPrincipal: add verification to spot move…

…d principal

git-svn-id: https://svn.apache.org/repos/asf/jackrabbit/oak/trunk@1863813 13f79535-47bb-0310-9956-ffa450edef68

oak-authorization-principalbased/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/FilterProviderImpl.java

@@ -169,7 +169,8 @@ private boolean isValidPrincipal(@NotNull Principal principal) {
             }
 
             String principalName = principal.getName();
-            if (validatedPrincipalNamesPathMap.containsKey(principalName)) {
+            String path = validatedPrincipalNamesPathMap.get(principalName);
+            if (path != null && isValidMapEntry(principal, path)) {
                 return true;
             }
 
@@ -182,15 +183,35 @@ private boolean isValidPrincipal(@NotNull Principal principal) {
             }
         }
 
+        /**
+         * Besteffort validation if the given entry in 'validatedPrincipalNamesPathMap' is points to the correct path.
+         * Note, that this will just be performed for instances of {@code ItemBasedPrincipal}, where obtaining the path
+         * doesn't require looking up the principal again.
+         *
+         * @param principal The target principal to be validated
+         * @param oakPath The Oak path stored in 'validatedPrincipalNamesPathMap' for the given principal.
+         * @return {@code true}, if the principal is an instance of {@code ItemBasedPrincipal}, whose Oak path is equal
+         * to the given {@code oakPath} and {@code false} if the paths are not equal. For any other types of principal
+         * this method will return {@code true} in order to avoid excessive principal lookup.
+         */
+        private boolean isValidMapEntry(@NotNull Principal principal, @NotNull String oakPath) {
+            if (principal instanceof ItemBasedPrincipal) {
+                return oakPath.equals(getOakPath((ItemBasedPrincipal) principal));
+            } else {
+                return true;
+            }
+        }
+
         @Nullable
         private String getPrincipalPath(@NotNull Principal principal) {
             String prinicpalOakPath = null;
             if (principal instanceof ItemBasedPrincipal) {
                 prinicpalOakPath = getOakPath((ItemBasedPrincipal) principal);
             }
             if (prinicpalOakPath == null || !root.getTree(prinicpalOakPath).exists()) {
-                // given principal is not ItemBasedPrincipal or it has been obtained with a different name-path-mapper
-                // making the conversion to oak-path return null -> try obtaining principal by name
+                // the given principal is not ItemBasedPrincipal or it has been obtained with a different name-path-mapper
+                // (making the conversion to oak-path return null) or it has been moved and the path no longer points to
+                // an existing tree -> try looking up principal by name
                 Principal p = principalProvider.getPrincipal(principal.getName());
                 if (p instanceof ItemBasedPrincipal) {
                     prinicpalOakPath = getOakPath((ItemBasedPrincipal) p);

oak-authorization-principalbased/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/AbstractPrincipalBasedTest.java

@@ -90,7 +90,7 @@ public void after() throws Exception {
         try {
             root.refresh();
             if (testSystemUser != null) {
-                testSystemUser.remove();
+                getUserManager(root).getAuthorizable(testSystemUser.getID()).remove();
                 root.commit();
             }
         } finally {

oak-authorization-principalbased/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/FilterImplTest.java

@@ -126,11 +126,21 @@ public void testCanHandleItemBasedSystemUserPrincipalUnsupportedPath() throws Ex
         assertFalse(filter.canHandle(Collections.singleton(principal)));
     }
 
-//    @Test
-//    public void testCanHandleItemBasedSystemUserPrincipalSupportedPath() {
-//        Principal principal = new TestPrincipal("name", PathUtils.concat(supportedPath, "oak:path/to/oak:principal"));
-//        assertTrue(filter.canHandle(Collections.singleton(principal)));
-//    }
+    @Test
+    public void testCanHandleMovedItemBasedSystemUserPrincipal() throws Exception {
+        Principal principal = getTestSystemUser().getPrincipal();
+        assertTrue(filter.canHandle(Collections.singleton(principal)));
+
+        String oakPath = filter.getOakPath(principal);
+        assertEquals(getNamePathMapper().getOakPath(getTestSystemUser().getPath()), oakPath);
+
+        String destPath = oakPath + "_moved";
+        root.move(oakPath, destPath);
+
+        Principal movedPrincipal = new TestPrincipal(principal.getName(), destPath);
+        assertTrue(filter.canHandle(Collections.singleton(movedPrincipal)));
+        assertEquals(destPath, filter.getOakPath(movedPrincipal));
+    }
 
     @Test
     public void testCanHandleGetPathThrows() {

oak-authorization-principalbased/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/MoveSystemUserTest.java

@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.oak.spi.security.authorization.principalbased.impl;
+
+import org.apache.jackrabbit.api.security.principal.ItemBasedPrincipal;
+import org.apache.jackrabbit.oak.commons.PathUtils;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.security.Principal;
+
+import static org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeConstants.JCR_READ;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+public class MoveSystemUserTest extends AbstractPrincipalBasedTest {
+
+    private Principal principal;
+
+    @Before
+    public void before() throws Exception {
+        super.before();
+        principal = getTestSystemUser().getPrincipal();
+        setupPrincipalBasedAccessControl(principal, "/content", JCR_READ);
+        root.commit();
+        PrincipalPolicyImpl policy = getPrincipalPolicyImpl(principal, getAccessControlManager(root));
+        assertEquals(1, policy.size());
+    }
+
+    @Test
+    public void testMoveUser() throws Exception {
+        String userId = getTestSystemUser().getID();
+        String srcJcrPath = getTestSystemUser().getPath();
+        String name = PathUtils.getName(srcJcrPath);
+        String destJcrPath = PathUtils.concat(PathUtils.getParentPath(srcJcrPath), name + "-moved");
+        assertTrue(root.move(getNamePathMapper().getOakPath(srcJcrPath), getNamePathMapper().getOakPath(destJcrPath)));
+        root.commit();
+
+        assertNull(getUserManager(root).getAuthorizableByPath(srcJcrPath));
+
+        Principal p = getPrincipalManager(root).getPrincipal(principal.getName());
+        assertNotNull(p);
+        assertTrue(p instanceof ItemBasedPrincipal);
+        assertEquals(destJcrPath, ((ItemBasedPrincipal) p).getPath());
+        PrincipalPolicyImpl policy = getPrincipalPolicyImpl(p, getAccessControlManager(root));
+        assertEquals(1, policy.size());
+    }
+}