Resolve Tests-003, -004, -005, -006 code-review findings

Tests-003: temp auth-DB directories leaked under %TEMP%. Added the
TempDatabaseDirectory IDisposable helper (clears the Sqlite connection pool,
then recursively deletes); SqliteAuthStoreTests and ApiKeyAdminCliRunnerTests
now dispose every directory they create.

Tests-004: added end-to-end coverage composing the real authorization
interceptor in front of the real MxAccessGatewayService, plus scope-resolver
tests confirming an unmapped request type fails closed to the admin scope.

Tests-005: added coverage for a worker faulting mid-command — a pipe
disconnect and a worker fault while an InvokeAsync is in flight both fail the
pending invoke. No product change needed.

Tests-006 (re-triaged): the flaky ReadLoop_WhenClientFaults_KillsOwnedWorkerProcess
is a test race, not a product bug — the kill runs synchronously inside
SetFaulted. Rewrote it to await FakeWorkerProcess exit deterministically, and
replaced fixed Task.Delay timing in the late-reply and heartbeat tests with
FIFO ordering and an injected ManualTimeProvider.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-18 21:44:55 -04:00
parent 98f9b7792b
commit 5ade3f4f48
8 changed files with 539 additions and 31 deletions
+11 -9
View File
@@ -7,7 +7,7 @@
| Review date | 2026-05-18 |
| Commit reviewed | `6c64030` |
| Status | Reviewed |
| Open findings | 10 |
| Open findings | 6 |
## Checklist coverage
@@ -65,13 +65,13 @@
| Severity | Medium |
| Category | Performance & resource management |
| Location | `src/MxGateway.Tests/Security/Authentication/SqliteAuthStoreTests.cs:170-176`, `src/MxGateway.Tests/Security/Authentication/ApiKeyAdminCliRunnerTests.cs:252-258` |
| Status | Open |
| Status | Resolved |
**Description:** `CreateTempDatabasePath` creates a fresh directory under `%TEMP%\mxgateway-auth-tests\<guid>` (and `...-cli-tests`) for every test but nothing ever deletes it. `WorkerProcessLauncherTests.TestDirectory` correctly implements `IDisposable` and cleans up; these two do not. SQLite connection pooling can also keep the `.db` handle open after the test. Over many CI runs this leaks temp files and open handles.
**Recommendation:** Wrap the temp directory in an `IDisposable`/`IAsyncDisposable` helper (as `WorkerProcessLauncherTests` does) and call `SqliteConnection.ClearAllPools()` before deletion, or use `Microsoft.Data.Sqlite` in-memory mode where a real file is not needed.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-18: confirmed root cause — both `CreateTempDatabasePath` helpers created `%TEMP%` directories with no cleanup, and `Microsoft.Data.Sqlite` pools connections by default so the `.db` handle outlives the test. Added a shared `TempDatabaseDirectory` (`src/MxGateway.Tests/Security/Authentication/TempDatabaseDirectory.cs`) `IDisposable` helper that calls `SqliteConnection.ClearAllPools()` and recursively deletes its directory. `SqliteAuthStoreTests` and `ApiKeyAdminCliRunnerTests` now implement `IDisposable`, track every directory created via `CreateTempDatabasePath`, and dispose them after each test. All affected tests still pass.
### Tests-004
@@ -80,13 +80,13 @@
| Severity | Medium |
| Category | Testing coverage |
| Location | `src/MxGateway.Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs` |
| Status | Open |
| Status | Resolved |
**Description:** The authorization interceptor and `MxAccessGatewayService` are each tested in isolation, but no test composes the interceptor in front of the real service to confirm scope enforcement gates real RPCs end-to-end. A wiring mistake — interceptor not registered, or a new RPC added without a scope mapping in `GatewayGrpcScopeResolver` — would pass every existing test. `GatewayGrpcScopeResolverTests` also only checks an enumerated allow-list; it never asserts an unmapped request type fails closed.
**Recommendation:** Add an end-to-end test that runs `OpenSession`/`Invoke` through the interceptor+service composition with insufficient scope and asserts `PermissionDenied`; add a `GatewayGrpcScopeResolver` test asserting an unknown/unmapped request type throws or denies rather than returning a permissive default.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-18: confirmed the coverage gap. Added three interceptor+service composition tests to `GatewayGrpcAuthorizationInterceptorTests` that run the real `GatewayGrpcAuthorizationInterceptor` continuation into a real `MxAccessGatewayService`: `InterceptorComposedWithService_OpenSessionMissingScope_DeniesBeforeServiceRuns` (asserts `PermissionDenied` and `OpenSessionCount == 0`), `InterceptorComposedWithService_OpenSessionWithScope_RunsServiceWithIdentity` (service runs and observes the interceptor-pushed identity), and `InterceptorComposedWithService_InvokeWriteCommandWithReadScope_DeniesBeforeServiceRuns` (a `Write` command with only `invoke:read` is denied). Added two `GatewayGrpcScopeResolverTests`: `ResolveRequiredScope_UnmappedRequestType_FailsClosedToAdminScope` confirms an unmapped request type resolves to the most-restrictive `Admin` scope (the resolver's `_ => GatewayScopes.Admin` default already fails closed — no product bug), and `ResolveRequiredScope_UnknownInvokeCommandKind_ReturnsInvokeReadScope` confirms an unknown command kind does not silently grant write/admin access.
### Tests-005
@@ -95,13 +95,13 @@
| Severity | Medium |
| Category | Testing coverage |
| Location | `src/MxGateway.Tests/Gateway/Grpc/EventStreamServiceTests.cs:239-261`, `src/MxGateway.Tests/Gateway/Sessions/SessionManagerTests.cs` |
| Status | Open |
| Status | Resolved |
**Description:** Worker-crash handling is only tested as a clean terminal exception from `ReadEventsAsync` or a pre-set `ShutdownException`. There is no test for a worker that faults mid-command — an `InvokeAsync` in flight when the pipe/worker dies — which is a core fault-handling path of the two-process design. `WorkerClientTests` covers pipe-disconnect faulting the read loop, but not the interaction where a pending `InvokeAsync` task observes the fault and surfaces a meaningful error code.
**Recommendation:** Add a `WorkerClient`/`SessionManager` test that disposes the worker pipe (or emits a `WorkerFault`) while an `InvokeAsync` is pending, and assert the invoke task fails with a `WorkerClientException`/`SessionManagerException` carrying the worker-faulted error code.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-18: confirmed the coverage gap and confirmed the product path already handles it correctly (`WorkerClient.ReadLoopAsync``SetFaulted``CompletePendingCommands(fault)` fails every pending command with the fault exception). Added two `WorkerClientTests`: `InvokeAsync_WhenPipeDisconnectsMidCommand_FailsPendingInvokeWithPipeDisconnected` (worker reads the command then disposes its pipe side; the pending invoke task fails with `WorkerClientErrorCode.PipeDisconnected`) and `InvokeAsync_WhenWorkerFaultsMidCommand_FailsPendingInvokeWithWorkerFaulted` (worker emits a `WorkerFault` envelope while the invoke is pending; the task fails with `WorkerClientErrorCode.WorkerFaulted`). Both also assert the client transitions to `Faulted`. No product change needed.
### Tests-006
@@ -110,13 +110,15 @@
| Severity | Medium |
| Category | Concurrency & thread safety |
| Location | `src/MxGateway.Tests/Gateway/Workers/WorkerClientTests.cs:76`, `src/MxGateway.Tests/Gateway/Workers/FakeWorkerHarnessTests.cs:122` |
| Status | Open |
| Status | Resolved |
**Description:** Several tests rely on fixed `Task.Delay` values: `WorkerClientTests.InvokeAsync_WithLateReply…` waits a hard-coded 50 ms after writing a late reply before issuing the second command, and the heartbeat tests use a 20 ms delay to make timestamps strictly increase. On a slow CI agent the 50 ms delay can be insufficient, and `DateTimeOffset.UtcNow` resolution can make the 20 ms heartbeat-advance assertion flaky.
**Recommendation:** Replace fixed delays with the existing `WaitUntilAsync` condition polling, and inject a controllable `TimeProvider` for heartbeat-timestamp comparisons instead of relying on wall-clock advance.
**Resolution:** _(open)_
**Re-triage note:** The brief flagged `ReadLoop_WhenClientFaults_KillsOwnedWorkerProcess` as "a real `WorkerClient` fault→kill bug". On inspection it is **not a product bug** — it is a test race. `WorkerClient.SetFaulted` publishes the `Faulted` state under lock *before* calling `KillOwnedProcess`, so the old test's `WaitUntilAsync(() => client.State == Faulted)` could return between those two statements and observe `process.KillCount == 0`. The kill itself always runs synchronously inside `SetFaulted`, and `ShutdownAsync`/`DisposeAsync` re-issue an idempotent kill, so no real consumer relies on "state==Faulted implies process dead". The fix is therefore a test-quality fix (correctly Medium / Concurrency), not a product fix.
**Resolution:** Resolved 2026-05-18: (1) Made `ReadLoop_WhenClientFaults_KillsOwnedWorkerProcess` deterministic — it now `await`s `FakeWorkerProcess.WaitForExitAsync` (the `TaskCompletionSource` completed inside `Kill()`), which completes exactly when the kill runs, eliminating the state-polling race; verified by running it five times in isolation (5/5 pass). (2) Removed the fixed 50 ms `Task.Delay` from `InvokeAsync_WithLateReply_IgnoresLateReplyAndKeepsClientReady` — the stale reply and the second reply are now sent in pipe (FIFO) order, so the read loop discards the stale reply before the second reply with no timing window. (3) Replaced the 20 ms `Task.Delay` heartbeat-advance hacks in `WorkerClientTests.ReadLoop_WhenHeartbeatArrives_UpdatesLastHeartbeatAndWorkerProcess` and `FakeWorkerHarnessTests.SendHeartbeatAsync_UpdatesClientHeartbeatState` with an injected `ManualTimeProvider` advanced by a fixed `TimeSpan`; both tests now assert the exact post-advance timestamp instead of `>` against wall-clock drift.
### Tests-007
@@ -110,16 +110,21 @@ public sealed class FakeWorkerHarnessTests
Assert.Equal(WorkerClientState.Faulted, client.State);
}
/// <summary>Verifies that sending a heartbeat updates the client heartbeat state.</summary>
/// <summary>
/// Verifies that sending a heartbeat updates the client heartbeat state. Uses a
/// <see cref="ManualTimeProvider"/> so the timestamp advance is deterministic rather
/// than relying on a wall-clock <c>Task.Delay</c> exceeding clock resolution.
/// </summary>
[Fact]
public async Task SendHeartbeatAsync_UpdatesClientHeartbeatState()
{
ManualTimeProvider clock = new(DateTimeOffset.Parse("2026-05-18T12:00:00Z", System.Globalization.CultureInfo.InvariantCulture));
await using FakeWorkerHarness fakeWorker = await FakeWorkerHarness.CreateConnectedPairAsync();
await using WorkerClient client = fakeWorker.CreateClient();
await using WorkerClient client = fakeWorker.CreateClient(timeProvider: clock);
await StartClientAsync(fakeWorker, client);
DateTimeOffset previousHeartbeat = client.LastHeartbeatAt;
await Task.Delay(TimeSpan.FromMilliseconds(20));
clock.Advance(TimeSpan.FromSeconds(1));
await fakeWorker.SendHeartbeatAsync(
configureHeartbeat: heartbeat => heartbeat.WorkerProcessId = 2468);
@@ -128,6 +133,7 @@ public sealed class FakeWorkerHarnessTests
TestTimeout);
Assert.Equal(WorkerClientState.Ready, client.State);
Assert.Equal(previousHeartbeat + TimeSpan.FromSeconds(1), client.LastHeartbeatAt);
}
/// <summary>Verifies that a hung worker times out pending command invocations.</summary>
@@ -215,4 +221,17 @@ public sealed class FakeWorkerHarnessTests
await Task.Delay(TimeSpan.FromMilliseconds(10), cancellationTokenSource.Token);
}
}
/// <summary>Time provider with a manually advanced clock for deterministic timestamp tests.</summary>
private sealed class ManualTimeProvider(DateTimeOffset start) : TimeProvider
{
private DateTimeOffset _now = start;
/// <inheritdoc />
public override DateTimeOffset GetUtcNow() => _now;
/// <summary>Advances the manual clock by the given amount.</summary>
/// <param name="delta">Amount of time to add to the current clock value.</param>
public void Advance(TimeSpan delta) => _now += delta;
}
}
@@ -71,9 +71,11 @@ public sealed class WorkerClientTests
async () => await timedOutInvokeTask);
Assert.Equal(WorkerClientErrorCode.CommandTimeout, exception.ErrorCode);
// Send the stale reply for the already-timed-out command, then the second
// command's reply. The pipe is FIFO, so the read loop processes (and discards)
// the stale reply before the second reply — no fixed Task.Delay needed.
await pipePair.WorkerWriter.WriteAsync(
CreateCommandReplyEnvelope(timedOutCommand.CorrelationId, MxCommandKind.Ping));
await Task.Delay(TimeSpan.FromMilliseconds(50));
Task<WorkerCommandReply> secondInvokeTask = client.InvokeAsync(
CreateCommand(MxCommandKind.GetWorkerInfo),
@@ -142,7 +144,14 @@ public sealed class WorkerClientTests
Assert.Equal(WorkerClientState.Faulted, client.State);
}
/// <summary>Verifies that the read loop faults the client when the pipe disconnects.</summary>
/// <summary>
/// Verifies that when the client faults it kills the owned worker process.
/// The assertion waits on <see cref="FakeWorkerProcess.WaitForExitAsync"/>, which
/// completes exactly when <c>Kill</c> runs, instead of polling <c>client.State</c>.
/// Polling state is racy: <see cref="WorkerClient.SetFaulted"/> publishes the
/// <c>Faulted</c> state before it calls <c>KillOwnedProcess</c>, so a state-based
/// wait can observe <c>Faulted</c> while <c>KillCount</c> is still 0.
/// </summary>
[Fact]
public async Task ReadLoop_WhenClientFaults_KillsOwnedWorkerProcess()
{
@@ -164,15 +173,77 @@ public sealed class WorkerClientTests
await pipePair.WorkerWriter.WriteAsync(
CreateEventEnvelope(sequence: 12, MxEventFamily.OnDataChange));
await WaitUntilAsync(
() => client.State == WorkerClientState.Faulted,
TestTimeout);
// Deterministic: this completes the instant Kill() runs, with no timing window.
using CancellationTokenSource exitTimeout = new(TestTimeout);
await process.WaitForExitAsync(exitTimeout.Token);
Assert.Equal(WorkerClientState.Faulted, client.State);
Assert.Equal(1, process.KillCount);
Assert.True(process.KillEntireProcessTree);
Assert.True(process.HasExited);
}
/// <summary>
/// Verifies that a worker faulting mid-command — the pipe dropping while an
/// <see cref="WorkerClient.InvokeAsync"/> is still pending — completes the pending
/// invoke task with a <see cref="WorkerClientException"/> carrying the
/// pipe-disconnected error code rather than hanging until the command timeout.
/// </summary>
[Fact]
public async Task InvokeAsync_WhenPipeDisconnectsMidCommand_FailsPendingInvokeWithPipeDisconnected()
{
await using PipePair pipePair = await PipePair.CreateAsync();
await using WorkerClient client = CreateClient(pipePair);
await CompleteHandshakeAsync(client, pipePair);
Task<WorkerCommandReply> invokeTask = client.InvokeAsync(
CreateCommand(MxCommandKind.Ping),
TestTimeout,
CancellationToken.None);
// The worker received the command but disconnects before replying.
WorkerEnvelope commandEnvelope = await pipePair.WorkerReader.ReadAsync().AsTask().WaitAsync(TestTimeout);
Assert.Equal(WorkerEnvelope.BodyOneofCase.WorkerCommand, commandEnvelope.BodyCase);
await pipePair.DisposeWorkerSideAsync();
WorkerClientException exception = await Assert.ThrowsAsync<WorkerClientException>(
async () => await invokeTask.WaitAsync(TestTimeout));
Assert.Equal(WorkerClientErrorCode.PipeDisconnected, exception.ErrorCode);
await WaitUntilAsync(() => client.State == WorkerClientState.Faulted, TestTimeout);
Assert.Equal(WorkerClientState.Faulted, client.State);
}
/// <summary>
/// Verifies that a worker emitting a <c>WorkerFault</c> envelope while an
/// <see cref="WorkerClient.InvokeAsync"/> is pending completes the pending invoke
/// task with a <see cref="WorkerClientException"/> carrying the worker-faulted
/// error code.
/// </summary>
[Fact]
public async Task InvokeAsync_WhenWorkerFaultsMidCommand_FailsPendingInvokeWithWorkerFaulted()
{
await using PipePair pipePair = await PipePair.CreateAsync();
await using WorkerClient client = CreateClient(pipePair);
await CompleteHandshakeAsync(client, pipePair);
Task<WorkerCommandReply> invokeTask = client.InvokeAsync(
CreateCommand(MxCommandKind.Ping),
TestTimeout,
CancellationToken.None);
WorkerEnvelope commandEnvelope = await pipePair.WorkerReader.ReadAsync().AsTask().WaitAsync(TestTimeout);
Assert.Equal(WorkerEnvelope.BodyOneofCase.WorkerCommand, commandEnvelope.BodyCase);
await pipePair.WorkerWriter.WriteAsync(CreateWorkerFaultEnvelope("scripted mid-command fault"));
WorkerClientException exception = await Assert.ThrowsAsync<WorkerClientException>(
async () => await invokeTask.WaitAsync(TestTimeout));
Assert.Equal(WorkerClientErrorCode.WorkerFaulted, exception.ErrorCode);
await WaitUntilAsync(() => client.State == WorkerClientState.Faulted, TestTimeout);
Assert.Equal(WorkerClientState.Faulted, client.State);
}
[Fact]
public async Task ReadLoop_WhenPipeDisconnects_FaultsClient()
{
@@ -244,15 +315,22 @@ public sealed class WorkerClientTests
Assert.True(process.Disposed);
}
/// <summary>
/// Verifies that a heartbeat envelope updates the last-heartbeat timestamp and worker
/// process id. Uses a <see cref="ManualTimeProvider"/> so the timestamp advance is
/// deterministic instead of relying on a wall-clock <c>Task.Delay</c> exceeding
/// <see cref="DateTimeOffset.UtcNow"/> resolution.
/// </summary>
[Fact]
public async Task ReadLoop_WhenHeartbeatArrives_UpdatesLastHeartbeatAndWorkerProcess()
{
ManualTimeProvider clock = new(DateTimeOffset.Parse("2026-05-18T12:00:00Z", System.Globalization.CultureInfo.InvariantCulture));
await using PipePair pipePair = await PipePair.CreateAsync();
await using WorkerClient client = CreateClient(pipePair);
await using WorkerClient client = CreateClient(pipePair, timeProvider: clock);
await CompleteHandshakeAsync(client, pipePair);
DateTimeOffset previousHeartbeat = client.LastHeartbeatAt;
await Task.Delay(TimeSpan.FromMilliseconds(20));
clock.Advance(TimeSpan.FromSeconds(1));
await pipePair.WorkerWriter.WriteAsync(CreateHeartbeatEnvelope(workerProcessId: 9876));
await WaitUntilAsync(
@@ -260,6 +338,7 @@ public sealed class WorkerClientTests
TestTimeout);
Assert.Equal(WorkerClientState.Ready, client.State);
Assert.Equal(previousHeartbeat + TimeSpan.FromSeconds(1), client.LastHeartbeatAt);
}
/// <summary>Verifies that the heartbeat monitor faults the client when the heartbeat expires.</summary>
@@ -288,7 +367,8 @@ public sealed class WorkerClientTests
PipePair pipePair,
WorkerClientOptions? options = null,
GatewayMetrics? metrics = null,
WorkerProcessHandle? processHandle = null)
WorkerProcessHandle? processHandle = null,
TimeProvider? timeProvider = null)
{
WorkerFrameProtocolOptions frameOptions = new(SessionId);
WorkerClientConnection connection = new(
@@ -298,7 +378,7 @@ public sealed class WorkerClientTests
frameOptions,
processHandle);
return new WorkerClient(connection, options, metrics);
return new WorkerClient(connection, options, metrics, timeProvider);
}
private static WorkerProcessHandle CreateProcessHandle(FakeWorkerProcess process)
@@ -399,6 +479,23 @@ public sealed class WorkerClientTests
});
}
private static WorkerEnvelope CreateWorkerFaultEnvelope(string diagnosticMessage)
{
return CreateWorkerEnvelope(
correlationId: string.Empty,
sequence: 30,
envelope => envelope.WorkerFault = new WorkerFault
{
Category = WorkerFaultCategory.MxaccessCommandFailed,
DiagnosticMessage = diagnosticMessage,
ProtocolStatus = new ProtocolStatus
{
Code = ProtocolStatusCode.WorkerUnavailable,
Message = diagnosticMessage,
},
});
}
private static WorkerEnvelope CreateHeartbeatEnvelope(int workerProcessId)
{
return CreateWorkerEnvelope(
@@ -509,6 +606,19 @@ public sealed class WorkerClientTests
}
}
/// <summary>Time provider with a manually advanced clock for deterministic timestamp tests.</summary>
private sealed class ManualTimeProvider(DateTimeOffset start) : TimeProvider
{
private DateTimeOffset _now = start;
/// <inheritdoc />
public override DateTimeOffset GetUtcNow() => _now;
/// <summary>Advances the manual clock by the given amount.</summary>
/// <param name="delta">Amount of time to add to the current clock value.</param>
public void Advance(TimeSpan delta) => _now += delta;
}
private sealed class FakeWorkerProcess : IWorkerProcess
{
private readonly TaskCompletionSource _exited = new(TaskCreationOptions.RunContinuationsAsynchronously);
@@ -6,8 +6,9 @@ using MxGateway.Server.Security.Authentication;
namespace MxGateway.Tests.Security.Authentication;
public sealed class ApiKeyAdminCliRunnerTests
public sealed class ApiKeyAdminCliRunnerTests : IDisposable
{
private readonly List<TempDatabaseDirectory> _tempDirectories = [];
/// <summary>Verifies that CreateKeyAsync creates an authenticating key and audits the action.</summary>
[Fact]
public async Task CreateKeyAsync_CreatesAuthenticatingKeyAndAudits()
@@ -249,12 +250,23 @@ public sealed class ApiKeyAdminCliRunnerTests
return services.BuildServiceProvider(validateScopes: true);
}
private static string CreateTempDatabasePath()
/// <summary>Clears SQLite pools and deletes every temporary directory created by this test.</summary>
public void Dispose()
{
string directory = Path.Combine(Path.GetTempPath(), "mxgateway-auth-cli-tests", Guid.NewGuid().ToString("N"));
Directory.CreateDirectory(directory);
foreach (TempDatabaseDirectory directory in _tempDirectories)
{
directory.Dispose();
}
return Path.Combine(directory, "gateway-auth.db");
_tempDirectories.Clear();
}
private string CreateTempDatabasePath()
{
TempDatabaseDirectory directory = TempDatabaseDirectory.Create("mxgateway-auth-cli-tests");
_tempDirectories.Add(directory);
return directory.DatabasePath();
}
private static string ReadApiKey(string json)
@@ -11,8 +11,9 @@ namespace MxGateway.Tests.Security.Authentication;
/// <summary>
/// Tests for <see cref="SqliteAuthStore"/>.
/// </summary>
public sealed class SqliteAuthStoreTests
public sealed class SqliteAuthStoreTests : IDisposable
{
private readonly List<TempDatabaseDirectory> _tempDirectories = [];
/// <summary>
/// Verifies that MigrateAsync initializes the database schema.
/// </summary>
@@ -167,12 +168,23 @@ public sealed class SqliteAuthStoreTests
return services.BuildServiceProvider(validateScopes: true);
}
private static string CreateTempDatabasePath()
/// <summary>Clears SQLite pools and deletes every temporary directory created by this test.</summary>
public void Dispose()
{
string directory = Path.Combine(Path.GetTempPath(), "mxgateway-auth-tests", Guid.NewGuid().ToString("N"));
Directory.CreateDirectory(directory);
foreach (TempDatabaseDirectory directory in _tempDirectories)
{
directory.Dispose();
}
return Path.Combine(directory, "gateway-auth.db");
_tempDirectories.Clear();
}
private string CreateTempDatabasePath()
{
TempDatabaseDirectory directory = TempDatabaseDirectory.Create("mxgateway-auth-tests");
_tempDirectories.Add(directory);
return directory.DatabasePath();
}
private static async Task CreateVersionZeroDatabaseAsync(string databasePath)
@@ -0,0 +1,73 @@
using Microsoft.Data.Sqlite;
namespace MxGateway.Tests.Security.Authentication;
/// <summary>
/// Disposable temporary directory for SQLite auth-store tests. Each instance owns a
/// unique directory under <c>%TEMP%</c>; <see cref="Dispose"/> clears SQLite connection
/// pools (which otherwise keep the <c>.db</c> file handle open) and deletes the directory
/// so test runs do not leak temp files or open handles.
/// </summary>
internal sealed class TempDatabaseDirectory : IDisposable
{
private bool _disposed;
private TempDatabaseDirectory(string path)
{
Path = path;
}
/// <summary>Gets the path to the temporary directory.</summary>
public string Path { get; }
/// <summary>Creates a new uniquely named temporary directory under the given prefix.</summary>
/// <param name="prefix">Folder name placed under <c>%TEMP%</c> to group related test directories.</param>
public static TempDatabaseDirectory Create(string prefix)
{
string path = System.IO.Path.Combine(
System.IO.Path.GetTempPath(),
prefix,
Guid.NewGuid().ToString("N"));
Directory.CreateDirectory(path);
return new TempDatabaseDirectory(path);
}
/// <summary>Returns a database file path inside this temporary directory.</summary>
/// <param name="fileName">Database file name; defaults to the gateway auth database name.</param>
public string DatabasePath(string fileName = "gateway-auth.db")
{
return System.IO.Path.Combine(Path, fileName);
}
/// <inheritdoc />
public void Dispose()
{
if (_disposed)
{
return;
}
_disposed = true;
// Microsoft.Data.Sqlite pools connections by default; clear the pools so the
// underlying file handle is released before the directory is deleted.
SqliteConnection.ClearAllPools();
try
{
if (Directory.Exists(Path))
{
Directory.Delete(Path, recursive: true);
}
}
catch (IOException)
{
// Best-effort cleanup; a transient handle should not fail the test.
}
catch (UnauthorizedAccessException)
{
// Best-effort cleanup; a transient handle should not fail the test.
}
}
}
@@ -1,9 +1,15 @@
using System.Runtime.CompilerServices;
using Grpc.Core;
using Microsoft.Extensions.Logging.Abstractions;
using Microsoft.Extensions.Options;
using MxGateway.Contracts;
using MxGateway.Contracts.Proto;
using MxGateway.Server.Configuration;
using MxGateway.Server.Grpc;
using MxGateway.Server.Metrics;
using MxGateway.Server.Security.Authentication;
using MxGateway.Server.Security.Authorization;
using MxGateway.Server.Sessions;
namespace MxGateway.Tests.Security.Authorization;
@@ -156,6 +162,110 @@ public sealed class GatewayGrpcAuthorizationInterceptorTests
Assert.Null(identityAccessor.Current);
}
/// <summary>
/// End-to-end composition test: runs an <c>OpenSession</c> call through the real
/// interceptor in front of the real <see cref="MxAccessGatewayService"/> with a key
/// that lacks the <c>session:open</c> scope, and asserts the interceptor denies the
/// call with <see cref="StatusCode.PermissionDenied"/> before the service runs.
/// </summary>
[Fact]
public async Task InterceptorComposedWithService_OpenSessionMissingScope_DeniesBeforeServiceRuns()
{
GatewayRequestIdentityAccessor identityAccessor = new();
RecordingSessionManager sessionManager = new();
GatewayGrpcAuthorizationInterceptor interceptor = CreateInterceptor(
new FakeApiKeyVerifier(SuccessWithScopes(GatewayScopes.EventsRead)),
identityAccessor);
MxAccessGatewayService service = CreateService(sessionManager, identityAccessor);
RpcException exception = await Assert.ThrowsAsync<RpcException>(
() => interceptor.UnaryServerHandler(
new OpenSessionRequest { ClientSessionName = "operator-session" },
ContextWithAuthorization("Bearer mxgw_operator01_secret"),
(request, context) => service.OpenSession(request, context)));
Assert.Equal(StatusCode.PermissionDenied, exception.StatusCode);
Assert.Contains(GatewayScopes.SessionOpen, exception.Status.Detail, StringComparison.Ordinal);
Assert.Equal(0, sessionManager.OpenSessionCount);
}
/// <summary>
/// End-to-end composition test: runs an <c>OpenSession</c> call through the real
/// interceptor in front of the real <see cref="MxAccessGatewayService"/> with a key
/// that holds <c>session:open</c>, and asserts the service runs and observes the
/// interceptor-supplied identity.
/// </summary>
[Fact]
public async Task InterceptorComposedWithService_OpenSessionWithScope_RunsServiceWithIdentity()
{
GatewayRequestIdentityAccessor identityAccessor = new();
RecordingSessionManager sessionManager = new();
GatewayGrpcAuthorizationInterceptor interceptor = CreateInterceptor(
new FakeApiKeyVerifier(SuccessWithScopes(GatewayScopes.SessionOpen)),
identityAccessor);
MxAccessGatewayService service = CreateService(sessionManager, identityAccessor);
OpenSessionReply reply = await interceptor.UnaryServerHandler(
new OpenSessionRequest { ClientSessionName = "operator-session" },
ContextWithAuthorization("Bearer mxgw_operator01_secret"),
(request, context) => service.OpenSession(request, context));
Assert.Equal("session-1", reply.SessionId);
Assert.Equal(1, sessionManager.OpenSessionCount);
Assert.Equal("Operator Key", sessionManager.LastClientIdentity);
}
/// <summary>
/// End-to-end composition test: an <c>Invoke</c> call through the real interceptor in
/// front of the real service with a key holding only <c>invoke:read</c> is denied
/// because the wrapped command is a write, confirming command-scope mapping is
/// enforced through the full composition.
/// </summary>
[Fact]
public async Task InterceptorComposedWithService_InvokeWriteCommandWithReadScope_DeniesBeforeServiceRuns()
{
GatewayRequestIdentityAccessor identityAccessor = new();
RecordingSessionManager sessionManager = new();
GatewayGrpcAuthorizationInterceptor interceptor = CreateInterceptor(
new FakeApiKeyVerifier(SuccessWithScopes(GatewayScopes.InvokeRead)),
identityAccessor);
MxAccessGatewayService service = CreateService(sessionManager, identityAccessor);
MxCommandRequest request = new()
{
SessionId = "session-1",
Command = new MxCommand
{
Kind = MxCommandKind.Write,
Write = new WriteCommand { ServerHandle = 1, ItemHandle = 2 },
},
};
RpcException exception = await Assert.ThrowsAsync<RpcException>(
() => interceptor.UnaryServerHandler(
request,
ContextWithAuthorization("Bearer mxgw_operator01_secret"),
(req, context) => service.Invoke(req, context)));
Assert.Equal(StatusCode.PermissionDenied, exception.StatusCode);
Assert.Contains(GatewayScopes.InvokeWrite, exception.Status.Detail, StringComparison.Ordinal);
Assert.Equal(0, sessionManager.InvokeCount);
}
private static MxAccessGatewayService CreateService(
ISessionManager sessionManager,
IGatewayRequestIdentityAccessor identityAccessor)
{
return new MxAccessGatewayService(
sessionManager,
identityAccessor,
new AllowAllConstraintEnforcer(),
new MxAccessGrpcRequestValidator(),
new MxAccessGrpcMapper(),
new NoOpEventStreamService(),
new GatewayMetrics(),
NullLogger<MxAccessGatewayService>.Instance);
}
private static GatewayGrpcAuthorizationInterceptor CreateInterceptor(
IApiKeyVerifier apiKeyVerifier,
IGatewayRequestIdentityAccessor identityAccessor,
@@ -188,6 +298,138 @@ public sealed class GatewayGrpcAuthorizationInterceptorTests
return new TestServerCallContext([new Metadata.Entry("authorization", authorizationHeader)]);
}
/// <summary>Records whether the gateway service ran past the interceptor for composition tests.</summary>
private sealed class RecordingSessionManager : ISessionManager
{
/// <summary>Gets the number of times OpenSessionAsync was invoked.</summary>
public int OpenSessionCount { get; private set; }
/// <summary>Gets the number of times InvokeAsync was invoked.</summary>
public int InvokeCount { get; private set; }
/// <summary>Gets the last client identity passed to OpenSessionAsync.</summary>
public string? LastClientIdentity { get; private set; }
/// <inheritdoc />
public Task<GatewaySession> OpenSessionAsync(
SessionOpenRequest request,
string? clientIdentity,
CancellationToken cancellationToken)
{
OpenSessionCount++;
LastClientIdentity = clientIdentity;
GatewaySession session = new(
"session-1",
GatewayContractInfo.DefaultBackendName,
"pipe",
"nonce",
clientIdentity ?? "client",
"client-session",
"client-correlation",
TimeSpan.FromSeconds(7),
TimeSpan.FromSeconds(30),
TimeSpan.FromSeconds(10),
DateTimeOffset.UtcNow);
return Task.FromResult(session);
}
/// <inheritdoc />
public bool TryGetSession(string sessionId, out GatewaySession session)
{
session = null!;
return false;
}
/// <inheritdoc />
public Task<WorkerCommandReply> InvokeAsync(
string sessionId,
WorkerCommand command,
CancellationToken cancellationToken)
{
InvokeCount++;
return Task.FromResult(new WorkerCommandReply());
}
/// <inheritdoc />
public IAsyncEnumerable<WorkerEvent> ReadEventsAsync(
string sessionId,
CancellationToken cancellationToken)
{
return AsyncEnumerable.Empty<WorkerEvent>();
}
/// <inheritdoc />
public Task<SessionCloseResult> CloseSessionAsync(
string sessionId,
CancellationToken cancellationToken)
{
return Task.FromResult(new SessionCloseResult(sessionId, SessionState.Closed, AlreadyClosed: false));
}
/// <inheritdoc />
public Task<int> CloseExpiredLeasesAsync(
DateTimeOffset now,
CancellationToken cancellationToken)
{
return Task.FromResult(0);
}
/// <inheritdoc />
public Task ShutdownAsync(CancellationToken cancellationToken)
{
return Task.CompletedTask;
}
}
/// <summary>Event stream service that yields nothing; alarm/event RPCs are not under test here.</summary>
private sealed class NoOpEventStreamService : IEventStreamService
{
/// <inheritdoc />
public async IAsyncEnumerable<MxEvent> StreamEventsAsync(
StreamEventsRequest request,
[EnumeratorCancellation] CancellationToken cancellationToken)
{
await Task.CompletedTask;
yield break;
}
}
/// <summary>Constraint enforcer that permits every operation for composition tests.</summary>
private sealed class AllowAllConstraintEnforcer : IConstraintEnforcer
{
/// <inheritdoc />
public Task<ConstraintFailure?> CheckReadTagAsync(
ApiKeyIdentity? identity,
string tagAddress,
CancellationToken cancellationToken) => Task.FromResult<ConstraintFailure?>(null);
/// <inheritdoc />
public Task<ConstraintFailure?> CheckReadHandleAsync(
ApiKeyIdentity? identity,
GatewaySession session,
int serverHandle,
int itemHandle,
CancellationToken cancellationToken) => Task.FromResult<ConstraintFailure?>(null);
/// <inheritdoc />
public Task<ConstraintFailure?> CheckWriteHandleAsync(
ApiKeyIdentity? identity,
GatewaySession session,
int serverHandle,
int itemHandle,
CancellationToken cancellationToken) => Task.FromResult<ConstraintFailure?>(null);
/// <inheritdoc />
public Task RecordDenialAsync(
ApiKeyIdentity? identity,
string commandKind,
string target,
ConstraintFailure failure,
CancellationToken cancellationToken) => Task.CompletedTask;
}
private sealed class FakeApiKeyVerifier(ApiKeyVerificationResult result) : IApiKeyVerifier
{
/// <summary>Gets whether the verifier was called.</summary>
@@ -61,4 +61,42 @@ public sealed class GatewayGrpcScopeResolverTests
Assert.Equal(expectedScope, scope);
}
/// <summary>
/// Verifies that an unmapped request type fails closed: the resolver returns the
/// most-restrictive <see cref="GatewayScopes.Admin"/> scope rather than a permissive
/// default, so a newly added RPC that is never mapped is denied to ordinary keys.
/// </summary>
[Fact]
public void ResolveRequiredScope_UnmappedRequestType_FailsClosedToAdminScope()
{
GatewayGrpcScopeResolver resolver = new();
string scope = resolver.ResolveRequiredScope(new UnmappedRequest());
Assert.Equal(GatewayScopes.Admin, scope);
}
/// <summary>
/// Verifies that an <see cref="MxCommandRequest"/> with an unrecognized command kind
/// resolves to the read scope rather than silently granting write or admin access.
/// </summary>
[Fact]
public void ResolveRequiredScope_UnknownInvokeCommandKind_ReturnsInvokeReadScope()
{
GatewayGrpcScopeResolver resolver = new();
string scope = resolver.ResolveRequiredScope(new MxCommandRequest
{
Command = new MxCommand
{
Kind = (MxCommandKind)9999,
},
});
Assert.Equal(GatewayScopes.InvokeRead, scope);
}
/// <summary>Request type intentionally not mapped by the scope resolver.</summary>
private sealed class UnmappedRequest;
}