fix(archreview-p1): SEC-02/12/20 dashboard + observability hardening

- SEC-02: DashboardAuthorizationHandler restricts the loopback and Authentication:Mode=Disabled bypasses to read-only. They now satisfy only a Viewer-bearing requirement (AnyDashboardRole), never AdminOnly, so anonymous localhost can view the dashboard but cannot reach API-key CRUD or session Close/Kill at the policy layer (previously guarded only by service re-checks). - SEC-12: DashboardSessionAdminService emits canonical AuditEvents through IAuditWriter (actions dashboard-close-session / dashboard-kill-worker, category SessionAdmin) on Success/Failure/Denied, mirroring the API-key audit path so destructive session actions leave durable, queryable rows. - SEC-20: drop the unbounded session_id tag from the exported mxgateway.heartbeats.failed counter (per-session detail stays in the snapshot/log). Docs updated same-change: CLAUDE.md (read-only loopback + 5-min bearer), GatewayDashboardDesign.md (bypass scoping + session-admin audit), Metrics.md. Server build clean; 30/30 targeted + 295/295 Dashboard/Security/App/Metrics sweep.
2026-07-09 07:47:51 -04:00
parent c2df4a0aae
commit 74e815c4d7
10 changed files with 347 additions and 20 deletions
@@ -128,11 +128,58 @@ public sealed class DashboardAuthorizationHandlerTests
        Assert.False(context.HasSucceeded);
    }

+    /// <summary>
+    /// Verifies that the anonymous-localhost bypass is read-only: a loopback request with
+    /// <c>AllowAnonymousLocalhost</c> on is denied the <see cref="DashboardAuthorizationRequirement.AdminOnly"/>
+    /// requirement, so destructive/admin surfaces are never reachable without a real Admin role.
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task HandleAsync_AnonymousLocalhostAllowed_DoesNotSatisfyAdminPolicy()
+    {
+        AuthorizationHandlerContext context = await AuthorizeAsync(
+            new ClaimsPrincipal(new ClaimsIdentity()),
+            IPAddress.Loopback,
+            allowAnonymousLocalhost: true,
+            DashboardAuthorizationRequirement.AdminOnly);
+
+        Assert.False(context.HasSucceeded);
+    }
+
+    /// <summary>
+    /// Verifies that with authentication fully disabled the environment bypass stays read-only:
+    /// a remote request satisfies <see cref="DashboardAuthorizationRequirement.AnyDashboardRole"/>
+    /// (Viewer) but is denied <see cref="DashboardAuthorizationRequirement.AdminOnly"/>.
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task HandleAsync_AuthenticationDisabled_GrantsViewerButNotAdmin()
+    {
+        AuthorizationHandlerContext viewerContext = await AuthorizeAsync(
+            new ClaimsPrincipal(new ClaimsIdentity()),
+            IPAddress.Parse("10.0.0.5"),
+            allowAnonymousLocalhost: false,
+            DashboardAuthorizationRequirement.AnyDashboardRole,
+            authenticationMode: AuthenticationMode.Disabled);
+
+        Assert.True(viewerContext.HasSucceeded);
+
+        AuthorizationHandlerContext adminContext = await AuthorizeAsync(
+            new ClaimsPrincipal(new ClaimsIdentity()),
+            IPAddress.Parse("10.0.0.5"),
+            allowAnonymousLocalhost: false,
+            DashboardAuthorizationRequirement.AdminOnly,
+            authenticationMode: AuthenticationMode.Disabled);
+
+        Assert.False(adminContext.HasSucceeded);
+    }
+
    private static async Task<AuthorizationHandlerContext> AuthorizeAsync(
        ClaimsPrincipal principal,
        IPAddress remoteAddress,
        bool allowAnonymousLocalhost,
-        DashboardAuthorizationRequirement requirement)
+        DashboardAuthorizationRequirement requirement,
+        AuthenticationMode authenticationMode = AuthenticationMode.ApiKey)
    {
        DefaultHttpContext httpContext = new();
        httpContext.Connection.RemoteIpAddress = remoteAddress;
@@ -140,6 +187,10 @@ public sealed class DashboardAuthorizationHandlerTests
            new HttpContextAccessor { HttpContext = httpContext },
            Options.Create(new GatewayOptions
            {
+                Authentication = new AuthenticationOptions
+                {
+                    Mode = authenticationMode,
+                },
                Dashboard = new DashboardOptions
                {
                    AllowAnonymousLocalhost = allowAnonymousLocalhost,
@@ -1,6 +1,8 @@
+using System.Collections.Concurrent;
 using System.Diagnostics.CodeAnalysis;
 using System.Security.Claims;
 using Microsoft.AspNetCore.Http;
+using ZB.MOM.WW.Audit;
 using ZB.MOM.WW.MxGateway.Contracts.Proto;
 using ZB.MOM.WW.MxGateway.Server.Dashboard;
 using ZB.MOM.WW.MxGateway.Server.Sessions;
@@ -198,14 +200,91 @@ public sealed class DashboardSessionAdminServiceTests
        Assert.False(string.IsNullOrWhiteSpace(result.Message));
    }

-    private static DashboardSessionAdminService CreateService(ISessionManager sessionManager)
+    /// <summary>
+    /// Verifies that a successful close writes a canonical <c>dashboard-close-session</c>
+    /// <see cref="AuditEvent"/> — actor, session-id target, and Success outcome — to the
+    /// audit store, not only the operational log (SEC-12).
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task CloseSessionAsync_AdminClose_WritesCanonicalAuditEvent()
+    {
+        FakeSessionManager sessionManager = new();
+        RecordingAuditWriter auditWriter = new();
+        DashboardSessionAdminService service = CreateService(sessionManager, auditWriter);
+
+        DashboardSessionAdminResult result = await service.CloseSessionAsync(
+            CreateUser(DashboardRoles.Admin),
+            "session-1",
+            CancellationToken.None);
+
+        Assert.True(result.Succeeded);
+        AuditEvent audit = Assert.Single(auditWriter.Events);
+        Assert.Equal(DashboardSessionAdminService.CloseSessionAction, audit.Action);
+        Assert.Equal(DashboardSessionAdminService.SessionAdminCategory, audit.Category);
+        Assert.Equal("session-1", audit.Target);
+        Assert.Equal("tester", audit.Actor);
+        Assert.Equal(AuditOutcome.Success, audit.Outcome);
+    }
+
+    /// <summary>
+    /// Verifies that a successful kill writes a canonical <c>dashboard-kill-worker</c>
+    /// <see cref="AuditEvent"/> to the audit store (SEC-12).
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task KillWorkerAsync_AdminKill_WritesCanonicalAuditEvent()
+    {
+        FakeSessionManager sessionManager = new();
+        RecordingAuditWriter auditWriter = new();
+        DashboardSessionAdminService service = CreateService(sessionManager, auditWriter);
+
+        DashboardSessionAdminResult result = await service.KillWorkerAsync(
+            CreateUser(DashboardRoles.Admin),
+            "session-1",
+            CancellationToken.None);
+
+        Assert.True(result.Succeeded);
+        AuditEvent audit = Assert.Single(auditWriter.Events);
+        Assert.Equal(DashboardSessionAdminService.KillWorkerAction, audit.Action);
+        Assert.Equal("session-1", audit.Target);
+        Assert.Equal(AuditOutcome.Success, audit.Outcome);
+    }
+
+    /// <summary>
+    /// Verifies that an unauthorized (viewer) close attempt still writes a <c>Denied</c>
+    /// audit row, so rejected destructive attempts are durably recorded (SEC-12).
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task CloseSessionAsync_ViewerDenied_WritesDeniedAuditEvent()
+    {
+        FakeSessionManager sessionManager = new();
+        RecordingAuditWriter auditWriter = new();
+        DashboardSessionAdminService service = CreateService(sessionManager, auditWriter);
+
+        DashboardSessionAdminResult result = await service.CloseSessionAsync(
+            CreateUser(DashboardRoles.Viewer),
+            "session-1",
+            CancellationToken.None);
+
+        Assert.False(result.Succeeded);
+        AuditEvent audit = Assert.Single(auditWriter.Events);
+        Assert.Equal(DashboardSessionAdminService.CloseSessionAction, audit.Action);
+        Assert.Equal(AuditOutcome.Denied, audit.Outcome);
+    }
+
+    private static DashboardSessionAdminService CreateService(
+        ISessionManager sessionManager,
+        IAuditWriter? auditWriter = null)
    {
        DefaultHttpContext httpContext = new();
        httpContext.Connection.RemoteIpAddress = System.Net.IPAddress.Loopback;

        return new DashboardSessionAdminService(
            sessionManager,
-            new HttpContextAccessor { HttpContext = httpContext });
+            new HttpContextAccessor { HttpContext = httpContext },
+            auditWriter ?? new RecordingAuditWriter());
    }

    private static ClaimsPrincipal CreateUser(string role)
@@ -219,6 +298,21 @@ public sealed class DashboardSessionAdminServiceTests
        return new ClaimsPrincipal(identity);
    }

+    private sealed class RecordingAuditWriter : IAuditWriter
+    {
+        private readonly ConcurrentQueue<AuditEvent> _events = new();
+
+        /// <summary>Gets the audit events written through this writer, in order.</summary>
+        public IReadOnlyList<AuditEvent> Events => _events.ToArray();
+
+        /// <inheritdoc />
+        public Task WriteAsync(AuditEvent evt, CancellationToken ct = default)
+        {
+            _events.Enqueue(evt);
+            return Task.CompletedTask;
+        }
+    }
+
    private sealed class FakeSessionManager : ISessionManager
    {
        /// <summary>Gets the number of times CloseSessionAsync was invoked.</summary>
@@ -157,6 +157,56 @@ public sealed class GatewayMetricsTests
        Assert.Equal(2, capturedMode);
    }

+    /// <summary>
+    /// Verifies that <see cref="GatewayMetrics.HeartbeatFailed"/> increments
+    /// <c>mxgateway.heartbeats.failed</c> without emitting a <c>session_id</c> tag: the tag is
+    /// unbounded cardinality (every session mints a new exporter time series), so per-session
+    /// attribution is deliberately kept out of the exported counter (SEC-20).
+    /// </summary>
+    [Fact]
+    public void HeartbeatFailed_IncrementsCounterWithoutSessionIdTag()
+    {
+        using GatewayMetrics metrics = new();
+        using MeterListener listener = new();
+
+        long capturedValue = 0;
+        bool sawSessionIdTag = false;
+
+        listener.InstrumentPublished = (instrument, meterListener) =>
+        {
+            if (ReferenceEquals(instrument.Meter, metrics.Meter)
+                && instrument.Name == "mxgateway.heartbeats.failed")
+            {
+                meterListener.EnableMeasurementEvents(instrument);
+            }
+        };
+        listener.SetMeasurementEventCallback<long>(
+            (instrument, measurement, tags, _) =>
+            {
+                if (!ReferenceEquals(instrument.Meter, metrics.Meter)
+                    || instrument.Name != "mxgateway.heartbeats.failed")
+                {
+                    return;
+                }
+
+                capturedValue += measurement;
+                foreach (KeyValuePair<string, object?> tag in tags)
+                {
+                    if (tag.Key == "session_id")
+                    {
+                        sawSessionIdTag = true;
+                    }
+                }
+            });
+        listener.Start();
+
+        metrics.HeartbeatFailed("session-1");
+
+        Assert.Equal(1, capturedValue);
+        Assert.False(sawSessionIdTag);
+        Assert.Equal(1, metrics.GetSnapshot().HeartbeatFailures);
+    }
+
    /// <summary>Verifies that removing session events only affects that session.</summary>
    [Fact]
    public void RemoveSessionEvents_RemovesOnlyThatSession()