From 7981c18041d54a7c9fe19f17833f2908ab77d9b5 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Fri, 10 Apr 2026 00:56:56 +0100 Subject: [PATCH 1/3] Add SignalR query-string auth, multi-endpoint 401, and scaling doc tests Enhance OAuthTokenLifecycleTests with: - SignalR query-string auth tests using raw HTTP POST to /hubs/boards/negotiate with ?access_token= parameter, exercising the OnMessageReceived handler - Expired JWT multi-endpoint Theory test proving systemic 401 + ApiErrorResponse contract across /api/boards, /api/capture/items, /api/auth/change-password - Scaling limitation documentation test verifying the static ConcurrentDictionary is process-scoped, with inline documentation referencing #676 - Improved section headers and class-level summary Part of #723 (TST-55) --- .../OAuthTokenLifecycleTests.cs | 191 +++++++++++++++++- 1 file changed, 180 insertions(+), 11 deletions(-) diff --git a/backend/tests/Taskdeck.Api.Tests/OAuthTokenLifecycleTests.cs b/backend/tests/Taskdeck.Api.Tests/OAuthTokenLifecycleTests.cs index e463da238..cebf032eb 100644 --- a/backend/tests/Taskdeck.Api.Tests/OAuthTokenLifecycleTests.cs +++ b/backend/tests/Taskdeck.Api.Tests/OAuthTokenLifecycleTests.cs @@ -19,9 +19,14 @@ namespace Taskdeck.Api.Tests; /// /// Integration tests for OAuth auth code store behavior and JWT token lifecycle. -/// Covers scenarios from #723 (TST-55): static auth code store, token issuance, -/// validation, expiration, wrong-key rejection, deactivated user, SignalR query-string auth, -/// and code cleanup semantics. +/// Covers scenarios from #723 (TST-55): +/// +/// Section 1: Auth code store — exchange, replay prevention, expiry, concurrent exchange, cleanup +/// Section 2: JWT token lifecycle — expired, wrong key, garbage, deactivated user, password reissue +/// Section 3: SignalR auth — header-based (HubConnection) AND query-string (?access_token=) paths +/// Section 4: Expired JWT across multiple endpoints (systemic 401 + ApiErrorResponse contract) +/// Section 5: GitHub OAuth config endpoints (login redirect, providers) +/// Section 6: Scaling limitation documentation — static ConcurrentDictionary is process-scoped (#676) /// public class OAuthTokenLifecycleTests : IClassFixture { @@ -314,14 +319,8 @@ public async Task ReissuedTokenAfterPasswordChange_CanAccessEndpoint() } // ───────────────────────────────────────────────────────── - // 3. SignalR auth + // 3. SignalR auth — header-based (HubConnection via AccessTokenProvider) // ───────────────────────────────────────────────────────── - // Note: The .NET SignalR client with HttpMessageHandlerFactory (in-process - // test transport) sends tokens via the Authorization header, not via the - // ?access_token= query string. These tests verify SignalR endpoint auth - // generally but do NOT exercise the OnMessageReceived query-string - // extraction path in AuthenticationRegistration.cs. True query-string - // auth testing would require WebSocket transport or manual URL construction. [Fact] public async Task SignalR_AcceptsValidJwt() @@ -380,7 +379,126 @@ await act.Should().ThrowAsync() } // ───────────────────────────────────────────────────────── - // 4. GitHub OAuth config endpoints + // 3b. SignalR query-string auth — exercises the OnMessageReceived + // handler in AuthenticationRegistration.cs that extracts + // ?access_token= from the query string for WebSocket connections. + // Uses raw HTTP POST to /hubs/boards/negotiate to bypass the + // .NET HubConnection client (which always uses the Authorization header). + // ───────────────────────────────────────────────────────── + + [Fact] + public async Task SignalR_QueryStringAuth_ValidTokenAccepted() + { + using var client = _factory.CreateClient(); + var user = await ApiTestHarness.AuthenticateAsync(client, "signalr-qsv"); + + // POST to the negotiate endpoint with the token in the query string, + // NOT in the Authorization header. This exercises the OnMessageReceived + // handler that extracts access_token from Request.Query. + using var rawClient = _factory.CreateClient(); + var negotiateUrl = $"/hubs/boards/negotiate?access_token={Uri.EscapeDataString(user.Token)}"; + var response = await rawClient.PostAsync(negotiateUrl, null); + + response.StatusCode.Should().Be(HttpStatusCode.OK, + "SignalR negotiate should accept a valid JWT passed via ?access_token= query string"); + } + + [Fact] + public async Task SignalR_QueryStringAuth_ExpiredTokenRejected() + { + using var client = _factory.CreateClient(); + var user = await ApiTestHarness.AuthenticateAsync(client, "signalr-qse"); + + var expiredToken = CreateCustomJwt( + userId: user.UserId, + username: user.Username, + email: user.Email, + secretKey: "TaskdeckDevelopmentOnlySecretKeyChangeMe123!", + issuer: "Taskdeck", + audience: "TaskdeckUsers", + expiresIn: TimeSpan.FromMinutes(-5)); + + using var rawClient = _factory.CreateClient(); + var negotiateUrl = $"/hubs/boards/negotiate?access_token={Uri.EscapeDataString(expiredToken)}"; + var response = await rawClient.PostAsync(negotiateUrl, null); + + response.StatusCode.Should().Be(HttpStatusCode.Unauthorized, + "SignalR negotiate should reject an expired JWT passed via ?access_token= query string"); + } + + [Fact] + public async Task SignalR_QueryStringAuth_WrongKeyRejected() + { + using var client = _factory.CreateClient(); + var user = await ApiTestHarness.AuthenticateAsync(client, "signalr-qsk"); + + var wrongKeyToken = CreateCustomJwt( + userId: user.UserId, + username: user.Username, + email: user.Email, + secretKey: "CompletelyDifferentWrongSecretKey_Padding1234!", + issuer: "Taskdeck", + audience: "TaskdeckUsers", + expiresIn: TimeSpan.FromMinutes(60)); + + using var rawClient = _factory.CreateClient(); + var negotiateUrl = $"/hubs/boards/negotiate?access_token={Uri.EscapeDataString(wrongKeyToken)}"; + var response = await rawClient.PostAsync(negotiateUrl, null); + + response.StatusCode.Should().Be(HttpStatusCode.Unauthorized, + "SignalR negotiate should reject a JWT signed with the wrong key via ?access_token= query string"); + } + + [Fact] + public async Task SignalR_QueryStringAuth_NoTokenRejected() + { + // POST to negotiate without any token at all — should be rejected + using var rawClient = _factory.CreateClient(); + var response = await rawClient.PostAsync("/hubs/boards/negotiate", null); + + response.StatusCode.Should().Be(HttpStatusCode.Unauthorized, + "SignalR negotiate should reject unauthenticated requests"); + } + + // ───────────────────────────────────────────────────────── + // 4. Expired JWT → multiple endpoints return 401 + // Verifies that the 401 + ApiErrorResponse contract is + // not accidental to one controller, but systemic. + // ───────────────────────────────────────────────────────── + + [Theory] + [InlineData("/api/boards")] + [InlineData("/api/capture/items")] + [InlineData("/api/auth/change-password")] + public async Task ExpiredJwt_MultipleEndpoints_Return401(string endpoint) + { + using var client = _factory.CreateClient(); + var user = await ApiTestHarness.AuthenticateAsync(client, "tok-multi"); + + var expiredToken = CreateCustomJwt( + userId: user.UserId, + username: user.Username, + email: user.Email, + secretKey: "TaskdeckDevelopmentOnlySecretKeyChangeMe123!", + issuer: "Taskdeck", + audience: "TaskdeckUsers", + expiresIn: TimeSpan.FromMinutes(-5)); + + using var expiredClient = _factory.CreateClient(); + expiredClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", expiredToken); + + // Use POST for change-password; GET for everything else. Either way should 401 before body validation. + var response = endpoint.Contains("change-password") + ? await expiredClient.PostAsJsonAsync(endpoint, new { CurrentPassword = "x", NewPassword = "y" }) + : await expiredClient.GetAsync(endpoint); + + response.StatusCode.Should().Be(HttpStatusCode.Unauthorized, + $"endpoint {endpoint} should reject expired JWT"); + await ApiTestHarness.AssertErrorContractAsync(response, HttpStatusCode.Unauthorized); + } + + // ───────────────────────────────────────────────────────── + // 5. GitHub OAuth config endpoints // ───────────────────────────────────────────────────────── [Fact] @@ -409,6 +527,57 @@ public async Task Providers_ReturnsGitHubStatus() "gitHub property should be a boolean"); } + // ───────────────────────────────────────────────────────── + // 6. Scaling limitation documentation + // ───────────────────────────────────────────────────────── + // + // IMPORTANT: The OAuth auth code store uses a static ConcurrentDictionary + // on AuthController. This has the following implications: + // + // 1. Codes created on one application instance are invisible to other instances. + // In a horizontally-scaled deployment (multiple pods/containers), a code generated + // by pod A cannot be exchanged on pod B. This breaks the OAuth flow unless sticky + // sessions or a shared store (Redis, database) are used. + // + // 2. CleanupExpiredCodes() is only called during ExchangeCode, not on a timer. + // Expired codes accumulate indefinitely until an exchange attempt triggers cleanup. + // Under low traffic this is a minor memory leak. + // + // 3. Codes persist across test runs within the same process since the dictionary + // is static. Tests must use unique code values and clean up after themselves. + // + // See #676 for the tracking issue to replace this with a distributed store. + + [Fact] + public void ScalingLimitation_StaticAuthCodeStore_IsProcessScoped() + { + // This test documents and verifies the process-scoped nature of the auth code store. + // A code injected into the static dictionary is visible within the same process but + // would NOT be visible on a different instance — see #676. + var code = $"scaling-doc-{Guid.NewGuid():N}"; + var dict = GetAuthCodesDict(); + var dummyResult = CreateDummyAuthResult(); + + dict[code] = (dummyResult, DateTimeOffset.UtcNow.AddSeconds(60)); + + // The code is visible in the same process (expected for single-instance deployment) + dict.ContainsKey(code).Should().BeTrue( + "codes are stored in a static ConcurrentDictionary, visible within the same process"); + + // In a multi-instance deployment, this code would NOT be visible on another pod. + // The ConcurrentDictionary is process-local, not distributed. + // See #676 for the tracking issue to migrate to a distributed store (Redis/DB). + + // Verify the dictionary is specifically a static field on AuthController + var field = typeof(AuthController).GetField("_authCodes", + BindingFlags.NonPublic | BindingFlags.Static); + field.Should().NotBeNull("auth code store should be a static field on AuthController"); + field!.IsStatic.Should().BeTrue("auth code store must be static (process-scoped)"); + + // Clean up to avoid cross-test interference + dict.TryRemove(code, out _); + } + // ───────────────────────────────────────────────────────── // Helpers // ───────────────────────────────────────────────────────── From b258bd1926fc3475b56b0dca2295a98106260150 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 11 Apr 2026 23:51:32 +0100 Subject: [PATCH 2/3] Wrap static dict cleanup in try/finally and remove redundant IsStatic check Address bot review comments on PR #815: - Wrap all three tests that directly mutate the static auth code dictionary in try/finally blocks so cleanup runs even if assertions fail, preventing cross-test state leaks. - Remove redundant field.IsStatic assertion since GetField with BindingFlags.Static already guarantees only static fields are returned. - Move the reflection field validation before dict mutation in ScalingLimitation test so it fails fast without injecting state. --- .../OAuthTokenLifecycleTests.cs | 83 ++++++++++++------- 1 file changed, 51 insertions(+), 32 deletions(-) diff --git a/backend/tests/Taskdeck.Api.Tests/OAuthTokenLifecycleTests.cs b/backend/tests/Taskdeck.Api.Tests/OAuthTokenLifecycleTests.cs index cebf032eb..012000194 100644 --- a/backend/tests/Taskdeck.Api.Tests/OAuthTokenLifecycleTests.cs +++ b/backend/tests/Taskdeck.Api.Tests/OAuthTokenLifecycleTests.cs @@ -158,17 +158,24 @@ public void AuthCodeStore_CleanupRemovesExpiredCodes() dict[expiredCode2] = (dummyResult, DateTimeOffset.UtcNow.AddSeconds(-5)); dict[validCode] = (dummyResult, DateTimeOffset.UtcNow.AddSeconds(60)); - // Trigger cleanup by calling CleanupExpiredCodes via reflection - var cleanupMethod = typeof(AuthController).GetMethod("CleanupExpiredCodes", - BindingFlags.NonPublic | BindingFlags.Static); - cleanupMethod!.Invoke(null, null); - - dict.ContainsKey(expiredCode1).Should().BeFalse("expired code should be cleaned up"); - dict.ContainsKey(expiredCode2).Should().BeFalse("expired code should be cleaned up"); - dict.ContainsKey(validCode).Should().BeTrue("valid code should survive cleanup"); - - // Clean up the valid code to avoid cross-test interference - dict.TryRemove(validCode, out _); + try + { + // Trigger cleanup by calling CleanupExpiredCodes via reflection + var cleanupMethod = typeof(AuthController).GetMethod("CleanupExpiredCodes", + BindingFlags.NonPublic | BindingFlags.Static); + cleanupMethod!.Invoke(null, null); + + dict.ContainsKey(expiredCode1).Should().BeFalse("expired code should be cleaned up"); + dict.ContainsKey(expiredCode2).Should().BeFalse("expired code should be cleaned up"); + dict.ContainsKey(validCode).Should().BeTrue("valid code should survive cleanup"); + } + finally + { + // Clean up injected codes to avoid cross-test interference + dict.TryRemove(expiredCode1, out _); + dict.TryRemove(expiredCode2, out _); + dict.TryRemove(validCode, out _); + } } [Fact] @@ -183,11 +190,16 @@ public void AuthCodeStore_CleanupOnlyTriggeredDuringExchange_ExpiredCodesAccumul dict[code] = (dummyResult, DateTimeOffset.UtcNow.AddSeconds(-60)); - // Without an exchange attempt, the expired code remains in the dictionary - dict.ContainsKey(code).Should().BeTrue("expired codes accumulate until cleanup is triggered"); - - // Clean up - dict.TryRemove(code, out _); + try + { + // Without an exchange attempt, the expired code remains in the dictionary + dict.ContainsKey(code).Should().BeTrue("expired codes accumulate until cleanup is triggered"); + } + finally + { + // Clean up to avoid cross-test interference + dict.TryRemove(code, out _); + } } // ───────────────────────────────────────────────────────── @@ -554,28 +566,35 @@ public void ScalingLimitation_StaticAuthCodeStore_IsProcessScoped() // This test documents and verifies the process-scoped nature of the auth code store. // A code injected into the static dictionary is visible within the same process but // would NOT be visible on a different instance — see #676. + + // Verify the dictionary is specifically a static field on AuthController. + // GetField with BindingFlags.Static only returns static fields, so a non-null + // result already proves the field is static (no separate IsStatic check needed). + var field = typeof(AuthController).GetField("_authCodes", + BindingFlags.NonPublic | BindingFlags.Static); + field.Should().NotBeNull("auth code store should be a static field on AuthController"); + var code = $"scaling-doc-{Guid.NewGuid():N}"; var dict = GetAuthCodesDict(); var dummyResult = CreateDummyAuthResult(); dict[code] = (dummyResult, DateTimeOffset.UtcNow.AddSeconds(60)); - // The code is visible in the same process (expected for single-instance deployment) - dict.ContainsKey(code).Should().BeTrue( - "codes are stored in a static ConcurrentDictionary, visible within the same process"); - - // In a multi-instance deployment, this code would NOT be visible on another pod. - // The ConcurrentDictionary is process-local, not distributed. - // See #676 for the tracking issue to migrate to a distributed store (Redis/DB). - - // Verify the dictionary is specifically a static field on AuthController - var field = typeof(AuthController).GetField("_authCodes", - BindingFlags.NonPublic | BindingFlags.Static); - field.Should().NotBeNull("auth code store should be a static field on AuthController"); - field!.IsStatic.Should().BeTrue("auth code store must be static (process-scoped)"); - - // Clean up to avoid cross-test interference - dict.TryRemove(code, out _); + try + { + // The code is visible in the same process (expected for single-instance deployment) + dict.ContainsKey(code).Should().BeTrue( + "codes are stored in a static ConcurrentDictionary, visible within the same process"); + + // In a multi-instance deployment, this code would NOT be visible on another pod. + // The ConcurrentDictionary is process-local, not distributed. + // See #676 for the tracking issue to migrate to a distributed store (Redis/DB). + } + finally + { + // Clean up to avoid cross-test interference + dict.TryRemove(code, out _); + } } // ───────────────────────────────────────────────────────── From 5beed598c7c3f1c431dd13c579cfd5469a5ea2f0 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sun, 12 Apr 2026 00:39:57 +0100 Subject: [PATCH 3/3] Use valid change-password payload in expired JWT test --- .../tests/Taskdeck.Api.Tests/OAuthTokenLifecycleTests.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/backend/tests/Taskdeck.Api.Tests/OAuthTokenLifecycleTests.cs b/backend/tests/Taskdeck.Api.Tests/OAuthTokenLifecycleTests.cs index 012000194..7ab6725b2 100644 --- a/backend/tests/Taskdeck.Api.Tests/OAuthTokenLifecycleTests.cs +++ b/backend/tests/Taskdeck.Api.Tests/OAuthTokenLifecycleTests.cs @@ -501,7 +501,11 @@ public async Task ExpiredJwt_MultipleEndpoints_Return401(string endpoint) // Use POST for change-password; GET for everything else. Either way should 401 before body validation. var response = endpoint.Contains("change-password") - ? await expiredClient.PostAsJsonAsync(endpoint, new { CurrentPassword = "x", NewPassword = "y" }) + ? await expiredClient.PostAsJsonAsync(endpoint, new + { + CurrentPassword = "password123", + NewPassword = "NewSecurePassword!789" + }) : await expiredClient.GetAsync(endpoint); response.StatusCode.Should().Be(HttpStatusCode.Unauthorized,