fix(auth): C1 review polish — guard name at seam, document seam contract (throws/O(n)), explicit cookie test (review #1/#2/#3/#5/#8)
This commit is contained in:
@@ -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
|
/// 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.
|
/// dependency on the underlying auth library, so consumers depend only on this contract.
|
||||||
/// </summary>
|
/// </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
|
public interface IInboundApiKeyAdmin
|
||||||
{
|
{
|
||||||
/// <summary>Creates a new key scoped to <paramref name="methods"/> and returns its
|
/// <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);
|
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>
|
/// <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);
|
Task<IReadOnlyList<string>> GetMethodsForKeyAsync(string keyId, CancellationToken ct = default);
|
||||||
|
|
||||||
/// <summary>Returns the identifiers of all keys whose scopes contain
|
/// <summary>Returns the identifiers of all keys whose scopes contain
|
||||||
/// <paramref name="methodName"/>.</summary>
|
/// <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);
|
Task<IReadOnlyList<string>> GetKeysForMethodAsync(string methodName, CancellationToken ct = default);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -34,8 +34,11 @@ public sealed class LibraryInboundApiKeyAdmin : IInboundApiKeyAdmin
|
|||||||
public async Task<InboundApiKeyCreated> CreateAsync(
|
public async Task<InboundApiKeyCreated> CreateAsync(
|
||||||
string name, IReadOnlyCollection<string> methods, CancellationToken ct = default)
|
string name, IReadOnlyCollection<string> methods, CancellationToken ct = default)
|
||||||
{
|
{
|
||||||
|
ArgumentException.ThrowIfNullOrWhiteSpace(name);
|
||||||
ArgumentNullException.ThrowIfNull(methods);
|
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 keyId = Guid.NewGuid().ToString("N");
|
||||||
var result = await _admin.CreateKeyAsync(
|
var result = await _admin.CreateKeyAsync(
|
||||||
keyId, name, methods.ToHashSet(StringComparer.Ordinal),
|
keyId, name, methods.ToHashSet(StringComparer.Ordinal),
|
||||||
|
|||||||
@@ -54,6 +54,18 @@ public sealed class LibraryInboundApiKeyAdminTests : IAsyncLifetime
|
|||||||
_sut = new LibraryInboundApiKeyAdmin(commands);
|
_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]
|
[Fact]
|
||||||
public async Task CreateAsync_ReturnsKeyIdAndToken_TokenStartsWith_sbk()
|
public async Task CreateAsync_ReturnsKeyIdAndToken_TokenStartsWith_sbk()
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -443,6 +443,9 @@ public class SecurityReviewRegressionTests
|
|||||||
services.AddLogging();
|
services.AddLogging();
|
||||||
services.AddDataProtection();
|
services.AddDataProtection();
|
||||||
services.AddSecurity();
|
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();
|
using var provider = services.BuildServiceProvider();
|
||||||
var cookieOptions = provider
|
var cookieOptions = provider
|
||||||
|
|||||||
Reference in New Issue
Block a user