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()); } // ========================================================================