From 0f335e58af5460615eb6ad72631f37e6dce47847 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20B=C3=BCrgi?= Date: Sat, 18 Jul 2026 11:34:52 +0200 Subject: [PATCH] fix(access): render denied subject thumbnails as benign fallback (#434) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Access & Control tab renders each AccessAssignment row via a MeshNodeThumbnailControl bound to the SUBJECT's node path. A subject often lives in a partition the viewer is not a member of (two users can share access to a node without reading each other's user partitions), so the RLS gate denies the subject read BY DESIGN — MeshNodeStreamCache .GateOnRead throws UnauthorizedAccessException. MeshNodeThumbnailView.BindData forwarded that denial straight to SurfaceError → LogError + PortalErrorSink.Report (the toast) + StateHasChanged, and AccessControlLayoutArea recomposed the whole tab on every upstream emission — re-instantiating each thumbnail, re-subscribing each denied subject stream, re-toasting. Result: repeated permission-denied toasts, flicker, and overlapping cards. Root-cause fix: 1. Classify the denial as benign. New tested predicate MeshNodeThumbnailControl.ShouldSurfaceStreamError delegates to AreaErrorClassifier.IsExpectedUserActionFailure; BindData's onError keeps the seeded title/description + initials-avatar fallback and logs at Debug for an access-denied read, and only surfaces a genuine infra fault (once). 2. Stabilize the tab composition. BuildPage depends only on the node's display name + the isAdmin bool, so project each CombineLatest input down to exactly that and DistinctUntilChanged — an unrelated node version bump or a repeated isAdmin emission no longer recomposes the tab (which is what tore down + re-subscribed the rows). Binding stays live; no .Take(1). Test: AccessThumbnailDeniedFallbackTest (MonolithMeshTestBase + RLS) — a non-member viewer's real subject-stream read is denied, and the fix classifies it as no-surface (no toast). Revert-proven: the ShouldSurfaceStreamError seam does not exist on main. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Components/MeshNodeThumbnailView.razor.cs | 18 ++- .../AccessControlLayoutArea.cs | 28 +++-- .../MeshNodeThumbnailControl.cs | 16 +++ .../AccessThumbnailDeniedFallbackTest.cs | 108 ++++++++++++++++++ 4 files changed, 161 insertions(+), 9 deletions(-) create mode 100644 test/MeshWeaver.AccessControl.Test/AccessThumbnailDeniedFallbackTest.cs diff --git a/src/MeshWeaver.Blazor/Components/MeshNodeThumbnailView.razor.cs b/src/MeshWeaver.Blazor/Components/MeshNodeThumbnailView.razor.cs index 09c5216a7f..665dc28422 100644 --- a/src/MeshWeaver.Blazor/Components/MeshNodeThumbnailView.razor.cs +++ b/src/MeshWeaver.Blazor/Components/MeshNodeThumbnailView.razor.cs @@ -4,6 +4,7 @@ using MeshWeaver.Mesh.Services; using Microsoft.AspNetCore.Components; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; namespace MeshWeaver.Blazor.Components; @@ -61,7 +62,22 @@ protected override void BindData() if (!string.IsNullOrEmpty(img)) ImageUrl = img; InvokeAsync(StateHasChanged); }, - ex => SurfaceError(ex, $"Loading thumbnail for {NodePath}"))); + ex => + { + // #434: a subject the viewer cannot read (a peer's user partition — DENIED BY + // DESIGN) is a benign, expected state, NOT an error. Keep the seeded title / + // description and render the initials-avatar fallback; log at Debug and do NOT + // raise an error toast. Only a genuine infrastructure fault surfaces (once) via + // SurfaceError — the classifier draws the line. + if (!MeshNodeThumbnailControl.ShouldSurfaceStreamError(ex)) + { + Logger.LogDebug(ex, + "Thumbnail subject {NodePath} is not readable by the viewer (access-denied) — rendering fallback card", + NodePath); + return; + } + SurfaceError(ex, $"Loading thumbnail for {NodePath}"); + })); } private void Navigate() diff --git a/src/MeshWeaver.Graph/AccessControlLayoutArea.cs b/src/MeshWeaver.Graph/AccessControlLayoutArea.cs index e090c6bb3a..709eaa3dbd 100644 --- a/src/MeshWeaver.Graph/AccessControlLayoutArea.cs +++ b/src/MeshWeaver.Graph/AccessControlLayoutArea.cs @@ -60,12 +60,24 @@ public static class AccessControlLayoutArea } if (nodeStream is null) - return isAdminStream.Select(isAdmin => (UiControl?)BuildPage(host, node: null, nodePath, isAdmin)); - - return nodeStream.CombineLatest(isAdminStream, (change, isAdmin) - => (UiControl?)BuildPage(host, change?.Value, nodePath, isAdmin)) - .Catch(_ => isAdminStream.Select(isAdmin => - (UiControl?)BuildPage(host, node: null, nodePath, isAdmin))); + return isAdminStream.DistinctUntilChanged() + .Select(isAdmin => (UiControl?)BuildPage(host, nodeName: null, nodePath, isAdmin)); + + // The page's ONLY dependency on the node is its display name (the section header); its only + // dependency on the permission probe is the admin bool. Project each input down to exactly + // that and DistinctUntilChanged, so an unrelated node version bump or a repeated identical + // isAdmin emission does NOT recompose the whole tab. A recompose re-instantiates the + // per-row AccessAssignment thumbnails, which re-subscribes every subject stream — the #434 + // flicker + repeated-toast loop. This keeps the binding LIVE (no .Take(1)); it only + // suppresses no-op re-renders on unrelated upstream emissions. + var nodeNameStream = nodeStream + .Select(change => change?.Value?.Name) + .DistinctUntilChanged(); + + return nodeNameStream.CombineLatest(isAdminStream.DistinctUntilChanged(), (nodeName, isAdmin) + => (UiControl?)BuildPage(host, nodeName, nodePath, isAdmin)) + .Catch(_ => isAdminStream.DistinctUntilChanged().Select(isAdmin => + (UiControl?)BuildPage(host, nodeName: null, nodePath, isAdmin))); } internal static AccessAssignment? DeserializeAssignment(MeshNode node) @@ -77,11 +89,11 @@ public static class AccessControlLayoutArea return null; } - private static UiControl BuildPage(LayoutAreaHost host, MeshNode? node, string nodePath, bool isAdmin) + private static UiControl BuildPage(LayoutAreaHost host, string? nodeName, string nodePath, bool isAdmin) { var stack = Controls.Stack.WithStyle("padding: 24px; gap: 20px; width: 100%;"); - var currentName = node?.Name ?? nodePath.Split('/').LastOrDefault() ?? nodePath; + var currentName = nodeName ?? nodePath.Split('/').LastOrDefault() ?? nodePath; if (string.IsNullOrEmpty(currentName)) currentName = "Root"; stack = stack.WithView(Controls.H2($"Access Control — {currentName}")); diff --git a/src/MeshWeaver.Graph/MeshNodeThumbnailControl.cs b/src/MeshWeaver.Graph/MeshNodeThumbnailControl.cs index cec5811c49..0f423bdf01 100644 --- a/src/MeshWeaver.Graph/MeshNodeThumbnailControl.cs +++ b/src/MeshWeaver.Graph/MeshNodeThumbnailControl.cs @@ -34,6 +34,22 @@ public static MeshNodeThumbnailControl FromNode(MeshNode? node, string fallbackP return new MeshNodeThumbnailControl(nodePath, title, description, imageUrl, nodeType); } + /// + /// Whether a fault raised while streaming this thumbnail's SUBJECT node should be SURFACED as an + /// error (a genuine infrastructure fault → a single toast + log) rather than treated as a benign, + /// expected fallback. + /// + /// An access-denied read is DENIED BY DESIGN here (issue #434): an AccessAssignment row's + /// subject frequently lives in a partition the viewer is not a member of — two users can share access + /// to a node without being able to read each other's user partitions. The row already carries the + /// subject id + roles, so the card renders its seeded initials-avatar fallback; the denial is a normal + /// state, NOT "Something went wrong". Delegates to + /// (true for / access-denied delivery failures), so only a + /// genuine infra fault surfaces. Pure — unit-tested without a renderer or a circuit. + /// + public static bool ShouldSurfaceStreamError(Exception? ex) + => !AreaErrorClassifier.IsExpectedUserActionFailure(ex); + /// /// Gets the image URL for a node. Public so other builders can reuse. /// Priority: content.avatar > content.logo > content.icon > MarkdownContent.Thumbnail > node.Icon > NodeType default. diff --git a/test/MeshWeaver.AccessControl.Test/AccessThumbnailDeniedFallbackTest.cs b/test/MeshWeaver.AccessControl.Test/AccessThumbnailDeniedFallbackTest.cs new file mode 100644 index 0000000000..e355d509ef --- /dev/null +++ b/test/MeshWeaver.AccessControl.Test/AccessThumbnailDeniedFallbackTest.cs @@ -0,0 +1,108 @@ +using System; +using System.Reactive.Linq; +using System.Reactive.Threading.Tasks; +using System.Threading.Tasks; +using MeshWeaver.Data; +using MeshWeaver.Graph; +using MeshWeaver.Hosting.Monolith.TestBase; +using MeshWeaver.Hosting.Security; +using MeshWeaver.Layout; +using MeshWeaver.Mesh; +using MeshWeaver.Mesh.Security; +using MeshWeaver.Messaging; +using Xunit; + +namespace MeshWeaver.AccessControl.Test; + +/// +/// Regression tests for issue #434 — the Access & Control tab's per-assignment subject +/// thumbnails must treat a permission-denied subject read as a BENIGN fallback, not an error +/// toast. +/// +/// Each AccessAssignment row renders a bound +/// to the SUBJECT's node path. The subject frequently lives in a partition the viewer is not a +/// member of (two users can share access to a node without being able to read each other's user +/// partitions), so the RLS gate denies the read BY DESIGN. Before the fix, +/// MeshNodeThumbnailView.BindData forwarded that denial straight to SurfaceError → +/// a log line + a PortalErrorSink.Report modal + a StateHasChanged, once per row +/// per recompose — the repeated toasts + flicker in the issue. The fix routes the onError through +/// : an access-denied read renders +/// the seeded initials-avatar fallback and logs at Debug, and only a genuine infra fault surfaces. +/// +public class AccessThumbnailDeniedFallbackTest(ITestOutputHelper output) : MonolithMeshTestBase(output) +{ + protected override MeshBuilder ConfigureMesh(MeshBuilder builder) + => ConfigureMeshBase(builder) + .AddRowLevelSecurity() + .AddMeshNodes( + new MeshNode("PeerHome") { Name = "Peer Home" }, + new MeshNode("PeerHome/Subject") { Name = "Peer Subject" }) + .ConfigureDefaultNodeHub(c => c.AddData(d => d)); + + private async Task EnsureHubStarted(Address address) + { + var client = GetClient(); + await client.Observe(new PingRequest(), o => o.WithTarget(address)).Should().Emit(); + } + + /// + /// Integration (RLS, real denial): a non-member viewer subscribes to the subject's node stream — + /// exactly what MeshNodeThumbnailView.BindData does — and the read is denied. The fix + /// classifies that real as a benign, expected fallback: + /// is false, so NO error + /// toast / SurfaceError fires. Revert-proven: on current main BindData surfaces the denial + /// unconditionally, and the ShouldSurfaceStreamError seam does not exist. + /// + [Fact(Timeout = 20000)] + public async Task DeniedSubjectRead_IsClassifiedBenign_NoToast() + { + // Admin seeds the subject; then switch to a viewer who is NOT a member of its partition. + await EnsureHubStarted(new Address("PeerHome/Subject")); + TestUsers.DevLogin(Mesh, new AccessContext { ObjectId = "viewer-bob", Name = "Bob" }); + + Exception? surfaced = null; + MeshNode? emitted = null; + try + { + emitted = await Mesh.GetMeshNodeStream("PeerHome/Subject") + .Where(n => n is not null) + .FirstAsync() + .Timeout(10.Seconds()) + .ToTask(); + } + catch (Exception ex) + { + surfaced = ex; + Output.WriteLine($"Surfaced: {ex.GetType().Name}: {ex.Message}"); + } + + emitted.Should().BeNull("a denied subject read must not emit a node value"); + surfaced.Should().NotBeNull( + "reading a subject in a partition the viewer isn't a member of is denied by design — " + + "this is the toast source in #434"); + surfaced.Should().NotBeOfType( + "the denial must surface as an access error, not hang to a timeout"); + + // The #434 root-cause fix — a denied subject read is BENIGN, never an error toast. + MeshNodeThumbnailControl.ShouldSurfaceStreamError(surfaced).Should().BeFalse( + "an access-denied subject read is an expected user-action failure — the thumbnail renders " + + "its seeded fallback card, so BindData must NOT call SurfaceError (no log/PortalErrorSink/StateHasChanged)"); + AreaErrorClassifier.IsExpectedUserActionFailure(surfaced).Should().BeTrue( + "the real RLS denial the mesh throws must classify as a benign, expected user-action failure"); + } + + /// + /// Pure boundary test of the decision seam: access-denied is swallowed (benign fallback), + /// a genuine infrastructure fault surfaces once. + /// + [Fact] + public void ShouldSurfaceStreamError_SwallowsAccessDenied_SurfacesInfraFaults() + { + MeshNodeThumbnailControl.ShouldSurfaceStreamError( + new UnauthorizedAccessException("User 'bob' lacks Read permission on 'PeerHome/Subject'")) + .Should().BeFalse("access-denied is denied-by-design — render the fallback, no toast"); + + MeshNodeThumbnailControl.ShouldSurfaceStreamError(new InvalidOperationException("boom")) + .Should().BeTrue("a genuine infrastructure fault must surface once"); + } +}