[jackrabbit-oak] branch trunk updated: OAK-10132 ; Duplication in XPathConditionVisitor and ImpersonationImpl (#866)

[an...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

angela pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git


The following commit(s) were added to refs/heads/trunk by this push:
     new f666b0dbdf OAK-10132 ; Duplication in XPathConditionVisitor and ImpersonationImpl (#866)
f666b0dbdf is described below

commit f666b0dbdf937dc0376fa4fc0f2abfb703c80428
Author: anchela <an...@apache.org>
AuthorDate: Sun Mar 5 16:58:02 2023 +0100

    OAK-10132 ; Duplication in XPathConditionVisitor and ImpersonationImpl (#866)
---
 .../oak/security/user/ImpersonationImpl.java       |  9 +--
 .../apache/jackrabbit/oak/security/user/Utils.java | 28 ++++++++-
 .../security/user/query/XPathConditionVisitor.java | 13 +---
 .../jackrabbit/oak/security/user/UtilsTest.java    | 72 ++++++++++++++++++++++
 4 files changed, 103 insertions(+), 19 deletions(-)

diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/ImpersonationImpl.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/ImpersonationImpl.java
index 51152a1b79..867cdbaffe 100644
--- a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/ImpersonationImpl.java
+++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/ImpersonationImpl.java
@@ -26,7 +26,6 @@ import org.apache.jackrabbit.api.security.principal.PrincipalIterator;
 import org.apache.jackrabbit.api.security.principal.PrincipalManager;
 import org.apache.jackrabbit.api.security.user.Authorizable;
 import org.apache.jackrabbit.api.security.user.Impersonation;
-import org.apache.jackrabbit.api.security.user.User;
 import org.apache.jackrabbit.oak.api.PropertyState;
 import org.apache.jackrabbit.oak.api.Tree;
 import org.apache.jackrabbit.oak.api.Type;
@@ -177,13 +176,7 @@ class ImpersonationImpl implements Impersonation, UserConstants {
         } else if (GroupPrincipals.isGroup(principal)) {
             return false;
         } else {
-            try {
-                Authorizable authorizable = user.getUserManager().getAuthorizable(principal);
-                return authorizable != null && !authorizable.isGroup() && ((User) authorizable).isAdmin();
-            } catch (RepositoryException e) {
-                log.debug(e.getMessage());
-                return false;
-            }
+            return Utils.canImpersonateAllUsers(principal, user.getUserManager());
         }
     }
 
diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/Utils.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/Utils.java
index 197f661ab0..a39bf98a6e 100644
--- a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/Utils.java
+++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/Utils.java
@@ -18,6 +18,7 @@ package org.apache.jackrabbit.oak.security.user;
 
 import org.apache.jackrabbit.api.security.user.Authorizable;
 import org.apache.jackrabbit.api.security.user.User;
+import org.apache.jackrabbit.api.security.user.UserManager;
 import org.apache.jackrabbit.oak.api.Root;
 import org.apache.jackrabbit.oak.api.Tree;
 import org.apache.jackrabbit.oak.commons.PathUtils;
@@ -28,11 +29,16 @@ import org.apache.jackrabbit.oak.spi.security.principal.EveryonePrincipal;
 import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import javax.jcr.AccessDeniedException;
 import javax.jcr.RepositoryException;
+import java.security.Principal;
 
-final class Utils {
+public final class Utils {
+
+    private static final Logger log = LoggerFactory.getLogger(Utils.class);
 
     private Utils() {}
 
@@ -83,6 +89,26 @@ final class Utils {
     static boolean isEveryone(@NotNull Authorizable authorizable) {
         return authorizable.isGroup() && EveryonePrincipal.NAME.equals(getPrincipalName(authorizable));
     }
+
+    /**
+     * Return {@code true} if the given principal can impersonate all users. 
+     * The implementation tests if the given principal refers to an existing {@code User} for which {@link User#isAdmin()} 
+     * returns {@code true}.
+     * 
+     * @param principal A non-null principal instance.
+     * @param userManager The user manager used for the lookup calling {@link UserManager#getAuthorizable(Principal))}
+     * @return {@code true} if the given principal can impersonate all users; {@code false} if that condition is not met 
+     * or if the evaluation failed.
+     */
+    public static boolean canImpersonateAllUsers(@NotNull Principal principal, @NotNull UserManager userManager) {
+        try {
+            Authorizable authorizable = userManager.getAuthorizable(principal);
+            return authorizable != null && !authorizable.isGroup() && ((User) authorizable).isAdmin();
+        } catch (RepositoryException e) {
+            log.debug(e.getMessage());
+            return false;        
+        }
+    }
     
     @Nullable
     private static String getPrincipalName(@NotNull Authorizable authorizable) {
diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/query/XPathConditionVisitor.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/query/XPathConditionVisitor.java
index 8beb4697ee..f5c6a2e0aa 100644
--- a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/query/XPathConditionVisitor.java
+++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/query/XPathConditionVisitor.java
@@ -19,14 +19,14 @@ package org.apache.jackrabbit.oak.security.user.query;
 import javax.jcr.RepositoryException;
 
 import org.apache.jackrabbit.JcrConstants;
-import org.apache.jackrabbit.api.security.user.Authorizable;
-import org.apache.jackrabbit.api.security.user.User;
 import org.apache.jackrabbit.api.security.user.UserManager;
 import org.apache.jackrabbit.oak.namepath.NamePathMapper;
 import org.apache.jackrabbit.oak.spi.security.principal.PrincipalImpl;
 import org.apache.jackrabbit.oak.spi.security.user.UserConstants;
 import org.jetbrains.annotations.NotNull;
 
+import static org.apache.jackrabbit.oak.security.user.Utils.canImpersonateAllUsers;
+
 /**
  * XPATH based condition visitor.
  */
@@ -98,14 +98,7 @@ class XPathConditionVisitor implements ConditionVisitor {
     @Override
     public void visit(@NotNull Condition.Impersonation condition) {
         String principalName = condition.getName();
-        boolean isAdmin = false;
-        try {
-            Authorizable authorizable = userMgr.getAuthorizable(new PrincipalImpl(principalName));
-            isAdmin = authorizable != null && !authorizable.isGroup() && ((User) authorizable).isAdmin();
-        } catch (RepositoryException e) {
-            // unable to retrieve authorizable
-        }
-        if (isAdmin) {
+        if (canImpersonateAllUsers(new PrincipalImpl(principalName), userMgr)) {
             statement.append('@')
                     .append(QueryUtil.escapeForQuery(JcrConstants.JCR_PRIMARYTYPE, namePathMapper))
                     .append("='")
diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UtilsTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UtilsTest.java
index 6462817d17..6a5df7e4ea 100644
--- a/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UtilsTest.java
+++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UtilsTest.java
@@ -16,29 +16,43 @@
  */
 package org.apache.jackrabbit.oak.security.user;
 
+import java.security.Principal;
 import java.util.Map;
 import com.google.common.collect.ImmutableMap;
+import org.apache.jackrabbit.api.security.principal.GroupPrincipal;
 import org.apache.jackrabbit.api.security.user.Authorizable;
 import org.apache.jackrabbit.api.security.user.Group;
+import org.apache.jackrabbit.api.security.user.User;
+import org.apache.jackrabbit.api.security.user.UserManager;
 import org.apache.jackrabbit.oak.AbstractSecurityTest;
 import org.apache.jackrabbit.oak.api.Root;
 import org.apache.jackrabbit.oak.api.Tree;
 import org.apache.jackrabbit.oak.commons.PathUtils;
 import org.apache.jackrabbit.oak.plugins.tree.TreeAware;
 import org.apache.jackrabbit.oak.spi.nodetype.NodeTypeConstants;
+import org.apache.jackrabbit.oak.spi.security.principal.AdminPrincipal;
 import org.apache.jackrabbit.oak.spi.security.principal.EveryonePrincipal;
+import org.apache.jackrabbit.oak.spi.security.principal.PrincipalImpl;
+import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration;
 import org.jetbrains.annotations.NotNull;
 import org.junit.Test;
 
 import javax.jcr.AccessDeniedException;
 import javax.jcr.RepositoryException;
 
+import static org.apache.jackrabbit.oak.security.user.Utils.canImpersonateAllUsers;
+import static org.apache.jackrabbit.oak.spi.security.user.UserConstants.DEFAULT_ADMIN_ID;
+import static org.apache.jackrabbit.oak.spi.security.user.UserConstants.PARAM_ADMIN_ID;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoInteractions;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
@@ -68,6 +82,13 @@ public class UtilsTest extends AbstractSecurityTest {
     private void assertEqualPath(@NotNull Tree expected, @NotNull Tree result) {
         assertEquals(expected.getPath(), result.getPath());
     }
+    
+    private @NotNull Principal getAdminPrincipal(@NotNull UserManager userManager) throws Exception {
+        String adminId = getConfig(UserConfiguration.class).getParameters().getConfigValue(PARAM_ADMIN_ID, DEFAULT_ADMIN_ID);
+        User admin = userManager.getAuthorizable(adminId, User.class);
+        assertNotNull(admin);
+        return admin.getPrincipal();
+    }
 
     @Test
     public void testGetOrAddTreeCurrentElement() throws Exception {
@@ -157,6 +178,57 @@ public class UtilsTest extends AbstractSecurityTest {
         assertFalse(Utils.isEveryone(a));
     }
     
+    @Test
+    public void testCanImpersonateAllNonExisting() throws Exception {
+        Principal principal = new PrincipalImpl("nonExisting");
+        UserManager userManager = getUserManager(root);
+        
+        assertNull(userManager.getAuthorizable(principal));
+        assertFalse(canImpersonateAllUsers(principal, userManager));
+    }
+    
+    @Test
+    public void testCanImpersonateAllForGroup() throws Exception {
+        Principal principal = new PrincipalImpl("aGroup");
+        Group gr = getUserManager(root).createGroup(principal);
+
+        UserManager userManager = spy(getUserManager(root));
+        assertFalse(canImpersonateAllUsers(principal, userManager));
+    }
+    
+    @Test
+    public void testCanImpersonateAllForAdminUser() throws Exception {
+        UserManager userManager = getUserManager(root);
+        Principal adminPrincipal = getAdminPrincipal(userManager);
+        
+        assertTrue(canImpersonateAllUsers(adminPrincipal, userManager));
+        assertTrue(canImpersonateAllUsers(new PrincipalImpl(adminPrincipal.getName()), userManager));
+        
+        GroupPrincipal gp = when(mock(GroupPrincipal.class).getName()).thenReturn(adminPrincipal.getName()).getMock();
+        assertTrue(canImpersonateAllUsers(gp, userManager));
+    }
+
+    @Test
+    public void testCanImpersonateAllForNonAdminUser() throws Exception {
+        User u = getTestUser();
+
+        UserManager userManager = getUserManager(root);
+        String userPrincipalName = u.getPrincipal().getName();
+
+        assertFalse(canImpersonateAllUsers(u.getPrincipal(), userManager));
+        assertFalse(canImpersonateAllUsers(new PrincipalImpl(userPrincipalName), userManager));
+        assertFalse(canImpersonateAllUsers((AdminPrincipal) () -> userPrincipalName, userManager));
+    }
+    
+    @Test
+    public void testCanImpersonateAllLookupFails() throws Exception {
+        UserManager umgrMock = when(mock(UserManager.class).getAuthorizable(any(Principal.class))).thenThrow(new RepositoryException()).getMock();
+
+        Principal adminPrincipal = getAdminPrincipal(getUserManager(root));
+
+        assertFalse(canImpersonateAllUsers(adminPrincipal, umgrMock));
+    }
+    
     @Test
     public void testGetTreeFromTreeAware() throws Exception {
         Tree t = mock(Tree.class);