neos / neos-development-collection

The unified repository containing the Neos core packages, used for Neos development.
https://www.neos.io/
GNU General Public License v3.0
260 stars 221 forks source link

Odd things about (user) workspaces #4566

Open mhsdesign opened 1 year ago

mhsdesign commented 1 year ago

While working on #4534 we found this logic

https://github.com/neos/neos-development-collection/blob/033c07ab1d999dc3edca93a8d58259e95514ad13/Neos.Neos/Classes/Service/EditorContentStreamZookeeper.php#L121-L126

As discussed in slack and here we came to the conclusion that this increment logic will never be executed.

editor logs in but there is no user-username workspace. So we load all user-username* workspaces to make sure that we don't create one that already exists.. But we know that user-username does not exist at this point!?

To reassure this i placed a logger at this point and tried a few scenarios. I couldnt prove our thesis wrong but i noticed other oddities that we need to address.

ahaeslich commented 1 year ago

Looking into #4588 and Sebobo/Shel.Neos.WorkspaceModule#33 I also found that creating a new workspace based on the title will lead to workspace names like "My funny workspace-p54t2". Is this expected as this is differes to the pre neos 9 behaviour?

mhsdesign commented 1 year ago

i think the auto suffix only happens with sebs workspace module and not via cli and the legacy module.

bwaidelich commented 11 months ago

As discussed today: Maybe we can move the whole Workspace ownership topic out of the CR to Neos. The simplest way to do so would be a property in the User model that refers to the user's workspace name, but probably it would make sense to directly add a proper relation AccountIdentifier to n owned/accessible workspace names per CR ID.

That way we could make sure to only ever create workspaces that have not been used yet and can still always resolve the owner from the workspace name vice versa. It would also greatly simplify the Neos implementation for our "Privilege Provider" because we can simply match the workspace name with that relation table...

nezaniel commented 5 months ago

Since users may have multiple accounts (e.g. one per authentication provider), personal workspace names should refer to user IDs and not workspace IDs. This would also solve the \Neos\Flow\Security\Account::setAccountIdentifier oddity as user IDs can never change. Or we assume (and thus must also enforce) that all accounts for a user must have the same id

Currently, we always use the security context account's id, except when comparing for accessibility in Neos.UI's WorkspaceService::getAllowedTargetWorkspaces

bwaidelich commented 5 months ago

I agree but turning workspace names into cryptic UUIDs could pose new issues regarding readability and "debuggability". What do you think of this idea:

  1. Move the Workspace model back to Neos (maybe even integrate it to the highlevel Workspace API we introduced with #4943)
  2. Add some relation from Neos user <-> Workspace (with that basis we could even support multiple private workspaces per user in theory)
  3. That "Neos Workspace Service" could implement all the features regarding lookup & access control

With that we could do something like:

// naming tbd of course
$this->someWorkspaceService->getDefaultPrivateWorkspace($neosUserId)->publishAllChanges();

The names of those workspaces could still be "human readable" with that, but more importantly they would be wired to a Neos user with an explicit mechanism

kitsunet commented 5 months ago

Yes please! Clear realation between user and workspace would be great

mhsdesign commented 4 months ago

I also noticed that getAllowedTargetWorkspaces in the Neos ui is partly wrong / dead code:

https://github.com/neos/neos-ui/blob/28fb9c5e8e92a88fe8d79666b454b19c5533cff3/Classes/ContentRepository/Service/WorkspaceService.php#L140

workspaceOwner is an id but not the instance thus it must be:

        $userIdentifier = $this->persistenceManager->getIdentifierByObject($user);
        if ($workspace->workspaceOwner !== $userIdentifier) {
                continue;
            }
bwaidelich commented 4 months ago

This will be addressed with the security issues