diff --git oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java index e1cdbda..f79e60b 100644 --- oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java +++ oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java @@ -168,15 +168,15 @@ class AccessControlValidator extends DefaultValidator implements AccessControlCo private static boolean isAccessControlEntry(Tree tree) { String ntName = TreeUtil.getPrimaryTypeName(tree); return NT_REP_DENY_ACE.equals(ntName) || NT_REP_GRANT_ACE.equals(ntName); } private static void checkIsAccessControlEntry(Tree tree) throws CommitFailedException { if (!isAccessControlEntry(tree)) { - throw accessViolation(2, "Access control entry node expected."); + throw accessViolation(2, "Access control entry node expected at " + tree.getPath()); } } private void checkValidPolicy(Tree parent, Tree policyTree, NodeState policyNode) throws CommitFailedException { if (REP_REPO_POLICY.equals(policyTree.getName())) { checkValidAccessControlledNode(parent, isRepoAccessControllable); checkValidRepoAccessControlled(parent); @@ -184,26 +184,26 @@ class AccessControlValidator extends DefaultValidator implements AccessControlCo checkValidAccessControlledNode(parent, isAccessControllable); } Collection validPolicyNames = (parent.isRoot()) ? POLICY_NODE_NAMES : Collections.singleton(REP_POLICY); if (!validPolicyNames.contains(policyTree.getName())) { - throw accessViolation(3, "Invalid policy name " + policyTree.getName()); + throw accessViolation(3, "Invalid policy name " + policyTree.getName() + " at " + parent.getPath()); } if (!policyNode.hasProperty(TreeConstants.OAK_CHILD_ORDER)) { - throw accessViolation(4, "Invalid policy node: Order of children is not stable."); + throw accessViolation(4, "Invalid policy node at " + policyTree.getPath() + ": Order of children is not stable."); } Set aceSet = Sets.newHashSet(); for (Tree child : policyTree.getChildren()) { if (isAccessControlEntry(child)) { if (!aceSet.add(new Entry(parent.getPath(), child))) { - throw accessViolation(13, "Duplicate ACE found in policy"); + throw accessViolation(13, "Duplicate ACE '" + child.getPath() + "' found in policy"); } } } } private static void checkValidAccessControlledNode(@Nonnull Tree accessControlledTree, @Nonnull TypePredicate requiredMixin) throws CommitFailedException { @@ -219,46 +219,48 @@ class AccessControlValidator extends DefaultValidator implements AccessControlCo } private void checkValidAccessControlEntry(@Nonnull Tree aceNode) throws CommitFailedException { Tree parent = aceNode.getParent(); if (!parent.exists() || !NT_REP_ACL.equals(TreeUtil.getPrimaryTypeName(parent))) { throw accessViolation(7, "Isolated access control entry at " + aceNode.getPath()); } - checkValidPrincipal(TreeUtil.getString(aceNode, REP_PRINCIPAL_NAME)); - checkValidPrivileges(TreeUtil.getStrings(aceNode, REP_PRIVILEGES)); + checkValidPrincipal(aceNode); + checkValidPrivileges(aceNode); checkValidRestrictions(aceNode); } - private void checkValidPrincipal(@CheckForNull String principalName) throws CommitFailedException { + private void checkValidPrincipal(@Nonnull Tree aceNode) throws CommitFailedException { + String principalName = TreeUtil.getString(aceNode, REP_PRINCIPAL_NAME); if (principalName == null || principalName.isEmpty()) { - throw accessViolation(8, "Missing principal name."); + 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. } - private void checkValidPrivileges(Iterable privilegeNames) throws CommitFailedException { + private void checkValidPrivileges(@Nonnull Tree aceNode) throws CommitFailedException { + Iterable privilegeNames = TreeUtil.getStrings(aceNode, REP_PRIVILEGES); if (privilegeNames == null || Iterables.isEmpty(privilegeNames)) { - throw accessViolation(9, "Missing privileges."); + throw accessViolation(9, "Missing privileges at " + aceNode.getPath()); } for (String privilegeName : privilegeNames) { try { Privilege privilege = privilegeManager.getPrivilege(privilegeName); if (privilege.isAbstract()) { - throw accessViolation(11, "Abstract privilege " + privilegeName); + throw accessViolation(11, "Abstract privilege " + privilegeName + " at " + aceNode.getPath()); } } catch (AccessControlException e) { - throw accessViolation(10, "Invalid privilege " + privilegeName); + throw accessViolation(10, "Invalid privilege " + privilegeName + " at " + aceNode.getPath()); } catch (RepositoryException e) { throw new IllegalStateException("Failed to read privileges", e); } } } - private void checkValidRestrictions(Tree aceTree) throws CommitFailedException { + private void checkValidRestrictions(@Nonnull Tree aceTree) throws CommitFailedException { String path; Tree aclTree = checkNotNull(aceTree.getParent()); String aclPath = aclTree.getPath(); if (REP_REPO_POLICY.equals(Text.getName(aclPath))) { path = null; } else { path = Text.getRelativeParent(aclPath, 1); @@ -278,15 +280,15 @@ class AccessControlValidator extends DefaultValidator implements AccessControlCo if (Iterables.contains(mixinNames, MIX_REP_REPO_ACCESS_CONTROLLABLE)) { checkValidRepoAccessControlled(parentTree); } } private static void checkValidRepoAccessControlled(Tree accessControlledTree) throws CommitFailedException { if (!accessControlledTree.isRoot()) { - throw accessViolation(12, "Only root can store repository level policies."); + throw accessViolation(12, "Only root can store repository level policies (" + accessControlledTree.getPath() + ')'); } } private static CommitFailedException accessViolation(int code, String message) { return new CommitFailedException(ACCESS_CONTROL, code, message); } diff --git oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidatorTest.java oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidatorTest.java index bbd6bba..67e447c 100644 --- oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidatorTest.java +++ oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidatorTest.java @@ -34,17 +34,18 @@ import org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeConstants; import org.apache.jackrabbit.oak.spi.state.NodeBuilder; import org.apache.jackrabbit.oak.spi.state.NodeState; import org.apache.jackrabbit.oak.util.NodeUtil; import org.junit.After; import org.junit.Before; import org.junit.Test; -import static org.junit.Assert.assertEquals; +import static org.hamcrest.CoreMatchers.containsString; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; public class AccessControlValidatorTest extends AbstractAccessControlTest implements AccessControlConstants { private final String testName = "testRoot"; private final String testPath = '/' + testName; @@ -106,43 +107,46 @@ public class AccessControlValidatorTest extends AbstractAccessControlTest implem try { root.commit(); fail("Policy node with child node ordering"); } catch (CommitFailedException e) { // success assertTrue(e.isAccessControlViolation()); - assertEquals("OakAccessControl0004: Invalid policy node: Order of children is not stable.", e.getMessage()); + assertThat(e.getMessage(), containsString("OakAccessControl0004")); // Order of children is not stable + assertThat(e.getMessage(), containsString("/testRoot/rep:policy")); } } @Test public void testOnlyRootIsRepoAccessControllable() { NodeUtil testRoot = getTestRoot(); testRoot.setNames(JcrConstants.JCR_MIXINTYPES, MIX_REP_REPO_ACCESS_CONTROLLABLE); try { root.commit(); fail("Only the root node can be made RepoAccessControllable."); } catch (CommitFailedException e) { // success assertTrue(e.isAccessControlViolation()); + assertThat(e.getMessage(), containsString("/testRoot")); } } @Test public void testAddInvalidRepoPolicy() throws Exception { NodeUtil testRoot = getTestRoot(); testRoot.setNames(JcrConstants.JCR_MIXINTYPES, MIX_REP_ACCESS_CONTROLLABLE); NodeUtil policy = getTestRoot().addChild(REP_REPO_POLICY, NT_REP_ACL); try { root.commit(); fail("Attempt to add repo-policy with rep:AccessControllable node."); } catch (CommitFailedException e) { // success assertTrue(e.isAccessControlViolation()); + assertThat(e.getMessage(), containsString("/testRoot")); } finally { policy.getTree().remove(); } } @Test public void testAddPolicyWithAcContent() throws Exception { @@ -154,14 +158,15 @@ public class AccessControlValidatorTest extends AbstractAccessControlTest implem NodeUtil policy = node.addChild(REP_POLICY, NT_REP_ACL); try { root.commit(); fail("Adding an ACL below access control content should fail"); } catch (CommitFailedException e) { // success assertTrue(e.isConstraintViolation()); + assertThat(e.getMessage(), containsString("/testRoot/rep:policy")); } finally { policy.getTree().remove(); } } } @Test @@ -174,14 +179,15 @@ public class AccessControlValidatorTest extends AbstractAccessControlTest implem NodeUtil policy = node.addChild(REP_REPO_POLICY, NT_REP_ACL); try { root.commit(); fail("Adding an ACL below access control content should fail"); } catch (CommitFailedException e) { // success assertTrue(e.isConstraintViolation()); + assertThat(e.getMessage(), containsString("/testRoot/rep:policy")); } finally { policy.getTree().remove(); } } } @Test @@ -194,14 +200,15 @@ public class AccessControlValidatorTest extends AbstractAccessControlTest implem NodeUtil entry = node.addChild("invalidACE", NT_REP_DENY_ACE); try { root.commit(); fail("Adding an ACE below an ACE or restriction should fail"); } catch (CommitFailedException e) { // success assertTrue(e.isConstraintViolation()); + assertThat(e.getMessage(), containsString("/testRoot/rep:policy/validAce")); } finally { entry.getTree().remove(); } } } @Test @@ -214,14 +221,15 @@ public class AccessControlValidatorTest extends AbstractAccessControlTest implem NodeUtil entry = node.addChild("invalidRestriction", NT_REP_RESTRICTIONS); try { root.commit(); fail("Adding an ACE below an ACE or restriction should fail"); } catch (CommitFailedException e) { // success assertTrue(e.isConstraintViolation()); + assertThat(e.getMessage(), containsString("/testRoot/rep:policy")); } finally { entry.getTree().remove(); } } } @Test @@ -233,14 +241,15 @@ public class AccessControlValidatorTest extends AbstractAccessControlTest implem NodeUtil policy = node.addChild(policyName, NT_REP_ACL); try { root.commit(); fail("Writing an isolated ACL without the parent being rep:AccessControllable should fail."); } catch (CommitFailedException e) { // success assertTrue(e.isAccessControlViolation()); + assertThat(e.getMessage(), containsString("/testRoot")); } finally { // revert pending changes that cannot be saved. policy.getTree().remove(); } } } @@ -254,14 +263,15 @@ public class AccessControlValidatorTest extends AbstractAccessControlTest implem NodeUtil ace = createACE(node, "isolatedACE", aceNtName, testPrincipal.getName(), PrivilegeConstants.JCR_READ); try { root.commit(); fail("Writing an isolated ACE should fail."); } catch (CommitFailedException e) { // success assertTrue(e.isAccessControlViolation()); + assertThat(e.getMessage(), containsString("/testRoot/isolatedACE")); } finally { // revert pending changes that cannot be saved. ace.getTree().remove(); } } } @@ -271,14 +281,15 @@ public class AccessControlValidatorTest extends AbstractAccessControlTest implem NodeUtil restriction = node.addChild("isolatedRestriction", NT_REP_RESTRICTIONS); try { root.commit(); fail("Writing an isolated Restriction should fail."); } catch (CommitFailedException e) { // success assertTrue(e.isAccessControlViolation()); + assertThat(e.getMessage(), containsString("/testRoot")); } finally { // revert pending changes that cannot be saved. restriction.getTree().remove(); } } @Test @@ -289,14 +300,15 @@ public class AccessControlValidatorTest extends AbstractAccessControlTest implem createACE(acl, "invalid", NT_REP_GRANT_ACE, testPrincipal.getName(), privName); try { root.commit(); fail("Creating an ACE with invalid privilege should fail."); } catch (CommitFailedException e) { // success assertTrue(e.isAccessControlViolation()); + assertThat(e.getMessage(), containsString("/testRoot/rep:policy")); } } @Test public void testAbstractPrivilege() throws Exception { PrivilegeManager pMgr = getPrivilegeManager(root); pMgr.registerPrivilege("abstractPrivilege", true, new String[0]); @@ -305,40 +317,43 @@ public class AccessControlValidatorTest extends AbstractAccessControlTest implem createACE(acl, "invalid", NT_REP_GRANT_ACE, testPrincipal.getName(), "abstractPrivilege"); try { root.commit(); fail("Creating an ACE with an abstract privilege should fail."); } catch (CommitFailedException e) { // success assertTrue(e.isAccessControlViolation()); + assertThat(e.getMessage(), containsString("/testRoot/rep:policy")); } } @Test public void testInvalidRestriction() throws Exception { NodeUtil restriction = createAcl().getChild(aceName).getChild(REP_RESTRICTIONS); restriction.setString("invalid", "value"); try { root.commit(); fail("Creating an unsupported restriction should fail."); } catch (CommitFailedException e) { // success assertTrue(e.isAccessControlViolation()); + assertThat(e.getMessage(), containsString("/testRoot/rep:policy")); } } @Test public void testRestrictionWithInvalidType() throws Exception { NodeUtil restriction = createAcl().getChild(aceName).getChild(REP_RESTRICTIONS); restriction.setName(REP_GLOB, "rep:glob"); try { root.commit(); fail("Creating restriction with invalid type should fail."); } catch (CommitFailedException e) { // success assertTrue(e.isAccessControlViolation()); + assertThat(e.getMessage(), containsString("/testRoot/rep:policy")); } } @Test public void testDuplicateAce() throws Exception { AccessControlManager acMgr = getAccessControlManager(root); JackrabbitAccessControlList acl = org.apache.jackrabbit.commons.jackrabbit.authorization.AccessControlUtils.getAccessControlList(acMgr, testPath); @@ -352,14 +367,15 @@ public class AccessControlValidatorTest extends AbstractAccessControlTest implem ace.setNames(AccessControlConstants.REP_PRIVILEGES, PrivilegeConstants.JCR_ADD_CHILD_NODES); try { root.commit(); fail("Creating duplicate ACE must be detected"); } catch (CommitFailedException e) { assertTrue(e.isAccessControlViolation()); + assertThat(e.getMessage(), containsString("/testRoot/rep:policy/duplicateAce")); } } @Test public void hiddenNodeAdded() throws CommitFailedException { AccessControlValidatorProvider provider = new AccessControlValidatorProvider(getSecurityProvider()); MemoryNodeStore store = new MemoryNodeStore();