Administrator trys to edit a workspace of an other user throws exception

[bego68]

Description

BUG: If I want like Administrator to edit a workspace of another user, system throws exception.

error

Steps to Reproduce

1. add workspace with one user
2. login with different user (Administrator)
3. try to edit workspace

Expected behavior

edit workspace

Actual behavior

Throws exception

Affected Versions

Neos: master (3.0)

[bego68]

It is the same issue, if I try to asign workspace to an other owner.

[kdambekalns]

So… the privilege target is defined in https://github.com/neos/neos-development-collection/blob/3.1/Neos.Neos/Configuration/Policy.yaml#L75 and in theory all is well. It's just that this.workspace.owner returns a string with an UUID, but current.userInformation.backendUser returns a User instance. No match here…

Update: Wrong, there are two User instances involved. Need to investigate further.

[kdambekalns]

New insights… this is generated from the policy (specifically the evaluate(this.workspace.owner !== current.userInformation.backendUser, this.workspace.personalWorkspace === false) part is of interest):

function(\Neos\Flow\Aop\JoinPointInterface $joinPoint, $objectManager) {
    $currentObject = $joinPoint->getProxy();
    $globalObjectNames = $objectManager->getSettingsByPath(array('Neos', 'Flow', 'aop', 'globalObjects'));
    $globalObjects = array_map(function($objectName) use ($objectManager) { return $objectManager->get($objectName); }, $globalObjectNames);
    return (\Neos\Utility\ObjectAccess::getPropertyPath($currentObject, 'workspace.owner') !== \Neos\Utility\ObjectAccess::getPropertyPath($globalObjects['userInformation'], 'backendUser') && \Neos\Utility\ObjectAccess::getPropertyPath($currentObject, 'workspace.personalWorkspace') === false);
},

As can be seen, this from the policy file is resolved as $currentObject - and that is the WorkspacesController. It has no workspace property, nor a getWorkspace() method, so both workspace.owner and workspace.personalWorkspace are null and the condition does not match.

[kdambekalns]

The behaviour of this is, btw, documented correctly in https://flowframework.readthedocs.io/en/stable/TheDefinitiveGuide/PartIII/AspectOrientedProgramming.html#evaluate

[kdambekalns]

There is a "crude" way to fix it… Looking at https://github.com/neos/flow-development-collection/blob/4.1/Neos.Flow/Classes/Aop/Pointcut/PointcutFilterComposite.php#L379-L393 the code from the policy is "adjusted" only if it contains this or current in a specific form. Otherwise it is used unchanged. Unchanged in the sense of "what passed the evaluate expression parser".

Indeed, this snippet here (note the parentheses around the "PHP code" and the use of -> instead of the usual . of policy files):

evaluate(($joinPoint->getMethodArgument("workspace")->getOwner()) !== current.userInformation.backendUser, ($joinPoint->getMethodArgument("workspace")->isPersonalWorkspace()) === false)

comes out as

(($joinPoint->getMethodArgument("workspace")->getOwner()) !== \Neos\Utility\ObjectAccess::getPropertyPath($globalObjects['userInformation'], 'backendUser') && ($joinPoint->getMethodArgument("workspace")->isPersonalWorkspace()) === false)

and works as expected. 🎉

QUESTIONS

• is this a sane way to solve the issue?
• is this even an intentionally supported thing?

Paging @foerthner, @bwaidelich and @kitsunet …

[bwaidelich]

@kdambekalns Just to recap: You suggest to replace the

evaluate(this.workspace.owner !== current.userInformation.backendUser, this.workspace.personalWorkspace === false)

part of the Neos.Neos:Backend.Module.Management.Workspaces.ManageAllPrivateWorkspaces Method Privilege to

evaluate(($joinPoint->getMethodArgument("workspace")->getOwner()) !== current.userInformation.backendUser, ($joinPoint->getMethodArgument("workspace")->isPersonalWorkspace()) === false)

right?

[foerthner]

Not sure if I got it right. The problem is, that we need to access the method argument "workspace.owner" instead of "this.workspace.owner", which would be a property of the Controller that doesn't exist, right? Does ist simply work to pull this condition out of the evaluate() expression, simply using a method argument condition like "workspace.owner !== current.userInformation.backendUser" ? Or don't we have a method argument "workspace" in the adviced method?

[kdambekalns]

@bwaidelich Well, that makes it work. I am not yet sure if I should really suggest that change…

@foerthner The argument workspace exists, hence I can use it in the changed version (by means of getArgument(…).) But I cannot check for both conditions like that, so the evaluate(…) would still be needed anyway for the isPersonalWorkspace() call in some way.

[foerthner]

ok, I see. So method(publishWorkspace|discardWorkspace|edit|update|delete)Action(workspace.owner != current.userInformation.backendUser)) && evaluate(this.workspace.personalWorkspace === false) doesn't work, right?

[foerthner]

Just to answer your question: The workaround way would definitely not be the intended way to write a policy. However, if the documented policy capabilites don't work, we could go for it for the moment. IMHO we then should add a comment to the policy, that this is not really best practice an a workaround for this special case...

[kdambekalns]

@foerthner No, evaluate(this.workspace.personalWorkspace === false) has the same issue, since the WorkspacesController has no workspace property.

I admit I didn't try a combination of both (i.e. method(publishWorkspace|discardWorkspace|edit|update|delete)Action(workspace.owner != current.userInformation.backendUser)) && evaluate(($joinPoint->getMethodArgument("workspace")->isPersonalWorkspace()) === false)), but even that would need the "workaround" for the second part of the check.

[kdambekalns]

So, given that

The workaround way would definitely not be the intended way to write a policy

how would we want to solve that? It'd be awesome to be able to write evaluate(arguments.workspace.personalWorkspace === false). So, we'd have current, this and arguments…

[foerthner]

Hm, I guess the way it should be stated is: method(publishWorkspace|discardWorkspace|edit|update|delete)Action(workspace.owner != current.userInformation.backendUser, workspace.personalWorkspace === false))

Or are the two workspace objects not the same?

[kdambekalns]

They are the same, and yes, that'd be nice as well. It might confuse people because it looks like a second argument to the method, but… well. 🤷‍♂️

[foerthner]

yes, we also could switch to something like method(publishWorkspace|discardWorkspace|edit|update|delete)Action(workspace.owner != current.userInformation.backendUser && workspace.personalWorkspace === false))

[kdambekalns]

I'd keep it consistent, so that evaluate(…) and method(…) use the same syntax. Whether that is && and || or a comma, is secondary. Given we have the comma already, I'd say we keep that.