diff --git a/docs/requirements/Component-CLI.md b/docs/requirements/Component-CLI.md index a7adfd64..9ea5d34f 100644 --- a/docs/requirements/Component-CLI.md +++ b/docs/requirements/Component-CLI.md @@ -354,6 +354,23 @@ The `--tracked-operation-id` value corresponds to the `MessageId` field of the p `SiteCall` row (visible on the Central UI Site Calls page and in the `SiteCalls` audit table). +### Secured-Write Commands (two-person MxGateway writes) + +Query two-person (MxGateway) secured writes — the pending queue plus terminal history. +Read-only; gated any-of **Operator** / **Verifier** / **Administrator** (the history +exposes process-sensitive tag values, so it is not open to every authenticated user). + +``` +scadabridge secured-write list [--status ] [--site ] [--skip ] [--take ] +``` + +`--status` / `--site` filter the results (omit to match all). `--skip` / `--take` provide +offset paging (arch-review P3); omitting them uses the historical unpaged default +(`--skip 0 --take 200`). `--take` is clamped to `1..500` server-side. The result carries a +`totalCount` (rows matching the filters, ignoring paging) alongside the returned page. +Overdue `Pending` rows are swept to `Expired` opportunistically each time the list is +queried (server-side TTL, arch-review S2). + The `--format json|table` option is recursive and accepted on every command above. ## Configuration diff --git a/docs/requirements/Component-ManagementService.md b/docs/requirements/Component-ManagementService.md index 90806428..188b9596 100644 --- a/docs/requirements/Component-ManagementService.md +++ b/docs/requirements/Component-ManagementService.md @@ -149,7 +149,7 @@ The two-person authorization workflow for writes through the MxAccess Gateway. B - **SubmitSecuredWriteCommand** (`Operator` role): validates the target connection exists and uses the **MxGateway** protocol (rejected otherwise), then inserts a `Pending` row capturing site / connection / tag / typed value / operator + comment. Nothing is written yet. - **ApproveSecuredWriteCommand** (`Verifier` role): enforces **no self-approval** (`VerifierUser ≠ OperatorUser`, checked **before** any state change) and a **compare-and-swap** race guard (`TryMarkApprovedAsync` — exactly one concurrent approver flips `Pending → Approved`; the loser is rejected). On winning, it validates the value type, decodes the value (guarded — a bad value fails the row deterministically rather than leaving it stuck `Approved`), and relays a `WriteTagRequest` to the site MxGateway via the Communication Layer, recording the outcome as `Executed` or `Failed`. - **RejectSecuredWriteCommand** (`Verifier` role): marks a `Pending` row `Rejected` with the verifier's reason. -- **ListSecuredWritesCommand** (any of `Operator` / `Verifier` / `Administrator`): pending queue + terminal history, global or per-site. Although read-only, the history table exposes process-sensitive tag values, so it is **not** open to any authenticated user — it is gated to the two Secured Write roles plus `Administrator` (any-of), aligning with the `RequireSecuredWriteAccess` Central UI policy (arch-review UA1). +- **ListSecuredWritesCommand** (any of `Operator` / `Verifier` / `Administrator`): pending queue + terminal history, global or per-site, **offset-paged** via `Skip` / `Take` (arch-review P3; additive — the defaults `Skip=0` / `Take=200` preserve the historical unpaged wire behaviour). The handler clamps `Take` to `1..500` and `Skip` to `>= 0`, calls `QueryAsync(Status, SiteId, skip, take)` for the page and `CountAsync(Status, SiteId)` for the unpaged total, and returns a `SecuredWriteListResult { Items, TotalCount }` so a client can render a pager. Although read-only, the history table exposes process-sensitive tag values, so it is **not** open to any authenticated user — it is gated to the two Secured Write roles plus `Administrator` (any-of), aligning with the `RequireSecuredWriteAccess` Central UI policy (arch-review UA1). **Server-side pending TTL (arch-review S2).** A `Pending` secured write is not valid indefinitely. Both `ApproveSecuredWriteCommand` and `RejectSecuredWriteCommand` call a shared `TryExpireIfStaleAsync` helper immediately after the `Pending`-status check: if the row's `SubmittedAtUtc` is older than the configured `SecuredWritePendingTtl` (`ManagementServiceOptions`, default **24 h**), the helper atomically transitions it `Pending → Expired` via `ISecuredWriteRepository.TryMarkExpiredAsync` (compare-and-swap — no verifier recorded, `VerifierUser` stays `NULL`), emits a best-effort `SecuredWriteExpire` audit row (system actor, `Discarded` status — the closest terminal `AuditStatus`), and the command fails with an "expired" error. A stale setpoint therefore can **never** be approved or executed days later, and no `WriteTagRequest` is relayed. A non-positive TTL disables expiry. `ListSecuredWritesCommand` additionally runs an **opportunistic sweep**: on every read it walks the returned page and calls the same helper for each `Pending` row, so overdue writes surface as `Expired` the next time anyone views the queue — no dedicated expiry-timer singleton is needed because the ManagementActor already runs on every central node and the CAS makes concurrent multi-node sweeps idempotent. diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/SecuredWriteCommands.cs b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/SecuredWriteCommands.cs new file mode 100644 index 00000000..3a65e059 --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/SecuredWriteCommands.cs @@ -0,0 +1,71 @@ +using System.CommandLine; +using System.CommandLine.Parsing; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.Management; + +namespace ZB.MOM.WW.ScadaBridge.CLI.Commands; + +/// +/// Builds the secured-write command group. Currently exposes the read-only +/// list sub-command, which relays a to the +/// central ManagementActor (gated any-of Operator / Verifier / +/// Administrator). The list is offset-paged via --skip / --take +/// (arch-review P3); omitting them preserves the historical unpaged default (first 200 rows). +/// +public static class SecuredWriteCommands +{ + // Options are static so the parsed values can be read back from both the SetAction + // and the internal BuildListCommand helper (used by command-construction tests). + private static readonly Option StatusOption = + new("--status") { Description = "Filter by status (e.g. Pending, Approved, Rejected, Expired, Executed, Failed); omit to match all" }; + private static readonly Option SiteOption = + new("--site") { Description = "Filter by site identifier; omit to match all sites" }; + private static readonly Option SkipOption = + new("--skip") { Description = "Number of rows to skip (offset paging)", DefaultValueFactory = _ => 0 }; + private static readonly Option TakeOption = + new("--take") { Description = "Maximum rows to return (clamped to 1..500 server-side)", DefaultValueFactory = _ => 200 }; + + /// Builds the secured-write command group with its list sub-command. + /// Global --url option for the management API endpoint. + /// Global --format option for output format. + /// Global --username option for authentication. + /// Global --password option for authentication. + /// The configured secured-write command. + public static Command Build(Option urlOption, Option formatOption, Option usernameOption, Option passwordOption) + { + var command = new Command("secured-write") { Description = "Query two-person (MxGateway) secured writes" }; + command.Add(BuildList(urlOption, formatOption, usernameOption, passwordOption)); + return command; + } + + private static Command BuildList(Option urlOption, Option formatOption, Option usernameOption, Option passwordOption) + { + var cmd = new Command("list") { Description = "List secured writes (pending queue + terminal history), optionally filtered and paged" }; + cmd.Add(StatusOption); + cmd.Add(SiteOption); + cmd.Add(SkipOption); + cmd.Add(TakeOption); + cmd.SetAction(async (ParseResult result) => + { + return await CommandHelpers.ExecuteCommandAsync( + result, urlOption, formatOption, usernameOption, passwordOption, + BuildListCommand(result)); + }); + return cmd; + } + + /// + /// Builds a from a parsed secured-write list + /// invocation. Exposed internally so command-construction tests can assert field mapping + /// (including the --skip / --take paging options) without triggering the HTTP call. + /// + /// The parsed command-line result from the secured-write list invocation. + /// A populated from the parsed result. + internal static ListSecuredWritesCommand BuildListCommand(ParseResult result) + { + var status = result.GetValue(StatusOption); + var site = result.GetValue(SiteOption); + var skip = result.GetValue(SkipOption); + var take = result.GetValue(TakeOption); + return new ListSecuredWritesCommand(status, site, skip, take); + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/Program.cs b/src/ZB.MOM.WW.ScadaBridge.CLI/Program.cs index 78af1213..fc49cac4 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/Program.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/Program.cs @@ -35,6 +35,7 @@ rootCommand.Add(DbConnectionCommands.Build(urlOption, formatOption, usernameOpti rootCommand.Add(ApiMethodCommands.Build(urlOption, formatOption, usernameOption, passwordOption)); rootCommand.Add(BundleCommands.Build(urlOption, formatOption, usernameOption, passwordOption)); rootCommand.Add(CachedCallCommands.Build(urlOption, formatOption, usernameOption, passwordOption)); +rootCommand.Add(SecuredWriteCommands.Build(urlOption, formatOption, usernameOption, passwordOption)); rootCommand.SetAction(_ => { diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/SecuredWriteCommands.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/SecuredWriteCommands.cs index 3ab790ba..721ea231 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/SecuredWriteCommands.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/SecuredWriteCommands.cs @@ -51,12 +51,16 @@ public record ApproveSecuredWriteCommand(long Id, string? Comment); public record RejectSecuredWriteCommand(long Id, string? Comment); /// -/// Read-only query for secured writes, optionally filtered by status and site. -/// A null filter matches every row. +/// Read-only query for secured writes, optionally filtered by status and site, +/// with offset paging. A null filter matches every row. The +/// / defaults preserve the historical unpaged wire behaviour +/// (first 200 rows) for callers that omit them (additive evolution). /// /// Status filter; null matches every status. /// Site id filter; null matches every site. -public record ListSecuredWritesCommand(string? Status, string? SiteId); +/// Number of rows to skip (offset paging); clamped to >= 0 server-side. +/// Maximum rows to return; clamped to 1..500 server-side. Default 200. +public record ListSecuredWritesCommand(string? Status, string? SiteId, int Skip = 0, int Take = 200); /// /// Result projection of a single pending secured write row, mirroring the @@ -79,5 +83,11 @@ public record SecuredWriteDto( DateTime? ExecutedAtUtc, string? ExecutionError); -/// Result wrapper for . -public record SecuredWriteListResult(IReadOnlyList Items); +/// +/// Result wrapper for . +/// is the count of rows matching the query filters ignoring paging, so the UI can render +/// a pager independent of the returned page size. +/// +/// The page of matching secured-write rows. +/// Total number of rows matching the filters (unpaged). +public record SecuredWriteListResult(IReadOnlyList Items, int TotalCount); diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index 618d7e64..4969ef77 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -1394,7 +1394,13 @@ public class ManagementActor : ReceiveActor IServiceProvider sp, ListSecuredWritesCommand cmd) { var repo = sp.GetRequiredService(); - var rows = await repo.QueryAsync(cmd.Status, cmd.SiteId, skip: 0, take: 200); + + // Offset paging (arch-review P3): clamp untrusted paging inputs so a crafted + // command can neither request a negative offset nor an unbounded page. + var take = Math.Clamp(cmd.Take, 1, 500); + var skip = Math.Max(0, cmd.Skip); + var rows = await repo.QueryAsync(cmd.Status, cmd.SiteId, skip, take); + var totalCount = await repo.CountAsync(cmd.Status, cmd.SiteId); // Opportunistic expiry sweep (arch-review S2): every time the list is read, walk // the page and expire any overdue Pending row. TryExpireIfStaleAsync self-filters @@ -1411,7 +1417,7 @@ public class ManagementActor : ReceiveActor await TryExpireIfStaleAsync(sp, repo, row); } - return new SecuredWriteListResult(rows.Select(ToSecuredWriteDto).ToList()); + return new SecuredWriteListResult(rows.Select(ToSecuredWriteDto).ToList(), totalCount); } // ======================================================================== diff --git a/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/Commands/SecuredWriteListCommandTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/Commands/SecuredWriteListCommandTests.cs new file mode 100644 index 00000000..46997686 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/Commands/SecuredWriteListCommandTests.cs @@ -0,0 +1,62 @@ +using System.CommandLine; +using ZB.MOM.WW.ScadaBridge.CLI.Commands; + +namespace ZB.MOM.WW.ScadaBridge.CLI.Tests.Commands; + +/// +/// PLAN-07 Task 17 (arch-review P3): secured-write list must expose --skip +/// / --take offset-paging options alongside the --status / --site +/// filters, and map them onto . +/// Omitting the paging flags preserves the historical unpaged default (skip 0, take 200). +/// +public class SecuredWriteListCommandTests +{ + private static readonly Option Url = new("--url") { Recursive = true }; + private static readonly Option Format = new("--format") { Recursive = true }; + private static readonly Option Username = new("--username") { Recursive = true }; + private static readonly Option Password = new("--password") { Recursive = true }; + + private static Command Group() => SecuredWriteCommands.Build(Url, Format, Username, Password); + + private static Command List() => Group().Subcommands.Single(c => c.Name == "list"); + + private static Option? FindOption(Command command, string name) + => command.Options.SingleOrDefault(o => o.Name == name); + + [Fact] + public void List_HasSkipAndTakeOptions() + { + var list = List(); + Assert.NotNull(FindOption(list, "--skip")); + Assert.NotNull(FindOption(list, "--take")); + } + + [Fact] + public void List_ParsesSkipTakeAndFilters_ForwardsToCommand() + { + var parse = Group().Parse(new[] + { + "list", "--status", "Pending", "--site", "SITE1", "--skip", "10", "--take", "25", + }); + + Assert.Empty(parse.Errors); + var command = SecuredWriteCommands.BuildListCommand(parse); + Assert.Equal("Pending", command.Status); + Assert.Equal("SITE1", command.SiteId); + Assert.Equal(10, command.Skip); + Assert.Equal(25, command.Take); + } + + [Fact] + public void List_OmittedPaging_UsesUnpagedDefaults() + { + var parse = Group().Parse(new[] { "list" }); + + Assert.Empty(parse.Errors); + var command = SecuredWriteCommands.BuildListCommand(parse); + Assert.Null(command.Status); + Assert.Null(command.SiteId); + Assert.Equal(0, command.Skip); + Assert.Equal(200, command.Take); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs index e9c1d930..4d2199f4 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs @@ -613,6 +613,26 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable Assert.Contains("not found", updated.ExecutionError); } + // ------------------------------------------------------------------------ + // List paging — Task 17 (arch-review P3) + // ------------------------------------------------------------------------ + + [Fact] + public void List_WithSkipTake_ForwardsPagingAndReturnsTotalCount() + { + _securedWriteRepo.QueryAsync(null, null, 10, 25, Arg.Any()) + .Returns(new List()); + _securedWriteRepo.CountAsync(null, null, Arg.Any()).Returns(87); + + var actor = CreateActor(); + actor.Tell(Envelope(new ListSecuredWritesCommand(null, null, Skip: 10, Take: 25), "carol", "Operator")); + + var resp = ExpectMsg(TimeSpan.FromSeconds(5)); + _securedWriteRepo.Received(1).QueryAsync(null, null, 10, 25, Arg.Any()); + _securedWriteRepo.Received(1).CountAsync(null, null, Arg.Any()); + Assert.Contains("\"totalCount\":87", resp.JsonData); + } + // ------------------------------------------------------------------------ // Opportunistic expiry sweep on list — Task 16 (arch-review S2) // ------------------------------------------------------------------------