dohertj2/ScadaBridge
1
0
Fork 0
Code Issues 2 Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(deployments): one paged read of DeploymentRecords + close its three drifts

Browse Source 
Deferred-work register rows 37, 86, 87, 88 — the structural duplication and the
three behavioral divergences it had accumulated, fixed together.

Row 37 (structural). `IDeploymentManagerRepository.QueryDeploymentSummariesAsync`
is DELETED. It and `QueryDeploymentListPageAsync` were two paged reads of the same
insert-only table with different contracts — raw offset vs page number, no total vs
total, no instance join vs joined, instance-id-list scoping vs site-id scoping. The
CLI/HTTP `QueryDeployments` handler now goes through `QueryDeploymentListPageAsync`
with a `DeploymentListFilter`, exactly like the Central UI page. The
`DeploymentRecordSummary` projection went with it (`DeploymentListRow` is the single
row shape); its `MaxPageSize` const moved to `DeploymentListPage`, which also gains
`DefaultPageSize`.

Row 86 (Medium, authorization — the headline). `SiteScopeService.ResolveAsync` was
claims-only, so an Administrator carrying `SiteId` claims saw the whole fleet through
the CLI (`ManagementActor.EnforceSiteScope` has always exempted Administrator) and
only their granted sites on the Deployments page. One user, one dataset, two answers.
Administrator = system-wide is the intended semantic on both sides; the UI was the
defect. The role claim is read explicitly with an OrdinalIgnoreCase comparison rather
than via `IsInRole`, which compares claim VALUES ordinally and only resolves when the
identity carries the matching roleType — the two sides must not disagree on a
differently-cased role name. An Administrator's site claims are not collected at all,
so the invariant "system-wide => empty permitted-id set" holds for every caller.
Designer is NOT exempt: the class doc claimed "any Admin/Design user" is system-wide,
which was true of neither side, so the doc was corrected rather than the bypass
widened.

Row 87 (Low). The scoped branch used to resolve the site grant into an instance-id
list by loading EVERY instance in the system, non-transactionally, on every scoped
query; the grant now rides as `SiteIdScope` and is resolved by the query's own join.
An unparseable permitted-site id is DROPPED, never ignored — dropping can only narrow
a grant, discarding it would widen it to fleet-wide. And `PageSize <= 0` no longer
means UNLIMITED down at the repository (`--page-size 0` materialized the whole
retention window): it means "unspecified" and takes the CLI's own default of 50,
capped at 1000. Bounding without telling would be silent truncation, so the handler
now returns a `DeploymentQueryResult` envelope carrying `totalCount` plus the page
coordinates actually applied — with a CLI table projection (mirroring row 57's
`AuditConfigTableProjection`) that unwraps it and appends a "showing N of M" note.

Row 88 (Low, UX). An unrecognised `--status` fell back to "no filter", so a typo like
`--status Sucess` returned every status — a complete-looking answer to a question the
caller never asked. It is now a `ManagementError` naming the valid values. Parsing
stays case-insensitive; a numeric string is rejected too, since `Enum.TryParse`
accepts "7" and yields an undefined value (guarded with `Enum.IsDefined`). Loud
failure over silent full results, per row 46.

Tests: 7 new `SiteScopeServiceTests` pinning Administrator = system-wide (verified
failing 6/7 against the pre-fix service, the Designer pin passing both ways); the
ManagementActor deployment suite migrated onto the consolidated query with new pins
for the status error, case-insensitive parse, numeric rejection, page-size default/
cap/floor, empty-scope-on-garbage-grant, and "reads no instances at all"; a repository
test for site-scope+status composing over a bounded page; and 6 CLI tests for the new
table projection.
This commit is contained in:
Joseph Doherty
2026-08-17 05:15:18 -04:00
parent 15e5887b52
commit 9f67147e7b
21 changed files with 760 additions and 256 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs
+92 -32
View File
@@ -25,6 +25,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Transport;
using ZB.MOM.WW.ScadaBridge.Commons.Messages.Management;
using ZB.MOM.WW.ScadaBridge.Commons.Messages.RemoteQuery;
using ZB.MOM.WW.ScadaBridge.Commons.Types.Audit;
using ZB.MOM.WW.ScadaBridge.Commons.Types.Deployment;
using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums;
using ZB.MOM.WW.ScadaBridge.Commons.Types.InboundApi;
using ZB.MOM.WW.ScadaBridge.Commons.Types.Transport;
@@ -2558,53 +2559,112 @@ public class ManagementActor : ReceiveActor
if (sp.GetService<DeploymentService>() is { } deploymentService)
await deploymentService.TryPurgeTerminalDeploymentRecordsAsync();
// Page and Status are honoured DB-side. Both arrived on the command
// contract (and the CLI has been sending --page / --page-size / --status
// all along) but the handler previously ignored all three and returned the
// whole table.
// Page, PageSize and Status are all honoured DB-side, through the SAME
// paged read the Central UI deployment page uses (register row 37 — there
// used to be a second, subtly different repository query behind this
// handler alone).
var status = ParseDeploymentStatus(cmd.Status);
var skip = Math.Max(0, (Math.Max(1, cmd.Page) - 1) * Math.Max(1, cmd.PageSize));
var take = cmd.PageSize > 0 ? cmd.PageSize : (int?)null;
var page = Math.Max(1, cmd.Page);
var pageSize = ResolveDeploymentPageSize(cmd.PageSize);
// Instance-scoped query: enforce against the target instance's site
// before reading its deployment history.
if (cmd.InstanceId.HasValue)
{
await EnforceSiteScopeForInstance(sp, user, cmd.InstanceId.Value);
return await repo.QueryDeploymentSummariesAsync(
cmd.InstanceId.Value, status, instanceIdScope: null, skip, take);
return await QueryDeploymentPage(
repo, new DeploymentListFilter(status, InstanceId: cmd.InstanceId.Value), page, pageSize);
}
// Unfiltered query: a site-scoped Deployment user must only see records
// for instances at sites within their scope. DeploymentRecord has no
// SiteId, so the scope is resolved to the set of in-scope instance ids and
// pushed into the query as an id filter — rather than loading every record
// AND every instance and intersecting them in memory, which made an
// unfiltered listing cost two full-table loads regardless of page size.
if (user.PermittedSiteIds.Length == 0 || user.Roles.Contains(Roles.Administrator, StringComparer.OrdinalIgnoreCase))
return await repo.QueryDeploymentSummariesAsync(null, status, instanceIdScope: null, skip, take);
var permittedIds = new HashSet<string>(user.PermittedSiteIds);
var templateRepo = sp.GetRequiredService<ITemplateEngineRepository>();
var allInstances = await templateRepo.GetAllInstancesAsync();
var scopedInstanceIds = allInstances
.Where(i => permittedIds.Contains(i.SiteId.ToString()))
.Select(i => i.Id)
.ToArray();
return await repo.QueryDeploymentSummariesAsync(null, status, scopedInstanceIds, skip, take);
// SiteId of its own, so the scope rides as a SITE-ID filter resolved by the
// query's own join to Instance. It used to be resolved here instead, by
// loading EVERY instance in the system on every scoped query to turn the
// site grant into an instance-id list (register row 87).
var siteScope = ResolveSiteIdScope(user);
return await QueryDeploymentPage(
repo, new DeploymentListFilter(status, SiteIdScope: siteScope), page, pageSize);
}
/// <summary>
/// Parses the wire <c>Status</c> filter into a <see cref="DeploymentStatus"/>.
/// An unparseable value is treated as "no filter" rather than an error, which
/// preserves the previous handler's behaviour of ignoring the field entirely.
/// Runs the shared paged deployment read and wraps it in the command's result
/// envelope, echoing the page coordinates that were actually applied.
/// </summary>
private static DeploymentStatus? ParseDeploymentStatus(string? status) =>
!string.IsNullOrWhiteSpace(status) && Enum.TryParse<DeploymentStatus>(status, ignoreCase: true, out var parsed)
? parsed
: null;
private static async Task<DeploymentQueryResult> QueryDeploymentPage(
IDeploymentManagerRepository repo, DeploymentListFilter filter, int page, int pageSize)
{
var result = await repo.QueryDeploymentListPageAsync(filter, page, pageSize);
return new DeploymentQueryResult(result.Rows, result.TotalCount, page, pageSize);
}
/// <summary>
/// Resolves the caller's site-scope grant into the query's
/// <see cref="DeploymentListFilter.SiteIdScope"/>: <see langword="null"/> for a
/// system-wide caller, otherwise the permitted site ids.
/// </summary>
/// <remarks>
/// Administrator is system-wide here, matching <see cref="EnforceSiteScope"/>
/// and the rest of this actor. A permitted-site id that is not an integer is
/// DROPPED rather than ignored: dropping can only narrow the grant, whereas
/// treating the whole grant as unparseable would widen it to fleet-wide. A
/// grant of nothing but garbage therefore yields an EMPTY scope, which the
/// filter treats as a real "sees nothing" constraint.
/// </remarks>
private static IReadOnlyCollection<int>? ResolveSiteIdScope(AuthenticatedUser user)
{
if (user.PermittedSiteIds.Length == 0) return null;
if (user.Roles.Contains(Roles.Administrator, StringComparer.OrdinalIgnoreCase)) return null;
var ids = new List<int>(user.PermittedSiteIds.Length);
foreach (var raw in user.PermittedSiteIds)
{
if (int.TryParse(raw, out var id))
ids.Add(id);
}
return ids;
}
/// <summary>
/// Bounds the requested deployment page size: a non-positive value means
/// "unspecified" and takes the default, and every value is capped at
/// <see cref="DeploymentListPage.MaxPageSize"/>.
/// </summary>
/// <remarks>
/// Register row 87. A non-positive size used to mean UNLIMITED all the way down
/// to the repository, so <c>--page-size 0</c> materialized the entire
/// terminal-record retention window into one response. The result envelope
/// carries the total count precisely so bounding the page does not hide rows.
/// </remarks>
private static int ResolveDeploymentPageSize(int requested) =>
requested <= 0
? DeploymentListPage.DefaultPageSize
: Math.Min(requested, DeploymentListPage.MaxPageSize);
/// <summary>
/// Parses the wire <c>Status</c> filter into a <see cref="DeploymentStatus"/>.
/// Absent/blank means "no filter"; an unrecognised value is an ERROR naming the
/// valid ones.
/// </summary>
/// <remarks>
/// Register row 88. An unparseable value used to fall back to "no filter", so
/// <c>--status Sucess</c> silently returned every status — the caller saw a
/// plausible, complete-looking result that did not answer the question they
/// asked. Same loud-failure principle as row 46.
/// </remarks>
/// <exception cref="ManagementCommandException">The value is not a deployment status.</exception>
private static DeploymentStatus? ParseDeploymentStatus(string? status)
{
if (string.IsNullOrWhiteSpace(status)) return null;
if (Enum.TryParse<DeploymentStatus>(status, ignoreCase: true, out var parsed)
&& Enum.IsDefined(parsed))
{
return parsed;
}
throw new ManagementCommandException(
$"Unknown deployment status '{status}'. Valid values: {string.Join(", ", Enum.GetNames<DeploymentStatus>())}.");
}
// ========================================================================
// Audit Log handler