nova28
76 lines
4.2 KiB
Markdown
76 lines
4.2 KiB
Markdown
---
|
|
id: "02-handler-error-mapping"
|
|
title: "Handler error mapping + HTTP regression tests"
|
|
status: pending
|
|
wave: 1
|
|
depends_on: ["01-service-authorization"]
|
|
plan: "plan.md"
|
|
spec: "../../specs/workflow-sync-workspace-authz/spec.md"
|
|
---
|
|
|
|
# Task 02: Handler error mapping + HTTP regression tests
|
|
|
|
Map the new `ErrWorkspaceNotFound` denial to a sanitized 404 in the HTTP layer, and add the
|
|
HTTP-level regression coverage this package currently lacks entirely (`handlers_test.go` does not
|
|
exist yet).
|
|
|
|
## Changes
|
|
|
|
1. `apps/backend/internal/workflowsync/handlers.go`:
|
|
- Import `errors` (already imported) and
|
|
`github.com/kandev/kandev/internal/task/repository/repoerrors`.
|
|
- Add `func workspaceDenied(err error) bool { return errors.Is(err, repoerrors.ErrWorkspaceNotFound) }`
|
|
(mirror `internal/jira/handlers.go:18-22`).
|
|
- In `httpGetConfig`, `httpSetConfig`, `httpDeleteConfig`, `httpForceSync`: check
|
|
`workspaceDenied(err)` before the existing generic-error branch and respond
|
|
`ctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"})`. Do not include the
|
|
underlying service error or any config fields in this response.
|
|
2. New file `apps/backend/internal/workflowsync/handlers_test.go` — HTTP-level tests using
|
|
`gin.New()` + `RegisterRoutes` + `httptest.NewRequest` + `authn.WithIdentity` on the request
|
|
context (pattern: `internal/github/controller_auth_identity_test.go`).
|
|
|
|
## Acceptance (from spec Scenarios)
|
|
|
|
- Owner identity: GET/POST/DELETE/sync all succeed against their own workspace, unchanged from
|
|
pre-fix behavior.
|
|
- Foreign member identity against a seeded victim workspace's config: all four routes return 404,
|
|
and the response body contains none of the victim's repo owner/name/branch/path/project_path.
|
|
- The victim's config is byte-for-byte unchanged after the denied POST/DELETE attempts (re-read via
|
|
the service and compare).
|
|
- Synthetic identity (auth disabled) succeeds on all four routes — no regression for the default
|
|
path.
|
|
- The two "foreign member GET/POST" cases are the regression tests: written to fail against
|
|
pre-task-01 code (they'd get 200 + leaked/accepted data) and pass after task 01 + this task.
|
|
|
|
## Verification
|
|
|
|
```bash
|
|
cd apps/backend && go test ./internal/workflowsync/... -run 'TestHTTP|TestConfig|TestForceSync' -v
|
|
cd apps/backend && go test ./internal/workflowsync/... -v
|
|
cd apps/backend && golangci-lint run ./internal/workflowsync/... --new-from-rev=main --timeout=5m
|
|
cd apps/backend && go test -race ./internal/workflowsync/...
|
|
```
|
|
|
|
## Results
|
|
|
|
- Status: **done**, including two follow-up fixes from an independent Codex review round.
|
|
- `go test ./internal/workflowsync/... -run 'TestHTTPHandlers' -v`: all pass
|
|
(`TestHTTPHandlers_OwnerSucceedsOnAllRoutes`, `TestHTTPHandlers_SyntheticIdentitySucceedsOnAllRoutes`,
|
|
`TestHTTPHandlers_ForeignMemberDeniedOnAllRoutesWithoutLeaking`, each table-driven across
|
|
get/post/delete/sync, plus `TestHTTPHandlers_ForceSyncDeniesSecondLookupAfterAccessRevoked`).
|
|
- Regression confirmed: before the task-01 service change, the get/post/delete/sync subtests failed
|
|
with 500s and leaked/accepted the foreign workspace's config (verified during TDD).
|
|
- **Codex review finding (blocker, fixed):** `httpForceSync`'s second call
|
|
(`GetConfigForWorkspace`, used to build the response after a successful sync) did not map
|
|
`ErrWorkspaceNotFound` to 404 — a race where access is revoked between the sync and the follow-up
|
|
read fell through to a generic 500. Fixed with the same `workspaceDenied` check; covered by
|
|
`TestHTTPHandlers_ForceSyncDeniesSecondLookupAfterAccessRevoked` (verified to fail pre-fix, pass
|
|
post-fix).
|
|
- **Codex review finding (suggestion, fixed):** owner/synthetic-identity coverage was GET-only and
|
|
the leak/mutation assertions didn't cover branch/path or the full config. Rewritten as
|
|
table-driven tests across all four operations (`TestHTTPHandlers_OwnerSucceedsOnAllRoutes`,
|
|
`TestHTTPHandlers_SyntheticIdentitySucceedsOnAllRoutes`), with distinctive branch/path fixture
|
|
values and a full-struct `before`/`after` config comparison in the denial test.
|
|
- `go test -race ./internal/workflowsync/... ./internal/backendapp/...`: both `ok`.
|
|
- `golangci-lint run ./internal/workflowsync/... --new-from-rev=main --timeout=5m`: 0 issues.
|