dohertj2/ScadaBridge
1
0
Fork 0
Code Issues 2 Pull Requests Actions Packages Projects Releases Wiki Activity

merge(agent-b): rows 98/99 — site-scope gate fail-closed on unresolvable targets (5 sites, not 2: the inline GetInstance/GetSite/GetDataConnection shapes were an id-enumeration oracle; all route through DenyUnresolvedTargetIfScoped, denial indistinguishable from out-of-scope); CLI audit query/export drop 7 case-sensitive AcceptOnlyFromAmong gates for help-text values + a new strict server-side ParseEnumListStrict (no silent drop, no ordinals)

Browse Source
This commit is contained in:
Joseph Doherty
2026-08-17 06:20:14 -04:00
parent 9dbda4a2f2 f33f25a10b
commit 152ec85e92
14 changed files with 818 additions and 101 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/requirements/Component-ManagementService.md
+19
View File
@@ -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):
docs/requirements/Component-Security.md
+1
View File
@@ -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.
src/ZB.MOM.WW.ScadaBridge.CLI/Commands/AuditCommands.cs
+26 -22
View File
@@ -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<string[]>("--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<string[]>("--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<string[]>("--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<string[]>("--site")
{
Description = "Filter by source site ID; repeatable",
@@ -143,34 +145,36 @@ public static class AuditCommands
{
var sinceOption = new Option<string>("--since") { Description = "Start time: relative (1h, 24h, 7d) or ISO-8601", Required = true };
var untilOption = new Option<string>("--until") { Description = "End time: relative (1h, 24h, 7d) or ISO-8601", Required = true };
var formatExportOption = new Option<string>("--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<string>("--format")
{
Description = "Export format (csv, jsonl, parquet; case-insensitive)",
Required = true,
};
var outputOption = new Option<string>("--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<string[]>("--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<string[]>("--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<string[]>("--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<string[]>("--site")
{
Description = "Filter by source site ID; repeatable",
src/ZB.MOM.WW.ScadaBridge.CLI/README.md
+16 -7
View File
@@ -1688,9 +1688,9 @@ scadabridge --url <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 <url> audit export --since <time> --until <time> --format <fmt
|--------|----------|---------|-------------|
| `--since` | yes | — | Start time: relative (`1h`, `24h`, `7d`) or ISO-8601 |
| `--until` | yes | — | End time: relative (`1h`, `24h`, `7d`) or ISO-8601 |
| `--format` | yes | — | Export format: `csv`, `jsonl`, or `parquet` |
| `--format` | yes | — | Export format: `csv`, `jsonl`, or `parquet` (case-insensitive) |
| `--output` | yes | — | Destination file path |
| `--channel` | no | — | Filter by channel |
| `--kind` | no | — | Filter by event kind |
| `--status` | no | — | Filter by status |
| `--channel` | no | — | Filter by channel (case-insensitive; same valid set as `audit query`) |
| `--kind` | no | — | Filter by event kind (case-insensitive; same valid set as `audit query`) |
| `--status` | no | — | Filter by status (case-insensitive; same valid set as `audit query`) |
| `--site` | no | — | Filter by source site ID |
| `--target` | no | — | Filter by target |
| `--actor` | no | — | Filter by actor |
src/ZB.MOM.WW.ScadaBridge.Commons/Types/Audit/AuditFilterParseException.cs
+21
View File
@@ -0,0 +1,21 @@
namespace ZB.MOM.WW.ScadaBridge.Commons.Types.Audit;
/// <summary>
/// Thrown by <see cref="AuditQueryParamParsers.ParseEnumListStrict{TEnum}"/> when an
/// Audit Log filter value is not a member name of its enum.
/// </summary>
/// <remarks>
/// The message is composed to be shown VERBATIM to the caller — it names the offending
/// value and the full valid set, and carries no server-internal detail — so the
/// ManagementService audit endpoints surface it directly as the body of a
/// <c>400 BAD_REQUEST</c> rather than replacing it with a generic fault string.
/// </remarks>
public class AuditFilterParseException : Exception
{
/// <summary>
/// Initializes a new instance with a caller-safe message naming the bad value
/// and the valid set.
/// </summary>
/// <param name="message">The caller-safe error message.</param>
public AuditFilterParseException(string message) : base(message) { }
}
src/ZB.MOM.WW.ScadaBridge.Commons/Types/Audit/AuditQueryParamParsers.cs
+82 -4
View File
@@ -21,11 +21,24 @@ namespace ZB.MOM.WW.ScadaBridge.Commons.Types.Audit;
/// </para>
///
/// <para>
/// <b>Lax-parse contract.</b> Every value of a repeated parameter is parsed
/// independently; an unparseable or blank element is silently dropped (NO 400)
/// rather than failing the whole set. An empty result collapses to <c>null</c> so
/// the corresponding filter dimension stays unconstrained.
/// <b>Two parse contracts, chosen per surface.</b>
/// </para>
/// <list type="bullet">
/// <item><b>Lax</b> (<see cref="ParseEnumList{TEnum}"/>): every value of a repeated
/// parameter is parsed independently; an unparseable or blank element is silently
/// dropped (NO 400) rather than failing the whole set. An empty result collapses to
/// <c>null</c> so the corresponding filter dimension stays unconstrained. This is the
/// right contract for the CentralUI <c>AuditLogPage</c> drill-in parser and the
/// CentralUI export endpoint it feeds: those read a query string a human may have
/// bookmarked or hand-edited, and a stale fragment must still render the page rather
/// than fault it.</item>
/// <item><b>Strict</b> (<see cref="ParseEnumListStrict{TEnum}"/>): an unrecognised
/// value is an ERROR naming the valid set. This is the right contract for the
/// ManagementService <c>/api/audit/query</c> + <c>/api/audit/export</c> endpoints,
/// which answer a CLI/API caller who asked a specific question: silently dropping
/// their filter returns a plausible, complete-looking result that does NOT answer it
/// (register rows 46/88/99 — the same loud-failure principle).</item>
/// </list>
/// </summary>
public static class AuditQueryParamParsers
{
@@ -57,6 +70,71 @@ public static class AuditQueryParamParsers
return parsed.Count > 0 ? parsed : null;
}
/// <summary>
/// Parses each raw value as <typeparamref name="TEnum"/> (case-insensitive),
/// REJECTING an unrecognised value with an
/// <see cref="AuditFilterParseException"/> that names the valid set. Blank
/// elements are still dropped silently (a repeated query param can legitimately
/// carry an empty slot); a set that yields no value collapses to <c>null</c> so
/// the filter dimension stays unconstrained.
/// </summary>
/// <remarks>
/// Register row 99. Two traps this closes that a bare
/// <see cref="Enum.TryParse{TEnum}(string, bool, out TEnum)"/> does not:
/// <list type="number">
/// <item><b>Numeric strings.</b> <c>TryParse</c> happily converts <c>"7"</c> —
/// and <c>"3"</c>, which lands on a REAL member — into an enum value. A filter
/// is a caller-facing name-based contract, never an ordinal one, so any value
/// starting with a digit or sign is rejected outright. Enum member names can
/// never start with those characters, so the check costs no legitimate input.</item>
/// <item><b>Undefined composites.</b> <see cref="Enum.IsDefined{TEnum}(TEnum)"/>
/// catches anything that parsed to a value with no declared member.</item>
/// </list>
/// </remarks>
/// <typeparam name="TEnum">The enum type to parse each raw value into.</typeparam>
/// <param name="rawValues">Raw query-parameter string values to parse; may be null.</param>
/// <param name="parameterName">Query-parameter name, used in the error message (e.g. <c>"status"</c>).</param>
/// <returns>A non-empty list of parsed values, or <c>null</c> if no values were supplied.</returns>
/// <exception cref="AuditFilterParseException">A supplied value is not a member name of <typeparamref name="TEnum"/>.</exception>
public static IReadOnlyList<TEnum>? ParseEnumListStrict<TEnum>(
IEnumerable<string?>? rawValues, string parameterName)
where TEnum : struct, Enum
{
if (rawValues is null)
{
return null;
}
var parsed = new List<TEnum>();
foreach (var raw in rawValues)
{
if (string.IsNullOrWhiteSpace(raw))
{
continue;
}
var trimmed = raw.Trim();
if (LooksNumeric(trimmed)
|| !Enum.TryParse<TEnum>(trimmed, ignoreCase: true, out var value)
|| !Enum.IsDefined(value))
{
throw new AuditFilterParseException(
$"Unknown {parameterName} '{trimmed}'. Valid values: {string.Join(", ", Enum.GetNames<TEnum>())}.");
}
parsed.Add(value);
}
return parsed.Count > 0 ? parsed : null;
}
/// <summary>
/// True when the value opens with a digit or a sign — i.e. an ordinal, not an
/// enum member name. No enum member name can start with these characters.
/// </summary>
private static bool LooksNumeric(string value)
=> value.Length > 0 && (char.IsAsciiDigit(value[0]) || value[0] == '+' || value[0] == '-');
/// <summary>
/// Trims each raw value and drops blank entries. Returns <c>null</c> when
/// <paramref name="rawValues"/> is <c>null</c>, empty, or every value was
src/ZB.MOM.WW.ScadaBridge.ManagementService/AuditEndpoints.cs
+60 -8
View File
@@ -133,7 +133,16 @@ public static class AuditEndpoints
return Forbidden("OperationalAudit");
}
var filter = ParseFilter(context.Request.Query);
AuditLogQueryFilter filter;
try
{
filter = ParseFilter(context.Request.Query);
}
catch (AuditFilterParseException ex)
{
return BadFilter(ex);
}
var restricted = ApplySiteScope(filter, auth.User!);
if (restricted is null)
{
@@ -216,7 +225,16 @@ public static class AuditEndpoints
statusCode: 400);
}
var filter = ParseFilter(context.Request.Query);
AuditLogQueryFilter filter;
try
{
filter = ParseFilter(context.Request.Query);
}
catch (AuditFilterParseException ex)
{
return BadFilter(ex);
}
var restricted = ApplySiteScope(filter, auth.User!);
if (restricted is null)
{
@@ -532,6 +550,21 @@ public static class AuditEndpoints
private static IResult Forbidden(string permission) => Results.Json(
new { error = $"Permission '{permission}' required.", code = "UNAUTHORIZED" }, statusCode: 403);
/// <summary>
/// Maps an <see cref="AuditFilterParseException"/> to the endpoint's existing
/// <c>400 BAD_REQUEST</c> shape — the same envelope the unsupported-export-format
/// branch already returns, so the CLI needs no new error handling.
/// </summary>
/// <remarks>
/// The exception message is surfaced VERBATIM. It is composed by the parser to be
/// caller-safe (offending value + the full valid set) and naming the valid set is
/// the entire point of the check (register row 99).
/// </remarks>
/// <param name="ex">The parse failure to surface.</param>
/// <returns>A 400 result carrying the caller-safe message.</returns>
private static IResult BadFilter(AuditFilterParseException ex) => Results.Json(
new { error = ex.Message, code = "BAD_REQUEST" }, statusCode: 400);
/// <summary>
/// Applies the caller's <see cref="AuthenticatedUser.PermittedSiteIds"/> to the
/// audit-log filter. System-wide callers (empty PermittedSiteIds —
@@ -587,23 +620,42 @@ public static class AuditEndpoints
/// <c>channel</c>/<c>kind</c>/<c>status</c>/<c>sourceSiteId</c> dimensions are
/// multi-value: a repeated query param (<c>channel=A&amp;channel=B</c>) yields
/// a multi-element filter list, while a single param yields a one-element
/// list. Unknown enum names / un-parseable Guids / dates are silently dropped
/// (no 400) — the same lax contract the CentralUI export endpoint uses; an
/// unparseable value within a repeated set is dropped, not the whole set.
/// list.
/// </summary>
/// <remarks>
/// <para>
/// <b>The enum filters parse STRICTLY (register row 99); the Guid/date filters
/// stay lax.</b> An unrecognised <c>channel</c>/<c>kind</c>/<c>status</c> throws
/// <see cref="AuditFilterParseException"/>, which the two handlers turn into a
/// <c>400 BAD_REQUEST</c> naming the valid set. Silently dropping a filter here
/// used to hand the CLI a plausible, complete-looking page that did not answer
/// the question asked — the same failure row 88 fixed for <c>deploy status</c>.
/// The CLI reaches this endpoint with case-insensitive values now that its own
/// case-SENSITIVE <c>AcceptOnlyFromAmong</c> guard is gone, so this is the
/// authoritative check for those three dimensions.
/// </para>
/// <para>
/// The CentralUI <c>AuditLogPage</c> drill-in parser and the CentralUI export
/// endpoint it feeds deliberately keep the LAX
/// <see cref="AuditQueryParamParsers.ParseEnumList{TEnum}"/> — they read a query
/// string a human may have bookmarked or hand-edited, where a stale fragment
/// must still render the page. Do not "reconcile" the two.
/// </para>
/// <para>
/// This endpoint reads the source-site filter from the <c>sourceSiteId</c>
/// query key, whereas the CentralUI export endpoint reads it as <c>site</c>.
/// The divergence is deliberate — each endpoint matches its own CLI / UI URL
/// builder — so do NOT "fix" the two to a single key name.
/// </para>
/// </remarks>
/// <param name="query">The HTTP query string collection to parse filter parameters from.</param>
/// <returns>An <see cref="AuditLogQueryFilter"/> built from the supplied query parameters.</returns>
/// <exception cref="AuditFilterParseException">A channel/kind/status value is not a recognised member name.</exception>
public static AuditLogQueryFilter ParseFilter(IQueryCollection query)
{
var channels = AuditQueryParamParsers.ParseEnumList<AuditChannel>(query["channel"]);
var kinds = AuditQueryParamParsers.ParseEnumList<AuditKind>(query["kind"]);
var statuses = AuditQueryParamParsers.ParseEnumList<AuditStatus>(query["status"]);
var channels = AuditQueryParamParsers.ParseEnumListStrict<AuditChannel>(query["channel"], "channel");
var kinds = AuditQueryParamParsers.ParseEnumListStrict<AuditKind>(query["kind"], "kind");
var statuses = AuditQueryParamParsers.ParseEnumListStrict<AuditStatus>(query["status"], "status");
var sourceSiteIds = AuditQueryParamParsers.ParseStringList(query["sourceSiteId"]);
Guid? correlationId = null;
src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs
+100 -11
View File
@@ -536,8 +536,49 @@ public class ManagementActor : ReceiveActor
}
}
/// <summary>
/// The fail-CLOSED arm of the scope gate: a target that could NOT be resolved is
/// denied for a site-scoped caller, and ignored for a system-wide one.
/// </summary>
/// <remarks>
/// Register row 98. The scope gate used to be fail-OPEN on an unresolvable id —
/// the resolve-then-check shape was written as <c>if (x != null) EnforceSiteScope(…)</c>,
/// so a null resolve skipped the check entirely and a site-scoped caller passed the
/// gate. Authorization now fails CLOSED: a target that cannot be PROVEN to sit
/// inside the caller's scope is treated as outside it.
///
/// <para>
/// <b>System-wide callers are untouched.</b> Administrator, or a Deployer with no
/// site grants (the rule documented on <see cref="EnforceSiteScope"/>), return at
/// the guards below — they were never scoped, so they still reach the handler and
/// still get its ordinary "not found"/null answer for a missing id.
/// </para>
///
/// <para>
/// <b>The denial is deliberately indistinguishable from an out-of-scope one.</b>
/// It raises the same <see cref="SiteScopeViolationException"/> (→
/// <c>ManagementUnauthorized</c>), and <paramref name="targetDescription"/> must
/// never state whether the target exists — otherwise the gate becomes an
/// enumeration oracle letting a scoped caller probe ids across the whole fleet.
/// </para>
/// </remarks>
/// <param name="user">The authenticated caller.</param>
/// <param name="targetDescription">Caller-safe description of the target, e.g. <c>"instance 42"</c>.</param>
/// <exception cref="SiteScopeViolationException">The caller is site-scoped.</exception>
private static void DenyUnresolvedTargetIfScoped(AuthenticatedUser user, string targetDescription)
{
if (user.PermittedSiteIds.Length == 0) return;
if (user.Roles.Contains(Roles.Administrator, StringComparer.OrdinalIgnoreCase)) return;
throw new SiteScopeViolationException(
$"Access denied: {targetDescription} is not within your permitted sites " +
$"[{string.Join(", ", user.PermittedSiteIds)}].");
}
/// <summary>
/// Resolves the site ID for an instance and enforces site-scope.
/// An instance that cannot be resolved is DENIED for a site-scoped caller
/// (see <see cref="DenyUnresolvedTargetIfScoped"/>).
/// </summary>
private static async Task EnforceSiteScopeForInstance(IServiceProvider sp, AuthenticatedUser user, int instanceId)
{
@@ -546,13 +587,20 @@ public class ManagementActor : ReceiveActor
var repo = sp.GetRequiredService<ITemplateEngineRepository>();
var instance = await repo.GetInstanceByIdAsync(instanceId);
if (instance != null)
EnforceSiteScope(user, instance.SiteId);
if (instance is null)
{
DenyUnresolvedTargetIfScoped(user, $"instance {instanceId}");
return;
}
EnforceSiteScope(user, instance.SiteId);
}
/// <summary>
/// Resolves a site by its string identifier and enforces site-scope.
/// Used by remote-query handlers that key off the site identifier rather than its ID.
/// A site identifier that cannot be resolved is DENIED for a site-scoped caller
/// (see <see cref="DenyUnresolvedTargetIfScoped"/>).
/// </summary>
private static async Task EnforceSiteScopeForIdentifier(IServiceProvider sp, AuthenticatedUser user, string siteIdentifier)
{
@@ -561,8 +609,13 @@ public class ManagementActor : ReceiveActor
var repo = sp.GetRequiredService<ISiteRepository>();
var site = await repo.GetSiteByIdentifierAsync(siteIdentifier);
if (site != null)
EnforceSiteScope(user, site.Id);
if (site is null)
{
DenyUnresolvedTargetIfScoped(user, $"site '{siteIdentifier}'");
return;
}
EnforceSiteScope(user, site.Id);
}
/// <summary>
@@ -836,12 +889,28 @@ public class ManagementActor : ReceiveActor
return Page(instances, cmd.Skip, cmd.Take);
}
/// <summary>
/// Reads one instance, scope-checked against its own site.
/// </summary>
/// <remarks>
/// This handler resolves the instance ITSELF rather than calling
/// <see cref="EnforceSiteScopeForInstance"/> — the helper would repeat the same
/// repository read — so it carries its own copy of the fail-CLOSED arm
/// (register row 98). A scoped caller must not be able to tell a nonexistent
/// instance (which used to answer success/null) from one at a site they may not
/// see, or the read becomes an id-enumeration oracle over the whole fleet.
/// </remarks>
private static async Task<object?> HandleGetInstance(IServiceProvider sp, GetInstanceCommand cmd, AuthenticatedUser user)
{
var repo = sp.GetRequiredService<ITemplateEngineRepository>();
var instance = await repo.GetInstanceByIdAsync(cmd.InstanceId);
if (instance != null)
EnforceSiteScope(user, instance.SiteId);
if (instance is null)
{
DenyUnresolvedTargetIfScoped(user, $"instance {cmd.InstanceId}");
return null;
}
EnforceSiteScope(user, instance.SiteId);
return instance;
}
@@ -1659,12 +1728,22 @@ public class ManagementActor : ReceiveActor
return sites;
}
/// <summary>
/// Reads one site, scope-checked against itself. Fail-CLOSED on an unresolvable
/// id for a scoped caller — see <see cref="DenyUnresolvedTargetIfScoped"/>
/// (register row 98).
/// </summary>
private static async Task<object?> HandleGetSite(IServiceProvider sp, GetSiteCommand cmd, AuthenticatedUser user)
{
var repo = sp.GetRequiredService<ISiteRepository>();
var site = await repo.GetSiteByIdAsync(cmd.SiteId);
if (site != null)
EnforceSiteScope(user, site.Id);
if (site is null)
{
DenyUnresolvedTargetIfScoped(user, $"site {cmd.SiteId}");
return null;
}
EnforceSiteScope(user, site.Id);
return site;
}
@@ -1801,13 +1880,23 @@ public class ManagementActor : ReceiveActor
return (await repo.GetAllDataConnectionsAsync()).Select(DataConnectionPublicShape).ToList();
}
/// <summary>
/// Reads one data connection, scope-checked against its owning site. Fail-CLOSED
/// on an unresolvable id for a scoped caller — see
/// <see cref="DenyUnresolvedTargetIfScoped"/> (register row 98).
/// </summary>
private static async Task<object?> HandleGetDataConnection(IServiceProvider sp, GetDataConnectionCommand cmd, AuthenticatedUser user)
{
var repo = sp.GetRequiredService<ISiteRepository>();
var conn = await repo.GetDataConnectionByIdAsync(cmd.DataConnectionId);
if (conn != null)
EnforceSiteScope(user, conn.SiteId);
return conn is null ? null : DataConnectionPublicShape(conn);
if (conn is null)
{
DenyUnresolvedTargetIfScoped(user, $"data connection {cmd.DataConnectionId}");
return null;
}
EnforceSiteScope(user, conn.SiteId);
return DataConnectionPublicShape(conn);
}
private static async Task<object?> HandleCreateDataConnection(IServiceProvider sp, CreateDataConnectionCommand cmd, string user)
tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/CommandTreeTests.cs
+81 -2
View File
@@ -231,8 +231,12 @@ public class CommandTreeTests
[Theory]
[InlineData("sideways")]
// AcceptOnlyFromAmong is case-SENSITIVE (same contract as the audit
// --channel / --kind / --status options), so the literals are lowercase only.
// AcceptOnlyFromAmong is case-SENSITIVE, so the literals are lowercase only.
// The audit --channel/--kind/--status options USED to share this contract; they
// dropped AcceptOnlyFromAmong in register row 99 precisely because the
// case-sensitivity rejected values the server accepts. --direction keeps it: it is
// a pure client-side literal that never reaches a server-side parse, so there is no
// authoritative check behind it to defer to.
[InlineData("Up")]
public void TemplateFolderReorder_RejectsAnythingElse(string direction)
{
@@ -255,6 +259,81 @@ public class CommandTreeTests
Assert.NotEmpty(parse.Errors);
}
// ========================================================================
// Register row 99: the audit filter options must NOT be case-sensitively
// gated client-side.
//
// `audit query --status delivered` used to fail at the CLI parse layer while
// the server would have accepted it — AcceptOnlyFromAmong matches ordinally
// against the capitalised literals. The valid set now lives in the help text
// and the server's strict, case-insensitive check is authoritative (the same
// treatment row 88 gave `deploy status`). These tests fail before that change.
// ========================================================================
public static TheoryData<string, string, string> AuditFilterValues() => new()
{
// subcommand, option, value — lowercase, uppercase and mixed case of a real member.
{ "query", "--status", "delivered" },
{ "query", "--status", "DELIVERED" },
{ "query", "--channel", "apioutbound" },
{ "query", "--kind", "apicall" },
{ "export", "--status", "parked" },
{ "export", "--channel", "notification" },
{ "export", "--kind", "dbwrite" },
};
[Theory]
[MemberData(nameof(AuditFilterValues))]
public void AuditFilterOptions_AcceptValuesInAnyCase(string subcommand, string option, string value)
{
var audit = AuditCommands.Build(Url, Format, Username, Password);
var cmd = audit.Subcommands.Single(c => c.Name == subcommand);
// `export` has three required options; supply them so the only thing under
// test is the filter value's own acceptance.
var args = subcommand == "export"
? new[] { "--since", "1h", "--until", "now", "--format", "csv", "--output", "out.csv", option, value }
: new[] { option, value };
var parse = cmd.Parse(args);
Assert.Empty(parse.Errors);
}
[Theory]
[InlineData("csv")]
[InlineData("CSV")]
[InlineData("jsonl")]
public void AuditExport_FormatOption_AcceptsValuesInAnyCase(string format)
{
// The server lowercases --format before matching and returns a 400 naming
// csv/jsonl/parquet for anything else, so the client-side literal gate was
// pure friction (register row 99).
var audit = AuditCommands.Build(Url, Format, Username, Password);
var export = audit.Subcommands.Single(c => c.Name == "export");
var parse = export.Parse(new[]
{
"--since", "1h", "--until", "now", "--format", format, "--output", "out.csv",
});
Assert.Empty(parse.Errors);
}
[Fact]
public void AuditQuery_UnrecognisedFilterValue_IsNotRejectedClientSide()
{
// Deliberate: the CLI no longer second-guesses the filter vocabulary. A bad
// value must reach the server, which answers 400 naming the valid set —
// one authoritative check instead of two that can drift apart.
var audit = AuditCommands.Build(Url, Format, Username, Password);
var query = audit.Subcommands.Single(c => c.Name == "query");
var parse = query.Parse(new[] { "--status", "NotAStatus" });
Assert.Empty(parse.Errors);
}
[Fact]
public void SiteArea_HasMoveVerb()
{
tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/Commands/AuditExportCommandTests.cs
+30 -9
View File
@@ -40,15 +40,34 @@ public class AuditExportCommandTests
}
[Fact]
public void Export_InvalidFormat_Rejected()
public void Export_InvalidFormat_IsNotRejectedAtTheParseLayer()
{
// Register row 99: the client-side AcceptOnlyFromAmong gate is gone. It
// matched case-SENSITIVELY, 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 root = AuditCommandTestHarness.BuildRoot();
var parse = root.Parse(new[]
{
"audit", "export", "--since", "1h", "--until", "0h",
"--format", "xml", "--output", "/tmp/out.xml",
});
Assert.NotEmpty(parse.Errors);
Assert.Empty(parse.Errors);
}
[Theory]
[InlineData("CSV")]
[InlineData("Csv")]
[InlineData("JSONL")]
public void Export_FormatInAnyCase_IsAccepted(string format)
{
var root = AuditCommandTestHarness.BuildRoot();
var parse = root.Parse(new[]
{
"audit", "export", "--since", "1h", "--until", "0h",
"--format", format, "--output", "/tmp/out.csv",
});
Assert.Empty(parse.Errors);
}
// ---- Query string -----------------------------------------------------
@@ -148,16 +167,18 @@ public class AuditExportCommandTests
}
[Fact]
public void Export_MultiValueChannel_WithOneInvalidName_FailsFast()
public void Export_MultiValueChannel_WithOneUnrecognisedName_StillParses()
{
// AcceptOnlyFromAmong validates EACH value of the multi-value option.
// Register row 99: the vocabulary gate moved server-side; the multi-value
// shape itself is unchanged, so both values reach the server to adjudicate.
var root = AuditCommandTestHarness.BuildRoot();
var (exit, _, err) = AuditCommandTestHarness.Invoke(
root, "audit", "export", "--since", "1h", "--until", "0h",
var parse = root.Parse(new[]
{
"audit", "export", "--since", "1h", "--until", "0h",
"--format", "csv", "--output", "/tmp/out.csv",
"--channel", "ApiOutbound", "OutboundApi");
Assert.NotEqual(0, exit);
Assert.NotEqual("", err);
"--channel", "ApiOutbound", "OutboundApi",
});
Assert.Empty(parse.Errors);
}
// ---- Streaming export to file -----------------------------------------
tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/Commands/AuditQueryCommandTests.cs
+35 -27
View File
@@ -424,43 +424,51 @@ public class AuditQueryCommandTests
Assert.Empty(parse.Errors);
}
[Fact]
public void Query_MultiValueChannel_WithOneInvalidName_FailsFast()
// ========================================================================
// Register row 99: filter-vocabulary validation moved OFF the CLI.
//
// These four tests used to assert that AcceptOnlyFromAmong rejected an
// unrecognised --channel/--kind/--status at the parse layer. That gate matched
// case-SENSITIVELY, so it also rejected `--status delivered`, which the server
// has always accepted — the CLI was stricter than the system it drives. The
// gate is gone; the parse layer now accepts any string and the server answers
// 400 naming the valid set (AuditEndpointsTests pins that side).
//
// What is asserted here is the CLIENT half of that contract: the value is
// carried through to the request rather than rejected locally.
// ========================================================================
[Theory]
[InlineData("--channel", "OutboundApi")] // old (non-existent) name; the real enum is "ApiOutbound"
[InlineData("--kind", "CachedCall")]
[InlineData("--status", "Bogus")]
public void Query_UnrecognisedFilterValue_IsNotRejectedAtTheParseLayer(string option, string value)
{
// AcceptOnlyFromAmong validates EACH value of the multi-value option.
var root = AuditCommandTestHarness.BuildRoot();
var (exit, _, err) = AuditCommandTestHarness.Invoke(
root, "audit", "query", "--channel", "ApiOutbound", "OutboundApi");
Assert.NotEqual(0, exit);
Assert.NotEqual("", err);
var parse = root.Parse(new[] { "audit", "query", option, value });
Assert.Empty(parse.Errors);
}
[Fact]
public void Query_ChannelWithInvalidName_FailsFast_NonZeroExit()
public void Query_MultiValueChannel_WithOneUnrecognisedName_StillParses()
{
// "OutboundApi" is the old (non-existent) name; the real enum is "ApiOutbound".
// The multi-value shape is unchanged; only the vocabulary gate is gone, so
// BOTH values reach the server for it to adjudicate.
var root = AuditCommandTestHarness.BuildRoot();
var (exit, _, err) = AuditCommandTestHarness.Invoke(root, "audit", "query", "--channel", "OutboundApi");
Assert.NotEqual(0, exit);
Assert.NotEqual("", err);
Assert.Contains("OutboundApi", err);
var parse = root.Parse(new[] { "audit", "query", "--channel", "ApiOutbound", "OutboundApi" });
Assert.Empty(parse.Errors);
}
[Fact]
public void Query_KindWithInvalidName_FailsFast_NonZeroExit()
[Theory]
[InlineData("--channel", "apioutbound")]
[InlineData("--kind", "apicall")]
[InlineData("--status", "delivered")]
[InlineData("--status", "DELIVERED")]
public void Query_FilterValueInAnyCase_IsAccepted(string option, string value)
{
// The defect row 99 names: these all failed client-side before the fix.
var root = AuditCommandTestHarness.BuildRoot();
var (exit, _, err) = AuditCommandTestHarness.Invoke(root, "audit", "query", "--kind", "CachedCall");
Assert.NotEqual(0, exit);
Assert.NotEqual("", err);
}
[Fact]
public void Query_StatusWithInvalidName_FailsFast_NonZeroExit()
{
var root = AuditCommandTestHarness.BuildRoot();
var (exit, _, err) = AuditCommandTestHarness.Invoke(root, "audit", "query", "--status", "Bogus");
Assert.NotEqual(0, exit);
Assert.NotEqual("", err);
var parse = root.Parse(new[] { "audit", "query", option, value });
Assert.Empty(parse.Errors);
}
}
tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Types/AuditQueryParamParsersTests.cs
+97 -5
View File
@@ -4,11 +4,17 @@ using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums;
namespace ZB.MOM.WW.ScadaBridge.Commons.Tests.Types;
/// <summary>
/// Audit Log #23 (M8): tests for the shared lax multi-value query-param parsers
/// used by the ManagementService + CentralUI audit endpoints and the
/// <c>AuditLogPage</c> drill-in parser. The contract under test: parse each
/// repeated value independently, silently drop unparseable/blank elements, and
/// collapse an empty result to <c>null</c>.
/// Audit Log #23 (M8): tests for the shared multi-value query-param parsers used by
/// the ManagementService + CentralUI audit endpoints and the <c>AuditLogPage</c>
/// drill-in parser. Two contracts are under test:
/// <list type="bullet">
/// <item><b>Lax</b> (<c>ParseEnumList</c>/<c>ParseStringList</c>, used by the
/// CentralUI surfaces): parse each repeated value independently, silently drop
/// unparseable/blank elements, collapse an empty result to <c>null</c>.</item>
/// <item><b>Strict</b> (<c>ParseEnumListStrict</c>, register row 99, used by the
/// ManagementService endpoints the CLI calls): an unrecognised value is an error
/// naming the valid set; blanks are still dropped.</item>
/// </list>
/// </summary>
public class AuditQueryParamParsersTests
{
@@ -53,6 +59,92 @@ public class AuditQueryParamParsersTests
Assert.Null(AuditQueryParamParsers.ParseEnumList<AuditStatus>(new[] { "Bogus", "" }));
}
// ========================================================================
// Strict variant (register row 99) — the contract the ManagementService
// /api/audit/query + /api/audit/export endpoints use, so the CLI can drop its
// case-SENSITIVE AcceptOnlyFromAmong guard and still fail loudly on garbage.
// ========================================================================
[Fact]
public void ParseEnumListStrict_NullInput_ReturnsNull()
{
Assert.Null(AuditQueryParamParsers.ParseEnumListStrict<AuditStatus>(null, "status"));
}
[Fact]
public void ParseEnumListStrict_EmptyInput_ReturnsNull()
{
Assert.Null(AuditQueryParamParsers.ParseEnumListStrict<AuditStatus>(Array.Empty<string?>(), "status"));
}
[Theory]
[InlineData("delivered")]
[InlineData("DELIVERED")]
[InlineData("Delivered")]
[InlineData(" delivered ")]
public void ParseEnumListStrict_IsCaseInsensitiveAndTrims(string raw)
{
var result = AuditQueryParamParsers.ParseEnumListStrict<AuditStatus>(new[] { raw }, "status");
Assert.Equal(new[] { AuditStatus.Delivered }, result);
}
[Fact]
public void ParseEnumListStrict_AllValuesValid_ParsesEverything()
{
var result = AuditQueryParamParsers.ParseEnumListStrict<AuditChannel>(
new[] { "ApiOutbound", "dboutbound" }, "channel");
Assert.Equal(new[] { AuditChannel.ApiOutbound, AuditChannel.DbOutbound }, result);
}
[Fact]
public void ParseEnumListStrict_UnrecognisedValue_ThrowsNamingTheValidSet()
{
var ex = Assert.Throws<AuditFilterParseException>(
() => AuditQueryParamParsers.ParseEnumListStrict<AuditStatus>(
new[] { "Delivered", "NotAStatus" }, "status"));
// The offending value AND the whole valid vocabulary must be in the message —
// naming the valid set is the entire point of the check.
Assert.Contains("NotAStatus", ex.Message);
Assert.Contains("status", ex.Message);
foreach (var name in Enum.GetNames<AuditStatus>())
{
Assert.Contains(name, ex.Message);
}
}
[Theory]
// "7" is out of range; "3" lands on a REAL member. Enum.TryParse accepts BOTH —
// a filter is a name-based contract, never an ordinal one, so both are rejected.
[InlineData("7")]
[InlineData("3")]
[InlineData("0")]
[InlineData("-1")]
[InlineData("+2")]
public void ParseEnumListStrict_NumericString_IsRejected(string raw)
{
var ex = Assert.Throws<AuditFilterParseException>(
() => AuditQueryParamParsers.ParseEnumListStrict<AuditStatus>(new[] { raw }, "status"));
Assert.Contains(raw, ex.Message);
}
[Fact]
public void ParseEnumListStrict_BlankElements_AreStillDroppedSilently()
{
// A repeated query param can legitimately carry an empty slot; only a
// NON-blank unrecognised value is an error.
var result = AuditQueryParamParsers.ParseEnumListStrict<AuditStatus>(
new[] { "Delivered", "", " ", null }, "status");
Assert.Equal(new[] { AuditStatus.Delivered }, result);
}
[Fact]
public void ParseEnumListStrict_AllBlank_ReturnsNull()
{
Assert.Null(AuditQueryParamParsers.ParseEnumListStrict<AuditStatus>(
new[] { "", " ", null }, "status"));
}
[Fact]
public void ParseStringList_NullInput_ReturnsNull()
{
tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/AuditEndpointsTests.cs
+63 -6
View File
@@ -416,23 +416,80 @@ public class AuditEndpointsTests
Assert.Null(filter.SourceSiteIds);
}
// ========================================================================
// Register row 99: the enum filter dimensions parse STRICTLY here.
//
// These used to assert the LAX contract — an unrecognised enum name was
// dropped and the query ran unfiltered on that dimension, so `--status
// Nonsense` returned a plausible, complete-looking page that did not answer
// the question asked. That is exactly the failure row 88 fixed for `deploy
// status`, and it became reachable from the CLI once the (case-SENSITIVE,
// and therefore over-strict) client-side AcceptOnlyFromAmong guard was
// removed. The Guid/date dimensions stay lax — see ParseFilter's remarks.
// ========================================================================
[Fact]
public void ParseFilter_UnparseableValuesInRepeatedSet_AreDroppedSilently()
public void ParseFilter_UnrecognisedEnumValue_Throws()
{
// Lax-parse contract: an unrecognised enum name is dropped, the rest of
// the repeated set survives — no 400, no whole-set drop.
var query = new Microsoft.AspNetCore.Http.QueryCollection(
new Dictionary<string, Microsoft.Extensions.Primitives.StringValues>
{
["channel"] = new[] { "ApiOutbound", "Bogus", "Notification" },
});
var ex = Assert.Throws<AuditFilterParseException>(() => AuditEndpoints.ParseFilter(query));
Assert.Contains("Bogus", ex.Message);
Assert.Contains("channel", ex.Message);
}
[Fact]
public void ParseFilter_UnrecognisedStatus_ThrowsNamingTheValidSet()
{
var query = new Microsoft.AspNetCore.Http.QueryCollection(
new Dictionary<string, Microsoft.Extensions.Primitives.StringValues>
{
["status"] = new[] { "Nonsense" },
});
var ex = Assert.Throws<AuditFilterParseException>(() => AuditEndpoints.ParseFilter(query));
foreach (var name in Enum.GetNames<AuditStatus>())
{
Assert.Contains(name, ex.Message);
}
}
[Fact]
public void ParseFilter_LowercaseEnumValues_AreAccepted()
{
// The CLI sends whatever the operator typed now that its own case-sensitive
// guard is gone; `--status delivered` must resolve here.
var query = new Microsoft.AspNetCore.Http.QueryCollection(
new Dictionary<string, Microsoft.Extensions.Primitives.StringValues>
{
["channel"] = new[] { "apioutbound" },
["kind"] = new[] { "apicall" },
["status"] = new[] { "delivered", "PARKED" },
});
var filter = AuditEndpoints.ParseFilter(query);
Assert.Equal(new[] { AuditChannel.ApiOutbound, AuditChannel.Notification }, filter.Channels);
// Every value unparseable → the dimension stays unconstrained (null).
Assert.Null(filter.Statuses);
Assert.Equal(new[] { AuditChannel.ApiOutbound }, filter.Channels);
Assert.Equal(new[] { AuditKind.ApiCall }, filter.Kinds);
Assert.Equal(new[] { AuditStatus.Delivered, AuditStatus.Parked }, filter.Statuses);
}
[Fact]
public void ParseFilter_NumericEnumValue_IsRejected()
{
// Enum.TryParse would accept "3" as an ordinal; a filter is a name-based
// contract, so it must not.
var query = new Microsoft.AspNetCore.Http.QueryCollection(
new Dictionary<string, Microsoft.Extensions.Primitives.StringValues>
{
["status"] = new[] { "3" },
});
Assert.Throws<AuditFilterParseException>(() => AuditEndpoints.ParseFilter(query));
}
[Fact]
tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs
+187
View File
@@ -985,6 +985,193 @@ public class ManagementActorTests : TestKit, IDisposable
ExpectMsg<ManagementSuccess>(TimeSpan.FromSeconds(5));
}
// ========================================================================
// Register row 98: the scope gate fails CLOSED on an unresolvable target.
//
// EnforceSiteScopeForInstance / EnforceSiteScopeForIdentifier used to skip
// EnforceSiteScope entirely when the id did not resolve, so a site-scoped
// caller PASSED the gate and fell through into the handler body. A gate that
// cannot prove the target is inside the caller's scope must treat it as
// outside. System-wide callers return at the guards above the lookup and are
// untouched — they still get the handler's ordinary "not found".
// ========================================================================
[Fact]
public void GetInstance_NonexistentIdForSiteScopedUser_ReturnsUnauthorized()
{
// _templateRepo is unstubbed for id 999 → GetInstanceByIdAsync returns null.
var actor = CreateActor();
var envelope = ScopedEnvelope(new GetInstanceCommand(999), new[] { "1" }, "Deployer");
actor.Tell(envelope);
var response = ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
Assert.Equal(envelope.CorrelationId, response.CorrelationId);
// The denial must not double as an existence oracle: the message says the
// instance is not in scope, never whether it exists.
Assert.DoesNotContain("not found", response.Message, StringComparison.OrdinalIgnoreCase);
Assert.DoesNotContain("exist", response.Message, StringComparison.OrdinalIgnoreCase);
}
[Fact]
public void GetInstance_NonexistentIdForSiteScopedUser_IsIndistinguishableFromOutOfScope()
{
// The security property: a scoped caller cannot tell "instance 7 lives at a
// site you may not see" apart from "instance 999 does not exist" — both are
// the same ManagementUnauthorized shape.
_templateRepo.GetInstanceByIdAsync(7, Arg.Any<CancellationToken>())
.Returns(new Instance("Pump7") { Id = 7, SiteId = 2 });
var actor = CreateActor();
actor.Tell(ScopedEnvelope(new GetInstanceCommand(7), new[] { "1" }, "Deployer"));
var outOfScope = ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
actor.Tell(ScopedEnvelope(new GetInstanceCommand(999), new[] { "1" }, "Deployer"));
var nonexistent = ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
Assert.Equal(outOfScope.GetType(), nonexistent.GetType());
}
[Fact]
public void GetInstance_NonexistentIdForAdminUser_IsUnchanged()
{
// An Administrator is system-wide, so the gate returns before the lookup:
// the handler runs and answers for itself. NOT a ManagementUnauthorized.
var actor = CreateActor();
var envelope = ScopedEnvelope(new GetInstanceCommand(999), new[] { "1" }, "Administrator");
actor.Tell(envelope);
var response = ExpectMsg<object>(TimeSpan.FromSeconds(5));
Assert.IsNotType<ManagementUnauthorized>(response);
}
[Fact]
public void GetInstance_NonexistentIdForSystemWideDeployer_IsUnchanged()
{
// The other system-wide arm: a Deployer with NO site grants. Same rule,
// same untouched behaviour.
var actor = CreateActor();
var envelope = ScopedEnvelope(new GetInstanceCommand(999), Array.Empty<string>(), "Deployer");
actor.Tell(envelope);
var response = ExpectMsg<object>(TimeSpan.FromSeconds(5));
Assert.IsNotType<ManagementUnauthorized>(response);
}
[Fact]
public void GetInstance_InScopeIdForSiteScopedUser_IsStillAllowed()
{
// The fail-closed change must not deny a legitimate in-scope target.
_templateRepo.GetInstanceByIdAsync(7, Arg.Any<CancellationToken>())
.Returns(new Instance("Pump7") { Id = 7, SiteId = 1 });
var actor = CreateActor();
var envelope = ScopedEnvelope(new GetInstanceCommand(7), new[] { "1" }, "Deployer");
actor.Tell(envelope);
ExpectMsg<ManagementSuccess>(TimeSpan.FromSeconds(5));
}
[Fact]
public void GetSite_NonexistentIdForSiteScopedUser_ReturnsUnauthorized()
{
// HandleGetSite resolves the site itself and carried its own copy of the
// fail-OPEN shape (`if (site != null) EnforceSiteScope(...)`), so a scoped
// caller used to get success/null for any id — an enumeration oracle.
var siteRepo = Substitute.For<ISiteRepository>();
siteRepo.GetSiteByIdAsync(999, Arg.Any<CancellationToken>())
.Returns((Commons.Entities.Sites.Site?)null);
_services.AddScoped(_ => siteRepo);
var actor = CreateActor();
var envelope = ScopedEnvelope(new GetSiteCommand(999), new[] { "1" }, "Deployer");
actor.Tell(envelope);
var response = ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
Assert.Equal(envelope.CorrelationId, response.CorrelationId);
}
[Fact]
public void GetSite_NonexistentIdForAdminUser_IsUnchanged()
{
var siteRepo = Substitute.For<ISiteRepository>();
siteRepo.GetSiteByIdAsync(999, Arg.Any<CancellationToken>())
.Returns((Commons.Entities.Sites.Site?)null);
_services.AddScoped(_ => siteRepo);
var actor = CreateActor();
var envelope = ScopedEnvelope(new GetSiteCommand(999), new[] { "1" }, "Administrator");
actor.Tell(envelope);
var response = ExpectMsg<object>(TimeSpan.FromSeconds(5));
Assert.IsNotType<ManagementUnauthorized>(response);
}
[Fact]
public void GetDataConnection_NonexistentIdForSiteScopedUser_ReturnsUnauthorized()
{
// Third copy of the same inline fail-OPEN shape.
var siteRepo = Substitute.For<ISiteRepository>();
siteRepo.GetDataConnectionByIdAsync(999, Arg.Any<CancellationToken>())
.Returns((Commons.Entities.Sites.DataConnection?)null);
_services.AddScoped(_ => siteRepo);
var actor = CreateActor();
var envelope = ScopedEnvelope(new GetDataConnectionCommand(999), new[] { "1" }, "Deployer");
actor.Tell(envelope);
var response = ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
Assert.Equal(envelope.CorrelationId, response.CorrelationId);
}
[Fact]
public void QueryEventLogs_NonexistentSiteIdentifierForSiteScopedUser_ReturnsUnauthorized()
{
// EnforceSiteScopeForIdentifier shares the shape: an identifier that
// resolves to no Site row is a denial for a scoped caller.
var siteRepo = Substitute.For<ISiteRepository>();
siteRepo.GetSiteByIdentifierAsync("GHOST", Arg.Any<CancellationToken>())
.Returns((Commons.Entities.Sites.Site?)null);
_services.AddScoped(_ => siteRepo);
var actor = CreateActor();
var envelope = ScopedEnvelope(
new QueryEventLogsCommand("GHOST"),
new[] { "1" },
"Deployer");
actor.Tell(envelope);
var response = ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
Assert.Equal(envelope.CorrelationId, response.CorrelationId);
}
[Fact]
public void QueryEventLogs_InScopeSiteIdentifierForSiteScopedUser_IsStillAllowed()
{
AddSiteRepoWithSite(1, "SITE1");
var actor = CreateActor();
var envelope = ScopedEnvelope(
new QueryEventLogsCommand("SITE1"),
new[] { "1" },
"Deployer");
actor.Tell(envelope);
// The scope gate passes; whatever the (unwired) CommunicationService does
// next is not this test's business — only that it is NOT a scope denial.
var response = ExpectMsg<object>(TimeSpan.FromSeconds(5));
Assert.IsNotType<ManagementUnauthorized>(response);
}
[Fact]
public void GetSite_OutOfScopeForSiteScopedUser_ReturnsUnauthorized()
{