From d5300e2490d9d0f36dc4c0e9579c7f901e01e9d3 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Thu, 31 Oct 2024 22:21:12 +0100 Subject: [PATCH 01/24] TASK: Introduce utility methods for #5132 --- .../Classes/Domain/Model/WorkspaceRoleAssignment.php | 7 +++++++ .../Classes/Domain/Model/WorkspaceRoleAssignments.php | 10 ++++++++++ 2 files changed, 17 insertions(+) diff --git a/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignment.php b/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignment.php index f8206c8d2ce..b8af8edae39 100644 --- a/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignment.php +++ b/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignment.php @@ -42,4 +42,11 @@ public static function createForGroup(string $flowRoleIdentifier, WorkspaceRole $role ); } + + public function equals(WorkspaceRoleAssignment $other): bool + { + return $this->subjectType === $other->subjectType + && $this->subject->equals($other->subject) + && $this->role === $other->role; + } } diff --git a/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignments.php b/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignments.php index a63eb23b899..ef9af39059a 100644 --- a/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignments.php +++ b/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignments.php @@ -47,4 +47,14 @@ public function count(): int { return count($this->assignments); } + + public function contains(WorkspaceRoleAssignment $assignment): bool + { + foreach ($this->assignments as $existingAssignment) { + if ($existingAssignment->equals($assignment)) { + return true; + } + } + return false; + } } From 1404d378e3483f20f95dd784e9fbf93638df7bf0 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Thu, 5 Dec 2024 21:58:57 +0100 Subject: [PATCH 02/24] TASK: Introduce `WorkspaceService::deleteWorkspace` to also removed assigned metadata and roles --- .../WorkspaceMetadataAndRoleRepository.php | 52 +++++++++++++++++++ .../Domain/Service/WorkspaceService.php | 19 +++++++ 2 files changed, 71 insertions(+) diff --git a/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php b/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php index cf320af71f5..bc551b40ba4 100644 --- a/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php +++ b/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php @@ -178,6 +178,58 @@ public function getMostPrivilegedWorkspaceRoleForSubjects(ContentRepositoryId $c return WorkspaceRole::from($role); } + public function deleteWorkspaceMetadata(ContentRepositoryId $contentRepositoryId, WorkspaceName $workspaceName): void + { + $table = self::TABLE_NAME_WORKSPACE_METADATA; + $query = <<dbal->executeStatement($query, [ + 'contentRepositoryId' => $contentRepositoryId->value, + 'workspaceName' => $workspaceName->value, + ]); + } catch (DbalException $e) { + throw new \RuntimeException(sprintf( + 'Failed to delete metadata for workspace "%s" (Content Repository "%s"): %s', + $workspaceName->value, + $contentRepositoryId->value, + $e->getMessage() + ), 1726821159, $e); + } + } + + public function deleteWorkspaceRoleAssignments(ContentRepositoryId $contentRepositoryId, WorkspaceName $workspaceName): void + { + $table = self::TABLE_NAME_WORKSPACE_ROLE; + $query = <<dbal->executeStatement($query, [ + 'contentRepositoryId' => $contentRepositoryId->value, + 'workspaceName' => $workspaceName->value, + ]); + } catch (DbalException $e) { + throw new \RuntimeException(sprintf( + 'Failed to delete role assignments for workspace "%s" (Content Repository "%s"): %s', + $workspaceName->value, + $contentRepositoryId->value, + $e->getMessage() + ), 1726821159, $e); + } + } + /** * Removes all workspace metadata records for the specified content repository id */ diff --git a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php index b4021fae155..fae30ec7d60 100644 --- a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php +++ b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php @@ -18,6 +18,7 @@ use Neos\ContentRepository\Core\Feature\WorkspaceCreation\Command\CreateRootWorkspace; use Neos\ContentRepository\Core\Feature\WorkspaceCreation\Command\CreateWorkspace; use Neos\ContentRepository\Core\Feature\WorkspaceCreation\Exception\WorkspaceAlreadyExists; +use Neos\ContentRepository\Core\Feature\WorkspaceModification\Command\DeleteWorkspace; use Neos\ContentRepository\Core\SharedModel\ContentRepository\ContentRepositoryId; use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamId; use Neos\ContentRepository\Core\SharedModel\Workspace\Workspace; @@ -204,6 +205,24 @@ public function unassignWorkspaceRole(ContentRepositoryId $contentRepositoryId, $this->metadataAndRoleRepository->unassignWorkspaceRole($contentRepositoryId, $workspaceName, $subject); } + /** + * Deletes a content repository workspace and also all role assignments and metadata + */ + public function deleteWorkspace(ContentRepositoryId $contentRepositoryId, WorkspaceName $workspaceName): void + { + $contentRepository = $this->contentRepositoryRegistry->get($contentRepositoryId); + $this->requireWorkspace($contentRepositoryId, $workspaceName); + + $contentRepository->handle( + DeleteWorkspace::create( + $workspaceName + ) + ); + + $this->metadataAndRoleRepository->deleteWorkspaceMetadata($contentRepositoryId, $workspaceName); + $this->metadataAndRoleRepository->deleteWorkspaceRoleAssignments($contentRepositoryId, $workspaceName); + } + /** * Get all role assignments for the specified workspace * From 909223e0edfaf045dd785b512867e0f5081cec17 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Fri, 6 Dec 2024 09:06:07 +0100 Subject: [PATCH 03/24] TASK: Test `ChangeBaseWorkspace` behaviour with security --- .../Security/WorkspacePermissions.feature | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature index 7b02cca647b..5e225f3275b 100644 --- a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature +++ b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature @@ -62,6 +62,8 @@ Feature: Workspace permission related features | Key | Value | | nodeAggregateId | "a1a1a" | | nodeVariantSelectionStrategy | "allVariants" | + And the shared workspace "shared-workspace" is created with the target workspace "live" + When the role COLLABORATOR is assigned to workspace "shared-workspace" for group "Neos.Neos:AbstractEditor" And the personal workspace "workspace" is created with the target workspace "live" for user "owner" And I am in workspace "workspace" And the role MANAGER is assigned to workspace "workspace" for user "manager" @@ -112,6 +114,34 @@ Feature: Workspace permission related features | collaborator | | owner | + Scenario Outline: Changing a base workspace without MANAGE permissions or READ permissions on the base workspace + Given I am authenticated as + When the command ChangeBaseWorkspace is executed with payload and exceptions are caught: + | Key | Value | + | workspaceName | "workspace" | + | baseWorkspaceName | "shared-workspace" | + Then the last command should have thrown an exception of type "AccessDenied" with code 1729086686 + + Examples: + | user | + | editor | + | restricted_editor | + | collaborator | + | uninvolved | + + Scenario Outline: Changing a base workspace with MANAGE permissions or READ permissions on the base workspace + Given I am authenticated as + When the command ChangeBaseWorkspace is executed with payload: + | Key | Value | + | workspaceName | "workspace" | + | baseWorkspaceName | "shared-workspace" | + + Examples: + | user | + | admin | + | manager | + | owner | + Scenario Outline: Deleting a workspace without MANAGE permissions Given I am authenticated as When the command DeleteWorkspace is executed with payload '{"workspaceName":"workspace"}' and exceptions are caught From 9755c9d84ccd0a9a0270edd97ddcd6333abc1e14 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Fri, 6 Dec 2024 09:24:19 +0100 Subject: [PATCH 04/24] TASK: Ensure the live workspace is created by the WorkspaceService for permissions test --- .../Bootstrap/WorkspaceServiceTrait.php | 10 +++++ .../Security/WorkspacePermissions.feature | 44 +++++++++++-------- 2 files changed, 36 insertions(+), 18 deletions(-) diff --git a/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php b/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php index af046a39443..2df842f00eb 100644 --- a/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php +++ b/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php @@ -62,6 +62,16 @@ public function theRootWorkspaceIsCreated(string $workspaceName, string $title = )); } + /** + * @Given the live workspace exists + */ + public function theLiveWorkspaceExists(): void + { + $this->getObject(WorkspaceService::class)->createLiveWorkspaceIfMissing( + $this->currentContentRepository->id + ); + } + /** * @When the personal workspace :workspaceName is created with the target workspace :targetWorkspace for user :username */ diff --git a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature index 5e225f3275b..f397b236781 100644 --- a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature +++ b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature @@ -51,28 +51,16 @@ Feature: Workspace permission related features | manager | Neos.Neos:Editor | | collaborator | Neos.Neos:Editor | | uninvolved | Neos.Neos:Editor | - And I am in workspace "live" - And I am in dimension space point {"language":"de"} - And the command TagSubtree is executed with payload: - | Key | Value | - | nodeAggregateId | "a" | - | nodeVariantSelectionStrategy | "allSpecializations" | - | tag | "subtree_a" | - And the command DisableNodeAggregate is executed with payload: - | Key | Value | - | nodeAggregateId | "a1a1a" | - | nodeVariantSelectionStrategy | "allVariants" | + + When content repository security is enabled + And the shared workspace "shared-workspace" is created with the target workspace "live" - When the role COLLABORATOR is assigned to workspace "shared-workspace" for group "Neos.Neos:AbstractEditor" + And the role COLLABORATOR is assigned to workspace "shared-workspace" for group "Neos.Neos:AbstractEditor" + + Given I am authenticated as owner And the personal workspace "workspace" is created with the target workspace "live" for user "owner" - And I am in workspace "workspace" And the role MANAGER is assigned to workspace "workspace" for user "manager" And the role COLLABORATOR is assigned to workspace "workspace" for user "collaborator" - # The following step was added in order to make the `AddDimensionShineThrough` command viable - And I change the content dimensions in content repository "default" to: - | Identifier | Values | Generalizations | - | language | mul, de, ch | ch->de->mul | - And content repository security is enabled Scenario Outline: Creating a root workspace Given I am authenticated as @@ -195,6 +183,26 @@ Feature: Workspace permission related features | owner | Scenario Outline: Handling commands that require WRITE permissions on the workspace + # Prepare the content repository so all commands are applicable + And I am in workspace "live" and dimension space point {"language":"de"} + And the command TagSubtree is executed with payload: + | Key | Value | + | nodeAggregateId | "a" | + | nodeVariantSelectionStrategy | "allSpecializations" | + | tag | "subtree_a" | + And the command DisableNodeAggregate is executed with payload: + | Key | Value | + | nodeAggregateId | "a1a1a" | + | nodeVariantSelectionStrategy | "allVariants" | + # The following step was added in order to make the `AddDimensionShineThrough` command viable + And I change the content dimensions in content repository "default" to: + | Identifier | Values | Generalizations | + | language | mul, de, ch | ch->de->mul | + And the command RebaseWorkspace is executed with payload: + | Key | Value | + | workspaceName | "workspace" | + And I am in workspace "workspace" + When I am authenticated as "uninvolved" And the command is executed with payload '' and exceptions are caught Then the last command should have thrown an exception of type "AccessDenied" with code 1729086686 From 0257b282c57556b66547caa2f3969b734317799b Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Fri, 6 Dec 2024 09:32:24 +0100 Subject: [PATCH 05/24] TASK: Ensure the live workspace is created by the WorkspaceService for permissions test --- .../Security/WorkspacePermissions.feature | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature index f397b236781..2d0f92f359e 100644 --- a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature +++ b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature @@ -20,11 +20,8 @@ Feature: Workspace permission related features """ And using identifier "default", I define a content repository And I am in content repository "default" - And the command CreateRootWorkspace is executed with payload: - | Key | Value | - | workspaceName | "live" | - | newContentStreamId | "cs-identifier" | - And I am in workspace "live" and dimension space point {} + And the live workspace exists + And I am in workspace "live" And the command CreateRootNodeAggregateWithNode is executed with payload: | Key | Value | | nodeAggregateId | "root" | From d514e61fe1ce9ed604f6298fb509096d5afa1e89 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Fri, 6 Dec 2024 10:36:57 +0100 Subject: [PATCH 06/24] TASK: Allow workspace creation with only base workspace read privilege as change base workspace would allow either way to repoint the base --- .../ContentRepositoryAuthProvider.php | 8 +++- .../Features/Bootstrap/UserServiceTrait.php | 8 ++-- .../Security/WorkspacePermissions.feature | 40 +++++++++++++++++-- 3 files changed, 49 insertions(+), 7 deletions(-) diff --git a/Neos.Neos/Classes/Security/ContentRepositoryAuthProvider/ContentRepositoryAuthProvider.php b/Neos.Neos/Classes/Security/ContentRepositoryAuthProvider/ContentRepositoryAuthProvider.php index 74bfd788aa4..98add1e9808 100644 --- a/Neos.Neos/Classes/Security/ContentRepositoryAuthProvider/ContentRepositoryAuthProvider.php +++ b/Neos.Neos/Classes/Security/ContentRepositoryAuthProvider/ContentRepositoryAuthProvider.php @@ -116,6 +116,13 @@ public function canExecuteCommand(CommandInterface $command): Privilege if ($command instanceof CreateRootWorkspace) { return Privilege::denied('Creation of root workspaces is currently only allowed with disabled authorization checks'); } + if ($command instanceof CreateWorkspace) { + $baseWorkspacePermissions = $this->getWorkspacePermissionsForCurrentUser($command->baseWorkspaceName); + if (!$baseWorkspacePermissions->read) { + return Privilege::denied(sprintf('Missing "read" permissions for base workspace "%s": %s', $command->baseWorkspaceName->value, $baseWorkspacePermissions->getReason())); + } + return Privilege::granted(sprintf('User has "read" permissions for base workspace "%s"', $command->baseWorkspaceName->value)); + } if ($command instanceof ChangeBaseWorkspace) { $workspacePermissions = $this->getWorkspacePermissionsForCurrentUser($command->workspaceName); if (!$workspacePermissions->manage) { @@ -139,7 +146,6 @@ public function canExecuteCommand(CommandInterface $command): Privilege PublishWorkspace::class, PublishIndividualNodesFromWorkspace::class, RebaseWorkspace::class => $this->requireWorkspaceWritePermission($command->workspaceName), - CreateWorkspace::class => $this->requireWorkspaceWritePermission($command->baseWorkspaceName), DeleteWorkspace::class => $this->requireWorkspaceManagePermission($command->workspaceName), default => Privilege::granted('Command not restricted'), }; diff --git a/Neos.Neos/Tests/Behavior/Features/Bootstrap/UserServiceTrait.php b/Neos.Neos/Tests/Behavior/Features/Bootstrap/UserServiceTrait.php index da251d624fb..7585fdf492f 100644 --- a/Neos.Neos/Tests/Behavior/Features/Bootstrap/UserServiceTrait.php +++ b/Neos.Neos/Tests/Behavior/Features/Bootstrap/UserServiceTrait.php @@ -16,10 +16,8 @@ use Neos\Flow\Persistence\PersistenceManagerInterface; use Neos\Flow\Security\AccountFactory; use Neos\Flow\Security\Cryptography\HashService; -use Neos\Flow\Security\Policy\PolicyService; use Neos\Neos\Domain\Model\User; use Neos\Neos\Domain\Service\UserService; -use Neos\Neos\Security\Authorization\Privilege\ReadNodePrivilege; use Neos\Party\Domain\Model\PersonName; use Neos\Utility\ObjectAccess; @@ -61,11 +59,14 @@ public function theNeosUserExists(string $username, string $id = null, string $f public function theFollowingNeosUsersExist(TableNode $usersTable): void { foreach ($usersTable->getHash() as $userData) { + if (empty($userData['Roles'])) { + throw new \InvalidArgumentException('Please specify explicit roles for the Neos user, to avoid using any fallbacks.'); + } $this->createUser( username: $userData['Username'], firstName: $userData['First name'] ?? null, lastName: $userData['Last name'] ?? null, - roleIdentifiers: !empty($userData['Roles']) ? explode(',', $userData['Roles']) : null, + roleIdentifiers: explode(',', $userData['Roles']), id: $userData['Id'] ?? null, ); } @@ -81,6 +82,7 @@ private function createUser(string $username, string $firstName = null, string $ $accountFactory = $this->getObject(AccountFactory::class); + // todo either hack the global hash service or fix flow to avoid having to inline this code // NOTE: We replace the original {@see HashService} by a "mock" for performance reasons (the default hashing strategy usually takes a while to create passwords) /** @var HashService $originalHashService */ diff --git a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature index 2d0f92f359e..1d289ac2ad8 100644 --- a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature +++ b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature @@ -42,12 +42,15 @@ Feature: Workspace permission related features And the following Neos users exist: | Username | Roles | | admin | Neos.Neos:Administrator | + # editors are Neos.Neos:LivePublisher | editor | Neos.Neos:Editor | - | restricted_editor | Neos.Neos:RestrictedEditor | | owner | Neos.Neos:Editor | | manager | Neos.Neos:Editor | | collaborator | Neos.Neos:Editor | + | restricted_editor | Neos.Neos:RestrictedEditor | | uninvolved | Neos.Neos:Editor | + # neos user with out any editing roles + | simple_user | Neos.Neos:UserManager | When content repository security is enabled @@ -72,8 +75,9 @@ Feature: Workspace permission related features | owner | | collaborator | | uninvolved | + | simple_user | - Scenario Outline: Creating a base workspace without WRITE permissions + Scenario Outline: Creating a nested workspace without READ permissions Given I am authenticated as And the shared workspace "some-shared-workspace" is created with the target workspace "workspace" Then an exception of type "AccessDenied" should be thrown with code 1729086686 @@ -87,8 +91,9 @@ Feature: Workspace permission related features | editor | | restricted_editor | | uninvolved | + | simple_user | - Scenario Outline: Creating a base workspace with WRITE permissions + Scenario Outline: Creating a nested workspace with READ permissions Given I am authenticated as And the shared workspace "some-shared-workspace" is created with the target workspace "workspace" @@ -99,6 +104,33 @@ Feature: Workspace permission related features | collaborator | | owner | + Scenario Outline: Creating a workspace without READ permissions (on live) + Given I am authenticated as + And the shared workspace "some-shared-workspace" is created with the target workspace "live" + Then an exception of type "AccessDenied" should be thrown with code 1729086686 + + And the personal workspace "some-other-personal-workspace" is created with the target workspace "live" for user + Then an exception of type "AccessDenied" should be thrown with code 1729086686 + + Examples: + | user | + | restricted_editor | + | simple_user | + + Scenario Outline: Creating a workspace with READ permissions (on live) + Given I am authenticated as + And the shared workspace "some-shared-workspace" is created with the target workspace "live" + + And the personal workspace "some-other-personal-workspace" is created with the target workspace "live" for user + + Examples: + | user | + | admin | + | editor | + | owner | + | collaborator | + | uninvolved | + Scenario Outline: Changing a base workspace without MANAGE permissions or READ permissions on the base workspace Given I am authenticated as When the command ChangeBaseWorkspace is executed with payload and exceptions are caught: @@ -136,6 +168,7 @@ Feature: Workspace permission related features | user | | collaborator | | uninvolved | + | simple_user | Scenario Outline: Deleting a workspace with MANAGE permissions Given I am authenticated as @@ -247,5 +280,6 @@ Feature: Workspace permission related features | PublishWorkspace | {} | | PublishIndividualNodesFromWorkspace | {"nodesToPublish":[{"nodeAggregateId":"a1"}]} | | RebaseWorkspace | {} | + # note, creating a core workspace will not grant permissions to it to the current user: Missing "read" permissions for base workspace "new-workspace" | CreateWorkspace | {"workspaceName":"new-workspace","baseWorkspaceName":"workspace","newContentStreamId":"any"} | From 484c1abfd1602f70bd95b10988c3ceb3cce9f41c Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Fri, 6 Dec 2024 11:03:32 +0100 Subject: [PATCH 07/24] BUGFIX: Only allow to publish if the current user can write to the base --- .../ContentRepositoryAuthProvider.php | 27 +++++++++- .../Security/WorkspacePermissions.feature | 54 ++++++++++++++++++- 2 files changed, 77 insertions(+), 4 deletions(-) diff --git a/Neos.Neos/Classes/Security/ContentRepositoryAuthProvider/ContentRepositoryAuthProvider.php b/Neos.Neos/Classes/Security/ContentRepositoryAuthProvider/ContentRepositoryAuthProvider.php index 98add1e9808..73c4bb6bf3f 100644 --- a/Neos.Neos/Classes/Security/ContentRepositoryAuthProvider/ContentRepositoryAuthProvider.php +++ b/Neos.Neos/Classes/Security/ContentRepositoryAuthProvider/ContentRepositoryAuthProvider.php @@ -26,6 +26,7 @@ use Neos\ContentRepository\Core\Feature\SubtreeTagging\Command\UntagSubtree; use Neos\ContentRepository\Core\Feature\WorkspaceCreation\Command\CreateRootWorkspace; use Neos\ContentRepository\Core\Feature\WorkspaceCreation\Command\CreateWorkspace; +use Neos\ContentRepository\Core\Feature\WorkspaceCreation\Exception\BaseWorkspaceDoesNotExist; use Neos\ContentRepository\Core\Feature\WorkspaceModification\Command\ChangeBaseWorkspace; use Neos\ContentRepository\Core\Feature\WorkspaceModification\Command\DeleteWorkspace; use Neos\ContentRepository\Core\Feature\WorkspacePublication\Command\DiscardIndividualNodesFromWorkspace; @@ -36,6 +37,8 @@ use Neos\ContentRepository\Core\Projection\ContentGraph\ContentGraphReadModelInterface; use Neos\ContentRepository\Core\Projection\ContentGraph\VisibilityConstraints; use Neos\ContentRepository\Core\SharedModel\ContentRepository\ContentRepositoryId; +use Neos\ContentRepository\Core\SharedModel\Exception\WorkspaceDoesNotExist; +use Neos\ContentRepository\Core\SharedModel\Exception\WorkspaceHasNoBaseWorkspaceName; use Neos\ContentRepository\Core\SharedModel\Node\NodeAddress; use Neos\ContentRepository\Core\SharedModel\Workspace\WorkspaceName; use Neos\Flow\Security\Context as SecurityContext; @@ -134,6 +137,28 @@ public function canExecuteCommand(CommandInterface $command): Privilege } return Privilege::granted(sprintf('User has "manage" permissions for workspace "%s" and "read" permissions for base workspace "%s"', $command->workspaceName->value, $command->baseWorkspaceName->value)); } + if ($command instanceof PublishWorkspace || $command instanceof PublishIndividualNodesFromWorkspace) { + $workspacePermissions = $this->getWorkspacePermissionsForCurrentUser($command->workspaceName); + if (!$workspacePermissions->write) { + return Privilege::denied(sprintf('Missing "write" permissions for workspace "%s": %s', $command->workspaceName->value, $workspacePermissions->getReason())); + } + $workspace = $this->contentGraphReadModel->findWorkspaceByName($command->workspaceName); + if ($workspace === null) { + throw WorkspaceDoesNotExist::butWasSupposedTo($command->workspaceName); + } + if ($workspace->baseWorkspaceName === null) { + throw WorkspaceHasNoBaseWorkspaceName::butWasSupposedTo($workspace->workspaceName); + } + $baseWorkspace = $this->contentGraphReadModel->findWorkspaceByName($workspace->baseWorkspaceName); + if ($baseWorkspace === null) { + throw BaseWorkspaceDoesNotExist::butWasSupposedTo($workspace->workspaceName); + } + $baseWorkspacePermissions = $this->getWorkspacePermissionsForCurrentUser($baseWorkspace->workspaceName); + if (!$baseWorkspacePermissions->write) { + return Privilege::denied(sprintf('Missing "write" permissions for base workspace "%s": %s', $baseWorkspace->workspaceName->value, $baseWorkspacePermissions->getReason())); + } + return Privilege::granted(sprintf('User has "manage" permissions for workspace "%s" and "write" permissions for base workspace "%s"', $command->workspaceName->value, $baseWorkspace->workspaceName->value)); + } return match ($command::class) { AddDimensionShineThrough::class, ChangeNodeAggregateName::class, @@ -143,8 +168,6 @@ public function canExecuteCommand(CommandInterface $command): Privilege UpdateRootNodeAggregateDimensions::class, DiscardWorkspace::class, DiscardIndividualNodesFromWorkspace::class, - PublishWorkspace::class, - PublishIndividualNodesFromWorkspace::class, RebaseWorkspace::class => $this->requireWorkspaceWritePermission($command->workspaceName), DeleteWorkspace::class => $this->requireWorkspaceManagePermission($command->workspaceName), default => Privilege::granted('Command not restricted'), diff --git a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature index 1d289ac2ad8..9f914acff6f 100644 --- a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature +++ b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature @@ -277,9 +277,59 @@ Feature: Workspace permission related features | UpdateRootNodeAggregateDimensions | {"nodeAggregateId":"root"} | | DiscardWorkspace | {} | | DiscardIndividualNodesFromWorkspace | {"nodesToDiscard":[{"nodeAggregateId":"a1"}]} | - | PublishWorkspace | {} | - | PublishIndividualNodesFromWorkspace | {"nodesToPublish":[{"nodeAggregateId":"a1"}]} | | RebaseWorkspace | {} | # note, creating a core workspace will not grant permissions to it to the current user: Missing "read" permissions for base workspace "new-workspace" | CreateWorkspace | {"workspaceName":"new-workspace","baseWorkspaceName":"workspace","newContentStreamId":"any"} | + Scenario Outline: Publishing a workspace without WRITE permissions to live + # make changes as owner + Given I am authenticated as owner + + And the following CreateNodeAggregateWithNode commands are executed: + | nodeAggregateId | nodeTypeName | parentNodeAggregateId | workspaceName | originDimensionSpacePoint | + | shernode-homes | Neos.Neos:Document | a | workspace | {"language":"de"} | + | other-node | Neos.Neos:Document | a | workspace | {"language":"de"} | + + # someone else attempts to publish + Given I am authenticated as + + And the command PublishIndividualNodesFromWorkspace is executed with payload and exceptions are caught: + | Key | Value | + | workspaceName | "workspace" | + | nodesToPublish | [{"nodeAggregateId":"shernode-homes"}] | + Then the last command should have thrown an exception of type "AccessDenied" with code 1729086686 + + And the command PublishWorkspace is executed with payload and exceptions are caught: + | Key | Value | + | workspaceName | "workspace" | + Then the last command should have thrown an exception of type "AccessDenied" with code 1729086686 + + Examples: + | user | + | restricted_editor | + | simple_user | + | uninvolved | + | editor | + | admin | + + Scenario Outline: Publishing a workspace with WRITE permissions to live + Given I am authenticated as + + And the following CreateNodeAggregateWithNode commands are executed: + | nodeAggregateId | nodeTypeName | parentNodeAggregateId | workspaceName | originDimensionSpacePoint | + | shernode-homes | Neos.Neos:Document | a | workspace | {"language":"de"} | + | other-node | Neos.Neos:Document | a | workspace | {"language":"de"} | + + And the command PublishIndividualNodesFromWorkspace is executed with payload: + | Key | Value | + | workspaceName | "workspace" | + | nodesToPublish | [{"nodeAggregateId":"shernode-homes"}] | + + And the command PublishWorkspace is executed with payload: + | Key | Value | + | workspaceName | "workspace" | + + Examples: + | user | + | owner | + | collaborator | From 74cfaaa05c5abc23f1fa0164d7f1c54f2b693e6d Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Fri, 6 Dec 2024 11:08:43 +0100 Subject: [PATCH 08/24] TASK: Simplify test to only have one `uninvolved_editor` --- .../Security/WorkspacePermissions.feature | 39 +++++++------------ 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature index 9f914acff6f..90d1f828b36 100644 --- a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature +++ b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature @@ -42,13 +42,12 @@ Feature: Workspace permission related features And the following Neos users exist: | Username | Roles | | admin | Neos.Neos:Administrator | - # editors are Neos.Neos:LivePublisher - | editor | Neos.Neos:Editor | + # all editors are Neos.Neos:LivePublisher | owner | Neos.Neos:Editor | | manager | Neos.Neos:Editor | | collaborator | Neos.Neos:Editor | | restricted_editor | Neos.Neos:RestrictedEditor | - | uninvolved | Neos.Neos:Editor | + | uninvolved_editor | Neos.Neos:Editor | # neos user with out any editing roles | simple_user | Neos.Neos:UserManager | @@ -70,11 +69,10 @@ Feature: Workspace permission related features Examples: | user | | admin | - | editor | | restricted_editor | | owner | | collaborator | - | uninvolved | + | uninvolved_editor | | simple_user | Scenario Outline: Creating a nested workspace without READ permissions @@ -88,9 +86,8 @@ Feature: Workspace permission related features Examples: | user | | admin | - | editor | | restricted_editor | - | uninvolved | + | uninvolved_editor | | simple_user | Scenario Outline: Creating a nested workspace with READ permissions @@ -126,10 +123,9 @@ Feature: Workspace permission related features Examples: | user | | admin | - | editor | | owner | | collaborator | - | uninvolved | + | uninvolved_editor | Scenario Outline: Changing a base workspace without MANAGE permissions or READ permissions on the base workspace Given I am authenticated as @@ -141,10 +137,9 @@ Feature: Workspace permission related features Examples: | user | - | editor | | restricted_editor | | collaborator | - | uninvolved | + | uninvolved_editor | Scenario Outline: Changing a base workspace with MANAGE permissions or READ permissions on the base workspace Given I am authenticated as @@ -165,10 +160,10 @@ Feature: Workspace permission related features Then the last command should have thrown an exception of type "AccessDenied" with code 1729086686 Examples: - | user | - | collaborator | - | uninvolved | - | simple_user | + | user | + | collaborator | + | uninvolved_editor | + | simple_user | Scenario Outline: Deleting a workspace with MANAGE permissions Given I am authenticated as @@ -195,9 +190,9 @@ Feature: Workspace permission related features Then an exception of type "AccessDenied" should be thrown with code 1731654519 Examples: - | user | - | collaborator | - | uninvolved | + | user | + | collaborator | + | uninvolved_editor | Scenario Outline: Managing metadata and roles of a workspace with MANAGE permissions Given I am authenticated as @@ -234,10 +229,7 @@ Feature: Workspace permission related features And I am in workspace "workspace" When I am authenticated as "uninvolved" - And the command is executed with payload '' and exceptions are caught - Then the last command should have thrown an exception of type "AccessDenied" with code 1729086686 - - When I am authenticated as "editor" + When I am authenticated as "uninvolved_editor" And the command is executed with payload '' and exceptions are caught Then the last command should have thrown an exception of type "AccessDenied" with code 1729086686 @@ -308,8 +300,7 @@ Feature: Workspace permission related features | user | | restricted_editor | | simple_user | - | uninvolved | - | editor | + | uninvolved_editor | | admin | Scenario Outline: Publishing a workspace with WRITE permissions to live From a7d887f7d8defe3567c13487817ffb7b0e2dc1df Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Fri, 6 Dec 2024 11:18:12 +0100 Subject: [PATCH 09/24] BUGFIX: Ensure that the live workspace is created WITH public viewing permission --- .../Classes/Service/EventMigrationService.php | 2 +- .../Domain/Import/LiveWorkspaceCreationProcessor.php | 7 +------ Neos.Neos/Classes/Domain/Service/WorkspaceService.php | 1 + .../ContentRepository/Security/EditNodePrivilege.feature | 5 +---- .../ContentRepository/Security/ReadNodePrivilege.feature | 6 +----- 5 files changed, 5 insertions(+), 16 deletions(-) diff --git a/Neos.ContentRepositoryRegistry/Classes/Service/EventMigrationService.php b/Neos.ContentRepositoryRegistry/Classes/Service/EventMigrationService.php index 80d849bb17c..3348f1b73b3 100644 --- a/Neos.ContentRepositoryRegistry/Classes/Service/EventMigrationService.php +++ b/Neos.ContentRepositoryRegistry/Classes/Service/EventMigrationService.php @@ -716,7 +716,7 @@ public function migrateWorkspaceMetadataToWorkspaceService(\Closure $outputFn): ]; $roleAssignments[] = [ 'subject_type' => WorkspaceRoleSubjectType::GROUP->value, - 'subject' => 'Neos.Neos:Everybody', + 'subject' => 'Neos.Flow:Everybody', 'role' => WorkspaceRole::VIEWER->value, ]; } elseif ($isInternalWorkspace) { diff --git a/Neos.Neos/Classes/Domain/Import/LiveWorkspaceCreationProcessor.php b/Neos.Neos/Classes/Domain/Import/LiveWorkspaceCreationProcessor.php index 788dad93734..58ba49be52e 100644 --- a/Neos.Neos/Classes/Domain/Import/LiveWorkspaceCreationProcessor.php +++ b/Neos.Neos/Classes/Domain/Import/LiveWorkspaceCreationProcessor.php @@ -19,10 +19,6 @@ use Neos\ContentRepository\Export\ProcessingContext; use Neos\ContentRepository\Export\ProcessorInterface; use Neos\ContentRepository\Export\Severity; -use Neos\Neos\Domain\Model\WorkspaceDescription; -use Neos\Neos\Domain\Model\WorkspaceRole; -use Neos\Neos\Domain\Model\WorkspaceRoleAssignment; -use Neos\Neos\Domain\Model\WorkspaceTitle; use Neos\Neos\Domain\Service\WorkspaceService; /** @@ -44,7 +40,6 @@ public function run(ProcessingContext $context): void $context->dispatch(Severity::NOTICE, 'Workspace already exists, skipping'); return; } - $this->workspaceService->createRootWorkspace($this->contentRepository->id, WorkspaceName::forLive(), WorkspaceTitle::fromString('Live workspace'), WorkspaceDescription::fromString('')); - $this->workspaceService->assignWorkspaceRole($this->contentRepository->id, WorkspaceName::forLive(), WorkspaceRoleAssignment::createForGroup('Neos.Neos:LivePublisher', WorkspaceRole::COLLABORATOR)); + $this->workspaceService->createLiveWorkspaceIfMissing($this->contentRepository->id); } } diff --git a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php index fae30ec7d60..96a5a5edb4d 100644 --- a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php +++ b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php @@ -140,6 +140,7 @@ public function createLiveWorkspaceIfMissing(ContentRepositoryId $contentReposit } $this->createRootWorkspace($contentRepositoryId, $workspaceName, WorkspaceTitle::fromString('Public live workspace'), WorkspaceDescription::empty()); $this->metadataAndRoleRepository->assignWorkspaceRole($contentRepositoryId, $workspaceName, WorkspaceRoleAssignment::createForGroup('Neos.Neos:LivePublisher', WorkspaceRole::COLLABORATOR)); + $this->metadataAndRoleRepository->assignWorkspaceRole($contentRepositoryId, $workspaceName, WorkspaceRoleAssignment::createForGroup('Neos.Flow:Everybody', WorkspaceRole::VIEWER)); } /** diff --git a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/EditNodePrivilege.feature b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/EditNodePrivilege.feature index d279d035287..2355a2f72f4 100644 --- a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/EditNodePrivilege.feature +++ b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/EditNodePrivilege.feature @@ -29,10 +29,7 @@ Feature: EditNodePrivilege related features """ And using identifier "default", I define a content repository And I am in content repository "default" - And the command CreateRootWorkspace is executed with payload: - | Key | Value | - | workspaceName | "live" | - | newContentStreamId | "cs-identifier" | + And the live workspace exists And I am in workspace "live" and dimension space point {} And the command CreateRootNodeAggregateWithNode is executed with payload: | Key | Value | diff --git a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/ReadNodePrivilege.feature b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/ReadNodePrivilege.feature index 2545f9c7bac..02cfd9b7e80 100644 --- a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/ReadNodePrivilege.feature +++ b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/ReadNodePrivilege.feature @@ -29,10 +29,7 @@ Feature: ReadNodePrivilege related features """ And using identifier "default", I define a content repository And I am in content repository "default" - And the command CreateRootWorkspace is executed with payload: - | Key | Value | - | workspaceName | "live" | - | newContentStreamId | "cs-identifier" | + And the live workspace exists And I am in workspace "live" and dimension space point {} And the command CreateRootNodeAggregateWithNode is executed with payload: | Key | Value | @@ -64,7 +61,6 @@ Feature: ReadNodePrivilege related features | nodeAggregateId | "a" | | nodeVariantSelectionStrategy | "allSpecializations" | | tag | "subtree_a" | - And the role VIEWER is assigned to workspace "live" for group "Neos.Flow:Everybody" When a personal workspace for user "editor" is created And content repository security is enabled From 28b98c50c69238e659f16cb6827299b5ee9cd782 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Sat, 7 Dec 2024 15:52:59 +0100 Subject: [PATCH 10/24] TASK: Add test for not authenticated case (https://github.com/neos/neos-development-collection/pull/5334#issuecomment-2523611432) --- .../Domain/Model/WorkspaceRoleAssignment.php | 3 +-- .../Features/Bootstrap/FlowSecurityTrait.php | 12 +++++++++++ .../Security/WorkspacePermissions.feature | 21 ++++++++----------- 3 files changed, 22 insertions(+), 14 deletions(-) diff --git a/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignment.php b/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignment.php index b8af8edae39..418e7134e22 100644 --- a/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignment.php +++ b/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignment.php @@ -45,8 +45,7 @@ public static function createForGroup(string $flowRoleIdentifier, WorkspaceRole public function equals(WorkspaceRoleAssignment $other): bool { - return $this->subjectType === $other->subjectType - && $this->subject->equals($other->subject) + return $this->subject->equals($other->subject) && $this->role === $other->role; } } diff --git a/Neos.Neos/Tests/Behavior/Features/Bootstrap/FlowSecurityTrait.php b/Neos.Neos/Tests/Behavior/Features/Bootstrap/FlowSecurityTrait.php index 5f4915dfcf1..b7c166602fa 100644 --- a/Neos.Neos/Tests/Behavior/Features/Bootstrap/FlowSecurityTrait.php +++ b/Neos.Neos/Tests/Behavior/Features/Bootstrap/FlowSecurityTrait.php @@ -132,4 +132,16 @@ public function getConfiguration(string $configurationType, string $configuratio } }); } + + /** + * @When I am not authenticated + */ + final public function iAmNotAuthenticated(): void + { + $this->flowSecurity_testingProvider->reset(); + $securityContext = $this->getObject(SecurityContext::class); + $securityContext->clearContext(); + $securityContext->setRequest($this->flowSecurity_mockActionRequest); + $this->getObject(AuthenticationProviderManager::class)->authenticate(); + } } diff --git a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature index 90d1f828b36..30644bc547d 100644 --- a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature +++ b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature @@ -101,18 +101,9 @@ Feature: Workspace permission related features | collaborator | | owner | - Scenario Outline: Creating a workspace without READ permissions (on live) - Given I am authenticated as + Scenario: Creating a workspace without Neos User but READ permissions on live + Given I am not authenticated And the shared workspace "some-shared-workspace" is created with the target workspace "live" - Then an exception of type "AccessDenied" should be thrown with code 1729086686 - - And the personal workspace "some-other-personal-workspace" is created with the target workspace "live" for user - Then an exception of type "AccessDenied" should be thrown with code 1729086686 - - Examples: - | user | - | restricted_editor | - | simple_user | Scenario Outline: Creating a workspace with READ permissions (on live) Given I am authenticated as @@ -126,6 +117,8 @@ Feature: Workspace permission related features | owner | | collaborator | | uninvolved_editor | + | restricted_editor | + | simple_user | Scenario Outline: Changing a base workspace without MANAGE permissions or READ permissions on the base workspace Given I am authenticated as @@ -226,9 +219,13 @@ Feature: Workspace permission related features And the command RebaseWorkspace is executed with payload: | Key | Value | | workspaceName | "workspace" | + And I am in workspace "workspace" - When I am authenticated as "uninvolved" + Given I am not authenticated + And the command is executed with payload '' and exceptions are caught + Then the last command should have thrown an exception of type "AccessDenied" with code 1729086686 + When I am authenticated as "uninvolved_editor" And the command is executed with payload '' and exceptions are caught Then the last command should have thrown an exception of type "AccessDenied" with code 1729086686 From 8ee852155bf28dcc743a4fb45100c9e44c8e53e5 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Sat, 7 Dec 2024 16:03:58 +0100 Subject: [PATCH 11/24] TASK: Fix workspace permission test to no be in authorizationChecksDisabled mode after enabling security --- .../Tests/Behavior/Features/Bootstrap/FlowSecurityTrait.php | 1 + .../ContentRepository/Security/WorkspacePermissions.feature | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/Neos.Neos/Tests/Behavior/Features/Bootstrap/FlowSecurityTrait.php b/Neos.Neos/Tests/Behavior/Features/Bootstrap/FlowSecurityTrait.php index b7c166602fa..23c6b9e347d 100644 --- a/Neos.Neos/Tests/Behavior/Features/Bootstrap/FlowSecurityTrait.php +++ b/Neos.Neos/Tests/Behavior/Features/Bootstrap/FlowSecurityTrait.php @@ -83,6 +83,7 @@ final protected function enableFlowSecurity(): void $this->flowSecurity_testingProvider = $tokenAndProviderFactory->getProviders()['TestingProvider']; $securityContext = $this->getObject(SecurityContext::class); + $securityContext->clearContext(); // enable authorizationChecks $httpRequest = $this->getObject(ServerRequestFactoryInterface::class)->createServerRequest('GET', 'http://localhost/'); $this->flowSecurity_mockActionRequest = ActionRequest::fromHttpRequest($httpRequest); $securityContext->setRequest($this->flowSecurity_mockActionRequest); diff --git a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature index 30644bc547d..800d45c8bcd 100644 --- a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature +++ b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature @@ -51,16 +51,15 @@ Feature: Workspace permission related features # neos user with out any editing roles | simple_user | Neos.Neos:UserManager | - When content repository security is enabled - And the shared workspace "shared-workspace" is created with the target workspace "live" And the role COLLABORATOR is assigned to workspace "shared-workspace" for group "Neos.Neos:AbstractEditor" - Given I am authenticated as owner And the personal workspace "workspace" is created with the target workspace "live" for user "owner" And the role MANAGER is assigned to workspace "workspace" for user "manager" And the role COLLABORATOR is assigned to workspace "workspace" for user "collaborator" + When content repository security is enabled + Scenario Outline: Creating a root workspace Given I am authenticated as When the command CreateRootWorkspace is executed with payload '{"workspaceName":"new-ws","newContentStreamId":"new-cs"}' and exceptions are caught @@ -202,6 +201,7 @@ Feature: Workspace permission related features Scenario Outline: Handling commands that require WRITE permissions on the workspace # Prepare the content repository so all commands are applicable + When I am authenticated as "owner" And I am in workspace "live" and dimension space point {"language":"de"} And the command TagSubtree is executed with payload: | Key | Value | From 6351ce69d3e82c3bb80beb822ba74856a0cdd9d7 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Mon, 9 Dec 2024 10:55:05 +0100 Subject: [PATCH 12/24] TASK: Use `deleteWorkspace` from service in cli command and test --- .../Command/WorkspaceCommandController.php | 7 +-- .../WorkspaceMetadataAndRoleRepository.php | 30 ++-------- .../Domain/Service/WorkspaceService.php | 1 + .../Bootstrap/WorkspaceServiceTrait.php | 59 +++++++++++++++---- .../WorkspaceService.feature | 21 ++++++- 5 files changed, 74 insertions(+), 44 deletions(-) diff --git a/Neos.Neos/Classes/Command/WorkspaceCommandController.php b/Neos.Neos/Classes/Command/WorkspaceCommandController.php index 876e5b8d199..fa834b4258d 100644 --- a/Neos.Neos/Classes/Command/WorkspaceCommandController.php +++ b/Neos.Neos/Classes/Command/WorkspaceCommandController.php @@ -15,7 +15,6 @@ namespace Neos\Neos\Command; use Neos\ContentRepository\Core\Feature\WorkspaceCreation\Exception\WorkspaceAlreadyExists; -use Neos\ContentRepository\Core\Feature\WorkspaceModification\Command\DeleteWorkspace; use Neos\ContentRepository\Core\Feature\WorkspaceRebase\Dto\RebaseErrorHandlingStrategy; use Neos\ContentRepository\Core\Feature\WorkspaceRebase\Exception\WorkspaceRebaseFailed; use Neos\ContentRepository\Core\Service\WorkspaceMaintenanceServiceFactory; @@ -400,11 +399,7 @@ public function deleteCommand(string $workspace, bool $force = false, string $co $this->workspacePublishingService->discardAllWorkspaceChanges($contentRepositoryId, $workspaceName); } - $contentRepositoryInstance->handle( - DeleteWorkspace::create( - $workspaceName - ) - ); + $this->workspaceService->deleteWorkspace($contentRepositoryId, $workspaceName); $this->outputLine('Deleted workspace "%s"', [$workspaceName->value]); } diff --git a/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php b/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php index bc551b40ba4..dd2eb788c72 100644 --- a/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php +++ b/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php @@ -180,19 +180,10 @@ public function getMostPrivilegedWorkspaceRoleForSubjects(ContentRepositoryId $c public function deleteWorkspaceMetadata(ContentRepositoryId $contentRepositoryId, WorkspaceName $workspaceName): void { - $table = self::TABLE_NAME_WORKSPACE_METADATA; - $query = <<dbal->executeStatement($query, [ - 'contentRepositoryId' => $contentRepositoryId->value, - 'workspaceName' => $workspaceName->value, + $this->dbal->delete(self::TABLE_NAME_WORKSPACE_METADATA, [ + 'content_repository_id' => $contentRepositoryId->value, + 'workspace_name' => $workspaceName->value, ]); } catch (DbalException $e) { throw new \RuntimeException(sprintf( @@ -206,19 +197,10 @@ public function deleteWorkspaceMetadata(ContentRepositoryId $contentRepositoryId public function deleteWorkspaceRoleAssignments(ContentRepositoryId $contentRepositoryId, WorkspaceName $workspaceName): void { - $table = self::TABLE_NAME_WORKSPACE_ROLE; - $query = <<dbal->executeStatement($query, [ - 'contentRepositoryId' => $contentRepositoryId->value, - 'workspaceName' => $workspaceName->value, + $this->dbal->delete(self::TABLE_NAME_WORKSPACE_ROLE, [ + 'content_repository_id' => $contentRepositoryId->value, + 'workspace_name' => $workspaceName->value, ]); } catch (DbalException $e) { throw new \RuntimeException(sprintf( diff --git a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php index 96a5a5edb4d..5a652be3354 100644 --- a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php +++ b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php @@ -231,6 +231,7 @@ public function deleteWorkspace(ContentRepositoryId $contentRepositoryId, Worksp */ public function getWorkspaceRoleAssignments(ContentRepositoryId $contentRepositoryId, WorkspaceName $workspaceName): WorkspaceRoleAssignments { + $this->requireWorkspace($contentRepositoryId, $workspaceName); return $this->metadataAndRoleRepository->getWorkspaceRoleAssignments($contentRepositoryId, $workspaceName); } diff --git a/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php b/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php index 2df842f00eb..b71ee82bb3b 100644 --- a/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php +++ b/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php @@ -62,6 +62,17 @@ public function theRootWorkspaceIsCreated(string $workspaceName, string $title = )); } + /** + * @When the workspace :workspaceName is deleted + */ + public function theWorkspaceIsDeleted(string $workspaceName): void + { + $this->tryCatchingExceptions(fn () => $this->getObject(WorkspaceService::class)->deleteWorkspace( + $this->currentContentRepository->id, + WorkspaceName::fromString($workspaceName), + )); + } + /** * @Given the live workspace exists */ @@ -125,18 +136,6 @@ public function aRootWorkspaceExistsWithoutMetadata(string $workspaceName): void )); } - /** - * @When a workspace :workspaceName with base workspace :baseWorkspaceName exists without metadata - */ - public function aWorkspaceWithBaseWorkspaceExistsWithoutMetadata(string $workspaceName, string $baseWorkspaceName): void - { - $this->currentContentRepository->handle(CreateWorkspace::create( - WorkspaceName::fromString($workspaceName), - WorkspaceName::fromString($baseWorkspaceName), - ContentStreamId::create(), - )); - } - /** * @When the title of workspace :workspaceName is set to :newTitle */ @@ -175,6 +174,42 @@ public function theWorkspaceShouldHaveTheFollowingMetadata($workspaceName, Table ]); } + /** + * @Then the metadata for workspace :workspaceName does not exist + */ + public function theWorkspaceMetadataFails($workspaceName): void + { + $metaData = $this->getObject(\Neos\Neos\Domain\Repository\WorkspaceMetadataAndRoleRepository::class)->loadWorkspaceMetadata($this->currentContentRepository->id, WorkspaceName::fromString($workspaceName)); + Assert::assertNull($metaData); + + // asking the API FAILS! + try { + $this->getObject(WorkspaceService::class)->getWorkspaceMetadata($this->currentContentRepository->id, WorkspaceName::fromString($workspaceName)); + } catch (\Throwable $e) { + Assert::assertInstanceOf(\RuntimeException::class, $e, $e->getMessage()); + return; + } + Assert::fail('Did not throw'); + } + + /** + * @Then the roles for workspace :workspaceName does not exist + */ + public function theWorkspaceRolesFails($workspaceName): void + { + $roles = $this->getObject(\Neos\Neos\Domain\Repository\WorkspaceMetadataAndRoleRepository::class)->getWorkspaceRoleAssignments($this->currentContentRepository->id, WorkspaceName::fromString($workspaceName)); + Assert::assertTrue($roles->isEmpty()); + + // asking the API FAILS! + try { + $this->getObject(WorkspaceService::class)->getWorkspaceRoleAssignments($this->currentContentRepository->id, WorkspaceName::fromString($workspaceName)); + } catch (\Throwable $e) { + Assert::assertInstanceOf(\RuntimeException::class, $e, $e->getMessage()); + return; + } + Assert::fail('Did not throw'); + } + /** * @When the role :role is assigned to workspace :workspaceName for group :groupName * @When the role :role is assigned to workspace :workspaceName for user :username diff --git a/Neos.Neos/Tests/Behavior/Features/ContentRepository/WorkspaceService.feature b/Neos.Neos/Tests/Behavior/Features/ContentRepository/WorkspaceService.feature index 1bdac437c30..453f393a0e1 100644 --- a/Neos.Neos/Tests/Behavior/Features/ContentRepository/WorkspaceService.feature +++ b/Neos.Neos/Tests/Behavior/Features/ContentRepository/WorkspaceService.feature @@ -102,13 +102,30 @@ Feature: Neos WorkspaceService related features | Title | Description | Classification | Owner user id | | some-shared-workspace | | SHARED | | - Scenario: Get metadata of non-existing sub workspace + Scenario: Get metadata of a sub workspace which is directly created via the content repository Given the root workspace "some-root-workspace" is created - When a workspace "some-workspace" with base workspace "some-root-workspace" exists without metadata + # dont use the workspace service here: + When the command CreateWorkspace is executed with payload: + | Key | Value | + | workspaceName | "some-workspace" | + | baseWorkspaceName | "some-root-workspace" | + | newContentStreamId | "any-cs" | + Then the workspace "some-workspace" should have the following metadata: | Title | Description | Classification | Owner user id | | some-workspace | | UNKNOWN | | + Scenario: Get metadata or roles if the workspace does not exist + Then the metadata for workspace "non-existing-workspace" does not exist + Then the roles for workspace "non-existing-workspace" does not exist + + When the root workspace "some-root-workspace" with title "Some root workspace" and description "Some description" is created + And the role COLLABORATOR is assigned to workspace "some-root-workspace" for group "Neos.Neos:AbstractEditor" + Given the workspace "some-root-workspace" is deleted + + Then the metadata for workspace "some-root-workspace" does not exist + Then the roles for workspace "some-root-workspace" does not exist + Scenario: Assign role to non-existing workspace When the role COLLABORATOR is assigned to workspace "some-workspace" for group "Neos.Neos:AbstractEditor" Then an exception of type "RuntimeException" should be thrown with message: From 06348d52a100322f9128e84417d1ee00242cdc62 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Mon, 9 Dec 2024 11:01:21 +0100 Subject: [PATCH 13/24] TASK: Throw specific `WorkspaceDoesNotExist` in workspace service to simplify handling --- .../Exception/WorkspaceDoesNotExist.php | 16 +++++++++++++--- .../Classes/Domain/Service/WorkspaceService.php | 6 +++++- .../Features/Bootstrap/WorkspaceServiceTrait.php | 7 +++---- .../ContentRepository/WorkspaceService.feature | 8 ++++---- 4 files changed, 25 insertions(+), 12 deletions(-) diff --git a/Neos.ContentRepository.Core/Classes/SharedModel/Exception/WorkspaceDoesNotExist.php b/Neos.ContentRepository.Core/Classes/SharedModel/Exception/WorkspaceDoesNotExist.php index 3def569da87..67939a4bea7 100644 --- a/Neos.ContentRepository.Core/Classes/SharedModel/Exception/WorkspaceDoesNotExist.php +++ b/Neos.ContentRepository.Core/Classes/SharedModel/Exception/WorkspaceDoesNotExist.php @@ -14,18 +14,28 @@ namespace Neos\ContentRepository\Core\SharedModel\Exception; +use Neos\ContentRepository\Core\SharedModel\ContentRepository\ContentRepositoryId; use Neos\ContentRepository\Core\SharedModel\Workspace\WorkspaceName; /** - * @api because exception is thrown during invariant checks on command execution + * @api because exception is thrown during invariant checks on command execution or when attempting to query a non-existing workspace */ -final class WorkspaceDoesNotExist extends \DomainException +final class WorkspaceDoesNotExist extends \RuntimeException { public static function butWasSupposedTo(WorkspaceName $name): self { return new self(sprintf( - 'The source workspace %s does not exist', + 'The workspace "%s" does not exist', $name->value ), 1513924741); } + + public static function butWasSupposedToInContentRepository(WorkspaceName $name, ContentRepositoryId $contentRepositoryId): self + { + return new self(sprintf( + 'The workspace "%s" does not exist in content repository "%s"', + $name->value, + $contentRepositoryId->value + ), 1733737361); + } } diff --git a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php index 5a652be3354..a15c2b583cc 100644 --- a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php +++ b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php @@ -20,6 +20,7 @@ use Neos\ContentRepository\Core\Feature\WorkspaceCreation\Exception\WorkspaceAlreadyExists; use Neos\ContentRepository\Core\Feature\WorkspaceModification\Command\DeleteWorkspace; use Neos\ContentRepository\Core\SharedModel\ContentRepository\ContentRepositoryId; +use Neos\ContentRepository\Core\SharedModel\Exception\WorkspaceDoesNotExist; use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamId; use Neos\ContentRepository\Core\SharedModel\Workspace\Workspace; use Neos\ContentRepository\Core\SharedModel\Workspace\WorkspaceName; @@ -278,13 +279,16 @@ private function createWorkspace(ContentRepositoryId $contentRepositoryId, Works $this->metadataAndRoleRepository->addWorkspaceMetadata($contentRepositoryId, $workspaceName, $title, $description, $classification, $ownerId); } + /** + * @throws WorkspaceDoesNotExist if the workspace does not exist + */ private function requireWorkspace(ContentRepositoryId $contentRepositoryId, WorkspaceName $workspaceName): Workspace { $workspace = $this->contentRepositoryRegistry ->get($contentRepositoryId) ->findWorkspaceByName($workspaceName); if ($workspace === null) { - throw new \RuntimeException(sprintf('Failed to find workspace with name "%s" for content repository "%s"', $workspaceName->value, $contentRepositoryId->value), 1718379722); + throw WorkspaceDoesNotExist::butWasSupposedToInContentRepository($workspaceName, $contentRepositoryId); } return $workspace; } diff --git a/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php b/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php index b71ee82bb3b..deee0129a52 100644 --- a/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php +++ b/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php @@ -15,10 +15,9 @@ use Behat\Gherkin\Node\TableNode; use Neos\ContentRepository\BehavioralTests\TestSuite\Behavior\CRBehavioralTestsSubjectProvider; use Neos\ContentRepository\Core\Feature\WorkspaceCreation\Command\CreateRootWorkspace; -use Neos\ContentRepository\Core\Feature\WorkspaceCreation\Command\CreateWorkspace; +use Neos\ContentRepository\Core\SharedModel\Exception\WorkspaceDoesNotExist; use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamId; use Neos\ContentRepository\Core\SharedModel\Workspace\WorkspaceName; -use Neos\Flow\Security\Context as SecurityContext; use Neos\Neos\Domain\Model\UserId; use Neos\Neos\Domain\Model\WorkspaceDescription; use Neos\Neos\Domain\Model\WorkspaceRole; @@ -186,7 +185,7 @@ public function theWorkspaceMetadataFails($workspaceName): void try { $this->getObject(WorkspaceService::class)->getWorkspaceMetadata($this->currentContentRepository->id, WorkspaceName::fromString($workspaceName)); } catch (\Throwable $e) { - Assert::assertInstanceOf(\RuntimeException::class, $e, $e->getMessage()); + Assert::assertInstanceOf(WorkspaceDoesNotExist::class, $e, $e->getMessage()); return; } Assert::fail('Did not throw'); @@ -204,7 +203,7 @@ public function theWorkspaceRolesFails($workspaceName): void try { $this->getObject(WorkspaceService::class)->getWorkspaceRoleAssignments($this->currentContentRepository->id, WorkspaceName::fromString($workspaceName)); } catch (\Throwable $e) { - Assert::assertInstanceOf(\RuntimeException::class, $e, $e->getMessage()); + Assert::assertInstanceOf(WorkspaceDoesNotExist::class, $e, $e->getMessage()); return; } Assert::fail('Did not throw'); diff --git a/Neos.Neos/Tests/Behavior/Features/ContentRepository/WorkspaceService.feature b/Neos.Neos/Tests/Behavior/Features/ContentRepository/WorkspaceService.feature index 453f393a0e1..936bd55036c 100644 --- a/Neos.Neos/Tests/Behavior/Features/ContentRepository/WorkspaceService.feature +++ b/Neos.Neos/Tests/Behavior/Features/ContentRepository/WorkspaceService.feature @@ -128,9 +128,9 @@ Feature: Neos WorkspaceService related features Scenario: Assign role to non-existing workspace When the role COLLABORATOR is assigned to workspace "some-workspace" for group "Neos.Neos:AbstractEditor" - Then an exception of type "RuntimeException" should be thrown with message: + Then an exception of type "WorkspaceDoesNotExist" should be thrown with message: """ - Failed to find workspace with name "some-workspace" for content repository "default" + The workspace "some-workspace" does not exist in content repository "default" """ Scenario: Assign group role to root workspace @@ -167,9 +167,9 @@ Feature: Neos WorkspaceService related features Scenario: Unassign role from non-existing workspace When the role for group "Neos.Neos:AbstractEditor" is unassigned from workspace "some-workspace" - Then an exception of type "RuntimeException" should be thrown with message: + Then an exception of type "WorkspaceDoesNotExist" should be thrown with message: """ - Failed to find workspace with name "some-workspace" for content repository "default" + The workspace "some-workspace" does not exist in content repository "default" """ Scenario: Unassign role from workspace that has not been assigned before From 6d0a1498664809cf08fa26be31f6b7c0087094fb Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Mon, 9 Dec 2024 11:36:46 +0100 Subject: [PATCH 14/24] BUGFIX: Allow to specify `WorkspaceRoleAssignments` when creating a new shared workspace ... otherwise authentication fails and the workspace roles cannot be assigned because management is not granted (yet) - only admins would be able to do that. --- .../Command/WorkspaceCommandController.php | 2 ++ .../Domain/Model/WorkspaceRoleAssignments.php | 10 +++++++ .../Domain/Service/WorkspaceService.php | 7 ++++- .../ContentRepositoryAuthorizationService.php | 4 +++ .../Bootstrap/WorkspaceServiceTrait.php | 17 ++++++++++- .../Security/WorkspacePermissions.feature | 9 +++--- .../Controller/WorkspaceController.php | 29 ++++++++----------- 7 files changed, 55 insertions(+), 23 deletions(-) diff --git a/Neos.Neos/Classes/Command/WorkspaceCommandController.php b/Neos.Neos/Classes/Command/WorkspaceCommandController.php index fa834b4258d..4dd8a0acd82 100644 --- a/Neos.Neos/Classes/Command/WorkspaceCommandController.php +++ b/Neos.Neos/Classes/Command/WorkspaceCommandController.php @@ -30,6 +30,7 @@ use Neos\Neos\Domain\Model\WorkspaceDescription; use Neos\Neos\Domain\Model\WorkspaceRole; use Neos\Neos\Domain\Model\WorkspaceRoleAssignment; +use Neos\Neos\Domain\Model\WorkspaceRoleAssignments; use Neos\Neos\Domain\Model\WorkspaceRoleSubject; use Neos\Neos\Domain\Model\WorkspaceRoleSubjectType; use Neos\Neos\Domain\Model\WorkspaceTitle; @@ -205,6 +206,7 @@ public function createSharedCommand(string $workspace, string $baseWorkspace = ' WorkspaceTitle::fromString($title ?? $workspaceName->value), WorkspaceDescription::fromString($description ?? ''), WorkspaceName::fromString($baseWorkspace), + WorkspaceRoleAssignments::createEmpty() ); $this->outputLine('Created shared workspace "%s"', [$workspaceName->value]); } diff --git a/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignments.php b/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignments.php index ef9af39059a..28d8aec2126 100644 --- a/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignments.php +++ b/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignments.php @@ -25,6 +25,16 @@ private function __construct(WorkspaceRoleAssignment ...$assignments) $this->assignments = $assignments; } + public static function createEmpty(): self + { + return new self(); + } + + public static function create(WorkspaceRoleAssignment ...$assignments): self + { + return new self(...$assignments); + } + /** * @param array $assignments */ diff --git a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php index a15c2b583cc..7dfebc3c93d 100644 --- a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php +++ b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php @@ -154,10 +154,15 @@ public function createPersonalWorkspace(ContentRepositoryId $contentRepositoryId /** * Create a new, potentially shared, workspace + * + * NOTE: By default - if no role assignments are specified - only administrators can manage workspaces without role assignments. */ - public function createSharedWorkspace(ContentRepositoryId $contentRepositoryId, WorkspaceName $workspaceName, WorkspaceTitle $title, WorkspaceDescription $description, WorkspaceName $baseWorkspaceName): void + public function createSharedWorkspace(ContentRepositoryId $contentRepositoryId, WorkspaceName $workspaceName, WorkspaceTitle $title, WorkspaceDescription $description, WorkspaceName $baseWorkspaceName, WorkspaceRoleAssignments $workspaceRoleAssignments): void { $this->createWorkspace($contentRepositoryId, $workspaceName, $title, $description, $baseWorkspaceName, null, WorkspaceClassification::SHARED); + foreach ($workspaceRoleAssignments as $assignment) { + $this->metadataAndRoleRepository->assignWorkspaceRole($contentRepositoryId, $workspaceName, $assignment); + } } /** diff --git a/Neos.Neos/Classes/Security/Authorization/ContentRepositoryAuthorizationService.php b/Neos.Neos/Classes/Security/Authorization/ContentRepositoryAuthorizationService.php index 6de599cbd7c..b187e63f7a6 100644 --- a/Neos.Neos/Classes/Security/Authorization/ContentRepositoryAuthorizationService.php +++ b/Neos.Neos/Classes/Security/Authorization/ContentRepositoryAuthorizationService.php @@ -59,6 +59,10 @@ public function getWorkspacePermissions(ContentRepositoryId $contentRepositoryId if ($userId !== null) { $subjects[] = WorkspaceRoleSubject::createForUser($userId); } + /** + * We hardcode the check against administrators to always grant manage permissions. This is done to allow administrators to fix permissions of all workspaces. + * We don't allow all rights like read and write. Admins should be able to grant themselves permissions to write to other personal workspaces, but they should not have this permission automagically. + */ $userIsAdministrator = in_array(self::ROLE_NEOS_ADMINISTRATOR, $roleIdentifiers, true); $userWorkspaceRole = $this->metadataAndRoleRepository->getMostPrivilegedWorkspaceRoleForSubjects($contentRepositoryId, $workspaceName, WorkspaceRoleSubjects::fromArray($subjects)); if ($userWorkspaceRole === null) { diff --git a/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php b/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php index deee0129a52..1910727fe20 100644 --- a/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php +++ b/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php @@ -22,7 +22,9 @@ use Neos\Neos\Domain\Model\WorkspaceDescription; use Neos\Neos\Domain\Model\WorkspaceRole; use Neos\Neos\Domain\Model\WorkspaceRoleAssignment; +use Neos\Neos\Domain\Model\WorkspaceRoleAssignments; use Neos\Neos\Domain\Model\WorkspaceRoleSubject; +use Neos\Neos\Domain\Model\WorkspaceRoleSubjectType; use Neos\Neos\Domain\Model\WorkspaceTitle; use Neos\Neos\Domain\Service\UserService; use Neos\Neos\Domain\Service\WorkspaceService; @@ -112,15 +114,28 @@ public function aPersonalWorkspaceForUserIsCreated(string $username): void /** * @When the shared workspace :workspaceName is created with the target workspace :targetWorkspace + * @When the shared workspace :workspaceName is created with the target workspace :targetWorkspace and role assignments: */ - public function theSharedWorkspaceIsCreatedWithTheTargetWorkspace(string $workspaceName, string $targetWorkspace): void + public function theSharedWorkspaceIsCreatedWithTheTargetWorkspace(string $workspaceName, string $targetWorkspace, ?TableNode $rawRoleAssignments = null): void { + $workspaceRoleAssignments = []; + foreach ($rawRoleAssignments?->getHash() ?? [] as $row) { + $workspaceRoleAssignments[] = WorkspaceRoleAssignment::create( + WorkspaceRoleSubject::create( + WorkspaceRoleSubjectType::from($row['Type']), + $row['Value'] + ), + WorkspaceRole::from($row['Role']) + ); + } + $this->tryCatchingExceptions(fn () => $this->getObject(WorkspaceService::class)->createSharedWorkspace( $this->currentContentRepository->id, WorkspaceName::fromString($workspaceName), WorkspaceTitle::fromString($workspaceName), WorkspaceDescription::fromString(''), WorkspaceName::fromString($targetWorkspace), + WorkspaceRoleAssignments::fromArray($workspaceRoleAssignments) )); } diff --git a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature index 800d45c8bcd..3fc1101b7d6 100644 --- a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature +++ b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature @@ -51,15 +51,16 @@ Feature: Workspace permission related features # neos user with out any editing roles | simple_user | Neos.Neos:UserManager | - And the shared workspace "shared-workspace" is created with the target workspace "live" - And the role COLLABORATOR is assigned to workspace "shared-workspace" for group "Neos.Neos:AbstractEditor" + When content repository security is enabled + And the shared workspace "shared-workspace" is created with the target workspace "live" and role assignments: + | Role | Type | Value | + | COLLABORATOR | GROUP | Neos.Neos:AbstractEditor | + Given I am authenticated as owner And the personal workspace "workspace" is created with the target workspace "live" for user "owner" And the role MANAGER is assigned to workspace "workspace" for user "manager" And the role COLLABORATOR is assigned to workspace "workspace" for user "collaborator" - When content repository security is enabled - Scenario Outline: Creating a root workspace Given I am authenticated as When the command CreateRootWorkspace is executed with payload '{"workspaceName":"new-ws","newContentStreamId":"new-cs"}' and exceptions are caught diff --git a/Neos.Workspace.Ui/Classes/Controller/WorkspaceController.php b/Neos.Workspace.Ui/Classes/Controller/WorkspaceController.php index e8f02988acb..99971e52e13 100644 --- a/Neos.Workspace.Ui/Classes/Controller/WorkspaceController.php +++ b/Neos.Workspace.Ui/Classes/Controller/WorkspaceController.php @@ -55,6 +55,7 @@ use Neos\Neos\Domain\Model\WorkspaceDescription; use Neos\Neos\Domain\Model\WorkspaceRole; use Neos\Neos\Domain\Model\WorkspaceRoleAssignment; +use Neos\Neos\Domain\Model\WorkspaceRoleAssignments; use Neos\Neos\Domain\Model\WorkspaceTitle; use Neos\Neos\Domain\Repository\SiteRepository; use Neos\Neos\Domain\Service\NodeTypeNameFactory; @@ -222,6 +223,16 @@ public function createAction( $title, $description, $baseWorkspace, + WorkspaceRoleAssignments::create( + WorkspaceRoleAssignment::createForUser( + $currentUser->getId(), + WorkspaceRole::MANAGER, + ), + WorkspaceRoleAssignment::createForGroup( + 'Neos.Neos:AbstractEditor', + WorkspaceRole::COLLABORATOR, + ) + ) ); } catch (WorkspaceAlreadyExists $exception) { $this->addFlashMessage( @@ -231,22 +242,6 @@ public function createAction( ); $this->redirect('new'); } - $this->workspaceService->assignWorkspaceRole( - $contentRepositoryId, - $workspaceName, - WorkspaceRoleAssignment::createForUser( - $currentUser->getId(), - WorkspaceRole::MANAGER, - ) - ); - $this->workspaceService->assignWorkspaceRole( - $contentRepositoryId, - $workspaceName, - WorkspaceRoleAssignment::createForGroup( - 'Neos.Neos:AbstractEditor', - WorkspaceRole::COLLABORATOR, - ) - ); $this->addFlashMessage($this->getModuleLabel('workspaces.workspaceHasBeenCreated', [$title->value])); $this->redirect('index'); } @@ -1029,7 +1024,7 @@ protected function prepareBaseWorkspaceOptions( continue; } $permissions = $this->contentRepositoryAuthorizationService->getWorkspacePermissions($contentRepository->id, $workspace->workspaceName, $this->securityContext->getRoles(), $currentUser?->getId()); - if (!$permissions->manage) { + if (!$permissions->read) { continue; } $baseWorkspaceOptions[$workspace->workspaceName->value] = $workspaceMetadata->title->value; From 3c678dd45ad3c21bafed8b424ae2f42e854d2a49 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Mon, 9 Dec 2024 11:50:53 +0100 Subject: [PATCH 15/24] TASK: Document how Neos.Neos:LivePublisher and Neos.Neos:AbstractEditor are enforced via the cr core --- Neos.Neos/Configuration/Policy.yaml | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/Neos.Neos/Configuration/Policy.yaml b/Neos.Neos/Configuration/Policy.yaml index c441d522255..c5f3beb3ca3 100644 --- a/Neos.Neos/Configuration/Policy.yaml +++ b/Neos.Neos/Configuration/Policy.yaml @@ -56,10 +56,6 @@ privilegeTargets: label: General access to content editing matcher: 'method(Neos\Neos\Service\Controller\NodeController->(show|getPrimaryChildNode|getChildNodesForTree|filterChildNodesForTree|getChildNodes|getChildNodesFromParent|create|createAndRender|createNodeForTheTree|move|moveBefore|moveAfter|moveInto|moveAndRender|copy|copyBefore|copyAfter|copyInto|copyAndRender|update|updateAndRender|delete|searchPage|error)Action()) || method(Neos\Neos\Controller\Backend\ContentController->(uploadAsset|assetsWithMetadata|imageWithMetadata|createImageVariant|error)Action()) || method(Neos\Neos\Controller\Service\AssetProxiesController->(index|show|import|error)Action()) || method(Neos\Neos\Controller\Service\AssetsController->(index|show|error)Action()) || method(Neos\Neos\Controller\Service\NodesController->(index|show|create|error)Action())' - 'Neos.Neos:Backend.PublishToLiveWorkspace': - label: Allowed to publish to the live workspace - matcher: 'method(Neos\ContentRepository\Domain\Model\Workspace->(publish|publishNode|publishNodes)(targetWorkspace.name === "live"))' - 'Neos.Neos:Backend.PublishOwnWorkspaceContent': label: Allowed to publish own personal workspace matcher: 'method(Neos\Neos\Service\Controller\WorkspaceController->(publishNode|publishNodes|error)Action()) || method(Neos\Neos\Service\Controller\WorkspaceController->publishAllAction(workspaceName = current.userInformation.personalWorkspaceName)) || method(Neos\Neos\Service\Controller\WorkspaceController->getWorkspaceWideUnpublishedNodesAction(workspace.name = current.userInformation.personalWorkspaceName))' @@ -170,14 +166,12 @@ roles: permission: GRANT 'Neos.Neos:LivePublisher': + # This group is assigned conventionally as collaborator for the live workspace. See WorkspaceService::assignWorkspaceRole label: Live publisher description: The role allows to publish to the live workspace - privileges: - - - privilegeTarget: 'Neos.Neos:Backend.PublishToLiveWorkspace' - permission: GRANT 'Neos.Neos:AbstractEditor': + # This group is assigned conventionally for new shared workspaces as collaborator. See WorkspaceService::assignWorkspaceRole abstract: true parentRoles: ['Neos.ContentRepository:Administrator'] privileges: From 1e5dae66f716a09da4d77a098481d81b61ce0e47 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Mon, 9 Dec 2024 12:13:53 +0100 Subject: [PATCH 16/24] BUGFIX: Fix root workspace creation to specify assignments and replace `createLiveWorkspaceIfMissing` The `WorkspaceRoleAssignments` have two static factories for sane defaults for the live workspace and share one so that information is not spread out too much over the packages --- .../Command/WorkspaceCommandController.php | 3 +- .../Import/LiveWorkspaceCreationProcessor.php | 11 ++++- .../Domain/Model/WorkspaceRoleAssignments.php | 44 +++++++++++++++++++ .../Domain/Service/SiteServiceInternals.php | 14 +++++- .../Domain/Service/WorkspaceService.php | 20 ++------- .../Bootstrap/WorkspaceServiceTrait.php | 17 ++++--- .../Controller/WorkspaceController.php | 11 +---- 7 files changed, 85 insertions(+), 35 deletions(-) diff --git a/Neos.Neos/Classes/Command/WorkspaceCommandController.php b/Neos.Neos/Classes/Command/WorkspaceCommandController.php index 4dd8a0acd82..ac290f02685 100644 --- a/Neos.Neos/Classes/Command/WorkspaceCommandController.php +++ b/Neos.Neos/Classes/Command/WorkspaceCommandController.php @@ -148,7 +148,8 @@ public function createRootCommand(string $name, string $contentRepository = 'def $contentRepositoryId, $workspaceName, WorkspaceTitle::fromString($title ?? $name), - WorkspaceDescription::fromString($description ?? '') + WorkspaceDescription::fromString($description ?? ''), + WorkspaceRoleAssignments::createEmpty() ); $this->outputLine('Created root workspace "%s" in content repository "%s"', [$workspaceName->value, $contentRepositoryId->value]); } diff --git a/Neos.Neos/Classes/Domain/Import/LiveWorkspaceCreationProcessor.php b/Neos.Neos/Classes/Domain/Import/LiveWorkspaceCreationProcessor.php index 58ba49be52e..3e0b582d438 100644 --- a/Neos.Neos/Classes/Domain/Import/LiveWorkspaceCreationProcessor.php +++ b/Neos.Neos/Classes/Domain/Import/LiveWorkspaceCreationProcessor.php @@ -19,6 +19,9 @@ use Neos\ContentRepository\Export\ProcessingContext; use Neos\ContentRepository\Export\ProcessorInterface; use Neos\ContentRepository\Export\Severity; +use Neos\Neos\Domain\Model\WorkspaceDescription; +use Neos\Neos\Domain\Model\WorkspaceRoleAssignments; +use Neos\Neos\Domain\Model\WorkspaceTitle; use Neos\Neos\Domain\Service\WorkspaceService; /** @@ -40,6 +43,12 @@ public function run(ProcessingContext $context): void $context->dispatch(Severity::NOTICE, 'Workspace already exists, skipping'); return; } - $this->workspaceService->createLiveWorkspaceIfMissing($this->contentRepository->id); + $this->workspaceService->createRootWorkspace( + $this->contentRepository->id, + WorkspaceName::forLive(), + WorkspaceTitle::fromString('Public live workspace'), + WorkspaceDescription::empty(), + WorkspaceRoleAssignments::createForLiveWorkspace() + ); } } diff --git a/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignments.php b/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignments.php index 28d8aec2126..f381d3c80da 100644 --- a/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignments.php +++ b/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignments.php @@ -5,6 +5,7 @@ namespace Neos\Neos\Domain\Model; use Neos\Flow\Annotations as Flow; +use Neos\Neos\Domain\Service\WorkspaceService; /** * A set of {@see WorkspaceRoleAssignment} instances @@ -43,6 +44,44 @@ public static function fromArray(array $assignments): self return new self(...$assignments); } + /** + * Default role assignment to be specified at creation via {@see WorkspaceService::createRootWorkspace()} + * + * Users with the role "Neos.Neos:LivePublisher" are collaborators and everybody can read. + */ + public static function createForLiveWorkspace(): self + { + return new self( + WorkspaceRoleAssignment::createForGroup( + 'Neos.Neos:LivePublisher', + WorkspaceRole::COLLABORATOR + ), + WorkspaceRoleAssignment::createForGroup( + 'Neos.Flow:Everybody', + WorkspaceRole::VIEWER + ) + ); + } + + /** + * Default role assignment to be specified at creation via {@see WorkspaceService::createSharedWorkspace()} + * + * Users with the role "Neos.Neos:AbstractEditor" are collaborators and the specified user is manager + */ + public static function createForSharedWorkspace(UserId $userId): self + { + return new self( + WorkspaceRoleAssignment::createForUser( + $userId, + WorkspaceRole::MANAGER, + ), + WorkspaceRoleAssignment::createForGroup( + 'Neos.Neos:AbstractEditor', + WorkspaceRole::COLLABORATOR, + ) + ); + } + public function isEmpty(): bool { return $this->assignments === []; @@ -67,4 +106,9 @@ public function contains(WorkspaceRoleAssignment $assignment): bool } return false; } + + public function add(WorkspaceRoleAssignment $assignment): self + { + return new self(...[...$this->assignments, $assignment]); + } } diff --git a/Neos.Neos/Classes/Domain/Service/SiteServiceInternals.php b/Neos.Neos/Classes/Domain/Service/SiteServiceInternals.php index 5842e6366db..09d64fefd46 100644 --- a/Neos.Neos/Classes/Domain/Service/SiteServiceInternals.php +++ b/Neos.Neos/Classes/Domain/Service/SiteServiceInternals.php @@ -33,6 +33,9 @@ use Neos\Neos\Domain\Exception\SiteNodeTypeIsInvalid; use Neos\Neos\Domain\Model\Site; use Neos\Neos\Domain\Model\SiteNodeName; +use Neos\Neos\Domain\Model\WorkspaceDescription; +use Neos\Neos\Domain\Model\WorkspaceRoleAssignments; +use Neos\Neos\Domain\Model\WorkspaceTitle; /** * @internal FIXME refactor and incorporate into SiteService @@ -89,7 +92,16 @@ public function removeSiteNode(SiteNodeName $siteNodeName): void public function createSiteNodeIfNotExists(Site $site, string $nodeTypeName): void { - $this->workspaceService->createLiveWorkspaceIfMissing($this->contentRepository->id); + $liveWorkspace = $this->contentRepository->findWorkspaceByName(WorkspaceName::forLive()); + if ($liveWorkspace === null) { + $this->workspaceService->createRootWorkspace( + $this->contentRepository->id, + WorkspaceName::forLive(), + WorkspaceTitle::fromString('Public live workspace'), + WorkspaceDescription::empty(), + WorkspaceRoleAssignments::createForLiveWorkspace() + ); + } $sitesNodeIdentifier = $this->getOrCreateRootNodeAggregate(); $siteNodeType = $this->nodeTypeManager->getNodeType($nodeTypeName); diff --git a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php index 7dfebc3c93d..271f3be27c6 100644 --- a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php +++ b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php @@ -115,7 +115,7 @@ public function getPersonalWorkspaceForUser(ContentRepositoryId $contentReposito * * @throws WorkspaceAlreadyExists */ - public function createRootWorkspace(ContentRepositoryId $contentRepositoryId, WorkspaceName $workspaceName, WorkspaceTitle $title, WorkspaceDescription $description): void + public function createRootWorkspace(ContentRepositoryId $contentRepositoryId, WorkspaceName $workspaceName, WorkspaceTitle $title, WorkspaceDescription $description, WorkspaceRoleAssignments $workspaceRoleAssignments): void { $contentRepository = $this->contentRepositoryRegistry->get($contentRepositoryId); $contentRepository->handle( @@ -125,23 +125,9 @@ public function createRootWorkspace(ContentRepositoryId $contentRepositoryId, Wo ) ); $this->metadataAndRoleRepository->addWorkspaceMetadata($contentRepositoryId, $workspaceName, $title, $description, WorkspaceClassification::ROOT, null); - } - - /** - * Create the "live" root workspace with the default role assignment (users with the role "Neos.Neos:LivePublisher" are collaborators) - */ - public function createLiveWorkspaceIfMissing(ContentRepositoryId $contentRepositoryId): void - { - $contentRepository = $this->contentRepositoryRegistry->get($contentRepositoryId); - $workspaceName = WorkspaceName::forLive(); - $liveWorkspace = $contentRepository->findWorkspaceByName($workspaceName); - if ($liveWorkspace !== null) { - // live workspace already exists - return; + foreach ($workspaceRoleAssignments as $assignment) { + $this->metadataAndRoleRepository->assignWorkspaceRole($contentRepositoryId, $workspaceName, $assignment); } - $this->createRootWorkspace($contentRepositoryId, $workspaceName, WorkspaceTitle::fromString('Public live workspace'), WorkspaceDescription::empty()); - $this->metadataAndRoleRepository->assignWorkspaceRole($contentRepositoryId, $workspaceName, WorkspaceRoleAssignment::createForGroup('Neos.Neos:LivePublisher', WorkspaceRole::COLLABORATOR)); - $this->metadataAndRoleRepository->assignWorkspaceRole($contentRepositoryId, $workspaceName, WorkspaceRoleAssignment::createForGroup('Neos.Flow:Everybody', WorkspaceRole::VIEWER)); } /** diff --git a/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php b/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php index 1910727fe20..f76f3f04d49 100644 --- a/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php +++ b/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php @@ -60,6 +60,7 @@ public function theRootWorkspaceIsCreated(string $workspaceName, string $title = WorkspaceName::fromString($workspaceName), WorkspaceTitle::fromString($title ?? $workspaceName), WorkspaceDescription::fromString($description ?? ''), + WorkspaceRoleAssignments::createEmpty() )); } @@ -79,8 +80,12 @@ public function theWorkspaceIsDeleted(string $workspaceName): void */ public function theLiveWorkspaceExists(): void { - $this->getObject(WorkspaceService::class)->createLiveWorkspaceIfMissing( - $this->currentContentRepository->id + $this->getObject(WorkspaceService::class)->createRootWorkspace( + $this->currentContentRepository->id, + WorkspaceName::forLive(), + WorkspaceTitle::fromString('Public live workspace'), + WorkspaceDescription::empty(), + WorkspaceRoleAssignments::createForLiveWorkspace() ); } @@ -118,15 +123,15 @@ public function aPersonalWorkspaceForUserIsCreated(string $username): void */ public function theSharedWorkspaceIsCreatedWithTheTargetWorkspace(string $workspaceName, string $targetWorkspace, ?TableNode $rawRoleAssignments = null): void { - $workspaceRoleAssignments = []; + $workspaceRoleAssignments = WorkspaceRoleAssignments::createEmpty(); foreach ($rawRoleAssignments?->getHash() ?? [] as $row) { - $workspaceRoleAssignments[] = WorkspaceRoleAssignment::create( + $workspaceRoleAssignments = $workspaceRoleAssignments->add(WorkspaceRoleAssignment::create( WorkspaceRoleSubject::create( WorkspaceRoleSubjectType::from($row['Type']), $row['Value'] ), WorkspaceRole::from($row['Role']) - ); + )); } $this->tryCatchingExceptions(fn () => $this->getObject(WorkspaceService::class)->createSharedWorkspace( @@ -135,7 +140,7 @@ public function theSharedWorkspaceIsCreatedWithTheTargetWorkspace(string $worksp WorkspaceTitle::fromString($workspaceName), WorkspaceDescription::fromString(''), WorkspaceName::fromString($targetWorkspace), - WorkspaceRoleAssignments::fromArray($workspaceRoleAssignments) + $workspaceRoleAssignments )); } diff --git a/Neos.Workspace.Ui/Classes/Controller/WorkspaceController.php b/Neos.Workspace.Ui/Classes/Controller/WorkspaceController.php index 99971e52e13..fbcbd7ebb2f 100644 --- a/Neos.Workspace.Ui/Classes/Controller/WorkspaceController.php +++ b/Neos.Workspace.Ui/Classes/Controller/WorkspaceController.php @@ -223,15 +223,8 @@ public function createAction( $title, $description, $baseWorkspace, - WorkspaceRoleAssignments::create( - WorkspaceRoleAssignment::createForUser( - $currentUser->getId(), - WorkspaceRole::MANAGER, - ), - WorkspaceRoleAssignment::createForGroup( - 'Neos.Neos:AbstractEditor', - WorkspaceRole::COLLABORATOR, - ) + WorkspaceRoleAssignments::createForSharedWorkspace( + $currentUser->getId() ) ); } catch (WorkspaceAlreadyExists $exception) { From a36e5648fcc59f192f0bb427a3dac5984f778a77 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Mon, 9 Dec 2024 14:54:39 +0100 Subject: [PATCH 17/24] TASK: Test current state of ordering if multiple workspaces exist for a user the order by was just introduced to make this case explicit, the new test pass without. --- .../Repository/WorkspaceMetadataAndRoleRepository.php | 3 +++ Neos.Neos/Classes/Domain/Service/WorkspaceService.php | 5 ++--- .../Features/Bootstrap/WorkspaceServiceTrait.php | 11 +++++++++++ .../ContentRepository/WorkspaceService.feature | 11 ++++++++++- 4 files changed, 26 insertions(+), 4 deletions(-) diff --git a/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php b/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php index dd2eb788c72..d11285d6414 100644 --- a/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php +++ b/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php @@ -352,6 +352,9 @@ public function findPrimaryWorkspaceNameForUser(ContentRepositoryId $contentRepo content_repository_id = :contentRepositoryId AND classification = :personalWorkspaceClassification AND owner_user_id = :userId + -- TODO introduce better deterministic selection of "primary" workspace if multiple exist + ORDER BY workspace_name + LIMIT 1 SQL; $workspaceName = $this->dbal->fetchOne($query, [ 'contentRepositoryId' => $contentRepositoryId->value, diff --git a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php index 271f3be27c6..11d692c621c 100644 --- a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php +++ b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php @@ -97,9 +97,9 @@ public function setWorkspaceDescription(ContentRepositoryId $contentRepositoryId } /** - * Retrieve the currently active personal workspace for the specified $userId + * Retrieve the first personal workspace for the specified user * - * NOTE: Currently there can only ever be a single personal workspace per user. But this API already prepares support for multiple personal workspaces per user + * NOTE: Conventionally each user has only a single personal workspace but in case multiple exist the first is returned */ public function getPersonalWorkspaceForUser(ContentRepositoryId $contentRepositoryId, UserId $userId): Workspace { @@ -153,7 +153,6 @@ public function createSharedWorkspace(ContentRepositoryId $contentRepositoryId, /** * Create a new, personal, workspace for the specified user if none exists yet - * @internal experimental api, until actually used by the Neos.Ui */ public function createPersonalWorkspaceForUserIfMissing(ContentRepositoryId $contentRepositoryId, User $user): void { diff --git a/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php b/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php index f76f3f04d49..0b969427e98 100644 --- a/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php +++ b/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php @@ -117,6 +117,17 @@ public function aPersonalWorkspaceForUserIsCreated(string $username): void )); } + /** + * @Then the personal workspace for user :username is :workspaceName + */ + public function thePersonalWorkspaceForUserIs(string $workspaceName, string $username): void + { + $ownerUserId = $this->userIdForUsername($username); + $actualWorkspace = $this->getObject(WorkspaceService::class)->getPersonalWorkspaceForUser($this->currentContentRepository->id, $ownerUserId); + Assert::assertNotNull($actualWorkspace); + Assert::assertSame($workspaceName, $actualWorkspace->workspaceName->value); + } + /** * @When the shared workspace :workspaceName is created with the target workspace :targetWorkspace * @When the shared workspace :workspaceName is created with the target workspace :targetWorkspace and role assignments: diff --git a/Neos.Neos/Tests/Behavior/Features/ContentRepository/WorkspaceService.feature b/Neos.Neos/Tests/Behavior/Features/ContentRepository/WorkspaceService.feature index 936bd55036c..c147d7b0007 100644 --- a/Neos.Neos/Tests/Behavior/Features/ContentRepository/WorkspaceService.feature +++ b/Neos.Neos/Tests/Behavior/Features/ContentRepository/WorkspaceService.feature @@ -87,14 +87,22 @@ Feature: Neos WorkspaceService related features | Title | Description | Classification | Owner user id | | some-root-workspace | Some new workspace description | ROOT | | - Scenario: Create a single personal workspace When the root workspace "some-root-workspace" is created And the personal workspace "some-user-workspace" is created with the target workspace "some-root-workspace" for user "jane.doe" + Then the personal workspace for user "jane.doe" is "some-user-workspace" Then the workspace "some-user-workspace" should have the following metadata: | Title | Description | Classification | Owner user id | | some-user-workspace | | PERSONAL | janedoe | + + Scenario: For multiple personal workspaces only one workspace is returned + When the root workspace "some-root-workspace" is created + And the personal workspace "b-user-workspace" is created with the target workspace "some-root-workspace" for user "jane.doe" + And the personal workspace "a-user-workspace" is created with the target workspace "some-root-workspace" for user "jane.doe" + And the personal workspace "c-user-workspace" is created with the target workspace "some-root-workspace" for user "jane.doe" + Then the personal workspace for user "jane.doe" is "a-user-workspace" + Scenario: Create a single shared workspace When the root workspace "some-root-workspace" is created And the shared workspace "some-shared-workspace" is created with the target workspace "some-root-workspace" @@ -196,6 +204,7 @@ Feature: Neos WorkspaceService related features Scenario: Workspace permissions for personal workspace for admin user Given the root workspace "live" is created When a personal workspace for user "jane.doe" is created + Then the personal workspace for user "jane.doe" is "jane-doe" Then the workspace "jane-doe" should have the following metadata: | Title | Description | Classification | Owner user id | | Jane Doe | | PERSONAL | janedoe | From 94c40655de258e68b22cb78437ca6dbd9955ff55 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Mon, 9 Dec 2024 14:55:29 +0100 Subject: [PATCH 18/24] TASK: Add test to ensure personal workspace names are always unique see bug https://github.com/neos/neos-development-collection/issues/2850 --- .../ContentRepository/WorkspaceService.feature | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/Neos.Neos/Tests/Behavior/Features/ContentRepository/WorkspaceService.feature b/Neos.Neos/Tests/Behavior/Features/ContentRepository/WorkspaceService.feature index c147d7b0007..0181b7b2d9c 100644 --- a/Neos.Neos/Tests/Behavior/Features/ContentRepository/WorkspaceService.feature +++ b/Neos.Neos/Tests/Behavior/Features/ContentRepository/WorkspaceService.feature @@ -95,6 +95,19 @@ Feature: Neos WorkspaceService related features | Title | Description | Classification | Owner user id | | some-user-workspace | | PERSONAL | janedoe | + Scenario: Personal workspace names are unique https://github.com/neos/neos-development-collection/issues/2850 + And the following Neos users exist: + | Id | Username | First name | Last name | Roles | + | 123 | test-user | Test | User | Neos.Neos:Administrator | + | 456 | test.user | Test | User | Neos.Neos:Administrator | + + When the root workspace "live" is created + + When a personal workspace for user "test-user" is created + Then the personal workspace for user "test-user" is "test-user" + + When a personal workspace for user "test.user" is created + Then the personal workspace for user "test.user" is "test-user-1" Scenario: For multiple personal workspaces only one workspace is returned When the root workspace "some-root-workspace" is created From 099274a32ad5793e763164f13a43a98d2ba5272d Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Mon, 9 Dec 2024 15:13:57 +0100 Subject: [PATCH 19/24] TASK: Document and cleanup `createRootWorkspace` and `createSharedWorkspace` --- .../Domain/Service/WorkspaceService.php | 62 +++++++++++++------ 1 file changed, 42 insertions(+), 20 deletions(-) diff --git a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php index 11d692c621c..1b97f6d670f 100644 --- a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php +++ b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php @@ -113,9 +113,19 @@ public function getPersonalWorkspaceForUser(ContentRepositoryId $contentReposito /** * Create a new root (aka base) workspace with the specified metadata * + * To ensure that editors can publish to the live workspace and to allow everybody to view it an assignment like {@see WorkspaceRoleAssignments::createForLiveWorkspace} needs to be specified: + * + * $this->workspaceService->createRootWorkspace( + * $contentRepositoryId, + * WorkspaceName::forLive(), + * WorkspaceTitle::fromString('Public live workspace'), + * WorkspaceDescription::empty(), + * WorkspaceRoleAssignments::createForLiveWorkspace() + * ); + * * @throws WorkspaceAlreadyExists */ - public function createRootWorkspace(ContentRepositoryId $contentRepositoryId, WorkspaceName $workspaceName, WorkspaceTitle $title, WorkspaceDescription $description, WorkspaceRoleAssignments $workspaceRoleAssignments): void + public function createRootWorkspace(ContentRepositoryId $contentRepositoryId, WorkspaceName $workspaceName, WorkspaceTitle $title, WorkspaceDescription $description, WorkspaceRoleAssignments $assignments): void { $contentRepository = $this->contentRepositoryRegistry->get($contentRepositoryId); $contentRepository->handle( @@ -124,8 +134,8 @@ public function createRootWorkspace(ContentRepositoryId $contentRepositoryId, Wo ContentStreamId::create() ) ); - $this->metadataAndRoleRepository->addWorkspaceMetadata($contentRepositoryId, $workspaceName, $title, $description, WorkspaceClassification::ROOT, null); - foreach ($workspaceRoleAssignments as $assignment) { + $this->metadataAndRoleRepository->addWorkspaceMetadata($contentRepositoryId, $workspaceName, $title, $description, WorkspaceClassification::ROOT, ownerUserId: null); + foreach ($assignments as $assignment) { $this->metadataAndRoleRepository->assignWorkspaceRole($contentRepositoryId, $workspaceName, $assignment); } } @@ -135,18 +145,43 @@ public function createRootWorkspace(ContentRepositoryId $contentRepositoryId, Wo */ public function createPersonalWorkspace(ContentRepositoryId $contentRepositoryId, WorkspaceName $workspaceName, WorkspaceTitle $title, WorkspaceDescription $description, WorkspaceName $baseWorkspaceName, UserId $ownerId): void { - $this->createWorkspace($contentRepositoryId, $workspaceName, $title, $description, $baseWorkspaceName, $ownerId, WorkspaceClassification::PERSONAL); + $contentRepository = $this->contentRepositoryRegistry->get($contentRepositoryId); + $contentRepository->handle( + CreateWorkspace::create( + $workspaceName, + $baseWorkspaceName, + ContentStreamId::create() + ) + ); + $this->metadataAndRoleRepository->addWorkspaceMetadata($contentRepositoryId, $workspaceName, $title, $description, WorkspaceClassification::PERSONAL, $ownerId); } /** * Create a new, potentially shared, workspace * + * To ensure that the user can manage the shared workspace and to enable collaborates an assignment like {@see WorkspaceRoleAssignments::createForSharedWorkspace} needs to be specified: + * + * $this->workspaceService->createWorkspace( + * ..., + * assignments: WorkspaceRoleAssignments::createForSharedWorkspace( + * $currentUser->getId() + * ) + * ); + * * NOTE: By default - if no role assignments are specified - only administrators can manage workspaces without role assignments. */ - public function createSharedWorkspace(ContentRepositoryId $contentRepositoryId, WorkspaceName $workspaceName, WorkspaceTitle $title, WorkspaceDescription $description, WorkspaceName $baseWorkspaceName, WorkspaceRoleAssignments $workspaceRoleAssignments): void + public function createSharedWorkspace(ContentRepositoryId $contentRepositoryId, WorkspaceName $workspaceName, WorkspaceTitle $title, WorkspaceDescription $description, WorkspaceName $baseWorkspaceName, WorkspaceRoleAssignments $assignments): void { - $this->createWorkspace($contentRepositoryId, $workspaceName, $title, $description, $baseWorkspaceName, null, WorkspaceClassification::SHARED); - foreach ($workspaceRoleAssignments as $assignment) { + $contentRepository = $this->contentRepositoryRegistry->get($contentRepositoryId); + $contentRepository->handle( + CreateWorkspace::create( + $workspaceName, + $baseWorkspaceName, + ContentStreamId::create() + ) + ); + $this->metadataAndRoleRepository->addWorkspaceMetadata($contentRepositoryId, $workspaceName, $title, $description, WorkspaceClassification::SHARED, ownerUserId: null); + foreach ($assignments as $assignment) { $this->metadataAndRoleRepository->assignWorkspaceRole($contentRepositoryId, $workspaceName, $assignment); } } @@ -256,19 +291,6 @@ public function getUniqueWorkspaceName(ContentRepositoryId $contentRepositoryId, // ------------------ - private function createWorkspace(ContentRepositoryId $contentRepositoryId, WorkspaceName $workspaceName, WorkspaceTitle $title, WorkspaceDescription $description, WorkspaceName $baseWorkspaceName, UserId|null $ownerId, WorkspaceClassification $classification): void - { - $contentRepository = $this->contentRepositoryRegistry->get($contentRepositoryId); - $contentRepository->handle( - CreateWorkspace::create( - $workspaceName, - $baseWorkspaceName, - ContentStreamId::create() - ) - ); - $this->metadataAndRoleRepository->addWorkspaceMetadata($contentRepositoryId, $workspaceName, $title, $description, $classification, $ownerId); - } - /** * @throws WorkspaceDoesNotExist if the workspace does not exist */ From 31473fb4efae71bb62a10665414a6cd545ae4f4b Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Mon, 9 Dec 2024 15:52:32 +0100 Subject: [PATCH 20/24] BUGFIX: Ensure for now that the api does not allow two personal workspaces for one user we might change the concepts later and add a mapping to the "current" users workspace in their session but for now we dont need the complexity --- .../WorkspaceMetadataAndRoleRepository.php | 5 ++--- .../Domain/Service/WorkspaceService.php | 12 +++++++----- .../Bootstrap/WorkspaceServiceTrait.php | 19 ++++++++++++++++++- .../Security/WorkspacePermissions.feature | 4 ++-- .../WorkspaceService.feature | 10 +++++++--- 5 files changed, 36 insertions(+), 14 deletions(-) diff --git a/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php b/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php index d11285d6414..aabe268cae3 100644 --- a/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php +++ b/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php @@ -352,10 +352,9 @@ public function findPrimaryWorkspaceNameForUser(ContentRepositoryId $contentRepo content_repository_id = :contentRepositoryId AND classification = :personalWorkspaceClassification AND owner_user_id = :userId - -- TODO introduce better deterministic selection of "primary" workspace if multiple exist - ORDER BY workspace_name - LIMIT 1 SQL; + // We don't order the results and return the first matching workspace. + // In case multiple exist - which is not desired currently - and in the happy path not possible via the api (race conditions aside) - the order is not defined. $workspaceName = $this->dbal->fetchOne($query, [ 'contentRepositoryId' => $contentRepositoryId->value, 'personalWorkspaceClassification' => WorkspaceClassification::PERSONAL->value, diff --git a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php index 1b97f6d670f..55376a4f563 100644 --- a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php +++ b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php @@ -97,15 +97,13 @@ public function setWorkspaceDescription(ContentRepositoryId $contentRepositoryId } /** - * Retrieve the first personal workspace for the specified user - * - * NOTE: Conventionally each user has only a single personal workspace but in case multiple exist the first is returned + * Retrieve the personal workspace for the specified user, if no workspace exist an exception is thrown. */ public function getPersonalWorkspaceForUser(ContentRepositoryId $contentRepositoryId, UserId $userId): Workspace { $workspaceName = $this->metadataAndRoleRepository->findPrimaryWorkspaceNameForUser($contentRepositoryId, $userId); if ($workspaceName === null) { - throw new \RuntimeException(sprintf('No workspace is assigned to the user with id "%s")', $userId->value), 1718293801); + throw new \RuntimeException(sprintf('No workspace is assigned to the user with id "%s")', $userId->value), 1733755300); } return $this->requireWorkspace($contentRepositoryId, $workspaceName); } @@ -141,10 +139,14 @@ public function createRootWorkspace(ContentRepositoryId $contentRepositoryId, Wo } /** - * Create a new, personal, workspace for the specified user + * Create a new, personal, workspace for the specified user (fails if the user already owns a workspace) */ public function createPersonalWorkspace(ContentRepositoryId $contentRepositoryId, WorkspaceName $workspaceName, WorkspaceTitle $title, WorkspaceDescription $description, WorkspaceName $baseWorkspaceName, UserId $ownerId): void { + $existingUserWorkspace = $this->metadataAndRoleRepository->findPrimaryWorkspaceNameForUser($contentRepositoryId, $ownerId); + if ($existingUserWorkspace !== null) { + throw new \RuntimeException(sprintf('Failed to create personal workspace "%s" for user with id "%s", because the workspace "%s" is already assigned to the user', $workspaceName->value, $ownerId->value, $existingUserWorkspace->value), 1733754904); + } $contentRepository = $this->contentRepositoryRegistry->get($contentRepositoryId); $contentRepository->handle( CreateWorkspace::create( diff --git a/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php b/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php index 0b969427e98..987c8304930 100644 --- a/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php +++ b/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php @@ -120,7 +120,7 @@ public function aPersonalWorkspaceForUserIsCreated(string $username): void /** * @Then the personal workspace for user :username is :workspaceName */ - public function thePersonalWorkspaceForUserIs(string $workspaceName, string $username): void + public function thePersonalWorkspaceForUserIs(string $username, string $workspaceName): void { $ownerUserId = $this->userIdForUsername($username); $actualWorkspace = $this->getObject(WorkspaceService::class)->getPersonalWorkspaceForUser($this->currentContentRepository->id, $ownerUserId); @@ -128,6 +128,23 @@ public function thePersonalWorkspaceForUserIs(string $workspaceName, string $use Assert::assertSame($workspaceName, $actualWorkspace->workspaceName->value); } + /** + * @Then the user :username does not have a personal workspace + */ + public function theUserDoesNotHaveAPersonalWorkspace(string $username): void + { + $ownerUserId = $this->userIdForUsername($username); + try { + $this->getObject(WorkspaceService::class)->getPersonalWorkspaceForUser($this->currentContentRepository->id, $ownerUserId); + } catch (\Throwable $e) { + // todo throw WorkspaceDoesNotExist instead?? + Assert::assertInstanceOf(\RuntimeException::class, $e, $e->getMessage()); + Assert::assertSame(1733755300, $e->getCode()); + return; + } + Assert::fail('Did not throw'); + } + /** * @When the shared workspace :workspaceName is created with the target workspace :targetWorkspace * @When the shared workspace :workspaceName is created with the target workspace :targetWorkspace and role assignments: diff --git a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature index 3fc1101b7d6..def32d52569 100644 --- a/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature +++ b/Neos.Neos/Tests/Behavior/Features/ContentRepository/Security/WorkspacePermissions.feature @@ -99,7 +99,7 @@ Feature: Workspace permission related features Examples: | user | | collaborator | - | owner | + # the "owner" user already owns a workspace Scenario: Creating a workspace without Neos User but READ permissions on live Given I am not authenticated @@ -114,11 +114,11 @@ Feature: Workspace permission related features Examples: | user | | admin | - | owner | | collaborator | | uninvolved_editor | | restricted_editor | | simple_user | + # the "owner" user already owns a workspace Scenario Outline: Changing a base workspace without MANAGE permissions or READ permissions on the base workspace Given I am authenticated as diff --git a/Neos.Neos/Tests/Behavior/Features/ContentRepository/WorkspaceService.feature b/Neos.Neos/Tests/Behavior/Features/ContentRepository/WorkspaceService.feature index 0181b7b2d9c..c91c1a3f3b6 100644 --- a/Neos.Neos/Tests/Behavior/Features/ContentRepository/WorkspaceService.feature +++ b/Neos.Neos/Tests/Behavior/Features/ContentRepository/WorkspaceService.feature @@ -89,6 +89,7 @@ Feature: Neos WorkspaceService related features Scenario: Create a single personal workspace When the root workspace "some-root-workspace" is created + Then the user "jane.doe" does not have a personal workspace And the personal workspace "some-user-workspace" is created with the target workspace "some-root-workspace" for user "jane.doe" Then the personal workspace for user "jane.doe" is "some-user-workspace" Then the workspace "some-user-workspace" should have the following metadata: @@ -109,11 +110,14 @@ Feature: Neos WorkspaceService related features When a personal workspace for user "test.user" is created Then the personal workspace for user "test.user" is "test-user-1" - Scenario: For multiple personal workspaces only one workspace is returned + Scenario: A user cannot have multiple personal workspaces When the root workspace "some-root-workspace" is created - And the personal workspace "b-user-workspace" is created with the target workspace "some-root-workspace" for user "jane.doe" And the personal workspace "a-user-workspace" is created with the target workspace "some-root-workspace" for user "jane.doe" - And the personal workspace "c-user-workspace" is created with the target workspace "some-root-workspace" for user "jane.doe" + And the personal workspace "b-user-workspace" is created with the target workspace "some-root-workspace" for user "jane.doe" + Then an exception of type "RuntimeException" should be thrown with message: + """ + Failed to create personal workspace "b-user-workspace" for user with id "janedoe", because the workspace "a-user-workspace" is already assigned to the user + """ Then the personal workspace for user "jane.doe" is "a-user-workspace" Scenario: Create a single shared workspace From 5cc7e0f42b9d84347b64ea5bb04ea795600f5339 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Tue, 10 Dec 2024 11:24:17 +0100 Subject: [PATCH 21/24] TASK: Apply suggestions from review --- .../Classes/SharedModel/Exception/WorkspaceDoesNotExist.php | 2 +- Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignments.php | 2 +- Neos.Neos/Classes/Domain/Service/WorkspaceService.php | 2 +- .../Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Neos.ContentRepository.Core/Classes/SharedModel/Exception/WorkspaceDoesNotExist.php b/Neos.ContentRepository.Core/Classes/SharedModel/Exception/WorkspaceDoesNotExist.php index 67939a4bea7..26004ba393b 100644 --- a/Neos.ContentRepository.Core/Classes/SharedModel/Exception/WorkspaceDoesNotExist.php +++ b/Neos.ContentRepository.Core/Classes/SharedModel/Exception/WorkspaceDoesNotExist.php @@ -20,7 +20,7 @@ /** * @api because exception is thrown during invariant checks on command execution or when attempting to query a non-existing workspace */ -final class WorkspaceDoesNotExist extends \RuntimeException +final class WorkspaceDoesNotExist extends \DomainException { public static function butWasSupposedTo(WorkspaceName $name): self { diff --git a/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignments.php b/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignments.php index f381d3c80da..87b33bc0d3b 100644 --- a/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignments.php +++ b/Neos.Neos/Classes/Domain/Model/WorkspaceRoleAssignments.php @@ -107,7 +107,7 @@ public function contains(WorkspaceRoleAssignment $assignment): bool return false; } - public function add(WorkspaceRoleAssignment $assignment): self + public function withAssignment(WorkspaceRoleAssignment $assignment): self { return new self(...[...$this->assignments, $assignment]); } diff --git a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php index 55376a4f563..0b0f63390a4 100644 --- a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php +++ b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php @@ -103,7 +103,7 @@ public function getPersonalWorkspaceForUser(ContentRepositoryId $contentReposito { $workspaceName = $this->metadataAndRoleRepository->findPrimaryWorkspaceNameForUser($contentRepositoryId, $userId); if ($workspaceName === null) { - throw new \RuntimeException(sprintf('No workspace is assigned to the user with id "%s")', $userId->value), 1733755300); + throw new \RuntimeException(sprintf('No workspace is assigned to the user with id "%s")', $userId->value), 1718293801); } return $this->requireWorkspace($contentRepositoryId, $workspaceName); } diff --git a/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php b/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php index 987c8304930..c89a13d6eb9 100644 --- a/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php +++ b/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php @@ -153,7 +153,7 @@ public function theSharedWorkspaceIsCreatedWithTheTargetWorkspace(string $worksp { $workspaceRoleAssignments = WorkspaceRoleAssignments::createEmpty(); foreach ($rawRoleAssignments?->getHash() ?? [] as $row) { - $workspaceRoleAssignments = $workspaceRoleAssignments->add(WorkspaceRoleAssignment::create( + $workspaceRoleAssignments = $workspaceRoleAssignments->withAssignment(WorkspaceRoleAssignment::create( WorkspaceRoleSubject::create( WorkspaceRoleSubjectType::from($row['Type']), $row['Value'] From 3f65a7435fa24813b8156cd9308c7083089314ed Mon Sep 17 00:00:00 2001 From: Denny Lubitz Date: Tue, 10 Dec 2024 12:07:34 +0100 Subject: [PATCH 22/24] BUGFIX: Use correct error code to assert --- .../Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php b/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php index c89a13d6eb9..938ce3f31a8 100644 --- a/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php +++ b/Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php @@ -139,7 +139,7 @@ public function theUserDoesNotHaveAPersonalWorkspace(string $username): void } catch (\Throwable $e) { // todo throw WorkspaceDoesNotExist instead?? Assert::assertInstanceOf(\RuntimeException::class, $e, $e->getMessage()); - Assert::assertSame(1733755300, $e->getCode()); + Assert::assertSame(1718293801, $e->getCode()); return; } Assert::fail('Did not throw'); From ddccba16f5168bd6c97b439ee729df253c68330a Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Thu, 12 Dec 2024 10:30:32 +0100 Subject: [PATCH 23/24] TASK: Ensure that the workspace owner is unique via sql index We still try to catch the error beforehand via php constraint (so that the `CreateWorkspace` is not handled), but in a race condition this will be thrown at last: > Integrity constraint violation: 1062 Duplicate entry 'default-janedoe' for key 'owner'' --- .../WorkspaceMetadataAndRoleRepository.php | 4 +- .../Domain/Service/WorkspaceService.php | 6 +-- .../Mysql/Version20240425223900.php | 2 +- .../Mysql/Version20241212000000.php | 40 +++++++++++++++++++ .../Postgresql/Version20240425223901.php | 2 +- .../Postgresql/Version20241212000001.php | 40 +++++++++++++++++++ 6 files changed, 86 insertions(+), 8 deletions(-) create mode 100644 Neos.Neos/Migrations/Mysql/Version20241212000000.php create mode 100644 Neos.Neos/Migrations/Postgresql/Version20241212000001.php diff --git a/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php b/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php index aabe268cae3..ba487eae2e7 100644 --- a/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php +++ b/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php @@ -340,7 +340,7 @@ public function addWorkspaceMetadata(ContentRepositoryId $contentRepositoryId, W } } - public function findPrimaryWorkspaceNameForUser(ContentRepositoryId $contentRepositoryId, UserId $userId): ?WorkspaceName + public function findWorkspaceNameByUser(ContentRepositoryId $contentRepositoryId, UserId $userId): ?WorkspaceName { $tableMetadata = self::TABLE_NAME_WORKSPACE_METADATA; $query = <<dbal->fetchOne($query, [ 'contentRepositoryId' => $contentRepositoryId->value, 'personalWorkspaceClassification' => WorkspaceClassification::PERSONAL->value, diff --git a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php index 0b0f63390a4..578f39b7a25 100644 --- a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php +++ b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php @@ -101,7 +101,7 @@ public function setWorkspaceDescription(ContentRepositoryId $contentRepositoryId */ public function getPersonalWorkspaceForUser(ContentRepositoryId $contentRepositoryId, UserId $userId): Workspace { - $workspaceName = $this->metadataAndRoleRepository->findPrimaryWorkspaceNameForUser($contentRepositoryId, $userId); + $workspaceName = $this->metadataAndRoleRepository->findWorkspaceNameByUser($contentRepositoryId, $userId); if ($workspaceName === null) { throw new \RuntimeException(sprintf('No workspace is assigned to the user with id "%s")', $userId->value), 1718293801); } @@ -143,7 +143,7 @@ public function createRootWorkspace(ContentRepositoryId $contentRepositoryId, Wo */ public function createPersonalWorkspace(ContentRepositoryId $contentRepositoryId, WorkspaceName $workspaceName, WorkspaceTitle $title, WorkspaceDescription $description, WorkspaceName $baseWorkspaceName, UserId $ownerId): void { - $existingUserWorkspace = $this->metadataAndRoleRepository->findPrimaryWorkspaceNameForUser($contentRepositoryId, $ownerId); + $existingUserWorkspace = $this->metadataAndRoleRepository->findWorkspaceNameByUser($contentRepositoryId, $ownerId); if ($existingUserWorkspace !== null) { throw new \RuntimeException(sprintf('Failed to create personal workspace "%s" for user with id "%s", because the workspace "%s" is already assigned to the user', $workspaceName->value, $ownerId->value, $existingUserWorkspace->value), 1733754904); } @@ -193,7 +193,7 @@ public function createSharedWorkspace(ContentRepositoryId $contentRepositoryId, */ public function createPersonalWorkspaceForUserIfMissing(ContentRepositoryId $contentRepositoryId, User $user): void { - $existingWorkspaceName = $this->metadataAndRoleRepository->findPrimaryWorkspaceNameForUser($contentRepositoryId, $user->getId()); + $existingWorkspaceName = $this->metadataAndRoleRepository->findWorkspaceNameByUser($contentRepositoryId, $user->getId()); if ($existingWorkspaceName !== null) { $this->requireWorkspace($contentRepositoryId, $existingWorkspaceName); return; diff --git a/Neos.Neos/Migrations/Mysql/Version20240425223900.php b/Neos.Neos/Migrations/Mysql/Version20240425223900.php index 69bd0305bcd..aca3a3a172a 100644 --- a/Neos.Neos/Migrations/Mysql/Version20240425223900.php +++ b/Neos.Neos/Migrations/Mysql/Version20240425223900.php @@ -29,7 +29,7 @@ public function up(Schema $schema): void $tableWorkspaceMetadata->addColumn('classification', 'string', ['length' => 255]); $tableWorkspaceMetadata->addColumn('owner_user_id', 'string', ['length' => 255, 'notnull' => false]); $tableWorkspaceMetadata->setPrimaryKey(['content_repository_id', 'workspace_name']); - $tableWorkspaceMetadata->addIndex(['owner_user_id']); + $tableWorkspaceMetadata->addIndex(['owner_user_id'], 'IDX_D6197E562B18554A'); $tableWorkspaceRole = $schema->createTable('neos_neos_workspace_role'); $tableWorkspaceRole->addColumn('content_repository_id', 'string', ['length' => 16]); diff --git a/Neos.Neos/Migrations/Mysql/Version20241212000000.php b/Neos.Neos/Migrations/Mysql/Version20241212000000.php new file mode 100644 index 00000000000..260398ed844 --- /dev/null +++ b/Neos.Neos/Migrations/Mysql/Version20241212000000.php @@ -0,0 +1,40 @@ +abortIf( + !$this->connection->getDatabasePlatform() instanceof \Doctrine\DBAL\Platforms\MariaDBPlatform, + "Migration can only be executed safely on '\Doctrine\DBAL\Platforms\MariaDBPlatform'." + ); + + $tableWorkspaceMetadata = $schema->getTable('neos_neos_workspace_metadata'); + $tableWorkspaceMetadata->addUniqueIndex(['content_repository_id', 'owner_user_id'], 'owner'); + $tableWorkspaceMetadata->dropIndex('IDX_D6197E562B18554A'); + } + + public function down(Schema $schema): void + { + $this->abortIf( + !$this->connection->getDatabasePlatform() instanceof \Doctrine\DBAL\Platforms\MariaDBPlatform, + "Migration can only be executed safely on '\Doctrine\DBAL\Platforms\MariaDBPlatform'." + ); + + $tableWorkspaceMetadata = $schema->getTable('neos_neos_workspace_metadata'); + $tableWorkspaceMetadata->addIndex(['owner_user_id'], 'IDX_D6197E562B18554A'); + $tableWorkspaceMetadata->dropIndex('owner'); + } +} diff --git a/Neos.Neos/Migrations/Postgresql/Version20240425223901.php b/Neos.Neos/Migrations/Postgresql/Version20240425223901.php index b370bd7b9d1..93cb4813b24 100644 --- a/Neos.Neos/Migrations/Postgresql/Version20240425223901.php +++ b/Neos.Neos/Migrations/Postgresql/Version20240425223901.php @@ -26,7 +26,7 @@ public function up(Schema $schema): void $tableWorkspaceMetadata->addColumn('classification', 'string', ['length' => 255]); $tableWorkspaceMetadata->addColumn('owner_user_id', 'string', ['length' => 255, 'notnull' => false]); $tableWorkspaceMetadata->setPrimaryKey(['content_repository_id', 'workspace_name']); - $tableWorkspaceMetadata->addIndex(['owner_user_id']); + $tableWorkspaceMetadata->addIndex(['owner_user_id'], 'IDX_D6197E562B18554A'); $tableWorkspaceRole = $schema->createTable('neos_neos_workspace_role'); $tableWorkspaceRole->addColumn('content_repository_id', 'string', ['length' => 16]); diff --git a/Neos.Neos/Migrations/Postgresql/Version20241212000001.php b/Neos.Neos/Migrations/Postgresql/Version20241212000001.php new file mode 100644 index 00000000000..85e574c2969 --- /dev/null +++ b/Neos.Neos/Migrations/Postgresql/Version20241212000001.php @@ -0,0 +1,40 @@ +abortIf( + !$this->connection->getDatabasePlatform() instanceof \Doctrine\DBAL\Platforms\PostgreSQLPlatform, + "Migration can only be executed safely on '\Doctrine\DBAL\Platforms\PostgreSQLPlatform'." + ); + + $tableWorkspaceMetadata = $schema->getTable('neos_neos_workspace_metadata'); + $tableWorkspaceMetadata->addUniqueIndex(['content_repository_id', 'owner_user_id'], 'owner'); + $tableWorkspaceMetadata->dropIndex('IDX_D6197E562B18554A'); + } + + public function down(Schema $schema): void + { + $this->abortIf( + !$this->connection->getDatabasePlatform() instanceof \Doctrine\DBAL\Platforms\PostgreSQLPlatform, + "Migration can only be executed safely on '\Doctrine\DBAL\Platforms\PostgreSQLPlatform'." + ); + + $tableWorkspaceMetadata = $schema->getTable('neos_neos_workspace_metadata'); + $tableWorkspaceMetadata->addIndex(['owner_user_id'], 'IDX_D6197E562B18554A'); + $tableWorkspaceMetadata->dropIndex('owner'); + } +} From 08d01e133bc554b1ac9139176a2d6f20f9f0fb54 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Thu, 12 Dec 2024 10:34:38 +0100 Subject: [PATCH 24/24] TASK: Make workspace role and metadata creation atomic --- .../WorkspaceMetadataAndRoleRepository.php | 9 ++++++++ .../Domain/Service/WorkspaceService.php | 22 ++++++++++++------- 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php b/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php index ba487eae2e7..c5ce393682a 100644 --- a/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php +++ b/Neos.Neos/Classes/Domain/Repository/WorkspaceMetadataAndRoleRepository.php @@ -360,4 +360,13 @@ public function findWorkspaceNameByUser(ContentRepositoryId $contentRepositoryId ]); return $workspaceName === false ? null : WorkspaceName::fromString($workspaceName); } + + /** + * @param \Closure(): void $fn + * @return void + */ + public function transactional(\Closure $fn): void + { + $this->dbal->transactional($fn); + } } diff --git a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php index 578f39b7a25..00ac32de62c 100644 --- a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php +++ b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php @@ -132,10 +132,13 @@ public function createRootWorkspace(ContentRepositoryId $contentRepositoryId, Wo ContentStreamId::create() ) ); - $this->metadataAndRoleRepository->addWorkspaceMetadata($contentRepositoryId, $workspaceName, $title, $description, WorkspaceClassification::ROOT, ownerUserId: null); - foreach ($assignments as $assignment) { - $this->metadataAndRoleRepository->assignWorkspaceRole($contentRepositoryId, $workspaceName, $assignment); - } + + $this->metadataAndRoleRepository->transactional(function () use ($contentRepositoryId, $workspaceName, $title, $description, $assignments) { + $this->metadataAndRoleRepository->addWorkspaceMetadata($contentRepositoryId, $workspaceName, $title, $description, WorkspaceClassification::ROOT, ownerUserId: null); + foreach ($assignments as $assignment) { + $this->metadataAndRoleRepository->assignWorkspaceRole($contentRepositoryId, $workspaceName, $assignment); + } + }); } /** @@ -182,10 +185,13 @@ public function createSharedWorkspace(ContentRepositoryId $contentRepositoryId, ContentStreamId::create() ) ); - $this->metadataAndRoleRepository->addWorkspaceMetadata($contentRepositoryId, $workspaceName, $title, $description, WorkspaceClassification::SHARED, ownerUserId: null); - foreach ($assignments as $assignment) { - $this->metadataAndRoleRepository->assignWorkspaceRole($contentRepositoryId, $workspaceName, $assignment); - } + + $this->metadataAndRoleRepository->transactional(function () use ($contentRepositoryId, $workspaceName, $title, $description, $assignments) { + $this->metadataAndRoleRepository->addWorkspaceMetadata($contentRepositoryId, $workspaceName, $title, $description, WorkspaceClassification::SHARED, ownerUserId: null); + foreach ($assignments as $assignment) { + $this->metadataAndRoleRepository->assignWorkspaceRole($contentRepositoryId, $workspaceName, $assignment); + } + }); } /**