-
-
Notifications
You must be signed in to change notification settings - Fork 222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
!!! FEATURE: Content Repository Privileges #5298
Conversation
```yaml privilegeTargets: 'Neos\Neos\Security\Authorization\Privilege\SubtreeTagPrivilege': 'Some.Package:FooInAllContentRepositories': matcher: 'foo' 'Some.Package:BarInDefaultContentRepository': matcher: 'default:bar' ```
# Conflicts: # Neos.Neos/Classes/View/FusionExceptionView.php
to `getAuthenticatedUserId()` and make return type nullable
… new `ContentRepositoryAuthorizationService` singleton
and add dedicated `AccessDenied` exception
..instead of the Neos `User`. Reasoning: - A Neos `User` can have multiple accounts assigned - It makes sense to evaluate all authenticated roles e.g. for frontend authentication)
and add doc comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels already a lot better with the WorkspaceService
being split up and the new $roles, $userId
signature in the authorisation service. Now everything is sound :) Left a few last comments and solved the nitpicks myself. After addressing those i think this can be merged finally! Thanks for sticking to this topic so long!!!
I also tested that the Neos ui still works (e2e ran locally) and adjusted it to the latest changes.
One more thing though, maybe @kitsunet wants to have a look at the things that are evaluated often and might need to be cached? See \Neos\Neos\Security\Authorization\ContentRepositoryAuthorizationService::getVisibilityConstraints
? And the write side now has to fetch the node again and its workspace but i guess that is ok though could be cached too at some point ... idk :D
But giving my +1 already because of goodwill :D
/** | ||
* @api | ||
*/ | ||
final class AccessDenied extends \Exception |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need some flow error option or anything so this is treated as flow AccessDeniedException
in the parts that matter? Or does it matter at all? I do know we have some special condition sometimes to let AccessDeniedException
always bubble up or something: (at least in the fusion runtime)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kitsunet and me discussed that this must be caught in controllers and wrapped into a flow AccessDeniedException
to initiate a redirect or set the correct flow status to 403
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am almost done reviewing and inclined to merge this now. we can figure out a fix for it after the fact...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least for typical neos ui use cases this isn't noticable as they all have controller privileges and throw the necessary exception that way already. Maybe we don't need anything for now... The frontend user case is still to be checked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perfect -> followup todo: #5372
throw new \RuntimeException('No user authenticated', 1720371024); | ||
$authenticatedAccount = $this->securityContext->getAccount(); | ||
if ($authenticatedAccount === null) { | ||
throw new AccessDeniedException('No user authenticated', 1720371024); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the new nullability i could actually imagine to just ignore that check here and use $currentUser?->getId()
like in other places ... that makes the code imo less quirky as the security part in flow is defined via yaml and not in the code.
But will do that inside of sebs workspace pr instead of messing with the workspace controller to much :D
Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php
Outdated
Show resolved
Hide resolved
# Conflicts: # Neos.Neos/Classes/Command/CrCommandController.php
# Conflicts: # Neos.ContentRepository.Core/Classes/ContentRepository.php # Neos.ContentRepository.Core/Classes/Factory/ContentRepositoryFactory.php # Neos.ContentRepositoryRegistry/Classes/ContentRepositoryRegistry.php
…vileges-tests # Conflicts: # Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php
# Conflicts: # Neos.ContentRepository.Core/Classes/ContentRepository.php
…vileges-tests # Conflicts: # Neos.ContentRepository.TestSuite/Classes/Behavior/Features/Bootstrap/GenericCommandExecutionAndEventPublication.php
TASK: Tests for "Content Repository Privileges"
Neos.Neos/Tests/Behavior/Features/Bootstrap/ContentRepositorySecurityTrait.php
Outdated
Show resolved
Hide resolved
- we must not write the settings to the file system via $crSecurity_testingPolicyPathAndFilename as following processes and tests in the same context would be affected. Also flushing and reloading the configuration catch (`flushConfigurationCache`) is expensive and the tests must not mess with that low level flow behaviour - instead we only rely on faking the `PolicyService`'s runtime caches. This is done by injecting a custom configuration into `$policyConfiguration` and then initialising the `PolicyService`
Now we throw the cr AccessDenied exception instead of flows access denied in all places of the workspace service to make behaviour consistent. $contentRepository->handle might for example throw the `AccessDenied` when creating a workspace
And introduce the behaviour of the `FunctionalTestCase`: neos/flow-development-collection@b9c89e3
assert that base workspace creation is only allowed for writing and that managing (setting title and roles) is only allowed to managers
{ | ||
public static function becauseCommandIsNotGranted(CommandInterface $command, string $reason): self | ||
{ | ||
return new self(sprintf('Command "%s" was denied: %s', $command::class, $reason), 1729086686); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm this should probably contain the cr id to ease debugging? But we have this missing alotta places in exceptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried various scenarios locally, checked performance, looked over the code. ✅🙌👏🚀
because it led to [debates](neos/neos-development-collection#5298 (comment))
Introduces basic security to the event sourced content repository
This is a breaking change mainly because it requires a new, explicit, workspace role assignment in order for the
live
workspace to be publicly visible:./flow workspace:assignrole live "Neos.Flow:Everybody" viewer
Otherwise youll face this error:
Adjustments to
VisibilityConstraints
:The method
VisibilityConstraints::frontend()
was renamed toVisibilityConstraints::default()
.But for the cases you have used
VisibilityConstraints
at all - as mandatory argument forgetSubgraph
you can now simplify that to usegetContentSubgraph
directly:before
after
Note that
getContentGraph(...)->getSubgraph(...)
still works but is only meant to be used if your you want to determine theVisibilityConstraints
for some case differently.Usage
Workspace Permissions
The following workspace roles exist:
VIEWER
: Can read from the workspaceVIEWER
to thelive
workspaceCOLLABORATOR
: Can read from and write to the workspaceMANAGER
: Can read from and write to the workspace and manage it (i.e. change metadata & role assignments)Furthermore, personal workspaces can be owned by a particular user. The workspace owner implicitly has the
MANAGER
role, i.e. has full access to that workspace.Note
Workspace roles are transitive. That means that
COLLABORATOR
has all privileges of theVIEWER
andMANAGER
has all privileges of theCOLLABORATOR
To publish changes to a workspace, the authenticated user has to have at least the
VIEWER
role on the workspace to publish andCOLLABORATOR
role on the target workspace.Workspace role assignments can be changed through the workspace module (WIP, see #5132) or via CLI:
ReadNodePrivilege
In general, the
ReadNodePrivilege
works like it did before Neos 9: It allows to completely hide sub trees of the content repository from a given user group.But the implementation and configuration format has changed.
This is how a
ReadNodePrivilege
can be configured viaPolicy.yaml
:The
matcher
refers to aSubtreeTag
, so this example would hide all nodes that are tagged withblog
(including their descendants) from all users that don't have aGRANT
on that specific privilege target.Tip
By default, the privilege is active for all configured content repositories. By prefixing the matcher with
<content-repository-id>:
this can be limited to a specific CR (for exampledefault:blog
)EditNodePrivilege
Likewise the purpose of the
EditNodePrivilege
is like before Neos 9: To restrict certain users from changing nodes (includes creation, setting properties or references, disabling/enabling, tagging and moving, ...) in a given subtree.The new configuration syntax is similar to the one for
ReadNodePrivilege
:Again, the
matcher
refers to aSubtreeTag
, so this example would disallow users without a correspondingGRANT
permission to edit nodes in the subtree that is taggedblog
.Note
Both privileges expect a subtree to be tagged. That is not yet possible via the Neos UI or CLI but requires interaction with the PHP API (
$contentRepository->handle(TagSubtree::create(....))
) – we'll provide a way to do that, follow #3732 for updatesRemoved privilege types
The following Content Repository privilege types have been removed because their behavior was inconsistent and/or because they led to (performance) issues:
NodeTreePrivilege
,CreateNodePrivilege
,ReadNodePropertyPrivilege
,EditNodePropertyPrivilege
We will most probably re-add some of the functionality in a future version, but for now you can implement the AuthProviderInterface in order to enforce custom authorization requirements.
Examples
Disallow a role to edit nodes of a subtree
With the following
Policy.yaml
:only administrators are allowed to edit nodes in the
blog
subtree.Note
The Neos UI does not currently properly reflect readonly nodes – The inspector will be empty and inline editable fields will be editable – but changing their content will lead to an AccessDenied exception
In order to also hide the uneditable subtree, the following can be added:
Allow a role to only edit within a given subtree
To achieve the opposite of the above, granting a user group to only edit nodes within a subtree, the whole tree must be tagged (i.e. the homepage node, all descendants will inherit the tag).
With all nodes being tagged
demosite
and the blog nodes beeing taggedblog
in addition, the followingPolicy.yaml
will do:With that, the administrator will be able to edit any node, but editors can only edit nodes within the
blog
subtree.Related: #3732