feat(management): secured-write list paging with TotalCount (arch-review P3)
Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
This commit is contained in:
@@ -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 <status>] [--site <identifier>] [--skip <n>] [--take <n>]
|
||||
```
|
||||
|
||||
`--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
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
@@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// Builds the <c>secured-write</c> command group. Currently exposes the read-only
|
||||
/// <c>list</c> sub-command, which relays a <see cref="ListSecuredWritesCommand"/> to the
|
||||
/// central <c>ManagementActor</c> (gated any-of <c>Operator</c> / <c>Verifier</c> /
|
||||
/// <c>Administrator</c>). The list is offset-paged via <c>--skip</c> / <c>--take</c>
|
||||
/// (arch-review P3); omitting them preserves the historical unpaged default (first 200 rows).
|
||||
/// </summary>
|
||||
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<string?> StatusOption =
|
||||
new("--status") { Description = "Filter by status (e.g. Pending, Approved, Rejected, Expired, Executed, Failed); omit to match all" };
|
||||
private static readonly Option<string?> SiteOption =
|
||||
new("--site") { Description = "Filter by site identifier; omit to match all sites" };
|
||||
private static readonly Option<int> SkipOption =
|
||||
new("--skip") { Description = "Number of rows to skip (offset paging)", DefaultValueFactory = _ => 0 };
|
||||
private static readonly Option<int> TakeOption =
|
||||
new("--take") { Description = "Maximum rows to return (clamped to 1..500 server-side)", DefaultValueFactory = _ => 200 };
|
||||
|
||||
/// <summary>Builds the <c>secured-write</c> command group with its <c>list</c> sub-command.</summary>
|
||||
/// <param name="urlOption">Global <c>--url</c> option for the management API endpoint.</param>
|
||||
/// <param name="formatOption">Global <c>--format</c> option for output format.</param>
|
||||
/// <param name="usernameOption">Global <c>--username</c> option for authentication.</param>
|
||||
/// <param name="passwordOption">Global <c>--password</c> option for authentication.</param>
|
||||
/// <returns>The configured <c>secured-write</c> command.</returns>
|
||||
public static Command Build(Option<string> urlOption, Option<string> formatOption, Option<string> usernameOption, Option<string> 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<string> urlOption, Option<string> formatOption, Option<string> usernameOption, Option<string> 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;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Builds a <see cref="ListSecuredWritesCommand"/> from a parsed <c>secured-write list</c>
|
||||
/// invocation. Exposed internally so command-construction tests can assert field mapping
|
||||
/// (including the <c>--skip</c> / <c>--take</c> paging options) without triggering the HTTP call.
|
||||
/// </summary>
|
||||
/// <param name="result">The parsed command-line result from the <c>secured-write list</c> invocation.</param>
|
||||
/// <returns>A <see cref="ListSecuredWritesCommand"/> populated from the parsed result.</returns>
|
||||
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);
|
||||
}
|
||||
}
|
||||
@@ -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(_ =>
|
||||
{
|
||||
|
||||
@@ -51,12 +51,16 @@ public record ApproveSecuredWriteCommand(long Id, string? Comment);
|
||||
public record RejectSecuredWriteCommand(long Id, string? Comment);
|
||||
|
||||
/// <summary>
|
||||
/// Read-only query for secured writes, optionally filtered by status and site.
|
||||
/// A <c>null</c> filter matches every row.
|
||||
/// Read-only query for secured writes, optionally filtered by status and site,
|
||||
/// with offset paging. A <c>null</c> filter matches every row. The <paramref name="Skip"/>
|
||||
/// / <paramref name="Take"/> defaults preserve the historical unpaged wire behaviour
|
||||
/// (first 200 rows) for callers that omit them (additive evolution).
|
||||
/// </summary>
|
||||
/// <param name="Status">Status filter; <c>null</c> matches every status.</param>
|
||||
/// <param name="SiteId">Site id filter; <c>null</c> matches every site.</param>
|
||||
public record ListSecuredWritesCommand(string? Status, string? SiteId);
|
||||
/// <param name="Skip">Number of rows to skip (offset paging); clamped to >= 0 server-side.</param>
|
||||
/// <param name="Take">Maximum rows to return; clamped to 1..500 server-side. Default 200.</param>
|
||||
public record ListSecuredWritesCommand(string? Status, string? SiteId, int Skip = 0, int Take = 200);
|
||||
|
||||
/// <summary>
|
||||
/// Result projection of a single pending secured write row, mirroring the
|
||||
@@ -79,5 +83,11 @@ public record SecuredWriteDto(
|
||||
DateTime? ExecutedAtUtc,
|
||||
string? ExecutionError);
|
||||
|
||||
/// <summary>Result wrapper for <see cref="ListSecuredWritesCommand"/>.</summary>
|
||||
public record SecuredWriteListResult(IReadOnlyList<SecuredWriteDto> Items);
|
||||
/// <summary>
|
||||
/// Result wrapper for <see cref="ListSecuredWritesCommand"/>. <paramref name="TotalCount"/>
|
||||
/// is the count of rows matching the query filters ignoring paging, so the UI can render
|
||||
/// a pager independent of the returned page size.
|
||||
/// </summary>
|
||||
/// <param name="Items">The page of matching secured-write rows.</param>
|
||||
/// <param name="TotalCount">Total number of rows matching the filters (unpaged).</param>
|
||||
public record SecuredWriteListResult(IReadOnlyList<SecuredWriteDto> Items, int TotalCount);
|
||||
|
||||
@@ -1394,7 +1394,13 @@ public class ManagementActor : ReceiveActor
|
||||
IServiceProvider sp, ListSecuredWritesCommand cmd)
|
||||
{
|
||||
var repo = sp.GetRequiredService<ISecuredWriteRepository>();
|
||||
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);
|
||||
}
|
||||
|
||||
// ========================================================================
|
||||
|
||||
@@ -0,0 +1,62 @@
|
||||
using System.CommandLine;
|
||||
using ZB.MOM.WW.ScadaBridge.CLI.Commands;
|
||||
|
||||
namespace ZB.MOM.WW.ScadaBridge.CLI.Tests.Commands;
|
||||
|
||||
/// <summary>
|
||||
/// PLAN-07 Task 17 (arch-review P3): <c>secured-write list</c> must expose <c>--skip</c>
|
||||
/// / <c>--take</c> offset-paging options alongside the <c>--status</c> / <c>--site</c>
|
||||
/// filters, and map them onto <see cref="Commons.Messages.Management.ListSecuredWritesCommand"/>.
|
||||
/// Omitting the paging flags preserves the historical unpaged default (skip 0, take 200).
|
||||
/// </summary>
|
||||
public class SecuredWriteListCommandTests
|
||||
{
|
||||
private static readonly Option<string> Url = new("--url") { Recursive = true };
|
||||
private static readonly Option<string> Format = new("--format") { Recursive = true };
|
||||
private static readonly Option<string> Username = new("--username") { Recursive = true };
|
||||
private static readonly Option<string> 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);
|
||||
}
|
||||
}
|
||||
@@ -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<CancellationToken>())
|
||||
.Returns(new List<PendingSecuredWrite>());
|
||||
_securedWriteRepo.CountAsync(null, null, Arg.Any<CancellationToken>()).Returns(87);
|
||||
|
||||
var actor = CreateActor();
|
||||
actor.Tell(Envelope(new ListSecuredWritesCommand(null, null, Skip: 10, Take: 25), "carol", "Operator"));
|
||||
|
||||
var resp = ExpectMsg<ManagementSuccess>(TimeSpan.FromSeconds(5));
|
||||
_securedWriteRepo.Received(1).QueryAsync(null, null, 10, 25, Arg.Any<CancellationToken>());
|
||||
_securedWriteRepo.Received(1).CountAsync(null, null, Arg.Any<CancellationToken>());
|
||||
Assert.Contains("\"totalCount\":87", resp.JsonData);
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------------
|
||||
// Opportunistic expiry sweep on list — Task 16 (arch-review S2)
|
||||
// ------------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user