fix(management): site-scope ListDataConnections + hoist scope checks above resolve/status oracles (arch-review 2026-08-19 P6-4 + P6-5)
P6-4: HandleListDataConnections took no user at all and returned EVERY site's connections to any authenticated caller, nullifying the row-98 fail-closed gate on HandleGetDataConnection — Get denied exactly what List handed over. DataConnectionPublicShape already scrubs secrets, so the leak was site topology plus a connection-id existence oracle, but it still defeated scoping. List now takes the AuthenticatedUser: an explicit SiteId goes through EnforceSiteScope (the ListAreas shape), and an unfiltered list from a site-scoped non-Administrator caller is narrowed to their permitted sites (the ListSites shape). GetRequiredRoles is untouched — scoping is not a role gate, so the frozen 138-command authz matrix is unaffected. The Central UI Design > Connections page bypasses the actor entirely (ISiteRepository.GetAllDataConnectionsAsync direct) and IS reachable by a scoped user: it is gated RequireDesign, and SiteScopeService explicitly does NOT exempt Designer (register row 86). It now filters sites through SiteScope.FilterSitesAsync and keeps only their connections, mirroring EventLogs/AlarmSummary. P6-5: four handlers resolved their target — and on approve/reject reported its status — BEFORE the scope check, so an out-of-scope caller could tell a real id from a fake one, and read a secured write's lifecycle status, from the error shape alone. The scope check is hoisted above the not-found and status checks in HandleSubmitSecuredWrite, HandleApproveSecuredWrite, HandleRejectSecuredWrite and HandleDebugSnapshot, with the unresolvable case routed through the existing DenyUnresolvedTargetIfScoped so the denial is identical whether or not the target exists. System-wide callers keep their ordinary not-found/status answers, and the P6-1 CAS semantics on approve/reject are unchanged (only the checks above the CAS moved). Tests: 8 new ManagementService tests + 2 CentralUI bUnit tests, all verified to FAIL against the unfixed handlers (src reverted, tests re-run) and pass after — including the unscoped/Administrator control cases proving behaviour for system-wide callers is unchanged. Full ManagementService.Tests 545 green, CentralUI.Tests 1073 green.
This commit is contained in:
Joseph Doherty
@@ -40,6 +40,9 @@ public class DataConnectionsPageTests : BunitContext
|
||||
// Satisfy the page's [Inject] IDialogService — the host that actually
|
||||
// renders the dialog lives in MainLayout, not in bUnit's render scope.
|
||||
Services.AddScoped<IDialogService, DialogService>();
|
||||
// arch-review P6-4: the page now filters its site/connection tree through
|
||||
// SiteScopeService, so it must be resolvable in every render test.
|
||||
Services.AddScoped<ZB.MOM.WW.ScadaBridge.CentralUI.Auth.SiteScopeService>();
|
||||
AddTestAuth();
|
||||
|
||||
// Default: no health known. Tests that assert badge state override this.
|
||||
@@ -271,4 +274,66 @@ public class DataConnectionsPageTests : BunitContext
|
||||
Assert.Contains(optionLabels, l => l.Contains("Plant-C"));
|
||||
Assert.DoesNotContain(optionLabels, l => l.Contains("Plant-A"));
|
||||
}
|
||||
|
||||
// ── arch-review P6-4: site scoping. This page reads ISiteRepository directly,
|
||||
// bypassing ManagementActor's scope gate, and Designer is NOT exempt from
|
||||
// site scope — so a scoped Designer used to see every site's connections. ──
|
||||
|
||||
[Fact]
|
||||
public void SiteScoping_ScopedDesigner_SeesOnlyPermittedSitesAndConnections()
|
||||
{
|
||||
SeedRepos(
|
||||
sites: new[]
|
||||
{
|
||||
new Site("Plant-A", "plant-a") { Id = 1 },
|
||||
new Site("Plant-B", "plant-b") { Id = 2 }
|
||||
},
|
||||
connections: new[]
|
||||
{
|
||||
new DataConnection("PLC-A", "OpcUa", 1) { Id = 100 },
|
||||
new DataConnection("PLC-B", "OpcUa", 2) { Id = 200 }
|
||||
});
|
||||
|
||||
var scopedUser = new ClaimsPrincipal(new ClaimsIdentity(new[]
|
||||
{
|
||||
new Claim(JwtTokenService.UsernameClaimType, "scoped"),
|
||||
new Claim(JwtTokenService.RoleClaimType, "Designer"),
|
||||
new Claim(JwtTokenService.SiteIdClaimType, "1"), // Plant-A only
|
||||
}, "TestAuth"));
|
||||
Services.AddSingleton<AuthenticationStateProvider>(new TestAuthStateProvider(scopedUser));
|
||||
|
||||
var cut = Render<DataConnectionsPage>();
|
||||
FindToggleForLabel(cut, "Plant-A")!.Click();
|
||||
|
||||
Assert.Contains("Plant-A", cut.Markup);
|
||||
Assert.Contains("PLC-A", cut.Markup);
|
||||
// Neither the out-of-scope site nor its connection may appear anywhere.
|
||||
Assert.DoesNotContain("Plant-B", cut.Markup);
|
||||
Assert.DoesNotContain("PLC-B", cut.Markup);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void SiteScoping_SystemWideUser_SeesEveryConnection()
|
||||
{
|
||||
// The unscoped case is unchanged: the default test principal is an
|
||||
// Administrator (system-wide), so the filter is a no-op.
|
||||
SeedRepos(
|
||||
sites: new[]
|
||||
{
|
||||
new Site("Plant-A", "plant-a") { Id = 1 },
|
||||
new Site("Plant-B", "plant-b") { Id = 2 }
|
||||
},
|
||||
connections: new[]
|
||||
{
|
||||
new DataConnection("PLC-A", "OpcUa", 1) { Id = 100 },
|
||||
new DataConnection("PLC-B", "OpcUa", 2) { Id = 200 }
|
||||
});
|
||||
|
||||
var cut = Render<DataConnectionsPage>();
|
||||
FindToggleForLabel(cut, "Plant-A")!.Click();
|
||||
FindToggleForLabel(cut, "Plant-B")!.Click();
|
||||
|
||||
Assert.Contains("PLC-A", cut.Markup);
|
||||
Assert.Contains("PLC-B", cut.Markup);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1309,6 +1309,129 @@ public class ManagementActorTests : TestKit, IDisposable
|
||||
Assert.Equal(envelope.CorrelationId, response.CorrelationId);
|
||||
}
|
||||
|
||||
// ========================================================================
|
||||
// Site scoping: list filtering + resolve-before-scope oracles
|
||||
// (arch-review 2026-08-19 P6-4 + P6-5)
|
||||
// ========================================================================
|
||||
|
||||
/// <summary>
|
||||
/// P6-4: the unfiltered list used to return EVERY site's connections to any
|
||||
/// authenticated caller, handing a scoped caller exactly the rows
|
||||
/// <c>GetDataConnection</c> denies them.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void ListDataConnections_SiteScopedUser_ReturnsOnlyPermittedSitesConnections()
|
||||
{
|
||||
var siteRepo = Substitute.For<ISiteRepository>();
|
||||
siteRepo.GetAllDataConnectionsAsync(Arg.Any<CancellationToken>())
|
||||
.Returns(new List<DataConnection>
|
||||
{
|
||||
new("ConnA", "OpcUa", 1) { Id = 1 },
|
||||
new("ConnB", "OpcUa", 2) { Id = 2 },
|
||||
});
|
||||
_services.AddScoped(_ => siteRepo);
|
||||
|
||||
var actor = CreateActor();
|
||||
actor.Tell(ScopedEnvelope(new ListDataConnectionsCommand(null), new[] { "1" }, "Deployer"));
|
||||
|
||||
var response = ExpectMsg<ManagementSuccess>(TimeSpan.FromSeconds(5));
|
||||
Assert.Contains("ConnA", response.JsonData);
|
||||
Assert.DoesNotContain("ConnB", response.JsonData);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ListDataConnections_SystemWideUser_ReturnsEveryConnection()
|
||||
{
|
||||
var siteRepo = Substitute.For<ISiteRepository>();
|
||||
siteRepo.GetAllDataConnectionsAsync(Arg.Any<CancellationToken>())
|
||||
.Returns(new List<DataConnection>
|
||||
{
|
||||
new("ConnA", "OpcUa", 1) { Id = 1 },
|
||||
new("ConnB", "OpcUa", 2) { Id = 2 },
|
||||
});
|
||||
_services.AddScoped(_ => siteRepo);
|
||||
|
||||
var actor = CreateActor();
|
||||
actor.Tell(Envelope(new ListDataConnectionsCommand(null), "Deployer"));
|
||||
|
||||
var response = ExpectMsg<ManagementSuccess>(TimeSpan.FromSeconds(5));
|
||||
Assert.Contains("ConnA", response.JsonData);
|
||||
Assert.Contains("ConnB", response.JsonData);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ListDataConnections_ScopedAdministrator_ReturnsEveryConnection()
|
||||
{
|
||||
// Administrator bypasses scope even with PermittedSiteIds set — same rule
|
||||
// EnforceSiteScope applies everywhere else.
|
||||
var siteRepo = Substitute.For<ISiteRepository>();
|
||||
siteRepo.GetAllDataConnectionsAsync(Arg.Any<CancellationToken>())
|
||||
.Returns(new List<DataConnection>
|
||||
{
|
||||
new("ConnA", "OpcUa", 1) { Id = 1 },
|
||||
new("ConnB", "OpcUa", 2) { Id = 2 },
|
||||
});
|
||||
_services.AddScoped(_ => siteRepo);
|
||||
|
||||
var actor = CreateActor();
|
||||
actor.Tell(ScopedEnvelope(new ListDataConnectionsCommand(null), new[] { "1" }, "Administrator"));
|
||||
|
||||
var response = ExpectMsg<ManagementSuccess>(TimeSpan.FromSeconds(5));
|
||||
Assert.Contains("ConnA", response.JsonData);
|
||||
Assert.Contains("ConnB", response.JsonData);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ListDataConnections_ExplicitOutOfScopeSiteFilter_ReturnsUnauthorized()
|
||||
{
|
||||
var siteRepo = Substitute.For<ISiteRepository>();
|
||||
siteRepo.GetDataConnectionsBySiteIdAsync(2, Arg.Any<CancellationToken>())
|
||||
.Returns(new List<DataConnection> { new("ConnB", "OpcUa", 2) { Id = 2 } });
|
||||
_services.AddScoped(_ => siteRepo);
|
||||
|
||||
var actor = CreateActor();
|
||||
var envelope = ScopedEnvelope(new ListDataConnectionsCommand(2), new[] { "1" }, "Deployer");
|
||||
|
||||
actor.Tell(envelope);
|
||||
|
||||
var response = ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
|
||||
Assert.Equal(envelope.CorrelationId, response.CorrelationId);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// P6-5: the not-found check used to run before the scope check, so an
|
||||
/// out-of-scope caller could tell a real instance id from a fake one by the
|
||||
/// error they got back. Both cases must now answer with the same denial.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void DebugSnapshot_NonexistentInstanceForSiteScopedUser_ReturnsUnauthorized()
|
||||
{
|
||||
_templateRepo.GetInstanceByIdAsync(999, Arg.Any<CancellationToken>())
|
||||
.Returns((Instance?)null);
|
||||
|
||||
var actor = CreateActor();
|
||||
var envelope = ScopedEnvelope(new DebugSnapshotCommand(999), new[] { "1" }, "Deployer");
|
||||
|
||||
actor.Tell(envelope);
|
||||
|
||||
var response = ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
|
||||
Assert.Equal(envelope.CorrelationId, response.CorrelationId);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void DebugSnapshot_NonexistentInstanceForSystemWideUser_StillReportsNotFound()
|
||||
{
|
||||
// The fail-closed arm must not change the unscoped caller's answer.
|
||||
_templateRepo.GetInstanceByIdAsync(999, Arg.Any<CancellationToken>())
|
||||
.Returns((Instance?)null);
|
||||
|
||||
var actor = CreateActor();
|
||||
actor.Tell(Envelope(new DebugSnapshotCommand(999), "Deployer"));
|
||||
|
||||
var response = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
|
||||
Assert.Contains("not found", response.Error);
|
||||
}
|
||||
|
||||
// ========================================================================
|
||||
// Serialization (finding ManagementService-007)
|
||||
//
|
||||
|
||||
@@ -1138,4 +1138,134 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable
|
||||
var response = ExpectMsg<object>(TimeSpan.FromSeconds(5));
|
||||
Assert.IsNotType<ManagementUnauthorized>(response);
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------------
|
||||
// Resolve-before-scope oracles (arch-review 2026-08-19 P6-5)
|
||||
//
|
||||
// Submit/approve/reject used to answer "not found" (and, on approve/reject,
|
||||
// "is 'Executed', not Pending") BEFORE the scope gate ran, so an out-of-scope
|
||||
// caller could enumerate row ids and read their lifecycle status by the error
|
||||
// shape alone. Every out-of-scope caller must now get the SAME scope denial
|
||||
// whether or not the target exists, and whatever its status.
|
||||
// ------------------------------------------------------------------------
|
||||
|
||||
/// <summary>Seeds a non-Pending (already executed) row for the status-oracle tests.</summary>
|
||||
private void SeedExecutedWrite(long id) =>
|
||||
_securedWriteRepo.GetAsync(id, Arg.Any<CancellationToken>()).Returns(new PendingSecuredWrite
|
||||
{
|
||||
Id = id, SiteId = "SITE1", ConnectionName = "Gw1", TagPath = "Tag.A",
|
||||
ValueJson = "true", ValueType = "Boolean", Status = "Executed",
|
||||
OperatorUser = "alice", SubmittedAtUtc = DateTime.UtcNow,
|
||||
});
|
||||
|
||||
[Fact]
|
||||
public void Approve_SiteScopedVerifier_NonexistentId_Unauthorized_NotNotFound()
|
||||
{
|
||||
_securedWriteRepo.GetAsync(999, Arg.Any<CancellationToken>())
|
||||
.Returns((PendingSecuredWrite?)null);
|
||||
|
||||
var actor = CreateActor();
|
||||
actor.Tell(ScopedEnvelope(new ApproveSecuredWriteCommand(999, "ok"), "bob", ["3"], "Verifier"));
|
||||
|
||||
ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Approve_SiteScopedVerifier_OutOfScopeExecutedRow_Unauthorized_NotStatusError()
|
||||
{
|
||||
SeedSiteWithConnection(1, "SITE1", "Gw1", "MxGateway");
|
||||
SeedExecutedWrite(42);
|
||||
|
||||
var actor = CreateActor();
|
||||
actor.Tell(ScopedEnvelope(new ApproveSecuredWriteCommand(42, "ok"), "bob", ["3"], "Verifier"));
|
||||
|
||||
// A ManagementError naming 'Executed' would tell the caller the row exists
|
||||
// and what happened to it — the oracle this hoist closes.
|
||||
ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
|
||||
Assert.Equal(0, _comms.CallCount);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Approve_SystemWideVerifier_NonexistentId_StillReportsNotFound()
|
||||
{
|
||||
// The fail-closed arm must leave the unscoped caller's answers untouched.
|
||||
_securedWriteRepo.GetAsync(999, Arg.Any<CancellationToken>())
|
||||
.Returns((PendingSecuredWrite?)null);
|
||||
|
||||
var actor = CreateActor();
|
||||
actor.Tell(Envelope(new ApproveSecuredWriteCommand(999, "ok"), "bob", "Verifier"));
|
||||
|
||||
var response = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
|
||||
Assert.Contains("not found", response.Error);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Reject_SiteScopedVerifier_NonexistentId_Unauthorized_NotNotFound()
|
||||
{
|
||||
_securedWriteRepo.GetAsync(999, Arg.Any<CancellationToken>())
|
||||
.Returns((PendingSecuredWrite?)null);
|
||||
|
||||
var actor = CreateActor();
|
||||
actor.Tell(ScopedEnvelope(new RejectSecuredWriteCommand(999, "no"), "bob", ["3"], "Verifier"));
|
||||
|
||||
ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Reject_SiteScopedVerifier_OutOfScopeExecutedRow_Unauthorized_NotStatusError()
|
||||
{
|
||||
SeedSiteWithConnection(1, "SITE1", "Gw1", "MxGateway");
|
||||
SeedExecutedWrite(42);
|
||||
|
||||
var actor = CreateActor();
|
||||
actor.Tell(ScopedEnvelope(new RejectSecuredWriteCommand(42, "no"), "bob", ["3"], "Verifier"));
|
||||
|
||||
ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
|
||||
_securedWriteRepo.DidNotReceiveWithAnyArgs().TryMarkRejectedAsync(
|
||||
default, default!, default, default, default);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Reject_SystemWideVerifier_ExecutedRow_StillReportsStatusConflict()
|
||||
{
|
||||
// In-scope/unscoped behavior is unchanged: the status error still names
|
||||
// the status that blocked the transition (the P6-1 CAS shape is intact).
|
||||
SeedExecutedWrite(42);
|
||||
|
||||
var actor = CreateActor();
|
||||
actor.Tell(Envelope(new RejectSecuredWriteCommand(42, "no"), "bob", "Verifier"));
|
||||
|
||||
var response = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
|
||||
Assert.Contains("Executed", response.Error);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Submit_SiteScopedOperator_NonexistentSite_Unauthorized_NotNotFound()
|
||||
{
|
||||
_siteRepo.GetSiteByIdentifierAsync("GHOST", Arg.Any<CancellationToken>())
|
||||
.Returns((Site?)null);
|
||||
|
||||
var actor = CreateActor();
|
||||
actor.Tell(ScopedEnvelope(
|
||||
new SubmitSecuredWriteCommand("GHOST", "Gw1", "Tag.A", "true", "Boolean", "go"),
|
||||
"alice", ["3"], "Operator"));
|
||||
|
||||
ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
|
||||
_securedWriteRepo.DidNotReceiveWithAnyArgs().AddAsync(default!, default);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Submit_SystemWideOperator_NonexistentSite_StillReportsNotFound()
|
||||
{
|
||||
_siteRepo.GetSiteByIdentifierAsync("GHOST", Arg.Any<CancellationToken>())
|
||||
.Returns((Site?)null);
|
||||
|
||||
var actor = CreateActor();
|
||||
actor.Tell(Envelope(
|
||||
new SubmitSecuredWriteCommand("GHOST", "Gw1", "Tag.A", "true", "Boolean", "go"),
|
||||
"alice", "Operator"));
|
||||
|
||||
var response = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
|
||||
Assert.Contains("not found", response.Error);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user