fix(auth): C2 review — not-found throws (no spurious audit) on update/delete/set-methods, reject empty methods (unusable-key/stealth-disable), richer set-methods response, token advisory to stderr

src/ZB.MOM.WW.ScadaBridge.CLI/Commands/SecurityCommands.cs

@@ -122,9 +122,10 @@ public static class SecurityCommands
     /// <summary>
     /// 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.
     /// </summary>
     /// <param name="json">The JSON success body returned by the management API.</param>
-    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;
     }

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_<keyId>_<secret>).
         // 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<IInboundApiKeyAdmin>();
         var created = await admin.CreateAsync(cmd.Name, cmd.Methods);
 
@@ -1333,6 +1336,8 @@ public class ManagementActor : ReceiveActor
     {
         var admin = sp.GetRequiredService<IInboundApiKeyAdmin>();
         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<IInboundApiKeyAdmin>();
-        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<IInboundApiKeyAdmin>();
         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<object?> HandleListScopeRules(IServiceProvider sp, ListScopeRulesCommand cmd)

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;
+
+/// <summary>
+/// Tests for <see cref="SecurityCommands"/> 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.
+/// </summary>
+[Collection("Console")]
+public class SecurityCommandsTests
+{
+    /// <summary>
+    /// The advisory line "Save this token now — it will not be shown again:" must be
+    /// written to stderr, not stdout.
+    /// </summary>
+    [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 });
+        }
+    }
+
+    /// <summary>
+    /// The token value is written to stdout; piping <c>| xargs</c> 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).
+    /// </summary>
+    [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 });
+        }
+    }
+}

tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ApiKeyCreationTests.cs

@@ -167,12 +167,99 @@ public class ApiKeyCreationTests : TestKit, IDisposable
         var response = ExpectMsg<ManagementSuccess>(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<string>(), Arg.Any<object?>());
     }
 
+    // =========================================================================
+    // 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<ManagementError>(TimeSpan.FromSeconds(5));
+
+        Assert.Contains("key-unknown", response.Error);
+        // Seam returned false → audit must not have fired.
+        _auditService.DidNotReceive().LogAsync(
+            Arg.Any<string>(), "Update", "ApiKey", Arg.Any<string>(), Arg.Any<string>(), Arg.Any<object?>());
+    }
+
+    [Fact]
+    public void SetApiKeyMethods_UnknownKey_ReturnsManagementError_AndDoesNotAudit()
+    {
+        var actor = CreateActor();
+        actor.Tell(Envelope(new SetApiKeyMethodsCommand("key-unknown", new[] { "M1" }), "Admin"));
+
+        var response = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
+
+        Assert.Contains("key-unknown", response.Error);
+        _auditService.DidNotReceive().LogAsync(
+            Arg.Any<string>(), "Update", "ApiKey", Arg.Any<string>(), Arg.Any<string>(), Arg.Any<object?>());
+    }
+
+    [Fact]
+    public void DeleteApiKey_UnknownKey_ReturnsManagementError_AndDoesNotAudit()
+    {
+        var actor = CreateActor();
+        actor.Tell(Envelope(new DeleteApiKeyCommand("key-unknown"), "Admin"));
+
+        var response = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
+
+        Assert.Contains("key-unknown", response.Error);
+        _auditService.DidNotReceive().LogAsync(
+            Arg.Any<string>(), "Delete", "ApiKey", Arg.Any<string>(), Arg.Any<string>(), Arg.Any<object?>());
+    }
+
+    // =========================================================================
+    // 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<string>()), "Admin"));
+
+        var response = ExpectMsg<ManagementError>(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<string>(), "Create", "ApiKey", Arg.Any<string>(), Arg.Any<string>(), Arg.Any<object?>());
+    }
+
+    [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<string>()), "Admin"));
+
+        var response = ExpectMsg<ManagementError>(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<string>(), "Update", "ApiKey", Arg.Any<string>(), Arg.Any<string>(), Arg.Any<object?>());
+    }
+
     [Theory]
     [MemberData(nameof(AllApiKeyCommands))]
     public void EveryApiKeyCommand_RequiresAdminRole(object command)