From f51ae0ee53175a15d6e65f6fca56021e5a3fd73c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20B=C3=BCrgi?= <6334612+rbuergi@users.noreply.github.com> Date: Fri, 4 Sep 2026 08:03:15 +0200 Subject: [PATCH 1/2] fix: a move preserves authorship instead of re-stamping it (#3263) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move is copy-to-target + delete-the-source, and the copy leg re-created every node as if it had just been written by whoever ran the move. Two mechanisms: 1. RetargetNode stamped LastModified = UtcNow unconditionally. A relocation writes no content, so this was the wrong field to touch. Reproduces on any backend. 2. The copy leg read the nodes it re-created from the QUERY, and a query result is a projection. PostgreSqlSqlGenerator's SELECT list carries no created_by, created_date or last_modified_by — they are real columns the point read and the INSERT both use, but no query projects them. So on the production backend every node arrived with the stamps blank and the create path filled them with "now, by the caller". Copy the projection, delete the source, and the authorship of the whole subtree is gone, with no version history behind it. Re-creating a node is a lifecycle operation, so it now reads the node from storage — the same rule the move's delete leg already follows for its path enumeration (#839). The query still decides WHICH nodes (it is the read RLS filters); storage decides WHAT they are. Move preserves all four stamps; copy deliberately does not — a copy is a new node and is stamped for the copier, because AccessContextScope impersonates MeshNode.CreatedBy and an inherited creator would run owner-scoped work as someone who never touched the node. CopyNodeRequest.PreserveAuthorship carries the distinction; no public member is removed or re-signed. Found and filed separately: #3272 — a move DELETES every satellite under the moved node and reports success (same handler, same family). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HhT5AXByEKo2fRQtWH2wCW --- .../Data/Architecture/CqrsAndContentAccess.md | 22 +++ ...09-04-moving-something-keeps-its-author.md | 24 +++ .../CreateNodeRequest.cs | 18 +++ .../MeshExtensions.cs | 107 +++++++++++-- .../MoveNodePreservesAuthorshipTest.cs | 147 ++++++++++++++++++ 5 files changed, 308 insertions(+), 10 deletions(-) create mode 100644 src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-04-moving-something-keeps-its-author.md create mode 100644 test/MeshWeaver.Hosting.Test/MoveNodePreservesAuthorshipTest.cs diff --git a/src/MeshWeaver.Documentation/Data/Architecture/CqrsAndContentAccess.md b/src/MeshWeaver.Documentation/Data/Architecture/CqrsAndContentAccess.md index 886c7fc735..18910b166a 100644 --- a/src/MeshWeaver.Documentation/Data/Architecture/CqrsAndContentAccess.md +++ b/src/MeshWeaver.Documentation/Data/Architecture/CqrsAndContentAccess.md @@ -344,6 +344,28 @@ The `DeleteNodeRequest` / `MoveNodeRequest` / `CopyNodeRequest` types are define > **Summary in one line:** `Query` gives you paths and shells; `GetMeshNodeStream` gives you live content. There is no third channel. +#### 🚨 A query result is a PROJECTION — never re-create a node from one + +The rule above is usually stated as "the query row is stale". It is also **incomplete**: a query row is not the node. A provider is free to omit fields, and the production provider does — `PostgreSqlSqlGenerator.GenerateSelectQuery` projects `id, namespace, name, node_type, description, category, icon, display_order, last_modified, version, state, content, desired_id, main_node, sync_behavior, exclude_from_context` and nothing else. `created_by`, `created_date` and `last_modified_by` are real columns — the storage adapter reads them on a point read and writes them on INSERT — but **no query projects them**, so every node a query hands back on Postgres carries `CreatedBy = null` and `CreatedDate = default`. + +That is harmless for a listing and destructive for a re-create, because `CreateNodeRequest` fills a blank stamp with "now, by the caller" (`CreatedDate == default ? now : node.CreatedDate`). `Move` is implemented as copy-to-target + delete-the-source, so a move that fed the create path a query row **rewrote the authorship of the whole subtree** and, with no version history behind it, destroyed the originals (issue #3263: one subtree move on memex re-stamped ~80 nodes). + +So a lifecycle operation that re-creates a node reads it from **storage**, exactly as the move's delete leg enumerates its paths from storage rather than the catalog (#839): + +- the **query** decides WHICH nodes — it is the read that row-level security filters, and bypassing it would copy nodes the caller may not read; +- **storage** decides WHAT they are — `IStorageAdapter.Read(path, options)` per selected path, before anything is re-created. + +And the four stamps carry an operation-specific answer, never an incidental one: + +| Operation | `CreatedDate` / `CreatedBy` | `LastModified` / `LastModifiedBy` | +|---|---|---| +| **Move** (`CopyNodeRequest.PreserveAuthorship = true`) | preserved verbatim — the node at the target IS the node that was at the source | preserved verbatim — a relocation writes no content | +| **Copy** | cleared, so the create stamps the copier | cleared, so the create stamps the copier | + +A copy must NOT inherit the original's `CreatedBy`: `AccessContextScope` derives the identity it impersonates from exactly that field, so an inherited creator makes owner-scoped work run as someone who never touched the node. If a move should be recorded at all it belongs in the activity log — never on top of who wrote the thing. + +> The same asymmetry has a second, still-open consequence: the copy leg's content query also **excludes satellite paths** (`IsExcludedFromResults` mirrors PG's per-prefix satellite tables) while the delete leg's `ListDescendantPaths` enumerates every table of the partition — so a move deletes the `_Comment` / `_Thread` / `_Access` satellites it never copied, and reports success. Measured and filed as #3272; it is not fixed by reading from storage, because the *enumeration* is what has to change and the query is the read row-level security filters. + --- ## 🚨 No "pedestrian queries" — use synced queries diff --git a/src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-04-moving-something-keeps-its-author.md b/src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-04-moving-something-keeps-its-author.md new file mode 100644 index 0000000000..5ec459b8b0 --- /dev/null +++ b/src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-04-moving-something-keeps-its-author.md @@ -0,0 +1,24 @@ +--- +Name: Moving something keeps its author and its dates +Category: Fix +Description: Moving a page or a folder no longer rewrites who created it and when — the original author, creation date and last-edited stamps now travel with it, for everything underneath too. +Icon: Sparkle +Order: -20260904 +--- + +# Moving something keeps its author and its dates + +Moving a page — from the node menu or with the `move` command — used to record it as if it had just +been written from scratch by whoever did the move. The creator, the creation date, the last-edited +date and the last editor were all replaced with "you, just now", and not only on the item you +moved: everything underneath it was rewritten the same way. One tidy-up of a client folder was +enough to make eighty documents, a commercial proposal among them, look like they had been authored +that minute by the person reorganising them. Nothing could recover the real values afterwards. + +A move changes where something lives. It does not change who wrote it, and it is not an edit, so all +four stamps now travel with the item and with everything under it. The path and the namespace change +— that is the point of a move — and nothing else does. + +Copying is deliberately different, and now says so consistently: a copy is a new item, so it is +recorded as created by you, at the moment you copied it, while the original keeps its own history +untouched. diff --git a/src/MeshWeaver.Mesh.Contract/CreateNodeRequest.cs b/src/MeshWeaver.Mesh.Contract/CreateNodeRequest.cs index d824b02c83..9f388b2d20 100644 --- a/src/MeshWeaver.Mesh.Contract/CreateNodeRequest.cs +++ b/src/MeshWeaver.Mesh.Contract/CreateNodeRequest.cs @@ -577,6 +577,24 @@ public record CopyNodeRequest(string SourcePath, string TargetPath) : IRequestIf true, copies satellite nodes (e.g. _Comment, _Activity) attached to the source. public bool IncludeSatellites { get; init; } + + /// + /// 🚨 true only for the copy leg of a : the node at the + /// target IS the node that was at the source, so , + /// , and + /// travel with it VERBATIM. A relocation changes WHERE a + /// node lives, never WHO wrote it or WHEN (issue #3263: one subtree move on + /// memex.systemorph.com re-stamped ~80 nodes — a commercial proposal among them — as created + /// that minute by whoever ran the move, unrecoverably). + /// + /// A plain COPY leaves this false, and that is deliberate rather than incidental: + /// the copy is a NEW node, so the four stamps are cleared here and the create path stamps the + /// copier and the moment of the copy. Preserving them on a copy would be actively wrong — + /// AccessContextScope derives the identity to impersonate from + /// , so a copy that kept the original author would run + /// owner-scoped work as someone who never touched it. + /// + public bool PreserveAuthorship { get; init; } } /// diff --git a/src/MeshWeaver.Mesh.Contract/MeshExtensions.cs b/src/MeshWeaver.Mesh.Contract/MeshExtensions.cs index f4f305b4f7..5bbf43e841 100644 --- a/src/MeshWeaver.Mesh.Contract/MeshExtensions.cs +++ b/src/MeshWeaver.Mesh.Contract/MeshExtensions.cs @@ -5042,18 +5042,42 @@ private static IMessageDelivery HandleMoveNodeRequest( { var logger = hub.ServiceProvider.GetRequiredService().CreateLogger("MeshWeaver.Mesh.Services.IMeshCatalog"); var moveRequest = request.Message; - var meshService = hub.ServiceProvider.GetRequiredService(); var storage = hub.ServiceProvider.GetRequiredService(); var changeFeed = hub.ServiceProvider.GetService(); + var accessService = hub.ServiceProvider.GetService(); var sourcePath = moveRequest.SourcePath; var targetPath = moveRequest.TargetPath; + // 🚨 PreserveAuthorship — the copy leg of a MOVE re-creates the SAME node at a new path, so + // its creation and modification stamps travel with it verbatim (issue #3263). This is why + // the request is posted here rather than through IMeshService.CopyNode: that surface says + // nothing about authorship, and a move is the one caller for which it is not free to invent + // it. Same seam MeshService uses — the issuing hub posts to the node-operation target and + // stamps the caller's AccessContext, so the per-node creates inside the copy authorise as + // the mover, exactly as before. + var copyRequest = new CopyNodeRequest(sourcePath, targetPath) + { + IncludeDescendants = true, + IncludeSatellites = true, + PreserveAuthorship = true, + }; + var callerContext = request.AccessContext + ?? accessService?.Context ?? accessService?.CircuitContext; + // Move = Copy (with satellites + descendants) → reactive delete of every source path. // Delete only fires after Copy succeeds (SelectMany short-circuits on copy error). // Source-subtree enumeration is AUTHORITATIVE from storage (ListDescendantPaths), // never the eventually-consistent catalog query — the same stale-plan defect that // left recursive-delete survivors (issue #839) would leave source rows behind here. - meshService.CopyNode(sourcePath, targetPath, includeDescendants: true, includeSatellites: true) + Observable.Defer(() => hub.NodeOperationIssuingHub().Observe(copyRequest, o => + { + o = o.WithTarget(hub.NodeOperationTarget()); + return callerContext != null ? o.WithAccessContext(callerContext) : o; + })) + .SelectMany(d => d.Message is { Success: true, Node: { } copiedRoot } + ? Observable.Return(copiedRoot) + : Observable.Throw( + new InvalidOperationException(d.Message.Error ?? "Node copy failed"))) .SelectMany(copied => storage.ListDescendantPaths(sourcePath) .Take(1) @@ -5120,6 +5144,7 @@ private static IMessageDelivery HandleCopyNodeRequest( var copyRequest = request.Message; var meshService = hub.ServiceProvider.GetRequiredService(); var accessService = hub.ServiceProvider.GetService(); + var persistence = hub.ServiceProvider.GetService(); var sourcePath = copyRequest.SourcePath; var targetPath = copyRequest.TargetPath; @@ -5146,14 +5171,45 @@ callerAccessContext is null || accessService is null () => accessService.SwitchAccessContext(callerAccessContext), _ => meshService.CreateNode(node)); - logger.LogDebug("[CopyNode] start source={Source} target={Target} (descendants={Desc} satellites={Sat})", - sourcePath, targetPath, copyRequest.IncludeDescendants, copyRequest.IncludeSatellites); + // 🚨 A QUERY RESULT IS A PROJECTION, NOT THE NODE. The subtree query below decides WHICH + // paths this caller may copy — that part is authoritative, because it is the read the + // row-level security filter runs through. What comes back is NOT the stored node: a + // provider is free to omit columns, and the production one does. PostgreSqlSqlGenerator's + // SELECT list carries id/namespace/name/node_type/description/category/icon/display_order/ + // last_modified/version/state/content/desired_id/main_node/sync_behavior/ + // exclude_from_context and NOTHING ELSE — `created_by`, `created_date` and + // `last_modified_by` exist as real columns (PostgreSqlStorageAdapter.AuthorCols reads them, + // the INSERT writes them) but no query projects them. So on PG every node this handler saw + // arrived with CreatedBy null and CreatedDate default, the create path filled the blanks + // with "now, by the caller", and a MOVE — copy + delete of the source — destroyed the real + // authorship of the whole subtree with nothing left to recover it from (issue #3263). + // + // Re-creating a node is a lifecycle operation, so it reads the node from STORAGE, exactly + // as the move's delete leg enumerates from storage rather than the catalog (#839). The + // query stays in charge of WHICH nodes; storage is in charge of WHAT they are. + // + // 🚨 DefaultIfEmpty, not just Take(1): an adapter whose Read COMPLETES EMPTY for a path it + // cannot serve would otherwise end this node's sequence without an emission — the copy + // would produce no root, post no response, and the move that awaits it would sit out the + // hub's request timeout. Falling back to the projection keeps a copy possible on a store + // that cannot answer a point read, rather than hanging it. + IObservable Authoritative(MeshNode projected) => + persistence is null + ? Observable.Return(projected) + : persistence.Read(projected.Path, hub.JsonSerializerOptions) + .Take(1) + .DefaultIfEmpty() + .Select(stored => stored ?? projected); + + logger.LogDebug("[CopyNode] start source={Source} target={Target} (descendants={Desc} satellites={Sat} preserveAuthorship={Preserve})", + sourcePath, targetPath, copyRequest.IncludeDescendants, copyRequest.IncludeSatellites, + copyRequest.PreserveAuthorship); // Subtree query covers source + descendants + satellites (anything under sourcePath). // Query's first emission is the initial result set; we Take(1) and project each // node into a CreateNode call at the new target path. meshService.Query(MeshQueryRequest.FromQuery( - $"path:{sourcePath} scope:subtree")) + $"path:{sourcePath} scope:subtree").Complete()) .Take(1) .Timeout(TimeSpan.FromSeconds(15)) .Catch, Exception>(ex => @@ -5193,13 +5249,16 @@ callerAccessContext is null || accessService is null // require all inserts to complete before the source is deleted. Every create runs // under the caller's identity (CreateUnderCaller) so the routed per-descendant // CreateNodeRequest carries a valid AccessContext across the scheduler hop. - return CreateUnderCaller(RetargetNode(sourceNode, sourcePath, targetPath)) + return Authoritative(sourceNode) + .Select(stored => RetargetNode(stored, sourcePath, targetPath, copyRequest.PreserveAuthorship)) + .SelectMany(CreateUnderCaller) .SelectMany(rootCreated => { if (others.Count == 0) return Observable.Return<(MeshNode Root, int Desc, int Sat)>((rootCreated, descCount, satCount)); return others.ToObservable() - .Select(n => RetargetNode(n, sourcePath, targetPath)) + .SelectMany(Authoritative) + .Select(n => RetargetNode(n, sourcePath, targetPath, copyRequest.PreserveAuthorship)) .SelectMany(retargeted => CreateUnderCaller(retargeted)) .ToList() .Select(_ => ((MeshNode Root, int Desc, int Sat))(rootCreated, descCount, satCount)); @@ -5231,8 +5290,28 @@ callerAccessContext is null || accessService is null /// Builds a new MeshNode by relocating from /// to . Path is derived from Namespace + Id; MainNode is rewritten /// when it pointed inside the old subtree. + /// + /// 🚨 decides the four stamps, and it is the ONLY + /// thing that decides them — the create at the target keeps whatever this method leaves set + /// (CreatedDate == default ? now : …) and fills in whatever it clears. + /// + /// A MOVE preserves them. The node at the target IS the node that was at the + /// source. CreatedDate and CreatedBy are documented on as + /// "set once at creation time; never updated thereafter" / "never changed", and + /// LastModified/LastModifiedBy answer "who last wrote this content" — a + /// relocation writes no content. This method used to stamp LastModified = UtcNow + /// unconditionally, which is half of issue #3263; the other half is the projection the copy + /// used to read its nodes from (see Authoritative in + /// ). A move that should be recorded belongs in the + /// activity log, not on top of who wrote the thing. + /// A COPY clears them so the create path stamps the copier and the moment of the + /// copy. Clearing is deliberate: inheriting the original's CreatedBy would hand the + /// copy an owner who never touched it, and AccessContextScope impersonates exactly that + /// identity. It also makes a copy's stamps identical on every backend instead of depending on + /// which fields the store's query happens to project. + /// /// - private static MeshNode RetargetNode(MeshNode node, string oldRoot, string newRoot) + private static MeshNode RetargetNode(MeshNode node, string oldRoot, string newRoot, bool preserveAuthorship) { var newPath = string.Equals(node.Path, oldRoot, StringComparison.Ordinal) ? newRoot @@ -5247,13 +5326,21 @@ private static MeshNode RetargetNode(MeshNode node, string oldRoot, string newRo : node.MainNode.StartsWith(oldRoot + "/", StringComparison.Ordinal) ? newRoot + node.MainNode[oldRoot.Length..] : node.MainNode; - return node with + var relocated = node with { Id = id, Namespace = ns, MainNode = newMainNode, - LastModified = DateTimeOffset.UtcNow }; + return preserveAuthorship + ? relocated + : relocated with + { + CreatedDate = default, + CreatedBy = null, + LastModified = default, + LastModifiedBy = null, + }; } /// diff --git a/test/MeshWeaver.Hosting.Test/MoveNodePreservesAuthorshipTest.cs b/test/MeshWeaver.Hosting.Test/MoveNodePreservesAuthorshipTest.cs new file mode 100644 index 0000000000..2bbdcad20c --- /dev/null +++ b/test/MeshWeaver.Hosting.Test/MoveNodePreservesAuthorshipTest.cs @@ -0,0 +1,147 @@ +using System; +using System.Reactive.Linq; +using System.Threading.Tasks; +using MeshWeaver.Fixture; +using MeshWeaver.Hosting.Monolith.TestBase; +using MeshWeaver.Mesh; +using MeshWeaver.Mesh.Security; +using MeshWeaver.Mesh.Services; +using MeshWeaver.Messaging; +using Microsoft.Extensions.DependencyInjection; +using Xunit; + +namespace MeshWeaver.Hosting.Test; + +/// +/// A MOVE relocates a node. It is not a creation and it is not a content edit, so +/// , , +/// and must survive it — +/// on the node named in the request AND on every descendant that rides along. Issue #3263. +/// +/// Measured on memex.systemorph.com 2026-09-03: one subtree move re-stamped ~80 nodes, +/// including a commercial proposal, as created that minute by the person who ran the move. Nothing +/// recovers the old values — there is no version history to fall back on — so this is permanent +/// data loss, not a cosmetic wrongness. +/// +/// The COPY case is the CONTROL ARM, and it is a different answer on purpose: a copy is a new +/// node and is stamped for the copier. Without it "preserve the stamps" reads as a rule about +/// re-creating nodes in general, which it is not. +/// +public class MoveNodePreservesAuthorshipTest(ITestOutputHelper output) : MonolithMeshTestBase(output) +{ + private AccessService Access => Mesh.ServiceProvider.GetRequiredService(); + + private static readonly AccessContext Author = + new() { ObjectId = "author-a", Name = "Author A" }; + + private static readonly AccessContext Mover = + new() { ObjectId = "mover-b", Name = "Mover B" }; + + [Fact(Timeout = 60000)] + public async Task Move_KeepsTheStamps_OnTheNodeAndOnItsDescendants() + { + var suffix = Guid.NewGuid().ToString("N")[..8]; + var sourceRoot = $"{TestPartition}/proposal-{suffix}"; + var sourceChild = $"{sourceRoot}/Pricing"; + var targetRoot = $"{TestPartition}/filed-{suffix}"; + var targetChild = $"{targetRoot}/Pricing"; + + Access.SetCircuitContext(Author); + await NodeFactory.CreateNode(MeshNode.FromPath(sourceRoot) with + { + Name = "Proposal", + NodeType = "Markdown", + State = MeshNodeState.Active, + }).Should().Within(TestTimeouts.Convergence).Emit(); + await NodeFactory.CreateNode(MeshNode.FromPath(sourceChild) with + { + Name = "Pricing", + NodeType = "Markdown", + State = MeshNodeState.Active, + }).Should().Within(TestTimeouts.Convergence).Emit(); + + var rootBefore = await ReadExisting(sourceRoot); + var childBefore = await ReadExisting(sourceChild); + + rootBefore.CreatedBy.Should().Be(Author.ObjectId, "precondition: the author created it"); + childBefore.CreatedBy.Should().Be(Author.ObjectId, "precondition: the author created it"); + rootBefore.CreatedDate.Should().NotBe(default, "precondition: creation is stamped"); + childBefore.LastModified.Should().NotBe(default, "precondition: modification is stamped"); + + Access.SetCircuitContext(Mover); + var moved = await ObserveNodeOperation(new MoveNodeRequest(sourceRoot, targetRoot)) + .Should().Within(TestTimeouts.Convergence).Emit(); + moved.Message.Success.Should().BeTrue(moved.Message.Error ?? "the move must succeed"); + + var rootAfter = await ReadExisting(targetRoot); + var childAfter = await ReadExisting(targetChild); + + Output.WriteLine($"root before: createdBy={rootBefore.CreatedBy} createdDate={rootBefore.CreatedDate:O} lastModified={rootBefore.LastModified:O} lastModifiedBy={rootBefore.LastModifiedBy}"); + Output.WriteLine($"root after : createdBy={rootAfter.CreatedBy} createdDate={rootAfter.CreatedDate:O} lastModified={rootAfter.LastModified:O} lastModifiedBy={rootAfter.LastModifiedBy}"); + Output.WriteLine($"child before: createdBy={childBefore.CreatedBy} createdDate={childBefore.CreatedDate:O} lastModified={childBefore.LastModified:O} lastModifiedBy={childBefore.LastModifiedBy}"); + Output.WriteLine($"child after : createdBy={childAfter.CreatedBy} createdDate={childAfter.CreatedDate:O} lastModified={childAfter.LastModified:O} lastModifiedBy={childAfter.LastModifiedBy}"); + + rootAfter.CreatedBy.Should().Be(rootBefore.CreatedBy, "a move is not a creation"); + rootAfter.CreatedDate.Should().Be(rootBefore.CreatedDate, "a move is not a creation"); + rootAfter.LastModified.Should().Be(rootBefore.LastModified, "a move is not a content edit"); + rootAfter.LastModifiedBy.Should().Be(rootBefore.LastModifiedBy, "a move is not a content edit"); + + childAfter.CreatedBy.Should().Be(childBefore.CreatedBy, "the subtree rides along"); + childAfter.CreatedDate.Should().Be(childBefore.CreatedDate, "the subtree rides along"); + childAfter.LastModified.Should().Be(childBefore.LastModified, "the subtree rides along"); + childAfter.LastModifiedBy.Should().Be(childBefore.LastModifiedBy, "the subtree rides along"); + } + + /// + /// The other half of the decision, pinned so it cannot be "simplified" into the move's rule: a + /// COPY is a NEW node and is stamped for the copier. Preserving the original's + /// here would hand the copy an owner who never touched it — + /// and AccessContextScope impersonates precisely that identity — so "both operations + /// preserve" is not a simplification, it is a different (wrong) answer. + /// + [Fact(Timeout = 60000)] + public async Task Copy_StampsTheCopier_BecauseACopyIsANewNode() + { + var suffix = Guid.NewGuid().ToString("N")[..8]; + var sourcePath = $"{TestPartition}/original-{suffix}"; + var targetPath = $"{TestPartition}/duplicate-{suffix}"; + + Access.SetCircuitContext(Author); + await NodeFactory.CreateNode(MeshNode.FromPath(sourcePath) with + { + Name = "Original", + NodeType = "Markdown", + State = MeshNodeState.Active, + }).Should().Within(TestTimeouts.Convergence).Emit(); + + var before = await ReadExisting(sourcePath); + before.CreatedBy.Should().Be(Author.ObjectId, "precondition: the author created it"); + + Access.SetCircuitContext(Mover); + var copied = await ObserveNodeOperation(new CopyNodeRequest(sourcePath, targetPath)) + .Should().Within(TestTimeouts.Convergence).Emit(); + copied.Message.Success.Should().BeTrue(copied.Message.Error ?? "the copy must succeed"); + + var after = await ReadExisting(targetPath); + + after.CreatedBy.Should().Be(Mover.ObjectId, + "the copy is a new node — it was created by whoever copied it"); + after.LastModifiedBy.Should().Be(Mover.ObjectId, + "nobody but the copier has ever written this node"); + after.CreatedDate.Should().NotBe(before.CreatedDate, + "the copy came into existence at the moment of the copy, not when the original was written"); + + var stillThere = await ReadExisting(sourcePath); + stillThere.CreatedBy.Should().Be(Author.ObjectId, + "a copy leaves the original — and its authorship — alone"); + } + + /// + /// The authoritative single-node read — the owner-hub round-trip the base class exposes, not a + /// query row: the whole point of this test is which stamps the node CARRIES, and a query result + /// is a projection that need not carry them at all. + /// + private async Task ReadExisting(string path) => + (await ReadNode(path).Should().Within(TestTimeouts.Convergence) + .Match(n => n is not null, $"the node at {path} must exist"))!; +} From 59c7cee957e52721797ae2e81f3024eb9735acd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20B=C3=BCrgi?= <6334612+rbuergi@users.noreply.github.com> Date: Fri, 4 Sep 2026 08:18:22 +0200 Subject: [PATCH 2/2] fix: gate PreserveAuthorship, and refuse rather than fall back when preserving MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the review of the first commit, both correct. 1. PreserveAuthorship was honoured unconditionally. CopyNodeRequest is a wire message, so the flag is caller-settable — and a node whose CreatedBy names somebody else is not inert: AccessContextScope.FromNode impersonates exactly that identity, so any caller who can read a node could mint one, in a place they control, that owner-scoped work then runs AS its author. Measured with the gate removed: an Editor (Read|Create|Update, no Delete) successfully copied an admin's node with its authorship intact. RLS does not close this — reading the source is precisely what an Editor may do. The flag now requires Delete on the source's namespace: what MoveNodePermissionAttribute already requires of a mover, so it confers nothing the platform did not already grant that caller. An UNDETERMINED fold refuses too, reported as unavailability rather than a denial (#974/#2742). 2. Authoritative() fell back to the query projection when the point read came back empty — which, for a preserving copy, is the exact projection that destroyed the authorship, so the fix would have quietly reopened the bug on the one store that cannot answer. It now refuses; since the delete leg only runs after the copy succeeds, a refused read leaves the source and its stamps where they are. A plain copy keeps the fallback: its stamps are cleared anyway, so nothing is lost. PreserveAuthorshipIsGatedTest pins both halves — the Editor is refused with Unauthorized, and the same Editor's plain copy still succeeds and is stamped for them. Without the second the first would be satisfied by a subject who could not copy at all. The third review comment (Observable.Using + SwitchAccessContext) is on pre-existing code this diff does not touch — the allow-listed site already recorded in test/ImpersonationScopeSites.allow. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HhT5AXByEKo2fRQtWH2wCW --- .../Data/Architecture/CqrsAndContentAccess.md | 4 + .../MeshExtensions.cs | 80 ++++++++++- .../PreserveAuthorshipIsGatedTest.cs | 125 ++++++++++++++++++ 3 files changed, 204 insertions(+), 5 deletions(-) create mode 100644 test/MeshWeaver.Hosting.Test/PreserveAuthorshipIsGatedTest.cs diff --git a/src/MeshWeaver.Documentation/Data/Architecture/CqrsAndContentAccess.md b/src/MeshWeaver.Documentation/Data/Architecture/CqrsAndContentAccess.md index 18910b166a..12ff775ff3 100644 --- a/src/MeshWeaver.Documentation/Data/Architecture/CqrsAndContentAccess.md +++ b/src/MeshWeaver.Documentation/Data/Architecture/CqrsAndContentAccess.md @@ -364,6 +364,10 @@ And the four stamps carry an operation-specific answer, never an incidental one: A copy must NOT inherit the original's `CreatedBy`: `AccessContextScope` derives the identity it impersonates from exactly that field, so an inherited creator makes owner-scoped work run as someone who never touched the node. If a move should be recorded at all it belongs in the activity log — never on top of who wrote the thing. +That same sentence is why `PreserveAuthorship` is **gated, not merely defaulted off**. `CopyNodeRequest` is a wire message, so the flag is caller-settable, and a caller who can read a node could otherwise mint one — in a place they control — that owner-scoped work then runs AS its author. The handler therefore honours the flag only for a caller holding `Permission.Delete` on the source's namespace: exactly what `MoveNodePermissionAttribute` requires of a mover, so the flag confers nothing the platform did not already grant that caller. An UNDETERMINED permission fold is a refusal too, reported as unavailability rather than as a denial. Measured with the gate removed: an **Editor** (`Read | Create | Update | …`, no `Delete`) succeeded in copying an admin's node with its authorship intact — row-level security does not close this, because reading the source is precisely what an Editor may do. + +Two facts pin it — `PreserveAuthorshipIsGatedTest`: the Editor is refused with `Unauthorized`, and the same Editor's *plain* copy still succeeds and is stamped for them. Without the second, the first is satisfied by a subject who could not copy at all, and the gate would be indistinguishable from RLS refusing the whole operation. + > The same asymmetry has a second, still-open consequence: the copy leg's content query also **excludes satellite paths** (`IsExcludedFromResults` mirrors PG's per-prefix satellite tables) while the delete leg's `ListDescendantPaths` enumerates every table of the partition — so a move deletes the `_Comment` / `_Thread` / `_Access` satellites it never copied, and reports success. Measured and filed as #3272; it is not fixed by reading from storage, because the *enumeration* is what has to change and the query is the read row-level security filters. --- diff --git a/src/MeshWeaver.Mesh.Contract/MeshExtensions.cs b/src/MeshWeaver.Mesh.Contract/MeshExtensions.cs index 5bbf43e841..f5492772ca 100644 --- a/src/MeshWeaver.Mesh.Contract/MeshExtensions.cs +++ b/src/MeshWeaver.Mesh.Contract/MeshExtensions.cs @@ -5191,24 +5191,80 @@ callerAccessContext is null || accessService is null // 🚨 DefaultIfEmpty, not just Take(1): an adapter whose Read COMPLETES EMPTY for a path it // cannot serve would otherwise end this node's sequence without an emission — the copy // would produce no root, post no response, and the move that awaits it would sit out the - // hub's request timeout. Falling back to the projection keeps a copy possible on a store - // that cannot answer a point read, rather than hanging it. + // hub's request timeout. + // + // 🚨 And the empty case is NOT the same question for the two operations. For a plain copy + // the stamps are cleared anyway, so falling back to the projection loses nothing and keeps + // a copy possible on a store that cannot answer a point read. For a PRESERVING copy the + // projection is precisely the thing that destroyed the authorship, so falling back to it + // would re-open this bug behind the fix — quietly, on exactly the store that cannot answer. + // It REFUSES instead, and because the delete leg only runs after the copy succeeds, a + // refused read leaves the source and its stamps untouched rather than moving them away. + IObservable ProjectionFallback(MeshNode projected) => + copyRequest.PreserveAuthorship + ? Observable.Throw(new InvalidOperationException( + $"Authoritative read of '{projected.Path}' returned nothing — refusing to relocate it " + + "from a query projection, which does not carry createdBy/createdDate/lastModifiedBy.")) + : Observable.Return(projected); + IObservable Authoritative(MeshNode projected) => persistence is null - ? Observable.Return(projected) + ? ProjectionFallback(projected) : persistence.Read(projected.Path, hub.JsonSerializerOptions) .Take(1) .DefaultIfEmpty() - .Select(stored => stored ?? projected); + .SelectMany(stored => stored is not null + ? Observable.Return(stored) + : ProjectionFallback(projected)); logger.LogDebug("[CopyNode] start source={Source} target={Target} (descendants={Desc} satellites={Sat} preserveAuthorship={Preserve})", sourcePath, targetPath, copyRequest.IncludeDescendants, copyRequest.IncludeSatellites, copyRequest.PreserveAuthorship); + // 🚨 PRESERVED AUTHORSHIP IS NOT FREE FOR THE ASKING. CopyNodeRequest is a wire message, so + // the flag is caller-settable, and a node whose CreatedBy names someone else is not inert: + // AccessContextScope.FromNode impersonates exactly that identity, so an unguarded flag would + // let anyone who can read a node mint one, in a place they control, that owner-scoped work + // then runs AS its author. + // + // The flag exists for one operation, so it is gated on that operation's own entitlement: + // Delete on the source's namespace, which is what MoveNodePermissionAttribute already + // requires of a mover. A caller who holds it could have MOVED the node and preserved the + // stamps that way, so the flag grants nothing the platform did not already grant them; a + // caller who does not is refused. UNDETERMINED is a refusal too, reported as unavailability + // rather than as a denial (#974/#2742) — a fold that reached no answer must never read as + // one, in either direction. + IObservable<(string Message, bool Undetermined)?> AuthorshipPreservationRefusal() => + !copyRequest.PreserveAuthorship + ? Observable.Return<(string, bool)?>(null) + : hub.CheckPermissionOutcome(NamespaceOf(sourcePath), Permission.Delete) + .Select(outcome => outcome.IsGranted + ? ((string, bool)?)null + : outcome.IsUndetermined + ? ($"Cannot determine whether '{sourcePath}' may be relocated with its authorship: " + + $"{outcome.UndeterminedReason}", true) + : ($"Preserving authorship on a copy of '{sourcePath}' requires Delete on its " + + "namespace — the same entitlement a move of it requires.", false)); + // Subtree query covers source + descendants + satellites (anything under sourcePath). // Query's first emission is the initial result set; we Take(1) and project each // node into a CreateNode call at the new target path. - meshService.Query(MeshQueryRequest.FromQuery( + AuthorshipPreservationRefusal() + .SelectMany(refusal => + { + if (refusal is { } denied) + { + logger.LogWarning("[CopyNode] REFUSED preserveAuthorship {Source} -> {Target}: {Reason}", + sourcePath, targetPath, denied.Message); + hub.Post(CopyNodeResponse.Fail(denied.Message, + denied.Undetermined + ? NodeCopyRejectionReason.Unknown + : NodeCopyRejectionReason.Unauthorized), + o => o.ResponseFor(request)); + return Observable.Empty<(MeshNode Root, int Desc, int Sat)>(); + } + + return meshService.Query(MeshQueryRequest.FromQuery( $"path:{sourcePath} scope:subtree").Complete()) .Take(1) .Timeout(TimeSpan.FromSeconds(15)) @@ -5263,6 +5319,7 @@ persistence is null .ToList() .Select(_ => ((MeshNode Root, int Desc, int Sat))(rootCreated, descCount, satCount)); }); + }); }) .Subscribe( t => @@ -5286,6 +5343,19 @@ persistence is null return request.Processed(); } + /// + /// The namespace a path lives in — everything before its last segment, or the path itself when + /// it has none. Deliberately the SAME computation + /// uses to decide which namespace a move must hold + /// Delete on, because the copy handler's preserve-authorship gate asks that same question and + /// the two answers must not be able to drift apart. + /// + private static string NamespaceOf(string path) + { + var lastSlash = path.LastIndexOf('/'); + return lastSlash > 0 ? path[..lastSlash] : path; + } + /// /// Builds a new MeshNode by relocating from /// to . Path is derived from Namespace + Id; MainNode is rewritten diff --git a/test/MeshWeaver.Hosting.Test/PreserveAuthorshipIsGatedTest.cs b/test/MeshWeaver.Hosting.Test/PreserveAuthorshipIsGatedTest.cs new file mode 100644 index 0000000000..fd50299509 --- /dev/null +++ b/test/MeshWeaver.Hosting.Test/PreserveAuthorshipIsGatedTest.cs @@ -0,0 +1,125 @@ +using System; +using System.Threading.Tasks; +using MeshWeaver.Fixture; +using MeshWeaver.Hosting.Monolith.TestBase; +using MeshWeaver.Mesh; +using MeshWeaver.Mesh.Security; +using MeshWeaver.Mesh.Services; +using MeshWeaver.Messaging; +using Microsoft.Extensions.DependencyInjection; +using Xunit; + +namespace MeshWeaver.Hosting.Test; + +/// +/// CopyNodeRequest.PreserveAuthorship mints a node whose +/// names somebody else, and AccessContextScope.FromNode impersonates exactly that identity — +/// so the flag is gated on the entitlement of the operation it exists for: +/// on the source's namespace, which is what MoveNodePermissionAttribute requires of a mover. +/// +/// 🚨 The subject is an Editor, and that is the whole design of this test. Editor is +/// Read | Create | Update | … — everything a copy needs and no Delete — so it is the +/// one caller for whom the gate can actually decide something: row-level security lets them read the +/// source and create at the target, the copy would otherwise go through, and only the gate stands +/// between them and a node in their own space that claims the admin as its author. A subject who +/// simply cannot read the source would be stopped by RLS, and a gate exercised only through that +/// case would be a gate that never fires where it matters. +/// +/// This suite therefore builds its mesh from +/// — the default one grants Public → Admin, under which every identity holds Delete and no refusal is +/// observable. +/// +public class PreserveAuthorshipIsGatedTest(ITestOutputHelper output) : MonolithMeshTestBase(output) +{ + private const string EditorId = "editor-e"; + + private AccessService Access => Mesh.ServiceProvider.GetRequiredService(); + + private static readonly AccessContext EditorContext = + new() { ObjectId = EditorId, Name = "Editor E" }; + + /// + /// Base mesh + ONE seeded grant: is an Editor on TestData. No + /// PublicAdminAccess — see the class remarks. + /// + protected override MeshBuilder ConfigureMesh(MeshBuilder builder) + => ConfigureMeshBase(builder) + .AddMeshNodes(new MeshNode($"{EditorId}_Access", $"{TestPartition}/_Access") + { + NodeType = "AccessAssignment", + Name = "Editor E — Editor on TestData", + MainNode = TestPartition, + Content = new AccessAssignment + { + AccessObject = EditorId, + DisplayName = "Editor E", + Roles = [new RoleAssignment { Role = "Editor" }], + }, + }); + + [Fact(Timeout = 60000)] + public async Task AnEditorWhoCouldNotMoveTheNode_IsRefusedAPreservingCopy() + { + var sourcePath = await SeedAsAdmin("gated"); + + Access.SetCircuitContext(EditorContext); + var response = await ObserveNodeOperation( + new CopyNodeRequest(sourcePath, NewPath("gated-copy")) + { + PreserveAuthorship = true, + }) + .Should().Within(TestTimeouts.Convergence).Emit(); + + Output.WriteLine($"refusal: success={response.Message.Success} reason={response.Message.RejectionReason} error={response.Message.Error}"); + + response.Message.Success.Should().BeFalse( + "an Editor holds no Delete on the namespace, so they could not have MOVED this node — " + + "preserving its authorship on a copy would hand them an impersonation they cannot " + + "otherwise obtain"); + response.Message.RejectionReason.Should().Be(NodeCopyRejectionReason.Unauthorized, + "the refusal is an authorisation decision about the flag, not a missing source"); + } + + /// + /// The control arm — without it the test above is satisfied by an Editor who simply cannot copy + /// at all, and the gate would be indistinguishable from RLS refusing the whole operation. + /// + [Fact(Timeout = 60000)] + public async Task TheSameEditor_CopiesFreely_WhenTheyDoNotAskToPreserveAuthorship() + { + var sourcePath = await SeedAsAdmin("plain"); + var targetPath = NewPath("plain-copy"); + + Access.SetCircuitContext(EditorContext); + var response = await ObserveNodeOperation(new CopyNodeRequest(sourcePath, targetPath)) + .Should().Within(TestTimeouts.Convergence).Emit(); + + Output.WriteLine($"plain copy: success={response.Message.Success} reason={response.Message.RejectionReason} error={response.Message.Error}"); + + response.Message.Success.Should().BeTrue( + response.Message.Error + ?? "the gate is about the flag alone — an Editor may still copy, and the copy is stamped for them"); + + var copy = await ReadNode(targetPath).Should().Within(TestTimeouts.Convergence) + .Match(n => n is not null, "the copy must exist"); + copy!.CreatedBy.Should().Be(EditorId, + "a copy is a new node, stamped for whoever made it — which is exactly why it needs no " + + "Delete on the source"); + } + + private static string NewPath(string prefix) => + $"{TestPartition}/{prefix}-{Guid.NewGuid().ToString("N")[..8]}"; + + private async Task SeedAsAdmin(string prefix) + { + Access.SetCircuitContext(TestUsers.Admin); + var path = NewPath(prefix); + await NodeFactory.CreateNode(MeshNode.FromPath(path) with + { + Name = prefix, + NodeType = "Markdown", + State = MeshNodeState.Active, + }).Should().Within(TestTimeouts.Convergence).Emit(); + return path; + } +}