BUGFIX: WorkspaceService adjust to real world use cases

[mhsdesign]

The adjustments which are needed for the new workspace UI #5132

Fix up for #5146 (original introduction of mighty WorkspaceService), and #5298 (partial refactoring yet again towards authentication provider)

As separate PR to have the bugfixes tested and also better discussed.

Todo:

• write test for workspace delete
• test for workspace low level pruning? -> should be tested as part of site:pruneAll
• test for live publisher (via "normal" editor)
• test to change base workspace
• cleanup old policy yaml, and explain "special" magic features (like live publisher is added as role on creation for live)
• Discuss deletion should also work from core command? Maybe catchup hook or delete dangling workspace metadata on creation of new identical

Upgrade instructions

Review instructions

Checklist

[mhsdesign]

Also im not sure why these changes are needed but they were made originally in 916e510:

diff --git a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php
index 8d7e11d2e4f..d515a0205cc 100644
--- a/Neos.Neos/Classes/Domain/Service/WorkspaceService.php
+++ b/Neos.Neos/Classes/Domain/Service/WorkspaceService.php
@@ -302,19 +302,30 @@ public function getWorkspacePermissionsForUser(ContentRepositoryId $contentRepos
         } catch (NoSuchRoleException $e) {
             throw new \RuntimeException(sprintf('Failed to determine roles for user "%s", check your package dependencies: %s', $user->getId()->value, $e->getMessage()), 1727084881, $e);
         }
+
+        $userIsAdministrator = in_array('Neos.Neos:Administrator', $userRoles, true);
+
+        if ($userIsAdministrator) {
+            return WorkspacePermissions::all();
+        }
+
         $workspaceMetadata = $this->loadWorkspaceMetadata($contentRepositoryId, $workspaceName);
-        if ($workspaceMetadata !== null && $workspaceMetadata->ownerUserId !== null && $workspaceMetadata->ownerUserId->equals($user->getId())) {
+        $userIsOwner = $workspaceMetadata !== null && $workspaceMetadata->ownerUserId !== null && $workspaceMetadata->ownerUserId->equals($user->getId());
+
+        if ($userIsOwner) {
             return WorkspacePermissions::all();
         }
+
         $userWorkspaceRole = $this->loadWorkspaceRoleOfUser($contentRepositoryId, $workspaceName, $user->getId(), $userRoles);
-        $userIsAdministrator = in_array('Neos.Neos:Administrator', $userRoles, true);
+
         if ($userWorkspaceRole === null) {
-            return WorkspacePermissions::create(false, false, $userIsAdministrator);
+            return WorkspacePermissions::none();
         }
+
         return WorkspacePermissions::create(
             read: $userWorkspaceRole->isAtLeast(WorkspaceRole::COLLABORATOR),
             write: $userWorkspaceRole->isAtLeast(WorkspaceRole::COLLABORATOR),
-            manage: $userIsAdministrator || $userWorkspaceRole->isAtLeast(WorkspaceRole::MANAGER),
+            manage: $userWorkspaceRole->isAtLeast(WorkspaceRole::MANAGER),
         );
     }

a lot of test fail with this change and im not sure what the reason for this was.

[mhsdesign]

Also it seems, that if we allow to create workspaces only with read permission this test does not pass because EVERY Neos user is EVERYBODY and they are VIEWER

Scenario Outline: Creating a workspace without READ permissions (on live)
  Given I am authenticated as <user>
  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 <user>
  Then an exception of type "AccessDenied" should be thrown with code 1729086686

  Examples:
    | user              |
    | restricted_editor |
    | simple_user       |

Technically - if would test this which we dont - this meand that even an unauthenticated request could just create workspaces? - we definitely need test for the not authenticated case as well imo, especially since the default => grant would be dangerous no? ->

neos-development-collection/Neos.Neos/Classes/Security/ContentRepositoryAuthProvider/ContentRepositoryAuthProvider.php

Line 173 in 484c1ab

default => Privilege::granted('Command not restricted'),

[mhsdesign]

basically theses test means that EVERYBODY can create a new workspace based on live, not even a neos user is required - im not sure if this is correct, but in 8.3 i also could find no restriction for new Workspace or adding that to the repository.

[bwaidelich]

@mhsdesign thanks for extracting this PR!

im not sure why these changes are needed

They make me suspicious, too. IMO an admin should be able to grant themselves permissions to write to other personal workspaces, but they should not have this permission automagically.

we definitely need test for the not authenticated case as well imo [...]

Yes, absolutely. I'm not sure, who is supposed to be able to create workspaces – maybe we need an explicit role/privilege for that

[...] especially since the default => grant would be dangerous no?

The default of grant just matches commands that don't exist yet (because other cases should be covered). That is potentially dangerous ofc (I tried to create an "allowlist"-test but it's not easily possible to get all instances of the CommandInterface without our Flow ReflectionService). But AFAIS that is not related to the unauthenticated case

[mhsdesign]

regarding that not authenticated users can create workspaces based on live (see 28b98c5)

maybe that is okay as in 8.3 that was only restricted via the endpoint and Neos.Neos:Backend.CreateWorkspaces now in 9.0 renamed to Neos.Workspace.Ui:Backend.CreateWorkspaces:

https://github.com/neos/neos-development-collection/blob/67c9ec4d2c1a198786b18a3da194edeca2bce5da/Neos.Workspace.Ui/Configuration/Policy.yaml#L9C6-L11

So the privilege still exists and covers the default endpoint and usecase. Still having a custom controller could contain code creating a workspace which is then not restricted by the cr. As long as this is the same behaviour as in 8.3 that might be just valid.

---

But AFAIS that is not related to the unauthenticated case

it is imo, because then we never ensure that the roles contain Authenticated or that a user exists...

[mhsdesign]

Fixed a conceptional bug to allow to specify WorkspaceRoleAssignments when creating a new shared workspace

otherwise authorisation fails when trying to issue assignWorkspaceRole: the workspace roles cannot be assigned because management is not granted (yet) - only admins would be able to do that.

I fixed that nicely by allowing to specify the WorkspaceRoleAssignments at creation:

$this->workspaceService->createSharedWorkspace(
    $contentRepositoryId,
    $workspaceName,
    $title,
    $description,
    $baseWorkspace,
    WorkspaceRoleAssignments::create(
        WorkspaceRoleAssignment::createForUser(
            $currentUser->getId(),
            WorkspaceRole::MANAGER,
        ),
        WorkspaceRoleAssignment::createForGroup(
            'Neos.Neos:AbstractEditor',
            WorkspaceRole::COLLABORATOR,
        )
    )
);

Edit i introduced static factories for that istead:

$this->workspaceService->createSharedWorkspace(
    ...,
    WorkspaceRoleAssignments::createForSharedWorkspace(
        $currentUser->getId()
    )
);

$this->workspaceService->createRootWorkspace(
    $this->contentRepository->id,
    WorkspaceName::forLive(),
    WorkspaceTitle::fromString('Public live workspace'),
    WorkspaceDescription::empty(),
    WorkspaceRoleAssignments::createForLiveWorkspace()
);

/**
 * 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,
        )
    );
}

[mhsdesign]

maybe for the 9.0 release we should disallow having two personal workspaces per user? Not really via sql index but at least via php api? That would allow us to reduce some insanity like seen here 😅 We can always relax that constraint later.

[mhsdesign]

Edit: with 31473fb i introduced a constraint to not allow that 😅

fully outdated

[dlubitz]

Looks good to me, once the tests have been fixed.

[bwaidelich]

Great, thanks a lot!
I just left some (partly nitpicky) comments for now, but in general it's a +1 from me