diff --git a/code-reviews/ManagementService/findings.md b/code-reviews/ManagementService/findings.md index 687cf5c..19d8fb3 100644 --- a/code-reviews/ManagementService/findings.md +++ b/code-reviews/ManagementService/findings.md @@ -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`). diff --git a/src/ScadaLink.ManagementService/ManagementActor.cs b/src/ScadaLink.ManagementService/ManagementActor.cs index e203a9a..22fd8b3 100644 --- a/src/ScadaLink.ManagementService/ManagementActor.cs +++ b/src/ScadaLink.ManagementService/ManagementActor.cs @@ -4,6 +4,7 @@ using System.Text.Json.Serialization; using Akka.Actor; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; +using ScadaLink.Commons.Entities.Deployment; using ScadaLink.Commons.Entities.ExternalSystems; using ScadaLink.Commons.Entities.InboundApi; using ScadaLink.Commons.Entities.Instances; @@ -128,7 +129,17 @@ public class ManagementActor : ReceiveActor _logger.LogError(cause, "Management command {Command} failed (CorrelationId={CorrelationId})", command.GetType().Name, correlationId); - return new ManagementError(correlationId, cause.Message, "COMMAND_FAILED"); + + // Curated handler failures (ManagementCommandException) carry a message + // that is safe to surface to the caller. Any other exception is an + // unanticipated fault whose raw .Message can disclose internal detail + // (server/database names, constraint names, stack context) — return a + // generic message and let the operator correlate via the server log + // using the correlation ID (finding ManagementService-016). + var clientMessage = cause is ManagementCommandException + ? cause.Message + : $"An internal error occurred (CorrelationId={correlationId})."; + return new ManagementError(correlationId, clientMessage, "COMMAND_FAILED"); } private static string? GetRequiredRole(object command) => command switch @@ -173,6 +184,7 @@ public class ManagementActor : ReceiveActor or SetInstanceAlarmOverrideCommand or DeleteInstanceAlarmOverrideCommand or GetDeploymentDiffCommand or MgmtDeployArtifactsCommand + or QueryDeploymentsCommand or RetryParkedMessageCommand or DiscardParkedMessageCommand or DebugSnapshotCommand => "Deployment", @@ -303,7 +315,7 @@ public class ManagementActor : ReceiveActor // Deployments MgmtDeployArtifactsCommand cmd => await HandleDeployArtifacts(sp, cmd, user.Username), - QueryDeploymentsCommand cmd => await HandleQueryDeployments(sp, cmd), + QueryDeploymentsCommand cmd => await HandleQueryDeployments(sp, cmd, user), GetDeploymentDiffCommand cmd => await HandleGetDeploymentDiff(sp, cmd, user), // Audit Log @@ -428,7 +440,7 @@ public class ManagementActor : ReceiveActor var result = await svc.CreateTemplateAsync(cmd.Name, cmd.Description, cmd.ParentTemplateId, user); return result.IsSuccess ? result.Value - : throw new InvalidOperationException(result.Error); + : throw new ManagementCommandException(result.Error); } private static async Task HandleUpdateTemplate(IServiceProvider sp, UpdateTemplateCommand cmd, string user) @@ -437,7 +449,7 @@ public class ManagementActor : ReceiveActor var result = await svc.UpdateTemplateAsync(cmd.TemplateId, cmd.Name, cmd.Description, cmd.ParentTemplateId, user); return result.IsSuccess ? result.Value - : throw new InvalidOperationException(result.Error); + : throw new ManagementCommandException(result.Error); } private static async Task HandleDeleteTemplate(IServiceProvider sp, DeleteTemplateCommand cmd, string user) @@ -446,7 +458,7 @@ public class ManagementActor : ReceiveActor var result = await svc.DeleteTemplateAsync(cmd.TemplateId, user); return result.IsSuccess ? result.Value - : throw new InvalidOperationException(result.Error); + : throw new ManagementCommandException(result.Error); } private static async Task HandleValidateTemplate(IServiceProvider sp, ValidateTemplateCommand cmd) @@ -455,7 +467,7 @@ public class ManagementActor : ReceiveActor // Load the template with all members var template = await repo.GetTemplateWithChildrenAsync(cmd.TemplateId) - ?? throw new InvalidOperationException($"Template with ID {cmd.TemplateId} not found."); + ?? throw new ManagementCommandException($"Template with ID {cmd.TemplateId} not found."); var attributes = await repo.GetAttributesByTemplateIdAsync(cmd.TemplateId); var alarms = await repo.GetAlarmsByTemplateIdAsync(cmd.TemplateId); @@ -527,35 +539,35 @@ public class ManagementActor : ReceiveActor { var svc = sp.GetRequiredService(); var result = await svc.CreateFolderAsync(cmd.Name, cmd.ParentFolderId, user); - return result.IsSuccess ? result.Value : throw new InvalidOperationException(result.Error); + return result.IsSuccess ? result.Value : throw new ManagementCommandException(result.Error); } private static async Task HandleRenameTemplateFolder(IServiceProvider sp, RenameTemplateFolderCommand cmd, string user) { var svc = sp.GetRequiredService(); var result = await svc.RenameFolderAsync(cmd.FolderId, cmd.NewName, user); - return result.IsSuccess ? result.Value : throw new InvalidOperationException(result.Error); + return result.IsSuccess ? result.Value : throw new ManagementCommandException(result.Error); } private static async Task HandleMoveTemplateFolder(IServiceProvider sp, MoveTemplateFolderCommand cmd, string user) { var svc = sp.GetRequiredService(); var result = await svc.MoveFolderAsync(cmd.FolderId, cmd.NewParentFolderId, user); - return result.IsSuccess ? result.Value : throw new InvalidOperationException(result.Error); + return result.IsSuccess ? result.Value : throw new ManagementCommandException(result.Error); } private static async Task HandleDeleteTemplateFolder(IServiceProvider sp, DeleteTemplateFolderCommand cmd, string user) { var svc = sp.GetRequiredService(); var result = await svc.DeleteFolderAsync(cmd.FolderId, user); - return result.IsSuccess ? result.Value : throw new InvalidOperationException(result.Error); + return result.IsSuccess ? result.Value : throw new ManagementCommandException(result.Error); } private static async Task HandleMoveTemplateToFolder(IServiceProvider sp, MoveTemplateToFolderCommand cmd, string user) { var svc = sp.GetRequiredService(); var result = await svc.MoveTemplateAsync(cmd.TemplateId, cmd.NewFolderId, user); - return result.IsSuccess ? result.Value : throw new InvalidOperationException(result.Error); + return result.IsSuccess ? result.Value : throw new ManagementCommandException(result.Error); } // ======================================================================== @@ -589,7 +601,7 @@ public class ManagementActor : ReceiveActor EnforceSiteScope(user, cmd.SiteId); var svc = sp.GetRequiredService(); var result = await svc.CreateInstanceAsync(cmd.UniqueName, cmd.TemplateId, cmd.SiteId, cmd.AreaId, user.Username); - if (!result.IsSuccess) throw new InvalidOperationException(result.Error); + if (!result.IsSuccess) throw new ManagementCommandException(result.Error); await AuditAsync(sp, user.Username, "Create", "Instance", result.Value.Id.ToString(), result.Value.UniqueName, result.Value); return result.Value; } @@ -601,7 +613,7 @@ public class ManagementActor : ReceiveActor var result = await svc.DeployInstanceAsync(cmd.InstanceId, user.Username); return result.IsSuccess ? result.Value - : throw new InvalidOperationException(result.Error); + : throw new ManagementCommandException(result.Error); } private static async Task HandleEnableInstance(IServiceProvider sp, MgmtEnableInstanceCommand cmd, AuthenticatedUser user) @@ -611,7 +623,7 @@ public class ManagementActor : ReceiveActor var result = await svc.EnableInstanceAsync(cmd.InstanceId, user.Username); return result.IsSuccess ? result.Value - : throw new InvalidOperationException(result.Error); + : throw new ManagementCommandException(result.Error); } private static async Task HandleDisableInstance(IServiceProvider sp, MgmtDisableInstanceCommand cmd, AuthenticatedUser user) @@ -621,7 +633,7 @@ public class ManagementActor : ReceiveActor var result = await svc.DisableInstanceAsync(cmd.InstanceId, user.Username); return result.IsSuccess ? result.Value - : throw new InvalidOperationException(result.Error); + : throw new ManagementCommandException(result.Error); } private static async Task HandleDeleteInstance(IServiceProvider sp, MgmtDeleteInstanceCommand cmd, AuthenticatedUser user) @@ -631,7 +643,7 @@ public class ManagementActor : ReceiveActor var result = await svc.DeleteInstanceAsync(cmd.InstanceId, user.Username); return result.IsSuccess ? result.Value - : throw new InvalidOperationException(result.Error); + : throw new ManagementCommandException(result.Error); } private static async Task HandleSetConnectionBindings(IServiceProvider sp, SetConnectionBindingsCommand cmd, AuthenticatedUser user) @@ -641,18 +653,45 @@ public class ManagementActor : ReceiveActor var result = await svc.SetConnectionBindingsAsync(cmd.InstanceId, cmd.Bindings, user.Username); return result.IsSuccess ? result.Value - : throw new InvalidOperationException(result.Error); + : throw new ManagementCommandException(result.Error); } private static async Task HandleSetInstanceOverrides(IServiceProvider sp, SetInstanceOverridesCommand cmd, AuthenticatedUser user) { await EnforceSiteScopeForInstance(sp, user, cmd.InstanceId); + + // Multi-override apply is all-or-nothing (finding ManagementService-015). + // InstanceService.SetAttributeOverrideAsync commits each override + // independently, so a mid-batch failure on an invalid attribute would + // otherwise leave the instance partially mutated. Validate every + // requested attribute up front against the instance's template; only + // apply once the whole batch is known to be valid. (A genuine database + // fault mid-apply remains theoretically possible without a shared + // transaction, but the realistic failure modes — unknown or locked + // attribute — are now rejected before any write.) + var repo = sp.GetRequiredService(); + var instance = await repo.GetInstanceByIdAsync(cmd.InstanceId) + ?? throw new ManagementCommandException($"Instance with ID {cmd.InstanceId} not found."); + + var templateAttrs = await repo.GetAttributesByTemplateIdAsync(instance.TemplateId); + var attrsByName = templateAttrs.ToDictionary(a => a.Name); + foreach (var attrName in cmd.Overrides.Keys) + { + if (!attrsByName.TryGetValue(attrName, out var templateAttr)) + throw new ManagementCommandException( + $"Attribute '{attrName}' does not exist in template {instance.TemplateId}. " + + "No overrides were applied."); + if (templateAttr.IsLocked) + throw new ManagementCommandException( + $"Attribute '{attrName}' is locked and cannot be overridden. No overrides were applied."); + } + var svc = sp.GetRequiredService(); var results = new List(); foreach (var (attrName, overrideValue) in cmd.Overrides) { var result = await svc.SetAttributeOverrideAsync(cmd.InstanceId, attrName, overrideValue, user.Username); - if (!result.IsSuccess) throw new InvalidOperationException(result.Error); + if (!result.IsSuccess) throw new ManagementCommandException(result.Error); results.Add(result.Value); } return results; @@ -665,7 +704,7 @@ public class ManagementActor : ReceiveActor var result = await svc.AssignToAreaAsync(cmd.InstanceId, cmd.AreaId, user.Username); return result.IsSuccess ? result.Value - : throw new InvalidOperationException(result.Error); + : throw new ManagementCommandException(result.Error); } private static async Task HandleSetInstanceAlarmOverride(IServiceProvider sp, SetInstanceAlarmOverrideCommand cmd, AuthenticatedUser user) @@ -678,7 +717,7 @@ public class ManagementActor : ReceiveActor user.Username); return result.IsSuccess ? result.Value - : throw new InvalidOperationException(result.Error); + : throw new ManagementCommandException(result.Error); } private static async Task HandleDeleteInstanceAlarmOverride(IServiceProvider sp, DeleteInstanceAlarmOverrideCommand cmd, AuthenticatedUser user) @@ -689,7 +728,7 @@ public class ManagementActor : ReceiveActor cmd.InstanceId, cmd.AlarmCanonicalName, user.Username); return result.IsSuccess ? result.Value - : throw new InvalidOperationException(result.Error); + : throw new ManagementCommandException(result.Error); } private static async Task HandleListInstanceAlarmOverrides(IServiceProvider sp, ListInstanceAlarmOverridesCommand cmd, AuthenticatedUser user) @@ -706,7 +745,7 @@ public class ManagementActor : ReceiveActor var result = await svc.GetDeploymentComparisonAsync(cmd.InstanceId); return result.IsSuccess ? result.Value - : throw new InvalidOperationException(result.Error); + : throw new ManagementCommandException(result.Error); } private static async Task HandleRetryParkedMessage(IServiceProvider sp, RetryParkedMessageCommand cmd, AuthenticatedUser user) @@ -775,7 +814,7 @@ public class ManagementActor : ReceiveActor { var repo = sp.GetRequiredService(); var site = await repo.GetSiteByIdAsync(cmd.SiteId) - ?? throw new InvalidOperationException($"Site with ID {cmd.SiteId} not found."); + ?? throw new ManagementCommandException($"Site with ID {cmd.SiteId} not found."); site.Name = cmd.Name; site.Description = cmd.Description; site.NodeAAddress = cmd.NodeAAddress; @@ -796,7 +835,7 @@ public class ManagementActor : ReceiveActor var site = await repo.GetSiteByIdAsync(cmd.SiteId); var instances = await repo.GetInstancesBySiteIdAsync(cmd.SiteId); if (instances.Count > 0) - throw new InvalidOperationException( + throw new ManagementCommandException( $"Cannot delete site {cmd.SiteId}: it has {instances.Count} instance(s)."); await repo.DeleteSiteAsync(cmd.SiteId); await repo.SaveChangesAsync(); @@ -876,7 +915,7 @@ public class ManagementActor : ReceiveActor { var repo = sp.GetRequiredService(); var conn = await repo.GetDataConnectionByIdAsync(cmd.DataConnectionId) - ?? throw new InvalidOperationException($"DataConnection with ID {cmd.DataConnectionId} not found."); + ?? throw new ManagementCommandException($"DataConnection with ID {cmd.DataConnectionId} not found."); conn.Name = cmd.Name; conn.Protocol = cmd.Protocol; conn.PrimaryConfiguration = cmd.PrimaryConfiguration; @@ -931,7 +970,7 @@ public class ManagementActor : ReceiveActor { var repo = sp.GetRequiredService(); var def = await repo.GetExternalSystemByIdAsync(cmd.ExternalSystemId) - ?? throw new InvalidOperationException($"ExternalSystem with ID {cmd.ExternalSystemId} not found."); + ?? throw new ManagementCommandException($"ExternalSystem with ID {cmd.ExternalSystemId} not found."); def.Name = cmd.Name; def.EndpointUrl = cmd.EndpointUrl; def.AuthType = cmd.AuthType; @@ -982,7 +1021,7 @@ public class ManagementActor : ReceiveActor { var repo = sp.GetRequiredService(); var method = await repo.GetExternalSystemMethodByIdAsync(cmd.MethodId) - ?? throw new InvalidOperationException($"ExternalSystemMethod with ID {cmd.MethodId} not found."); + ?? throw new ManagementCommandException($"ExternalSystemMethod with ID {cmd.MethodId} not found."); if (cmd.Name != null) method.Name = cmd.Name; if (cmd.HttpMethod != null) method.HttpMethod = cmd.HttpMethod; if (cmd.Path != null) method.Path = cmd.Path; @@ -1037,7 +1076,7 @@ public class ManagementActor : ReceiveActor { var repo = sp.GetRequiredService(); var list = await repo.GetNotificationListByIdAsync(cmd.NotificationListId) - ?? throw new InvalidOperationException($"NotificationList with ID {cmd.NotificationListId} not found."); + ?? throw new ManagementCommandException($"NotificationList with ID {cmd.NotificationListId} not found."); list.Name = cmd.Name; var existingRecipients = await repo.GetRecipientsByListIdAsync(cmd.NotificationListId); @@ -1079,7 +1118,7 @@ public class ManagementActor : ReceiveActor { var repo = sp.GetRequiredService(); var config = await repo.GetSmtpConfigurationByIdAsync(cmd.SmtpConfigId) - ?? throw new InvalidOperationException($"SmtpConfiguration with ID {cmd.SmtpConfigId} not found."); + ?? throw new ManagementCommandException($"SmtpConfiguration with ID {cmd.SmtpConfigId} not found."); config.Host = cmd.Server; config.Port = cmd.Port; config.AuthType = cmd.AuthMode; @@ -1114,7 +1153,7 @@ public class ManagementActor : ReceiveActor { var repo = sp.GetRequiredService(); var mapping = await repo.GetMappingByIdAsync(cmd.MappingId) - ?? throw new InvalidOperationException($"RoleMapping with ID {cmd.MappingId} not found."); + ?? throw new ManagementCommandException($"RoleMapping with ID {cmd.MappingId} not found."); mapping.LdapGroupName = cmd.LdapGroupName; mapping.Role = cmd.Role; await repo.UpdateMappingAsync(mapping); @@ -1168,15 +1207,45 @@ public class ManagementActor : ReceiveActor var result = await svc.DeployToAllSitesAsync(user); return result.IsSuccess ? result.Value - : throw new InvalidOperationException(result.Error); + : throw new ManagementCommandException(result.Error); } - private static async Task HandleQueryDeployments(IServiceProvider sp, QueryDeploymentsCommand cmd) + private static async Task HandleQueryDeployments(IServiceProvider sp, QueryDeploymentsCommand cmd, AuthenticatedUser user) { var repo = sp.GetRequiredService(); + + // Instance-scoped query: enforce against the target instance's site + // before reading its deployment history (finding ManagementService-014). if (cmd.InstanceId.HasValue) + { + await EnforceSiteScopeForInstance(sp, user, cmd.InstanceId.Value); return await repo.GetDeploymentsByInstanceIdAsync(cmd.InstanceId.Value); - return await repo.GetAllDeploymentRecordsAsync(); + } + + // Unfiltered query: a site-scoped Deployment user must only see records + // for instances at sites within their scope. DeploymentRecord has no + // SiteId, so resolve each record's instance to its site and filter + // (mirrors the HandleListInstances / HandleListSites filter pattern). + var records = await repo.GetAllDeploymentRecordsAsync(); + if (user.PermittedSiteIds.Length == 0 || user.Roles.Contains("Admin", StringComparer.OrdinalIgnoreCase)) + return records; + + var permittedIds = new HashSet(user.PermittedSiteIds); + var templateRepo = sp.GetRequiredService(); + var instanceSiteCache = new Dictionary(); + var scoped = new List(); + foreach (var record in records) + { + if (!instanceSiteCache.TryGetValue(record.InstanceId, out var siteId)) + { + var instance = await templateRepo.GetInstanceByIdAsync(record.InstanceId); + siteId = instance?.SiteId; + instanceSiteCache[record.InstanceId] = siteId; + } + if (siteId.HasValue && permittedIds.Contains(siteId.Value.ToString())) + scoped.Add(record); + } + return scoped; } // ======================================================================== @@ -1223,7 +1292,7 @@ public class ManagementActor : ReceiveActor IsLocked = cmd.IsLocked }; var result = await svc.AddAttributeAsync(cmd.TemplateId, attr, user); - return result.IsSuccess ? result.Value : throw new InvalidOperationException(result.Error); + return result.IsSuccess ? result.Value : throw new ManagementCommandException(result.Error); } private static async Task HandleUpdateAttribute(IServiceProvider sp, UpdateTemplateAttributeCommand cmd, string user) @@ -1238,14 +1307,14 @@ public class ManagementActor : ReceiveActor IsLocked = cmd.IsLocked }; var result = await svc.UpdateAttributeAsync(cmd.AttributeId, attr, user); - return result.IsSuccess ? result.Value : throw new InvalidOperationException(result.Error); + return result.IsSuccess ? result.Value : throw new ManagementCommandException(result.Error); } private static async Task HandleDeleteAttribute(IServiceProvider sp, DeleteTemplateAttributeCommand cmd, string user) { var svc = sp.GetRequiredService(); var result = await svc.DeleteAttributeAsync(cmd.AttributeId, user); - return result.IsSuccess ? result.Value : throw new InvalidOperationException(result.Error); + return result.IsSuccess ? result.Value : throw new ManagementCommandException(result.Error); } private static async Task HandleAddAlarm(IServiceProvider sp, AddTemplateAlarmCommand cmd, string user) @@ -1260,7 +1329,7 @@ public class ManagementActor : ReceiveActor IsLocked = cmd.IsLocked }; var result = await svc.AddAlarmAsync(cmd.TemplateId, alarm, user); - return result.IsSuccess ? result.Value : throw new InvalidOperationException(result.Error); + return result.IsSuccess ? result.Value : throw new ManagementCommandException(result.Error); } private static async Task HandleUpdateAlarm(IServiceProvider sp, UpdateTemplateAlarmCommand cmd, string user) @@ -1275,14 +1344,14 @@ public class ManagementActor : ReceiveActor IsLocked = cmd.IsLocked }; var result = await svc.UpdateAlarmAsync(cmd.AlarmId, alarm, user); - return result.IsSuccess ? result.Value : throw new InvalidOperationException(result.Error); + return result.IsSuccess ? result.Value : throw new ManagementCommandException(result.Error); } private static async Task HandleDeleteAlarm(IServiceProvider sp, DeleteTemplateAlarmCommand cmd, string user) { var svc = sp.GetRequiredService(); var result = await svc.DeleteAlarmAsync(cmd.AlarmId, user); - return result.IsSuccess ? result.Value : throw new InvalidOperationException(result.Error); + return result.IsSuccess ? result.Value : throw new ManagementCommandException(result.Error); } private static async Task HandleAddScript(IServiceProvider sp, AddTemplateScriptCommand cmd, string user) @@ -1297,7 +1366,7 @@ public class ManagementActor : ReceiveActor ReturnDefinition = cmd.ReturnDefinition }; var result = await svc.AddScriptAsync(cmd.TemplateId, script, user); - return result.IsSuccess ? result.Value : throw new InvalidOperationException(result.Error); + return result.IsSuccess ? result.Value : throw new ManagementCommandException(result.Error); } private static async Task HandleUpdateScript(IServiceProvider sp, UpdateTemplateScriptCommand cmd, string user) @@ -1312,28 +1381,28 @@ public class ManagementActor : ReceiveActor ReturnDefinition = cmd.ReturnDefinition }; var result = await svc.UpdateScriptAsync(cmd.ScriptId, script, user); - return result.IsSuccess ? result.Value : throw new InvalidOperationException(result.Error); + return result.IsSuccess ? result.Value : throw new ManagementCommandException(result.Error); } private static async Task HandleDeleteScript(IServiceProvider sp, DeleteTemplateScriptCommand cmd, string user) { var svc = sp.GetRequiredService(); var result = await svc.DeleteScriptAsync(cmd.ScriptId, user); - return result.IsSuccess ? result.Value : throw new InvalidOperationException(result.Error); + return result.IsSuccess ? result.Value : throw new ManagementCommandException(result.Error); } private static async Task HandleAddComposition(IServiceProvider sp, AddTemplateCompositionCommand cmd, string user) { var svc = sp.GetRequiredService(); var result = await svc.AddCompositionAsync(cmd.TemplateId, cmd.ComposedTemplateId, cmd.InstanceName, user); - return result.IsSuccess ? result.Value : throw new InvalidOperationException(result.Error); + return result.IsSuccess ? result.Value : throw new ManagementCommandException(result.Error); } private static async Task HandleDeleteComposition(IServiceProvider sp, DeleteTemplateCompositionCommand cmd, string user) { var svc = sp.GetRequiredService(); var result = await svc.DeleteCompositionAsync(cmd.CompositionId, user); - return result.IsSuccess ? result.Value : throw new InvalidOperationException(result.Error); + return result.IsSuccess ? result.Value : throw new ManagementCommandException(result.Error); } // ======================================================================== @@ -1356,21 +1425,21 @@ public class ManagementActor : ReceiveActor { var svc = sp.GetRequiredService(); var result = await svc.CreateSharedScriptAsync(cmd.Name, cmd.Code, cmd.ParameterDefinitions, cmd.ReturnDefinition, user); - return result.IsSuccess ? result.Value : throw new InvalidOperationException(result.Error); + return result.IsSuccess ? result.Value : throw new ManagementCommandException(result.Error); } private static async Task HandleUpdateSharedScript(IServiceProvider sp, UpdateSharedScriptCommand cmd, string user) { var svc = sp.GetRequiredService(); var result = await svc.UpdateSharedScriptAsync(cmd.SharedScriptId, cmd.Code, cmd.ParameterDefinitions, cmd.ReturnDefinition, user); - return result.IsSuccess ? result.Value : throw new InvalidOperationException(result.Error); + return result.IsSuccess ? result.Value : throw new ManagementCommandException(result.Error); } private static async Task HandleDeleteSharedScript(IServiceProvider sp, DeleteSharedScriptCommand cmd, string user) { var svc = sp.GetRequiredService(); var result = await svc.DeleteSharedScriptAsync(cmd.SharedScriptId, user); - return result.IsSuccess ? result.Value : throw new InvalidOperationException(result.Error); + return result.IsSuccess ? result.Value : throw new ManagementCommandException(result.Error); } // ======================================================================== @@ -1403,7 +1472,7 @@ public class ManagementActor : ReceiveActor { var repo = sp.GetRequiredService(); var def = await repo.GetDatabaseConnectionByIdAsync(cmd.DatabaseConnectionId) - ?? throw new InvalidOperationException($"DatabaseConnection with ID {cmd.DatabaseConnectionId} not found."); + ?? throw new ManagementCommandException($"DatabaseConnection with ID {cmd.DatabaseConnectionId} not found."); def.Name = cmd.Name; def.ConnectionString = cmd.ConnectionString; await repo.UpdateDatabaseConnectionAsync(def); @@ -1457,7 +1526,7 @@ public class ManagementActor : ReceiveActor { var repo = sp.GetRequiredService(); var method = await repo.GetApiMethodByIdAsync(cmd.ApiMethodId) - ?? throw new InvalidOperationException($"ApiMethod with ID {cmd.ApiMethodId} not found."); + ?? throw new ManagementCommandException($"ApiMethod with ID {cmd.ApiMethodId} not found."); method.Script = cmd.Script; method.TimeoutSeconds = cmd.TimeoutSeconds; method.ParameterDefinitions = cmd.ParameterDefinitions; @@ -1489,7 +1558,7 @@ public class ManagementActor : ReceiveActor { var repo = sp.GetRequiredService(); var key = await repo.GetApiKeyByIdAsync(cmd.ApiKeyId) - ?? throw new InvalidOperationException($"ApiKey with ID {cmd.ApiKeyId} not found."); + ?? throw new ManagementCommandException($"ApiKey with ID {cmd.ApiKeyId} not found."); key.IsEnabled = cmd.IsEnabled; await repo.UpdateApiKeyAsync(key); await repo.SaveChangesAsync(); @@ -1530,7 +1599,7 @@ public class ManagementActor : ReceiveActor { var repo = sp.GetRequiredService(); var area = await repo.GetAreaByIdAsync(cmd.AreaId) - ?? throw new InvalidOperationException($"Area with ID {cmd.AreaId} not found."); + ?? throw new ManagementCommandException($"Area with ID {cmd.AreaId} not found."); area.Name = cmd.Name; await repo.UpdateAreaAsync(area); await repo.SaveChangesAsync(); @@ -1576,13 +1645,13 @@ public class ManagementActor : ReceiveActor { var instanceRepo = sp.GetRequiredService(); var instance = await instanceRepo.GetInstanceByIdAsync(cmd.InstanceId) - ?? throw new InvalidOperationException($"Instance {cmd.InstanceId} not found."); + ?? throw new ManagementCommandException($"Instance {cmd.InstanceId} not found."); EnforceSiteScope(user, instance.SiteId); var siteRepo = sp.GetRequiredService(); var site = await siteRepo.GetSiteByIdAsync(instance.SiteId) - ?? throw new InvalidOperationException($"Site {instance.SiteId} not found."); + ?? throw new ManagementCommandException($"Site {instance.SiteId} not found."); var commService = sp.GetRequiredService(); var request = new DebugSnapshotRequest(instance.UniqueName, Guid.NewGuid().ToString("N")); @@ -1597,3 +1666,16 @@ public class SiteScopeViolationException : Exception { public SiteScopeViolationException(string message) : base(message) { } } + +/// +/// Thrown by a command handler to signal a curated, caller-safe failure (finding +/// ManagementService-016). Its is intended to be +/// surfaced verbatim to the HTTP/CLI caller — e.g. a validation result or a +/// "not found" message. Unanticipated exceptions (database faults, parse errors, +/// null-reference, etc.) must NOT be this type, so that MapFault can return +/// a generic message for them and avoid leaking internal detail. +/// +public class ManagementCommandException : Exception +{ + public ManagementCommandException(string message) : base(message) { } +} diff --git a/tests/ScadaLink.ManagementService.Tests/ManagementActorTests.cs b/tests/ScadaLink.ManagementService.Tests/ManagementActorTests.cs index 15df581..d2b4787 100644 --- a/tests/ScadaLink.ManagementService.Tests/ManagementActorTests.cs +++ b/tests/ScadaLink.ManagementService.Tests/ManagementActorTests.cs @@ -218,7 +218,10 @@ public class ManagementActorTests : TestKit, IDisposable var response = ExpectMsg(TimeSpan.FromSeconds(5)); Assert.Equal(envelope.CorrelationId, response.CorrelationId); Assert.Equal("COMMAND_FAILED", response.ErrorCode); - Assert.Contains("Database connection lost", response.Error); + // An unanticipated repository fault must NOT leak its raw message to the + // caller (finding ManagementService-016); a generic message is returned. + Assert.DoesNotContain("Database connection lost", response.Error); + Assert.Contains(envelope.CorrelationId, response.Error); } [Fact] @@ -480,7 +483,9 @@ public class ManagementActorTests : TestKit, IDisposable var response = ExpectMsg(TimeSpan.FromSeconds(5)); Assert.Equal("COMMAND_FAILED", response.ErrorCode); - Assert.Contains("Connection refused", response.Error); + // Raw repository fault text is not leaked (finding ManagementService-016). + Assert.DoesNotContain("Connection refused", response.Error); + Assert.Contains(envelope.CorrelationId, response.Error); } // ======================================================================== @@ -757,4 +762,262 @@ public class ManagementActorTests : TestKit, IDisposable Assert.Equal(envelope.CorrelationId, response.CorrelationId); Assert.Equal("COMMAND_FAILED", response.ErrorCode); } + + // ======================================================================== + // QueryDeployments authorization + site-scope (findings -014 / -017) + // + // QueryDeploymentsCommand is gated to the Deployment role and, like every + // other Deployment-role handler, must enforce site scoping: a site-scoped + // user must not see deployment records for instances at other sites. + // ======================================================================== + + private static Commons.Entities.Deployment.DeploymentRecord DeploymentRecordFor(int instanceId) => + new("deploy-" + instanceId, "operator") { Id = instanceId, InstanceId = instanceId }; + + [Fact] + public void QueryDeployments_WithDesignRole_ReturnsUnauthorized() + { + var actor = CreateActor(); + var envelope = Envelope(new QueryDeploymentsCommand(), "Design"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Contains("Deployment", response.Message); + } + + [Fact] + public void QueryDeployments_UnfilteredWithDeploymentRole_ReturnsAllRecords() + { + var deployRepo = Substitute.For(); + deployRepo.GetAllDeploymentRecordsAsync(Arg.Any()) + .Returns(new List + { + DeploymentRecordFor(1), DeploymentRecordFor(2) + }); + _services.AddScoped(_ => deployRepo); + + var actor = CreateActor(); + var envelope = Envelope(new QueryDeploymentsCommand(), "Deployment"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Contains("deploy-1", response.JsonData); + Assert.Contains("deploy-2", response.JsonData); + } + + [Fact] + public void QueryDeployments_FilteredByInstanceId_ReturnsInstanceRecords() + { + var deployRepo = Substitute.For(); + deployRepo.GetDeploymentsByInstanceIdAsync(5, Arg.Any()) + .Returns(new List { DeploymentRecordFor(5) }); + _services.AddScoped(_ => deployRepo); + + var actor = CreateActor(); + var envelope = Envelope(new QueryDeploymentsCommand(InstanceId: 5), "Deployment"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Contains("deploy-5", response.JsonData); + } + + [Fact] + public void QueryDeployments_FilteredByOutOfScopeInstance_ReturnsUnauthorized() + { + // Instance 5 belongs to site 2; user is scoped to site 1. + _templateRepo.GetInstanceByIdAsync(5, Arg.Any()) + .Returns(new Instance("Pump5") { Id = 5, SiteId = 2 }); + var deployRepo = Substitute.For(); + _services.AddScoped(_ => deployRepo); + + var actor = CreateActor(); + var envelope = ScopedEnvelope(new QueryDeploymentsCommand(InstanceId: 5), new[] { "1" }, "Deployment"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Equal(envelope.CorrelationId, response.CorrelationId); + // The out-of-scope instance's deployment history must not be queried. + deployRepo.DidNotReceiveWithAnyArgs().GetDeploymentsByInstanceIdAsync(default); + } + + [Fact] + public void QueryDeployments_FilteredByInScopeInstance_ReturnsRecords() + { + _templateRepo.GetInstanceByIdAsync(5, Arg.Any()) + .Returns(new Instance("Pump5") { Id = 5, SiteId = 1 }); + var deployRepo = Substitute.For(); + deployRepo.GetDeploymentsByInstanceIdAsync(5, Arg.Any()) + .Returns(new List { DeploymentRecordFor(5) }); + _services.AddScoped(_ => deployRepo); + + var actor = CreateActor(); + var envelope = ScopedEnvelope(new QueryDeploymentsCommand(InstanceId: 5), new[] { "1" }, "Deployment"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Contains("deploy-5", response.JsonData); + } + + [Fact] + public void QueryDeployments_UnfilteredForSiteScopedUser_DropsOutOfScopeRecords() + { + // Records for instances 1 (site 1, in scope) and 2 (site 2, out of scope). + _templateRepo.GetInstanceByIdAsync(1, Arg.Any()) + .Returns(new Instance("Pump1") { Id = 1, SiteId = 1 }); + _templateRepo.GetInstanceByIdAsync(2, Arg.Any()) + .Returns(new Instance("Pump2") { Id = 2, SiteId = 2 }); + var deployRepo = Substitute.For(); + deployRepo.GetAllDeploymentRecordsAsync(Arg.Any()) + .Returns(new List + { + DeploymentRecordFor(1), DeploymentRecordFor(2) + }); + _services.AddScoped(_ => deployRepo); + + var actor = CreateActor(); + var envelope = ScopedEnvelope(new QueryDeploymentsCommand(), new[] { "1" }, "Deployment"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Contains("deploy-1", response.JsonData); + Assert.DoesNotContain("deploy-2", response.JsonData); + } + + [Fact] + public void QueryDeployments_UnfilteredForAdminUser_ReturnsAllRecords() + { + // Admin role bypasses site scoping even with PermittedSiteIds set. + // (The user also holds Deployment so it passes the role gate.) + var deployRepo = Substitute.For(); + deployRepo.GetAllDeploymentRecordsAsync(Arg.Any()) + .Returns(new List + { + DeploymentRecordFor(1), DeploymentRecordFor(2) + }); + _services.AddScoped(_ => deployRepo); + + var actor = CreateActor(); + var envelope = ScopedEnvelope(new QueryDeploymentsCommand(), new[] { "1" }, "Admin", "Deployment"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Contains("deploy-1", response.JsonData); + Assert.Contains("deploy-2", response.JsonData); + } + + // ======================================================================== + // SetInstanceOverrides atomicity (finding -015) + // + // A multi-override command must be all-or-nothing: if any requested override + // is invalid (unknown/locked attribute), the handler must reject the whole + // command up front WITHOUT persisting any of the overrides. + // ======================================================================== + + [Fact] + public void SetInstanceOverrides_WithOneInvalidAttribute_PersistsNoOverrides() + { + // Instance 3, template 1 with a single valid attribute "Good". + var instance = new Instance("Pump3") { Id = 3, SiteId = 1, TemplateId = 1 }; + _templateRepo.GetInstanceByIdAsync(3, Arg.Any()).Returns(instance); + _templateRepo.GetAttributesByTemplateIdAsync(1, Arg.Any()) + .Returns(new List { new("Good") { Id = 1, TemplateId = 1 } }); + _templateRepo.GetOverridesByInstanceIdAsync(3, Arg.Any()) + .Returns(new List()); + + var actor = CreateActor(); + // "Good" is valid, "Bogus" is not — the whole command must fail with + // nothing written. + var overrides = new Dictionary { ["Good"] = "1", ["Bogus"] = "2" }; + var envelope = Envelope(new SetInstanceOverridesCommand(3, overrides), "Deployment"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Equal("COMMAND_FAILED", response.ErrorCode); + // No override write occurred for the valid attribute either. + _templateRepo.DidNotReceiveWithAnyArgs() + .AddInstanceAttributeOverrideAsync(default!, default); + _templateRepo.DidNotReceiveWithAnyArgs() + .UpdateInstanceAttributeOverrideAsync(default!, default); + } + + [Fact] + public void SetInstanceOverrides_AllValid_PersistsAllOverrides() + { + var instance = new Instance("Pump4") { Id = 4, SiteId = 1, TemplateId = 1 }; + _templateRepo.GetInstanceByIdAsync(4, Arg.Any()).Returns(instance); + _templateRepo.GetAttributesByTemplateIdAsync(1, Arg.Any()) + .Returns(new List + { + new("A") { Id = 1, TemplateId = 1 }, + new("B") { Id = 2, TemplateId = 1 } + }); + _templateRepo.GetOverridesByInstanceIdAsync(4, Arg.Any()) + .Returns(new List()); + _templateRepo.SaveChangesAsync(Arg.Any()).Returns(1); + + var actor = CreateActor(); + var overrides = new Dictionary { ["A"] = "1", ["B"] = "2" }; + var envelope = Envelope(new SetInstanceOverridesCommand(4, overrides), "Deployment"); + + actor.Tell(envelope); + + ExpectMsg(TimeSpan.FromSeconds(5)); + _templateRepo.ReceivedWithAnyArgs(2) + .AddInstanceAttributeOverrideAsync(default!, default); + } + + // ======================================================================== + // Unexpected exception messages not leaked to callers (finding -016) + // + // MapFault must distinguish handler-curated failures (safe to surface) from + // unanticipated faults (whose raw .Message can disclose internal detail). + // ======================================================================== + + [Fact] + public void UnexpectedFault_ReturnsGenericMessage_NotRawExceptionText() + { + // Repository throws an unanticipated fault carrying sensitive-looking + // detail. The raw text must NOT reach the caller. + const string secret = "Server=db-internal-prod;constraint FK_secret"; + _templateRepo.GetAllTemplatesAsync(Arg.Any()) + .ThrowsAsync(new InvalidProgramException(secret)); + + var actor = CreateActor(); + var envelope = Envelope(new ListTemplatesCommand()); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Equal("COMMAND_FAILED", response.ErrorCode); + Assert.DoesNotContain(secret, response.Error); + // The correlation ID is surfaced so the operator can find the server log. + Assert.Contains(envelope.CorrelationId, response.Error); + } + + [Fact] + public void CuratedHandlerFailure_SurfacesTheCuratedMessage() + { + // A handler-thrown ManagementCommandException carries a message that is + // intentionally safe to surface (e.g. a validation result). + _templateRepo.GetTemplateByIdAsync(99, Arg.Any()) + .Returns((Template?)null); + + var actor = CreateActor(); + var envelope = Envelope(new CreateInstanceCommand("BadInst", 99, 1), "Deployment"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Equal("COMMAND_FAILED", response.ErrorCode); + // The curated InstanceService failure message is still surfaced verbatim. + Assert.Contains("99", response.Error); + } }