From b621d692d0766d773035ef41c6929448dde04f97 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 17 Aug 2026 05:23:23 -0400 Subject: [PATCH] =?UTF-8?q?docs+test(closeout):=20final-review=20reservati?= =?UTF-8?q?ons=20=E2=80=94=20stale=20ACL=20prose,=20worker=20test=20gaps,?= =?UTF-8?q?=20config=20sample=20fix?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- archreview/remediation/00-tracking.md | 1 + docs/Authorization.md | 17 +++++-- docs/DesignDecisions.md | 20 +++++--- docs/GatewayConfiguration.md | 4 +- docs/GatewayDashboardDesign.md | 6 ++- docs/GatewayProcessDesign.md | 2 +- docs/Sessions.md | 2 +- .../2026-07-10-dashboard-session-acl-tst15.md | 3 +- docs/plans/2026-08-17-deferred-closeout.md | 2 +- ...2026-08-17-deferred-closeout.md.tasks.json | 2 +- .../Alarms/IGatewayAlarmService.cs | 16 ++++-- .../Hubs/DashboardEventBroadcaster.cs | 9 ++-- .../Dashboard/IDashboardSessionAcl.cs | 9 ++-- .../Ipc/WorkerFrameProtocolTests.cs | 50 +++++++++++++++---- .../MxAccess/FailoverAlarmConsumerTests.cs | 38 ++++++++++++++ .../Ipc/WorkerPipeSession.cs | 21 +++++--- .../MxAccess/IAlarmCommandHandler.cs | 9 +++- stillpending.md | 2 +- 18 files changed, 167 insertions(+), 46 deletions(-) diff --git a/archreview/remediation/00-tracking.md b/archreview/remediation/00-tracking.md index f13be4a..1bbeec2 100644 --- a/archreview/remediation/00-tracking.md +++ b/archreview/remediation/00-tracking.md @@ -253,6 +253,7 @@ Findings the review flagged as one coordinated design pass — sequence them tog | Date | Change | |---|---| +| 2026-08-17 | **Alarm-snapshot truncation now has a structural degraded-status signal** (branch `feat/deferred-closeout`, commits `693a78d` + `b9fb0dd`). No review ID — this is branch work outside the 153-finding register, recorded here so the tracker is not silent on a shipped change to the alarm surface. Before it, a capped `GetXmlCurrentAlarms2` fetch suppressed absence-implies-Clear inference and said so only in a rate-limited worker stderr warning, so no client and no operator could tell a complete active set from a capped one. Two additive proto3 booleans carry the verdict out — `QueryActiveAlarmsReplyPayload.snapshot_truncated = 2` and `ActiveAlarmSnapshot.from_truncated_snapshot = 16` (per record, because `QueryActiveAlarms` is a bare `stream ActiveAlarmSnapshot` with no envelope; the reply payload states it too, since a prefix filter can leave zero records and a capped fetch with nothing to report still has to say so). Flow: `WnWrapAlarmConsumer` → `AlarmDispatcher` / `IAlarmCommandHandler` → `MxAccessCommandExecutor` reply → `GatewayAlarmMonitor` → `IGatewayAlarmService.SnapshotTruncated` → `AlarmsPage` banner. `b9fb0dd` then made the pairing structural after review: `IMxAccessAlarmConsumer` and `IAlarmCommandHandler` expose one accessor (`SnapshotActiveAlarms(out bool truncated)` / `QueryActive(..., out bool snapshotTruncated)`) satisfied from a single lock acquisition, so the snapshot and its verdict can no longer be read across a poll; the separate `LastSnapshotTruncated` property is gone from every layer. Detection is deliberately unchanged (`fetchedRecordCount >= maxAlarmsPerFetch`); switching to `ALARM_RECORDS/@COUNT` stays blocked on probe evidence (`docs/AlarmProbeFindings.md`). Not latched, and dropped with the cache generation by `ClearCache`. Additive gateway metadata about our fetch mechanics, not MXAccess behaviour — no synthesized event, so not a parity deviation. Docs: `gateway.md` alarm surface, `docs/DesignDecisions.md`. | | 2026-08-17 | **TST-15 → `Done` (discharges the ACL half of SEC-25): per-session dashboard event ACL shipped** (branch `feat/deferred-closeout`, commits `693a78d` + `7ec0b35`). Implements `docs/plans/2026-07-10-dashboard-session-acl-tst15.md`, whose header is now `Implemented` with as-built notes in its §12. `IDashboardSessionAcl.CanViewSession` is the single decision **both** subscribe seams consult — `EventsHub.SubscribeSession` (denial is a `HubException`; the caller is neither joined to the group nor registered in `EventsHubViewerRegistry`, so the mirror stays off) and `SessionDetailsPage`'s in-process subscribe (inline denial, no subscription) — so neither path is the weaker one and the `TODO(per-session-acl)` is gone. Decision order, fail-closed on every branch: authenticated Administrator → allow (evaluated **before** the registry lookup, so Admin × unknown-session allows — pinned by a test because reordering the two checks is a plausible refactor); session not found → deny; untagged session → `Dashboard:UntaggedSessionVisibility` (`AdminOnly` default); else allow iff `session.Tags ∩ zb:dashboardtag` claims, ordinal-ignore-case. Session tags are inherited from the owning API key's `dashboard_tags` constraint (`apikey --dashboard-tags`, already in the `ApiKeyConstraints` JSON blob — no SQLite migration) and never from the client's wire request. Viewer grants come from `Dashboard:GroupToTag` over the user's LDAP groups, stamped at cookie login (`DashboardAuthenticator.CreatePrincipal`) and **re-resolved, not copied**, at hub-token mint (`HubTokenService.Issue`), so the 5-minute token lifetime bounds a stale grant. Anonymous localhost is an empty-grant Viewer; `Dashboard:DisableLogin` auto-login carries both roles and so takes the admin bypass unchanged. Tests: `DashboardSessionAclTests` (decision table, every branch asserted in its denying direction too), `EventsHubTests`, `DashboardAuthenticatorTests`, `HubTokenServiceTests`, a `GatewayOptionsTests` case proving `Dashboard:GroupToTag` keeps its ordinal-ignore-case lookup through configuration binding, and two `[LiveLdapFact]`s in `DashboardLdapLiveTests` that drive a real bind against the shared GLAuth (`gw-viewer` → `team-a` grant admits the `team-a` session and refuses the `team-b` one; `multi-role` bypasses on the sessions its own grant does not cover). The live pair needed **no GLAuth change** — the tag layer is config-side, keyed on the existing `GwAdmin`/`GwReader` groups (recorded in `glauth.md`). Docs: `docs/Sessions.md` (session-tag model), `gateway.md` + CLAUDE.md dashboard-auth paragraphs, `docs/GatewayDashboardDesign.md` (three passages that described the ACL as outstanding), `docs/GatewayConfiguration.md` (`ShowTagValues` row: redaction is now the second of two layers, not the only one), `docs/Authentication.md` (`--dashboard-tags` is the only *constraint* flag that splits on commas). | | 2026-08-10 | **TST-25 acceptance Check 6 (forced-failure nightly issue) → Done.** The 2026-07-13 record wrote this check off as "abandoned to shared-runner congestion"; that was wrong on both counts. The 2026-07-13 probe *did* land (issue #125, `[CHECK6 PROBE]`, run 375), and since 2026-07-17 the `nightly-windev` `if: failure()` step has filed an issue on **every** red nightly — #126–#139, all authored by the `gitea-actions` bot. Traced run 672 (schedule, main, red) line by line: main step fails → `exitcode '1': failure` → the `if: failure()` step runs → `POST /api/v1/repos/dohertj2/mxaccessgw/issues` with the built-in token masked to `***` → issue #139 created at the matching timestamp. Re-confirmed by a fresh forced-failure probe on the throwaway branch `test/tst25-check6-nightly-issue` (temporary `tst25-check6-probe.yml` reproducing the job shape with `exit 1` for the live step; run 677 → issue #140). Branch deleted, issues #125 and #140 closed with explanatory comments. **One real defect found and fixed** (`fix/tst25-nightly-issue-path`, not merged): `${{ github.server_url }}` is the runner-internal `http://gitea:3000`, so every filed issue's run link was unreachable from a browser. The API call must keep using it (the job container resolves `gitea` only on the docker network and has no LAN egress to the public origin), so the fix adds a `PUBLIC_SERVER_URL: https://gitea.dohertylan.com` job env used **only** for the browser-facing link in the issue body; the probe validated the fixed template (#140 carries a `https://gitea.dohertylan.com/...` link that returns 200). **Separately observed, not fixed:** the nightly has been red continuously since at least 2026-07-17 (run 672: `x86 Worker.Tests failed with exit code 1`, 1 failed / 398 passed / 11 skipped — the known `EventBurst_DrainLoopCoalescesFlushes` class of flake), and the step de-duplicates nothing, so 14 issues are open, seven of them (#132–#138) for the identical SHA `47c0b64`. Worth a follow-up: fix the red nightly, and consider having the step reuse an open issue with the same title instead of filing a new one. | | 2026-08-10 | **TST-24 → `Done`: per-client wire tests land for the two clients that lacked them** (branch `feat/tst-24-client-wire-tests`). Audit first corrected the finding's premise: **Go, Rust, and Java already had real-server wire tests** — `newBufconnClient`/`fakeGatewayServer` over `grpc/test/bufconn`, `spawn_fake_gateway` over a loopback `TcpListener` with tonic's `Server`, and `InProcessGateway`/`TestGatewayService` over `InProcessServerBuilder` — each already asserting the round trip, the server-observed `authorization` bearer header, and the `ReplayGap` sentinel. The real gaps were **.NET** (every test substituted `FakeGatewayTransport` for `IMxGatewayClientTransport`, and the test project had no server package) and **Python** (stub monkeypatching everywhere except one opt-in TLS test serving only `OpenSession`). Added `WireFakeGatewayServer` + `MxGatewayClientWireTests` (Kestrel h2c on `127.0.0.1:0` serving `MxAccessGatewayBase`; new `Grpc.AspNetCore.Server` 2.76.0 + `Microsoft.AspNetCore.App` refs on the test project) and `clients/python/tests/test_wire_fake_gateway.py` (`grpc.aio` server on `127.0.0.1:0`, no new deps). Four shapes each: full round trip with every reply field asserted, the bearer header **as received by the server** on the streaming RPC too, the `ReplayGap` sentinel surfaced as the client's typed signal, and a genuine `PERMISSION_DENIED` mapping to the typed authorization error. CI: the `portable` job only *built* the .NET client, so a `dotnet test` step was added. **The new tests immediately caught a shipped bug** — Python `GatewayClient.connect()`/`GalaxyRepositoryClient.connect()` constructed the `grpc.aio` channel inside `asyncio.to_thread`, which raises `RuntimeError: There is no current event loop in thread 'asyncio_0'` because a `grpc.aio` channel binds to the loop current on the constructing thread; every non-stub connection failed, and the one test guarding the off-loop behaviour (Client.Python-028) monkeypatched `create_channel` and so asserted the bug. Fixed by splitting `resolve_channel_security` (blocking TOFU probe, off-loop) from `create_channel` (on-loop), with the `-028` tests retargeted to assert both halves. Verified: .NET 133 passed/1 skipped (pre-existing live-gateway skip), Python 168 passed/1 skipped plus 6/6 opt-in TLS. Docs: `docs/GatewayTesting.md` § Client Wire Tests, `clients/dotnet/README.md`, `clients/python/README.md`. | diff --git a/docs/Authorization.md b/docs/Authorization.md index 2762fa5..bc4e7c9 100644 --- a/docs/Authorization.md +++ b/docs/Authorization.md @@ -198,10 +198,19 @@ Tags are set at key creation with trimmed and de-duplicated ordinal-ignore-case). Keys created from the dashboard API Keys page are currently always untagged. -The tag is carried end to end today; the dashboard ACL that consumes it — scoping -a Viewer's `EventsHub` subscriptions to the sessions their LDAP groups are -granted — is a separate change. Until it lands, the tag affects nothing at -runtime. +The dashboard ACL that consumes the tag shipped on 2026-08-17 (SEC-25 / TST-15). +`IDashboardSessionAcl.CanViewSession` is consulted at both dashboard subscribe +seams — the SignalR `EventsHub.SubscribeSession` join and the in-process +`IDashboardSessionEventSubscriber.Subscribe` behind the session-details page — so +per-session event visibility is enforced at runtime: a Viewer observes a session +only when the session's tags intersect the tags their LDAP groups are granted +through `MxGateway:Dashboard:GroupToTag`. Administrators bypass the intersection, +and a session with no tags is visible to Administrators only unless +`MxGateway:Dashboard:UntaggedSessionVisibility` is set to `AllViewers`. + +That enforcement is still *visibility*, not data access. The ACL decides which +sessions' mirrored events a dashboard principal may observe; it does not widen or +narrow what any API key may read, write, browse, or subscribe to over gRPC. Glob matching is anchored, case-insensitive, and supports `*` and `?`. Subtree and tag glob lists are alternatives: matching either list allows that diff --git a/docs/DesignDecisions.md b/docs/DesignDecisions.md index 6b60257..f3fd10b 100644 --- a/docs/DesignDecisions.md +++ b/docs/DesignDecisions.md @@ -256,11 +256,19 @@ decisions rather than one open backlog. shipped as **TST-01** (`GatewayEndToEndReconnectReplayTests`). Task 14 (client `ReplayGap` handling) shipped as **CLI-15** for four of five clients (.NET/Go/Rust/Python); the Java client is the only remainder. -- **Phase 4 (per-session dashboard ACL)** — scoped, not yet built. Tracked as archreview - **TST-15**. The Viewer-default decision is settled: admin-sees-all, Viewer strictly - scoped to sessions it owns or is granted — matching the gRPC owner-binding decision in - [Session Reconnect](#session-reconnect) above, for consistency between the gRPC and - dashboard surfaces. +- **Phase 4 (per-session dashboard ACL)** — shipped 2026-08-17 (branch + `feat/deferred-closeout`), tracked as archreview **TST-15**. The Viewer default is + admin-sees-all, Viewer strictly scoped — but the scope is the session **tag**, not + session ownership. The dashboard authenticates LDAP users while sessions are owned by + API keys, two disjoint identity domains, so there is no "sessions it owns" branch to + write: `GatewaySession.Tags` is inherited from the owning key's `DashboardTags`, a + dashboard group grants tags via `MxGateway:Dashboard:GroupToTag`, and + `IDashboardSessionAcl.CanViewSession` allows a Viewer iff the two sets intersect. + Administrators bypass the intersection; an untagged session is Admin-only under the + default `MxGateway:Dashboard:UntaggedSessionVisibility=AdminOnly`. The decision is + taken at both subscribe seams (`EventsHub.SubscribeSession` and the in-process + `IDashboardSessionEventSubscriber.Subscribe`), never per event. See + `docs/plans/2026-07-10-dashboard-session-acl-tst15.md` and `docs/Authorization.md`. - **Phase 5 (orphan-worker reattach)** — deferred, not planned. It would reverse the "Gateway restart does not reattach orphan workers" invariant (see CLAUDE.md), adding a stable gateway-instance id, an adoption-manifest SQLite store, a worker phone-home @@ -270,7 +278,7 @@ decisions rather than one open backlog. if it does** until that task actually lands. `docs/plans/2026-06-15-session-resilience.md.tasks.json` remains the sole resume state -for the still-pending Phase 4 tasks (16-19) and the deferred Phase 5 tasks (20-28) — one +for the Phase 4 tasks (16-19, now shipped) and the deferred Phase 5 tasks (20-28) — one authority, no mirror. ## Authentication diff --git a/docs/GatewayConfiguration.md b/docs/GatewayConfiguration.md index e99ee78..0eb0d41 100644 --- a/docs/GatewayConfiguration.md +++ b/docs/GatewayConfiguration.md @@ -58,7 +58,7 @@ paths, timeouts, queue sizes, enum values, or protocol values are invalid. "RecentSessionLimit": 200, "ShowTagValues": false, "GroupToRole": { - "GwAdmin": "Admin", + "GwAdmin": "Administrator", "GwReader": "Viewer" }, "GroupToTag": { @@ -193,7 +193,7 @@ events (a "gap") and must re-snapshot; whatever is still retained is replayed. | `MxGateway:Dashboard:RecentFaultLimit` | `100` | Maximum number of fault summaries projected into each dashboard snapshot. | | `MxGateway:Dashboard:RecentSessionLimit` | `200` | Maximum number of session summaries projected into each dashboard snapshot. | | `MxGateway:Dashboard:ShowTagValues` | `false` | Controls whether tag values reach the dashboard's SignalR events hub mirror. `false` (default): `DashboardEventBroadcaster` blanks tag values from a deep-cloned copy of each `MxEvent` before it reaches any hub subscriber — event metadata (tag reference, quality, status, timestamps) still renders; see `docs/GatewayDashboardDesign.md`'s `EventsHub` row for the mechanism. This is now the second of two independent layers, not the only one: `IDashboardSessionAcl` decides *which* sessions a caller may subscribe to at all (see `GroupToTag` / `UntaggedSessionVisibility` below), while this flag decides what a permitted subscriber sees. Setting it `true` therefore exposes tag values to everyone the ACL admits — every Administrator, plus each Viewer holding a matching tag. The flag gates only the SignalR hub mirror — it does **not** cover the `/browse` live-value display, which remains a separate, still-open residual. | -| `MxGateway:Dashboard:GroupToRole` | _(empty)_ | LDAP group → dashboard role mapping. Keys are LDAP group names (short CN or full DN — leading-RDN match). Values must be `Admin` (read/write, API-key CRUD) or `Viewer` (read-only). A user whose LDAP groups don't intersect this map cannot sign in; with no mapping at all, only the loopback bypass admits anyone. | +| `MxGateway:Dashboard:GroupToRole` | _(empty)_ | LDAP group → dashboard role mapping. Keys are LDAP group names (short CN or full DN — leading-RDN match). Values must be `Administrator` (read/write, API-key CRUD) or `Viewer` (read-only) — matched ordinally by the startup validator, so the spelling is exact and `Admin` is rejected. A user whose LDAP groups don't intersect this map cannot sign in; with no mapping at all, only the loopback bypass admits anyone. | | `MxGateway:Dashboard:GroupToTag` | _(empty)_ | LDAP group → dashboard visibility tags. Keys follow the same convention as `GroupToRole` (short CN or full DN — leading-RDN match, case-insensitive); values are tag lists. A dashboard user's granted tag set is the union over the groups they belong to; an unmapped group contributes nothing. **Visibility only:** tags scope which sessions' event streams a Viewer may observe on the dashboard — they never grant or deny data access, which stays with the API key's scopes and constraints. Independent of `GroupToRole`: a group may appear in either map, both, or neither. Empty (the default) means Viewers hold no tags, so under the default `UntaggedSessionVisibility` they observe no session's events. | | `MxGateway:Dashboard:UntaggedSessionVisibility` | `AdminOnly` | Who may observe a session that carries no tags (its owning API key declared none). `AdminOnly` (default, fail-closed) restricts untagged sessions to dashboard Administrators. `AllViewers` shows them to every Viewer — opt-in for a single-tenant deployment that wants the pre-tag behaviour. Administrators always see every session regardless of tags. | | `MxGateway:Dashboard:DisableLogin` | `false` | Dev/test only. When `true`, replaces the cookie authentication handler with `DashboardAutoLoginAuthenticationHandler`, which auto-authenticates every dashboard request — including requests from remote browsers, not just loopback — as `AutoLoginUser` holding both `Administrator` and `Viewer` roles. No login form, LDAP bind, or cookie is involved. A loud one-time startup warning is logged. Differs from `AllowAnonymousLocalhost`: `DisableLogin` mints a real authenticated principal (so role-gated write affordances appear), whereas `AllowAnonymousLocalhost` satisfies the authorization requirement on loopback only without minting a principal (write affordances stay hidden). Never enable in production. **Production hard-stop (SEC-04):** when the host runs in the `Production` environment and `DisableLogin` is `true`, startup validation fails and the process aborts — the flag is only accepted outside Production, where the one-time startup warning still fires. | diff --git a/docs/GatewayDashboardDesign.md b/docs/GatewayDashboardDesign.md index 6ddce40..572d70a 100644 --- a/docs/GatewayDashboardDesign.md +++ b/docs/GatewayDashboardDesign.md @@ -801,7 +801,11 @@ the deliberate mitigation: it bounds that exposure window without the cost of a revocation store. It now bounds a stale *tag* grant the same way: the token carries the tags resolved from the caller's LDAP groups at mint time, so removing a `GroupToTag` entry takes effect for token-authenticated hub connections within one -lifetime. That is where the per-session ACL's revocation need landed — a jti +lifetime. That 5-minute staleness bound covers token-authenticated connections only: +a cookie principal carries the `zb:dashboardtag` claims stamped at login for the +cookie's whole life, so for cookie-authenticated (in-process page) subscriptions a +revoked `GroupToTag` grant takes effect at the user's next login, not within five +minutes. That is where the per-session ACL's revocation need landed — a jti denylist stays deferred, since the short lifetime already bounds every grant the token carries. diff --git a/docs/GatewayProcessDesign.md b/docs/GatewayProcessDesign.md index 194800b..0fe7cd9 100644 --- a/docs/GatewayProcessDesign.md +++ b/docs/GatewayProcessDesign.md @@ -751,7 +751,7 @@ secure, and strict SameSite. It is named `__Host-MxGatewayDashboard` when `MxGateway:Dashboard:CookieName` override is set; otherwise it falls back to the plain `MxGatewayDashboard` name (the `__Host-` prefix requires a Secure cookie). Logout clears it. Login and logout posts validate antiforgery tokens. SignalR -connections additionally accept a 30-minute data-protected bearer minted at +connections additionally accept a 5-minute data-protected bearer minted at `/hubs/token`. `Dashboard:AllowAnonymousLocalhost` permits loopback requests to bypass the cookie requirement and defaults to `true`. diff --git a/docs/Sessions.md b/docs/Sessions.md index c582a9b..77f8621 100644 --- a/docs/Sessions.md +++ b/docs/Sessions.md @@ -6,7 +6,7 @@ The sessions subsystem owns the in-memory representation of an active gateway-to A session is the gateway-side handle that callers use to invoke worker commands, stream worker events, and tear the worker down. The subsystem is split between the per-session state machine (`GatewaySession`), an in-memory directory (`SessionRegistry`), the orchestrator that opens and closes sessions (`SessionManager`), the worker construction step (`SessionWorkerClientFactory`), and a hosted service that drains sessions during host shutdown (`SessionShutdownHostedService`). -All four interfaces (`ISessionManager`, `ISessionRegistry`, `ISessionWorkerClientFactory`) plus `SessionShutdownHostedService` are wired as singletons by `SessionServiceCollectionExtensions.AddGatewaySessions`. +All three interfaces (`ISessionManager`, `ISessionRegistry`, `ISessionWorkerClientFactory`) plus `SessionShutdownHostedService` are wired as singletons by `SessionServiceCollectionExtensions.AddGatewaySessions`. ## Key Types diff --git a/docs/plans/2026-07-10-dashboard-session-acl-tst15.md b/docs/plans/2026-07-10-dashboard-session-acl-tst15.md index 2c3ece6..558410d 100644 --- a/docs/plans/2026-07-10-dashboard-session-acl-tst15.md +++ b/docs/plans/2026-07-10-dashboard-session-acl-tst15.md @@ -87,7 +87,8 @@ public sealed record ApiKeyConstraints( At `OpenSession`, the resolved `ApiKeyIdentity.EffectiveConstraints.DashboardTags` is copied onto the new `GatewaySession.Tags`. The `apikey` admin CLI gains -`--dashboard-tags team-a,team-b` on `create`/`update`. +`--dashboard-tags team-a,team-b` on `create-key` (there is no `update` +subcommand — see `docs/Authentication.md`). Semantic note: `ApiKeyConstraints` today scopes *data-access* authorization (read/write subtrees, globs, classification). A dashboard *visibility* tag is a diff --git a/docs/plans/2026-08-17-deferred-closeout.md b/docs/plans/2026-08-17-deferred-closeout.md index 11e03eb..a5d8830 100644 --- a/docs/plans/2026-08-17-deferred-closeout.md +++ b/docs/plans/2026-08-17-deferred-closeout.md @@ -171,7 +171,7 @@ Task 10/11 gates it → commit **Files:** - Modify: `src/ZB.MOM.WW.MxGateway.Server/Security/Authentication/ApiKeyConstraints.cs` (add `IReadOnlyList DashboardTags`, default empty) - Modify: `src/ZB.MOM.WW.MxGateway.Server/Security/Authentication/ApiKeyConstraintSerializer.cs` (round-trip the new field; absent-in-JSON → empty — old rows keep deserializing) -- Modify: `src/ZB.MOM.WW.MxGateway.Server/Security/Authentication/ApiKeyAdminCommandLineParser.cs` + `ApiKeyAdminCliRunner.cs` + `ApiKeyAdminCommand.cs` + `ApiKeyAdminListedKey.cs` (CLI `--dashboard-tags team-a,team-b` on create/update; shown in list output) +- Modify: `src/ZB.MOM.WW.MxGateway.Server/Security/Authentication/ApiKeyAdminCommandLineParser.cs` + `ApiKeyAdminCliRunner.cs` + `ApiKeyAdminCommand.cs` + `ApiKeyAdminListedKey.cs` (CLI `--dashboard-tags team-a,team-b` on `create-key` — there is no `update` subcommand; shown in list output) - Modify: `src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs` (new `public IReadOnlySet Tags { get; }`, set at construction from the owner key's effective constraints; empty = untagged) - Modify: `src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionManager.cs` (`OpenSession` path copies `ApiKeyIdentity.EffectiveConstraints.DashboardTags` onto the session) - Test: serializer round-trip incl. legacy-JSON-without-field; CLI parse; session tag inheritance via the fake-worker harness diff --git a/docs/plans/2026-08-17-deferred-closeout.md.tasks.json b/docs/plans/2026-08-17-deferred-closeout.md.tasks.json index 0e49fb5..1c22e58 100644 --- a/docs/plans/2026-08-17-deferred-closeout.md.tasks.json +++ b/docs/plans/2026-08-17-deferred-closeout.md.tasks.json @@ -11,7 +11,7 @@ { "id": 8, "subject": "Task 8: Alarm-truncation degraded-status signal — proto + worker + gateway + dashboard", "status": "completed", "blockedBy": [3] }, { "id": 9, "subject": "Task 9: Client regeneration + rebuild for new alarm fields", "status": "completed", "blockedBy": [8] }, { "id": 10, "subject": "Task 10: Phase gate — full gateway suite on macOS", "status": "completed", "blockedBy": [1, 4, 5, 6, 7, 8, 9] }, - { "id": 11, "subject": "Task 11: windev gate — full Windows verification", "status": "pending", "blockedBy": [2, 10] }, + { "id": 11, "subject": "Task 11: windev gate — full Windows verification", "status": "completed", "blockedBy": [2, 10] }, { "id": 12, "subject": "Task 12: Wrap-up — closure notes, umbrella check, final review", "status": "pending", "blockedBy": [11] } ], "lastUpdated": "2026-08-17T00:00:00Z" diff --git a/src/ZB.MOM.WW.MxGateway.Server/Alarms/IGatewayAlarmService.cs b/src/ZB.MOM.WW.MxGateway.Server/Alarms/IGatewayAlarmService.cs index fd596bd..413fbcb 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Alarms/IGatewayAlarmService.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Alarms/IGatewayAlarmService.cs @@ -40,11 +40,21 @@ public interface IGatewayAlarmService /// /// True when the worker's most recent reconcile fetch hit the provider's - /// per-fetch cap, so may be missing active - /// alarms. The monitor is otherwise healthy — this is not a fault, it is - /// a completeness caveat, which is why it is separate from + /// per-fetch cap, so the active-alarm set may be missing alarms. The + /// monitor is otherwise healthy — this is not a fault, it is a + /// completeness caveat, which is why it is separate from /// and . Cleared by the first /// reconcile whose fetch comes back under the cap. + /// + /// Read it as "as of the last full reconcile, the fetch was capped", not as + /// a property of a particular array: the two are + /// separate reads, and live transitions keep moving the cached set between + /// reconciles. A consumer that reads both — the dashboard poll does — can + /// therefore straddle a reconcile, in which case its caveat describes the + /// adjacent generation and the banner is at worst one poll stale. That is + /// the intended granularity for a completeness hint; pairing them exactly + /// would need a combined accessor this seam deliberately does not have. + /// /// bool SnapshotTruncated { get; } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardEventBroadcaster.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardEventBroadcaster.cs index cf94e1d..909e7cc 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardEventBroadcaster.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardEventBroadcaster.cs @@ -20,9 +20,12 @@ namespace ZB.MOM.WW.MxGateway.Server.Dashboard.Hubs; /// values are stripped from a redacted copy of the event before it reaches any /// dashboard client. The source is shared with the gRPC /// event path and the reconnect replay ring, so it is never mutated in place — -/// the redaction is applied to a deep clone. This closes the value-leak seam at -/// the mirror independently of the still-outstanding per-session hub ACL -/// (see ). +/// the redaction is applied to a deep clone. This is the second of two +/// independent layers: decides at the +/// subscribe seam which sessions a caller may observe at all (see +/// ), while the redaction decides what a permitted +/// subscriber sees — so the value-leak seam stays closed whatever the ACL +/// admits. /// /// Hub context used to send to the session's group. /// diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/IDashboardSessionAcl.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/IDashboardSessionAcl.cs index 5ae4f87..4d62553 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/IDashboardSessionAcl.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/IDashboardSessionAcl.cs @@ -23,9 +23,12 @@ public interface IDashboardSessionAcl /// session identified by . /// /// - /// The dashboard caller. , unauthenticated, or claim-less - /// principals (including the anonymous-localhost path) are treated as Viewers - /// holding an empty tag grant. + /// The dashboard caller. is denied outright — there is no + /// caller to grant tags to, so it never reaches the untagged-session branch and is + /// refused even under UntaggedSessionVisibility=AllViewers. An + /// unauthenticated or claim-less principal (the anonymous-localhost path included) + /// is a Viewer holding an empty tag grant, which denies every tagged session but + /// still follows that branch. /// /// Session id the caller wants to observe. /// when the caller may observe the session; otherwise . diff --git a/src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerFrameProtocolTests.cs b/src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerFrameProtocolTests.cs index 9426325..8ab6d0b 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerFrameProtocolTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerFrameProtocolTests.cs @@ -1000,6 +1000,16 @@ public sealed class WorkerFrameProtocolTests /// two passes over the same stream, and a double-release would either do that or throw /// out of a later drain. Contiguous 1..N sequences with no /// duplicates and no trailing bytes is the observable form of both. + /// + /// The contention has to be real, so the stream is gated rather than a plain + /// : against a synchronously-completing stream each call finishes its own + /// drain before the next one starts, and neither a lost lock race nor a detached acquisition ever + /// happens. Gating write 1 parks the drainer while all 2 * perClass frames are queued, so + /// every one of those callers provably loses the race; gating the pass's first event write — + /// write 1 plus the perClass control frames — stops the pass with the control run flushed + /// and completed while the lock is still held, which is what makes the detached path deterministic + /// rather than merely likely: those callers can only have returned on their own completion. + /// /// /// A task that represents the asynchronous operation. [Fact] @@ -1007,24 +1017,46 @@ public sealed class WorkerFrameProtocolTests { const int perClass = 40; WorkerFrameProtocolOptions options = CreateOptions(); - using MemoryStream stream = new(); + using GatedWriteStream stream = new(secondGateWriteIndex: perClass + 2); WorkerFrameWriter writer = new(stream, options); - Task[] writes = new Task[perClass * 2]; + // The drainer: it takes the lock, then parks inside its own write with the lock held. + Task drainer = writer.WriteAsync(CreateGatewayHelloEnvelope(), WorkerFrameWritePriority.Control); + await AwaitWithTimeoutAsync(stream.FirstWriteStarted); + + // Queued against a held lock, so all 2 * perClass callers contend and all of them lose: each + // one's frame is written by the drainer's pass, never by its own. + Task[] controlWrites = new Task[perClass]; + Task[] eventWrites = new Task[perClass]; for (int index = 0; index < perClass; index++) { - writes[index * 2] = writer.WriteAsync(CreateGatewayHelloEnvelope(), WorkerFrameWritePriority.Control); - writes[(index * 2) + 1] = writer.WriteAsync(CreateEventEnvelope(), WorkerFrameWritePriority.Event); + controlWrites[index] = writer.WriteAsync(CreateGatewayHelloEnvelope(), WorkerFrameWritePriority.Control); + eventWrites[index] = writer.WriteAsync(CreateEventEnvelope(), WorkerFrameWritePriority.Event); } - await AwaitWithTimeoutAsync(Task.WhenAll(writes)); + Assert.All(controlWrites, write => Assert.False(write.IsCompleted)); + Assert.All(eventWrites, write => Assert.False(write.IsCompleted)); - // A detached acquisition drains whatever it finds and releases; this write goes through the - // same lock afterwards, so it can only succeed if the lock was left in a usable state. + stream.ReleaseFirstWrite(); + await AwaitWithTimeoutAsync(stream.SecondGateWriteStarted); + + // The boundary flush delivered every control frame, and the drainer is now parked on the first + // event write — so the lock cannot be free. Each of these callers therefore returned on its own + // completion with a live acquisition behind it: the detached path, taken perClass times. + await AwaitWithTimeoutAsync(Task.WhenAll(controlWrites)); + Assert.False(drainer.IsCompleted); + Assert.All(eventWrites, write => Assert.False(write.IsCompleted)); + + stream.ReleaseSecondGateWrite(); + await AwaitWithTimeoutAsync(Task.WhenAll(eventWrites)); + await AwaitWithTimeoutAsync(drainer); + + // Every detached acquisition drains what it finds and releases; this write goes through the + // same lock afterwards, so it can only succeed if none of them stranded or double-released it. await AwaitWithTimeoutAsync( writer.WriteAsync(CreateShutdownAckEnvelope(), WorkerFrameWritePriority.Control)); - const int total = (perClass * 2) + 1; + const int total = (perClass * 2) + 2; int controlCount = 0; int eventCount = 0; stream.Position = 0; @@ -1043,7 +1075,7 @@ public sealed class WorkerFrameProtocolTests } } - Assert.Equal(perClass + 1, controlCount); + Assert.Equal(perClass + 2, controlCount); Assert.Equal(perClass, eventCount); // No frame was written twice and none was left queued. Assert.Equal(stream.Length, stream.Position); diff --git a/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/FailoverAlarmConsumerTests.cs b/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/FailoverAlarmConsumerTests.cs index cabc520..f95beca 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/FailoverAlarmConsumerTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/FailoverAlarmConsumerTests.cs @@ -330,6 +330,44 @@ public sealed class FailoverAlarmConsumerTests Assert.Equal(22, sut.AcknowledgeByName("a", "p", "g", "c", "n", "node", "dom", "full")); } + /// + /// Proves that the snapshot truncation verdict is read from whichever child + /// is currently active, not cached from the primary: a capped primary reports + /// , and after failover the standby's own verdict + /// replaces it. The verdict drives the dashboard's completeness caveat, so a + /// stale one would either keep a banner on screen for a feed that is now + /// complete or, worse, clear it for one that is not. + /// + [Fact] + public void SnapshotActiveAlarms_TruncationVerdictComesFromActiveChild() + { + FlakyPrimary primary = new FlakyPrimary { ThrowOnPoll = false, SnapshotTruncated = true }; + StubStandby standby = new StubStandby { SnapshotTruncated = false }; + FailoverSettings settings = new FailoverSettings(threshold: 1, probeIntervalSeconds: 0, stableProbes: 1); + using FailoverAlarmConsumer sut = new FailoverAlarmConsumer(primary, standby, settings); + + sut.Subscribe(@"\\HOST\Galaxy!Area"); + Assert.Equal(AlarmProviderMode.Alarmmgr, sut.Mode); + + // Active = Primary → the primary's capped fetch surfaces. + _ = sut.SnapshotActiveAlarms(out bool truncatedOnPrimary); + Assert.True(truncatedOnPrimary); + + // Force a failover by failing the primary past threshold. + primary.ThrowOnPoll = true; + sut.PollOnce(); // threshold=1 → switch to Subtag + Assert.Equal(AlarmProviderMode.Subtag, sut.Mode); + + // Active = Standby → its own verdict, not the primary's leftover true. + _ = sut.SnapshotActiveAlarms(out bool truncatedOnStandby); + Assert.False(truncatedOnStandby); + + // And the standby really is the source: flip its verdict and the answer follows. + standby.SnapshotTruncated = true; + _ = sut.SnapshotActiveAlarms(out bool truncatedAfterStandbyCaps); + Assert.True(truncatedAfterStandbyCaps); + } + /// /// Proves that an intermittent failure during failback probing resets the /// clean-probe counter to zero, requiring a fresh unbroken run of diff --git a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeSession.cs b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeSession.cs index f849813..060f841 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeSession.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeSession.cs @@ -168,13 +168,20 @@ public sealed class WorkerPipeSession // Closing the transport is what actually ends a pipe read parked in the kernel: on net48 // NamedPipeClientStream.ReadAsync ignores its CancellationToken, so the message loop's // cancellation can never reach one (WRK-31). It is deliberately the LAST teardown step, - // because in the ordinary case every frame this session will ever write has completed by - // the time control reaches here: WorkerFrameWriter.WriteAsync signals only after the - // frame is written AND flushed, and each exit path awaits its final write before - // unwinding — the shutdown ack and shutdown-timeout fault inside the loop's dispatch, the - // event-drain and oversized-event faults inside the drain task the loop awaits, the - // watchdog fault inside the heartbeat task the loop awaits, and the handshake fault - // inside CompleteStartupHandshakeAsync's catch. + // because in the ordinary case every frame this session will ever write has been written + // AND flushed by the time control reaches here: WorkerFrameWriter.WriteAsync signals only + // after both, and each exit path awaits its final write before unwinding — the shutdown + // ack and shutdown-timeout fault inside the loop's dispatch, the event-drain and + // oversized-event faults inside the drain task the loop awaits, the watchdog fault inside + // the heartbeat task the loop awaits, and the handshake fault inside + // CompleteStartupHandshakeAsync's catch. + // + // That is a statement about frames, not about the writer being idle. A caller that + // returned on its own completion while another drainer held the write lock leaves a + // detached lock acquisition behind (WorkerFrameWriter.DetachLockWait), so a drain pass can + // still be scheduled after every caller has unwound. It is harmless here — the queues are + // empty by then, and a pass with nothing to dequeue writes and flushes nothing — but the + // invariant to rely on is "no frame is left undelivered", not "no writer work remains". // // "Ordinary" is the honest word, not "always": the loop's wait on the heartbeat and // drain tasks is budgeted (BackgroundTaskStopTimeout), and a stream write is genuinely diff --git a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/IAlarmCommandHandler.cs b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/IAlarmCommandHandler.cs index 638f84f..eb98010 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/IAlarmCommandHandler.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/IAlarmCommandHandler.cs @@ -77,10 +77,15 @@ public interface IAlarmCommandHandler : IDisposable /// rather than read from a separate property, both so the pair comes from /// one atomic consumer read and because it is the only carrier left once /// (or an empty galaxy) filters the - /// records down to none. when there is no active - /// subscription: no fetch has happened, so nothing is capped. + /// records down to none. Never assigned when there is no active + /// subscription — that case throws rather than reporting an empty, + /// never-capped set, so a query issued before SubscribeAlarms is a + /// caller error and not a silent all-clear. /// /// The currently active alarms matching the filter. + /// + /// Thrown when there is no active subscription. + /// IReadOnlyList QueryActive(string? alarmFilterPrefix, out bool snapshotTruncated); /// diff --git a/stillpending.md b/stillpending.md index 929ccf8..edfd2d2 100644 --- a/stillpending.md +++ b/stillpending.md @@ -62,7 +62,7 @@ These are documented, deliberate, and mostly enforced. Listed so the deferred su - 🔵 **Lazy browse is wire-only** — no lazy SQL / cache loading. `docs/DesignDecisions.md:365-376`, `docs/plans/2026-05-28-lazy-browse-design.md:30`. - 🔵 **No server-side / streaming browse search** — `docs/plans/2026-05-28-lazy-browse-design.md:208`. - 🔵 **Alarm command surface is ack + query only** — no Clear/Disable/Enable/Silence/Shelve/Inhibit; matches the MXAccess alarm-client set. `Worker/MxAccess/AlarmCommandHandler.cs`, shelve/suppress out of scope per `docs/AlarmClientDiscovery.md:60-66`. -- 🟡 **Dashboard EventsHub has no per-session ACL — still true on `main`, planned (epic Phase 4, not started).** Any authenticated dashboard user may still subscribe to any session group (`Dashboard/Hubs/EventsHub.cs` `TODO(per-session-acl)`). The enabling foundation (session `OwnerKeyId`) already merged in epic Phase 1; epic Phase 4 (Tasks 16–19) adds the gRPC session-owner gate, a session tag + group-to-tag config, and EventsHub per-session ACL with a hub-token tag claim. `docs/plans/2026-06-15-session-resilience.md` Phase 4. (See also §8.) +- ✅ **Dashboard EventsHub per-session ACL — RESOLVED (2026-08-17, branch `feat/deferred-closeout`, archreview TST-15).** The `TODO(per-session-acl)` no longer exists in `src/`. `IDashboardSessionAcl.CanViewSession` is consulted at both subscribe seams (`Dashboard/Hubs/EventsHub.cs` `SubscribeSession` and the in-process `IDashboardSessionEventSubscriber.Subscribe` behind the session-details page), so an authenticated dashboard user reaches a session's events only as an Administrator or as a Viewer whose `Dashboard:GroupToTag` grant intersects the session's tags (inherited from the owning API key); untagged sessions are Admin-only under the default `Dashboard:UntaggedSessionVisibility=AdminOnly`. Epic Phase 4 (Tasks 16–19) is complete. `docs/plans/2026-07-10-dashboard-session-acl-tst15.md`, `docs/Authorization.md`. (See also §8.) - ✅ **Adopt the shared `ZB.MOM.WW.GalaxyRepository` library (cross-repo normalization) — RESOLVED (2026-06-25, branch `feat/galaxyrepository-adoption`).** The inline Galaxy-browse code (`src/ZB.MOM.WW.MxGateway.Server/Galaxy/**` + `Grpc/GalaxyRepositoryGrpcService.cs`/`GalaxyProtoMapper.cs`, 27 files / −2959 LOC) is deleted; mxaccessgw now consumes `ZB.MOM.WW.GalaxyRepository` **0.2.0** (published to Gitea), same `galaxy_repository.v1` wire (no client change). The authz-parity gate was resolved by pushing the per-key browse-subtree filter **upstream** as an injectable `IGalaxyBrowseScopeProvider` (default no-op; HistorianGateway @ 0.1.0 unaffected); mxaccessgw supplies `GatewayBrowseScopeProvider` and switched `GatewayGrpcScopeResolver` to the lib proto types. Alarm-attribute discovery (`GetAlarmAttributesAsync`) was upstreamed too; the dashboard summary stays host-side (`DashboardGalaxySummaryProjector`). Lib 64 tests green; gateway 327 targeted green. Full record + caveats (NSSM config, pre-existing NU1903 + IntegrationTests `EventStreamService` breaks): **`A2-galaxyrepository-adoption-handoff.md`**. Remaining: cross-repo propagation (HistorianGateway `pending.md` §A2, scadaproj index) + opt-in live Galaxy-SQL validation. Full handoff: **`A2-galaxyrepository-adoption-handoff.md`** (repo root). Tracked cross-repo in HistorianGateway `pending.md` §A2 + scadaproj component normalization. ---