BUGFIX: Do not create inline editors for nodes without edit permission - #4175
Open
ru3fu5z wants to merge 1 commit into
Open
BUGFIX: Do not create inline editors for nodes without edit permission#4175ru3fu5z wants to merge 1 commit into
ru3fu5z wants to merge 1 commit into
Conversation
Content of nodes that the user has no edit permission for (via an `EditNodePrivilege`) was still initialized with an inline editor. Typing was possible, but persisting the change failed with an `AccessDenied` exception from the Content Repository. The node policy already carries that information, but it was neither respected when creating the inline editors nor available in time: The policies are loaded lazily after the node data arrived, while the inline editors are initialized right after the guest frame merged the node data. They are now loaded together with the node data, so the guest frame can rely on them.
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


Resolves #4171
What I did
Inline editors were created for every inline editable property, regardless of whether the user is allowed to edit the node. Editors could type into content protected by an
EditNodePrivilege, and the change was only rejected once it was persisted — with a rawAccessDeniedexception from the Content Repository. This is the behaviour described in the note of neos/neos-development-collection#5298.The node policy already carries the required information (
policy.canEdit, computed server side byContentRepositoryAuthorizationService::getNodePermissions()), but it was never consulted when creating the inline editors — and it was not even available at that point in time.How I did it
Two changes, both in
neos-ui-guest-frame:initializePropertyDomNode.jsnow respectspolicy.canEdit. The decision moved into a small pure functionisPropertyInlineEditable()so it can be unit tested. A missingcanEditis deliberately treated as "editable", so nothing is locked down if the policy is unavailable.initializeGuestFrame.jsloads the node policies together with the node data (all([call(loadNodeData, …), call(loadNodePolicies, …)])) and merges them in a single store update. Previously the policies were fetched lazily by theCR/Policiessaga after the node data had been merged, while the content DOM nodes are initialized immediately afterwards — andinitializeContentDomNodereads the store at call time. Whether the policy was there was therefore a race.The second part is what makes the first one reliable. Gating on
canEditalone is flaky: measured on the same page over three identical reloads, a gate-only build initialized 1, 5 and 1 editors — i.e. forbidden editors leak in depending on when the policy request returns. With the policies loaded up front the result is deterministic.Because both requests now run concurrently, this costs no extra latency: in the network timeline the guest frame's
flow-queryandget-additional-node-metadatastart simultaneously. The merged policies also keep theCR/Policiessaga from requesting them a second time (its filter skips nodes that already have a policy).No security logic was added on the client side — the decision stays entirely server side, as requested in #4079. The removed
NodePolicyServiceis not revived and no new endpoint is introduced.Regarding the ongoing security concept work: I asked in neos/neos-development-collection#5650 (comment) whether a UX fix against the current privilege API is welcome now; no objection in two weeks, so here it is — happy to rework if the concept lands on a different model.
How to verify it
Reproduction on the Neos demo site (a small command controller is needed to apply the subtree tag, since there is no CLI for it yet — see neos/neos-development-collection#5931):
./flow site:importAll --package-key Neos.DemoTagSubtree::create(...)Policy.yaml— note the site-wide target: for nodes that no privilege target matches at all, Flow defaults to "granted", so without it the restricted role could edit everything and nothing would reproduce. Note alsoLivePublisher: without it the personal workspace is reported read-only and the UI disables editing altogether, which hides the node-level problem behind the workspace-level one.Neos.Demo:BlogEditorand open a document outside the tagged subtreeCommand "…SetNodeProperties" was denied: No edit permissions for node "…"After: the text is not editable; content inside the tagged subtree still is
Measured on the demo site with the setup above, comparing every property against what
get-additional-node-metadatareports for its node:canEdit: falsetrue, 1×false(element outside the blog subtree)And in a real 8.3→9.1 customer migration (74 inline editable properties on one page, editor role scoped to a single tagged element):
Before/after GIFs recorded on the demo site follow in a comment.
Side note, unrelated to this PR: the deny message names the wrong privilege type — it says
No privilege of type "…\ReadNodePrivilege" matchedwhile evaluating an Edit target ("Neos.Demo:EditAllNodes": ABSTAIN). Happy to file that separately.Tests: 7 new Jest cases in
initializePropertyDomNode.spec.js; full suite green (151 suites / 826 tests / 52 snapshots), ESLint clean, production build succeeds.Known limitations (deliberately out of scope)
canEditas a visibility predicate (Inspector/index.js,TabPanel/index.js), so it renders empty instead of read-only for such nodes. That is a separate change and overlaps with the inspector part of Bugfix: add ability for view only users to interact with preview page and view data in inspector #4152 — happy to prepare it once the direction here is clear.policy.disallowedPropertiesis still hardcoded to[]server side (// not implemented for Neos 9.0), so per-property restrictions remain unavailable. That belongs to the security concept in Security / Permissions in Neos 9 neos-development-collection#5650.