From 9f67147e7b4a53ffbdfe9ec330ff053dbad4982a Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 17 Aug 2026 05:15:18 -0400 Subject: [PATCH] refactor(deployments): one paged read of DeploymentRecords + close its three drifts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deferred-work register rows 37, 86, 87, 88 — the structural duplication and the three behavioral divergences it had accumulated, fixed together. Row 37 (structural). `IDeploymentManagerRepository.QueryDeploymentSummariesAsync` is DELETED. It and `QueryDeploymentListPageAsync` were two paged reads of the same insert-only table with different contracts — raw offset vs page number, no total vs total, no instance join vs joined, instance-id-list scoping vs site-id scoping. The CLI/HTTP `QueryDeployments` handler now goes through `QueryDeploymentListPageAsync` with a `DeploymentListFilter`, exactly like the Central UI page. The `DeploymentRecordSummary` projection went with it (`DeploymentListRow` is the single row shape); its `MaxPageSize` const moved to `DeploymentListPage`, which also gains `DefaultPageSize`. Row 86 (Medium, authorization — the headline). `SiteScopeService.ResolveAsync` was claims-only, so an Administrator carrying `SiteId` claims saw the whole fleet through the CLI (`ManagementActor.EnforceSiteScope` has always exempted Administrator) and only their granted sites on the Deployments page. One user, one dataset, two answers. Administrator = system-wide is the intended semantic on both sides; the UI was the defect. The role claim is read explicitly with an OrdinalIgnoreCase comparison rather than via `IsInRole`, which compares claim VALUES ordinally and only resolves when the identity carries the matching roleType — the two sides must not disagree on a differently-cased role name. An Administrator's site claims are not collected at all, so the invariant "system-wide => empty permitted-id set" holds for every caller. Designer is NOT exempt: the class doc claimed "any Admin/Design user" is system-wide, which was true of neither side, so the doc was corrected rather than the bypass widened. Row 87 (Low). The scoped branch used to resolve the site grant into an instance-id list by loading EVERY instance in the system, non-transactionally, on every scoped query; the grant now rides as `SiteIdScope` and is resolved by the query's own join. An unparseable permitted-site id is DROPPED, never ignored — dropping can only narrow a grant, discarding it would widen it to fleet-wide. And `PageSize <= 0` no longer means UNLIMITED down at the repository (`--page-size 0` materialized the whole retention window): it means "unspecified" and takes the CLI's own default of 50, capped at 1000. Bounding without telling would be silent truncation, so the handler now returns a `DeploymentQueryResult` envelope carrying `totalCount` plus the page coordinates actually applied — with a CLI table projection (mirroring row 57's `AuditConfigTableProjection`) that unwraps it and appends a "showing N of M" note. Row 88 (Low, UX). An unrecognised `--status` fell back to "no filter", so a typo like `--status Sucess` returned every status — a complete-looking answer to a question the caller never asked. It is now a `ManagementError` naming the valid values. Parsing stays case-insensitive; a numeric string is rejected too, since `Enum.TryParse` accepts "7" and yields an undefined value (guarded with `Enum.IsDefined`). Loud failure over silent full results, per row 46. Tests: 7 new `SiteScopeServiceTests` pinning Administrator = system-wide (verified failing 6/7 against the pre-fix service, the Designer pin passing both ways); the ManagementActor deployment suite migrated onto the consolidated query with new pins for the status error, case-insensitive parse, numeric rejection, page-size default/ cap/floor, empty-scope-on-garbage-grant, and "reads no instances at all"; a repository test for site-scope+status composing over a bounded page; and 6 CLI tests for the new table projection. --- docs/components/CentralUI.md | 4 +- .../sql/AddDeploymentRecordListSortIndex.sql | 6 +- docs/requirements/Component-CentralUI.md | 1 + .../Component-ManagementService.md | 9 +- docs/requirements/Component-Security.md | 1 + .../Commands/DeployCommands.cs | 15 +- .../DeploymentStatusTableProjection.cs | 120 ++++++++ src/ZB.MOM.WW.ScadaBridge.CLI/README.md | 18 +- .../Auth/SiteScopeService.cs | 50 +++- .../IDeploymentManagerRepository.cs | 44 ++- .../Messages/Management/DeploymentCommands.cs | 32 +++ .../Types/Deployment/DeploymentListPage.cs | 18 ++ .../Types/Deployment/DeploymentListRow.cs | 9 +- .../Deployment/DeploymentRecordSummary.cs | 54 ---- .../Configurations/DeploymentConfiguration.cs | 5 +- .../DeploymentManagerRepository.cs | 54 +--- .../ManagementActor.cs | 124 ++++++--- .../DeploymentStatusTableProjectionTests.cs | 89 ++++++ .../Auth/SiteScopeServiceTests.cs | 71 +++++ .../DeploymentListQueryRepositoryTests.cs | 33 ++- .../ManagementActorTests.cs | 259 +++++++++++++----- 21 files changed, 760 insertions(+), 256 deletions(-) create mode 100644 src/ZB.MOM.WW.ScadaBridge.CLI/Commands/DeploymentStatusTableProjection.cs delete mode 100644 src/ZB.MOM.WW.ScadaBridge.Commons/Types/Deployment/DeploymentRecordSummary.cs create mode 100644 tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/DeploymentStatusTableProjectionTests.cs diff --git a/docs/components/CentralUI.md b/docs/components/CentralUI.md index 08e8de53..315fab77 100644 --- a/docs/components/CentralUI.md +++ b/docs/components/CentralUI.md @@ -51,7 +51,9 @@ Because `CookieAuthenticationStateProvider` serves a frozen principal, the circu ### Site-scoped authorization -`SiteScopeService` (scoped, registered in `AddCentralUI`) reads `SiteId` claims from the circuit principal. Deployment users whose LDAP mapping carries site-scope rules carry one integer `SiteId` claim per permitted site; Admin and Design users carry none (system-wide). Pages that enumerate sites or issue cross-site commands call `SiteScopeService.FilterSitesAsync` and `IsSiteAllowedAsync` to enforce the grant before any operation. +`SiteScopeService` (scoped, registered in `AddCentralUI`) reads the role and `SiteId` claims off the circuit principal. Deployment users whose LDAP mapping carries site-scope rules carry one integer `SiteId` claim per permitted site; a user with no `SiteId` claims is system-wide. Pages that enumerate sites or issue cross-site commands call `SiteScopeService.FilterSitesAsync` and `IsSiteAllowedAsync` to enforce the grant before any operation. + +**A user holding the `Administrator` role is system-wide regardless of the `SiteId` claims they carry** (deferred-work register row 86). `ManagementActor.EnforceSiteScope` has always exempted Administrator, so before this the same user could act fleet-wide through the CLI/HTTP surface while the UI hid the same sites — one user, one dataset, two answers. The role claim is read explicitly (`JwtTokenService.RoleClaimType`, `OrdinalIgnoreCase`) rather than via `ClaimsPrincipal.IsInRole`, because `IsInRole` compares claim *values* ordinally and only resolves at all when the identity was built with the matching `roleType`; the actor compares case-insensitively, and the two sides must not disagree on a differently-cased role name. **`Designer` is NOT exempt** — the enforced rule has only ever bypassed Administrator, and this document previously claimed "Admin and Design users carry none (system-wide)", which was true of neither side. An Administrator's `SiteId` claims are not collected at all, so the invariant *system-wide ⇒ empty permitted-id set* holds for every caller and a consumer reading the id set without consulting `IsSystemWideAsync` cannot silently narrow an Administrator to a subset. ### Repository access pattern diff --git a/docs/plans/sql/AddDeploymentRecordListSortIndex.sql b/docs/plans/sql/AddDeploymentRecordListSortIndex.sql index 1f537ae9..1bfcc7de 100644 --- a/docs/plans/sql/AddDeploymentRecordListSortIndex.sql +++ b/docs/plans/sql/AddDeploymentRecordListSortIndex.sql @@ -2,8 +2,10 @@ -- -- Replaces the single-column [IX_DeploymentRecords_DeployedAt] with the composite -- [IX_DeploymentRecords_DeployedAt_Id] ([DeployedAt] DESC, [Id] DESC), which is exactly --- the order DeploymentManagerRepository.QueryDeploymentListPageAsync (and --- QueryDeploymentSummariesAsync) asks for. With both key columns descending, a forward +-- the order DeploymentManagerRepository.QueryDeploymentListPageAsync asks for. (It named +-- QueryDeploymentSummariesAsync too; register row 37 folded that second paged read into +-- QueryDeploymentListPageAsync, so both the UI and the CLI now share this one key.) +-- With both key columns descending, a forward -- scan delivers the requested order, so a paged Skip/Take reads a Top-N off the index -- instead of sorting the whole filtered set on every page request. -- diff --git a/docs/requirements/Component-CentralUI.md b/docs/requirements/Component-CentralUI.md index c1a65cd1..f75b1008 100644 --- a/docs/requirements/Component-CentralUI.md +++ b/docs/requirements/Component-CentralUI.md @@ -149,6 +149,7 @@ Central cluster only. Sites have no user interface. - `IDeploymentManagerRepository.QueryDeploymentListPageAsync(filter, pageNumber, pageSize)` returns one page of `DeploymentListRow` — `DeploymentRecord` **inner-joined to `Instance`**, so the instance's display name and site travel with the rows that need them (the join is exact: the FK is `Restrict` and instance deletion removes the records first) — plus the **total count** of the filtered set. - `GetDeploymentStatusCountsAsync(filter)` returns the tile counts from **one grouped aggregation**. It deliberately **ignores the filter's `Status`** — the tiles are the status *breakdown* of the otherwise-identically-filtered set, so each keeps its own total while it is the selected one. - **Site scoping runs in the query.** The permitted-site grant is pushed in as `SiteIdScope` and resolved through the record's instance; an **empty** grant is a real filter matching nothing, never "unconstrained". `DeploymentRecord` has no `SiteId` of its own — the join is what makes this expressible. + - **This is now the ONLY paged read of `DeploymentRecords`.** The CLI/HTTP `QueryDeployments` command had a second one (`QueryDeploymentSummariesAsync`: offset instead of page number, no total, no instance join, instance-id-list scoping); it is **deleted**, and both surfaces share this query. See `Component-ManagementService.md` → *QueryDeployments* for the three behavioral drifts the duplication had accumulated (deferred-work register rows 37, 86–88). - **Offset paging, not the Audit Log's keyset cursor — deliberately.** This page's pager is numbered and jump-to-any-page, so it needs a page count, which only a total can give it; a keyset cursor can express neither. The total is required for the tiles regardless. The deep-offset cost that pushes high-volume tables to keyset is bounded here by the terminal-record retention purge, unlike the 365-day central `AuditLog`. This mirrors the Notification Outbox, which is offset-paged for the same reason. Ordering is `DeployedAt DESC, Id DESC`; the `Id` tie-break is load-bearing, because `DeployedAt` ties on rapid redeploys and an unstable sort key makes offset paging repeat or drop rows between pages. - The whole-table `GetAllDeploymentRecordsAsync` read was **deleted** with its last caller. - **Filtering.** Status filter (the tiles double as the control — clicking one applies it, clicking it again clears) plus a free-text search matched DB-side against instance unique name, deployment id, revision hash and initiating user. A filter change resets to page 1. Search input is **trailing-edge debounced at 500 ms**, so a typed term is one query rather than one per keystroke. diff --git a/docs/requirements/Component-ManagementService.md b/docs/requirements/Component-ManagementService.md index ac5fa61b..aefc8ba2 100644 --- a/docs/requirements/Component-ManagementService.md +++ b/docs/requirements/Component-ManagementService.md @@ -147,7 +147,14 @@ The whole folder surface is reachable from the CLI as `template folder list|crea - **DeployArtifacts**: Deploy system-wide artifacts (shared scripts, external system definitions, DB connections, data connections) to all sites or a specific site. The command's `SiteId` is honored (arch-review C2): a value routes to the single-site deploy path (`ArtifactDeploymentService.DeployToSiteAsync`), while `null` deploys fleet-wide (`DeployToAllSitesAsync`). Site scope is enforced — a site-scoped (non-Administrator) Deployer may only target a site within its `PermittedSiteIds`, and may **not** deploy fleet-wide (`SiteId is null` is rejected with `SiteScopeViolationException` → `ManagementUnauthorized`, since `EnforceSiteScope(null)` is a deliberate no-op); fleet-wide deployment requires a system-wide Deployer or Administrator. - **DeploySite** (`MgmtDeploySite`, WP2.5): Bulk-deploy every deployable instance at one site — see the Instances section above and `Component-DeploymentManager.md`. - **GetDeploymentStatus**: Query deployment status. -- **QueryDeployments** (`InstanceId?`, `Status?`, `Page`, `PageSize`): List deployment records. **All four arguments are honoured DB-side (WP2.5)** and the result is a `DeploymentRecordSummary` projection. The handler previously ignored `Status`, `Page` and `PageSize` entirely (the CLI had been sending them all along) and loaded the whole insert-only `DeploymentRecords` table; for a site-scoped user it additionally loaded every instance and intersected the two sets in memory. Site scope is now resolved to the set of in-scope instance ids and pushed into the query as an id filter — an **empty** scope stays a real filter (a user permitted no in-scope instances sees nothing), never a no-op. This read path also drives the opportunistic, rate-limited terminal-record retention sweep described in `Component-DeploymentManager.md`. +- **QueryDeployments** (`InstanceId?`, `Status?`, `Page`, `PageSize`): List deployment records. **All four arguments are honoured DB-side (WP2.5)** and the result is a `DeploymentQueryResult` envelope — `{ deployments, totalCount, page, pageSize }` over `DeploymentListRow`. The handler previously ignored `Status`, `Page` and `PageSize` entirely (the CLI had been sending them all along) and loaded the whole insert-only `DeploymentRecords` table; for a site-scoped user it additionally loaded every instance and intersected the two sets in memory. This read path also drives the opportunistic, rate-limited terminal-record retention sweep described in `Component-DeploymentManager.md`. + + **One paged read, three drifts closed (deferred-work register rows 37, 86–88).** This handler used to own a second repository query, `QueryDeploymentSummariesAsync`, alongside the Central UI page's `QueryDeploymentListPageAsync`. That query is **deleted**; both surfaces now go through `QueryDeploymentListPageAsync` with a `DeploymentListFilter`, and the `DeploymentRecordSummary` projection went with it (`DeploymentListRow` is the single row shape). The duplication had drifted into three behavioral differences, all resolved here: + - **Site scope is a SITE-ID filter, resolved by the query's own join to `Instance`** (row 87). It used to be resolved in the handler by loading **every instance in the system** on every scoped query, purely to turn the site grant into an instance-id list — a second whole-table read, non-transactional with the one that mattered. An **empty** scope remains a real filter (a user permitted no sites sees nothing), never a no-op, and a permitted-site id that is not an integer is **dropped rather than ignored** — dropping can only narrow the grant, whereas discarding the grant would widen it to fleet-wide. + - **`PageSize` is bounded** (row 87). A non-positive value used to mean *unlimited* all the way down to the repository, so `--page-size 0` materialized the entire retention window into one response. It now means "unspecified" and takes `DeploymentListPage.DefaultPageSize` (50, the CLI's own default); every value is capped at `DeploymentListPage.MaxPageSize` (1000). `totalCount` travels with the bounded page precisely so a truncated page is distinguishable from a complete one. + - **An unrecognised `Status` is an ERROR, not "no filter"** (row 88). `--status Sucess` used to fall back to unfiltered and return every status — a plausible, complete-looking answer to a question the caller never asked. It now returns `ManagementError` naming the valid values (`Pending, InProgress, Success, Failed`). Parsing stays case-insensitive; a numeric string is rejected too (`Enum.TryParse` accepts `"7"` and yields an undefined value, so the parse is guarded by `Enum.IsDefined`). Same loud-failure principle as register row 46. + + **Administrator is system-wide** on this path, as on every other site-scoped handler (`EnforceSiteScope`). The Central UI's `SiteScopeService` was claims-only and disagreed — an Administrator carrying `SiteId` claims saw the fleet here and only their sites on the Deployments page. The UI was corrected to match (register row 86); see `Component-CentralUI.md`. ### Secured Writes (MxGateway, two-person) diff --git a/docs/requirements/Component-Security.md b/docs/requirements/Component-Security.md index 6a59f0fe..d5f5f473 100644 --- a/docs/requirements/Component-Security.md +++ b/docs/requirements/Component-Security.md @@ -116,6 +116,7 @@ Set in a local or docker-dev environment via the environment variable `ScadaBrid - Monitor and manage the Notification Outbox (retry and discard parked notifications). - 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). > **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/DeployCommands.cs b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/DeployCommands.cs index 08b43a3d..4ff88e96 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/DeployCommands.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/DeployCommands.cs @@ -94,10 +94,18 @@ public static class DeployCommands private static Command BuildStatus(Option urlOption, Option formatOption, Option usernameOption, Option passwordOption) { var instanceIdOption = new Option("--instance-id") { Description = "Filter by instance ID" }; - var statusOption = new Option("--status") { Description = "Filter by status" }; + // The valid set is named in the help text rather than enforced with + // AcceptOnlyFromAmong: that check is case-SENSITIVE, and the server has + // always accepted `--status success`. The server rejects an unrecognised + // value with the same list (register row 88) and is the authoritative + // check — HTTP API callers never pass through this option at all. + var statusOption = new Option("--status") + { + Description = "Filter by status (Pending, InProgress, Success, Failed; case-insensitive)", + }; var pageOption = new Option("--page") { Description = "Page number" }; pageOption.DefaultValueFactory = _ => 1; - var pageSizeOption = new Option("--page-size") { Description = "Page size" }; + var pageSizeOption = new Option("--page-size") { Description = "Page size (1-1000; 0 or unset uses 50)" }; pageSizeOption.DefaultValueFactory = _ => 50; var cmd = new Command("status") { Description = "Query deployment status" }; @@ -113,7 +121,8 @@ public static class DeployCommands var pageSize = result.GetValue(pageSizeOption); return await CommandHelpers.ExecuteCommandAsync( result, urlOption, formatOption, usernameOption, passwordOption, - new QueryDeploymentsCommand(instanceId, status, page, pageSize)); + new QueryDeploymentsCommand(instanceId, status, page, pageSize), + tableProjector: DeploymentStatusTableProjection.ProjectRows); }); return cmd; } diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/DeploymentStatusTableProjection.cs b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/DeploymentStatusTableProjection.cs new file mode 100644 index 00000000..d98aa916 --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/DeploymentStatusTableProjection.cs @@ -0,0 +1,120 @@ +using System.Text.Json; +using System.Text.Json.Nodes; + +namespace ZB.MOM.WW.ScadaBridge.CLI.Commands; + +/// +/// Table projection for deploy status (deferred-work register rows 37/87). +/// +/// +/// The handler now returns a DeploymentQueryResult envelope — +/// { "deployments": [ … ], "totalCount": N, "page": P, "pageSize": S } — where +/// it used to return a bare row array. Handed to the generic renderer unprojected, +/// an object takes the Property/Value branch and the whole deployments array +/// lands in a single unreadable cell. This unwraps the envelope to the row array the +/// renderer expects, exactly as does for +/// audit-config query. +/// +/// +/// +/// The envelope's counts would be lost by a plain unwrap, and losing THOSE is the one +/// thing this projection must not do: the page size is bounded server-side now, so a +/// full page is no longer proof that there are no more rows. When the page is a strict +/// subset a single trailing row carries a note cell saying how many of how many +/// are shown and how to get the rest. When the page IS the whole result set the column +/// is not emitted at all. +/// +/// +/// +/// JSON output is never touched — this runs only on the table path. +/// +/// +internal static class DeploymentStatusTableProjection +{ + /// + /// Projects a DeploymentQueryResult envelope (or a bare row array) to the + /// row array the table renderer consumes. Returns the input unchanged when it is + /// not JSON or not one of those shapes, so the renderer's own fallbacks apply. + /// + /// The raw success JSON body from the management API. + /// JSON suitable for table rendering. + internal static string ProjectRows(string json) + { + JsonDocument doc; + try + { + doc = JsonDocument.Parse(json); + } + catch (JsonException) + { + // Not JSON (e.g. a proxy error page) — let the renderer print it verbatim. + return json; + } + + using (doc) + { + var root = doc.RootElement; + + // A bare array is accepted so the command still renders usefully against + // an older server that returns the pre-envelope shape. + if (root.ValueKind == JsonValueKind.Array) + return BuildRows(root, totalCount: null).ToJsonString(); + + if (root.ValueKind == JsonValueKind.Object + && TryGetPropertyCI(root, "deployments", out var rows) + && rows.ValueKind == JsonValueKind.Array) + { + int? total = TryGetPropertyCI(root, "totalCount", out var tc) + && tc.ValueKind == JsonValueKind.Number + && tc.TryGetInt32(out var n) + ? n + : null; + return BuildRows(rows, total).ToJsonString(); + } + + // Some other object (e.g. an error envelope) — leave it to the renderer. + return json; + } + } + + private static JsonArray BuildRows(JsonElement rows, int? totalCount) + { + var arr = new JsonArray(); + var shown = 0; + foreach (var item in rows.EnumerateArray()) + { + arr.Add(item.ValueKind == JsonValueKind.Object + ? JsonNode.Parse(item.GetRawText())! + : JsonValue.Create(item.ToString())!); + shown++; + } + + // Only when the page is a strict subset — otherwise the count is just the row + // count the operator can already see, and the extra column would be noise. + if (totalCount is int total && total > shown) + { + arr.Add(new JsonObject + { + ["note"] = JsonValue.Create( + $"showing {shown} of {total} — use --page / --page-size for the rest"), + }); + } + + return arr; + } + + private static bool TryGetPropertyCI(JsonElement obj, string name, out JsonElement value) + { + foreach (var prop in obj.EnumerateObject()) + { + if (string.Equals(prop.Name, name, StringComparison.OrdinalIgnoreCase)) + { + value = prop.Value; + return true; + } + } + + value = default; + return false; + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md index 0555e0dc..dd9a0772 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md @@ -1011,9 +1011,23 @@ scadabridge --url deploy status [--instance-id ] [--status ] | Option | Required | Default | Description | |--------|----------|---------|-------------| | `--instance-id` | no | — | Filter by instance ID | -| `--status` | no | — | Filter by deployment status string | +| `--status` | no | — | Filter by deployment status: `Pending`, `InProgress`, `Success`, `Failed` (case-insensitive) | | `--page` | no | `1` | Page number | -| `--page-size` | no | `50` | Results per page | +| `--page-size` | no | `50` | Results per page; capped at `1000`. `0` means "unspecified" and takes the default — it does **not** mean unlimited | + +Output is an envelope: `deployments` (the page of rows, newest first), `totalCount` +(rows matching the filter across all pages), plus the `page` and `pageSize` the server +actually applied. `--format table` renders the rows and appends a `showing N of M` +note when the page is a strict subset. + +An unrecognised `--status` is an **error** naming the valid values, not a silent +fallback to "all statuses" — a typo like `--status Sucess` used to return every +deployment record, which looks exactly like a complete answer to the question you +meant to ask. + +Site scoping: a site-scoped Deployer sees only deployments for instances at their +permitted sites. A user holding `Administrator` is system-wide regardless of any site +grant, matching the Central UI. --- diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Auth/SiteScopeService.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Auth/SiteScopeService.cs index 3925f4f2..c99c92b4 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Auth/SiteScopeService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Auth/SiteScopeService.cs @@ -6,14 +6,26 @@ namespace ZB.MOM.WW.ScadaBridge.CentralUI.Auth; /// /// Resolves the set of sites the current user is permitted to operate on, from -/// the SiteId claims attached at login. +/// the roles and SiteId claims attached at login. /// /// The design (Component-CentralUI, CLAUDE.md "Security & Auth") makes the /// Deployment role site-scoped: a Deployment user mapped through an LDAP group /// with site-scope rules carries one /// claim per permitted site (the claim value is the integer Site.Id). -/// A Deployment user with no SiteId claim — and any Admin/Design user — is -/// system-wide. +/// A user with no SiteId claim is system-wide, and so is any user holding +/// the role — regardless of the SiteId +/// claims they carry. +/// +/// +/// The Administrator bypass is not a convenience, it is the agreed semantic on +/// the other side of the same rule. ManagementActor has always exempted +/// Administrator from site scoping (EnforceSiteScope and every handler that +/// calls it), so a user holding Administrator ALONGSIDE a scoped Deployer grant +/// could act fleet-wide through the CLI/HTTP surface while this service hid the +/// same sites in the UI — the UI was the side that disagreed with the documented +/// rule (deferred-work register row 86). Note that Designer is NOT exempt: the +/// enforced rule has only ever bypassed Administrator, and this doc used to claim +/// "any Admin/Design user" is system-wide, which was never true of either side. /// /// /// Deployment and Monitoring pages must filter every site/instance list through @@ -35,8 +47,9 @@ public sealed class SiteScopeService } /// - /// True when the user is not restricted to a site subset (no SiteId - /// claims). System-wide users see and act on every site. + /// True when the user is not restricted to a site subset — either because they + /// hold or because they carry no + /// SiteId claims. System-wide users see and act on every site. /// /// A task that resolves to true if the user has no site-scope restriction. public async Task IsSystemWideAsync() @@ -83,19 +96,36 @@ public sealed class SiteScopeService return cached; var state = await _authStateProvider.GetAuthenticationStateAsync(); - var siteClaims = state.User.FindAll(JwtTokenService.SiteIdClaimType); + + // Administrator => system-wide, whatever SiteId claims are present. Read + // the role CLAIM rather than IsInRole: the principal's role claim type is + // only bound to JwtTokenService.RoleClaimType when the identity was built + // by SessionClaimBuilder, and the comparison is OrdinalIgnoreCase to match + // ManagementActor's — IsInRole compares claim VALUES ordinally, which would + // make the two sides disagree on a differently-cased role name. + var isAdministrator = state.User + .FindAll(JwtTokenService.RoleClaimType) + .Any(c => string.Equals(c.Value, Roles.Administrator, StringComparison.OrdinalIgnoreCase)); var ids = new HashSet(); - foreach (var claim in siteClaims) + if (!isAdministrator) { - if (int.TryParse(claim.Value, out var id)) - ids.Add(id); + // An Administrator's SiteId claims are not merely overridden, they are + // not collected at all: the invariant "system-wide => empty set" then + // holds for every caller, so a consumer that reads the id set without + // first consulting IsSystemWide cannot silently narrow an Administrator + // to a subset of sites. + foreach (var claim in state.User.FindAll(JwtTokenService.SiteIdClaimType)) + { + if (int.TryParse(claim.Value, out var id)) + ids.Add(id); + } } // No SiteId claims => system-wide. Absence of scope rules means an // unrestricted deployer (this service is the sole // site-scoping mechanism — there is no separate handler to mirror). - var result = (IsSystemWide: ids.Count == 0, Sites: (IReadOnlySet)ids); + var result = (IsSystemWide: isAdministrator || ids.Count == 0, Sites: (IReadOnlySet)ids); _cached = result; return result; } diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Repositories/IDeploymentManagerRepository.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Repositories/IDeploymentManagerRepository.cs index a2d528d0..b59dd411 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Repositories/IDeploymentManagerRepository.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Repositories/IDeploymentManagerRepository.cs @@ -16,8 +16,21 @@ public interface IDeploymentManagerRepository /// The deployment record, or null if not found. Task GetDeploymentRecordByIdAsync(int id, CancellationToken cancellationToken = default); /// - /// Database-side paged, filtered and instance-joined deployment listing — - /// the read path behind the Central UI deployment-status page. + /// Database-side paged, filtered and instance-joined deployment listing — THE + /// paged read of DeploymentRecords, shared by the Central UI + /// deployment-status page and the CLI/HTTP QueryDeployments command. + /// + /// + /// It was two reads until deferred-work register row 37. The other one + /// (QueryDeploymentSummariesAsync) differed in three ways that were all + /// defects rather than requirements: it took a raw skip instead of a page + /// number, returned no total (so a truncated page was indistinguishable from a + /// complete one), skipped the Instance join, and scoped by a caller-resolved + /// INSTANCE-ID LIST — which its one caller built by loading every instance in the + /// system on every scoped query. Site scoping is expressed here as + /// and resolved by + /// the join, so no caller needs that read. + /// /// /// /// Replaces a GetAllDeploymentRecords whole-table read. That method's only @@ -50,7 +63,7 @@ public interface IDeploymentManagerRepository /// /// Status / search / site-scope / instance constraints; all applied DB-side. /// 1-based page number; values below 1 are floored at 1. - /// Rows per page; clamped to [1, DeploymentRecordSummary.MaxPageSize]. + /// Rows per page; clamped to [1, DeploymentListPage.MaxPageSize]. There is no "unlimited" value. /// A cancellation token that can be used to cancel the operation. /// The requested page of rows plus the total count of the filtered set. Task QueryDeploymentListPageAsync( @@ -80,31 +93,6 @@ public interface IDeploymentManagerRepository Types.Deployment.DeploymentListFilter filter, CancellationToken cancellationToken = default); /// - /// Database-side paged and filtered deployment query returning row summaries. - /// Backs the QueryDeployments management command, which previously - /// loaded the ENTIRE deployment-record table (insert-only, one row per deploy - /// attempt for the life of the system) and ignored the page arguments the CLI - /// was already sending. - /// - /// Restrict to one instance's history, or for all. - /// Restrict to one deployment status, or for all. - /// - /// When non-null, restrict to these instance ids. Used to apply a site-scoped - /// user's permitted-site filter IN THE DATABASE rather than by loading every - /// record and every instance and filtering in memory. - /// - /// Rows to skip; negative values are floored at 0. - /// Page size; non-positive or null returns every remaining row. - /// A cancellation token that can be used to cancel the operation. - /// One page of deployment summaries, newest first. - Task> QueryDeploymentSummariesAsync( - int? instanceId, - DeploymentStatus? status, - IReadOnlyCollection? instanceIdScope, - int skip, - int? take, - CancellationToken cancellationToken = default); - /// /// Deletes TERMINAL deployment records ( / /// ) that completed before /// , in one bounded batch. diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/DeploymentCommands.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/DeploymentCommands.cs index e193e292..0f8ce2a6 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/DeploymentCommands.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/DeploymentCommands.cs @@ -23,4 +23,36 @@ public record MgmtDeployArtifactsCommand(int? SiteId = null); /// Database id of the target site. Required — this command is never fleet-wide. public record MgmtDeploySiteCommand(int SiteId); public record QueryDeploymentsCommand(int? InstanceId = null, string? Status = null, int Page = 1, int PageSize = 50); + +/// +/// Result wrapper for . +/// +/// +/// The handler used to return a bare row array. It cannot any more: the page size +/// is now BOUNDED (a non-positive PageSize no longer means "unlimited" — +/// deferred-work register row 87), and a bounded page without a total is a silent +/// truncation. is what lets a caller tell "that is all of +/// them" from "there is more, ask for page 2". Mirrors +/// AuditLogQueryResult / SecuredWriteListResult, the other paged +/// management query results. +/// +/// +/// +/// and echo the values the server +/// actually applied, which are not necessarily the ones requested: both are +/// floored/clamped/defaulted server-side, so the echo is the only way a caller +/// learns the effective slice. Additive contract; new fields go on the end with a +/// default. +/// +/// +/// The requested page of deployment rows, newest first. +/// Total rows matching the filter across all pages. +/// The 1-based page number actually served. +/// The page size actually applied. +public record DeploymentQueryResult( + IReadOnlyList Deployments, + int TotalCount, + int Page, + int PageSize); + public record GetDeploymentDiffCommand(int InstanceId); diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Deployment/DeploymentListPage.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Deployment/DeploymentListPage.cs index 81e23f85..0f169440 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Deployment/DeploymentListPage.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Deployment/DeploymentListPage.cs @@ -19,6 +19,24 @@ public sealed record DeploymentListPage( IReadOnlyList Rows, int TotalCount) { + /// + /// Hard ceiling applied to a requested page size by every caller of the paged + /// deployment read. Lives on the page contract because the page is the single + /// shape that read now has: the older DeploymentRecordSummary projection + /// (which owned this constant, and whose query accepted an unlimited + /// take) was folded into this one by deferred-work register row 37. + /// + public const int MaxPageSize = 1000; + + /// + /// Page size used when a caller supplies none (or a non-positive one). Matches + /// the CLI's own --page-size default, so deploy status with no + /// paging flags behaves identically whether the value travels on the wire or + /// is defaulted here (register row 87 — a non-positive size used to mean + /// "unlimited" and materialized the whole retention window). + /// + public const int DefaultPageSize = 50; + /// An empty page — no rows, no matches. public static DeploymentListPage Empty { get; } = new(Array.Empty(), 0); diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Deployment/DeploymentListRow.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Deployment/DeploymentListRow.cs index d78c1a65..f92e5b6d 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Deployment/DeploymentListRow.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Deployment/DeploymentListRow.cs @@ -3,8 +3,11 @@ using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; namespace ZB.MOM.WW.ScadaBridge.Commons.Types.Deployment; /// -/// One rendered row of the Central UI deployment-status list: a -/// DeploymentRecord projection already joined to its Instance. +/// One row of a deployment listing: a DeploymentRecord projection already +/// joined to its Instance. THE row projection — the Central UI +/// deployment-status page and the CLI/HTTP QueryDeployments command share +/// it, having previously had a projection each (deferred-work register row 37; +/// the CLI-side DeploymentRecordSummary was the one that went). /// /// /// The join is what removes the page's SECOND full-table read. Rendering a row @@ -14,7 +17,7 @@ namespace ZB.MOM.WW.ScadaBridge.Commons.Types.Deployment; /// /// /// -/// Like , the RowVersion +/// The RowVersion /// optimistic-concurrency token is deliberately absent — this is a READ projection /// and must never be mistaken for something that can be written back. Mutating /// paths keep loading the tracked entity. diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Deployment/DeploymentRecordSummary.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Deployment/DeploymentRecordSummary.cs deleted file mode 100644 index 804ba256..00000000 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Deployment/DeploymentRecordSummary.cs +++ /dev/null @@ -1,54 +0,0 @@ -using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; - -namespace ZB.MOM.WW.ScadaBridge.Commons.Types.Deployment; - -/// -/// Row-shaped projection of a DeploymentRecord for list surfaces (CLI -/// deploy list / instance history, the Central UI deployment-status -/// page). -/// -/// -/// Exists so a deployment listing can be paged and filtered in the DATABASE. -/// Deployment records are insert-only — one row per deploy attempt, retained for -/// the configured retention window — so the previous "load the whole table, then -/// filter in memory" read path degraded with the age of the system rather than -/// with the size of the requested page. -/// -/// -/// -/// The RowVersion optimistic-concurrency token is deliberately absent: this -/// is a READ projection, and a summary must never be mistaken for something that -/// can be written back. Mutating paths keep loading the tracked entity. -/// -/// -/// -/// Message-contract evolution rule: additive-only. New fields go on the end with a -/// default. -/// -/// -/// Deployment record row id. -/// The logical deployment id (GUID, "N" format). -/// Instance the deployment targeted. -/// Terminal or in-flight deployment status. -/// Revision hash of the deployed configuration. -/// User who initiated the deployment. -/// When the deployment was initiated. -/// When the deployment reached a terminal status, if it has. -/// Failure detail when the deployment did not succeed. -public record DeploymentRecordSummary( - int Id, - string DeploymentId, - int InstanceId, - DeploymentStatus Status, - string? RevisionHash, - string DeployedBy, - DateTimeOffset DeployedAt, - DateTimeOffset? CompletedAt, - string? ErrorMessage) -{ - /// - /// Hard ceiling applied to a requested page size, mirroring the in-memory - /// Page clamp the management actor already applies elsewhere. - /// - public const int MaxPageSize = 1000; -} diff --git a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Configurations/DeploymentConfiguration.cs b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Configurations/DeploymentConfiguration.cs index 69b437ba..28099c2c 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Configurations/DeploymentConfiguration.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Configurations/DeploymentConfiguration.cs @@ -41,8 +41,9 @@ public class DeploymentRecordConfiguration : IEntityTypeConfiguration d.InstanceId); // Deployment-list sort key. Every list read orders DeployedAt DESC, Id DESC - // (DeploymentManagerRepository.QueryDeploymentListPageAsync and - // QueryDeploymentSummariesAsync) — Id is the deterministic tie-break, because + // (DeploymentManagerRepository.QueryDeploymentListPageAsync, which register + // row 37 made the single paged read behind BOTH the Central UI page and the + // CLI/HTTP QueryDeployments command) — Id is the deterministic tie-break, because // a bulk site deploy stamps many rows in the same DeployedAt instant and a // non-deterministic order would let a row repeat or vanish across pages. // Declaring both key columns descending lets a forward scan feed the paged diff --git a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/DeploymentManagerRepository.cs b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/DeploymentManagerRepository.cs index a938a1cd..9c978c17 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/DeploymentManagerRepository.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/DeploymentManagerRepository.cs @@ -115,7 +115,7 @@ public class DeploymentManagerRepository : IDeploymentManagerRepository CancellationToken cancellationToken = default) { var page = Math.Max(1, pageNumber); - var size = Math.Clamp(pageSize, 1, DeploymentRecordSummary.MaxPageSize); + var size = Math.Clamp(pageSize, 1, DeploymentListPage.MaxPageSize); var query = BuildListQuery(filter, applyStatus: true); @@ -396,58 +396,6 @@ public class DeploymentManagerRepository : IDeploymentManagerRepository return expired.Count; } - /// - public async Task> QueryDeploymentSummariesAsync( - int? instanceId, - DeploymentStatus? status, - IReadOnlyCollection? instanceIdScope, - int skip, - int? take, - CancellationToken cancellationToken = default) - { - var query = _dbContext.DeploymentRecords.AsNoTracking(); - - if (instanceId.HasValue) - query = query.Where(d => d.InstanceId == instanceId.Value); - - if (status.HasValue) - query = query.Where(d => d.Status == status.Value); - - if (instanceIdScope != null) - { - // Site scoping applied DB-side. An empty scope is a real, meaningful - // filter — a user permitted no in-scope instances sees nothing — so it - // must not be short-circuited into "no filter". - var scope = instanceIdScope as int[] ?? instanceIdScope.ToArray(); - query = query.Where(d => scope.Contains(d.InstanceId)); - } - - // ThenByDescending(Id) for the same reason GetCurrentDeploymentStatusAsync - // does it: DeployedAt ties on rapid redeploys, and an unstable sort key - // makes Skip/Take paging non-deterministic (rows repeat or vanish between - // pages). - var paged = query - .OrderByDescending(d => d.DeployedAt) - .ThenByDescending(d => d.Id) - .Skip(Math.Max(0, skip)); - - if (take is > 0) - paged = paged.Take(Math.Min(take.Value, DeploymentRecordSummary.MaxPageSize)); - - return await paged - .Select(d => new DeploymentRecordSummary( - d.Id, - d.DeploymentId, - d.InstanceId, - d.Status, - d.RevisionHash, - d.DeployedBy, - d.DeployedAt, - d.CompletedAt, - d.ErrorMessage)) - .ToListAsync(cancellationToken); - } - /// public async Task PurgeTerminalDeploymentRecordsAsync( DateTimeOffset cutoffUtc, diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index 572567c8..d69ff60a 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -25,6 +25,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Transport; using ZB.MOM.WW.ScadaBridge.Commons.Messages.Management; using ZB.MOM.WW.ScadaBridge.Commons.Messages.RemoteQuery; using ZB.MOM.WW.ScadaBridge.Commons.Types.Audit; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Deployment; using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; using ZB.MOM.WW.ScadaBridge.Commons.Types.InboundApi; using ZB.MOM.WW.ScadaBridge.Commons.Types.Transport; @@ -2558,53 +2559,112 @@ public class ManagementActor : ReceiveActor if (sp.GetService() is { } deploymentService) await deploymentService.TryPurgeTerminalDeploymentRecordsAsync(); - // Page and Status are honoured DB-side. Both arrived on the command - // contract (and the CLI has been sending --page / --page-size / --status - // all along) but the handler previously ignored all three and returned the - // whole table. + // Page, PageSize and Status are all honoured DB-side, through the SAME + // paged read the Central UI deployment page uses (register row 37 — there + // used to be a second, subtly different repository query behind this + // handler alone). var status = ParseDeploymentStatus(cmd.Status); - var skip = Math.Max(0, (Math.Max(1, cmd.Page) - 1) * Math.Max(1, cmd.PageSize)); - var take = cmd.PageSize > 0 ? cmd.PageSize : (int?)null; + var page = Math.Max(1, cmd.Page); + var pageSize = ResolveDeploymentPageSize(cmd.PageSize); // Instance-scoped query: enforce against the target instance's site // before reading its deployment history. if (cmd.InstanceId.HasValue) { await EnforceSiteScopeForInstance(sp, user, cmd.InstanceId.Value); - return await repo.QueryDeploymentSummariesAsync( - cmd.InstanceId.Value, status, instanceIdScope: null, skip, take); + return await QueryDeploymentPage( + repo, new DeploymentListFilter(status, InstanceId: cmd.InstanceId.Value), page, pageSize); } // Unfiltered query: a site-scoped Deployment user must only see records // for instances at sites within their scope. DeploymentRecord has no - // SiteId, so the scope is resolved to the set of in-scope instance ids and - // pushed into the query as an id filter — rather than loading every record - // AND every instance and intersecting them in memory, which made an - // unfiltered listing cost two full-table loads regardless of page size. - if (user.PermittedSiteIds.Length == 0 || user.Roles.Contains(Roles.Administrator, StringComparer.OrdinalIgnoreCase)) - return await repo.QueryDeploymentSummariesAsync(null, status, instanceIdScope: null, skip, take); - - var permittedIds = new HashSet(user.PermittedSiteIds); - var templateRepo = sp.GetRequiredService(); - - var allInstances = await templateRepo.GetAllInstancesAsync(); - var scopedInstanceIds = allInstances - .Where(i => permittedIds.Contains(i.SiteId.ToString())) - .Select(i => i.Id) - .ToArray(); - - return await repo.QueryDeploymentSummariesAsync(null, status, scopedInstanceIds, skip, take); + // SiteId of its own, so the scope rides as a SITE-ID filter resolved by the + // query's own join to Instance. It used to be resolved here instead, by + // loading EVERY instance in the system on every scoped query to turn the + // site grant into an instance-id list (register row 87). + var siteScope = ResolveSiteIdScope(user); + return await QueryDeploymentPage( + repo, new DeploymentListFilter(status, SiteIdScope: siteScope), page, pageSize); } /// - /// Parses the wire Status filter into a . - /// An unparseable value is treated as "no filter" rather than an error, which - /// preserves the previous handler's behaviour of ignoring the field entirely. + /// Runs the shared paged deployment read and wraps it in the command's result + /// envelope, echoing the page coordinates that were actually applied. /// - private static DeploymentStatus? ParseDeploymentStatus(string? status) => - !string.IsNullOrWhiteSpace(status) && Enum.TryParse(status, ignoreCase: true, out var parsed) - ? parsed - : null; + private static async Task QueryDeploymentPage( + IDeploymentManagerRepository repo, DeploymentListFilter filter, int page, int pageSize) + { + var result = await repo.QueryDeploymentListPageAsync(filter, page, pageSize); + return new DeploymentQueryResult(result.Rows, result.TotalCount, page, pageSize); + } + + /// + /// Resolves the caller's site-scope grant into the query's + /// : for a + /// system-wide caller, otherwise the permitted site ids. + /// + /// + /// Administrator is system-wide here, matching + /// and the rest of this actor. A permitted-site id that is not an integer is + /// DROPPED rather than ignored: dropping can only narrow the grant, whereas + /// treating the whole grant as unparseable would widen it to fleet-wide. A + /// grant of nothing but garbage therefore yields an EMPTY scope, which the + /// filter treats as a real "sees nothing" constraint. + /// + private static IReadOnlyCollection? ResolveSiteIdScope(AuthenticatedUser user) + { + if (user.PermittedSiteIds.Length == 0) return null; + if (user.Roles.Contains(Roles.Administrator, StringComparer.OrdinalIgnoreCase)) return null; + + var ids = new List(user.PermittedSiteIds.Length); + foreach (var raw in user.PermittedSiteIds) + { + if (int.TryParse(raw, out var id)) + ids.Add(id); + } + return ids; + } + + /// + /// Bounds the requested deployment page size: a non-positive value means + /// "unspecified" and takes the default, and every value is capped at + /// . + /// + /// + /// Register row 87. A non-positive size used to mean UNLIMITED all the way down + /// to the repository, so --page-size 0 materialized the entire + /// terminal-record retention window into one response. The result envelope + /// carries the total count precisely so bounding the page does not hide rows. + /// + private static int ResolveDeploymentPageSize(int requested) => + requested <= 0 + ? DeploymentListPage.DefaultPageSize + : Math.Min(requested, DeploymentListPage.MaxPageSize); + + /// + /// Parses the wire Status filter into a . + /// Absent/blank means "no filter"; an unrecognised value is an ERROR naming the + /// valid ones. + /// + /// + /// Register row 88. An unparseable value used to fall back to "no filter", so + /// --status Sucess silently returned every status — the caller saw a + /// plausible, complete-looking result that did not answer the question they + /// asked. Same loud-failure principle as row 46. + /// + /// The value is not a deployment status. + private static DeploymentStatus? ParseDeploymentStatus(string? status) + { + if (string.IsNullOrWhiteSpace(status)) return null; + if (Enum.TryParse(status, ignoreCase: true, out var parsed) + && Enum.IsDefined(parsed)) + { + return parsed; + } + + throw new ManagementCommandException( + $"Unknown deployment status '{status}'. Valid values: {string.Join(", ", Enum.GetNames())}."); + } // ======================================================================== // Audit Log handler diff --git a/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/DeploymentStatusTableProjectionTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/DeploymentStatusTableProjectionTests.cs new file mode 100644 index 00000000..ed2ee8a9 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/DeploymentStatusTableProjectionTests.cs @@ -0,0 +1,89 @@ +using System.Text.Json; +using ZB.MOM.WW.ScadaBridge.CLI.Commands; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.Management; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Deployment; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; + +namespace ZB.MOM.WW.ScadaBridge.CLI.Tests; + +/// +/// Tests for the deploy status table projection (deferred-work register rows +/// 37/87). The handler now returns a OBJECT where +/// it used to return a bare row array; without a projector the generic renderer takes +/// its Property/Value branch and prints the whole deployments array as raw JSON +/// inside one cell. The projection unwraps the envelope and keeps the total visible as +/// a trailing note — which matters more here than for the audit query, because the +/// page size is bounded server-side and a full page is no longer proof there is +/// nothing more. +/// +public class DeploymentStatusTableProjectionTests +{ + /// + /// The real server shape, serialised from the REAL + /// / types rather than hand-written JSON, so renaming + /// one of their properties fails this test instead of silently blanking a column. + /// + private static string RealPayload(int rowCount, int totalCount) => + JsonSerializer.Serialize( + new DeploymentQueryResult( + Enumerable.Range(1, rowCount).Select(i => new DeploymentListRow( + i, $"dep-{i:D3}", i, $"Line3.Pump{i}", 2, DeploymentStatus.Success, + $"rev-{i}", "alice", + new DateTimeOffset(2026, 8, 15, 12, 0, 0, TimeSpan.Zero), null, null)).ToList(), + totalCount, + Page: 1, + PageSize: rowCount), + new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }); + + [Fact] + public void Projection_UnwrapsTheEnvelopeToARowArray() + { + var projected = DeploymentStatusTableProjection.ProjectRows(RealPayload(2, 2)); + + using var doc = JsonDocument.Parse(projected); + Assert.Equal(JsonValueKind.Array, doc.RootElement.ValueKind); + Assert.Equal(2, doc.RootElement.GetArrayLength()); + Assert.Equal("dep-001", doc.RootElement[0].GetProperty("deploymentId").GetString()); + Assert.Equal("Line3.Pump1", doc.RootElement[0].GetProperty("instanceUniqueName").GetString()); + } + + [Fact] + public void Projection_WhenPageIsAStrictSubset_AppendsATotalNote() + { + var projected = DeploymentStatusTableProjection.ProjectRows(RealPayload(2, 57)); + + using var doc = JsonDocument.Parse(projected); + Assert.Equal(3, doc.RootElement.GetArrayLength()); + var note = doc.RootElement[2].GetProperty("note").GetString(); + Assert.Contains("showing 2 of 57", note); + Assert.Contains("--page-size", note); + } + + [Fact] + public void Projection_WhenPageIsTheWholeResult_EmitsNoNoteColumn() + { + var projected = DeploymentStatusTableProjection.ProjectRows(RealPayload(2, 2)); + + using var doc = JsonDocument.Parse(projected); + Assert.All(doc.RootElement.EnumerateArray(), + e => Assert.False(e.TryGetProperty("note", out _))); + } + + [Fact] + public void Projection_AcceptsABareRowArray_FromAPreEnvelopeServer() + { + var projected = DeploymentStatusTableProjection.ProjectRows("""[{"deploymentId":"dep-001"}]"""); + + using var doc = JsonDocument.Parse(projected); + Assert.Equal("dep-001", Assert.Single(doc.RootElement.EnumerateArray()) + .GetProperty("deploymentId").GetString()); + } + + [Theory] + [InlineData("not json at all")] + [InlineData("""{"error":"boom"}""")] + public void Projection_LeavesUnrecognisedBodiesUntouched(string body) + { + Assert.Equal(body, DeploymentStatusTableProjection.ProjectRows(body)); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Auth/SiteScopeServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Auth/SiteScopeServiceTests.cs index eab55fb4..e78fa568 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Auth/SiteScopeServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Auth/SiteScopeServiceTests.cs @@ -90,4 +90,75 @@ public class SiteScopeServiceTests Assert.True(await svc.IsSiteAllowedAsync(1)); Assert.True(await svc.IsSiteAllowedAsync(999)); } + + // ── Administrator is system-wide regardless of SiteId claims (register 86) ── + // + // ManagementActor has always exempted Administrator from site scoping + // (EnforceSiteScope and every handler that calls it). This service was + // claims-only, so an Administrator who ALSO carried a scoped Deployer grant + // could act fleet-wide through the CLI/HTTP surface while the UI hid the same + // sites. The UI was the side that disagreed with the documented rule. + + [Fact] + public async Task AdministratorWithSiteClaims_IsSystemWide() + { + var svc = ForUser(Role("Administrator"), Role("Deployer"), SiteClaim(1), SiteClaim(3)); + + Assert.True(await svc.IsSystemWideAsync()); + } + + [Fact] + public async Task AdministratorWithSiteClaims_FilterSites_ReturnsAllSites() + { + var svc = ForUser(Role("Administrator"), Role("Deployer"), SiteClaim(1)); + + var filtered = await svc.FilterSitesAsync(Sites(1, 2, 3)); + + Assert.Equal(new[] { 1, 2, 3 }, filtered.Select(s => s.Id)); + } + + [Fact] + public async Task AdministratorWithSiteClaims_IsSiteAllowed_ForSitesOutsideTheClaims() + { + var svc = ForUser(Role("Administrator"), SiteClaim(1)); + + Assert.True(await svc.IsSiteAllowedAsync(2)); + } + + [Fact] + public async Task AdministratorWithSiteClaims_ReportsNoPermittedIds() + { + // The invariant "system-wide => empty id set" must hold for every caller, so + // a consumer that reads the ids without consulting IsSystemWide cannot + // silently narrow an Administrator to a subset of sites. + var svc = ForUser(Role("Administrator"), SiteClaim(1), SiteClaim(3)); + + Assert.Empty(await svc.PermittedSiteIdsAsync()); + } + + [Theory] + [InlineData("administrator")] + [InlineData("ADMINISTRATOR")] + public async Task AdministratorRoleMatch_IsCaseInsensitive(string role) + { + // ManagementActor compares roles with OrdinalIgnoreCase. ClaimsPrincipal + // .IsInRole compares claim VALUES ordinally, so matching the actor requires + // reading the role claims explicitly — otherwise the two sides would + // disagree on a differently-cased role name. + var svc = ForUser(Role(role), SiteClaim(1)); + + Assert.True(await svc.IsSystemWideAsync()); + } + + [Fact] + public async Task DesignerWithSiteClaims_IsStillScoped() + { + // The class doc used to claim "any Admin/Design user" is system-wide. Only + // Administrator is — on either side of the rule — and the doc was corrected + // rather than the bypass widened. + var svc = ForUser(Role("Designer"), SiteClaim(1)); + + Assert.False(await svc.IsSystemWideAsync()); + Assert.False(await svc.IsSiteAllowedAsync(2)); + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/DeploymentListQueryRepositoryTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/DeploymentListQueryRepositoryTests.cs index 90554c0e..e9cccd12 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/DeploymentListQueryRepositoryTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/DeploymentListQueryRepositoryTests.cs @@ -164,7 +164,7 @@ public class DeploymentListQueryRepositoryTests : IDisposable new DeploymentListFilter(), pageNumber: 1, pageSize: int.MaxValue); Assert.Single(page.Rows); - Assert.True(DeploymentRecordSummary.MaxPageSize > 0); + Assert.True(DeploymentListPage.MaxPageSize > 0); } [Fact] @@ -300,6 +300,37 @@ public class DeploymentListQueryRepositoryTests : IDisposable Assert.Empty(page.Rows); } + [Fact] + public async Task QueryPage_SiteScopeAndStatus_Compose_ForTheCliQueryPath() + { + // Register row 37: this is now the ONLY paged read of DeploymentRecords — + // the CLI/HTTP QueryDeployments command reaches it through the same filter + // the UI page uses, where it used to have a second repository method with + // instance-id-list scoping and no total. Its combination (site scope AND a + // status filter, over more rows than one page holds) is pinned here. + var s1 = await SeedSiteAsync("S1"); + var s2 = await SeedSiteAsync("S2"); + var a = await SeedInstanceAsync("Inst-A", s1); + var b = await SeedInstanceAsync("Inst-B", s2); + for (var i = 0; i < 4; i++) + { + await SeedRecordAsync($"dep-a{i}", a, DeploymentStatus.Failed, _base.AddMinutes(i)); + await SeedRecordAsync($"dep-b{i}", b, DeploymentStatus.Failed, _base.AddMinutes(i)); + } + await SeedRecordAsync("dep-a-ok", a, DeploymentStatus.Success, _base.AddHours(1)); + + var page = await _repository.QueryDeploymentListPageAsync( + new DeploymentListFilter(Status: DeploymentStatus.Failed, SiteIdScope: new[] { s1 }), + pageNumber: 1, pageSize: 2); + + // Four in-scope failures exist; the page is bounded to two and the total + // reports the rest, so a bounded page is never mistaken for a complete one. + Assert.Equal(4, page.TotalCount); + Assert.Equal(2, page.Rows.Count); + Assert.All(page.Rows, r => Assert.Equal(s1, r.SiteId)); + Assert.All(page.Rows, r => Assert.Equal(DeploymentStatus.Failed, r.Status)); + } + // ── Status counts ───────────────────────────────────────────────────────── [Fact] diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs index 0dc3d93e..72ea7d4a 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs @@ -1,3 +1,4 @@ +using System.Text.Json; using Akka.Actor; using Akka.TestKit.Xunit2; using Microsoft.Extensions.DependencyInjection; @@ -1222,51 +1223,63 @@ public class ManagementActorTests : TestKit, IDisposable new("deploy-" + instanceId, "operator") { Id = instanceId, InstanceId = instanceId }; /// - /// Row-summary equivalent of . QueryDeployments - /// now returns DB-side projections instead of whole tracked entities. + /// Row-projection equivalent of , on the site + /// given. QueryDeployments returns DB-side projections joined to their instance, + /// not whole tracked entities — and the join is what carries the + /// SiteId the scope filter is evaluated against. /// - private static DeploymentRecordSummary DeploymentSummaryFor(int instanceId) => - new(instanceId, "deploy-" + instanceId, instanceId, DeploymentStatus.Success, - null, "operator", DateTimeOffset.UtcNow, DateTimeOffset.UtcNow, null); + private static DeploymentListRow DeploymentRowFor(int instanceId, int siteId) => + new(instanceId, "deploy-" + instanceId, instanceId, "Pump" + instanceId, siteId, + DeploymentStatus.Success, null, "operator", DateTimeOffset.UtcNow, DateTimeOffset.UtcNow, null); /// - /// Stubs QueryDeploymentSummariesAsync so it applies the same - /// instance / status / scope / paging predicates the database would. + /// Stubs the single paged deployment read so it applies the same + /// instance / status / site-scope / paging predicates the database would. /// /// - /// The filtering moved OUT of the handler and INTO the query, so a stub that - /// ignored the arguments would make these tests pass no matter what the handler - /// passed down. Honouring them here is what keeps the scope-enforcement - /// assertions meaningful. + /// The filtering lives entirely in the query now (register row 37 folded the + /// handler's own second repository method away), so a stub that ignored the + /// filter would make these tests pass no matter what the handler passed down. + /// Honouring it here is what keeps the scope-enforcement assertions meaningful. /// /// - private static void StubDeploymentSummaryQuery( + /// The substituted repository to stub. + /// The seed rows, as (instance id, site id) pairs. + private static void StubDeploymentPageQuery( IDeploymentManagerRepository repo, - IEnumerable instanceIds) + params (int InstanceId, int SiteId)[] rows) { - var rows = instanceIds.Select(DeploymentSummaryFor).ToList(); - repo.QueryDeploymentSummariesAsync( - Arg.Any(), Arg.Any(), Arg.Any?>(), - Arg.Any(), Arg.Any(), Arg.Any()) + var all = rows.Select(r => DeploymentRowFor(r.InstanceId, r.SiteId)).ToList(); + repo.QueryDeploymentListPageAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) .Returns(ci => { - var instanceId = ci.ArgAt(0); - var scope = ci.ArgAt?>(2); - var skip = ci.ArgAt(3); - var take = ci.ArgAt(4); + var filter = ci.ArgAt(0); + var page = Math.Max(1, ci.ArgAt(1)); + var size = Math.Max(1, ci.ArgAt(2)); - IEnumerable q = rows; - if (instanceId.HasValue) - q = q.Where(r => r.InstanceId == instanceId.Value); - if (scope != null) - q = q.Where(r => scope.Contains(r.InstanceId)); - q = q.Skip(Math.Max(0, skip)); - if (take is > 0) - q = q.Take(take.Value); - return (IReadOnlyList)q.ToList(); + IEnumerable q = all; + if (filter.InstanceId.HasValue) + q = q.Where(r => r.InstanceId == filter.InstanceId.Value); + if (filter.Status.HasValue) + q = q.Where(r => r.Status == filter.Status.Value); + if (filter.SiteIdScope != null) + q = q.Where(r => filter.SiteIdScope.Contains(r.SiteId)); + + var matched = q.ToList(); + var slice = matched.Skip((page - 1) * size).Take(size).ToList(); + return new DeploymentListPage(slice, matched.Count); }); } + /// Captures the filter the handler pushed into the paged read. + private static DeploymentListFilter CapturedDeploymentFilter(IDeploymentManagerRepository repo) + { + var call = repo.ReceivedCalls() + .Single(c => c.GetMethodInfo().Name == nameof(IDeploymentManagerRepository.QueryDeploymentListPageAsync)); + return (DeploymentListFilter)call.GetArguments()[0]!; + } + /// /// Stubs GetTemplateSummariesAsync over synthetic /// templates, applying the requested Skip/Take exactly as the database would. @@ -1305,7 +1318,7 @@ public class ManagementActorTests : TestKit, IDisposable public void QueryDeployments_UnfilteredWithDeploymentRole_ReturnsAllRecords() { var deployRepo = Substitute.For(); - StubDeploymentSummaryQuery(deployRepo, [1, 2]); + StubDeploymentPageQuery(deployRepo, (1, 1), (2, 2)); _services.AddScoped(_ => deployRepo); var actor = CreateActor(); @@ -1322,7 +1335,7 @@ public class ManagementActorTests : TestKit, IDisposable public void QueryDeployments_FilteredByInstanceId_ReturnsInstanceRecords() { var deployRepo = Substitute.For(); - StubDeploymentSummaryQuery(deployRepo, [5]); + StubDeploymentPageQuery(deployRepo, (5, 1)); _services.AddScoped(_ => deployRepo); var actor = CreateActor(); @@ -1351,8 +1364,7 @@ public class ManagementActorTests : TestKit, IDisposable var response = ExpectMsg(TimeSpan.FromSeconds(5)); Assert.Equal(envelope.CorrelationId, response.CorrelationId); // The out-of-scope instance's deployment history must not be queried. - deployRepo.DidNotReceiveWithAnyArgs().QueryDeploymentSummariesAsync( - default, default, default, default, default); + deployRepo.DidNotReceiveWithAnyArgs().QueryDeploymentListPageAsync(default!, default, default); } [Fact] @@ -1361,7 +1373,7 @@ public class ManagementActorTests : TestKit, IDisposable _templateRepo.GetInstanceByIdAsync(5, Arg.Any()) .Returns(new Instance("Pump5") { Id = 5, SiteId = 1 }); var deployRepo = Substitute.For(); - StubDeploymentSummaryQuery(deployRepo, [5]); + StubDeploymentPageQuery(deployRepo, (5, 1)); _services.AddScoped(_ => deployRepo); var actor = CreateActor(); @@ -1376,17 +1388,8 @@ public class ManagementActorTests : TestKit, IDisposable [Fact] public void QueryDeployments_UnfilteredForSiteScopedUser_DropsOutOfScopeRecords() { - // ManagementService-023: the unfiltered branch now bulk-loads instances - // once via GetAllInstancesAsync (instead of N+1 GetInstanceByIdAsync per - // distinct InstanceId). Mock the bulk path accordingly. - _templateRepo.GetAllInstancesAsync(Arg.Any()) - .Returns(new List - { - new("Pump1") { Id = 1, SiteId = 1 }, - new("Pump2") { Id = 2, SiteId = 2 }, - }); var deployRepo = Substitute.For(); - StubDeploymentSummaryQuery(deployRepo, [1, 2]); + StubDeploymentPageQuery(deployRepo, (1, 1), (2, 2)); _services.AddScoped(_ => deployRepo); var actor = CreateActor(); @@ -1397,28 +1400,19 @@ public class ManagementActorTests : TestKit, IDisposable var response = ExpectMsg(TimeSpan.FromSeconds(5)); Assert.Contains("deploy-1", response.JsonData); Assert.DoesNotContain("deploy-2", response.JsonData); - // The per-instance lookup must NOT have been used for the unfiltered - // branch — that was the N+1 the bulk load replaced. - _templateRepo.DidNotReceiveWithAnyArgs().GetInstanceByIdAsync(default); + Assert.Equal(new[] { 1 }, CapturedDeploymentFilter(deployRepo).SiteIdScope); } [Fact] - public void QueryDeployments_UnfilteredForSiteScopedUser_UsesBulkInstanceLoad_NotPerRecordLookup() + public void QueryDeployments_UnfilteredForSiteScopedUser_ReadsNoInstancesAtAll() { - // ManagementService-023 regression pin: the unfiltered branch must issue - // GetAllInstancesAsync ONCE and never call GetInstanceByIdAsync, regardless - // of how many DeploymentRecords reference distinct InstanceIds. Before the - // fix, three distinct instance ids would have produced three per-instance - // lookups (textbook N+1). - _templateRepo.GetAllInstancesAsync(Arg.Any()) - .Returns(new List - { - new("Pump1") { Id = 1, SiteId = 1 }, - new("Pump2") { Id = 2, SiteId = 2 }, - new("Pump3") { Id = 3, SiteId = 1 }, - }); + // Register row 87. The scoped branch used to resolve the site grant into an + // instance-id LIST, which meant loading EVERY instance in the system on + // every scoped query (a bulk load that had itself replaced an N+1 of + // per-record lookups). The grant now rides as a site-id filter resolved by + // the query's own join, so neither instance read may happen. var deployRepo = Substitute.For(); - StubDeploymentSummaryQuery(deployRepo, [1, 2, 3]); + StubDeploymentPageQuery(deployRepo, (1, 1), (2, 2), (3, 1)); _services.AddScoped(_ => deployRepo); var actor = CreateActor(); @@ -1427,17 +1421,41 @@ public class ManagementActorTests : TestKit, IDisposable actor.Tell(envelope); ExpectMsg(TimeSpan.FromSeconds(5)); - _templateRepo.Received(1).GetAllInstancesAsync(Arg.Any()); + _templateRepo.DidNotReceiveWithAnyArgs().GetAllInstancesAsync(); _templateRepo.DidNotReceiveWithAnyArgs().GetInstanceByIdAsync(default); } + [Fact] + public void QueryDeployments_SiteScopedUserWithUnparseableGrant_SeesNothing() + { + // A permitted-site id that is not an integer is dropped, never ignored: + // ignoring it would silently widen the grant to fleet-wide. A grant of + // nothing but garbage therefore yields an EMPTY scope — a real "sees + // nothing" filter, not "no filter". + var deployRepo = Substitute.For(); + StubDeploymentPageQuery(deployRepo, (1, 1), (2, 2)); + _services.AddScoped(_ => deployRepo); + + var actor = CreateActor(); + var envelope = ScopedEnvelope(new QueryDeploymentsCommand(), new[] { "not-an-id" }, "Deployer"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.DoesNotContain("deploy-1", response.JsonData); + Assert.DoesNotContain("deploy-2", response.JsonData); + Assert.Empty(CapturedDeploymentFilter(deployRepo).SiteIdScope!); + } + [Fact] public void QueryDeployments_UnfilteredForAdminUser_ReturnsAllRecords() { // Admin role bypasses site scoping even with PermittedSiteIds set. // (The user also holds Deployment so it passes the role gate.) + // The UI's SiteScopeService now agrees — see SiteScopeServiceTests + // (register row 86). var deployRepo = Substitute.For(); - StubDeploymentSummaryQuery(deployRepo, [1, 2]); + StubDeploymentPageQuery(deployRepo, (1, 1), (2, 2)); _services.AddScoped(_ => deployRepo); var actor = CreateActor(); @@ -1448,6 +1466,119 @@ public class ManagementActorTests : TestKit, IDisposable var response = ExpectMsg(TimeSpan.FromSeconds(5)); Assert.Contains("deploy-1", response.JsonData); Assert.Contains("deploy-2", response.JsonData); + Assert.Null(CapturedDeploymentFilter(deployRepo).SiteIdScope); + } + + // ── Loud status parsing + bounded page size (register rows 87 / 88) ──────── + + [Fact] + public void QueryDeployments_UnknownStatus_FailsLoudlyNamingTheValidValues() + { + // Register row 88: `--status Sucess` used to fall back to "no filter" and + // return EVERY status — a plausible, complete-looking answer to a question + // the caller did not ask. + var deployRepo = Substitute.For(); + StubDeploymentPageQuery(deployRepo, (1, 1)); + _services.AddScoped(_ => deployRepo); + + var actor = CreateActor(); + actor.Tell(Envelope(new QueryDeploymentsCommand(Status: "Sucess"), "Deployer")); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Contains("Sucess", response.Error); + Assert.Contains("Success", response.Error); + Assert.Contains("Failed", response.Error); + deployRepo.DidNotReceiveWithAnyArgs().QueryDeploymentListPageAsync(default!, default, default); + } + + [Theory] + [InlineData("success")] + [InlineData("SUCCESS")] + public void QueryDeployments_KnownStatus_IsCaseInsensitive(string status) + { + var deployRepo = Substitute.For(); + StubDeploymentPageQuery(deployRepo, (1, 1)); + _services.AddScoped(_ => deployRepo); + + var actor = CreateActor(); + actor.Tell(Envelope(new QueryDeploymentsCommand(Status: status), "Deployer")); + + ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Equal(DeploymentStatus.Success, CapturedDeploymentFilter(deployRepo).Status); + } + + [Fact] + public void QueryDeployments_NumericStatus_IsRejected() + { + // Enum.TryParse happily accepts "7" and yields an undefined enum value, so + // the parse needs the IsDefined guard as well as the TryParse. + var deployRepo = Substitute.For(); + StubDeploymentPageQuery(deployRepo, (1, 1)); + _services.AddScoped(_ => deployRepo); + + var actor = CreateActor(); + actor.Tell(Envelope(new QueryDeploymentsCommand(Status: "7"), "Deployer")); + + ExpectMsg(TimeSpan.FromSeconds(5)); + } + + [Theory] + [InlineData(0)] + [InlineData(-1)] + public void QueryDeployments_NonPositivePageSize_TakesTheDefault_NotUnlimited(int requested) + { + // Register row 87: a non-positive page size used to mean "unlimited" all the + // way down to the repository, materializing the whole retention window. + var deployRepo = Substitute.For(); + StubDeploymentPageQuery(deployRepo, Enumerable.Range(1, 200).Select(i => (i, 1)).ToArray()); + _services.AddScoped(_ => deployRepo); + + var actor = CreateActor(); + actor.Tell(Envelope(new QueryDeploymentsCommand(PageSize: requested), "Deployer")); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + var call = deployRepo.ReceivedCalls() + .Single(c => c.GetMethodInfo().Name == nameof(IDeploymentManagerRepository.QueryDeploymentListPageAsync)); + Assert.Equal(DeploymentListPage.DefaultPageSize, (int)call.GetArguments()[2]!); + + // The total travels with the bounded page, so a caller can tell a truncated + // page from a complete one. + var result = JsonSerializer.Deserialize(response.JsonData); + Assert.Equal(200, result.GetProperty("totalCount").GetInt32()); + Assert.Equal(DeploymentListPage.DefaultPageSize, result.GetProperty("deployments").GetArrayLength()); + Assert.Equal(DeploymentListPage.DefaultPageSize, result.GetProperty("pageSize").GetInt32()); + } + + [Fact] + public void QueryDeployments_OversizedPageSize_IsCappedAtTheMaximum() + { + var deployRepo = Substitute.For(); + StubDeploymentPageQuery(deployRepo, (1, 1)); + _services.AddScoped(_ => deployRepo); + + var actor = CreateActor(); + actor.Tell(Envelope(new QueryDeploymentsCommand(PageSize: int.MaxValue), "Deployer")); + + ExpectMsg(TimeSpan.FromSeconds(5)); + var call = deployRepo.ReceivedCalls() + .Single(c => c.GetMethodInfo().Name == nameof(IDeploymentManagerRepository.QueryDeploymentListPageAsync)); + Assert.Equal(DeploymentListPage.MaxPageSize, (int)call.GetArguments()[2]!); + } + + [Fact] + public void QueryDeployments_PageIsFlooredAtOne() + { + var deployRepo = Substitute.For(); + StubDeploymentPageQuery(deployRepo, (1, 1)); + _services.AddScoped(_ => deployRepo); + + var actor = CreateActor(); + actor.Tell(Envelope(new QueryDeploymentsCommand(Page: 0), "Deployer")); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Contains("deploy-1", response.JsonData); + var result = JsonSerializer.Deserialize(response.JsonData); + Assert.Equal(1, result.GetProperty("page").GetInt32()); } // ========================================================================