diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs index bd884279..2a802c85 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs @@ -506,6 +506,23 @@ public class InboundScriptExecutor _logger.LogDebug("Inbound API request for method {Method} cancelled by client", method.Name); return new InboundScriptResult(false, null, "Request cancelled by client"); } + catch (RouteSiteScopeException ex) + { + // Arch-review F2: the script routed to a site outside the caller's site + // scope. RouteTarget.ResolveSiteAsync refused BEFORE any site traffic, so + // the script never saw a routed reply — there is nothing to roll back. + // The message is the operator-facing denial, safe to surface verbatim + // (it names only the instance and its site), unlike the generic + // SCRIPT_ERROR path which deliberately hides internals. + // + // Ordered before the SiteUnreachableException catch purely for + // readability: the two types are siblings under InvalidOperationException, + // so neither shadows the other. + _logger.LogWarning( + "API method {Method} refused a routed call outside the caller's site scope: {Message}", + method.Name, ex.Message); + return new InboundScriptResult(false, null, ex.Message, "SITE_SCOPE_DENIED"); + } catch (SiteUnreachableException) { // C3: a routed Route.To(...).Call(...) reached a site that could not be diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs index 1ca627a1..b9a745d1 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs @@ -36,6 +36,34 @@ public sealed class SiteUnreachableException : InvalidOperationException public SiteUnreachableException(string message, Exception innerException) : base(message, innerException) { } } +/// +/// Arch-review F2: a routed call whose target site is outside the caller's site +/// scope. Raised by RouteTarget.ResolveSiteAsync — the single choke point every routed +/// operation passes through — so it is thrown before any site traffic: nothing is sent, +/// nothing is read, nothing is written. +/// +/// +/// The scope exists for the Flow Debugger's method-entry Live run, where an operator picks a +/// method rather than a site: a site-scoped Deployer must not reach outside their sites just +/// because the method's script routes there. The ordinary HTTP inbound path attaches no scope +/// (an API key is not a site-scoped principal) and is unaffected. +/// +/// +/// +/// Derives from so existing callers catching the base +/// type are unaffected, while catches this more specific +/// type to surface the SITE_SCOPE_DENIED error code instead of a generic +/// SCRIPT_ERROR. Deliberately NOT the ManagementService's SiteScopeViolationException +/// — InboundAPI cannot reference ManagementService; the runner converts the code back. +/// +/// +public sealed class RouteSiteScopeException : InvalidOperationException +{ + /// Initializes a new with the given message. + /// The scope-denial message naming the instance and its site. + public RouteSiteScopeException(string message) : base(message) { } +} + /// /// Route.To() helper for cross-site calls from inbound API scripts. /// Resolves instance to site, routes via , blocks until @@ -57,6 +85,7 @@ public class RouteHelper private readonly INodeIdentityProvider? _nodeIdentity; private readonly bool _trace; private readonly Action? _traceSink; + private readonly IReadOnlySet? _permittedSiteIdentifiers; /// /// Initializes a new with no deadline token and no parent execution id. @@ -73,7 +102,7 @@ public class RouteHelper ICentralAuditWriter? auditWriter = null, INodeIdentityProvider? nodeIdentity = null) : this(instanceLocator, instanceRouter, CancellationToken.None, CancellationToken.None, parentExecutionId: null, - auditWriter, nodeIdentity, trace: false, traceSink: null) + auditWriter, nodeIdentity, trace: false, traceSink: null, permittedSiteIdentifiers: null) { } @@ -86,8 +115,10 @@ public class RouteHelper ICentralAuditWriter? auditWriter, INodeIdentityProvider? nodeIdentity, bool trace, - Action? traceSink) + Action? traceSink, + IReadOnlySet? permittedSiteIdentifiers) { + _permittedSiteIdentifiers = permittedSiteIdentifiers; _instanceLocator = instanceLocator; _instanceRouter = instanceRouter; _deadlineToken = deadlineToken; @@ -110,7 +141,7 @@ public class RouteHelper /// A new inheriting the given deadline token. public RouteHelper WithDeadline(CancellationToken deadlineToken) => new(_instanceLocator, _instanceRouter, deadlineToken, _requestAbortedToken, _parentExecutionId, - _auditWriter, _nodeIdentity, _trace, _traceSink); + _auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers); /// /// Returns a carrying the raw request-abort @@ -124,7 +155,7 @@ public class RouteHelper /// A new carrying the given request-abort token. public RouteHelper WithRequestAborted(CancellationToken requestAbortedToken) => new(_instanceLocator, _instanceRouter, _deadlineToken, requestAbortedToken, _parentExecutionId, - _auditWriter, _nodeIdentity, _trace, _traceSink); + _auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers); /// /// Audit Log (ParentExecutionId): returns a whose @@ -139,7 +170,7 @@ public class RouteHelper /// A new carrying the given parent execution id. public RouteHelper WithParentExecutionId(Guid? parentExecutionId) => new(_instanceLocator, _instanceRouter, _deadlineToken, _requestAbortedToken, parentExecutionId, - _auditWriter, _nodeIdentity, _trace, _traceSink); + _auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers); /// /// Flow Debugger (design 2026-09-05 §5.2): returns a whose routed @@ -150,7 +181,7 @@ public class RouteHelper /// A new carrying the flag. public RouteHelper WithTrace(bool trace) => new(_instanceLocator, _instanceRouter, _deadlineToken, _requestAbortedToken, _parentExecutionId, - _auditWriter, _nodeIdentity, trace, _traceSink); + _auditWriter, _nodeIdentity, trace, _traceSink, _permittedSiteIdentifiers); /// /// Flow Debugger (design 2026-09-05 §7.1): returns a that also @@ -162,7 +193,7 @@ public class RouteHelper /// A new carrying the sink. public RouteHelper WithTraceSink(Action? traceSink) => new(_instanceLocator, _instanceRouter, _deadlineToken, _requestAbortedToken, _parentExecutionId, - _auditWriter, _nodeIdentity, _trace, traceSink); + _auditWriter, _nodeIdentity, _trace, traceSink, _permittedSiteIdentifiers); /// /// Flow Debugger outbound calls (plan 2026-09-06, D5/D7): the trace sink this helper carries, @@ -172,6 +203,32 @@ public class RouteHelper /// public Action? TraceSink => _traceSink; + /// + /// Arch-review F2: returns a that refuses a routed call + /// whose target site is not in — site + /// identifiers (e.g. site-a), the values returns, + /// NOT the numeric ids an AuthenticatedUser.PermittedSiteIds carries. Matching is + /// whatever comparer the supplied set uses, so pass an ordinal-ignore-case set. + /// + /// + /// null = unscoped, the default and the state of every ordinary inbound HTTP request. + /// An EMPTY set is not the same thing: it permits nothing, which is the correct outcome for + /// a site-scoped user whose sites no longer exist. + /// + /// + /// + /// The Flow Debugger's method-entry Live run sets this from the operator's site-scoped + /// Deployer role, because the operator picks a method — the site is only known once + /// the script routes, so the check cannot happen up front the way it does for a value-change + /// run's named instance. + /// + /// + /// The site identifiers routed calls may reach; null = unscoped. + /// A new carrying the scope. + public RouteHelper WithSiteScope(IReadOnlySet? permittedSiteIdentifiers) => + new(_instanceLocator, _instanceRouter, _deadlineToken, _requestAbortedToken, _parentExecutionId, + _auditWriter, _nodeIdentity, _trace, _traceSink, permittedSiteIdentifiers); + /// /// Creates a route target for the specified instance. /// @@ -181,7 +238,7 @@ public class RouteHelper { return new RouteTarget( instanceCode, _instanceLocator, _instanceRouter, _deadlineToken, _requestAbortedToken, _parentExecutionId, - _auditWriter, _nodeIdentity, _trace, _traceSink); + _auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers); } } @@ -200,6 +257,7 @@ public class RouteTarget private readonly INodeIdentityProvider? _nodeIdentity; private readonly bool _trace; private readonly Action? _traceSink; + private readonly IReadOnlySet? _permittedSiteIdentifiers; /// /// Initializes a new for the specified instance. @@ -210,6 +268,11 @@ public class RouteTarget /// Cancellation token representing the method-level deadline. /// Raw client-disconnect token, independent of the method timeout. /// Optional parent execution id for audit correlation on routed calls. + /// Central-direct-write audit writer for the always-on RouteCall row; null = no row. + /// Stamps SourceNode on the RouteCall row; null = NULL. + /// True to trace the routed site run. + /// Receives the emitted audit events in order; null = none. + /// Arch-review F2: site identifiers this call may reach; null = unscoped. internal RouteTarget( string instanceCode, IInstanceLocator instanceLocator, @@ -220,8 +283,10 @@ public class RouteTarget ICentralAuditWriter? auditWriter = null, INodeIdentityProvider? nodeIdentity = null, bool trace = false, - Action? traceSink = null) + Action? traceSink = null, + IReadOnlySet? permittedSiteIdentifiers = null) { + _permittedSiteIdentifiers = permittedSiteIdentifiers; _instanceCode = instanceCode; _instanceLocator = instanceLocator; _instanceRouter = instanceRouter; @@ -549,6 +614,16 @@ public class RouteTarget $"Instance '{_instanceCode}' not found or has no assigned site"); } + // Arch-review F2: the site scope gate. This method is the single choke point — + // Call, GetAttributes, WaitForAttribute and SetAttributes all resolve the site + // here first — so refusing at this point guarantees no site traffic of any kind + // is emitted for an out-of-scope instance. + if (_permittedSiteIdentifiers is not null && !_permittedSiteIdentifiers.Contains(siteId)) + { + throw new RouteSiteScopeException( + $"Access denied: instance '{_instanceCode}' is at site '{siteId}', outside your site scope."); + } + return siteId; } diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/FlowDebug/FlowDebugRunner.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/FlowDebug/FlowDebugRunner.cs index 90cf04fe..b1198356 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/FlowDebug/FlowDebugRunner.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/FlowDebug/FlowDebugRunner.cs @@ -38,6 +38,14 @@ public sealed class FlowDebugRunner /// Error code when the method / instance / site does not exist. public const string NotFoundCode = "NOT_FOUND"; + /// + /// Arch-review F2: 's code for a routed call refused + /// because the target site is outside the operator's site scope. InboundAPI cannot + /// reference this assembly, so it raises its own RouteSiteScopeException and the + /// runner converts the code back to . + /// + public const string SiteScopeDeniedCode = "SITE_SCOPE_DENIED"; + /// The Extra.source value stamped on rows this runner synthesizes. public const string SourceTag = "flow-debugger"; @@ -103,7 +111,12 @@ public sealed class FlowDebugRunner var sw = Stopwatch.StartNew(); var timeout = TimeSpan.FromSeconds(Math.Max(1, method.TimeoutSeconds)); - var route = sp.GetRequiredService().WithTraceSink(events.Enqueue); + // Arch-review F2: a method Live run has no single target site to check up front the + // way RunValueChangeAsync does — the operator picks a METHOD, and where it routes is + // only known once the script runs. So the operator's scope is pushed into the + // RouteHelper and enforced at route time, before any site traffic. + var scope = await ResolvePermittedSiteIdentifiersAsync(sp, user, ct).ConfigureAwait(false); + var route = sp.GetRequiredService().WithTraceSink(events.Enqueue).WithSiteScope(scope); var executor = sp.GetRequiredService(); InboundScriptResult result; @@ -140,6 +153,16 @@ public sealed class FlowDebugRunner extra: new ScriptTraceExtra { Source = SourceTag }.ToJson()); await WriteCentralAsync(sp, inboundRow, ct).ConfigureAwait(false); + // Arch-review F2: convert the executor's code into the same authorization failure + // RunValueChangeAsync raises, so both Live entry points surface one exception type + // (→ ManagementUnauthorized). Deliberately AFTER the inbound row is written: a + // denied attempt must still be audited against the operator's name. + if (string.Equals(result.ErrorCode, SiteScopeDeniedCode, StringComparison.Ordinal)) + { + throw new SiteScopeViolationException( + result.ErrorMessage ?? "Access denied: routed call outside your site scope."); + } + var ordered = new List(events.Count + 1) { inboundRow }; ordered.AddRange(events.OrderBy(e => e.OccurredAtUtc).ThenBy(e => e.EventId)); @@ -301,6 +324,35 @@ public sealed class FlowDebugRunner } } + /// + /// Arch-review F2: the operator's PermittedSiteIds — NUMERIC site ids as strings — + /// resolved to the site IDENTIFIERS speaks, for + /// RouteHelper.WithSiteScope. Returns null (unscoped) for an unscoped user or an + /// Administrator, exactly as treats them. + /// + /// + /// A returned set may be EMPTY — a scoped user whose sites no longer exist routes nowhere. + /// That is the intended fail-closed outcome, and is why null and empty are not the same. + /// + /// + private static async Task?> ResolvePermittedSiteIdentifiersAsync( + IServiceProvider sp, AuthenticatedUser user, CancellationToken ct) + { + if (user.PermittedSiteIds.Length == 0) return null; + if (user.Roles.Contains(Roles.Administrator, StringComparer.OrdinalIgnoreCase)) return null; + + var sites = sp.GetRequiredService(); + var set = new HashSet(StringComparer.OrdinalIgnoreCase); + foreach (var raw in user.PermittedSiteIds) + { + if (!int.TryParse(raw, out var id)) continue; + var site = await sites.GetSiteByIdAsync(id, ct).ConfigureAwait(false); + if (site is not null) set.Add(site.SiteIdentifier); + } + + return set; + } + private async Task WriteCentralAsync(IServiceProvider sp, AuditEvent evt, CancellationToken ct) { var writer = sp.GetService(); diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/RouteHelperTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/RouteHelperTests.cs index 70712c81..e07d4baf 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/RouteHelperTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/RouteHelperTests.cs @@ -621,4 +621,86 @@ public class RouteHelperTests await Assert.ThrowsAsync( () => CreateHelper().To("inst").GetAttributes(new[] { "a" })); } + + // --- Arch-review F2: site scope on routed calls --- + // + // The gate lives in RouteTarget.ResolveSiteAsync — the single choke point every + // routed operation passes through — so it fires BEFORE any site traffic. + + private static IReadOnlySet Scope(params string[] siteIdentifiers) => + new HashSet(siteIdentifiers, StringComparer.OrdinalIgnoreCase); + + [Fact] + public async Task Call_OutsideSiteScope_ThrowsRouteSiteScopeException_BeforeRouting() + { + SiteResolves("inst-1", "site-b"); + + var helper = CreateHelper().WithSiteScope(Scope("site-a")); + + var ex = await Assert.ThrowsAsync( + () => helper.To("inst-1").Call("doWork")); + Assert.Contains("site-b", ex.Message); + Assert.Contains("inst-1", ex.Message); + // Nothing was routed: the check runs before the site is contacted. + await _router.DidNotReceiveWithAnyArgs().RouteToCallAsync(default!, default!, default); + } + + [Fact] + public async Task Call_InsideSiteScope_Routes() + { + SiteResolves("inst-1", "site-b"); + _router.RouteToCallAsync("site-b", Arg.Any(), Arg.Any()) + .Returns(ci => new RouteToCallResponse( + ((RouteToCallRequest)ci[1]).CorrelationId, true, 42, null, DateTimeOffset.UtcNow)); + + var helper = CreateHelper().WithSiteScope(Scope("SITE-B")); + + Assert.Equal(42, await helper.To("inst-1").Call("doWork")); + } + + [Fact] + public async Task Call_NullScope_IsUnscoped() + { + SiteResolves("inst-1", "site-b"); + _router.RouteToCallAsync("site-b", Arg.Any(), Arg.Any()) + .Returns(ci => new RouteToCallResponse( + ((RouteToCallRequest)ci[1]).CorrelationId, true, 42, null, DateTimeOffset.UtcNow)); + + var helper = CreateHelper().WithSiteScope(null); + + Assert.Equal(42, await helper.To("inst-1").Call("doWork")); + } + + [Fact] + public async Task SetAttributes_OutsideSiteScope_ThrowsRouteSiteScopeException_BeforeRouting() + { + // ResolveSiteAsync is shared by all four routed operations, so the gate is + // not Call-specific. + SiteResolves("inst-1", "site-b"); + + var helper = CreateHelper().WithSiteScope(Scope("site-a")); + + await Assert.ThrowsAsync( + () => helper.To("inst-1").SetAttributes(new Dictionary { ["x"] = "1" })); + await _router.DidNotReceiveWithAnyArgs().RouteToSetAttributesAsync(default!, default!, default); + } + + [Fact] + public async Task WithSiteScope_SurvivesTheOtherWithCopies() + { + // The scope must not be dropped by the copies InboundScriptExecutor makes + // (WithDeadline / WithRequestAborted / WithParentExecutionId / WithTrace). + SiteResolves("inst-1", "site-b"); + + var helper = CreateHelper() + .WithSiteScope(Scope("site-a")) + .WithDeadline(CancellationToken.None) + .WithRequestAborted(CancellationToken.None) + .WithParentExecutionId(Guid.NewGuid()) + .WithTrace(true) + .WithTraceSink(null); + + await Assert.ThrowsAsync( + () => helper.To("inst-1").Call("doWork")); + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/FlowDebugRunnerTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/FlowDebugRunnerTests.cs index 8ca0642b..b7701941 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/FlowDebugRunnerTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/FlowDebugRunnerTests.cs @@ -233,6 +233,87 @@ public class FlowDebugRunnerTests Assert.Equal(FlowDebugRunner.NotFoundCode, empty.ErrorCode); } + // --- Arch-review F2: a method-entry Live run is bound by the operator's site scope --- + // + // RunValueChange already called EnforceSiteScope on the target instance; a method + // Live run has no single target site up front, so the scope is pushed into the + // RouteHelper and enforced at route time (RouteTarget.ResolveSiteAsync), before any + // site traffic. The denial comes back as SITE_SCOPE_DENIED and the runner converts it. + + /// Wires a method whose script routes to , resident at . + private ISiteRepository WireRoutedMethod(int methodId, string instanceCode, string siteIdentifier) + { + var method = new ApiMethod("M", "x") { Id = methodId, TimeoutSeconds = 5 }; + _apiRepo.GetApiMethodByIdAsync(methodId, Arg.Any()).Returns(method); + _locator.GetSiteIdForInstanceAsync(instanceCode, Arg.Any()).Returns(siteIdentifier); + _router.RouteToCallAsync(siteIdentifier, Arg.Any(), Arg.Any()) + .Returns(ci => new RouteToCallResponse( + ((RouteToCallRequest)ci[1]).CorrelationId, true, 7, null, DateTimeOffset.UtcNow)); + _executor.RegisterHandler("M", ctx => ctx.Route.To(instanceCode).Call("work")); + + // The user's PermittedSiteIds are NUMERIC ids; the locator speaks site IDENTIFIERS, + // so the runner resolves one to the other through the repository. + var sites = Substitute.For(); + sites.GetSiteByIdAsync(1, Arg.Any()).Returns(new Site("Site A", "site-a") { Id = 1 }); + _services.AddSingleton(sites); + return sites; + } + + [Fact] + public async Task RunMethod_ScopedDeployer_RouteToOtherSite_IsUnauthorized_AndInboundRowIsWritten() + { + WireRoutedMethod(11, "inst", "site-b"); + var scoped = new AuthenticatedUser("ops", "Ops", new[] { "Deployer" }, new[] { "1" }); + + await Assert.ThrowsAsync(() => Runner().RunMethodAsync( + _services.BuildServiceProvider(), new FlowDebugRunMethodCommand(11, null), scoped, CancellationToken.None)); + + // The site was never contacted... + await _router.DidNotReceiveWithAnyArgs().RouteToCallAsync(default!, default!, default); + // ...but the denied attempt IS recorded against the operator's name. + Assert.Contains(_central, e => e.AsRow().Kind == AuditKind.InboundRequest && e.Actor == "ops"); + } + + [Fact] + public async Task RunMethod_ScopedDeployer_RouteWithinScope_Runs() + { + // The same scoped user, routing to a site their numeric id DOES resolve to. + WireRoutedMethod(12, "inst", "site-a"); + var scoped = new AuthenticatedUser("ops", "Ops", new[] { "Deployer" }, new[] { "1" }); + + var result = await Runner().RunMethodAsync( + _services.BuildServiceProvider(), new FlowDebugRunMethodCommand(12, null), scoped, CancellationToken.None); + + Assert.True(result.Completed, result.Error); + await _router.ReceivedWithAnyArgs(1).RouteToCallAsync(default!, default!, default); + } + + [Fact] + public async Task RunMethod_Administrator_IsNeverScoped() + { + WireRoutedMethod(13, "inst", "site-b"); + var admin = new AuthenticatedUser("boss", "Boss", new[] { "Deployer", "Administrator" }, new[] { "1" }); + + var result = await Runner().RunMethodAsync( + _services.BuildServiceProvider(), new FlowDebugRunMethodCommand(13, null), admin, CancellationToken.None); + + Assert.True(result.Completed, result.Error); + await _router.ReceivedWithAnyArgs(1).RouteToCallAsync(default!, default!, default); + } + + [Fact] + public async Task RunMethod_UnscopedUser_IsNeverScoped() + { + WireRoutedMethod(14, "inst", "site-b"); + + // Deployer with an EMPTY PermittedSiteIds = global scope, as EnforceSiteScope treats it. + var result = await Runner().RunMethodAsync( + _services.BuildServiceProvider(), new FlowDebugRunMethodCommand(14, null), Deployer, CancellationToken.None); + + Assert.True(result.Completed, result.Error); + await _router.ReceivedWithAnyArgs(1).RouteToCallAsync(default!, default!, default); + } + [Fact] public void RoleGate_AllThreeCommands_AreDeployerOnly() {