diff --git a/docs/requirements/Component-Security.md b/docs/requirements/Component-Security.md index 56ad975f..6a59f0fe 100644 --- a/docs/requirements/Component-Security.md +++ b/docs/requirements/Component-Security.md @@ -138,7 +138,7 @@ Set in a local or docker-dev environment via the environment variable `ScadaBrid - Approve or reject a pending secured write from the Secured Writes page — the *approving* half of the two-person write workflow. - **Purpose**: The approving counterpart to **Operator**. Separation of duties is enforced **server-side**: the ManagementActor rejects any approval where the approving user equals the submitting Operator (no self-approval), so the two roles must be held by distinct principals for a write to execute. (See Component-ManagementService.md and Component-CentralUI.md.) -> **Two-person secured-write workflow.** `Operator` and `Verifier` are deliberately separate global roles so a single principal cannot both initiate and approve a write through the MxAccess Gateway. Both are coarse global roles like the others; any site scoping is layered on at the LDAP-mapping level. +> **Two-person secured-write workflow.** `Operator` and `Verifier` are deliberately separate global roles so a single principal cannot both initiate and approve a write through the MxAccess Gateway. Both are coarse global roles like the others. **Site scoping from the LDAP mapping is enforced server-side on every secured-write command** (arch-review R2 N3): submit checks the target site, approve/reject check the row's site (before the TTL/self-approval/CAS chain), and the list constrains a scoped caller to their permitted sites — mirroring the deployment commands' `EnforceSiteScope` rule. Administrators and system-wide principals (empty permitted-site set) are unrestricted. > > **Deployment-configuration hazard — never grant one principal both roles.** The whole control collapses if the same identity (or LDAP group) maps to *both* `Operator` and `Verifier`: server-side no-self-approval blocks approving the *exact* write you submitted, but a dual-role principal can still trivially pair-approve with a second submission, so a single compromised or careless account regains unilateral write. Keep `SCADA-Operators` and `SCADA-Verifiers` group membership disjoint. The dev `DisableLogin` caveat is the extreme of this: with `DisableLogin` on, the auto-login principal holds **all** roles, so the two-person flow cannot be exercised end-to-end by a single identity — which is exactly why `DisableLogin=true` is refused outside Development (see the `DisableLoginGuard` note above). No-self-approval is covered by handler tests; real two-person use requires two real, distinct-role identities. > diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index 3cc76e66..1fe68472 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -467,7 +467,7 @@ public class ManagementActor : ReceiveActor SubmitSecuredWriteCommand cmd => await HandleSubmitSecuredWrite(sp, cmd, user), ApproveSecuredWriteCommand cmd => await HandleApproveSecuredWrite(sp, cmd, user), RejectSecuredWriteCommand cmd => await HandleRejectSecuredWrite(sp, cmd, user), - ListSecuredWritesCommand cmd => await HandleListSecuredWrites(sp, cmd), + ListSecuredWritesCommand cmd => await HandleListSecuredWrites(sp, cmd, user), // Transport bundle operations ExportBundleCommand cmd => await HandleExportBundle(sp, cmd, user.Username), @@ -1493,16 +1493,36 @@ public class ManagementActor : ReceiveActor } private static async Task HandleListSecuredWrites( - IServiceProvider sp, ListSecuredWritesCommand cmd) + IServiceProvider sp, ListSecuredWritesCommand cmd, AuthenticatedUser user) { + // Site scoping (arch-review R2 N3): an explicit SiteId filter is scope-checked + // like every other identifier-keyed command; an UNFILTERED list from a + // site-scoped non-admin caller is constrained to their permitted sites at the + // query level (rows AND totalCount both scoped — see ISecuredWriteRepository). + IReadOnlyCollection? permittedIdentifiers = null; + if (cmd.SiteId is not null) + { + await EnforceSiteScopeForIdentifier(sp, user, cmd.SiteId); + } + else if (user.PermittedSiteIds.Length > 0 + && !user.Roles.Contains(Roles.Administrator, StringComparer.OrdinalIgnoreCase)) + { + var siteRepo = sp.GetRequiredService(); + var permitted = new HashSet(user.PermittedSiteIds); + permittedIdentifiers = (await siteRepo.GetAllSitesAsync()) + .Where(s => permitted.Contains(s.Id.ToString())) + .Select(s => s.SiteIdentifier) + .ToList(); + } + var repo = sp.GetRequiredService(); // 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); + var rows = await repo.QueryAsync(cmd.Status, cmd.SiteId, skip, take, permittedIdentifiers); + var totalCount = await repo.CountAsync(cmd.Status, cmd.SiteId, permittedIdentifiers); // Opportunistic expiry sweep (arch-review S2): every time the list is read, walk // the page and expire any overdue Pending row. TryExpireIfStaleAsync self-filters diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs index b52d42e5..9a336316 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs @@ -145,7 +145,7 @@ public class ManagementActorTests : TestKit, IDisposable var securedWriteRepo = Substitute.For(); securedWriteRepo.QueryAsync( Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any()) + Arg.Any?>(), Arg.Any()) .Returns(Task.FromResult>( Array.Empty())); _services.AddScoped(_ => securedWriteRepo); diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs index 7c8a6450..05c6dc22 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs @@ -349,7 +349,7 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable [Fact] public void List_FiltersByStatusAndSite() { - _securedWriteRepo.QueryAsync("Pending", "SITE1", 0, 200, Arg.Any()) + _securedWriteRepo.QueryAsync("Pending", "SITE1", 0, 200, Arg.Any?>(), Arg.Any()) .Returns(new List { new() @@ -372,13 +372,13 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable Assert.Equal(envelope.CorrelationId, response.CorrelationId); Assert.Contains("\"id\":3", response.JsonData); Assert.Contains("alice", response.JsonData); - _securedWriteRepo.Received(1).QueryAsync("Pending", "SITE1", 0, 200, Arg.Any()); + _securedWriteRepo.Received(1).QueryAsync("Pending", "SITE1", 0, 200, Arg.Any?>(), Arg.Any()); } [Fact] public void List_NoFilters_PassesNullsToRepository() { - _securedWriteRepo.QueryAsync(null, null, 0, 200, Arg.Any()) + _securedWriteRepo.QueryAsync(null, null, 0, 200, Arg.Any?>(), Arg.Any()) .Returns(new List()); var actor = CreateActor(); @@ -388,7 +388,49 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable actor.Tell(envelope); ExpectMsg(TimeSpan.FromSeconds(5)); - _securedWriteRepo.Received(1).QueryAsync(null, null, 0, 200, Arg.Any()); + _securedWriteRepo.Received(1).QueryAsync(null, null, 0, 200, Arg.Any?>(), Arg.Any()); + } + + [Fact] + public void List_SiteScopedReader_ExplicitOutOfScopeSiteFilter_Unauthorized() + { + SeedSiteWithConnection(1, "SITE1", "Gw1", "MxGateway"); + var actor = CreateActor(); + actor.Tell(ScopedEnvelope(new ListSecuredWritesCommand(null, "SITE1"), "bob", ["3"], "Verifier")); + ExpectMsg(TimeSpan.FromSeconds(5)); + } + + [Fact] + public void List_SiteScopedReader_Unfiltered_QueriesOnlyPermittedIdentifiers() + { + _siteRepo.GetAllSitesAsync(Arg.Any()).Returns(new List + { + new("Site1", "SITE1") { Id = 1 }, + new("Site3", "SITE3") { Id = 3 }, + }); + _securedWriteRepo.QueryAsync(null, null, 0, 200, + Arg.Any?>(), Arg.Any()) + .Returns(new List()); + var actor = CreateActor(); + actor.Tell(ScopedEnvelope(new ListSecuredWritesCommand(null, null), "bob", ["3"], "Verifier")); + ExpectMsg(TimeSpan.FromSeconds(5)); + _securedWriteRepo.Received(1).QueryAsync(null, null, 0, 200, + Arg.Is?>(s => s != null && s.Single() == "SITE3"), + Arg.Any()); + } + + [Fact] + public void List_SystemWideReader_PassesNullPermittedFilter() + { + _securedWriteRepo.QueryAsync(null, null, 0, 200, + Arg.Any?>(), Arg.Any()) + .Returns(new List()); + var actor = CreateActor(); + actor.Tell(Envelope(new ListSecuredWritesCommand(null, null), "carol", "Operator")); + ExpectMsg(TimeSpan.FromSeconds(5)); + _securedWriteRepo.Received(1).QueryAsync(null, null, 0, 200, + Arg.Is?>(s => s == null), + Arg.Any()); } // ------------------------------------------------------------------------ @@ -623,16 +665,16 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable [Fact] public void List_WithSkipTake_ForwardsPagingAndReturnsTotalCount() { - _securedWriteRepo.QueryAsync(null, null, 10, 25, Arg.Any()) + _securedWriteRepo.QueryAsync(null, null, 10, 25, Arg.Any?>(), Arg.Any()) .Returns(new List()); - _securedWriteRepo.CountAsync(null, null, Arg.Any()).Returns(87); + _securedWriteRepo.CountAsync(null, null, Arg.Any?>(), 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()); + _securedWriteRepo.Received(1).QueryAsync(null, null, 10, 25, Arg.Any?>(), Arg.Any()); + _securedWriteRepo.Received(1).CountAsync(null, null, Arg.Any?>(), Arg.Any()); Assert.Contains("\"totalCount\":87", resp.JsonData); } @@ -655,7 +697,7 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable ValueJson = "false", ValueType = "Boolean", Status = "Pending", OperatorUser = "alice", SubmittedAtUtc = DateTime.UtcNow.AddHours(-25) // TTL default 24h }; - _securedWriteRepo.QueryAsync(null, null, 0, 200, Arg.Any()) + _securedWriteRepo.QueryAsync(null, null, 0, 200, Arg.Any?>(), Arg.Any()) .Returns(new List { fresh, stale }); _securedWriteRepo.TryMarkExpiredAsync(2, Arg.Any(), Arg.Any()) .Returns(true);