Skip to content
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: Add new NodePrivilege matchers restrict editing a node if user is in a defined workspace #4884

Draft
wants to merge 5 commits into
base: 8.3
Choose a base branch
from
Draft
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -1,4 +1,5 @@
<?php

namespace Neos\ContentRepository\Security\Authorization\Privilege\Node;

/*
Expand All @@ -15,8 +16,10 @@
use Neos\Flow\Annotations as Flow;
use Neos\Flow\Security\Context as SecurityContext;
use Neos\ContentRepository\Domain\Model\NodeInterface;
use Neos\ContentRepository\Domain\Model\Workspace;
use Neos\ContentRepository\Domain\Service\ContentDimensionPresetSourceInterface;
use Neos\ContentRepository\Domain\Service\ContextFactory;
use Neos\Neos\Service\UserService;

/**
* An Eel context matching expression for the node privileges
Expand Down Expand Up @@ -47,6 +50,12 @@ class NodePrivilegeContext
*/
protected $contentDimensionPresetSource;

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

/**
* @var NodeInterface
*/
Expand Down Expand Up @@ -145,9 +154,16 @@ public function nodeIsOfType($nodeTypes)
/**
* Matches if the selected node belongs to one of the given $workspaceNames
*
* Example: isInWorkspace(['live', 'user-admin']) matches if the selected node is in one of the workspaces "user-admin" or "live"
* Note: If you want to restrict editing a node if the user is in one of the given workspaces,
* use `userIsInTargetWorkspace` instead, as this method only checks the node's workspace.
* In general most nodes are in the "live" workspace until they are changed, so this will
* only apply if User A edits a node in the "live" workspace and pushes the changes to
* workspace "workspace-abcd" and User B tries to edit the node in "workspace-abcd".
*
* Example: isInWorkspace(['workspace-abcd', 'user-admin', 'live'])
* matches if the selected node is in one of the workspaces "workspace-abcd", "user-admin" or "live"
*
* @param array $workspaceNames An array of workspace names, e.g. ["live", "user-admin"]
* @param array $workspaceNames An array of workspace names, e.g. ["live", "user-admin", "workspace-abcd"]
* @return boolean true if the selected node matches the $workspaceNames, otherwise false
*/
public function isInWorkspace($workspaceNames)
Expand All @@ -159,6 +175,42 @@ public function isInWorkspace($workspaceNames)
return in_array($this->node->getWorkspace()->getName(), $workspaceNames);
}

/**
* Matches if the currently-selected workspace is the target workspace of the current user.
*
* Example: userIsInTargetWorkspace(['preview-1234']) matches if the current user
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

What about just isTargetWorkspace()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* 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): bool
{
if ($this->node === null) {
return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

$userWorkspace = $this->userService->getPersonalWorkspace();

if (!($userWorkspace instanceof Workspace)) {
// User is not logged in
return false;
}

$baseWorkspace = $userWorkspace->getBaseWorkspace();

if ($baseWorkspace === null) {
// User is not logged in or has no base workspace
return false;
}

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

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

/**
* Matches if the currently-selected preset in the passed $dimensionName is one of $presets.
*
Expand Down