Author: alexparvulescu Date: Fri Mar 3 12:53:21 2017 New Revision: 1785287
URL: http://svn.apache.org/viewvc?rev=1785287&view=rev Log: OAK-5887 Stricter validation on primary type change Modified: jackrabbit/oak/trunk/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugValidatorTest.java jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditor.java jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditorTest.java jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/RepositoryTest.java jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/NodeTypeManagementTest.java Modified: jackrabbit/oak/trunk/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugValidatorTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugValidatorTest.java?rev=1785287&r1=1785286&r2=1785287&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugValidatorTest.java (original) +++ jackrabbit/oak/trunk/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugValidatorTest.java Fri Mar 3 12:53:21 2017 @@ -48,8 +48,10 @@ public class CugValidatorTest extends Ab @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) { Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditor.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditor.java?rev=1785287&r1=1785286&r2=1785287&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditor.java (original) +++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditor.java Fri Mar 3 12:53:21 2017 @@ -36,6 +36,7 @@ import static org.apache.jackrabbit.oak. 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.com 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); + } + } + } + } Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditorTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditorTest.java?rev=1785287&r1=1785286&r2=1785287&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditorTest.java (original) +++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditorTest.java Fri Mar 3 12:53:21 2017 @@ -19,6 +19,7 @@ package org.apache.jackrabbit.oak.plugin 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()); + } + } } Modified: jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/RepositoryTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/RepositoryTest.java?rev=1785287&r1=1785286&r2=1785287&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/RepositoryTest.java (original) +++ jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/RepositoryTest.java Fri Mar 3 12:53:21 2017 @@ -1872,7 +1872,6 @@ public class RepositoryTest extends Abst } @Test - @Ignore("OAK-5229") public void setPrimaryTypeShouldFail() throws RepositoryException { Node testNode = getNode(TEST_PATH); assertEquals("nt:unstructured", testNode.getPrimaryNodeType().getName()); @@ -1888,6 +1887,7 @@ public class RepositoryTest extends Abst fail("Changing the primary type to nt:folder should fail."); } catch (RepositoryException e) { // ok + getAdminSession().refresh(false); } Session session2 = createAnonymousSession(); Modified: jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/NodeTypeManagementTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/NodeTypeManagementTest.java?rev=1785287&r1=1785286&r2=1785287&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/NodeTypeManagementTest.java (original) +++ jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/NodeTypeManagementTest.java Fri Mar 3 12:53:21 2017 @@ -115,7 +115,7 @@ public class NodeTypeManagementTest exte 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 exte 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();
