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 2023/02/22 17:20:19 UTC
[jackrabbit-oak] branch trunk updated: OAK-10120 : SessionImpl.hasCapability is prone to NPE, (#855)
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 <an...@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));
+ }
}