diff --git a/docs/requirements/Component-ManagementService.md b/docs/requirements/Component-ManagementService.md index aefc8ba2..4e077dce 100644 --- a/docs/requirements/Component-ManagementService.md +++ b/docs/requirements/Component-ManagementService.md @@ -83,6 +83,12 @@ In addition to `/management`, the Management Service exposes a dedicated REST su Both endpoints honour any site-scope rules attached to the caller's audit role by intersecting the caller-supplied `sourceSiteId` filter with the user's `PermittedSiteIds` (out-of-scope requests yield HTTP 403). Permission denial returns HTTP 403 with the same envelope shape used by `/management`. +**Filter parsing is strict on the enum dimensions** (`channel`, `kind`, `status`; register row 99). Values are matched **case-insensitively** against the enum member names, and an unrecognised value returns **HTTP 400** `{ error, code: "BAD_REQUEST" }` naming the full valid set — it is not silently dropped. Silently dropping a filter would leave that dimension unconstrained and hand the caller a plausible, complete-looking page that does not answer the question they asked (the same failure row 88 fixed for `deploy status`). **Ordinals are rejected**: a filter is a name-based contract, so `status=3` is an error even though `Enum.TryParse` would accept it and `3` is a real member. The `correlationId`/`executionId`/`parentExecutionId`/date dimensions stay **lax** (unparseable values are dropped). + +This is the *authoritative* check for those three dimensions — the CLI deliberately carries no client-side vocabulary gate. It used to (`AcceptOnlyFromAmong`), but that check matches **case-sensitively**, so `audit query --status delivered` failed at the CLI before ever reaching a server that would have accepted it. The valid set now lives in the CLI's option help text only. + +> The CentralUI `AuditLogPage` drill-in parser and the CentralUI export endpoint it feeds deliberately keep the **lax** parse. They read a query string a human may have bookmarked or hand-edited, where a stale fragment must still render the page rather than fault it. Both contracts live side by side in `AuditQueryParamParsers` (`ParseEnumList` vs `ParseEnumListStrict`); do not reconcile them to one. + ## Message Groups ### Templates @@ -244,6 +250,19 @@ Every incoming message carries the authenticated user's identity and roles. The Unauthorized commands receive an `Unauthorized` response message. Failed authorization attempts are not audit logged (consistent with existing behavior). +#### Site-scope gate: fail-closed on an unresolvable target + +The site-scope helpers resolve the command's target (instance id, site id, site identifier, data-connection id) and check the resolved site against the caller's `PermittedSiteIds`. **A target that cannot be resolved is DENIED for a site-scoped caller** — `SiteScopeViolationException` → `ManagementUnauthorized`, exactly as if it were out of scope. + +The gate used to be **fail-open** here (deferred-work register row 98). The resolve-then-check shape was written as `if (target != null) EnforceSiteScope(...)`, so a null resolve skipped the check entirely and a site-scoped caller fell through into the handler body — reaching `HandleGetInstance`/`HandleGetSite`/`HandleGetDataConnection`'s null answer, or a handler fault, for any id in the system. The rule is now: *a target that cannot be PROVEN to sit inside the caller's scope is treated as outside it.* + +Two properties are load-bearing: + +- **System-wide callers are unaffected.** `Administrator`, and a `Deployer` carrying no site grants, return at the guards above the lookup — they were never scoped, so they still reach the handler and still receive its ordinary "not found" / null answer for a missing id. Only a scoped caller is denied. +- **The denial does not leak existence.** A scoped caller gets the identical `ManagementUnauthorized` shape for "does not exist" and for "exists at a site you may not see", and the message never states which. Otherwise the gate would be an id-enumeration oracle over the whole fleet — which is what the fail-open behaviour was, since `GetInstance`/`GetSite`/`GetDataConnection` answered success-with-null for a nonexistent id and `Unauthorized` for an out-of-scope one. + +The shared denial arm is `DenyUnresolvedTargetIfScoped`; `EnforceSiteScopeForInstance`, `EnforceSiteScopeForIdentifier` and the three read handlers that resolve their own target all route through it. + ## Service Dependencies (DI) The ManagementActor receives the following services and repositories via DI (injected through the actor's constructor or via a service provider): diff --git a/docs/requirements/Component-Security.md b/docs/requirements/Component-Security.md index d5f5f473..c8a1f28f 100644 --- a/docs/requirements/Component-Security.md +++ b/docs/requirements/Component-Security.md @@ -117,6 +117,7 @@ Set in a local or docker-dev environment via the environment variable `ScadaBrid - View site event logs. - **Site scoping**: A user with site-scoped Deployer role can only perform these actions for instances at their permitted sites. - **Administrator overrides site scoping.** A user who holds `Administrator` is system-wide **regardless of the `SiteId` claims they carry** — a scoped Deployer grant does not narrow an Administrator. This is enforced identically on both surfaces: `ManagementActor.EnforceSiteScope` (CLI / HTTP management API) and the Central UI's `SiteScopeService`. The two disagreed until deferred-work register row 86 — the UI was claims-only, so the same user saw the whole fleet through the CLI and only their sites in the UI. **No other role carries this bypass**; in particular `Designer` does not (Designer has no site-scoped permissions to bypass in the first place). +- **The scope check fails CLOSED on an unresolvable target.** Site scoping works by resolving the command's target (instance id, site id/identifier, data-connection id) and comparing its site to the permitted set. When the target does not resolve, a **site-scoped caller is DENIED** — the same `Unauthorized` result an out-of-scope target produces. The rule is *a target that cannot be PROVEN to sit inside the caller's scope is treated as outside it*; it fails open only for callers who were never scoped (Administrator, or a Deployer with no site grants), who keep their ordinary "not found" answer. The check was fail-open until deferred-work register row 98, which also made the denial **indistinguishable from an out-of-scope denial** — same response shape, and a message that never says whether the target exists — so a scoped caller cannot use the gate to enumerate ids across the fleet. See `Component-ManagementService.md` → *Site-scope gate: fail-closed on an unresolvable target*. > **Area management (arch-review C6)**: Managing area definitions per site is gated **any-of [Designer, Deployer]**, not Administrator. Areas are authored both in the Designer tooling and inside the Central UI Deployment Topology workflow (`Topology.razor` is `RequireDeployment`), so either role qualifies; Administrator is not implicitly included in the actor gate. diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/AuditCommands.cs b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/AuditCommands.cs index 7510b371..fdfc6968 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/AuditCommands.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/AuditCommands.cs @@ -41,28 +41,30 @@ public static class AuditCommands // --channel/--kind/--status/--site are multi-valued: System.CommandLine accepts // both repeated tokens (--channel A --channel B) and, with // AllowMultipleArgumentsPerToken, a single token carrying several values - // (--channel A B). AcceptOnlyFromAmong validates EACH supplied value. + // (--channel A B). + // + // The valid sets are named in the help text rather than enforced with + // AcceptOnlyFromAmong: that check is case-SENSITIVE, so `--status delivered` + // failed client-side even though the server has always parsed these + // case-insensitively (register row 99 — same treatment row 88 gave + // `deploy status`). The server rejects an unrecognised value with a 400 naming + // the same list and is the authoritative check; HTTP API callers never pass + // through these options at all. var channelOption = new Option("--channel") { - Description = "Filter by channel (ApiOutbound, DbOutbound, Notification, ApiInbound); repeatable", + Description = "Filter by channel (ApiOutbound, DbOutbound, Notification, ApiInbound; case-insensitive); repeatable", AllowMultipleArgumentsPerToken = true, }; - channelOption.AcceptOnlyFromAmong("ApiOutbound", "DbOutbound", "Notification", "ApiInbound"); var kindOption = new Option("--kind") { - Description = "Filter by event kind (ApiCall, ApiCallCached, DbWrite, DbWriteCached, NotifySend, NotifyDeliver, InboundRequest, InboundAuthFailure, CachedSubmit, CachedResolve); repeatable", + Description = "Filter by event kind (ApiCall, ApiCallCached, DbWrite, DbWriteCached, NotifySend, NotifyDeliver, InboundRequest, InboundAuthFailure, CachedSubmit, CachedResolve; case-insensitive); repeatable", AllowMultipleArgumentsPerToken = true, }; - kindOption.AcceptOnlyFromAmong( - "ApiCall", "ApiCallCached", "DbWrite", "DbWriteCached", "NotifySend", - "NotifyDeliver", "InboundRequest", "InboundAuthFailure", "CachedSubmit", "CachedResolve"); var statusOption = new Option("--status") { - Description = "Filter by status (Submitted, Forwarded, Attempted, Delivered, Failed, Parked, Discarded, Skipped); repeatable", + Description = "Filter by status (Submitted, Forwarded, Attempted, Delivered, Failed, Parked, Discarded, Skipped; case-insensitive); repeatable", AllowMultipleArgumentsPerToken = true, }; - statusOption.AcceptOnlyFromAmong( - "Submitted", "Forwarded", "Attempted", "Delivered", "Failed", "Parked", "Discarded", "Skipped"); var siteOption = new Option("--site") { Description = "Filter by source site ID; repeatable", @@ -143,34 +145,36 @@ public static class AuditCommands { var sinceOption = new Option("--since") { Description = "Start time: relative (1h, 24h, 7d) or ISO-8601", Required = true }; var untilOption = new Option("--until") { Description = "End time: relative (1h, 24h, 7d) or ISO-8601", Required = true }; - var formatExportOption = new Option("--format") { Description = "Export format", Required = true }; - formatExportOption.AcceptOnlyFromAmong("csv", "jsonl", "parquet"); + // Same rationale as --channel/--kind/--status below and on `query` (register + // row 99): AcceptOnlyFromAmong is case-SENSITIVE, so it rejected `--format CSV` + // even though the server lowercases the value before matching. The server + // returns a 400 naming csv/jsonl/parquet and is the authoritative check. + var formatExportOption = new Option("--format") + { + Description = "Export format (csv, jsonl, parquet; case-insensitive)", + Required = true, + }; var outputOption = new Option("--output") { Description = "Destination file path", Required = true }; // --channel/--kind/--status/--site are multi-valued — same shape as the // `query` subcommand: repeated tokens (--channel A --channel B) and, with // AllowMultipleArgumentsPerToken, a single token carrying several values - // (--channel A B). AcceptOnlyFromAmong validates EACH supplied value. + // (--channel A B). Valid sets are named in the help text and enforced + // server-side, case-insensitively — see the `query` builder for the full note. var channelOption = new Option("--channel") { - Description = "Filter by channel (ApiOutbound, DbOutbound, Notification, ApiInbound); repeatable", + Description = "Filter by channel (ApiOutbound, DbOutbound, Notification, ApiInbound; case-insensitive); repeatable", AllowMultipleArgumentsPerToken = true, }; - channelOption.AcceptOnlyFromAmong("ApiOutbound", "DbOutbound", "Notification", "ApiInbound"); var kindOption = new Option("--kind") { - Description = "Filter by event kind (ApiCall, ApiCallCached, DbWrite, DbWriteCached, NotifySend, NotifyDeliver, InboundRequest, InboundAuthFailure, CachedSubmit, CachedResolve); repeatable", + Description = "Filter by event kind (ApiCall, ApiCallCached, DbWrite, DbWriteCached, NotifySend, NotifyDeliver, InboundRequest, InboundAuthFailure, CachedSubmit, CachedResolve; case-insensitive); repeatable", AllowMultipleArgumentsPerToken = true, }; - kindOption.AcceptOnlyFromAmong( - "ApiCall", "ApiCallCached", "DbWrite", "DbWriteCached", "NotifySend", - "NotifyDeliver", "InboundRequest", "InboundAuthFailure", "CachedSubmit", "CachedResolve"); var statusOption = new Option("--status") { - Description = "Filter by status (Submitted, Forwarded, Attempted, Delivered, Failed, Parked, Discarded, Skipped); repeatable", + Description = "Filter by status (Submitted, Forwarded, Attempted, Delivered, Failed, Parked, Discarded, Skipped; case-insensitive); repeatable", AllowMultipleArgumentsPerToken = true, }; - statusOption.AcceptOnlyFromAmong( - "Submitted", "Forwarded", "Attempted", "Delivered", "Failed", "Parked", "Discarded", "Skipped"); var siteOption = new Option("--site") { Description = "Filter by source site ID; repeatable", diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md index dd9a0772..81e97487 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md @@ -1688,9 +1688,9 @@ scadabridge --url audit query [options] |--------|----------|---------|-------------| | `--since` | no | — | Start time: relative (`1h`, `24h`, `7d`) or ISO-8601 | | `--until` | no | — | End time: relative (`1h`, `24h`, `7d`) or ISO-8601 | -| `--channel` | no | — | Filter by channel (`ApiOutbound`, `DbOutbound`, `Notification`, `ApiInbound`); repeatable — multiple values are OR-combined | -| `--kind` | no | — | Filter by event kind (`ApiCall`, `ApiCallCached`, `DbWrite`, `DbWriteCached`, `NotifySend`, `NotifyDeliver`, `InboundRequest`, `InboundAuthFailure`, `CachedSubmit`, `CachedResolve`); repeatable — multiple values are OR-combined | -| `--status` | no | — | Filter by status (`Submitted`, `Forwarded`, `Attempted`, `Delivered`, `Failed`, `Parked`, `Discarded`, `Skipped`); repeatable — multiple values are OR-combined | +| `--channel` | no | — | Filter by channel (`ApiOutbound`, `DbOutbound`, `Notification`, `ApiInbound`; case-insensitive); repeatable — multiple values are OR-combined | +| `--kind` | no | — | Filter by event kind (`ApiCall`, `ApiCallCached`, `DbWrite`, `DbWriteCached`, `NotifySend`, `NotifyDeliver`, `InboundRequest`, `InboundAuthFailure`, `CachedSubmit`, `CachedResolve`; case-insensitive); repeatable — multiple values are OR-combined | +| `--status` | no | — | Filter by status (`Submitted`, `Forwarded`, `Attempted`, `Delivered`, `Failed`, `Parked`, `Discarded`, `Skipped`; case-insensitive); repeatable — multiple values are OR-combined | | `--site` | no | — | Filter by source site ID; repeatable — multiple values are OR-combined | | `--target` | no | — | Filter by target (external system, DB connection, notification list) | | `--actor` | no | — | Filter by actor | @@ -1707,6 +1707,15 @@ either as repeated flags (`--channel ApiOutbound --channel DbOutbound`) or space-separated after one flag (`--channel ApiOutbound DbOutbound`). Values within one filter are OR-combined; the different filters are AND-combined. +**Filter values are case-insensitive** — `--status delivered`, `--status Delivered` +and `--status DELIVERED` are the same query. The CLI does not validate the +vocabulary itself; the server does, and an unrecognised value is an **error naming +the valid set**, not a silently-dropped filter. That matters: dropping the filter +would return a plausible, complete-looking page that does not answer the question +asked (the same failure mode as `deploy status`). Ordinals are rejected — a filter +is a name-based contract, so `--status 3` is an error even though `3` is a real +enum value. + With `--format table`, events render as an aligned text table with columns `OccurredAtUtc`, `Channel`, `Kind`, `Status`, `Target`, `Actor`, `DurationMs`, `HttpStatus`; long `Target`/`Actor` values are truncated with an ellipsis. With @@ -1726,11 +1735,11 @@ scadabridge --url audit export --since