From baab6ac78e2570b0d70d3001c7f12ce0fcecb704 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 09:23:39 -0400 Subject: [PATCH] fix(management): ListInstances pages, site-scopes and prefix-searches in SQL (review 2026-10-21 08/P2) --- .../Component-ManagementService.md | 2 +- src/ZB.MOM.WW.ScadaBridge.CLI/README.md | 2 +- .../ManagementActor.cs | 28 ++++++--- .../ManagementActorTests.cs | 61 ++++++++++++++----- 4 files changed, 69 insertions(+), 24 deletions(-) diff --git a/docs/requirements/Component-ManagementService.md b/docs/requirements/Component-ManagementService.md index 67fd6c80..758c93a3 100644 --- a/docs/requirements/Component-ManagementService.md +++ b/docs/requirements/Component-ManagementService.md @@ -120,7 +120,7 @@ The whole folder surface is reachable from the CLI as `template folder list|crea ### Instances -- **ListInstances** / **GetInstance**: Query instances, with filtering by site and area. `ListInstances` supports additive offset paging via `Skip` / `Take` (arch-review P2), applied **after** the in-memory site-scope filter so a site-scoped user never sees an out-of-scope instance surface into their page window; same `Take = null` unlimited default and `1..1000` clamp as `ListTemplates`. (`ExportBundle` still loads the full table internally — a logged scale follow-up, arch-review P2 deferred. `QueryDeployments` was closed by WP2.5, below.) +- **ListInstances** / **GetInstance**: Query instances, with filtering by site and area. `ListInstances` supports additive offset paging via `Skip` / `Take` (arch-review P2); same `Take = null` unlimited default and `1..1000` clamp as `ListTemplates`. Paging, the site-scope filter and search all run **in SQL** (`ICentralUiRepository.GetInstancesPagedAsync`, review 2026-10-21 08/P2): a site-scoped caller's `PermittedSiteIds` become the query's site filter (intersected with an explicit `SiteId`; a non-integer grant is dropped, which can only narrow), so a page can never surface an out-of-scope instance. **`SearchTerm` is a unique-name prefix match (`StartsWith`)**, not a substring match — the previous leading-wildcard `LIKE '%term%'` could not use `IX_Instances_UniqueName`. The response is still a bare array (the total count is not returned). (`ExportBundle` still loads the full table internally — a logged scale follow-up, arch-review P2 deferred. `QueryDeployments` was closed by WP2.5, below.) - **CreateInstance**: Create a new instance from a template. - **UpdateInstanceOverrides** (`SetInstanceOverrides`): Set attribute overrides on an instance. Every requested attribute is validated against the instance's template up front (unknown or locked ⇒ the whole batch is rejected before any write), then the batch is applied as **one bulk read of the existing override rows plus one commit** (WP2.5). It previously ran K independent read-modify-commit cycles — each re-reading the instance, the template's attributes and the existing overrides, then committing and writing its own audit row — so the all-or-nothing promise was only as good as the pre-validation. It is now a real single-transaction apply, with one audit row summarising the batch. - **MgmtDeploySite** (`SiteId`, WP2.5): Bulk-deploy every deployable instance at one site. Site scope is enforced on the SITE (the command's target); every instance the batch touches belongs to that site by construction. Same `Deployer` authority as `MgmtDeployInstance` — it is N of those, not a new class of privilege — and registered as a long-running command so the caller does not time out mid-batch. Semantics, phasing and failure policy: see `Component-DeploymentManager.md` → Bulk site deployment. diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md index c798227b..8a7fb7a5 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md @@ -573,7 +573,7 @@ scadabridge --url instance list [--site-id ] [--template-id ] [- |--------|----------|-------------| | `--site-id` | no | Filter by site ID | | `--template-id` | no | Filter by template ID | -| `--search` | no | Search term matched against instance name | +| `--search` | no | Unique-name **prefix** (`StartsWith`; was a substring match before review 2026-10-21 08/P2) | | `--skip` | no | Offset paging: number of items to skip (default 0). Applied after site-scope filtering. | | `--take` | no | Offset paging: max items to return (clamped 1..1000; omit for unlimited) | diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index 96e8fd80..afe1fb59 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -946,17 +946,29 @@ public class ManagementActor : ReceiveActor private static async Task HandleListInstances(IServiceProvider sp, ListInstancesCommand cmd, AuthenticatedUser user) { - var repo = sp.GetRequiredService(); - var instances = await repo.GetInstancesFilteredAsync(cmd.SiteId, cmd.TemplateId, cmd.SearchTerm); - // Filter by permitted sites for site-scoped users + // Paged, site-scoped and searched IN SQL (review 2026-10-21 08/P2): this used to read + // every matching instance (the whole table for an unfiltered CLI `instance list`), + // scope-filter and page in memory, and search with a leading-wildcard LIKE. The scope + // is passed as the repository's site filter, so a page can never surface an + // out-of-scope instance. Search is a unique-name PREFIX (StartsWith), which can use + // IX_Instances_UniqueName. The response stays a bare array; the total is not returned. + IReadOnlyCollection? siteIds = cmd.SiteId is { } siteId ? new[] { siteId } : null; if (user.PermittedSiteIds.Length > 0 && !user.Roles.Contains(Roles.Administrator, StringComparer.OrdinalIgnoreCase)) { - var permittedIds = new HashSet(user.PermittedSiteIds); - instances = instances.Where(i => permittedIds.Contains(i.SiteId.ToString())).ToList(); + // A non-integer grant is DROPPED (can only narrow), never ignored (would widen). + var permitted = user.PermittedSiteIds + .Select(id => int.TryParse(id, out var parsed) ? (int?)parsed : null) + .OfType() + .ToHashSet(); + siteIds = siteIds is null + ? permitted.ToList() + : siteIds.Where(permitted.Contains).ToList(); } - // Paging is applied AFTER the site-scope filter so a scoped user never sees - // an out-of-scope instance surface into their page window. - return Page(instances, cmd.Skip, cmd.Take); + + var take = cmd.Take is > 0 ? Math.Min(cmd.Take.Value, 1000) : int.MaxValue; + var repo = sp.GetRequiredService(); + var (rows, _) = await repo.GetInstancesPagedAsync(siteIds, cmd.TemplateId, cmd.SearchTerm, Math.Max(0, cmd.Skip), take); + return rows; } /// diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs index 6b977f20..ed59e58c 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs @@ -2490,34 +2490,67 @@ public class ManagementActorTests : TestKit, IDisposable Assert.Contains("T-030", response.JsonData); } - /// Verifies that list instances: paging applies after site scope filter. + /// + /// Verifies that list instances pages and site-scopes IN SQL (review 2026-10-21 08/P2): a + /// scoped user's permitted sites are passed as the repository's site filter, so paging + /// can never surface an out-of-scope instance, and Skip/Take/search reach the query. + /// [Fact] public void ListInstances_PagingAppliesAfterSiteScopeFilter() { - // Scoped user permitted to site 1 only. The repo returns 3 in-scope (site 1) - // instances plus 2 out-of-scope (site 2). Take:2 must yield 2 of THEIR - // instances and never an out-of-scope one. var uiRepo = Substitute.For(); - uiRepo.GetInstancesFilteredAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) - .Returns(new List + IReadOnlyCollection? passedSites = null; + uiRepo.GetInstancesPagedAsync( + Arg.Do?>(s => passedSites = s), null, "I-", 0, 2, Arg.Any()) + .Returns((new List { new("I-001") { Id = 1, SiteId = 1 }, new("I-002") { Id = 2, SiteId = 1 }, - new("I-003") { Id = 3, SiteId = 1 }, - new("I-101") { Id = 101, SiteId = 2 }, - new("I-102") { Id = 102, SiteId = 2 }, - }); + }, 3)); _services.AddScoped(_ => uiRepo); var actor = CreateActor(); - actor.Tell(ScopedEnvelope(new ListInstancesCommand(Take: 2), new[] { "1" }, "Deployer")); + actor.Tell(ScopedEnvelope(new ListInstancesCommand(SearchTerm: "I-", Take: 2), new[] { "1", "not-an-int" }, "Deployer")); var response = ExpectMsg(TimeSpan.FromSeconds(5)); Assert.Contains("I-001", response.JsonData); Assert.Contains("I-002", response.JsonData); - Assert.DoesNotContain("I-003", response.JsonData); // paged out - Assert.DoesNotContain("I-101", response.JsonData); // out of scope - Assert.DoesNotContain("I-102", response.JsonData); // out of scope + Assert.StartsWith("[", response.JsonData.TrimStart()); // response shape unchanged: a bare array + Assert.Equal(new[] { 1 }, passedSites); // non-integer grant dropped, never widened + uiRepo.DidNotReceive().GetInstancesFilteredAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + /// + /// A scoped user's explicit SiteId is intersected with their grant; an out-of-grant site + /// yields an empty filter (nothing), and an unscoped caller's SiteId passes through. + /// + [Fact] + public void ListInstances_SiteIdIsIntersectedWithTheScope_AndUnlimitedTakeIsUnbounded() + { + var uiRepo = Substitute.For(); + var calls = new List<(IReadOnlyCollection? Sites, int Take)>(); + uiRepo.GetInstancesPagedAsync(Arg.Any?>(), Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(ci => + { + calls.Add(((IReadOnlyCollection?)ci[0], (int)ci[4])); + return (Array.Empty(), 0); + }); + _services.AddScoped(_ => uiRepo); + var actor = CreateActor(); + + actor.Tell(ScopedEnvelope(new ListInstancesCommand(SiteId: 2), new[] { "1" }, "Deployer")); + ExpectMsg(TimeSpan.FromSeconds(5)); + actor.Tell(Envelope(new ListInstancesCommand(SiteId: 2), "Deployer")); + ExpectMsg(TimeSpan.FromSeconds(5)); + actor.Tell(Envelope(new ListInstancesCommand(), "Deployer")); + ExpectMsg(TimeSpan.FromSeconds(5)); + + Assert.Empty(calls[0].Sites!); + Assert.Equal(new[] { 2 }, calls[1].Sites); + Assert.Null(calls[2].Sites); + Assert.Equal(int.MaxValue, calls[2].Take); } // ========================================================================