[jira] [Comment Edited] (OAK-5229) Using Node.setPrimaryType() should fail if non-matching childnodes

["angela (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/OAK-5229?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15855676#comment-15855676 ] 

angela edited comment on OAK-5229 at 2/7/17 10:06 AM:
------------------------------------------------------

[~alexparvulescu], just had a quick look at the patch and the questions raised above:

{quote}
UUID structure will only be validated if the proper mixin is present. If the events are reversed (captured by a test now): first bad UUID, then mixin is added, the validator will not accept the changes. I think this respects the requirements while also not eagerly validating UUID formats.
{quote}

That looks ok to me and corresponds to the behaviour we had in Jackrabbit core. If a {{jcr:uuid}} property is used outside of the context of {{jcr:referenceable}} mixin type and thus it's defining node type is a another primary/mixin type, it's no longer what it looks like... so the behaviour is for sure compliant with the contract. I would consider all items with the "jcr" prefix specially those that are protected to be reserved and using them outside of the context is bad practice that will lead to undefined behaviour. If it turned out to be an issue in practice or from a security point of view we may still consider enforcing this.

{quote}
There is still the issue with CugValidatorTest that no one picked up.
{quote}

The {{CugValidator}} for security reasons has an extra validation to prevent _changing_ the primary node type of any node from or to {{rep:CugPolicy}}. The test verifies that this constraint is enforced and that the expected error code is thrown as listed in the documentation of the feature (see oak docu). 
If the test fails with your changes as described it means that the {{CugValidator}} is not triggered and a previous validator already aborted. This could either highlight an issue in your changes or it might highlight the fact that changing the primary type of the target node to {{rep:CugPolicy}} is not possible for nt-constraint reasons or that the change is incomplete (e.g. missing mandatory child items) that trigger the {{TypeEditor}}. The right fix for the latter would be to adjust the test-case (-> TypeEditor doesn't complain) and thus have the {{CugValidator}} trigger.
Please don't exclude the test-case from execution.

Regarding [~tripod] comment above:
{quote}
but adding a mixin also checks immediately, and so should changing the primary type; as mandated by the JCR spec.
{quote}

From the Javadoc quoted above I am not convinced that the JCR specification actually mandates the validation upon transient modification... I rather read it in way that this was conditional. However, changing this behavior surely would break compatibility with Jackrabbit 2.x and may force API consumers to perform the validation themselves, which IMHO was pretty bad and is prone to cause other issues.
Therefore, I would suggest that we perform an additional best-effort check in {{oak-jcr}} and fail as early as possible. I don't think that changing the primary type is a very common operation and I would be surprised if performing the validation twice would have a huge performance impact in general. [~alexparvulescu], [~tripod], what do you think?





was (Author: anchela):
[~alexparvulescu], just had a quick look at the patch and the questions raised above:

{quote}
UUID structure will only be validated if the proper mixin is present. If the events are reversed (captured by a test now): first bad UUID, then mixin is added, the validator will not accept the changes. I think this respects the requirements while also not eagerly validating UUID formats.
{quote}

That looks ok to me and corresponds to the behaviour we had in Jackrabbit core. If a {{jcr:uuid}} property is used outside of the context of {{jcr:referenceable}} mixin type and thus it's defining node type is a another primary/mixin type, it's no longer what it looks like... so the behaviour is for sure compliant with the contract. I would consider all items with the "jcr" prefix specially those that are protected to be reserved and using them outside of the context is bad practice that will lead to undefined behaviour. If it turned out to be an issue in practice or from a security point of view we may still consider enforcing this.

{quote}
There is still the issue with CugValidatorTest that no one picked up.
{quote}

The {{CugValidator}} for security reasons has an extra validation to prevent _changing_ the primary node type of any node from or to {{rep:CugPolicy}}. The test verifies that this constraint is enforced and that the expected error code is thrown as listed in the documentation of the feature (see oak docu). 
If the test fails with your changes as described it means that the {{CugValidator}} is not triggered and a previous validator already aborted. This could either highlight an issue in your changes or it might highlight the fact that changing the primary type of the target node to {{rep:CugPolicy}} is not possible for nt-constraint reasons or that the change is incomplete (e.g. missing mandatory child items) that trigger the {{TypeEditor}}. The right fix for the latter would be to adjust the test-case (-> TypeEditor doesn't complain) and thus have the {{CugValidator}} trigger.
Please don't exclude the test-case from execution.

Regarding [~tripod] comment above:
{quote}
but adding a mixin also checks immediately, and so should changing the primary type; as mandated by the JCR spec.
{quote}

From the Javadoc quoted above I am not convinced that the JCR specification actually mandates the validation upon transient modification... I rather read it in way that this was conditional. However, changing this behavior surely would break compatibility with Jackrabbit 2.x and may force API consumers to perform the validation themselves, which IMHO was pretty bad and is prone to cause other issues.
Therefore, I would suggest that we perform a best-effort check in {{oak-jcr}} and fail as early as possible. I don't think that changing the primary type is a very common operation and I would be surprised if performing the validation twice would have a huge performance impact in general. [~alexparvulescu], [~tripod], what do you think?




> Using Node.setPrimaryType() should fail if non-matching childnodes
> ------------------------------------------------------------------
>
>                 Key: OAK-5229
>                 URL: https://issues.apache.org/jira/browse/OAK-5229
>             Project: Jackrabbit Oak
>          Issue Type: Bug
>          Components: core
>    Affects Versions: 1.5.14
>            Reporter: Tobias Bocanegra
>            Assignee: Alex Parvulescu
>            Priority: Critical
>             Fix For: 1.8, 1.6.1
>
>         Attachments: OAK-5229.patch, OAK-5229-tests.patch, OAK-5229-v2.patch
>
>
> 1. Assume the following:
> {noformat}
> /testNode [nt:unstructured]
>   /unstructured_child [nt:unstructured]
> {noformat}
> 2. setting "/testNode".setPrimaryType("nt:folder")
> 3. save the session.
> Altering the primary type works, thus leaving the repository in an inconsistent state.
> Interestingly, subsequent calls to "/testNiode/unstructured_child".setProperty() will fail:
> {noformat}
> javax.jcr.nodetype.ConstraintViolationException: OakConstraint0001: /test_node[[nt:folder]]: No matching definition found for child node unstructured_child with effective type [nt:unstructured]
> {noformat}



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)