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 2019/06/06 14:15:01 UTC
svn commit: r1860716 - in /jackrabbit/oak/trunk/oak-core/src:
main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/
test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/
Author: angela
Date: Thu Jun 6 14:15:01 2019
New Revision: 1860716
URL: http://svn.apache.org/viewvc?rev=1860716&view=rev
Log:
OAK-8389 : AccessControlValidator prone to NPE
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidatorTest.java
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java?rev=1860716&r1=1860715&r2=1860716&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java Thu Jun 6 14:15:01 2019
@@ -23,6 +23,7 @@ import javax.jcr.RepositoryException;
import javax.jcr.security.AccessControlException;
import javax.jcr.security.Privilege;
+import com.google.common.base.Strings;
import com.google.common.collect.Iterables;
import com.google.common.collect.Sets;
import org.apache.jackrabbit.JcrConstants;
@@ -30,7 +31,6 @@ import org.apache.jackrabbit.api.securit
import org.apache.jackrabbit.oak.api.CommitFailedException;
import org.apache.jackrabbit.oak.api.PropertyState;
import org.apache.jackrabbit.oak.api.Tree;
-import org.apache.jackrabbit.oak.api.Type;
import org.apache.jackrabbit.oak.plugins.nodetype.TypePredicate;
import org.apache.jackrabbit.oak.plugins.tree.TreeConstants;
import org.apache.jackrabbit.oak.plugins.tree.TreeProvider;
@@ -236,20 +236,19 @@ class AccessControlValidator extends Def
checkValidRestrictions(aceNode);
}
- private void checkValidPrincipal(@NotNull Tree aceNode) throws CommitFailedException {
+ @NotNull
+ private String checkValidPrincipal(@NotNull Tree aceNode) throws CommitFailedException {
String principalName = TreeUtil.getString(aceNode, REP_PRINCIPAL_NAME);
- if (principalName == null || principalName.isEmpty()) {
+ if (Strings.isNullOrEmpty(principalName)) {
throw accessViolation(8, "Missing principal name at " + aceNode.getPath());
}
// validity of principal is only a JCR specific contract and will not be
// enforced on the oak level.
+ return principalName;
}
private void checkValidPrivileges(@NotNull Tree aceNode) throws CommitFailedException {
- Iterable<String> privilegeNames = TreeUtil.getStrings(aceNode, REP_PRIVILEGES);
- if (privilegeNames == null || Iterables.isEmpty(privilegeNames)) {
- throw accessViolation(9, "Missing privileges at " + aceNode.getPath());
- }
+ Iterable<String> privilegeNames = getPrivilegeNames(aceNode);
for (String privilegeName : privilegeNames) {
try {
Privilege privilege = privilegeManager.getPrivilege(privilegeName);
@@ -264,6 +263,15 @@ class AccessControlValidator extends Def
}
}
+ @NotNull
+ private Iterable<String> getPrivilegeNames(@NotNull Tree aceNode) throws CommitFailedException {
+ Iterable<String> privilegeNames = TreeUtil.getNames(aceNode, REP_PRIVILEGES);
+ if (Iterables.isEmpty(privilegeNames)) {
+ throw accessViolation(9, "Missing privileges at " + aceNode.getPath());
+ }
+ return privilegeNames;
+ }
+
private void checkValidRestrictions(@NotNull Tree aceTree) throws CommitFailedException {
String path;
Tree aclTree = checkNotNull(aceTree.getParent());
@@ -290,20 +298,21 @@ class AccessControlValidator extends Def
}
}
- private static void checkValidRepoAccessControlled(Tree accessControlledTree) throws CommitFailedException {
+ private static void checkValidRepoAccessControlled(@NotNull Tree accessControlledTree) throws CommitFailedException {
if (!accessControlledTree.isRoot()) {
throw accessViolation(12, "Only root can store repository level policies (" + accessControlledTree.getPath() + ')');
}
}
+ @NotNull
private static CommitFailedException accessViolation(int code, String message) {
return new CommitFailedException(ACCESS_CONTROL, code, message);
}
- private ValidationEntry createAceEntry(@Nullable String path, @NotNull Tree aceTree) {
- String principalName = aceTree.getProperty(REP_PRINCIPAL_NAME).getValue(Type.STRING);
- PrivilegeBits privilegeBits = privilegeBitsProvider.getBits(aceTree.getProperty(REP_PRIVILEGES).getValue(Type.NAMES));
-
+ @NotNull
+ private ValidationEntry createAceEntry(@Nullable String path, @NotNull Tree aceTree) throws CommitFailedException {
+ String principalName = checkValidPrincipal(aceTree);
+ PrivilegeBits privilegeBits = privilegeBitsProvider.getBits(getPrivilegeNames(aceTree));
boolean isAllow = NT_REP_GRANT_ACE.equals(TreeUtil.getPrimaryTypeName(aceTree));
Set<Restriction> restrictions = restrictionProvider.readRestrictions(path, aceTree);
return new ValidationEntry(principalName, privilegeBits, isAllow, restrictions);
Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidatorTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidatorTest.java?rev=1860716&r1=1860715&r2=1860716&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidatorTest.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidatorTest.java Thu Jun 6 14:15:01 2019
@@ -16,6 +16,7 @@
*/
package org.apache.jackrabbit.oak.security.authorization.accesscontrol;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import org.apache.jackrabbit.JcrConstants;
@@ -25,6 +26,7 @@ import org.apache.jackrabbit.commons.jac
import org.apache.jackrabbit.oak.AbstractSecurityTest;
import org.apache.jackrabbit.oak.api.CommitFailedException;
import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.api.Type;
import org.apache.jackrabbit.oak.commons.PathUtils;
import org.apache.jackrabbit.oak.plugins.memory.MemoryNodeStore;
import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
@@ -57,6 +59,8 @@ import javax.jcr.ValueFactory;
import javax.jcr.security.AccessControlManager;
import java.security.Principal;
+import static org.apache.jackrabbit.JcrConstants.JCR_MIXINTYPES;
+import static org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeConstants.JCR_READ;
import static org.hamcrest.CoreMatchers.containsString;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
@@ -90,6 +94,7 @@ public class AccessControlValidatorTest
@After
public void after() throws Exception {
try {
+ root.refresh();
Tree testRoot = root.getTree(testPath);
if (testRoot.exists()) {
testRoot.remove();
@@ -109,6 +114,25 @@ public class AccessControlValidatorTest
return new AccessControlValidatorProvider((AuthorizationConfigurationImpl) cac.getDefaultConfig());
}
+ @NotNull
+ private Validator createRootValidator(@NotNull Tree rootTree) {
+ NodeState ns = getTreeProvider().asNodeState(rootTree);
+ return createValidatorProvider().getRootValidator(ns, ns, new CommitInfo("sid", null));
+ }
+
+ @NotNull
+ private Tree createPolicy(@NotNull Tree tree, boolean createRestrictionNode) throws AccessDeniedException {
+ tree.setProperty(JCR_MIXINTYPES, ImmutableList.of(MIX_REP_ACCESS_CONTROLLABLE), Type.NAMES);
+
+ Tree acl = TreeUtil.addChild(tree, REP_POLICY, NT_REP_ACL);
+ acl.setOrderableChildren(true);
+ Tree ace = createACE(acl, aceName, NT_REP_GRANT_ACE, testPrincipal.getName(), JCR_READ);
+ if (createRestrictionNode) {
+ TreeUtil.addChild(ace, REP_RESTRICTIONS, NT_REP_RESTRICTIONS);
+ }
+ return acl;
+ }
+
private NodeUtil createAcl() throws AccessDeniedException {
NodeUtil testRoot = getTestRoot();
testRoot.setNames(JcrConstants.JCR_MIXINTYPES, MIX_REP_ACCESS_CONTROLLABLE);
@@ -126,6 +150,20 @@ public class AccessControlValidatorTest
return ace;
}
+ @NotNull
+ private static Tree createACE(@NotNull Tree acl, @NotNull String aceName, @NotNull String ntName, @NotNull String principalName, @NotNull String... privilegeNames) throws AccessDeniedException {
+ Tree ace = TreeUtil.addChild(acl, aceName, ntName);
+ ace.setProperty(REP_PRINCIPAL_NAME, principalName);
+ ace.setProperty(REP_PRIVILEGES, ImmutableList.copyOf(privilegeNames), Type.NAMES);
+ return ace;
+ }
+
+ private static CommitFailedException assertCommitFailedException(@NotNull CommitFailedException e, @NotNull String type, int expectedCode) {
+ assertTrue(e.isOfType(type));
+ assertEquals(expectedCode, e.getCode());
+ return e;
+ }
+
@Test
public void testPolicyWithOutChildOrder() throws AccessDeniedException {
NodeUtil testRoot = getTestRoot();
@@ -550,4 +588,66 @@ public class AccessControlValidatorTest
root.refresh();
}
}
+
+
+
+ @Test(expected = CommitFailedException.class)
+ public void testAddEntyWithEmptyPrivileges() throws Exception {
+ Tree rootTree = root.getTree(PathUtils.ROOT_PATH);
+ Tree policy = createPolicy(rootTree, false);
+ Tree entry = policy.getChild(aceName);
+ entry.setProperty(REP_PRIVILEGES, ImmutableList.of(), Type.NAMES);
+
+ Validator v = createRootValidator(rootTree);
+ try {
+ v.childNodeAdded(policy.getName(), getTreeProvider().asNodeState(policy)).childNodeAdded(entry.getName(), getTreeProvider().asNodeState(entry));
+ } catch (CommitFailedException e) {
+ throw assertCommitFailedException(e, CommitFailedException.ACCESS_CONTROL, 9);
+ }
+ }
+
+ @Test(expected = CommitFailedException.class)
+ public void testAddEntyWithNullrivileges() throws Exception {
+ Tree rootTree = root.getTree(PathUtils.ROOT_PATH);
+ Tree policy = createPolicy(rootTree, false);
+ Tree entry = policy.getChild(aceName);
+ entry.removeProperty(REP_PRIVILEGES);
+
+ Validator v = createRootValidator(rootTree);
+ try {
+ v.childNodeAdded(policy.getName(), getTreeProvider().asNodeState(policy)).childNodeAdded(entry.getName(), getTreeProvider().asNodeState(entry));
+ } catch (CommitFailedException e) {
+ throw assertCommitFailedException(e, CommitFailedException.ACCESS_CONTROL, 9);
+ }
+ }
+
+ @Test(expected = CommitFailedException.class)
+ public void testAddEntyWithEmptyPrincipalName() throws Exception {
+ Tree rootTree = root.getTree(PathUtils.ROOT_PATH);
+ Tree policy = createPolicy(rootTree, false);
+ Tree entry = policy.getChild(aceName);
+ entry.setProperty(REP_PRINCIPAL_NAME, "");
+
+ Validator v = createRootValidator(rootTree);
+ try {
+ v.childNodeAdded(policy.getName(), getTreeProvider().asNodeState(policy)).childNodeAdded(entry.getName(), getTreeProvider().asNodeState(entry));
+ } catch (CommitFailedException e) {
+ throw assertCommitFailedException(e, CommitFailedException.ACCESS_CONTROL, 8);
+ }
+ }
+
+ @Test(expected = CommitFailedException.class)
+ public void testAddEntyWithNullPrincipalName() throws Exception {
+ Tree rootTree = root.getTree(PathUtils.ROOT_PATH);
+ Tree policy = createPolicy(rootTree, false);
+ Tree entry = policy.getChild(aceName);
+ entry.removeProperty(REP_PRINCIPAL_NAME);
+
+ Validator v = createRootValidator(rootTree);
+ try {
+ v.childNodeAdded(policy.getName(), getTreeProvider().asNodeState(policy)).childNodeAdded(entry.getName(), getTreeProvider().asNodeState(entry));
+ } catch (CommitFailedException e) {
+ throw assertCommitFailedException(e, CommitFailedException.ACCESS_CONTROL, 8);
+ }
+ }
}
\ No newline at end of file