FEATURE: Add new NodePrivilege matchers restrict editing a node if user is in a defined workspace

[Benjamin-K]

This PR adds a new NodePrivilege matcher userIsInTargetWorkspace, that will check, if the user is in one of the given workspaces.

There is an existing matcher isInWorkspace, but it does not handle that case in a totally correct way, as it will only match after the node has been edited and published to one of the defined workspace and therefore can raise some questions when editing a node that is (by default) in the live workspace and gets published to a restricted workspace workspace or (with a whitelist approach) the node can not be edited until some user with access to the node pushes the node to an allowed workspace.

Example:

privilegeTargets:
  'Neos\ContentRepository\Security\Authorization\Privilege\Node\EditNodePrivilege':
    'Vendor.Package:EditAllNodes':
      matcher: 'TRUE' # Switch to whitelist approach

    'Vendor.Package:EditWithIsInWorkspace':
      matcher: 'isDescendantNodeOf("some-node-uuid") && isInWorkspace(["preview-abcd"])'
    'Vendor.Package:EditWithUserIsInTargetWorkspace':
      matcher: 'isDescendantNodeOf("some-node-uuid") && userIsInTargetWorkspace(["preview-abcd"])'

roles:
  'Vendor.Package:WorkspaceRole':
    label: 'Partial preview workspace editor'
    parentRoles: ['Neos.Neos:RestrictedEditor']
    privileges:
      - privilegeTarget: 'Vendor.Package:EditWithIsInWorkspace'
        permission: GRANT

  
  'Vendor.Package:TargetWorkspaceRole':
    label: 'Full preview workspace editor'
    parentRoles: ['Neos.Neos:RestrictedEditor']
    privileges:
      - privilegeTarget: 'Vendor.Package:EditWithUserIsInTargetWorkspace'
        permission: GRANT

Setup:

• Selected workspace: Preview
• Workspace of node: live

Now a user with role Vendor.Package:WorkspaceRole won't be able to edit the node until a user with the required permissions publishes some changes to workspace "preview-abcd", while a user with role Vendor.Package:TargetWorkspaceRole can.

resolved: #3893

Upgrade instructions

If you previously used the isInWorkspace matcher, consider updating to userIsInTargetWorkspace (unless the described behaviour above is really what you wanted).

Review instructions

The behaviour above can also apply to the current implementation of the isInDimensionPreset matcher. This can be the case, if fallback rules are defined in the dimension settings. I will add this here, too, but first need to figure out how to get the current dimension of the user instead of the selected node.

Checklist

• Code follows the PSR-2 coding style
• Tests have been created, run and adjusted as needed
• The PR is created against the lowest maintained branch
• Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
• Reviewer - The first section explains the change briefly for change-logs
• Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

[crydotsnake]

Hey @Benjamin-K !

Thanks for this change.

This sounds more like a feature instead of a task. So your PR should target Neos 9.0 then 🙂

[Benjamin-K]

Hey @crydotsnake,

You're right, this is kind of more of a feature. But as (IMO) the current implementation of isInWorkspace is not exactly what is expected and as this was already discussed in sandstorm/neosacl#37 and #3893 I would prefer to still push it to 8.3, as 9.0 would take a while I think until most developers upgrade to it. And 8.3 still gets regular release updates.

What do you think?

[bwaidelich]

Thanks so much for taking care!
I added some inline comments, but more importantly I wonder:

• Should we add it to 8.3? I would vote for it since it is a really important improvement to make ACLs work (I would still not call it a "TASK" but a "FEATURE" which it clearly is)
• I wonder if it is a problem to inject the UserService into this context but I can't think of another way to get hold of the currently authenticated user (and s.th. like userIsInTargetWorkspace(current.userService.currentUser, "workspace-name")in the Policy seems hacky, too
• Can we come up with a test for this?

[bwaidelich]

I'm not 100% sure about the naming tbh.
is the user really in the target workspace?

What about just isTargetWorkspace()?

[Benjamin-K]

The also not 100 % sure. But isTargetWorkspace() is not what this matcher will test IMO, as it is (in theory) a matcher on the node and not on the user.

We could use sth like canPublishToTargetWorkspace() or sth. similar if we want to remove the user. I'm in for new suggestions though.

[bwaidelich]

That seems dangerous to me, but it's the same in the other matchers so it's probably fine to keep it here as well

[Benjamin-K]

This might really be a case, where accurate tests are needed. I wondered about that one, too.

[kitsunet]

Given that this for once is code that is completely ignored unless you actually use this Privilege I would be fine with adding it (with FEATURE) to 8.3.

[mhsdesign]

Thanks for your contribution. I have no problems when merging this little feature into 8.3 but the timing is a bit bad ^^
The whole policy stuff will look completely different in 9.0 and thus im not sure if its worth to introduce new behaviour now? But on the other-hand exactly that makes it matter way less :P This code will be good as gone :D
I could imagine that this could also be solved as a plugin? If so, this might be a better step? Especially as there is no real gain to preserve this feature for the future in the Neos core.

[kitsunet]

In regrads to:

will add this here, too, but first need to figure out how to get the current dimension of the user instead of the selected node.

I don't think this is "a thing". The user has no current dimesions, you have two directions you can go, which one is more correct depends I guess,

a) parse dimensions from URL (again) and use those - I would prefer to NOT do that.
b) $node->context IS the current context and not specifically for that node, so dimensions in there should be the right ones.. To get the specific Node dimensions you would $node->getNodeData()->getDimensionValues(). This is btw. the same for the workspace, the context should by any sensible measure contain the "current" workspace the user is working in, the actual workspace a given node might exist in at the time is $this->getNodeData()->getWorkspace() (which unfortunately - for reasons - $node->getWorspace() returns), $node->context->getWorkspace() should be correct as well, so I would prefer using that in this change.

[bwaidelich]

I could imagine that this could also be solved as a plugin?

You mean: A 3rd party package that provides a custom NodePrivilege and NodePrivilegeContext?
That's actually a good idea IMO because it gives us more freedom.

@Benjamin-K What do you think of that suggestion?

[Benjamin-K]

I don't think this is "a thing". The user has no current dimesions, you have two directions you can go, which one is more correct depends I guess

I think, we could just leave this for now.

... the context should by any sensible measure contain the "current" workspace the user is working in, the actual workspace a given node might exist in at the time is $this->getNodeData()->getWorkspace() (which unfortunately - for reasons - $node->getWorspace() returns), $node->context->getWorkspace() should be correct as well, so I would prefer using that in this change.

@kitsunet Would be nice, if that was the case. But it's actually not. Both, $node->getWorkspace and $node->getContext()->getWorkspace() return the actual workspace of the node. So if a node has not been changed, both will return 'live', even if the user is currently working in a workspace workspace-abcd. Or did I get you wrong?

I could imagine that this could also be solved as a plugin?

We could do that. But right now, if one privilege (for example for the EditNodePrivilege) matches, an editor can edit that part of the page, right? I'm not that deep into the privilege implementation, but what would be the case if one keeps his/her existing privileges and adds a new privilegeTarget from the plugin. Sth. like:

# Policy.yaml
privilegeTargets:
  'Neos\ContentRepository\Security\Authorization\Privilege\Node\EditNodePrivilege':
    'Vendor.Package:PrivilegeTargetOld':
      matcher: 'isDescendantNodeOf("n0de-0001-0000-0000-111111111111")'
  'Vendor\Package\Security\Authorization\Privilege\Node\EditNodePrivilege':
    'Vendor.Package:PrivilegeTargetNew':
      matcher: 'isDescendantNodeOf("n0de-0002-0000-0000-222222222222") && customMatcher(['workspace-abcd'])'

I think we have to create multiple new privilege classes, one for each existing privilege class in Neos.ContentRepository/Classes/Security/Authorization/Privilege/Node/.... Not sure, if I want that "much" code for such a little addition.

Can we come up with a test for this?

I would really like to have tests for this feature. And for the other node privileges as well, as they are also missing 🫣 I will try to add a test, but first need to figure out, how to get all the required context values (UserService with a mocked user, a mocked node, ...).

[kitsunet]

@Benjamin-K then I would like to understand your testcase better because if the context says live then you should be seeing live, it is never node specific.

[Benjamin-K]

@Benjamin-K then I would like to understand your testcase better because if the context says live then you should be seeing live, it is never node specific.

@kitsunet Let's say, you are logged in and work in workspace "Preview", but have not done any changes so far.

If I get you right, $node->getContext()->getWorkspace()->getName() should return "Preview" (even if the node has not been changed yet, and therefore is still in the "live" workspace).

This currently is not the case and will return "live" until some user pushed some changes to the "Preview" workspace.

The problem with this is: Currently you cannot allow a user to only work in a specified workspace ("Preview" in this example) without having some other user (with more privileges) pushing changes to that workspace.

It's actually not about what a user sees (which is "live"), but what a user wants to achieve (make changes to a node in "live" workspace and push them to the "Preview" workspace).

[kitsunet]

If I get you right, $node->getContext()->getWorkspace()->getName() should return "Preview" (even if the node has not been changed yet, and therefore is still in the "live" workspace).

It absolutely must, what you describe as the behavior cannot work. I guess I will have to try and reproduce this to understand what is going wrong here....

[Benjamin-K]

It absolutely must, what you describe as the behavior cannot work. I guess I will have to try and reproduce this to understand what is going wrong here....

@kitsunet Would be really nice, if that was the case, as most of this PR could be removed then. Currently both

• $node->getWorkspace() and
• $node->getContext()->getWorkspace()

return the same workspace – the one, the node is in, no matter which workspace the user is currently working in.

[kitsunet]

I just had another look at the code, I think there is some very nasty conceptual dragons there. The ReadNodePrivilege eg. extends the EntityPrivilege. This is important because Node(Interface) is not an entity. The Entity is NodeData as ReadNodePrivilege also says. BUT matchesSubject does this

$nodeContext = new NodePrivilegeContext($subject->getNode()); and here getNode returns a Node(Interface) so I am not quite sure what really is used here., because \Neos\ContentRepository\Security\Authorization\Privilege\Node\Doctrine\ConditionGenerator::isInWorkspace should operate on the workspace field of the NodeData table. So something here I am missing. Might be that we (somewhere I haven't seen yet) create a context matching the NodeData values... Which would be problematic because then we would have what you are experiencing, but that would then not reflect which context is actually "active".

[Benjamin-K]

... \Neos\ContentRepository\Security\Authorization\Privilege\Node\Doctrine\ConditionGenerator::isInWorkspace should operate on the workspace field of the NodeData table.

This is, what actually happens. BUT: The workspace of the NodeData table is holding the actual workspace of the node you want to edit. And that is the problem here.

Let's say, i have switched to workspace Preview and no node has been changed, so all nodes are in workspace live and therefore shine through to my workspace. But with isInWorkspace the workspace field of the NodeData table is used, so isInWorkspace('preview-<hash>') will not allow me to edit any node, as no node has actually been created in that workspace (they only shine through).

So for me (let's say i am User A) to be able to read/edit/... a node:

• User B (maybe an admin) needs to change a node and publish those changes to my Preview workspace.
• Now I can read/edit/... the node.
• As soon as i publish my changes back to live, I will no longer be able to read/edit/...