From 6c9de9f7abbd7ad71d6adfb4b110ac91a0710072 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 10 Jul 2026 05:03:51 -0400 Subject: [PATCH] feat(management): any-of role gates; restrict ListSecuredWrites to Operator/Verifier/Admin (arch-review UA1) Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj --- docs/requirements/Component-CentralUI.md | 3 +- .../Component-ManagementService.md | 3 +- .../Pages/Operations/SecuredWrites.razor | 5 +- .../ManagementActor.cs | 52 ++++++++++++++----- .../AuthorizationPolicies.cs | 24 +++++++++ .../ManagementActorTests.cs | 34 ++++++++++++ .../SecuredWriteHandlerTests.cs | 8 ++- .../SecurityTests.cs | 22 ++++++++ 8 files changed, 133 insertions(+), 18 deletions(-) diff --git a/docs/requirements/Component-CentralUI.md b/docs/requirements/Component-CentralUI.md index 795f4ed4..f358c286 100644 --- a/docs/requirements/Component-CentralUI.md +++ b/docs/requirements/Component-CentralUI.md @@ -206,8 +206,9 @@ Per-leaf alarm rendering (leaf nodes are individual conditions for native alarms - **Stuck rows are visually badged** — a notification is stuck if it is `Pending` or `Retrying` and older than the configurable stuck-age threshold. Stuck detection is display-only; there is no automated escalation or alerting. - All queries are served from the central `Notifications` table — no remote per-site queries are needed, unlike the Parked Message Management page. -### Secured Writes (Operator / Verifier Roles) +### Secured Writes (Operator / Verifier / Administrator Roles) - A **Secured Writes** page (`/operations/secured-writes`) drives the **two-person** authorization workflow for writes through the MxAccess Gateway: an **Operator** initiates the write, a separate **Verifier** approves it, and only an approved write reaches the site. +- The **page itself is gated by `RequireSecuredWriteAccess`** — satisfied by any of `Operator` / `Verifier` / `Administrator`. Although the pending/history lists are read-only, they expose process-sensitive tag values, so the page is not open to every authenticated user; this aligns with the ManagementActor `ListSecuredWritesCommand` any-of gate (arch-review UA1). The submit and approve/reject sub-actions are further gated by `RequireOperator` / `RequireVerifier` respectively (below). - **Operator (submit)** — a submit form gated by `RequireOperator`: pick the site → an **MxGateway** connection on that site → the tag path → a typed value → an optional comment. Submission inserts a `Pending` `PendingSecuredWrite` row centrally; it does **not** write anything yet. - **Verifier (approve / reject)** — a pending queue gated by `RequireVerifier` with **Approve** / **Reject** (+comment) actions. Approve shows a confirmation of the exact site / connection / tag / value before firing. The verifier's **own submissions are disabled in the UI and rejected server-side** (no self-approval). On approve, central marks the row `Approved` and relays the write to the site MxGateway (records `Executed` / `Failed`); reject moves it to `Rejected` with a reason. - **History** — terminal rows (Executed / Failed / Rejected / Expired) with the full who/when/outcome trail (operator, verifier, comments, timestamps, any execution error). diff --git a/docs/requirements/Component-ManagementService.md b/docs/requirements/Component-ManagementService.md index c92e16a9..c29d2c88 100644 --- a/docs/requirements/Component-ManagementService.md +++ b/docs/requirements/Component-ManagementService.md @@ -147,7 +147,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. -- **List / query secured writes**: pending queue + terminal history, global or per-site. +- **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). ### External Systems @@ -215,6 +215,7 @@ Every incoming message carries the authenticated user's identity and roles. The - **Deployment** role required for: instance management (including instance alarm overrides and native alarm source overrides), deployments, debug view, debug snapshot, parked message queries, site event log queries. Site scoping is enforced for site-scoped Deployment users. - **Operator** role required for: submitting a secured write (`SubmitSecuredWriteCommand`). - **Verifier** role required for: approving / rejecting a secured write (`ApproveSecuredWriteCommand` / `RejectSecuredWriteCommand`). The no-self-approval rule (`Operator ≠ Verifier`) is enforced in the handler, independent of the role check. +- **Any of Operator / Verifier / Administrator** required for: listing / querying secured writes (`ListSecuredWritesCommand`). Read-only, but the history exposes process-sensitive tag values, so it is gated any-of rather than open to every authenticated user (arch-review UA1). Enforced as an any-of gate (`GetRequiredRoles` returns the permitted set; the caller must hold at least one). - **Admin** role additionally required for: server-certificate trust/remove/list (`TrustServerCertCommand` / `RemoveServerCertCommand` / `ListServerCertsCommand`). - **Read-only access** (any authenticated role): health summary, health site, site event log queries, parked message queries. diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Operations/SecuredWrites.razor b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Operations/SecuredWrites.razor index df54c94a..f99a9e58 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Operations/SecuredWrites.razor +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Operations/SecuredWrites.razor @@ -1,5 +1,8 @@ @page "/operations/secured-writes" -@attribute [Authorize] +@* arch-review UA1: the Secured Writes history exposes process-sensitive tag + values, so the page is gated to the Secured Write roles + Administrator + (any-of), aligning with the ManagementActor ListSecuredWrites gate. *@ +@attribute [Authorize(Policy = AuthorizationPolicies.RequireSecuredWriteAccess)] @using ZB.MOM.WW.ScadaBridge.CentralUI.Services @using ZB.MOM.WW.ScadaBridge.Commons.Entities.Sites @using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index 63d59395..c773c540 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -100,12 +100,14 @@ public class ManagementActor : ReceiveActor var correlationId = envelope.CorrelationId; var user = envelope.User; - // Check authorization - var requiredRole = GetRequiredRole(envelope.Command); - if (requiredRole != null && !user.Roles.Contains(requiredRole, StringComparer.OrdinalIgnoreCase)) + // Check authorization. A command may require ANY of a set of roles + // (any-of): the caller is authorized when they hold at least one. + var requiredRoles = GetRequiredRoles(envelope.Command); + if (requiredRoles is not null + && !requiredRoles.Any(r => user.Roles.Contains(r, StringComparer.OrdinalIgnoreCase))) { sender.Tell(new ManagementUnauthorized(correlationId, - $"Role '{requiredRole}' required for {envelope.Command.GetType().Name}")); + $"One of roles [{string.Join(", ", requiredRoles)}] required for {envelope.Command.GetType().Name}")); return; } @@ -157,7 +159,28 @@ public class ManagementActor : ReceiveActor return new ManagementError(correlationId, clientMessage, "COMMAND_FAILED"); } - private static string? GetRequiredRole(object command) => command switch + // Static readonly role sets for the authorization gate. Single-role arms + // stay one-element arrays so every arm has the uniform any-of shape. + private static readonly string[] AdminOnly = [Roles.Administrator]; + private static readonly string[] DesignerOnly = [Roles.Designer]; + private static readonly string[] DeployerOnly = [Roles.Deployer]; + private static readonly string[] OperatorOnly = [Roles.Operator]; + private static readonly string[] VerifierOnly = [Roles.Verifier]; + + // arch-review UA1: the Secured Writes history table exposes + // process-sensitive tag values, so reading it is restricted to the + // two-person Secured Write roles plus Administrator (any-of) — NOT any + // authenticated user like the other list/query commands. + private static readonly string[] SecuredWriteReaders = + [Roles.Operator, Roles.Verifier, Roles.Administrator]; + + /// + /// Returns the set of roles that satisfy the authorization gate for + /// (any-of semantics), or null when the + /// command is available to any authenticated user. Internal so the + /// authorization matrix can be asserted directly in tests. + /// + internal static IReadOnlyList? GetRequiredRoles(object command) => command switch { // Administrator operations CreateSiteCommand or UpdateSiteCommand or DeleteSiteCommand @@ -180,7 +203,7 @@ public class ManagementActor : ReceiveActor // gate must match — otherwise a Designer blocked in the UI could still // rotate a production credential via the CLI/Management API. or UpdateSmtpConfigCommand - or UpdateSmsConfigCommand => Roles.Administrator, + or UpdateSmsConfigCommand => AdminOnly, // Designer operations CreateAreaCommand or DeleteAreaCommand @@ -208,13 +231,13 @@ public class ManagementActor : ReceiveActor or CreateTemplateFolderCommand or RenameTemplateFolderCommand or MoveTemplateFolderCommand or ReorderTemplateFolderCommand or DeleteTemplateFolderCommand or MoveTemplateToFolderCommand - or ExportBundleCommand => Roles.Designer, + or ExportBundleCommand => DesignerOnly, // Transport import operations (mirror the Central UI gating: // Administrator for inbound bundle handling because they mutate // cross-cutting configuration; Export stays Designer because it only // reads). - PreviewBundleCommand or ImportBundleCommand => Roles.Administrator, + PreviewBundleCommand or ImportBundleCommand => AdminOnly, // Deployer operations CreateInstanceCommand or MgmtDeployInstanceCommand or MgmtEnableInstanceCommand @@ -226,16 +249,19 @@ public class ManagementActor : ReceiveActor or MgmtDeployArtifactsCommand or QueryDeploymentsCommand or RetryParkedMessageCommand or DiscardParkedMessageCommand - or DebugSnapshotCommand => Roles.Deployer, + or DebugSnapshotCommand => DeployerOnly, // Two-person secured write. Submit is an Operator action; // approve/reject are Verifier actions. Separation of duties (a write may // not be verified by its submitter) is enforced inside the handler — the // role gate only ensures the caller holds the right coarse role. - SubmitSecuredWriteCommand => Roles.Operator, - RejectSecuredWriteCommand or ApproveSecuredWriteCommand => Roles.Verifier, - // ListSecuredWritesCommand is read-only -> falls through to "any - // authenticated user" below, like the other list/query commands. + SubmitSecuredWriteCommand => OperatorOnly, + RejectSecuredWriteCommand or ApproveSecuredWriteCommand => VerifierOnly, + // ListSecuredWritesCommand is read-only, but the history table exposes + // process-sensitive tag values, so it is NOT open to any authenticated + // user — restricted to the Secured Write roles + Administrator + // (arch-review UA1). + ListSecuredWritesCommand => SecuredWriteReaders, // Read-only queries -- any authenticated user _ => null diff --git a/src/ZB.MOM.WW.ScadaBridge.Security/AuthorizationPolicies.cs b/src/ZB.MOM.WW.ScadaBridge.Security/AuthorizationPolicies.cs index 595ed444..07dad10d 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Security/AuthorizationPolicies.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Security/AuthorizationPolicies.cs @@ -88,6 +88,24 @@ public static class AuthorizationPolicies /// public const string RequireVerifier = "RequireVerifier"; + /// + /// Read access to the Secured Writes surface (the Secured Writes page + + /// its Pending/history list). The history table exposes process-sensitive + /// tag values, so it is NOT open to any authenticated user — satisfied by + /// any of {, , + /// } (arch-review UA1). Aligns with the + /// ManagementActor's ListSecuredWritesCommand any-of gate. + /// + public const string RequireSecuredWriteAccess = "RequireSecuredWriteAccess"; + + /// + /// Roles that satisfy . Held in one + /// place so the policy, the docs, and the ManagementActor gate stay in + /// lockstep: {Operator, Verifier, Administrator}. + /// + public static readonly string[] SecuredWriteAccessRoles = + { Roles.Operator, Roles.Verifier, Roles.Administrator }; + /// /// Read access to the Audit Log surface (Audit Log page, /// Configuration Audit Log page, Audit nav group). Granted to the @@ -155,6 +173,12 @@ public static class AuthorizationPolicies options.AddPolicy(RequireVerifier, policy => policy.RequireClaim(JwtTokenService.RoleClaimType, Roles.Verifier)); + // Any-of read gate for the Secured Writes surface (arch-review UA1): + // succeeds when the principal holds any of Operator/Verifier/ + // Administrator, mirroring the ManagementActor ListSecuredWrites gate. + options.AddPolicy(RequireSecuredWriteAccess, policy => + policy.RequireClaim(JwtTokenService.RoleClaimType, SecuredWriteAccessRoles)); + // Multi-role permission policies — the policy succeeds when the // principal holds ANY of the mapped roles. RequireClaim with // multiple allowed values is the right primitive: it checks diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs index 4c6cd189..a19c52ae 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs @@ -6,6 +6,7 @@ using NSubstitute; using NSubstitute.ExceptionExtensions; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Instances; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Schemas; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.SecuredWrites; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Scripts; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Templates; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; @@ -86,6 +87,39 @@ public class ManagementActorTests : TestKit, IDisposable Assert.Contains("Administrator", response.Message); } + // ======================================================================== + // ListSecuredWrites role gate (arch-review UA1): read-only, but the + // history table exposes process-sensitive tag values, so it is restricted + // to the two-person Secured Write roles + Administrator (any-of). + // ======================================================================== + + [Fact] + public void ListSecuredWrites_ViewerOnly_Unauthorized() + { + var actor = CreateActor(); + actor.Tell(Envelope(new ListSecuredWritesCommand(null, null), "Viewer")); + ExpectMsg(TimeSpan.FromSeconds(5)); + } + + [Theory] + [InlineData("Operator")] + [InlineData("Verifier")] + [InlineData("Administrator")] + public void ListSecuredWrites_SecuredWriteRoles_Allowed(string role) + { + var securedWriteRepo = Substitute.For(); + securedWriteRepo.QueryAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any()) + .Returns(Task.FromResult>( + Array.Empty())); + _services.AddScoped(_ => securedWriteRepo); + + var actor = CreateActor(); + actor.Tell(Envelope(new ListSecuredWritesCommand(null, null), role)); + ExpectMsg(TimeSpan.FromSeconds(5)); + } + [Fact] public void DeploymentCommand_WithDesignRole_ReturnsUnauthorized() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs index 08da5859..48fad4f8 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs @@ -352,7 +352,10 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable }); var actor = CreateActor(); - var envelope = Envelope(new ListSecuredWritesCommand("Pending", "SITE1"), "carol", "Viewer"); + // arch-review UA1: ListSecuredWrites is gated to the Secured Write + // roles + Administrator, so use Operator here (this test asserts filter + // forwarding, not the role gate — see ListSecuredWrites_* auth tests). + var envelope = Envelope(new ListSecuredWritesCommand("Pending", "SITE1"), "carol", "Operator"); actor.Tell(envelope); @@ -370,7 +373,8 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable .Returns(new List()); var actor = CreateActor(); - var envelope = Envelope(new ListSecuredWritesCommand(null, null), "carol"); + // arch-review UA1: gated read — Operator satisfies the any-of gate. + var envelope = Envelope(new ListSecuredWritesCommand(null, null), "carol", "Operator"); actor.Tell(envelope); diff --git a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/SecurityTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/SecurityTests.cs index 79c3367b..9bc9498f 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/SecurityTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/SecurityTests.cs @@ -1242,6 +1242,28 @@ public class AuthorizationPolicyTests Assert.False(await EvaluatePolicy(AuthorizationPolicies.RequireOperator, verifierOnly)); } + [Theory] + [InlineData("Operator")] + [InlineData("Verifier")] + [InlineData("Administrator")] + public async Task RequireSecuredWriteAccess_SecuredWriteRoles_Satisfy(string role) + { + // arch-review UA1: the Secured Writes history exposes process-sensitive + // tag values, so read access is any-of {Operator, Verifier, Administrator}. + var principal = CreatePrincipal(new[] { role }); + Assert.True(await EvaluatePolicy(AuthorizationPolicies.RequireSecuredWriteAccess, principal)); + } + + [Theory] + [InlineData("Viewer")] + [InlineData("Designer")] + [InlineData("Deployer")] + public async Task RequireSecuredWriteAccess_OtherRoles_Denied(string role) + { + var principal = CreatePrincipal(new[] { role }); + Assert.False(await EvaluatePolicy(AuthorizationPolicies.RequireSecuredWriteAccess, principal)); + } + private static ClaimsPrincipal CreatePrincipal(string[] roles, string[]? siteIds = null) { var claims = new List();