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 re...@apache.org on 2021/11/10 10:24:03 UTC
[jackrabbit-oak] branch 1.6 updated: OAK-5887 Stricter validation
on primary type change
This is an automated email from the ASF dual-hosted git repository.
reschke pushed a commit to branch 1.6
in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
The following commit(s) were added to refs/heads/1.6 by this push:
new 6c031b0 OAK-5887 Stricter validation on primary type change
6c031b0 is described below
commit 6c031b0a0855a7375b10148b63ed5614eca636f6
Author: Alexandru Parvulescu <al...@apache.org>
AuthorDate: Fri Mar 3 12:53:21 2017 +0000
OAK-5887 Stricter validation on primary type change
git-svn-id: https://svn.apache.org/repos/asf/jackrabbit/oak/trunk@1785287 13f79535-47bb-0310-9956-ffa450edef68
---
.../authorization/cug/impl/CugValidatorTest.java | 2 +
.../oak/plugins/nodetype/TypeEditor.java | 150 ++++++++++++++-------
.../oak/plugins/nodetype/TypeEditorTest.java | 95 +++++++++++++
.../apache/jackrabbit/oak/jcr/RepositoryTest.java | 2 +-
.../authorization/NodeTypeManagementTest.java | 4 +-
5 files changed, 205 insertions(+), 48 deletions(-)
diff --git a/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugValidatorTest.java b/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugValidatorTest.java
index c9f158a..78b0aab 100644
--- a/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugValidatorTest.java
+++ b/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugValidatorTest.java
@@ -48,8 +48,10 @@ public class CugValidatorTest extends AbstractCugTest {
@Test
public void testChangePrimaryType() {
+ node = new NodeUtil(root.getTree(SUPPORTED_PATH2));
try {
node.setName(JcrConstants.JCR_PRIMARYTYPE, NT_REP_CUG_POLICY);
+ node.setStrings(REP_PRINCIPAL_NAMES, EveryonePrincipal.NAME);
root.commit();
fail();
} catch (CommitFailedException e) {
diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditor.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditor.java
index f733066..91c1aff 100644
--- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditor.java
+++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditor.java
@@ -36,6 +36,7 @@ import static org.apache.jackrabbit.oak.plugins.memory.EmptyNodeState.MISSING_NO
import static org.apache.jackrabbit.oak.plugins.nodetype.NodeTypeConstants.JCR_IS_ABSTRACT;
import static org.apache.jackrabbit.oak.plugins.nodetype.constraint.Constraints.valueConstraint;
+import java.util.Collections;
import java.util.List;
import java.util.Set;
@@ -44,7 +45,9 @@ import javax.annotation.Nonnull;
import javax.jcr.PropertyType;
import javax.jcr.Value;
+import com.google.common.base.Objects;
import com.google.common.base.Predicate;
+import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import org.apache.jackrabbit.oak.api.CommitFailedException;
import org.apache.jackrabbit.oak.api.PropertyState;
@@ -55,6 +58,7 @@ import org.apache.jackrabbit.oak.spi.commit.DefaultEditor;
import org.apache.jackrabbit.oak.spi.commit.Editor;
import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
import org.apache.jackrabbit.oak.spi.state.NodeState;
+import org.apache.jackrabbit.oak.spi.state.NodeStateUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -75,7 +79,7 @@ class TypeEditor extends DefaultEditor {
private final Set<String> typesToCheck;
- private final boolean checkThisNode;
+ private boolean checkThisNode;
private final TypeEditor parent;
@@ -87,6 +91,8 @@ class TypeEditor extends DefaultEditor {
private final NodeBuilder builder;
+ private final boolean validate;
+
TypeEditor(
boolean strict, Set<String> typesToCheck, NodeState types,
String primary, Iterable<String> mixins, NodeBuilder builder)
@@ -102,11 +108,13 @@ class TypeEditor extends DefaultEditor {
this.types = checkNotNull(types);
this.effective = createEffectiveType(null, null, primary, mixins);
this.builder = checkNotNull(builder);
+ this.validate = false;
}
private TypeEditor(
@Nonnull TypeEditor parent, @Nonnull String name,
- @CheckForNull String primary, @Nonnull Iterable<String> mixins, @Nonnull NodeBuilder builder)
+ @CheckForNull String primary, @Nonnull Iterable<String> mixins, @Nonnull NodeBuilder builder,
+ boolean validate)
throws CommitFailedException {
this.strict = parent.strict;
this.typesToCheck = parent.typesToCheck;
@@ -119,6 +127,7 @@ class TypeEditor extends DefaultEditor {
this.types = parent.types;
this.effective = createEffectiveType(parent.effective, name, primary, mixins);
this.builder = checkNotNull(builder);
+ this.validate = validate;
}
/**
@@ -133,6 +142,7 @@ class TypeEditor extends DefaultEditor {
this.types = EMPTY_NODE;
this.effective = checkNotNull(effective);
this.builder = EMPTY_NODE.builder();
+ this.validate = false;
}
/**
@@ -174,23 +184,7 @@ class TypeEditor extends DefaultEditor {
public void propertyChanged(PropertyState before, PropertyState after)
throws CommitFailedException {
if (checkThisNode) {
- NodeState definition = effective.getDefinition(after);
- if (definition == null) {
- constraintViolation(4, "No matching property definition found for " + after);
- } else if (JCR_UUID.equals(after.getName())
- && effective.isNodeType(MIX_REFERENCEABLE)) {
- // special handling for the jcr:uuid property of mix:referenceable
- // TODO: this should be done in a pluggable extension
- if (!isValidUUID(after.getValue(Type.STRING))) {
- constraintViolation(12, "Invalid UUID value in the jcr:uuid property");
- }
- } else {
- int requiredType = getRequiredType(definition);
- if (requiredType != PropertyType.UNDEFINED) {
- checkRequiredType(after, requiredType);
- checkValueConstraints(definition, after, requiredType);
- }
- }
+ checkPropertyTypeConstraints(after);
}
}
@@ -205,31 +199,20 @@ class TypeEditor extends DefaultEditor {
}
@Override
- public Editor childNodeAdded(String name, NodeState after)
- throws CommitFailedException {
- TypeEditor editor = childNodeChanged(name, MISSING_NODE, after);
-
- if (editor != null && editor.checkThisNode) {
- // TODO: add any auto-created items that are still missing
-
- // verify the presence of all mandatory items
- for (String property : editor.getEffective().getMandatoryProperties()) {
- if (!after.hasProperty(property)) {
- editor.constraintViolation(
- 21, "Mandatory property " + property
- + " not found in a new node");
- }
- }
- for (String child : editor.getEffective().getMandatoryChildNodes()) {
- if (!after.hasChildNode(child)) {
- editor.constraintViolation(
- 25, "Mandatory child node " + child
- + " not found in a new node");
- }
- }
+ public void enter(NodeState before, NodeState after) throws CommitFailedException {
+ if (checkThisNode && validate) {
+ // when adding a new node, or changing node type on an existing
+ // node, we need to reverify type constraints
+ checkNodeTypeConstraints(after);
+ checkThisNode = false;
}
+ }
- return editor;
+ @Override
+ public Editor childNodeAdded(String name, NodeState after)
+ throws CommitFailedException {
+ // TODO: add any auto-created items that are still missing
+ return childNodeChanged(name, MISSING_NODE, after);
}
@Override
@@ -239,7 +222,6 @@ class TypeEditor extends DefaultEditor {
String primary = after.getName(JCR_PRIMARYTYPE);
Iterable<String> mixins = after.getNames(JCR_MIXINTYPES);
- NodeBuilder childBuilder = builder.getChildNode(name);
if (primary == null && effective != null) {
// no primary type defined, find and apply a default type
primary = effective.getDefaultType(name);
@@ -252,8 +234,11 @@ class TypeEditor extends DefaultEditor {
}
}
- TypeEditor editor = new TypeEditor(this, name, primary, mixins, childBuilder);
- if (checkThisNode && !getEffective().isValidChildNode(name, editor.getEffective())) {
+ // if node type didn't change no need to validate child node
+ boolean validate = primaryChanged(before, primary) || mixinsChanged(before, mixins);
+ NodeBuilder childBuilder = builder.getChildNode(name);
+ TypeEditor editor = new TypeEditor(this, name, primary, mixins, childBuilder, validate);
+ if (checkThisNode && validate && !effective.isValidChildNode(name, editor.getEffective())) {
constraintViolation(
1, "No matching definition found for child node " + name
+ " with effective type " + editor.getEffective());
@@ -380,4 +365,79 @@ class TypeEditor extends DefaultEditor {
constraintViolation(5, "Value constraint violation in " + property);
}
+ private static boolean primaryChanged(NodeState before, String after) {
+ String pre = before.getName(JCR_PRIMARYTYPE);
+ return !Objects.equal(pre, after);
+ }
+
+ private static boolean mixinsChanged(NodeState before, Iterable<String> after) {
+ List<String> pre = Lists.newArrayList(before.getNames(JCR_MIXINTYPES));
+ Collections.sort(pre);
+ List<String> post = Lists.newArrayList(after);
+ Collections.sort(post);
+ if (pre.isEmpty() && post.isEmpty()) {
+ return false;
+ } else if (pre.isEmpty() || post.isEmpty()) {
+ return true;
+ } else {
+ return !Iterables.elementsEqual(pre, post);
+ }
+ }
+
+ private void checkNodeTypeConstraints(NodeState after) throws CommitFailedException {
+ EffectiveType effective = getEffective();
+
+ Set<String> properties = effective.getMandatoryProperties();
+ for (PropertyState ps : after.getProperties()) {
+ properties.remove(ps.getName());
+ checkPropertyTypeConstraints(ps);
+ }
+ // verify the presence of all mandatory items
+ if (!properties.isEmpty()) {
+ constraintViolation(21, "Mandatory property " + properties.iterator().next() + " not found in a new node");
+ }
+
+ List<String> names = Lists.newArrayList(after.getChildNodeNames());
+ for (String child : effective.getMandatoryChildNodes()) {
+ if (!names.remove(child)) {
+ constraintViolation(25, "Mandatory child node " + child + " not found in a new node");
+ }
+ }
+ if (!names.isEmpty()) {
+ for (String name : names) {
+ NodeState child = after.getChildNode(name);
+ String primary = child.getName(JCR_PRIMARYTYPE);
+ Iterable<String> mixins = child.getNames(JCR_MIXINTYPES);
+ NodeBuilder childBuilder = builder.getChildNode(name);
+ TypeEditor editor = new TypeEditor(this, name, primary, mixins, childBuilder, false);
+ if (!effective.isValidChildNode(name, editor.getEffective())) {
+ constraintViolation(25, "Unexpected child node " + names + " found in a new node");
+ }
+ }
+ }
+ }
+
+ private void checkPropertyTypeConstraints(PropertyState after)
+ throws CommitFailedException {
+ if (NodeStateUtils.isHidden(after.getName())) {
+ return;
+ }
+ NodeState definition = effective.getDefinition(after);
+ if (definition == null) {
+ constraintViolation(4, "No matching property definition found for " + after);
+ } else if (JCR_UUID.equals(after.getName()) && effective.isNodeType(MIX_REFERENCEABLE)) {
+ // special handling for the jcr:uuid property of mix:referenceable
+ // TODO: this should be done in a pluggable extension
+ if (!isValidUUID(after.getValue(Type.STRING))) {
+ constraintViolation(12, "Invalid UUID value in the jcr:uuid property");
+ }
+ } else {
+ int requiredType = getRequiredType(definition);
+ if (requiredType != PropertyType.UNDEFINED) {
+ checkRequiredType(after, requiredType);
+ checkValueConstraints(definition, after, requiredType);
+ }
+ }
+ }
+
}
diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditorTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditorTest.java
index 6087328..36e02ed 100644
--- a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditorTest.java
+++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditorTest.java
@@ -19,6 +19,7 @@ package org.apache.jackrabbit.oak.plugins.nodetype;
import static org.apache.jackrabbit.JcrConstants.JCR_MIXINTYPES;
import static org.apache.jackrabbit.JcrConstants.JCR_PRIMARYTYPE;
import static org.apache.jackrabbit.JcrConstants.NT_FOLDER;
+import static org.apache.jackrabbit.JcrConstants.MIX_REFERENCEABLE;
import static org.apache.jackrabbit.oak.plugins.memory.EmptyNodeState.EMPTY_NODE;
import static org.apache.jackrabbit.oak.plugins.nodetype.write.InitialContent.INITIAL_CONTENT;
import static org.easymock.EasyMock.createControl;
@@ -225,4 +226,98 @@ public class TypeEditorTest {
builder.setProperty("any", 134.34, Type.DOUBLE);
hook.processCommit(after, builder.getNodeState(), CommitInfo.EMPTY);
}
+
+ @Test
+ public void changeNodeTypeWExtraNodes() throws CommitFailedException {
+ EditorHook hook = new EditorHook(new TypeEditorProvider());
+
+ NodeState root = INITIAL_CONTENT;
+ NodeBuilder builder = root.builder();
+
+ NodeState before = builder.getNodeState();
+
+ builder.child("testcontent").setProperty(JCR_PRIMARYTYPE, "nt:unstructured", Type.NAME);
+ builder.child("testcontent").child("unstructured_child").setProperty(JCR_PRIMARYTYPE, "nt:unstructured",
+ Type.NAME);
+ NodeState after = builder.getNodeState();
+ root = hook.processCommit(before, after, CommitInfo.EMPTY);
+
+ builder = root.builder();
+ before = builder.getNodeState();
+ builder.child("testcontent").setProperty(JCR_PRIMARYTYPE, "nt:folder", Type.NAME);
+ try {
+ hook.processCommit(before, builder.getNodeState(), CommitInfo.EMPTY);
+ fail("should not be able to change node type due to extra nodes");
+ } catch (CommitFailedException e) {
+ assertTrue(e.isConstraintViolation());
+ }
+ }
+
+ @Test
+ public void changeNodeTypeWExtraProps() throws CommitFailedException {
+ EditorHook hook = new EditorHook(new TypeEditorProvider());
+
+ NodeState root = INITIAL_CONTENT;
+ NodeBuilder builder = root.builder();
+
+ NodeState before = builder.getNodeState();
+
+ builder.child("testcontent").setProperty(JCR_PRIMARYTYPE, "nt:unstructured", Type.NAME);
+ builder.child("testcontent").setProperty("extra", "information");
+
+ NodeState after = builder.getNodeState();
+ root = hook.processCommit(before, after, CommitInfo.EMPTY);
+
+ builder = root.builder();
+ before = builder.getNodeState();
+ builder.child("testcontent").setProperty(JCR_PRIMARYTYPE, "nt:folder", Type.NAME);
+ try {
+ hook.processCommit(before, builder.getNodeState(), CommitInfo.EMPTY);
+ fail("should not be able to change node type due to extra properties");
+ } catch (CommitFailedException e) {
+ assertTrue(e.isConstraintViolation());
+ }
+ }
+
+ @Test
+ public void changeNodeTypeNewBroken() throws CommitFailedException {
+ EditorHook hook = new EditorHook(new TypeEditorProvider());
+
+ NodeState root = INITIAL_CONTENT;
+ NodeBuilder builder = root.builder();
+
+ NodeState before = builder.getNodeState();
+ builder.child("testcontent").setProperty(JCR_PRIMARYTYPE, "nt:folder", Type.NAME);
+ builder.child("testcontent").setProperty("extra", "information");
+ try {
+ hook.processCommit(before, builder.getNodeState(), CommitInfo.EMPTY);
+ fail("should not be able to change node type due to extra properties");
+ } catch (CommitFailedException e) {
+ assertTrue(e.isConstraintViolation());
+ }
+ }
+
+ @Test
+ public void malformedUUID() throws CommitFailedException {
+ EditorHook hook = new EditorHook(new TypeEditorProvider());
+
+ NodeState root = INITIAL_CONTENT;
+ NodeBuilder builder = root.builder();
+
+ NodeState before = builder.getNodeState();
+ builder.child("testcontent").setProperty(JCR_PRIMARYTYPE, "nt:unstructured", Type.NAME);
+ builder.child("testcontent").setProperty("jcr:uuid", "not-a-uuid");
+ NodeState after = builder.getNodeState();
+ root = hook.processCommit(before, after, CommitInfo.EMPTY);
+
+ builder = root.builder();
+ before = builder.getNodeState();
+ builder.child("testcontent").setProperty(JCR_MIXINTYPES, ImmutableList.of(MIX_REFERENCEABLE), Type.NAMES);
+ try {
+ hook.processCommit(before, builder.getNodeState(), CommitInfo.EMPTY);
+ fail("should not be able to change mixin due to illegal uuid format");
+ } catch (CommitFailedException e) {
+ assertTrue(e.isConstraintViolation());
+ }
+ }
}
diff --git a/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/RepositoryTest.java b/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/RepositoryTest.java
index 9085e4b..976e57c 100644
--- a/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/RepositoryTest.java
+++ b/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/RepositoryTest.java
@@ -1903,7 +1903,6 @@ public class RepositoryTest extends AbstractRepositoryTest {
}
@Test
- @Ignore("OAK-5229")
public void setPrimaryTypeShouldFail() throws RepositoryException {
Node testNode = getNode(TEST_PATH);
assertEquals("nt:unstructured", testNode.getPrimaryNodeType().getName());
@@ -1919,6 +1918,7 @@ public class RepositoryTest extends AbstractRepositoryTest {
fail("Changing the primary type to nt:folder should fail.");
} catch (RepositoryException e) {
// ok
+ getAdminSession().refresh(false);
}
Session session2 = createAnonymousSession();
diff --git a/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/NodeTypeManagementTest.java b/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/NodeTypeManagementTest.java
index ad01663..99eae00 100644
--- a/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/NodeTypeManagementTest.java
+++ b/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/NodeTypeManagementTest.java
@@ -115,7 +115,7 @@ public class NodeTypeManagementTest extends AbstractEvaluationTest {
String ntName = child.getPrimaryNodeType().getName();
try {
- childNode.setPrimaryType("nt:folder");
+ childNode.setPrimaryType("oak:Unstructured");
testSession.save();
fail("TestSession does not have sufficient privileges to change the primary type.");
} catch (AccessDeniedException e) {
@@ -134,7 +134,7 @@ public class NodeTypeManagementTest extends AbstractEvaluationTest {
Node child = (Node) superuser.getItem(childNode.getPath());
String ntName = child.getPrimaryNodeType().getName();
- String changedNtName = "nt:folder";
+ String changedNtName = "oak:Unstructured";
child.setPrimaryType(changedNtName);
testSession.save();