Skip to content

An Editor can now delete the satellite it was allowed to publish — one rule, one answer (#2913) - #2945

Merged
rbuergi merged 2 commits into
mainfrom
fix/2913-delete-nodetype-access-rule
Sep 1, 2026
Merged

rbuergi merged 2 commits into
mainfrom
fix/2913-delete-nodetype-access-rule

Conversation

@rbuergi

@rbuergi rbuergi commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Closes #2913

The defect: one question, two answers, both on the same request

Role.Editor holds Update and not Delete. SatelliteAccessRule maps a satellite's Delete to Permission.Update on its MainNode — creating a satellite is a modification of its main node, and so is removing it. So an Editor may publish a satellite. But it could not erase one:

  • RlsNodeValidator routes Read / Create / Update / Delete through the node type's registered INodeTypeAccessRule.
  • HandleDeleteNodeRequest's pre-flight — MeshExtensions.CheckDeletePermissionForNode, the first gate a DeleteNodeRequest meets on the off-router node-operation hub where IMeshService.DeleteNode lands — demanded Permission.Delete on MainNode ?? Path outright and never looked at a rule.

Both are correct in isolation. The effective policy was their conjunction, so wherever a rule was more permissive than Permission.Delete the rule silently did not apply. "I can turn it on but not off" is the exact state a revocable-consent feature exists to prevent, and it was reachable by an ordinary Editor. It surfaced only when someone ran the writer (session presence, MeshWeaver.Plugins#1031) — never by reading either file.

The fix is structural, not a second copy of the lookup

NodeTypeAccessRuleSet (src/MeshWeaver.Mesh.Contract/Services/NodeTypeAccessRuleSet.cs) is now the one index — a mesh-scoped singleton registered in MeshBuilder, holding the selection semantics in one place (keyed by node type, case-insensitive, last registration wins, SupportedOperations honoured). Both RlsNodeValidator and the delete pre-flight resolve their rule from it, so the two paths cannot drift apart again. Two code paths answering one question was the actual defect; fixing only the symptom would have left it live.

What widened — precisely — and what did not

Case Before After
Type with a Delete-supporting rule that allows rule ∧ Permission.Delete the rule — this is the fix
Type with no rule Permission.Delete Permission.Delete — byte-for-byte, closed by default
Rule that denies refused refused (now at the pre-flight, so Unauthorized rather than ValidationFailed)
Rule that faults / reaches no verdict n/a refused as Unavailable — fail-closed
WellKnownUsers.System allowed via the fold's Permission.All allowed before any rule is consulted (unchanged outcome)

The only types that gained anything are those whose rule chose a lighter demand: the SatelliteAccessRule family — Comment, TrackedChange, Kernel, Activity, UserActivity, ActivityLogSegment, and in MeshWeaver.Plugins Thread, ThreadMessage, TokenUsage, Portal.

🚨 Space and Partition were the trap. Their rules also said Update for Delete — unreachable, because the pre-flight's own Permission.Delete demand sat in front of it. Making the rule authoritative would have let anyone with Update on a Space delete the Space and drop its backing store. Both rules now state Permission.Delete for Delete explicitly, which is exactly the policy that was already enforced. A disagreement between two gates resolves to the STRICTER of the two unless there is a stated reason for the looser one — and for satellites the issue states it.

Fail-closed, deliberately

A rule reaches the same starve-able permission fold every other check does, so it can fault or complete without emitting. Neither is permission to proceed: both yield DeletePreflight.Unestablished, the delete is refused, and the response says NodeDeletionRejectionReason.Unavailable — an availability failure, not a statement about the caller's rights (#1446). There is deliberately no .Catch(_ => true) on this path; the identical instinct on the security-fold twin is what made a group deny fail OPEN (#2011). The same third terminal is added to the no-rule leg, where an empty fold previously posted no response at all and the caller waited out its own timeout (the #2742 shape).

Not changed, on purpose

DeleteNodeRequest / ValidateDeleteRequest carry [RequiresPermission(Permission.Delete)], so on a route through a hub with the AccessControlPipeline (a per-node hub) the delivery gate still demands Permission.Delete on the receiving hub's own path. Widening the message-level gate is a separate decision with a much larger blast radius; it has not been made.

Verification

test/MeshWeaver.Security.Test/DeleteHonoursNodeTypeAccessRuleTest.cs pins all four rows of the table, using the real SatelliteAccessRule. Each was proven falsifiable:

  • rule lookup removed from the pre-flight (the pre-fix behaviour) → tests 1, 3, 4 red;
  • the no-rule fallback relaxed from Delete to Update (an over-permissive fix) → test 2 red — the no-widening test, the one that matters most.

Green locally, Release + -warnaserror, 0 Error(s) on every touched project: Graph 1578/1578 · Security 378/378 · NodeOperations 76/76 · Hosting 269/269 · AccessControl 21/21 · Documentation 147/147 (link integrity + the timeout ratchet — the new test adopts TestTimeouts, it does not re-seed the baseline) · Hosting.Monolith delete + node-operation-target suite 37/37.

Work product conserved

The two-paths-one-question finding, the widening table and the still-unrouted message gate are committed to Doc/Architecture/AccessControl → "One question, ONE answer". What's New entry: Category: Fix.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Kwar1ZGt5FMEpHiXmkzKCm

…ule the validator does (#2913)

An Editor could PUBLISH a satellite onto a node and then be refused when erasing it —
"I can turn it on but not off", reachable by an ordinary Editor, and the exact state a
revocable-consent feature exists to prevent.

Root cause: ONE question, TWO answers, both on the same request.
  * RlsNodeValidator routed Delete through the node type's registered INodeTypeAccessRule.
  * HandleDeleteNodeRequest's pre-flight (CheckDeletePermissionForNode) demanded
    Permission.Delete on MainNode ?? Path outright and never looked at a rule.
The effective policy was their CONJUNCTION, so wherever a rule was more permissive than
Permission.Delete it silently did not apply. SatelliteAccessRule maps a satellite's Delete
to Permission.Update on its MainNode; Role.Editor holds Update and not Delete.

The fix is structural, not a second copy of the lookup: NodeTypeAccessRuleSet is the ONE
index (a mesh-scoped singleton registered in MeshBuilder), and both RlsNodeValidator and
the delete pre-flight resolve their rule from it, so the two cannot drift apart again.

What widened, exactly:
  * A node type WITH a Delete-supporting rule that ALLOWS -> the rule decides. That is the
    fix, and it is confined to the SatelliteAccessRule family (Comment, TrackedChange,
    Kernel, Activity, UserActivity, ActivityLogSegment; in Plugins Thread, ThreadMessage,
    TokenUsage, Portal), whose Delete is Update-on-MainNode by design.
  * A node type with NO rule -> Permission.Delete, byte-for-byte as before. Closed by default.
  * Space and Partition also carried Update-for-Delete in their rules, unreachable because
    the pre-flight's own Permission.Delete demand sat in front of it. Making the rule
    authoritative would have widened them, so both rules now say Permission.Delete for
    Delete explicitly — the policy that was already enforced. Deleting a Space still
    requires Delete rights.

Fail-closed, deliberately: a rule that FAULTS or completes without emitting produces
DeletePreflight.Unestablished — the delete is refused and reported as Unavailable, an
availability failure rather than a statement about the caller's rights (#1446). There is no
.Catch(_ => true) on this path; that instinct is what made a group deny fail OPEN (#2011).
The same third terminal is added to the no-rule leg, where an empty fold previously posted
no response at all. System is decided before any rule, exactly as RlsNodeValidator does.

Verified: DeleteHonoursNodeTypeAccessRuleTest pins all four outcomes, and each was proven
falsifiable by inversion (rule lookup removed -> 1/3/4 red; the no-rule fallback relaxed to
Update -> 2 red). Green: Graph 1578, Security 378, NodeOperations 76, Hosting 269,
AccessControl 21, Documentation 147, Hosting.Monolith delete suite 37.

Closes #2913

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kwar1ZGt5FMEpHiXmkzKCm
Copilot AI lite review requested due to automatic review settings September 1, 2026 08:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes an access-control inconsistency where DeleteNodeRequest pre-flight checks could bypass INodeTypeAccessRule, causing “can create/publish but cannot delete” behavior for satellites (notably impacting Editors). It centralizes node-type access rule selection so both the RLS validator and delete pre-flight answer the same permission question.

Changes:

  • Introduces NodeTypeAccessRuleSet as the single index/selector for INodeTypeAccessRule resolution and wires it into both RlsNodeValidator and delete pre-flight.
  • Updates delete pre-flight to honor node-type rules (with fail-closed “Unestablished” handling) while keeping closed-by-default behavior when no rule applies.
  • Adds targeted security tests and updates documentation + What’s New entry describing the behavior change.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/MeshWeaver.Security.Test/DeleteHonoursNodeTypeAccessRuleTest.cs Adds regression tests covering allow/deny/no-rule/faulting-rule delete outcomes.
src/MeshWeaver.Mesh.Contract/Services/NodeTypeAccessRuleSet.cs New mesh-scoped rule index ensuring one shared rule selection semantics.
src/MeshWeaver.Mesh.Contract/MeshExtensions.cs Routes delete pre-flight through the shared rule set; adds tri-state preflight verdict and fail-closed handling.
src/MeshWeaver.Mesh.Contract/MeshBuilder.cs Registers NodeTypeAccessRuleSet as a mesh-root singleton for consistent resolution.
src/MeshWeaver.Graph/Security/RlsNodeValidator.cs Switches from per-validator dictionary to shared NodeTypeAccessRuleSet.
src/MeshWeaver.Graph/Configuration/SpaceNodeType.cs Makes Space delete explicitly require Permission.Delete now that the rule is authoritative.
src/MeshWeaver.Graph/Configuration/PartitionNodeType.cs Makes Partition delete explicitly require Permission.Delete now that the rule is authoritative.
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-01-you-can-remove-what-you-were-allowed-to-add.md Adds a user-facing “Fix” entry describing the behavior change.
src/MeshWeaver.Documentation/Data/Architecture/AccessControl.md Documents the “one question, one answer” rule and the new shared rule-resolution mechanism.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/MeshWeaver.Mesh.Contract/MeshExtensions.cs Outdated
@github-actions

github-actions Bot commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 0)

793 tests   789 ✅  2m 36s ⏱️
  7 suites    4 💤
  7 files      0 ❌

Results for commit 4e5ef7b.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 5)

    8 files      8 suites   4m 3s ⏱️
1 498 tests 1 497 ✅ 1 💤 0 ❌
2 203 runs  2 202 ✅ 1 💤 0 ❌

Results for commit 4e5ef7b.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 1)

1 138 tests   1 138 ✅  4m 6s ⏱️
    7 suites      0 💤
    7 files        0 ❌

Results for commit 4e5ef7b.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 4)

1 165 tests   1 165 ✅  4m 39s ⏱️
    7 suites      0 💤
    7 files        0 ❌

Results for commit 4e5ef7b.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 2)

3 030 tests   2 838 ✅  6m 59s ⏱️
    8 suites    192 💤
    8 files        0 ❌

Results for commit 4e5ef7b.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 3)

1 153 tests   1 153 ✅  8m 6s ⏱️
    7 suites      0 💤
    7 files        0 ❌

Results for commit 4e5ef7b.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

Test Results

   44 files     44 suites   30m 31s ⏱️
8 777 tests 8 580 ✅ 197 💤 0 ❌
9 482 runs  9 285 ✅ 197 💤 0 ❌

Results for commit 4e5ef7b.

♻️ This comment has been updated with latest results.

… TYPE, not its message

An INodeTypeAccessRule is arbitrary code and its exception text can carry an internal
path, a connection string or another tenant's identifier. That text was echoed into the
DeleteNodeResponse via preflight.Detail. The type is all a caller can act on ("retry, or
ask an operator"); the full exception — message and stack — was already on the LogWarning,
where only an operator sees it.

Pinned: the faulting test rule now throws a distinctive marker string and the test asserts
the response names InvalidOperationException and does NOT contain that marker. Proven
falsifiable — restoring {ex.Message} reddens it.

Copilot review, #2945.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kwar1ZGt5FMEpHiXmkzKCm
@rbuergi

rbuergi commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the Copilot finding in 4e5ef7b: the caller-visible detail now names the exception type only (the 'X' access rule failed (InvalidOperationException)) — the full exception, message and stack, stays on the LogWarning where only an operator sees it.

Turned it into a guard rather than just a fix: the faulting test rule throws a distinctive marker string, and AFaultingRule_RefusesTheDelete_AndReportsUnavailableNotDenied now asserts the response contains InvalidOperationException and does not contain that marker. Proven falsifiable — restoring {ex.Message} reddens it.

Note on scope: the neighbouring RlsNodeValidator.UnestablishedCheck still includes cause.Message in its result. That is pre-existing behaviour on an error contract other code may match on, so tightening it is a separate decision; this change keeps the new path the stricter of the two.

@rbuergi
rbuergi added this pull request to the merge queue Sep 1, 2026
Merged via the queue into main with commit 820dc3f Sep 1, 2026
25 checks passed
@rbuergi
rbuergi deleted the fix/2913-delete-nodetype-access-rule branch October 24, 2026 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

An Editor can publish a satellite it cannot delete: the delete pipeline bypasses INodeTypeAccessRule

2 participants