Skip to content

Commit

Permalink
OAK-10120 : SessionImpl.hasCapability is prone to NPE, (apache#855)
Browse files Browse the repository at this point in the history 
OAK-10121 : Extend SessionImpl.hasCapability to cover access control write operations
  • Loading branch information
@anchela
anchela authored Feb 22, 2023
1 parent 3bd9e66 commit a3dc6a9
Show file tree
Hide file tree
Showing 2 changed files with 203 additions and 27 deletions.
44 changes: 40 additions & 4 deletions oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/session/SessionImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
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;
Expand All @@ -68,6 +69,7 @@
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;
Expand Down Expand Up @@ -679,6 +681,7 @@ public boolean hasCapability(String methodName, Object target, Object[] argument
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()) {
Expand All @@ -696,17 +699,20 @@ public boolean hasCapability(String methodName, Object target, Object[] argument
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)) {
Expand Down Expand Up @@ -742,11 +748,41 @@ public boolean hasCapability(String methodName, Object target, Object[] argument
&& !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();
Expand Down
186 changes: 163 additions & 23 deletions ...org/apache/jackrabbit/oak/jcr/session/SessionImplCapabilityWithMountInfoProviderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -39,13 +31,34 @@
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();
{
Expand Down Expand Up @@ -78,48 +91,109 @@ public void prepare() throws Exception {
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
public void addNode() throws Exception {

// 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";

Expand All @@ -129,4 +203,70 @@ public void itemOperations() throws Exception {
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));
}
}

0 comments on commit a3dc6a9

Please sign in to comment.