Skip to content
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

Open
wants to merge 76 commits into
base: 9.0
Choose a base branch
from

Conversation

bwaidelich
Copy link
Member

@bwaidelich bwaidelich commented Oct 17, 2024

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:

Read access denied for workspace "live": User is no Neos Administrator and has no explicit role for workspace "live"

Adjustments to VisibilityConstraints:

The method VisibilityConstraints::frontend() was renamed to VisibilityConstraints::default().
But for the cases you have used VisibilityConstraints at all - as mandatory argument for getSubgraph you can now simplify that to use getContentSubgraph directly:

before

$subgraph = $contentRepository->getContentGraph($nodeAddress->workspaceName)->getSubgraph(
    $nodeAddress->dimensionSpacePoint,
    $nodeAddress->workspaceName->isLive() ? VisibilityConstraints::frontend() : VisibilityConstraints::withoutRestrictions()
);

after

$subgraph = $contentRepository->getContentSubgraph($nodeAddress->workspaceName, $nodeAddress->dimensionSpacePoint);

Note that getContentGraph(...)->getSubgraph(...) still works but is only meant to be used if your you want to determine the VisibilityConstraints for some case differently.

Usage

Workspace Permissions

The following workspace roles exist:

  • VIEWER: Can read from the workspace
    • By default, everybody is a VIEWER to the live workspace
  • COLLABORATOR: Can read from and write to the workspace
  • MANAGER: 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 the VIEWER and MANAGER has all privileges of the COLLABORATOR

To publish changes to a workspace, the authenticated user has to have at least the VIEWER role on the workspace to publish and COLLABORATOR role on the target workspace.

Workspace role assignments can be changed through the workspace module (WIP, see #5132) or via CLI:

./flow workspace:assignrole

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 via Policy.yaml:

privilegeTargets:
  'Neos\Neos\Security\Authorization\Privilege\ReadNodePrivilege':
    'Neos.Demo:ReadBlog':
      matcher: 'blog'

The matcher refers to a SubtreeTag, so this example would hide all nodes that are tagged with blog (including their descendants) from all users that don't have a GRANT 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 example default: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:

privilegeTargets:
  'Neos\Neos\Security\Authorization\Privilege\EditNodePrivilege':
    'Neos.Demo:EditBlogNodes':
      matcher: 'blog'

Again, the matcher refers to a SubtreeTag, so this example would disallow users without a corresponding GRANT permission to edit nodes in the subtree that is tagged blog.

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 updates

Removed 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:

privilegeTargets:
  'Neos\Neos\Security\Authorization\Privilege\EditNodePrivilege':
    'Neos.Demo:EditBlogNodes':
      matcher: 'blog'
roles:
  'Neos.Neos:Administrator':
    privileges:
      -
        privilegeTarget: 'Neos.Demo:EditBlogNodes'
        permission: GRANT

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:

privilegeTargets:
  # ...
  'Neos\Neos\Security\Authorization\Privilege\ReadNodePrivilege':
    'Neos.Demo:ReadBlogNodes':
      matcher: 'blog'
roles:
  'Neos.Neos:Administrator':
    privileges:
      # ...
      -
        privilegeTarget: 'Neos.Demo:ReadBlogNodes'
        permission: GRANT

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 tagged blog in addition, the following Policy.yaml will do:

privilegeTargets:
  'Neos\Neos\Security\Authorization\Privilege\EditNodePrivilege':
    'Neos.Demo:EditAnyNode':
      matcher: 'demosite'
    'Neos.Demo:EditBlogNodes':
      matcher: 'blog'
roles:
  'Neos.Neos:Administrator':
    privileges:
      -
        privilegeTarget: 'Neos.Demo:EditAnyNode'
        permission: GRANT
  'Neos.Neos:Editor':
    privileges:
      -
        privilegeTarget: 'Neos.Demo:EditBlogNodes'
        permission: GRANT

With that, the administrator will be able to edit any node, but editors can only edit nodes within the blog subtree.

Related: #3732

```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
@bwaidelich bwaidelich marked this pull request as ready for review November 10, 2024 13:21
@bwaidelich bwaidelich changed the title FEATURE: Content Repository Privileges !!! FEATURE: Content Repository Privileges Nov 10, 2024
bwaidelich and others added 10 commits November 11, 2024 17:11
…vileges-tests

# Conflicts:
#	Neos.Neos/Tests/Behavior/Features/Bootstrap/WorkspaceServiceTrait.php
…epository`

By moving the db implementation logic in a separate class, the dependency chain is cleaned up, as the `WorkspaceService` would previously hold the cr which would hold the `ContentRepositoryAuthorizationService` which has the `WorkspaceService`

Also we didnt want to add further zickzack and previously it would not have been able to acquire the `ContentRepositoryAuthorizationService` without hacks in the `WorkspaceService` to evaluate permissions for `setWorkspaceTitle` or `assignWorkspaceRole`
…al by not exposing on the `WorkspaceService`

They were introduce in #5306 for the pruning, but this is a purely internal task and there is no need for this to be api for the Neos User.
Also, the reason is that it cannot be protected with security easily.
Copy link
Member

@mhsdesign mhsdesign left a 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

$this->userService->getCurrentUser()?->getId()
);
if (!$workspacePermissions->manage) {
throw new AccessDeniedException(sprintf('The current user does not have manage permissions for workspace "%s" in content repository "%s"', $workspaceName->value, $contentRepositoryId->value), 1731343473);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we throw the flow access denied here or the CR's \Neos\ContentRepository\Core\Feature\Security\Exception\AccessDenied like in the other methods? Probably the latter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the more i think about it, yes, otherwise some methods would throw the one and others another :D

Comment on lines 216 to +218
public function getWorkspaceRoleAssignments(ContentRepositoryId $contentRepositoryId, WorkspaceName $workspaceName): WorkspaceRoleAssignments
{
$table = self::TABLE_NAME_WORKSPACE_ROLE;
$query = <<<SQL
SELECT
*
FROM
{$table}
WHERE
content_repository_id = :contentRepositoryId
AND workspace_name = :workspaceName
SQL;
try {
$rows = $this->dbal->fetchAllAssociative($query, [
'contentRepositoryId' => $contentRepositoryId->value,
'workspaceName' => $workspaceName->value,
]);
} catch (DbalException $e) {
throw new \RuntimeException(sprintf('Failed to fetch workspace role assignments for workspace "%s" (Content Repository "%s"): %s', $workspaceName->value, $contentRepositoryId->value, $e->getMessage()), 1728474440, $e);
}
return WorkspaceRoleAssignments::fromArray(
array_map(static fn (array $row) => WorkspaceRoleAssignment::create(
WorkspaceRoleSubjectType::from($row['subject_type']),
WorkspaceRoleSubject::fromString($row['subject']),
WorkspaceRole::from($row['role']),
), $rows)
);
}

/**
* Determines the permission the given user has for the specified workspace {@see WorkspacePermissions}
*/
public function getWorkspacePermissionsForUser(ContentRepositoryId $contentRepositoryId, WorkspaceName $workspaceName, User $user): WorkspacePermissions
{
try {
$userRoles = array_keys($this->userService->getAllRoles($user));
} 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);
}
$workspaceMetadata = $this->loadWorkspaceMetadata($contentRepositoryId, $workspaceName);
if ($workspaceMetadata !== null && $workspaceMetadata->ownerUserId !== null && $workspaceMetadata->ownerUserId->equals($user->getId())) {
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::create(
read: $userWorkspaceRole->isAtLeast(WorkspaceRole::COLLABORATOR),
write: $userWorkspaceRole->isAtLeast(WorkspaceRole::COLLABORATOR),
manage: $userIsAdministrator || $userWorkspaceRole->isAtLeast(WorkspaceRole::MANAGER),
);
return $this->metadataAndRoleRepository->getWorkspaceRoleAssignments($contentRepositoryId, $workspaceName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now that we can make things @internal by not exposing it here on the workspace service we can consider if getWorkspaceRoleAssignments should really be API or if Neos can just use at this one place the metadataAndRoleRepository directly? ... But probably keep it api :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why am i thinking about this? Because the

NOTE: This should never be used to evaluate permissions, instead {@see ContentRepositoryAuthorizationService::getWorkspacePermissions()} should be used!

Makes it already seem like an internal thing somehow :D

/**
* @api
*/
final class AccessDenied extends \Exception
Copy link
Member

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)?

throw new \RuntimeException('No user authenticated', 1720371024);
$authenticatedAccount = $this->securityContext->getAccount();
if ($authenticatedAccount === null) {
throw new AccessDeniedException('No user authenticated', 1720371024);
Copy link
Member

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

WorkspaceName::fromString($workspaceName),
WorkspaceTitle::fromString($newTitle),
));
$this->getObject(SecurityContext::class)->withoutAuthorizationChecks(fn () =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i didnt add tests yet for these things that they work with security will try to do that in your followup pr :) For now its just withoutAuthorizationChecks :D

bwaidelich and others added 11 commits November 12, 2024 12:22
# 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"
Comment on lines +164 to +168
self::$crSecurity_testingPolicyPathAndFilename = $this->getObject(Environment::class)->getPathToTemporaryDirectory() . 'Policy.yaml';
file_put_contents(self::$crSecurity_testingPolicyPathAndFilename, Yaml::dump($mergedPolicyConfiguration));

ObjectAccess::setProperty($policyService, 'initialized', false, true);
$this->getObject(ConfigurationManager::class)->flushConfigurationCache();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this latter part really needed? As we dont have sub processes we dont have to preserve the state and mess with the file system right?

... but ill look into it :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without that I got more failing tests because we have so many runtime caches on all layers

... but ill look into it :)

Great, thank you. And let me know if you need support (contentually or mentally *g)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants