Add HTTP integration tests for untested API surfaces#738
Conversation
Cover DataPortability, AbuseContainment, Metrics, Search,
AgentProfiles, and AgentRuns controllers with WebApplicationFactory-based
tests. Each test file validates auth enforcement (401), happy-path
responses, error contract compliance, and cross-user isolation.
Extend AuthzRegressionMatrix with 17 new unauthenticated endpoint
entries for /api/agents/*, /api/abuse/*, /api/account/*,
/api/metrics/*, and /api/search/*.
Two agent list endpoints (GET /api/agents, GET /api/agents/{id}/runs)
return 500 UnexpectedError — tests are Skip-annotated as known bugs.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Self-review findingsTests actually hit the HTTP pipelineAll tests use
Auth tests prove real enforcement
Coverage gaps (acceptable for first pass)
No issues found in diff
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive suite of integration tests for several API areas, including abuse containment, agent profiles, agent runs, data portability, metrics, and search. It also expands the authorization regression matrix to include these new endpoints. Feedback identifies a high-severity security vulnerability in the abuse containment logic where missing role-based access control allows regular users to override their own status. Additionally, there are recommendations to improve the specificity of test assertions in the data portability suite by utilizing the existing test harness helpers for error responses.
| var response = await client.PostAsJsonAsync("/api/abuse/actors/override", | ||
| new AbuseOverrideRequestDto(user.UserId, AbuseState.Restricted, "operator test override")); | ||
|
|
||
| response.StatusCode.Should().Be(HttpStatusCode.OK); |
There was a problem hiding this comment.
This assertion confirms a security vulnerability: a regular user is able to successfully override their own abuse status. The AbuseContainmentController (line 63) is intended for 'Operator override' but lacks role-based authorization (e.g., [Authorize(Roles = "Admin")]), allowing any user to potentially unblock themselves.
Additionally, this test suite is missing cross-user isolation tests for GetActorStatus, GetAuditTrail, and EvaluateActor. Unlike the AgentProfiles and AgentRuns tests in this PR, these endpoints do not verify that users are prevented from accessing or triggering actions on other users' accounts. It is recommended to add tests verifying that these endpoints return 403 Forbidden or 404 Not Found when accessed by a non-admin user targeting a different actorUserId.
| response.StatusCode.Should().NotBe(HttpStatusCode.OK, | ||
| "account deletion with wrong password should not succeed"); | ||
| response.Content.Headers.ContentType?.MediaType.Should().Be("application/json"); |
There was a problem hiding this comment.
The assertion Should().NotBe(HttpStatusCode.OK) is non-specific. Since you have the ApiTestHarness.AssertErrorContractAsync helper, you should use it to verify the exact expected status code (likely HttpStatusCode.BadRequest) and the error code (e.g., ValidationError). This ensures the API adheres to the expected error response structure.
await ApiTestHarness.AssertErrorContractAsync(response, HttpStatusCode.BadRequest, "ValidationError");| response.StatusCode.Should().NotBe(HttpStatusCode.OK, | ||
| "account deletion with wrong confirmation phrase should not succeed"); | ||
| response.Content.Headers.ContentType?.MediaType.Should().Be("application/json"); |
There was a problem hiding this comment.
Similar to the previous test case, using Should().NotBe(HttpStatusCode.OK) is too broad. It is better to assert the specific error response using the available test harness helper.
await ApiTestHarness.AssertErrorContractAsync(response, HttpStatusCode.BadRequest, "ValidationError");There was a problem hiding this comment.
Pull request overview
This PR adds HTTP-level integration test coverage (via WebApplicationFactory + HttpClient) for previously untested API controllers and extends the unauthenticated authz regression matrix to include those new API surfaces.
Changes:
- Added new HTTP integration tests for
/api/search,/api/metrics,/api/account/*(data portability),/api/abuse/*, and/api/agents/*(profiles + runs). - Extended
AuthzRegressionMatrixApiTestswith additional unauthenticated endpoint cases for agents/abuse/account/metrics/search. - Documented two known 500s via
[Fact(Skip=...)]tests on agent list endpoints.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/tests/Taskdeck.Api.Tests/SearchApiTests.cs | Adds integration tests for search auth, basic query behavior, pagination fields, special characters, and cross-user isolation. |
| backend/tests/Taskdeck.Api.Tests/MetricsApiTests.cs | Adds integration tests for board metrics auth, ownership boundaries, default/custom date range behavior, and empty-board behavior. |
| backend/tests/Taskdeck.Api.Tests/DataPortabilityApiTests.cs | Adds integration tests for account export + deletion flows, cache headers, and post-deletion login behavior. |
| backend/tests/Taskdeck.Api.Tests/AuthzRegressionMatrixApiTests.cs | Expands unauthenticated 401 regression matrix to cover agents/abuse/account/metrics/search endpoints. |
| backend/tests/Taskdeck.Api.Tests/AgentRunsApiTests.cs | Adds integration tests for agent run create/get and cross-user isolation; includes a skipped test for known 500 on list runs. |
| backend/tests/Taskdeck.Api.Tests/AgentProfilesApiTests.cs | Adds integration tests for agent profile CRUD and cross-user isolation; includes a skipped test for known 500 on list profiles. |
| backend/tests/Taskdeck.Api.Tests/AbuseContainmentApiTests.cs | Adds integration tests for abuse containment endpoints including validation (limit bounds, empty reason/actorId). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Should reject with an error (401 or 400 depending on implementation) | ||
| response.StatusCode.Should().NotBe(HttpStatusCode.OK, | ||
| "account deletion with wrong password should not succeed"); | ||
| response.Content.Headers.ContentType?.MediaType.Should().Be("application/json"); |
There was a problem hiding this comment.
DeleteAccount_WithWrongPassword_ShouldFail is currently too permissive: it only asserts the response is not 200. The implementation maps invalid password to AuthenticationFailed → HTTP 401 via ResultExtensions.ToHttpStatusCode, so this test should assert 401 and validate the ApiErrorResponse contract (including errorCode == "AuthenticationFailed") to avoid allowing regressions (e.g., 409/500) to pass.
| // Should reject with an error (401 or 400 depending on implementation) | |
| response.StatusCode.Should().NotBe(HttpStatusCode.OK, | |
| "account deletion with wrong password should not succeed"); | |
| response.Content.Headers.ContentType?.MediaType.Should().Be("application/json"); | |
| response.StatusCode.Should().Be(HttpStatusCode.Unauthorized, | |
| "account deletion with an invalid password should map to AuthenticationFailed"); | |
| response.Content.Headers.ContentType?.MediaType.Should().Be("application/json"); | |
| var error = await response.Content.ReadFromJsonAsync<ApiErrorResponse>(); | |
| error.Should().NotBeNull(); | |
| error!.ErrorCode.Should().Be("AuthenticationFailed"); |
| response.StatusCode.Should().NotBe(HttpStatusCode.OK, | ||
| "account deletion with wrong confirmation phrase should not succeed"); | ||
| response.Content.Headers.ContentType?.MediaType.Should().Be("application/json"); |
There was a problem hiding this comment.
DeleteAccount_WithWrongConfirmation_ShouldFail only checks “not 200”. The service returns ValidationError → HTTP 400 when the confirmation phrase doesn’t exactly match AccountDeletionService.RequiredConfirmationPhrase. Please assert BadRequest and validate the error contract (and expected errorCode == "ValidationError") so the test actually locks down the intended behavior.
| response.StatusCode.Should().NotBe(HttpStatusCode.OK, | |
| "account deletion with wrong confirmation phrase should not succeed"); | |
| response.Content.Headers.ContentType?.MediaType.Should().Be("application/json"); | |
| response.StatusCode.Should().Be(HttpStatusCode.BadRequest, | |
| "account deletion with an invalid confirmation phrase should return a validation error"); | |
| response.Content.Headers.ContentType?.MediaType.Should().Be("application/json"); | |
| var responseBody = await response.Content.ReadAsStringAsync(); | |
| using var json = JsonDocument.Parse(responseBody); | |
| json.RootElement.TryGetProperty("errorCode", out var errorCodeProperty).Should().BeTrue( | |
| "validation error responses should include an errorCode"); | |
| errorCodeProperty.GetString().Should().Be("ValidationError"); |
| loginResponse.StatusCode.Should().NotBe(HttpStatusCode.OK, | ||
| "deleted user should not be able to log in"); |
There was a problem hiding this comment.
DeleteAccount_ThenLogin_ShouldFail currently asserts only “not 200”. Since account deletion anonymizes the email and changes the password hash, /api/auth/login should deterministically return 401 with an ApiErrorResponse (likely errorCode == "AuthenticationFailed"). Tightening the assertion to the expected status + contract will prevent false positives (e.g., 500s) from passing.
| loginResponse.StatusCode.Should().NotBe(HttpStatusCode.OK, | |
| "deleted user should not be able to log in"); | |
| loginResponse.StatusCode.Should().Be(HttpStatusCode.Unauthorized, | |
| "deleted user should deterministically fail authentication"); | |
| var error = await loginResponse.Content.ReadFromJsonAsync<ApiErrorResponse>(); | |
| error.Should().NotBeNull(); | |
| error!.ErrorCode.Should().Be("AuthenticationFailed"); |
| private async Task<(HttpClient Client, AgentProfileDto Profile)> SetupAgentAsync(string stem) | ||
| { | ||
| var client = _factory.CreateClient(); | ||
| await ApiTestHarness.AuthenticateAsync(client, stem); | ||
| var createResponse = await client.PostAsJsonAsync("/api/agents", | ||
| new CreateAgentProfileDto($"{stem}-agent", "triage", AgentScopeType.Workspace)); | ||
| createResponse.StatusCode.Should().Be(HttpStatusCode.Created); | ||
| var profile = await createResponse.Content.ReadFromJsonAsync<AgentProfileDto>(); | ||
| profile.Should().NotBeNull(); | ||
| return (client, profile!); | ||
| } |
There was a problem hiding this comment.
SetupAgentAsync creates an HttpClient and returns it to callers, but the tests that call it never dispose that client. Over a large test suite this can lead to handler/socket/resource leaks and flaky runs. Consider changing the helper to accept an existing client (created with using var client = _factory.CreateClient();) or return only the created AgentProfileDto while keeping client disposal in the test.
| public async Task ExportUserData_ShouldReturnVersionedExport_ForAuthenticatedUser() | ||
| { | ||
| using var client = _factory.CreateClient(); | ||
| var user = await ApiTestHarness.AuthenticateAsync(client, "export-user"); | ||
|
|
||
| // Create a board so export has some content | ||
| await ApiTestHarness.CreateBoardAsync(client, "export-board"); | ||
|
|
There was a problem hiding this comment.
var user = await ApiTestHarness.AuthenticateAsync(...) is unused in this test, which adds noise and can trigger analyzer warnings in stricter configurations. Either remove the variable or use it to assert the exported userId matches the authenticated user.
Replace non-specific NotBe(OK) assertions with exact status code and error contract checks. Fix HttpClient leak in CrossUserIsolation test. Add note about intentional omission of abuse RBAC tests.
Adversarial Review (Round 2)Findings addressed in commit e54b2e9 -> 70754f71. Non-specific error assertions in DataPortabilityApiTests.cs (FIXED)
2. HttpClient leak in AgentRunsApiTests.cs (FIXED)
3. Missing RBAC documentation in AbuseContainmentApiTests.cs (NOTED)
Findings NOT addressed (pre-existing, out of scope for this PR)4. Security: AbuseContainmentController lacks operator-only authorization
5. Known 500s on agent list endpoints
6. Data export empty boards
Style/Convention
Bot review comments addressed
|
Final StatusCI: All checks passing (17/17 required checks green, 6 optional skipping as expected) Commits pushed
Remaining items for follow-up (out of scope for this PR)
PR is ready for merge. |
Summary
AuthzRegressionMatrixApiTestswith 17 new unauthenticated endpoint entries covering/api/agents/*,/api/abuse/*,/api/account/*,/api/metrics/*, and/api/search/*What's tested
ApiErrorResponsecontract (errorCode + message)Known bugs discovered
GET /api/agentsandGET /api/agents/{id}/runsreturn 500UnexpectedErroreven for valid authenticated requests. Tests are Skip-annotated. These should be investigated separately.Test plan
dotnet build backend/Taskdeck.sln -c Release— 0 errorsdotnet test backend/Taskdeck.sln -c Release -m:1— 592 total (590 passed, 2 skipped)Closes #702