diff --git a/code-reviews/Tests/findings.md b/code-reviews/Tests/findings.md index 225d577..63f180e 100644 --- a/code-reviews/Tests/findings.md +++ b/code-reviews/Tests/findings.md @@ -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\` (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 diff --git a/src/MxGateway.Tests/Gateway/Workers/FakeWorkerHarnessTests.cs b/src/MxGateway.Tests/Gateway/Workers/FakeWorkerHarnessTests.cs index 4a92174..5da5caa 100644 --- a/src/MxGateway.Tests/Gateway/Workers/FakeWorkerHarnessTests.cs +++ b/src/MxGateway.Tests/Gateway/Workers/FakeWorkerHarnessTests.cs @@ -110,16 +110,21 @@ public sealed class FakeWorkerHarnessTests Assert.Equal(WorkerClientState.Faulted, client.State); } - /// Verifies that sending a heartbeat updates the client heartbeat state. + /// + /// Verifies that sending a heartbeat updates the client heartbeat state. Uses a + /// so the timestamp advance is deterministic rather + /// than relying on a wall-clock Task.Delay exceeding clock resolution. + /// [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); } /// Verifies that a hung worker times out pending command invocations. @@ -215,4 +221,17 @@ public sealed class FakeWorkerHarnessTests await Task.Delay(TimeSpan.FromMilliseconds(10), cancellationTokenSource.Token); } } + + /// Time provider with a manually advanced clock for deterministic timestamp tests. + private sealed class ManualTimeProvider(DateTimeOffset start) : TimeProvider + { + private DateTimeOffset _now = start; + + /// + public override DateTimeOffset GetUtcNow() => _now; + + /// Advances the manual clock by the given amount. + /// Amount of time to add to the current clock value. + public void Advance(TimeSpan delta) => _now += delta; + } } diff --git a/src/MxGateway.Tests/Gateway/Workers/WorkerClientTests.cs b/src/MxGateway.Tests/Gateway/Workers/WorkerClientTests.cs index 862191b..2974c48 100644 --- a/src/MxGateway.Tests/Gateway/Workers/WorkerClientTests.cs +++ b/src/MxGateway.Tests/Gateway/Workers/WorkerClientTests.cs @@ -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 secondInvokeTask = client.InvokeAsync( CreateCommand(MxCommandKind.GetWorkerInfo), @@ -142,7 +144,14 @@ public sealed class WorkerClientTests Assert.Equal(WorkerClientState.Faulted, client.State); } - /// Verifies that the read loop faults the client when the pipe disconnects. + /// + /// Verifies that when the client faults it kills the owned worker process. + /// The assertion waits on , which + /// completes exactly when Kill runs, instead of polling client.State. + /// Polling state is racy: publishes the + /// Faulted state before it calls KillOwnedProcess, so a state-based + /// wait can observe Faulted while KillCount is still 0. + /// [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); } + /// + /// Verifies that a worker faulting mid-command — the pipe dropping while an + /// is still pending — completes the pending + /// invoke task with a carrying the + /// pipe-disconnected error code rather than hanging until the command timeout. + /// + [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 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( + async () => await invokeTask.WaitAsync(TestTimeout)); + + Assert.Equal(WorkerClientErrorCode.PipeDisconnected, exception.ErrorCode); + await WaitUntilAsync(() => client.State == WorkerClientState.Faulted, TestTimeout); + Assert.Equal(WorkerClientState.Faulted, client.State); + } + + /// + /// Verifies that a worker emitting a WorkerFault envelope while an + /// is pending completes the pending invoke + /// task with a carrying the worker-faulted + /// error code. + /// + [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 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( + 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); } + /// + /// Verifies that a heartbeat envelope updates the last-heartbeat timestamp and worker + /// process id. Uses a so the timestamp advance is + /// deterministic instead of relying on a wall-clock Task.Delay exceeding + /// resolution. + /// [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); } /// Verifies that the heartbeat monitor faults the client when the heartbeat expires. @@ -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 } } + /// Time provider with a manually advanced clock for deterministic timestamp tests. + private sealed class ManualTimeProvider(DateTimeOffset start) : TimeProvider + { + private DateTimeOffset _now = start; + + /// + public override DateTimeOffset GetUtcNow() => _now; + + /// Advances the manual clock by the given amount. + /// Amount of time to add to the current clock value. + public void Advance(TimeSpan delta) => _now += delta; + } + private sealed class FakeWorkerProcess : IWorkerProcess { private readonly TaskCompletionSource _exited = new(TaskCreationOptions.RunContinuationsAsynchronously); diff --git a/src/MxGateway.Tests/Security/Authentication/ApiKeyAdminCliRunnerTests.cs b/src/MxGateway.Tests/Security/Authentication/ApiKeyAdminCliRunnerTests.cs index 80399d5..2b7c977 100644 --- a/src/MxGateway.Tests/Security/Authentication/ApiKeyAdminCliRunnerTests.cs +++ b/src/MxGateway.Tests/Security/Authentication/ApiKeyAdminCliRunnerTests.cs @@ -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 _tempDirectories = []; /// Verifies that CreateKeyAsync creates an authenticating key and audits the action. [Fact] public async Task CreateKeyAsync_CreatesAuthenticatingKeyAndAudits() @@ -249,12 +250,23 @@ public sealed class ApiKeyAdminCliRunnerTests return services.BuildServiceProvider(validateScopes: true); } - private static string CreateTempDatabasePath() + /// Clears SQLite pools and deletes every temporary directory created by this test. + 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) diff --git a/src/MxGateway.Tests/Security/Authentication/SqliteAuthStoreTests.cs b/src/MxGateway.Tests/Security/Authentication/SqliteAuthStoreTests.cs index 7602cc3..752f1ab 100644 --- a/src/MxGateway.Tests/Security/Authentication/SqliteAuthStoreTests.cs +++ b/src/MxGateway.Tests/Security/Authentication/SqliteAuthStoreTests.cs @@ -11,8 +11,9 @@ namespace MxGateway.Tests.Security.Authentication; /// /// Tests for . /// -public sealed class SqliteAuthStoreTests +public sealed class SqliteAuthStoreTests : IDisposable { + private readonly List _tempDirectories = []; /// /// Verifies that MigrateAsync initializes the database schema. /// @@ -167,12 +168,23 @@ public sealed class SqliteAuthStoreTests return services.BuildServiceProvider(validateScopes: true); } - private static string CreateTempDatabasePath() + /// Clears SQLite pools and deletes every temporary directory created by this test. + 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) diff --git a/src/MxGateway.Tests/Security/Authentication/TempDatabaseDirectory.cs b/src/MxGateway.Tests/Security/Authentication/TempDatabaseDirectory.cs new file mode 100644 index 0000000..1fb28f5 --- /dev/null +++ b/src/MxGateway.Tests/Security/Authentication/TempDatabaseDirectory.cs @@ -0,0 +1,73 @@ +using Microsoft.Data.Sqlite; + +namespace MxGateway.Tests.Security.Authentication; + +/// +/// Disposable temporary directory for SQLite auth-store tests. Each instance owns a +/// unique directory under %TEMP%; clears SQLite connection +/// pools (which otherwise keep the .db file handle open) and deletes the directory +/// so test runs do not leak temp files or open handles. +/// +internal sealed class TempDatabaseDirectory : IDisposable +{ + private bool _disposed; + + private TempDatabaseDirectory(string path) + { + Path = path; + } + + /// Gets the path to the temporary directory. + public string Path { get; } + + /// Creates a new uniquely named temporary directory under the given prefix. + /// Folder name placed under %TEMP% to group related test directories. + 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); + } + + /// Returns a database file path inside this temporary directory. + /// Database file name; defaults to the gateway auth database name. + public string DatabasePath(string fileName = "gateway-auth.db") + { + return System.IO.Path.Combine(Path, fileName); + } + + /// + 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. + } + } +} diff --git a/src/MxGateway.Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs b/src/MxGateway.Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs index 718593c..6e5779d 100644 --- a/src/MxGateway.Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs +++ b/src/MxGateway.Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs @@ -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); } + /// + /// End-to-end composition test: runs an OpenSession call through the real + /// interceptor in front of the real with a key + /// that lacks the session:open scope, and asserts the interceptor denies the + /// call with before the service runs. + /// + [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( + () => 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); + } + + /// + /// End-to-end composition test: runs an OpenSession call through the real + /// interceptor in front of the real with a key + /// that holds session:open, and asserts the service runs and observes the + /// interceptor-supplied identity. + /// + [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); + } + + /// + /// End-to-end composition test: an Invoke call through the real interceptor in + /// front of the real service with a key holding only invoke:read is denied + /// because the wrapped command is a write, confirming command-scope mapping is + /// enforced through the full composition. + /// + [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( + () => 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.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)]); } + /// Records whether the gateway service ran past the interceptor for composition tests. + private sealed class RecordingSessionManager : ISessionManager + { + /// Gets the number of times OpenSessionAsync was invoked. + public int OpenSessionCount { get; private set; } + + /// Gets the number of times InvokeAsync was invoked. + public int InvokeCount { get; private set; } + + /// Gets the last client identity passed to OpenSessionAsync. + public string? LastClientIdentity { get; private set; } + + /// + public Task 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); + } + + /// + public bool TryGetSession(string sessionId, out GatewaySession session) + { + session = null!; + return false; + } + + /// + public Task InvokeAsync( + string sessionId, + WorkerCommand command, + CancellationToken cancellationToken) + { + InvokeCount++; + return Task.FromResult(new WorkerCommandReply()); + } + + /// + public IAsyncEnumerable ReadEventsAsync( + string sessionId, + CancellationToken cancellationToken) + { + return AsyncEnumerable.Empty(); + } + + /// + public Task CloseSessionAsync( + string sessionId, + CancellationToken cancellationToken) + { + return Task.FromResult(new SessionCloseResult(sessionId, SessionState.Closed, AlreadyClosed: false)); + } + + /// + public Task CloseExpiredLeasesAsync( + DateTimeOffset now, + CancellationToken cancellationToken) + { + return Task.FromResult(0); + } + + /// + public Task ShutdownAsync(CancellationToken cancellationToken) + { + return Task.CompletedTask; + } + } + + /// Event stream service that yields nothing; alarm/event RPCs are not under test here. + private sealed class NoOpEventStreamService : IEventStreamService + { + /// + public async IAsyncEnumerable StreamEventsAsync( + StreamEventsRequest request, + [EnumeratorCancellation] CancellationToken cancellationToken) + { + await Task.CompletedTask; + yield break; + } + } + + /// Constraint enforcer that permits every operation for composition tests. + private sealed class AllowAllConstraintEnforcer : IConstraintEnforcer + { + /// + public Task CheckReadTagAsync( + ApiKeyIdentity? identity, + string tagAddress, + CancellationToken cancellationToken) => Task.FromResult(null); + + /// + public Task CheckReadHandleAsync( + ApiKeyIdentity? identity, + GatewaySession session, + int serverHandle, + int itemHandle, + CancellationToken cancellationToken) => Task.FromResult(null); + + /// + public Task CheckWriteHandleAsync( + ApiKeyIdentity? identity, + GatewaySession session, + int serverHandle, + int itemHandle, + CancellationToken cancellationToken) => Task.FromResult(null); + + /// + public Task RecordDenialAsync( + ApiKeyIdentity? identity, + string commandKind, + string target, + ConstraintFailure failure, + CancellationToken cancellationToken) => Task.CompletedTask; + } + private sealed class FakeApiKeyVerifier(ApiKeyVerificationResult result) : IApiKeyVerifier { /// Gets whether the verifier was called. diff --git a/src/MxGateway.Tests/Security/Authorization/GatewayGrpcScopeResolverTests.cs b/src/MxGateway.Tests/Security/Authorization/GatewayGrpcScopeResolverTests.cs index 2df832c..ca1ad6f 100644 --- a/src/MxGateway.Tests/Security/Authorization/GatewayGrpcScopeResolverTests.cs +++ b/src/MxGateway.Tests/Security/Authorization/GatewayGrpcScopeResolverTests.cs @@ -61,4 +61,42 @@ public sealed class GatewayGrpcScopeResolverTests Assert.Equal(expectedScope, scope); } + + /// + /// Verifies that an unmapped request type fails closed: the resolver returns the + /// most-restrictive scope rather than a permissive + /// default, so a newly added RPC that is never mapped is denied to ordinary keys. + /// + [Fact] + public void ResolveRequiredScope_UnmappedRequestType_FailsClosedToAdminScope() + { + GatewayGrpcScopeResolver resolver = new(); + + string scope = resolver.ResolveRequiredScope(new UnmappedRequest()); + + Assert.Equal(GatewayScopes.Admin, scope); + } + + /// + /// Verifies that an with an unrecognized command kind + /// resolves to the read scope rather than silently granting write or admin access. + /// + [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); + } + + /// Request type intentionally not mapped by the scope resolver. + private sealed class UnmappedRequest; }