dohertj2/scadalink-design
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(management-service): resolve ManagementService-014..017 — site-scope enforcement on QueryDeployments, atomic override validation, curated fault messages, test coverage

Browse Source
This commit is contained in:
Joseph Doherty
2026-05-17 03:18:33 -04:00
parent 73a393076a
commit bf6bd8de5a
3 changed files with 447 additions and 64 deletions
Download Patch File Download Diff File Expand all files Collapse all files

56
code-reviews/ManagementService/findings.md
View File

@@ -8,7 +8,7 @@
| Last reviewed | 2026-05-17 |
| Reviewer | claude-agent |
| Commit reviewed | `39d737e` |
| Open findings | 4 (1 Deferred — see ManagementService-012) |
| Open findings | 0 (1 Deferred — see ManagementService-012) |
## Summary
@@ -576,7 +576,7 @@ pre-existing site-scope and DebugStreamHub suites. `dotnet test` -> 48 passed.
|--|--|
| Severity | High |
| Category | Security |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.ManagementService/ManagementActor.cs:306`, `:1174` |
**Description**
@@ -609,7 +609,20 @@ instances.
**Resolution**
_Unresolved._
Resolved 2026-05-17 (commit pending). **Re-triage:** the finding understated the gap — it
claimed `QueryDeploymentsCommand` was already "gated to the `Deployment` role by
`GetRequiredRole` (lines 170–177)". Verified against the source: `QueryDeploymentsCommand`
appeared nowhere in `GetRequiredRole`, so it required *no* role at all — any authenticated
user could read every deployment record system-wide. Fix applied both gates: added
`QueryDeploymentsCommand` to the `Deployment`-role group in `GetRequiredRole`, and threaded
`AuthenticatedUser` into `HandleQueryDeployments` — the `InstanceId` branch now calls
`EnforceSiteScopeForInstance`; the unfiltered branch resolves each record's instance to its
`SiteId` (cached) and drops records outside `PermittedSiteIds`, mirroring `HandleListInstances`.
Regression tests: `QueryDeployments_WithDesignRole_ReturnsUnauthorized`,
`QueryDeployments_FilteredByOutOfScopeInstance_ReturnsUnauthorized`,
`QueryDeployments_FilteredByInScopeInstance_ReturnsRecords`,
`QueryDeployments_UnfilteredForSiteScopedUser_DropsOutOfScopeRecords`,
`QueryDeployments_UnfilteredForAdminUser_ReturnsAllRecords`.
### ManagementService-015 — HandleSetInstanceOverrides applies overrides non-atomically
@@ -617,7 +630,7 @@ _Unresolved._
|--|--|
| Severity | Medium |
| Category | Error handling & resilience |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.ManagementService/ManagementActor.cs:647`–`:659` |
**Description**
@@ -644,7 +657,16 @@ can reconcile.
**Resolution**
_Unresolved._
Resolved 2026-05-17 (commit pending). Confirmed: each `SetAttributeOverrideAsync` call
commits independently, so a mid-batch failure left earlier overrides persisted.
`HandleSetInstanceOverrides` now validates every requested attribute up front against the
instance's template (exists, not locked) and only begins applying once the whole batch is
known valid — eliminating the realistic partial-mutation failure modes (unknown / locked
attribute). `InstanceService` is outside this module's edit scope and offers no batch/
transactional method, so a genuine database fault mid-apply remains theoretically possible;
this residual is documented in a code comment on the handler. Regression tests:
`SetInstanceOverrides_WithOneInvalidAttribute_PersistsNoOverrides`,
`SetInstanceOverrides_AllValid_PersistsAllOverrides`.
### ManagementService-016 — Unexpected exception messages returned verbatim to HTTP callers
@@ -652,7 +674,7 @@ _Unresolved._
|--|--|
| Severity | Low |
| Category | Security |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.ManagementService/ManagementActor.cs:121`–`:131` |
**Description**
@@ -679,7 +701,17 @@ can still correlate to the server log via the correlation ID.
**Resolution**
_Unresolved._
Resolved 2026-05-17 (commit pending). Added a `ManagementCommandException` type for curated,
caller-safe failures and converted every curated `throw` in `ManagementActor` (the 34
`result.Error` rethrows and 15 "not found" / delete-blocked messages) to it. `MapFault` now
returns the message verbatim only for `ManagementCommandException`; any other fault (DB
errors, `Enum.Parse` `ArgumentException`, `NullReferenceException`, the unknown-command
`NotSupportedException`, etc.) yields a generic `"An internal error occurred
(CorrelationId=...)"` string while the full exception is still logged server-side. Regression
tests: `UnexpectedFault_ReturnsGenericMessage_NotRawExceptionText`,
`CuratedHandlerFailure_SurfacesTheCuratedMessage`; the two pre-existing
`*_WhenRepoThrows_*` tests were updated to assert the raw repository message is no longer
leaked.
### ManagementService-017 — QueryDeploymentsCommand has no test coverage
@@ -687,7 +719,7 @@ _Unresolved._
|--|--|
| Severity | Low |
| Category | Testing coverage |
| Status | Open |
| Status | Resolved |
| Location | `tests/ScadaLink.ManagementService.Tests/ManagementActorTests.cs:1` |
**Description**
@@ -709,4 +741,10 @@ Deployment user against in-scope and out-of-scope deployment records.
**Resolution**
_Unresolved._
Resolved 2026-05-17 (commit pending). Added seven `QueryDeployments_*` tests to
`ManagementActorTests`: role gate (`_WithDesignRole_ReturnsUnauthorized`), the
`InstanceId`-filtered and unfiltered branches (`_FilteredByInstanceId_ReturnsInstanceRecords`,
`_UnfilteredWithDeploymentRole_ReturnsAllRecords`), and site-scope coverage for a site-scoped
Deployment user and an Admin user, in- and out-of-scope
(`_FilteredByOutOfScopeInstance_ReturnsUnauthorized`, `_FilteredByInScopeInstance_ReturnsRecords`,
`_UnfilteredForSiteScopedUser_DropsOutOfScopeRecords`, `_UnfilteredForAdminUser_ReturnsAllRecords`).