FEATURE: Add new method to NodePrivilegeContext to restrict the target workspace

[Benjamin-K]

Is there an existing issue for this?

• [X] I have searched the existing issues

Description

Description

If you want to restrict the nodes that can be changed by a user, you currently can restrict by node tree, by node type, by dimension preset or by the current workspace of the node.

The problem with the current workspace is this:

• User A is allowed to edit nodes in workspace "internal".
• Before being able to edit any nodes, he requires User B to push changes on all nodes he is required to edit to this workspace. If not restricted, he can create new nodes in this workspace only
• As soon as workspace "internal" is published to its base workspace again, User A is no longer able to edit anything.

It would be better to restrict editing etc. by the current base workspace of the currently logged in user.

Steps to reproduce

1. Create a new privilegeTarget that can edit all nodes and add that privilegeTarget to the 'Neos.Neos:Editor' (see https://docs.neos.io/cms/manual/backend-permissions/real-world-examples).
2. Create a new workspace.
3. Create a new privilegeTarget that can edit all nodes if they are in the given workspace (matcher: 'isInWorkspace(["new-worspace-abcd"])') and assign it to a new role that has "Neos.Neos:RestrictedEditor" as parentRole.
4. Try to reproduce the described behaviour.

Expected behaviour

There is a method to restrict privileges by the current target workspace of the logged in user.

Actual behaviour

No method exists to restrict editing etc. depending on the currently selected base workspace.

Possible Solution

The easiest way to do this, is by adding a new method to Neos\ContentRepository\Security\Authorization\Privilege\Node\NodePrivilegeContext. Something like this:

// Only changes/additions in the file are added here.

use Neos\Flow\Annotations as Flow;
use Neos\ContentRepository\Domain\Model\Workspace;
use Neos\ContentRepository\Domain\Repository\WorkspaceRepository;
use Neos\Neos\Service\UserService;

class NodePrivilegeContext
{
    /**
     * @Flow\Inject
     * @var UserService
     */
    protected $userService;

    /**
     * @Flow\Inject
     * @var WorkspaceRepository
     */
    protected $workspaceRepository;

    /**
     * Matches if the current user is in one of the given target workspaces
     *
     * Example userIsInTargetWorkspace(['preview-svxg3']) matches if the current user has selected workspace "preview-svxg3" as target workspace
     *
     * @param string|array $workspaceNames An array of workspace names, e.g. ["live", "preview-svxg3"]
     * @return boolean true if the current user is in one of the given $workspaceNames, otherwise false
     */
    public function userIsInTargetWorkspace($workspaceNames)
    {
        if ($this->node === null) {
            return true;
        }

        $userWorkspaceName = $this->userService->getPersonalWorkspaceName();

        if ($userWorkspaceName === null) {
            // User is not logged in
            return true;
        }

        /** @var Workspace $userWorkspace */
        $userWorkspace = $this->workspaceRepository->findOneByName(
            $userWorkspaceName,
            true,
            /* cache results: */ true
        );
        if ($userWorkspace === null) {
            return false;
        }

        if (!is_array($workspaceNames)) {
            $workspaceNames = [$workspaceNames];
        }

        return in_array($userWorkspace->getBaseWorkspace()->getName(), $workspaceNames);
    }
}

[Benjamin-K]

In addition to this: The isInDimensionPreset privilege matcher also only checks the current state of a node. If you have dimension fallbacks, not all nodes may exist in the targeted dimension. Therefore some similar check as the target workspace should be done instead of checking the current node state in my opinion.

[patricekaufmann]

I would also like to see this being implemented. I am currently trying to restrict editing to a single named workspace.

Diving a bit inside the matchers,

    public function isInWorkspace($workspaceNames)
    {
        if ($this->node === null) {
            return true;
        }

        \Neos\Flow\var_dump($this->node->getContext()->getWorkspace()->getBaseWorkspace()->getName());

        return in_array($this->node->getWorkspace()->getName(), $workspaceNames);
    }

The dumped output is actually the workspace I would like to check against, however $this->node->getWorkspace()->getName() returns live even if inside another workspace that is merely based on live. For me this brings up the question...

What is even the purpose of the original isInWorkspace matcher? Is there any situation this could be useful in? The only way it would return another workspace would be if another user already created / modified a node in that workspace. However I would argue that this sounds like an edge case and the proposed change should actually be the default behaviour.

[Benjamin-K]

@patricekaufmann I temporarily solved this using AOP. I created a gist for this: https://gist.github.com/Benjamin-K/78c99f19a6dd9de8dfa276cdfdfa82f8

The extended UserService class is just for little better performance.

[patricekaufmann]

Really valuable hint with the Flow AOP, thanks! 🥳 I dropped in your solution and it works like a charm already. However, I also added the following function to the trait ( not really clean adding it to the same trait but it will do for some testing):

    public function isInBaseWorkspace($workspaceNames): bool
    {
        if ($this->node === null) {
            return true;
        }

        return in_array($this->node->getContext()->getWorkspace()->getBaseWorkspace()->getName(), $workspaceNames);
    }

I will probably test both solutions and see if I can break them in any way :+1:

[bwaidelich]

It seems like the isInWorkspace() matcher really doesn't make sense in its current form, so this might be turned into a bug ticket.

But I wonder if we can find a way to solve this in a backwards compatible manner – maybe a 2nd argument, like isInWorkspace(["new-worspace-abcd"], true)?

It would be really helpful if someone could come up with a PR, so that we can already test this using composer patches

[Benjamin-K]

Should we still use the existing method name? Or create a new method? And would you prefer the second parameter to be able to the fixed version or to the new version?

I can hardly think of anybody using the current implementation and having it meeting all their requirements. IMO it would make sense to "override" the existing method with an option to use the old check. But I'm not someone that is happy with the current state, so I might be wrong.

[bwaidelich]

Should we still use the existing method name? Or create a new method?

Good point, a new method makes much more sense and I just realized that you had already suggested isInBaseWorkspace which works great IMO.

I can hardly think of anybody using the current implementation and having it meeting all their requirements

Yes, probably..

IMO it would make sense to "override" the existing method with an option to use the old check

Yes, but with a new matcher method we're on the safe side and we can even deprecated the old one. Let's keep it a feature and target 8.3, wdyt?

[Benjamin-K]

Good point, a new method makes much more sense and I just realized that you had already suggested isInBaseWorkspace which works great IMO.

This implementation is from @patricekaufmann. I named it userIsInTargetWorkspace, which is a bit clumsy, but should be self-explaining.

I will try to create a PR till the end of this week as i already used a NodePrivilegeContextAspect.php.

[bwaidelich]

@Benjamin-K did you have any success re

I will try to create a PR till the end of this week as i already used a NodePrivilegeContextAspect

?

[Benjamin-K]

@bwaidelich see the linked PR from friday: #4884 :)

[bwaidelich]

Ah great, pardon me. You're the man!

[bwaidelich]

How can we proceed with this one?