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 a3dc6a9e7f OAK-10120 : SessionImpl.hasCapability is prone to NPE, (#855) a3dc6a9e7f is described below commit a3dc6a9e7ff06e27b3218b5801e7bb28ed6173f1 Author: anchela <ang...@apache.org> AuthorDate: Wed Feb 22 18:20:13 2023 +0100 OAK-10120 : SessionImpl.hasCapability is prone to NPE, (#855) OAK-10121 : Extend SessionImpl.hasCapability to cover access control write operations --- .../jackrabbit/oak/jcr/session/SessionImpl.java | 44 ++++- ...ionImplCapabilityWithMountInfoProviderTest.java | 186 ++++++++++++++++++--- 2 files changed, 203 insertions(+), 27 deletions(-) diff --git a/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/session/SessionImpl.java b/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/session/SessionImpl.java index 41e579766b..23414daf09 100644 --- a/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/session/SessionImpl.java +++ b/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/session/SessionImpl.java @@ -50,6 +50,7 @@ import org.apache.jackrabbit.api.JackrabbitSession; import org.apache.jackrabbit.api.security.principal.PrincipalManager; import org.apache.jackrabbit.api.security.user.UserManager; import org.apache.jackrabbit.api.stats.RepositoryStatistics.Type; +import org.apache.jackrabbit.commons.jackrabbit.authorization.AccessControlUtils; import org.apache.jackrabbit.commons.xml.DocumentViewExporter; import org.apache.jackrabbit.commons.xml.Exporter; import org.apache.jackrabbit.commons.xml.ParsingContentHandler; @@ -68,6 +69,7 @@ import org.apache.jackrabbit.oak.jcr.xml.ImportHandler; import org.apache.jackrabbit.oak.spi.mount.MountInfoProvider; import org.apache.jackrabbit.oak.spi.security.authentication.ImpersonationCredentials; import org.apache.jackrabbit.oak.spi.security.authorization.permission.Permissions; +import org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeConstants; import org.apache.jackrabbit.oak.stats.CounterStats; import org.apache.jackrabbit.util.Text; import org.jetbrains.annotations.NotNull; @@ -679,6 +681,7 @@ public class SessionImpl implements JackrabbitSession { requireNonNull(target, "parameter 'target' must not be null"); checkAlive(); + AccessManager accessMgr = sessionContext.getAccessManager(); if (target instanceof ItemImpl) { ItemDelegate dlg = ((ItemImpl<?>) target).dlg; if (dlg.isProtected()) { @@ -696,17 +699,20 @@ public class SessionImpl implements JackrabbitSession { return false; } - AccessManager accessMgr = sessionContext.getAccessManager(); long permission = Permissions.NO_PERMISSION; if (isNode) { Tree tree = ((NodeDelegate) dlg).getTree(); if ("addNode".equals(methodName)) { - if (arguments != null && arguments.length > 0) { + String relPath = getFirstArgument(arguments); + if (relPath != null) { // add-node needs to be checked on the (path of) the // new node that has/will be added - String path = PathUtils.concat(tree.getPath(), - sessionContext.getOakName(arguments[0].toString())); + String path = PathUtils.concat(tree.getPath(), sessionContext.getOakPathOrThrow(relPath)); return accessMgr.hasPermissions(path, Session.ACTION_ADD_NODE) && !isMountedReadOnly(path); + } else { + // invalid arguments -> cannot verify + log.warn("Cannot verify capability to '{}' due to missing or invalid arguments, required a valid relative path.", methodName); + return false; } } else if ("setPrimaryType".equals(methodName) || "addMixin".equals(methodName) || "removeMixin".equals(methodName)) { @@ -742,11 +748,41 @@ public class SessionImpl implements JackrabbitSession { && !isMountedReadOnly(dlg.getPath()); } } + } else if (target instanceof AccessControlManager && isPolicyWriteMethod(methodName)) { + if (!hasArguments(arguments)) { + log.warn("Cannot verify capability to '{}' due to missing arguments.", methodName); + return false; + } + String path = getFirstArgument(arguments); + if (path == null) { + return getAccessControlManager().hasPrivileges(null, AccessControlUtils.privilegesFromNames(this, PrivilegeConstants.JCR_MODIFY_ACCESS_CONTROL)); + } else { + String oakPath = getOakPathOrThrow(path); + return !isMountedReadOnly(oakPath) && accessMgr.hasPermissions(oakPath, JackrabbitSession.ACTION_MODIFY_ACCESS_CONTROL); + } } // TODO: add more best-effort checks return true; } + private static boolean hasArguments(@Nullable Object[] arguments) { + return arguments != null && arguments.length > 0; + } + + @Nullable + private static String getFirstArgument(@Nullable Object[] arguments) { + if (arguments != null && arguments.length > 0) { + Object arg = arguments[0]; + return (arg == null) ? null : arg.toString(); + } else { + return null; + } + } + + private static boolean isPolicyWriteMethod(@NotNull String methodName) { + return "setPolicy".equals(methodName) || "removePolicy".equals(methodName); + } + private boolean isMountedReadOnly(String path) { MountInfoProvider mip = sessionContext.getMountInfoProvider(); return mip != null && mip.getMountByPath(path).isReadOnly(); diff --git a/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/session/SessionImplCapabilityWithMountInfoProviderTest.java b/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/session/SessionImplCapabilityWithMountInfoProviderTest.java index 578dae8f35..4b2650cc01 100644 --- a/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/session/SessionImplCapabilityWithMountInfoProviderTest.java +++ b/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/session/SessionImplCapabilityWithMountInfoProviderTest.java @@ -16,16 +16,8 @@ */ package org.apache.jackrabbit.oak.jcr.session; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; - -import java.util.Collections; - -import javax.jcr.Repository; -import javax.jcr.Session; -import javax.jcr.SimpleCredentials; - import org.apache.jackrabbit.JcrConstants; +import org.apache.jackrabbit.oak.commons.PathUtils; import org.apache.jackrabbit.oak.composite.CompositeNodeStore; import org.apache.jackrabbit.oak.jcr.Jcr; import org.apache.jackrabbit.oak.plugins.memory.MemoryNodeStore; @@ -39,13 +31,34 @@ import org.apache.jackrabbit.oak.spi.whiteboard.Whiteboard; import org.junit.Before; import org.junit.Test; +import javax.jcr.GuestCredentials; +import javax.jcr.Node; +import javax.jcr.Repository; +import javax.jcr.Session; +import javax.jcr.SimpleCredentials; +import javax.jcr.security.AccessControlManager; +import javax.jcr.security.AccessControlPolicy; +import java.util.Collections; + +import static org.apache.jackrabbit.oak.commons.PathUtils.ROOT_PATH; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verifyNoInteractions; + public class SessionImplCapabilityWithMountInfoProviderTest { + private static final String MOUNT_PATH = "/private"; + private static final String MOUNT_PATH_FOO = "/private/foo"; + private static final String GLOBAL_PATH_FOO = "/foo"; + + private Session adminSession; + private Session guestSession; @Before public void prepare() throws Exception { - MountInfoProvider mip = Mounts.newBuilder().readOnlyMount("ro", "/private").build(); + MountInfoProvider mip = Mounts.newBuilder().readOnlyMount("ro", MOUNT_PATH).build(); MemoryNodeStore roStore = new MemoryNodeStore(); { @@ -78,7 +91,8 @@ public class SessionImplCapabilityWithMountInfoProviderTest { jcr.createContentRepository(); Repository repository = jcr.createRepository(); - adminSession = repository.login(new SimpleCredentials("admin", "admin".toCharArray())); + adminSession = repository.login(new SimpleCredentials("admin", "admin".toCharArray())); + guestSession = repository.login(new GuestCredentials()); } @Test @@ -86,40 +100,100 @@ public class SessionImplCapabilityWithMountInfoProviderTest { // unable to add nodes in the read-only mount assertFalse("Must not be able to add a child not under the private mount root", - adminSession.hasCapability("addNode", adminSession.getNode("/private"), new String[] {"foo"})); + adminSession.hasCapability("addNode", adminSession.getNode(MOUNT_PATH), new String[] {"foo"})); assertFalse("Must not be able to add a child not under the private mount", - adminSession.hasCapability("addNode", adminSession.getNode("/private/foo"), new String[] {"bar"})); + adminSession.hasCapability("addNode", adminSession.getNode(MOUNT_PATH_FOO), new String[] {"bar"})); // able to add nodes outside the read-only mount assertTrue("Must be able to add a child node under the root", - adminSession.hasCapability("addNode", adminSession.getNode("/"), new String[] {"not-private"})); + adminSession.hasCapability("addNode", adminSession.getNode(ROOT_PATH), new String[] {"not-private"})); // unable to add node at the root of the read-only mount ( even though it already exists ) assertFalse("Must not be able to add a child node in place of the private mount", - adminSession.hasCapability("addNode", adminSession.getNode("/"), new String[] {"private"})); + adminSession.hasCapability("addNode", adminSession.getNode(ROOT_PATH), new String[] {"private"})); + } + + @Test + public void addNodeWithRelativePath() throws Exception { + String relativePath = "rel/path/for/add/node"; + // unable to add nodes in the read-only mount + assertFalse("Must not be able to add a child not under the private mount root", + adminSession.hasCapability("addNode", adminSession.getNode(MOUNT_PATH), new String[] {relativePath})); + assertFalse("Must not be able to add a child not under the private mount", + adminSession.hasCapability("addNode", adminSession.getNode(MOUNT_PATH_FOO), new String[] {relativePath})); + // able to add nodes outside the read-only mount + assertTrue("Must be able to add a child node under the root", + adminSession.hasCapability("addNode", adminSession.getNode(ROOT_PATH), new String[] {relativePath})); + // unable to add node at the root of the read-only mount ( even though it already exists ) + assertFalse("Must not be able to add a child node in place of the private mount", + adminSession.hasCapability("addNode", adminSession.getNode(ROOT_PATH), new String[] {"private/rel/path"})); + } + + @Test + public void addNodeWithInvalidPathArgument() throws Exception { + Node n = adminSession.getNode(ROOT_PATH); + assertFalse("Must not be able to add a child if invalid path argument is passed", + adminSession.hasCapability("addNode", n, new String[] {null})); + assertFalse("Must not be able to add a child if no path argument is passed", + adminSession.hasCapability("addNode", n, new String[] {})); + assertFalse("Must not be able to add a child if no path argument is passed", + adminSession.hasCapability("addNode", n, null)); + } + + + @Test + public void addNodeMissingPermissions() throws Exception { + // FIXME: guest does not have access to the root node in the test setup + Node n = adminSession.getNode(ROOT_PATH); + // unable to add nodes outside the read-only mount + assertFalse("Must be not able to add a child node under the root", + guestSession.hasCapability("addNode", n, new String[] {"not-private"})); + // unable to add node at the root of the read-only mount ( even though it already exists ) + assertFalse("Must not be able to add a child node in place of the private mount", + guestSession.hasCapability("addNode", n, new String[] {"private"})); } @Test public void orderBefore() throws Exception { // able to order the root of the mount since the operation is performed on the parent - assertTrue(adminSession.hasCapability("orderBefore", adminSession.getNode("/private"), null)); - assertFalse(adminSession.hasCapability("orderBefore", adminSession.getNode("/private/foo"), null)); + assertTrue(adminSession.hasCapability("orderBefore", adminSession.getNode(MOUNT_PATH), null)); + assertFalse(adminSession.hasCapability("orderBefore", adminSession.getNode(MOUNT_PATH_FOO), null)); + // root node can never be reordered + assertFalse(adminSession.hasCapability("orderBefore", adminSession.getNode(ROOT_PATH), null)); } @Test public void simpleNodeOperations() throws Exception { for ( String operation : new String[] { "setPrimaryType", "addMixin", "removeMixin" , "setProperty", "remove"} ) { - for ( String privateMountNode : new String[] { "/private", "/private/foo" } ) { + for ( String privateMountNode : new String[] { MOUNT_PATH, MOUNT_PATH_FOO } ) { assertFalse("Unexpected return value for hasCapability(" + operation+ ") on node '" + privateMountNode +"' from the private mount", adminSession.hasCapability(operation, adminSession.getNode(privateMountNode), null)); } - String globalMountNode = "/foo"; - assertTrue("Unexpected return value for hasCapability(" + operation+ ") on node '" + globalMountNode +"' from the global mount", - adminSession.hasCapability(operation, adminSession.getNode(globalMountNode), null)); + assertTrue("Unexpected return value for hasCapability(" + operation+ ") on node '" + GLOBAL_PATH_FOO +"' from the global mount", + adminSession.hasCapability(operation, adminSession.getNode(GLOBAL_PATH_FOO), null)); } - } + } + + @Test + public void simpleNodeOperationsMissingPermission() throws Exception { + // FIXME: guest does not have access to the root node in the test setup + Node n = adminSession.getNode(ROOT_PATH); + for ( String operation : new String[] { "setPrimaryType", "addMixin", "removeMixin" , "setProperty", "remove"} ) { + assertFalse("Unexpected return value for hasCapability(" + operation+ ") on node '" + ROOT_PATH +"' from the global mount", + guestSession.hasCapability(operation, n, null)); + } + } + + @Test + public void uncoveredNodeOperation() throws Exception { + String unknownOperation = "unknown"; + assertFalse("Unexpected return value for hasCapability('+unknownOperation+') on node '/private' from the private mount.", + adminSession.hasCapability(unknownOperation, adminSession.getNode(MOUNT_PATH), null)); + assertTrue("Unexpected return value for hasCapability('+unknownOperation+') on node '/foo' from the global mount.", + adminSession.hasCapability(unknownOperation, adminSession.getNode(GLOBAL_PATH_FOO), null)); + } @Test public void itemOperations() throws Exception { - for ( String operation : new String[] { "setValue", "remove"} ) { + for ( String operation : new String[] { "setValue", "remove", "unknown"} ) { String privateMountProp = "/private/prop"; String globalMountProp = "/foo/prop"; @@ -129,4 +203,70 @@ public class SessionImplCapabilityWithMountInfoProviderTest { adminSession.hasCapability(operation, adminSession.getItem(globalMountProp), null)); } } + + @Test + public void policyWriteOperation() throws Exception { + AccessControlManager acMgr = adminSession.getAccessControlManager(); + AccessControlPolicy policy = mock(AccessControlPolicy.class); + for (String operation : new String[]{"setPolicy", "removePolicy"}) { + assertFalse("Unexpected return value for hasCapability(" + operation + ") on the private mount", + adminSession.hasCapability(operation, acMgr, new Object[] {MOUNT_PATH_FOO, policy})); + assertTrue("Unexpected return value for hasCapability(" + operation + ") on the global mount", + adminSession.hasCapability(operation, acMgr, new Object[] {GLOBAL_PATH_FOO, policy})); + } + verifyNoInteractions(policy); + } + + @Test + public void policyWriteOperationNullPath() throws Exception { + AccessControlManager acMgr = adminSession.getAccessControlManager(); + AccessControlPolicy policy = mock(AccessControlPolicy.class); + for (String operation : new String[]{"setPolicy", "removePolicy"}) { + assertTrue("Unexpected return value for hasCapability(" + operation +") on the global mount (null path)", + adminSession.hasCapability(operation, acMgr, new Object[] {null, policy})); + } + verifyNoInteractions(policy); + } + + @Test + public void policyWriteOperationMissingPermission() throws Exception { + AccessControlManager acMgr = guestSession.getAccessControlManager(); + AccessControlPolicy policy = mock(AccessControlPolicy.class); + for (String operation : new String[]{"setPolicy", "removePolicy"}) { + assertFalse("Unexpected return value for hasCapability(" + operation + ") on the private mount", + guestSession.hasCapability(operation, acMgr, new Object[] {MOUNT_PATH_FOO, policy})); + assertFalse("Unexpected return value for hasCapability(" + operation + ") on the global mount", + guestSession.hasCapability(operation, acMgr, new Object[] {GLOBAL_PATH_FOO, policy})); + } + verifyNoInteractions(policy); + } + + @Test + public void policyWriteOperationMissingArguments() throws Exception { + AccessControlManager acMgr = guestSession.getAccessControlManager(); + for (String operation : new String[]{"setPolicy", "removePolicy"}) { + // missing arguments => cannot perform capability check + assertFalse("Unexpected return value for hasCapability(" + operation + ") with insufficient arguments.", + guestSession.hasCapability(operation, acMgr, new Object[0])); + assertFalse("Unexpected return value for hasCapability(" + operation + ") with null arguments.", + guestSession.hasCapability(operation, acMgr, null)); + } + } + + @Test + public void uncoveredAccessControlMethod() throws Exception { + AccessControlManager acMgr = guestSession.getAccessControlManager(); + AccessControlPolicy policy = mock(AccessControlPolicy.class); + for (String operation : new String[]{"getApplicablePolicies", "getPolicies", "getEffectivePolicies"}) { + assertTrue("Unexpected return value for hasCapability(" + operation + ").", + guestSession.hasCapability(operation, acMgr, new Object[] {GLOBAL_PATH_FOO, policy})); + } + verifyNoInteractions(policy); + } + + @Test + public void uncoveredTarget() throws Exception { + assertTrue("Default return value for unsupported method/target object must be true.", + guestSession.hasCapability("unknownMethod", new Object(), null)); + } }