Open skurfuerst opened 2 years ago
Some rough ideas that I just discussed with @skurfuerst
Policy.yaml
-approach does not work out for the ESCRWe extend the restriction edges (that are currently used to disable nodes recursively) so that we can have multiple dimensions (!=content dimensions - we need to find a better wording). For example "disabled" and "member_area", ...
The VisibilityConstraints
could be adjusted accordingly, so that you could filter nodes not only based on "frontend" or "backend" but based on that "visibility dimension". Along the lines of:
VisibilityConstraints::forAttributes('disabled', 'member_area');
That will allow to omit nodes based on the attributes¹ the currently authenticated user has access to.
We should probably not use this mechanism to hide nodes from the Neos Backend UI. Instead, there should be a performant way to determine the attributes of a given node (or subtree).
The easiest way would be to extend the Node
model by some Attributes
property (that would include "disabled" and all custom attributes that are defined for this CR).
Lastly we need a way to restrict Commands based on some ACL. But I would consider this easier to do, especially if the ReadModel knows about the "attributes" of the node to deal with, i.e. it could be another constraint check like:
$parentNodeAggregate = $this->requireProjectedNodeAggregate(
$command->contentStreamId,
$command->parentNodeAggregateId,
$contentRepository
);
$this->requireNodeAttributes($parentNodeAggregate, ...);
¹ I used "attributes" here for the "visibility constraints dimension", but maybe we find a better name
@skurfuerst I just moved this to Prioritized
because I found out that you can access the preview of unpublished content without login (via /neos/preview?node=user-<username>__<base64DSP>__<nodeAggregateId>
) and I think that's definitely something we need to fix for a release
Thanks, good catch!
We could extend the VisibilityConstraints
to sth like (naming to be defined):
final class VisibilityConstraints
{
private function __construct(
public readonly AccessibleAttributes $accessibleTags,
public readonly AccessibleContentStreams $accessibleContentStreams,
) {}
// ...
public static function withoutRestrictions(): self
{
return new VisibilityConstraints(AccessibleAttributes::all(), AccessibleContentStreams::all());
}
}
VisibilityConstraints::withoutRestrictions()
is currently used in ~50 places and can probably left as-is in most cases (in some we should re-check whether we really want to ignore all restrictions, e.g. in \Neos\Neos\Ui\ContentRepository\Service\NodeService::getNodeFromContextPath()
).
VisibilityConstraints::frontend()
is used only in ~6 places currently.
They should be replaced with some factory/builder that can return an instance based on the current site/CR and authenticated user:
ContextOperation::evaluate()
$subgraphIdentity = $subgraphIdentity->withVisibilityConstraints(
$invisibleContentShown
? VisibilityConstraints::withoutRestrictions()
: VisibilityConstraints::frontend()
);
=>
$visibilityConstraints = $subgraphIdentity->visibilityConstraints;
if ($invisibleContentShown) {
$visibilityConstraints->withAccessibleAttribute(Attribute::fromString('Neos.Neos:disabledNodes'));
} else {
$visibilityConstraints->withoutAccessibleAttribute(Attribute::fromString('Neos.Neos:disabledNodes'));
}
NodeController::previewAction()
and NodeController::showAction()
$visibilityConstraints = VisibilityConstraints::frontend();
if ($this->privilegeManager->isPrivilegeTargetGranted('Neos.Neos:Backend.GeneralAccess')) {
$visibilityConstraints = VisibilityConstraints::withoutRestrictions();
}
=>
$visibilityConstraints = $this->visibiliyConstraintsFactory->build($siteDetectionResult->contentRepositoryId);
NodeSiteResolvingService::findSiteNodeForNodeAddress()
$nodeAddress->isInLiveWorkspace() ? VisibilityConstraints::frontend() : VisibilityConstraints::withoutRestrictions();
=>
?
LinkingService::createNodeUri()
!$workspace->isPublicWorkspace() ? VisibilityConstraints::withoutRestrictions() : VisibilityConstraints::frontend()
=>
?
FusionExceptionView::render()
$currentSiteNode = $this->siteNodeUtility->findCurrentSiteNode(
$siteDetectionResult->contentRepositoryId,
$contentStreamId,
$dimensionSpacePoint,
VisibilityConstraints::frontend()
);
=>
?
After thinking about this again, I think we should rename VisibilityConstraints to something along the lines of "ViewingMode" and then have:
VisibilityConstraints::frontend()
=> ViewingMode::default()
VisibilityContrains:: withoutRestrictions()
=> ViewingMode::fullAccess()
And then something like: ViewingMode::custom('Some.Namespace:SomeMode')
or ViewingMode::forTags($tags)
depending on whether we want to calculate the "tags" (aka attributes) from some pre-configured preset or specify them directly in the PHP API?!
The PrivilegeProvider
could have an interface like:
interface PrivilegeProviderInterface {
function getWritePrivileges(): WritePrivileges;
function getReadPrivileges(ViewingMode $mode): ReadPrivileges;
}
WritePrivileges
could, amongst others have a method like isCommandAllowed(Command $command): bool
.
ReadPrivileges
could provide details about accessible ContenStreams and tags.
I like the concept :) I feel ViewingMode is still a bit "German" but I do not have a better idea yet. Is ViewMode better?
I am still unsure whether ViewingMode is so much better than VisibilityConstraints, so that it justifies a backwards compatibility break? Because we will make s really hard time to our users if we start renaming such core classes again and again - imho this time should be over now, as more and more people will start building on our subgraph API...)
Have a nice holiday @bwaidelich ❤️ Sebastian
@skurfuerst you are right.. I was still stuck on the thought that we need something on top of visibility constraints for things like "Document Tree Context", but we can probably just "reuse" them for this.
We still have to think of some kind of migration path for < Neos 9 privileges.
Also, what would be a corresponding feature to "protect all nodes underneath X" or "protect all nodes of type Y"?
Will there be a declarative way (i.e. should we add s.th. like "defaultTags" to the NodeType schema) or will this require some AddTagToNodeAggregate
command?
In the export format it will occur just like NodeAggregateWasDisabled
events – in fact, the latter should probably be replaced by some TagWasAddedToNodeAggregate
event (We could keep the Disable*
command for convenience.
I also wonder whether the current behavior of the restriction edges might lead to confusion. E.g.: You could tag a node and that tag would be "inherited" to all descendants. But you could additionally tag descendants with the same tag.. Maybe just a matter of good documentation.
so should we actually revert this whole wip Neos.ContentRepository.Security
package?
https://github.com/neos/neos-development-collection/commit/2ce2b4fc690686755eae955e82858cc271239113
Some updates after talking to @mhsdesign , @skurfuerst and @nezaniel :
Node
read model by a tags
property. But for this to work performantly we'll have to add the tags to the edges (aka "hierarchy relation")
SubgraphInterface::getTagsForNodeIds(...)
)isCommandAllowed(Command $command): bool
ContentRepository::handle()
method would invoke each and fail on the first false
ContentRepository::isCommandAllowed(): bool
that we could use to adjust UI based on allowed commands – Features from the NodePolicyService could be re-implemented, be instantiating the corresponding commands for each. If that turns out to be slower than the current privilege target approach, we can create dedicated projections or (my preference) postpone the calculations until they are actually needed (e.g. disallowedNodeTypes
when the create node dialog is displayed) via AJAX
Neos.ContentRepository.Security
package https://github.com/neos/neos-development-collection/pull/4544