From 7f7ea3f3c9ace03cb1f9b49a7408f1f8a089b724 Mon Sep 17 00:00:00 2001
From: Joseph Doherty <dohejw01@gmail.com>
Date: Tue, 2 Jun 2026 04:01:43 -0400
Subject: [PATCH] =?UTF-8?q?fix(auth):=20C1=20review=20polish=20=E2=80=94?=
 =?UTF-8?q?=20guard=20name=20at=20seam,=20document=20seam=20contract=20(th?=
 =?UTF-8?q?rows/O(n)),=20explicit=20cookie=20test=20(review=20#1/#2/#3/#5/?=
 =?UTF-8?q?#8)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 .../Interfaces/Security/IInboundApiKeyAdmin.cs       |  9 +++++++++
 .../LibraryInboundApiKeyAdmin.cs                     |  3 +++
 .../LibraryInboundApiKeyAdminTests.cs                | 12 ++++++++++++
 .../SecurityTests.cs                                 |  3 +++
 4 files changed, 27 insertions(+)

diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Security/IInboundApiKeyAdmin.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Security/IInboundApiKeyAdmin.cs
index 64b26b1d..ac1db4c1 100644
--- a/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Security/IInboundApiKeyAdmin.cs
+++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Security/IInboundApiKeyAdmin.cs
@@ -34,6 +34,13 @@ public sealed record InboundApiKeyCreated(string KeyId, string Token);
 /// their method-scopes. The interface lives in Commons and is deliberately free of any
 /// dependency on the underlying auth library, so consumers depend only on this contract.
 /// </summary>
+/// <remarks>
+/// Mutating operations (<see cref="CreateAsync"/>, <see cref="SetEnabledAsync"/>,
+/// <see cref="SetMethodsAsync"/>, <see cref="DeleteAsync"/>) may <b>throw</b> on
+/// store-level or configuration failures (e.g. an unavailable pepper) rather than
+/// exclusively signalling failure via their <c>bool</c> return — callers must handle
+/// exceptions in addition to checking the return value.
+/// </remarks>
 public interface IInboundApiKeyAdmin
 {
     /// <summary>Creates a new key scoped to <paramref name="methods"/> and returns its
@@ -58,9 +65,11 @@ public interface IInboundApiKeyAdmin
     Task<bool> DeleteAsync(string keyId, CancellationToken ct = default);
 
     /// <summary>Returns the method-scope set for a key, or an empty list if not found.</summary>
+    /// <remarks>Enumerates the full key list (O(n)); intended for admin-scale use, not hot paths.</remarks>
     Task<IReadOnlyList<string>> GetMethodsForKeyAsync(string keyId, CancellationToken ct = default);
 
     /// <summary>Returns the identifiers of all keys whose scopes contain
     /// <paramref name="methodName"/>.</summary>
+    /// <remarks>Enumerates the full key list (O(n)); intended for admin-scale use, not hot paths.</remarks>
     Task<IReadOnlyList<string>> GetKeysForMethodAsync(string methodName, CancellationToken ct = default);
 }
diff --git a/src/ZB.MOM.WW.ScadaBridge.Security/LibraryInboundApiKeyAdmin.cs b/src/ZB.MOM.WW.ScadaBridge.Security/LibraryInboundApiKeyAdmin.cs
index bfb02cb5..778ef99c 100644
--- a/src/ZB.MOM.WW.ScadaBridge.Security/LibraryInboundApiKeyAdmin.cs
+++ b/src/ZB.MOM.WW.ScadaBridge.Security/LibraryInboundApiKeyAdmin.cs
@@ -34,8 +34,11 @@ public sealed class LibraryInboundApiKeyAdmin : IInboundApiKeyAdmin
     public async Task<InboundApiKeyCreated> CreateAsync(
         string name, IReadOnlyCollection<string> methods, CancellationToken ct = default)
     {
+        ArgumentException.ThrowIfNullOrWhiteSpace(name);
         ArgumentNullException.ThrowIfNull(methods);
 
+        // "N" format = 32 hex chars, no hyphens/underscores — the library rejects underscores
+        // in keyId because they delimit the sbk_<keyId>_<secret> token.
         var keyId = Guid.NewGuid().ToString("N");
         var result = await _admin.CreateKeyAsync(
             keyId, name, methods.ToHashSet(StringComparer.Ordinal),
diff --git a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/LibraryInboundApiKeyAdminTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/LibraryInboundApiKeyAdminTests.cs
index 31c8de26..2f277dba 100644
--- a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/LibraryInboundApiKeyAdminTests.cs
+++ b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/LibraryInboundApiKeyAdminTests.cs
@@ -54,6 +54,18 @@ public sealed class LibraryInboundApiKeyAdminTests : IAsyncLifetime
         _sut = new LibraryInboundApiKeyAdmin(commands);
     }
 
+    [Theory]
+    [InlineData(null)]
+    [InlineData("")]
+    [InlineData("   ")]
+    public async Task CreateAsync_NullOrWhitespaceName_Throws(string? name)
+    {
+        // ThrowIfNullOrWhiteSpace throws ArgumentNullException for null and ArgumentException
+        // for empty/whitespace; both are ArgumentException subtypes, so ThrowsAnyAsync covers all.
+        await Assert.ThrowsAnyAsync<ArgumentException>(
+            () => _sut.CreateAsync(name!, new[] { "MethodA" }, CancellationToken.None));
+    }
+
     [Fact]
     public async Task CreateAsync_ReturnsKeyIdAndToken_TokenStartsWith_sbk()
     {
diff --git a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/SecurityTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/SecurityTests.cs
index 3eabd496..2fe3dcb3 100644
--- a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/SecurityTests.cs
+++ b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/SecurityTests.cs
@@ -443,6 +443,9 @@ public class SecurityReviewRegressionTests
         services.AddLogging();
         services.AddDataProtection();
         services.AddSecurity();
+        // Explicitly set RequireHttpsCookie=true so the test asserts SecurePolicy.Always
+        // without relying on the SecurityOptions default value.
+        services.Configure<SecurityOptions>(o => o.RequireHttpsCookie = true);
 
         using var provider = services.BuildServiceProvider();
         var cookieOptions = provider