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

WIP: FEATURE: Content Repository Privileges #4185

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
use Neos\ContentRepository\Core\DimensionSpace\OriginDimensionSpacePoint;
use Neos\ContentRepository\Core\NodeType\NodeTypeManager;
use Neos\ContentRepository\Core\NodeType\NodeTypeName;
use Neos\ContentRepository\Core\SharedModel\Privilege\PrivilegeProviderInterface;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamId;
use Neos\EventStore\CatchUp\CatchUp;
use Neos\EventStore\DoctrineAdapter\DoctrineCheckpointStorage;
Expand Down Expand Up @@ -95,6 +96,7 @@ public function __construct(
private readonly ProjectionContentGraph $projectionContentGraph,
private readonly CatchUpHookFactoryInterface $catchUpHookFactory,
private readonly string $tableNamePrefix,
private readonly PrivilegeProviderInterface $privilegeProvider,
) {
$this->checkpointStorage = new DoctrineCheckpointStorage(
$this->dbalClient->getConnection(),
Expand Down Expand Up @@ -261,7 +263,8 @@ public function getState(): ContentGraph
$this->dbalClient,
$this->nodeFactory,
$this->nodeTypeManager,
$this->tableNamePrefix
$this->tableNamePrefix,
$this->privilegeProvider,
);
}
return $this->contentGraph;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ public function build(
$tableNamePrefix
),
$catchUpHookFactory,
$tableNamePrefix
$tableNamePrefix,
$projectionFactoryDependencies->privilegeProvider,
)
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
use Neos\ContentRepository\Core\Projection\ContentGraph\ContentGraphInterface;
use Neos\ContentRepository\Core\Projection\ContentGraph\ContentSubgraphInterface;
use Neos\ContentRepository\Core\Projection\ContentGraph\NodeAggregate;
use Neos\ContentRepository\Core\SharedModel\Privilege\PrivilegeProviderInterface;
use Neos\ContentRepository\Core\SharedModel\User\UserId;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamId;
use Neos\ContentRepository\Core\DimensionSpace\DimensionSpacePoint;
use Neos\ContentRepository\Core\SharedModel\Node\NodeAggregateId;
Expand All @@ -56,7 +58,8 @@ public function __construct(
private readonly DbalClientInterface $client,
private readonly NodeFactory $nodeFactory,
private readonly NodeTypeManager $nodeTypeManager,
private readonly string $tableNamePrefix
private readonly string $tableNamePrefix,
private readonly PrivilegeProviderInterface $privilegeProvider,
) {
}

Expand All @@ -67,6 +70,10 @@ final public function getSubgraph(
): ContentSubgraphInterface {
$index = $contentStreamId->value . '-' . $dimensionSpacePoint->hash . '-' . $visibilityConstraints->getHash();
if (!isset($this->subgraphs[$index])) {
$privileges = $this->privilegeProvider->getPrivileges($visibilityConstraints);
if (!$privileges->isContentStreamAllowed($contentStreamId)) {
throw new \RuntimeException(sprintf('No access to content stream "%s"', $contentStreamId->value), 1681306937);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

does it make sense to check the privileges here already?

Copy link
Member

Choose a reason for hiding this comment

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

I would say yes (if we can, like it is done here). Ofc people could work around this if they manually instanciated ContentSubgraph; but we could never prevent this.

Copy link
Member Author

@bwaidelich bwaidelich Oct 24, 2023

Choose a reason for hiding this comment

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

rename to sth like $this->privilegeProvider->isFetchingOfNodesAllowedInSubgraph($contentStreamId, $dimensionSpacePoint) (consider using result object for more context)

$this->subgraphs[$index] = new ContentSubgraphWithRuntimeCaches(
new ContentSubgraph(
$contentStreamId,
Expand All @@ -75,7 +82,8 @@ final public function getSubgraph(
$this->client,
$this->nodeFactory,
$this->nodeTypeManager,
$this->tableNamePrefix
$this->tableNamePrefix,
$this->privilegeProvider,
)
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
use Neos\ContentRepository\Core\SharedModel\Node\NodeAggregateId;
use Neos\ContentRepository\Core\SharedModel\Node\NodeName;
use Neos\ContentRepository\Core\SharedModel\Node\PropertyName;
use Neos\ContentRepository\Core\SharedModel\Privilege\PrivilegeProviderInterface;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamId;

/**
Expand Down Expand Up @@ -102,7 +103,8 @@ public function __construct(
private readonly DbalClientInterface $client,
private readonly NodeFactory $nodeFactory,
private readonly NodeTypeManager $nodeTypeManager,
private readonly string $tableNamePrefix
private readonly string $tableNamePrefix,
private readonly PrivilegeProviderInterface $privilegeProvider,
) {
}

Expand Down
2 changes: 2 additions & 0 deletions Neos.ContentRepository.Core/Classes/ContentRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
use Neos\ContentRepository\Core\Projection\Projections;
use Neos\ContentRepository\Core\Projection\ProjectionStateInterface;
use Neos\ContentRepository\Core\Projection\Workspace\WorkspaceFinder;
use Neos\ContentRepository\Core\SharedModel\Privilege\PrivilegeProviderInterface;
use Neos\ContentRepository\Core\SharedModel\User\StaticUserIdProvider;
use Neos\ContentRepository\Core\SharedModel\User\UserId;
use Neos\ContentRepository\Core\SharedModel\User\UserIdProviderInterface;
Expand Down Expand Up @@ -67,6 +68,7 @@ public function __construct(
private readonly InterDimensionalVariationGraph $variationGraph,
private readonly ContentDimensionSourceInterface $contentDimensionSource,
private readonly UserIdProviderInterface $userIdProvider,
private readonly PrivilegeProviderInterface $privilegeProvider,
private readonly ClockInterface $clock,
) {
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
use Neos\ContentRepository\Core\NodeType\NodeTypeManager;
use Neos\ContentRepository\Core\Projection\ProjectionCatchUpTriggerInterface;
use Neos\ContentRepository\Core\Projection\Projections;
use Neos\ContentRepository\Core\SharedModel\Privilege\PrivilegeProviderInterface;
use Neos\ContentRepository\Core\SharedModel\User\UserIdProviderInterface;
use Neos\EventStore\EventStoreInterface;
use Psr\Clock\ClockInterface;
Expand All @@ -54,6 +55,7 @@ public function __construct(
ProjectionsFactory $projectionsFactory,
private readonly ProjectionCatchUpTriggerInterface $projectionCatchUpTrigger,
private readonly UserIdProviderInterface $userIdProvider,
private readonly PrivilegeProviderInterface $privilegeProvider,
private readonly ClockInterface $clock,
) {
$contentDimensionZookeeper = new ContentDimensionZookeeper($contentDimensionSource);
Expand All @@ -70,7 +72,8 @@ public function __construct(
$contentDimensionSource,
$contentDimensionZookeeper,
$interDimensionalVariationGraph,
new PropertyConverter($propertySerializer)
new PropertyConverter($propertySerializer),
$this->privilegeProvider,
);

$this->projections = $projectionsFactory->build($this->projectionFactoryDependencies);
Expand Down Expand Up @@ -99,6 +102,7 @@ public function build(): ContentRepository
$this->projectionFactoryDependencies->interDimensionalVariationGraph,
$this->projectionFactoryDependencies->contentDimensionSource,
$this->userIdProvider,
$this->privilegeProvider,
$this->clock,
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
use Neos\ContentRepository\Core\Infrastructure\Property\PropertyConverter;
use Neos\ContentRepository\Core\NodeType\NodeTypeManager;
use Neos\ContentRepository\Core\Factory\ContentRepositoryId;
use Neos\ContentRepository\Core\SharedModel\Privilege\PrivilegeProviderInterface;
use Neos\EventStore\EventStoreInterface;

/**
Expand All @@ -37,6 +38,7 @@ public function __construct(
public readonly ContentDimensionZookeeper $contentDimensionZookeeper,
public readonly InterDimensionalVariationGraph $interDimensionalVariationGraph,
public readonly PropertyConverter $propertyConverter,
public readonly PrivilegeProviderInterface $privilegeProvider,
) {
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?php

declare(strict_types=1);

namespace Neos\ContentRepository\Core\SharedModel\Privilege;

use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamIds;

/**
* @internal except for custom PrivilegeProviderInterface implementations
*/
final class ContentStreamPrivilege
{
private function __construct(
public readonly ?ContentStreamIds $allowedContentStreamIds,
public readonly ?ContentStreamIds $disallowedContentStreamIds,
Copy link
Member

Choose a reason for hiding this comment

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

what is the semantics of having both of these lists? :) I don't fully understand this yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Me neither :)
But I thought, that might need both. an allow list and a deny list..

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at this after a break, I think the terms "allowed" and "disallowed" are not helpful.
i.e. what does $privileges->isContentStreamAllowed($contentStreamId) exactly mean?

Also we should re-evaluate whether we really need allow- and deny list

) {
}

public static function create(): self
{
return new self(null, null);
}

public function with(
ContentStreamIds $allowedContentStreamIds = null,
ContentStreamIds $disallowedContentStreamIds = null,
): self
{
return new self(
$allowedContentStreamIds ?? $this->allowedContentStreamIds,
$disallowedContentStreamIds ?? $this->disallowedContentStreamIds,
);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?php

declare(strict_types=1);

namespace Neos\ContentRepository\Core\SharedModel\Privilege;

use Neos\ContentRepository\Core\Projection\ContentGraph\VisibilityConstraints;

/**
* @internal except for CR factory implementations
*/
interface PrivilegeProviderInterface
{
public function getPrivileges(VisibilityConstraints $visibilityConstraints): Privileges;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<?php

declare(strict_types=1);

namespace Neos\ContentRepository\Core\SharedModel\Privilege;

use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamId;

/**
* @api
*/
final class Privileges
Copy link
Member Author

Choose a reason for hiding this comment

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

This is missing the "restriction edge attributes" still

Copy link
Member Author

Choose a reason for hiding this comment

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

(see #4557 -> but they will be called node tags as discussed)

Copy link
Member Author

Choose a reason for hiding this comment

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

Rename to sth like ReadPrivileges

{
private function __construct(
public readonly ?ContentStreamPrivilege $contentStreamPrivilege,

) {
}

public static function create(): self
{
return new self(null);
}

public function with(
ContentStreamPrivilege $contentStreamPrivilege = null,
): self
{
return new self(
$contentStreamPrivilege ?? $this->contentStreamPrivilege,
);
}

public function isContentStreamAllowed(ContentStreamId $contentStreamId): bool
{
if ($this->contentStreamPrivilege === null) {
return true;
}
return $this->contentStreamPrivilege->allowedContentStreamIds->contain($contentStreamId);
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I am not so sure about this one - IMHO this must be done lazily (otherwise we could under some scenarios have REALLY long lists of contentStreamIds; where only a single one will be relevant here)

Copy link
Member Author

Choose a reason for hiding this comment

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

isn't it just the currently active Content Streams of the user workspace (and its base workspace(s))?

Copy link
Member Author

Choose a reason for hiding this comment

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

rename to sth. that makes clearer that this is about filtering nodes

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
<?php

/*
* This file is part of the Neos.ContentRepository package.
*
* (c) Contributors of the Neos Project - www.neos.io
*
* This package is Open Source Software. For the full copyright and license
* information, please view the LICENSE file which was distributed with this
* source code.
*/

declare(strict_types=1);

namespace Neos\ContentRepository\Core\SharedModel\Workspace;


/**
* @api
* @implements \IteratorAggregate<ContentStreamId>
*/
final class ContentStreamIds implements \IteratorAggregate
{
/**
* @param ContentStreamId[] $contentStreamIds
*/
private function __construct(
private readonly array $contentStreamIds,
) {
if ($this->contentStreamIds === []) {
throw new \InvalidArgumentException('ContentStreamIds must not be empty', 1681306355);
}
}

public static function fromContentStreamIds(ContentStreamId ...$contentStreamIds): self
{
return new self($contentStreamIds);
}

public function getIterator(): \Traversable
{
return new \ArrayIterator($this->contentStreamIds);
}

public function contain(ContentStreamId $contentStreamId): bool
{
foreach ($this->contentStreamIds as $id) {
if ($id->equals($contentStreamId)) {
return true;
}
}
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,14 @@
use Neos\ContentRepository\Core\Projection\ProjectionCatchUpTriggerInterface;
use Neos\ContentRepository\Core\Projection\ProjectionFactoryInterface;
use Neos\ContentRepository\Core\NodeType\NodeTypeManager;
use Neos\ContentRepository\Core\SharedModel\Privilege\PrivilegeProviderInterface;
use Neos\ContentRepositoryRegistry\Exception\ContentRepositoryNotFound;
use Neos\ContentRepositoryRegistry\Exception\InvalidConfigurationException;
use Neos\ContentRepositoryRegistry\Factory\Clock\ClockFactoryInterface;
use Neos\ContentRepositoryRegistry\Factory\ContentDimensionSource\ContentDimensionSourceFactoryInterface;
use Neos\ContentRepositoryRegistry\Factory\EventStore\EventStoreFactoryInterface;
use Neos\ContentRepositoryRegistry\Factory\NodeTypeManager\NodeTypeManagerFactoryInterface;
use Neos\ContentRepositoryRegistry\Factory\PrivilegeProvider\PrivilegeProviderFactoryInterface;
use Neos\ContentRepositoryRegistry\Factory\ProjectionCatchUpTrigger\ProjectionCatchUpTriggerFactoryInterface;
use Neos\ContentRepository\Core\Factory\ContentRepositoryId;
use Neos\ContentRepositoryRegistry\Factory\UserIdProvider\UserIdProviderFactoryInterface;
Expand Down Expand Up @@ -138,6 +140,7 @@ private function buildFactory(ContentRepositoryId $contentRepositoryId): Content
}
try {
$clock = $this->buildClock($contentRepositoryId, $contentRepositorySettings);
$userIdProvider = $this->buildUserIdProvider($contentRepositoryId, $contentRepositorySettings);
return new ContentRepositoryFactory(
$contentRepositoryId,
$this->buildEventStore($contentRepositoryId, $contentRepositorySettings, $clock),
Expand All @@ -146,7 +149,8 @@ private function buildFactory(ContentRepositoryId $contentRepositoryId): Content
$this->buildPropertySerializer($contentRepositoryId, $contentRepositorySettings),
$this->buildProjectionsFactory($contentRepositoryId, $contentRepositorySettings),
$this->buildProjectionCatchUpTrigger($contentRepositoryId, $contentRepositorySettings),
$this->buildUserIdProvider($contentRepositoryId, $contentRepositorySettings),
$userIdProvider,
$this->buildPrivilegeProvider($contentRepositoryId, $contentRepositorySettings, $userIdProvider, $this),
$clock,
);
} catch (\Exception $exception) {
Expand Down Expand Up @@ -248,6 +252,16 @@ private function buildUserIdProvider(ContentRepositoryId $contentRepositoryId, a
return $userIdProviderFactory->build($contentRepositoryId, $contentRepositorySettings['userIdProvider']['options'] ?? []);
}

private function buildPrivilegeProvider(ContentRepositoryId $contentRepositoryId, array $contentRepositorySettings, UserIdProviderInterface $userIdProvider, self $contentRepositoryRegistry): PrivilegeProviderInterface
{
isset($contentRepositorySettings['privilegeProvider']['factoryObjectName']) || throw InvalidConfigurationException::fromMessage('Content repository "%s" does not have privilegeProvider.factoryObjectName configured.', $contentRepositoryId->value);
$privilegeProviderFactory = $this->objectManager->get($contentRepositorySettings['privilegeProvider']['factoryObjectName']);
if (!$privilegeProviderFactory instanceof PrivilegeProviderFactoryInterface) {
throw InvalidConfigurationException::fromMessage('privilegeProvider.factoryObjectName for content repository "%s" is not an instance of %s but %s.', $contentRepositoryId->value, PrivilegeProviderFactoryInterface::class, get_debug_type($privilegeProviderFactory));
}
return $privilegeProviderFactory->build($contentRepositoryId, $contentRepositorySettings['userIdProvider']['options'] ?? [], $userIdProvider, $contentRepositoryRegistry);
Copy link
Member Author

Choose a reason for hiding this comment

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

The PrivilegeProvider currently has a dependency to...

  • the UserIdProvider to load policies of the authenticated user
  • the ContentRepositoryRegistry to retrieve ContentStreams for the user (via WorkspaceFinder)..

This is nasty

Copy link
Member

Choose a reason for hiding this comment

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

UserIdProvider is fine IMHO, ContentRepositoryRegistry should go away. What about making the $contentRepository available to the provider at runtime? would IMHO be easier and more consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

@skurfuerst can you clarify what you mean by "available [...] at runtime"? A parameter of the PrivilegeProvider::getPrivileges() method? But than the ContentGraph would need to pass it

Copy link
Member

Choose a reason for hiding this comment

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

yep, that's what I thought - similar to what we did in the command handlers. But you are right, the projection does not know it yet...

}

private function buildClock(ContentRepositoryId $contentRepositoryIdentifier, array $contentRepositorySettings): ClockInterface
{
isset($contentRepositorySettings['clock']['factoryObjectName']) || throw InvalidConfigurationException::fromMessage('Content repository "%s" does not have clock.factoryObjectName configured.', $contentRepositoryIdentifier->value);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?php
declare(strict_types=1);
namespace Neos\ContentRepositoryRegistry\Factory\PrivilegeProvider;

use Neos\ContentRepository\Core\Factory\ContentRepositoryId;
use Neos\ContentRepository\Core\Projection\ContentGraph\VisibilityConstraints;
use Neos\ContentRepository\Core\SharedModel\Privilege\ContentStreamPrivilege;
use Neos\ContentRepository\Core\SharedModel\Privilege\PrivilegeProviderInterface;
use Neos\ContentRepository\Core\SharedModel\Privilege\Privileges;
use Neos\ContentRepository\Core\SharedModel\User\UserIdProviderInterface;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamIds;
use Neos\ContentRepositoryRegistry\ContentRepositoryRegistry;

/**
* @internal
*/
final class FakePrivilegeProvider implements PrivilegeProviderInterface
Copy link
Member Author

Choose a reason for hiding this comment

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

Not really a FakePrivilegeProvider but a first implementation to test content stream access

{
public function __construct(
private readonly UserIdProviderInterface $userIdProvider,
private readonly ContentRepositoryRegistry $contentRepositoryRegistry,
private readonly ContentRepositoryId $contentRepositoryId,
) {}

public function getPrivileges(VisibilityConstraints $visibilityConstraints): Privileges
{
$userId = $this->userIdProvider->getUserId();
$contentRepository = $this->contentRepositoryRegistry->get($this->contentRepositoryId);

$privileges = Privileges::create();

$userWorkspace = $contentRepository->getWorkspaceFinder()->findOneByWorkspaceOwner($userId->value);
Copy link
Member Author

Choose a reason for hiding this comment

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

This won't necessarily return the actual user workspace, but any shared workspace with the same owner.
Also this won't include base workspaces currently

Copy link
Member

Choose a reason for hiding this comment

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

dangerous code - people without user workspace will automatically be admins. let's discuss :)

Copy link
Member Author

Choose a reason for hiding this comment

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

hehe, good catch. That can't stay like this ofc

if ($userWorkspace === null) {
return $privileges;
}
return $privileges->with(
contentStreamPrivilege: ContentStreamPrivilege::create()->with(allowedContentStreamIds: ContentStreamIds::fromContentStreamIds($userWorkspace->currentContentStreamId))
);
}
}
Loading