diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/SecurityCommands.cs b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/SecurityCommands.cs index b55865b4..50e2da93 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/SecurityCommands.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/SecurityCommands.cs @@ -122,9 +122,10 @@ public static class SecurityCommands /// /// Renders the create-key response, surfacing the one-time bearer token prominently — /// it is the only moment the secret is available and cannot be retrieved afterwards. + /// The advisory line is written to stderr so that piping stdout captures only the token. /// /// The JSON success body returned by the management API. - private static int PrintCreatedKey(string json) + internal static int PrintCreatedKey(string json) { using var doc = System.Text.Json.JsonDocument.Parse(json); var root = doc.RootElement; @@ -133,7 +134,7 @@ public static class SecurityCommands Console.WriteLine($"API key created. KeyId: {keyId}"); Console.WriteLine(); - Console.WriteLine("Save this token now — it will not be shown again:"); + Console.Error.WriteLine("Save this token now — it will not be shown again:"); Console.WriteLine($" {token}"); return 0; } diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index a82b7326..dd1b7e3e 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -1315,6 +1315,9 @@ public class ManagementActor : ReceiveActor // peppered hash, and assembles the one-time bearer token (sbk__). // The token is shown to the operator only here, in the create response; it cannot // be retrieved later. No hash/secret is stored or returned by ScadaBridge. + if (cmd.Methods is null || cmd.Methods.Count == 0) + throw new ManagementCommandException("At least one method must be specified for an API key."); + var admin = sp.GetRequiredService(); var created = await admin.CreateAsync(cmd.Name, cmd.Methods); @@ -1333,6 +1336,8 @@ public class ManagementActor : ReceiveActor { var admin = sp.GetRequiredService(); var deleted = await admin.DeleteAsync(cmd.KeyId); + if (!deleted) + throw new ManagementCommandException($"API key '{cmd.KeyId}' not found."); await AuditAsync(sp, user, "Delete", "ApiKey", cmd.KeyId, cmd.KeyId, null); return deleted; } @@ -1757,7 +1762,9 @@ public class ManagementActor : ReceiveActor { // Inbound-API key re-arch (C2): enable/disable via the shared seam (no secret change). var admin = sp.GetRequiredService(); - await admin.SetEnabledAsync(cmd.KeyId, cmd.IsEnabled); + var updated = await admin.SetEnabledAsync(cmd.KeyId, cmd.IsEnabled); + if (!updated) + throw new ManagementCommandException($"API key '{cmd.KeyId}' not found."); await AuditAsync(sp, user, "Update", "ApiKey", cmd.KeyId, cmd.KeyId, new { cmd.KeyId, cmd.IsEnabled }); return new { cmd.KeyId, cmd.IsEnabled }; @@ -1767,11 +1774,16 @@ public class ManagementActor : ReceiveActor { // Inbound-API key re-arch (C2): replace a key's method-scope set via the shared seam // (no secret change). The library is authoritative for the scope replacement. + if (cmd.Methods is null || cmd.Methods.Count == 0) + throw new ManagementCommandException("At least one method must be specified for an API key."); + var admin = sp.GetRequiredService(); var updated = await admin.SetMethodsAsync(cmd.KeyId, cmd.Methods); + if (!updated) + throw new ManagementCommandException($"API key '{cmd.KeyId}' not found."); await AuditAsync(sp, user, "Update", "ApiKey", cmd.KeyId, cmd.KeyId, new { cmd.KeyId, cmd.Methods }); - return updated; + return new { cmd.KeyId, Methods = cmd.Methods }; } private static async Task HandleListScopeRules(IServiceProvider sp, ListScopeRulesCommand cmd) diff --git a/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/Commands/SecurityCommandsTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/Commands/SecurityCommandsTests.cs new file mode 100644 index 00000000..564fa36b --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/Commands/SecurityCommandsTests.cs @@ -0,0 +1,83 @@ +using ZB.MOM.WW.ScadaBridge.CLI.Commands; + +namespace ZB.MOM.WW.ScadaBridge.CLI.Tests.Commands; + +/// +/// Tests for static helpers. +/// Fix 4 (review): the "Save this token now" advisory must reach stderr so that +/// piping stdout captures only the token (the actual secret), not the advisory text. +/// +[Collection("Console")] +public class SecurityCommandsTests +{ + /// + /// The advisory line "Save this token now — it will not be shown again:" must be + /// written to stderr, not stdout. + /// + [Fact] + public void PrintCreatedKey_AdvisoryLine_WrittenToStderr_NotStdout() + { + var json = """{"keyId":"abc123","name":"Test","token":"sbk_abc123_secret"}"""; + + var stdoutWriter = new StringWriter(); + var stderrWriter = new StringWriter(); + Console.SetOut(stdoutWriter); + Console.SetError(stderrWriter); + + try + { + var exitCode = SecurityCommands.PrintCreatedKey(json); + + Assert.Equal(0, exitCode); + + var stdout = stdoutWriter.ToString(); + var stderr = stderrWriter.ToString(); + + // The advisory must appear on stderr. + Assert.Contains("Save this token now", stderr); + Assert.Contains("will not be shown again", stderr); + + // The advisory must NOT appear on stdout (so pipe captures only the token). + Assert.DoesNotContain("Save this token now", stdout); + + // The token itself must appear on stdout. + Assert.Contains("sbk_abc123_secret", stdout); + } + finally + { + Console.SetOut(new StreamWriter(Console.OpenStandardOutput()) { AutoFlush = true }); + Console.SetError(new StreamWriter(Console.OpenStandardError()) { AutoFlush = true }); + } + } + + /// + /// The token value is written to stdout; piping | xargs captures only the token. + /// The keyId info line also appears on stdout (it is not sensitive and does not impede piping + /// since operators pipe the token line, not the whole output). + /// + [Fact] + public void PrintCreatedKey_Token_WrittenToStdout() + { + var json = """{"keyId":"key-42","name":"MES","token":"sbk_key-42_mysecret"}"""; + + var stdoutWriter = new StringWriter(); + var stderrWriter = new StringWriter(); + Console.SetOut(stdoutWriter); + Console.SetError(stderrWriter); + + try + { + SecurityCommands.PrintCreatedKey(json); + + var stdout = stdoutWriter.ToString(); + + Assert.Contains("sbk_key-42_mysecret", stdout); + Assert.Contains("key-42", stdout); + } + finally + { + Console.SetOut(new StreamWriter(Console.OpenStandardOutput()) { AutoFlush = true }); + Console.SetError(new StreamWriter(Console.OpenStandardError()) { AutoFlush = true }); + } + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ApiKeyCreationTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ApiKeyCreationTests.cs index e48ccc1c..c994a189 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ApiKeyCreationTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ApiKeyCreationTests.cs @@ -167,12 +167,99 @@ public class ApiKeyCreationTests : TestKit, IDisposable var response = ExpectMsg(TimeSpan.FromSeconds(5)); Assert.Equal(new[] { "New1", "New2", "New3" }, _admin.Keys["key-1"].Methods); - Assert.Equal("true", response.JsonData); + + // Fix 3 (review): response is now the richer { KeyId, Methods } shape. + using var doc = JsonDocument.Parse(response.JsonData); + Assert.Equal("key-1", doc.RootElement.GetProperty("keyId").GetString()); + var methods = doc.RootElement.GetProperty("methods").EnumerateArray() + .Select(m => m.GetString()).ToArray(); + Assert.Equal(new[] { "New1", "New2", "New3" }, methods); _auditService.Received(1).LogAsync( "admin", "Update", "ApiKey", "key-1", Arg.Any(), Arg.Any()); } + // ========================================================================= + // Fix 1 (review): not-found on mutating ops → ManagementError, no audit + // ========================================================================= + + [Fact] + public void UpdateApiKey_UnknownKey_ReturnsManagementError_AndDoesNotAudit() + { + // No keys seeded — "key-unknown" does not exist. + var actor = CreateActor(); + actor.Tell(Envelope(new UpdateApiKeyCommand("key-unknown", false), "Admin")); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + + Assert.Contains("key-unknown", response.Error); + // Seam returned false → audit must not have fired. + _auditService.DidNotReceive().LogAsync( + Arg.Any(), "Update", "ApiKey", Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public void SetApiKeyMethods_UnknownKey_ReturnsManagementError_AndDoesNotAudit() + { + var actor = CreateActor(); + actor.Tell(Envelope(new SetApiKeyMethodsCommand("key-unknown", new[] { "M1" }), "Admin")); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + + Assert.Contains("key-unknown", response.Error); + _auditService.DidNotReceive().LogAsync( + Arg.Any(), "Update", "ApiKey", Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public void DeleteApiKey_UnknownKey_ReturnsManagementError_AndDoesNotAudit() + { + var actor = CreateActor(); + actor.Tell(Envelope(new DeleteApiKeyCommand("key-unknown"), "Admin")); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + + Assert.Contains("key-unknown", response.Error); + _auditService.DidNotReceive().LogAsync( + Arg.Any(), "Delete", "ApiKey", Arg.Any(), Arg.Any(), Arg.Any()); + } + + // ========================================================================= + // Fix 2 (review): empty methods set is rejected before seam + audit + // ========================================================================= + + [Fact] + public void CreateApiKey_EmptyMethods_ReturnsManagementError() + { + var actor = CreateActor(); + actor.Tell(Envelope(new CreateApiKeyCommand("MES-Production", Array.Empty()), "Admin")); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + + Assert.Contains("method", response.Error, StringComparison.OrdinalIgnoreCase); + // No key should have been created. + Assert.Empty(_admin.Keys); + _auditService.DidNotReceive().LogAsync( + Arg.Any(), "Create", "ApiKey", Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public void SetApiKeyMethods_EmptyMethods_ReturnsManagementError() + { + _admin.Seed("key-1", "Service A", enabled: true, "M1"); + + var actor = CreateActor(); + actor.Tell(Envelope(new SetApiKeyMethodsCommand("key-1", Array.Empty()), "Admin")); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + + Assert.Contains("method", response.Error, StringComparison.OrdinalIgnoreCase); + // Existing scope set must be unchanged. + Assert.Equal(new[] { "M1" }, _admin.Keys["key-1"].Methods); + _auditService.DidNotReceive().LogAsync( + Arg.Any(), "Update", "ApiKey", Arg.Any(), Arg.Any(), Arg.Any()); + } + [Theory] [MemberData(nameof(AllApiKeyCommands))] public void EveryApiKeyCommand_RequiresAdminRole(object command)