diff --git a/code-reviews/ManagementService/findings.md b/code-reviews/ManagementService/findings.md index d744536..523e033 100644 --- a/code-reviews/ManagementService/findings.md +++ b/code-reviews/ManagementService/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 10 | +| Open findings | 5 | ## Summary @@ -171,7 +171,7 @@ tests: `IsInstanceAccessAllowed_SiteScopedUser_OutOfScopeInstance_Denied`, |--|--| | Severity | Medium | | Category | Akka.NET conventions | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ManagementService/ManagementActor.cs:61` | **Description** @@ -196,7 +196,14 @@ that explicit with a router/dispatcher rather than ad-hoc `Task.Run`. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Confirmed: `HandleEnvelope` ran every command via +`Task.Run` and replied from inside the continuation, contrary to the project's PipeTo +convention. Replaced it with a `ProcessCommand` method returning a `Task` and +`PipeTo(sender, success, failure)`; faults are now mapped uniformly in a `MapFault` failure +continuation (`SiteScopeViolationException` -> `ManagementUnauthorized`, otherwise +`ManagementError`), which also unwraps `AggregateException`. Regression test: +`UnknownCommandType_FaultMappedToManagementError`. Existing success/error/unauthorized +mapping tests confirm behaviour is preserved. ### ManagementService-005 — ManagementActor declares no supervision strategy @@ -231,7 +238,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Performance & resource management | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ManagementService/ManagementEndpoints.cs:83`, `:112` | **Description** @@ -250,7 +257,16 @@ object, or restructure so the fallback path does not parse a throwaway document. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Confirmed: the request `JsonDocument` was never +disposed and the empty-payload path allocated a second throwaway `JsonDocument`. Extracted +request parsing into a testable `ManagementEndpoints.ParseCommand` helper that wraps the +document in `using`; the missing-payload case now deserializes from the `"{}"` literal +string rather than parsing a throwaway document. Regression tests: +`ParseCommand_WithExplicitPayload_DeserializesIntoCommandType`, +`ParseCommand_WithMissingPayload_DeserializesParameterlessCommand`, +`ParseCommand_WithInvalidJson_ReturnsFailure`, +`ParseCommand_WithMissingCommandField_ReturnsFailure`, +`ParseCommand_WithUnknownCommand_ReturnsFailure`. ### ManagementService-007 — Inconsistent and cycle-prone serialization of repository entities @@ -258,7 +274,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Code organization & conventions | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ManagementService/ManagementActor.cs:67`; `src/ScadaLink.ManagementService/ManagementEndpoints.cs:113` | **Description** @@ -281,7 +297,14 @@ correctly. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Confirmed: the actor serialized results with +`Newtonsoft.Json` (not even a direct package reference) while the HTTP endpoint uses +`System.Text.Json`. Standardised the actor on `System.Text.Json` via a new +`ManagementActor.SerializeResult` helper using a shared `JsonSerializerOptions` with +`ReferenceHandler.IgnoreCycles` (cycle-safe for EF entity graphs) and camelCase naming +(matches the CLI's case-insensitive deserializer). Removed the `Newtonsoft.Json` import. +Regression tests: `SerializeResult_WithCyclicGraph_DoesNotThrow`, +`SerializeResult_UsesCamelCasePropertyNames`. ### ManagementService-008 — HandleResolveRoles constructs RoleMapper manually instead of via DI @@ -312,9 +335,9 @@ _Unresolved._ | | | |--|--| -| Severity | Medium | -| Category | Error handling & resilience | -| Status | Open | +| Severity | Low — re-triaged from Medium; the claimed audit gap does not exist (see Description), leaving only an undocumented-convention issue. | +| Category | Code organization & conventions | +| Status | Resolved | | Location | `src/ScadaLink.ManagementService/ManagementActor.cs:357`, `:1134`, `:1085`, `:526`, `:1275` | **Description** @@ -325,21 +348,36 @@ external-system/notification/security/area mutations), but the handlers that del domain service do **not** — `HandleCreateTemplate`/`HandleUpdateTemplate`/`HandleDeleteTemplate`, all template-member handlers (`HandleAddAttribute` ... `HandleDeleteComposition`), template-folder handlers, shared-script handlers, `HandleDeployArtifacts`, `HandleDeployInstance`, -`HandleEnableInstance`/`Disable`/`Delete`, and the instance-binding/override handlers. This is -correct only if every one of those services performs its own audit logging internally; the -mixed pattern makes that impossible to verify by reading this module and creates a real risk -of silent audit gaps for template authoring and deployment operations. +`HandleEnableInstance`/`Disable`/`Delete`, and the instance-binding/override handlers. + +**Re-triage (2026-05-16):** the original finding claimed this "creates a real risk of silent +audit gaps for template authoring and deployment operations." That claim was verified against +the actual sources and is **false**. Every domain service the delegating handlers call — +`TemplateService`, `SharedScriptService`, `InstanceService`, `AreaService`, `SiteService`, +`TemplateFolderService`, `DeploymentService`, `ArtifactDeploymentService` — injects +`IAuditService` and calls `LogAsync` on every mutation (`grep` confirms an `_auditService.LogAsync` +call after each `Create`/`Update`/`Delete` in `TemplateService.cs`, `DeploymentService.cs`, +`ArtifactDeploymentService.cs`, etc.). There is therefore no audit gap; if anything, adding +explicit `AuditAsync` to a delegating handler would *double-log*. The genuine issue is purely +organizational: the two-layer split (actor audits repo-direct mutations, services audit their +own) was undocumented, which is what made it look risky. This is a Low-severity +code-organization issue, not a Medium error-handling/resilience defect. **Recommendation** -Decide on one layer that owns auditing. Either route all mutations through services that audit -internally (and remove the explicit `AuditAsync` calls here), or audit uniformly in the actor -after every successful mutation. Document the chosen contract so the inconsistency cannot -recur, and confirm template/deployment services actually audit. +Document the chosen contract so the split cannot be misread as a gap. (The original +alternative — moving all auditing into the actor — would require un-auditing eight services +and is not warranted given they already audit correctly.) **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Re-triaged to Low / Code organization after verifying +all eight delegated-to services audit internally — no audit gap exists. Documented the +two-layer audit contract in an XML `` block on `ManagementActor.AuditAsync`: +repository-direct mutations call `AuditAsync`; service-delegating handlers must not, because +the services own auditing and a duplicate call would double-log. No behavioural change, so +no new regression test; existing `CreateInstanceCommand_WithDeploymentRole_ReturnsSuccess` +covers the explicit-audit path. ### ManagementService-010 — ManagementServiceOptions.CommandTimeout is defined but never used @@ -433,7 +471,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Testing coverage | -| Status | Open | +| Status | Resolved | | Location | `tests/ScadaLink.ManagementService.Tests/ManagementActorTests.cs:1` | **Description** @@ -457,4 +495,14 @@ malformed bodies, unknown commands, and the 200/400/403/401/504 mappings. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). The site-scope and `DebugStreamHub` coverage gaps +were closed by the resolution of findings 001/002/003 (the `ScopedEnvelope` helper plus the +`*_OutOfScopeForSiteScopedUser_ReturnsUnauthorized` tests and `DebugStreamHubTests`). The +remaining HTTP-endpoint gap is now covered by a new `ManagementEndpointsTests.cs` exercising +`ManagementEndpoints.ParseCommand` — command deserialization, malformed JSON, missing +`command` field, and unknown commands. Full `WebApplicationFactory` auth-flow tests were +deliberately not added: `ManagementEndpoints` depends on `LdapAuthService` and live LDAP +infrastructure, so the testable command-parsing/dispatch logic was extracted into the pure +`ParseCommand` helper and covered instead. Tests: `ParseCommand_*` (5), +`SerializeResult_*` (2), `UnknownCommandType_FaultMappedToManagementError`, plus the +pre-existing site-scope and DebugStreamHub suites. `dotnet test` -> 48 passed. diff --git a/src/ScadaLink.ManagementService/ManagementActor.cs b/src/ScadaLink.ManagementService/ManagementActor.cs index 12c7f1e..bd2f129 100644 --- a/src/ScadaLink.ManagementService/ManagementActor.cs +++ b/src/ScadaLink.ManagementService/ManagementActor.cs @@ -1,6 +1,7 @@ using System.Security.Cryptography; +using System.Text.Json; +using System.Text.Json.Serialization; using Akka.Actor; -using Newtonsoft.Json; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using ScadaLink.Commons.Entities.ExternalSystems; @@ -42,6 +43,25 @@ public class ManagementActor : ReceiveActor Receive(HandleEnvelope); } + /// + /// Serializer settings for command results. + /// keeps EF-backed entity graphs with bidirectional navigation properties from throwing; + /// camelCase matches what the CLI / HTTP layer expect (finding ManagementService-007). + /// + private static readonly JsonSerializerOptions ResultSerializerOptions = new() + { + ReferenceHandler = ReferenceHandler.IgnoreCycles, + PropertyNamingPolicy = JsonNamingPolicy.CamelCase, + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull + }; + + /// + /// Serializes a command result to JSON using — the same + /// serializer the HTTP endpoint uses — with cycle-safe settings. + /// + public static string SerializeResult(object? result) => + JsonSerializer.Serialize(result, ResultSerializerOptions); + private void HandleEnvelope(ManagementEnvelope envelope) { var sender = Sender; @@ -57,27 +77,42 @@ public class ManagementActor : ReceiveActor return; } - // Process command asynchronously with scoped DI - Task.Run(async () => - { - using var scope = _serviceProvider.CreateScope(); - try - { - var result = await DispatchCommand(scope.ServiceProvider, envelope.Command, user); - var json = JsonConvert.SerializeObject(result, Formatting.None); - sender.Tell(new ManagementSuccess(correlationId, json)); - } - catch (SiteScopeViolationException ex) - { - sender.Tell(new ManagementUnauthorized(correlationId, ex.Message)); - } - catch (Exception ex) - { - _logger.LogError(ex, "Management command {Command} failed (CorrelationId={CorrelationId})", - envelope.Command.GetType().Name, correlationId); - sender.Tell(new ManagementError(correlationId, ex.Message, "COMMAND_FAILED")); - } - }); + // Process the command and pipe the mapped result back to the captured sender. + // PipeTo (rather than Task.Run + Tell-from-continuation) is the project's Akka.NET + // convention: faults are mapped in the failure continuation, no actor state is + // captured in the closure, and synchronous faults in command setup are still mapped. + ProcessCommand(envelope, user) + .PipeTo(sender, + success: result => result, + failure: ex => MapFault(ex, correlationId, envelope.Command)); + } + + /// + /// Runs a command on a scoped service provider and maps the result to a management + /// response message. Returns a faulted task on error so the PipeTo failure + /// continuation maps it uniformly. + /// + private async Task ProcessCommand(ManagementEnvelope envelope, AuthenticatedUser user) + { + using var scope = _serviceProvider.CreateScope(); + var result = await DispatchCommand(scope.ServiceProvider, envelope.Command, user); + return new ManagementSuccess(envelope.CorrelationId, SerializeResult(result)); + } + + /// + /// Maps an exception from command processing to the appropriate management response. + /// + private object MapFault(Exception ex, string correlationId, object command) + { + // PipeTo wraps continuation exceptions; unwrap to find the real cause. + var cause = ex is AggregateException agg ? agg.Flatten().InnerException ?? ex : ex; + + if (cause is SiteScopeViolationException scope) + return new ManagementUnauthorized(correlationId, scope.Message); + + _logger.LogError(cause, "Management command {Command} failed (CorrelationId={CorrelationId})", + command.GetType().Name, correlationId); + return new ManagementError(correlationId, cause.Message, "COMMAND_FAILED"); } private static string? GetRequiredRole(object command) => command switch @@ -345,8 +380,23 @@ public class ManagementActor : ReceiveActor } /// - /// Helper to log an audit entry after a successful mutation. + /// Logs an audit entry after a successful mutation. /// + /// + /// Audit-logging contract (finding ManagementService-009). Every mutating operation is + /// audited exactly once, by whichever layer owns the write: + /// + /// Handlers that mutate a repository directly (site, area, data-connection, + /// external-system, notification, security, API-key, scope-rule) call this helper + /// explicitly after the successful change. + /// Handlers that delegate to a domain service (TemplateService, + /// SharedScriptService, InstanceService, AreaService, + /// SiteService, TemplateFolderService, DeploymentService, + /// ArtifactDeploymentService) do NOT call this helper — those services own their + /// own dependency and audit internally. Adding an explicit + /// call in such a handler would double-log. + /// + /// private static async Task AuditAsync(IServiceProvider sp, string user, string action, string entityType, string entityId, string entityName, object? afterState) { var auditService = sp.GetRequiredService(); diff --git a/src/ScadaLink.ManagementService/ManagementEndpoints.cs b/src/ScadaLink.ManagementService/ManagementEndpoints.cs index 4f1c83e..06d2073 100644 --- a/src/ScadaLink.ManagementService/ManagementEndpoints.cs +++ b/src/ScadaLink.ManagementService/ManagementEndpoints.cs @@ -77,46 +77,19 @@ public static class ManagementEndpoints permittedSiteIds); // 4. Parse command from request body - JsonDocument doc; - try + string body; + using (var reader = new StreamReader(context.Request.Body, Encoding.UTF8)) { - doc = await JsonDocument.ParseAsync(context.Request.Body); - } - catch - { - return Results.Json(new { error = "Invalid JSON body.", code = "BAD_REQUEST" }, statusCode: 400); + body = await reader.ReadToEndAsync(); } - if (!doc.RootElement.TryGetProperty("command", out var commandNameElement)) + var parse = ParseCommand(body); + if (!parse.Success) { - return Results.Json(new { error = "Missing 'command' field.", code = "BAD_REQUEST" }, statusCode: 400); + return Results.Json(new { error = parse.ErrorMessage, code = parse.ErrorCode }, statusCode: 400); } - var commandName = commandNameElement.GetString(); - if (string.IsNullOrWhiteSpace(commandName)) - { - return Results.Json(new { error = "Empty 'command' field.", code = "BAD_REQUEST" }, statusCode: 400); - } - - var commandType = ManagementCommandRegistry.Resolve(commandName); - if (commandType == null) - { - return Results.Json(new { error = $"Unknown command: '{commandName}'.", code = "BAD_REQUEST" }, statusCode: 400); - } - - object command; - try - { - var payloadElement = doc.RootElement.TryGetProperty("payload", out var p) - ? p - : JsonDocument.Parse("{}").RootElement; - command = JsonSerializer.Deserialize(payloadElement.GetRawText(), commandType, - new JsonSerializerOptions { PropertyNameCaseInsensitive = true })!; - } - catch (Exception ex) - { - return Results.Json(new { error = $"Failed to deserialize payload: {ex.Message}", code = "BAD_REQUEST" }, statusCode: 400); - } + var command = parse.Command!; // 5. Dispatch to ManagementActor var holder = context.RequestServices.GetRequiredService(); @@ -148,4 +121,68 @@ public static class ManagementEndpoints _ => Results.Json(new { error = "Unexpected response.", code = "INTERNAL_ERROR" }, statusCode: 500) }; } + + /// + /// Result of parsing a management request body into a strongly-typed command. + /// + public readonly record struct CommandParseResult( + bool Success, object? Command, string? ErrorMessage, string? ErrorCode) + { + public static CommandParseResult Ok(object command) => new(true, command, null, null); + public static CommandParseResult Fail(string message) => new(false, null, message, "BAD_REQUEST"); + } + + /// + /// Parses a management request body — a JSON object with a command name and an + /// optional payload — into the strongly-typed command record. The parsed + /// is disposed deterministically and the missing-payload + /// case does not allocate a throwaway document (finding ManagementService-006). + /// + public static CommandParseResult ParseCommand(string body) + { + using JsonDocument doc = ParseDocument(body, out var parseError); + if (parseError != null) + return CommandParseResult.Fail(parseError); + + if (!doc.RootElement.TryGetProperty("command", out var commandNameElement)) + return CommandParseResult.Fail("Missing 'command' field."); + + var commandName = commandNameElement.GetString(); + if (string.IsNullOrWhiteSpace(commandName)) + return CommandParseResult.Fail("Empty 'command' field."); + + var commandType = ManagementCommandRegistry.Resolve(commandName); + if (commandType == null) + return CommandParseResult.Fail($"Unknown command: '{commandName}'."); + + try + { + // Missing payload: deserialize from the empty-object literal rather than + // allocating (and leaking) a throwaway JsonDocument. + var payloadJson = doc.RootElement.TryGetProperty("payload", out var p) + ? p.GetRawText() + : "{}"; + var command = JsonSerializer.Deserialize(payloadJson, commandType, + new JsonSerializerOptions { PropertyNameCaseInsensitive = true })!; + return CommandParseResult.Ok(command); + } + catch (Exception ex) + { + return CommandParseResult.Fail($"Failed to deserialize payload: {ex.Message}"); + } + } + + private static JsonDocument ParseDocument(string body, out string? error) + { + try + { + error = null; + return JsonDocument.Parse(body); + } + catch + { + error = "Invalid JSON body."; + return JsonDocument.Parse("{}"); + } + } } diff --git a/tests/ScadaLink.ManagementService.Tests/ManagementActorTests.cs b/tests/ScadaLink.ManagementService.Tests/ManagementActorTests.cs index 53df81e..fce9699 100644 --- a/tests/ScadaLink.ManagementService.Tests/ManagementActorTests.cs +++ b/tests/ScadaLink.ManagementService.Tests/ManagementActorTests.cs @@ -668,4 +668,66 @@ public class ManagementActorTests : TestKit, IDisposable var response = ExpectMsg(TimeSpan.FromSeconds(5)); Assert.Equal(envelope.CorrelationId, response.CorrelationId); } + + // ======================================================================== + // Serialization (finding ManagementService-007) + // + // Command results are serialized with System.Text.Json configured with + // ReferenceHandler.IgnoreCycles, so an entity graph with a bidirectional + // navigation property does not throw. Property names are camelCase, which + // the CLI's case-insensitive deserializer accepts. + // ======================================================================== + + private sealed class CyclicNode + { + public string Name { get; set; } = ""; + public CyclicNode? Parent { get; set; } + public List Children { get; set; } = new(); + } + + [Fact] + public void SerializeResult_WithCyclicGraph_DoesNotThrow() + { + var parent = new CyclicNode { Name = "Parent" }; + var child = new CyclicNode { Name = "Child", Parent = parent }; + parent.Children.Add(child); // parent <-> child cycle + + var json = ManagementActor.SerializeResult(parent); + + Assert.Contains("Parent", json); + Assert.Contains("Child", json); + } + + [Fact] + public void SerializeResult_UsesCamelCasePropertyNames() + { + var json = ManagementActor.SerializeResult(new CyclicNode { Name = "X" }); + + Assert.Contains("\"name\"", json); + Assert.DoesNotContain("\"Name\"", json); + } + + // ======================================================================== + // PipeTo fault mapping (finding ManagementService-004) + // + // Command processing is piped back via PipeTo; a fault raised inside + // DispatchCommand must be mapped to ManagementError by the failure + // continuation rather than escaping or being silently dropped. + // ======================================================================== + + [Fact] + public void UnknownCommandType_FaultMappedToManagementError() + { + // ManagementEnvelope.Command is typed object; an unrecognised payload + // makes DispatchCommand throw NotSupportedException. The PipeTo failure + // continuation must map it to ManagementError. + var actor = CreateActor(); + var envelope = Envelope("not-a-command"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Equal(envelope.CorrelationId, response.CorrelationId); + Assert.Equal("COMMAND_FAILED", response.ErrorCode); + } } diff --git a/tests/ScadaLink.ManagementService.Tests/ManagementEndpointsTests.cs b/tests/ScadaLink.ManagementService.Tests/ManagementEndpointsTests.cs new file mode 100644 index 0000000..9cca22f --- /dev/null +++ b/tests/ScadaLink.ManagementService.Tests/ManagementEndpointsTests.cs @@ -0,0 +1,65 @@ +using ScadaLink.Commons.Messages.Management; +using ScadaLink.ManagementService; + +namespace ScadaLink.ManagementService.Tests; + +/// +/// Tests for request-body parsing +/// (findings ManagementService-006 / -013). +/// +public class ManagementEndpointsTests +{ + [Fact] + public void ParseCommand_WithExplicitPayload_DeserializesIntoCommandType() + { + var json = """{ "command": "CreateSite", "payload": { "name": "Site1", "siteIdentifier": "SITE1", "description": "Desc" } }"""; + + var result = ManagementEndpoints.ParseCommand(json); + + Assert.True(result.Success); + var command = Assert.IsType(result.Command); + Assert.Equal("Site1", command.Name); + Assert.Equal("SITE1", command.SiteIdentifier); + Assert.Equal("Desc", command.Description); + } + + [Fact] + public void ParseCommand_WithMissingPayload_DeserializesParameterlessCommand() + { + // No "payload" field at all -- the fallback must not allocate a throwaway + // JsonDocument and must still produce a valid parameterless command. + var json = """{ "command": "ListTemplates" }"""; + + var result = ManagementEndpoints.ParseCommand(json); + + Assert.True(result.Success); + Assert.IsType(result.Command); + } + + [Fact] + public void ParseCommand_WithInvalidJson_ReturnsFailure() + { + var result = ManagementEndpoints.ParseCommand("{ not json"); + + Assert.False(result.Success); + Assert.Equal("BAD_REQUEST", result.ErrorCode); + } + + [Fact] + public void ParseCommand_WithMissingCommandField_ReturnsFailure() + { + var result = ManagementEndpoints.ParseCommand("""{ "payload": {} }"""); + + Assert.False(result.Success); + Assert.Equal("BAD_REQUEST", result.ErrorCode); + } + + [Fact] + public void ParseCommand_WithUnknownCommand_ReturnsFailure() + { + var result = ManagementEndpoints.ParseCommand("""{ "command": "NoSuchCommand" }"""); + + Assert.False(result.Success); + Assert.Equal("BAD_REQUEST", result.ErrorCode); + } +}