-
-
Notifications
You must be signed in to change notification settings - Fork 221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FEATURE: isDescendantOfNodetype
matcher
#5291
base: 8.4
Are you sure you want to change the base?
Conversation
isDescendantOfNodetype
matcher
The Tests here will fail before the other pull request is merged I think. |
2671cdc
to
94e6956
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @vcg-development ,
thanks for your contribution :) I have some suggestions for this change - there are some detail adjustments, and the biggest recommendation is moving the things from neos/flow-development-collection#3403 into this repo (as explained in the inline comments).
Thanks and all the best,
Sebastian
PS: I often do not see the Github notifications -> so it would be extremely helpful for me in case you work on this and need another review if you could quickly ping me on Slack (sebastian - as private message). Thanks <3
public function isDescendantOfNodetype($nodeTypes) | ||
{ | ||
$propertyConditionGenerator1 = new DecendantOfNodetypeConditionGenerator($nodeTypes); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return new DescentantOfNodeTypeConditionGenerator(...);
=> and I'd suggest to put the DescendantOfNodeTypeConditionGenerator() into Neos.ContentRepository/Classes/Security/Authorization/Privilege/Node/Doctrine/... -> no need to place this in Flow IMHO (and it would make Flow depend on Neos, which we want the other way around.
* @param string|array $nodeTypes A single or an array of fully qualified NodeType name(s), e.g. "Neos.Neos:Document" | ||
* @return boolean true if the given node matches otherwise false | ||
*/ | ||
public function isDescendantOfNodetype($nodeTypes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming suggestion: isDescendantOfType (we have other methods in here where we abbreviate NodeType with "type" in this class)
| 4f7230ba-36b2-4dc3-96fa-b4159371cd3b | /sites/content-repository/service/collection/text | Neos.ContentRepository.Testing:Text | {"text": "Cool text"} | live | | ||
|
||
@Isolated @fixtures | ||
Scenario: Anonymous users are not granted to edit childnodes on ContenCollection nodetypes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ContentCollection (T missing :) )
And I should get false when asking the node authorization service if editing this node is granted | ||
|
||
@Isolated @fixtures | ||
Scenario: Administrators are granted to edit childnodes on ContenCollection nodetypes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ContentCollection
I have added the isDescendantOfNodetype Condition in the Condition Generator, which uses the newly created DecendantOfNodetypeConditionGenerator in the flow-development-collection Repository (merge request pending).
It makes it possible to add a matcher to the Policies that matches all Nodes that are a descendant of the given Nodetype.
Example:
Iww.IwwDe:Artikel:
matcher: isDescendantOfNodetype(['Iww.NodeTypes:Documents.ArtikelCenshare'])
The above shown example matches all Nodes (and the node itself) that are of a certain type and makes it possible for example, using this privilegeTarget matcher, to forbid the editing of the given nodetype and all its children.
Review instructions
We needed that functionality, so we can have nodetypes that are only editable by someone with the correct role.
You can verify this change by using the isDescendantOfNodetype privilege target in a Policy.yaml and adding it to a role or use the added tests.
Checklist
FEATURE|TASK|BUGFIX
!!!
and have upgrade-instructions