diff --git a/.planning/phases/16.3-backend-admin-personal-api-tokens-with-scopes/16.3-REVIEW.md b/.planning/phases/16.3-backend-admin-personal-api-tokens-with-scopes/16.3-REVIEW.md new file mode 100644 index 0000000..7e3a646 --- /dev/null +++ b/.planning/phases/16.3-backend-admin-personal-api-tokens-with-scopes/16.3-REVIEW.md @@ -0,0 +1,155 @@ +--- +phase: 16.3-backend-admin-personal-api-tokens-with-scopes +reviewed: 2026-10-11T11:47:43Z +depth: standard +files_reviewed: 59 +files_reviewed_list: + - admin/openapi/admin.json + - admin/src/api/schema.d.ts + - admin/src/api/types.ts + - admin/src/components/account/ApiTokenCreateDialog.vue + - admin/src/components/account/ApiTokensPanel.vue + - admin/src/components/ui/ConfirmDialog.vue + - admin/src/views/AccountView.vue + - admin/tests/account/ApiTokenCreateDialog.test.ts + - admin/tests/account/ApiTokensPanel.test.ts + - admin/tests/fixtures/lang.json + - admin/tests/views/AccountView.test.ts + - cmd/summer/runtime.go + - docs/backend/admin-spa.md + - docs/backend/users-and-permissions.md + - docs/console/introduction.md + - docs/console/setup-and-maintenance.md + - docs/examples/blog/token_scopes.go + - docs/plugins/registration.md + - docs/services/authentication.md + - modules/bouncer/context.go + - modules/bouncer/context_test.go + - modules/cabana/account_tokens.go + - modules/cabana/account_tokens_test.go + - modules/cabana/admin_openapi.go + - modules/cabana/api_tokens.go + - modules/cabana/api_tokens_test.go + - modules/cabana/contracts.go + - modules/cabana/example_tokens_test.go + - modules/cabana/http.go + - modules/cabana/phase10_csrf_test.go + - modules/cabana/registry.go + - modules/cabana/token_commands.go + - modules/cabana/token_commands_test.go + - modules/cabana/token_guard.go + - modules/cabana/token_guard_test.go + - modules/cabana/token_postgres_smoke_test.go + - modules/cabana/token_postgres_test.go + - modules/cabana/token_routes_test.go + - modules/cabana/token_scopes.go + - modules/cabana/token_scopes_test.go + - modules/cabana/token_smoke_test.go + - modules/lagoon/backend_admin_migrations.go + - modules/lagoon/backend_admin_migrations_test.go + - modules/pact/capabilities.go + - modules/party/registry.go + - modules/party/registry_test.go + - modules/phrasebook/backend/lang/en/lang.yaml + - modules/phrasebook/backend/lang/pl/lang.yaml + - scripts/check-phase16.3.sh + - ../sm-journal-plugin/controllers/api/auth.go + - ../sm-journal-plugin/controllers/api/auth_test.go + - ../sm-journal-plugin/go.mod + - ../sm-journal-plugin/journal_api_token_smoke_test.go + - ../sm-journal-plugin/journal_api_tokens_test.go + - ../sm-journal-plugin/lang/en/lang.yaml + - ../sm-journal-plugin/lang/pl/lang.yaml + - ../sm-journal-plugin/README.md + - ../sm-journal-plugin/token_scopes.go + - ../sm-journal-plugin/token_scopes_test.go +findings: + critical: 0 + warning: 2 + info: 2 + total: 4 +status: issues_found +--- + +# Phase 16.3: Code Review Report + +**Reviewed:** 2026-10-11T11:47:43Z +**Depth:** standard +**Files Reviewed:** 59 +**Status:** issues_found + +## Summary + +Reviewed personal API token auth (cabana composite guard, scopes, account API, CLI), admin SPA tokens UI, Journal `ResolveBackend` integration, and phase unit/gate tests from plans 01–04 SUMMARY `key-files`. + +Token auth design is sound: plaintext never stored (sha256 + hint), Bearer `g15_` routed before JWT, `interactiveOnly` blocks token use on account/token/session routes, `ResolveBackend` is cookie-less, CSRF wraps token create/revoke, and Journal maps `ErrTokenScope` → 403. No Critical security defects found in the auth path. + +Two Warnings: a stale CSRF coverage golden leaves `go test ./modules/cabana` red while the phase gate stays green, and the create dialog enables Submit with no scope selected. + +## Warnings + +### WR-01: TestPhase10Coverage golden list omits account token routes + +**File:** `modules/cabana/phase10_coverage_test.go:90-124` +**Issue:** Plan 01 added `POST /account/tokens` and `DELETE /account/tokens/{id}` and updated `TestPhase10CSRF` to expect 32 unsafe routes, but `TestPhase10Coverage/every_unsafe_mounted_route_is_CSRF-walked` still asserts the pre-token golden list (message still says "expects 30 besides login"). Confirmed failing: + +```text +go test ./modules/cabana -run 'TestPhase10Coverage/every_unsafe' +# FAIL: missing DELETE /account/tokens/{id} and POST /account/tokens +``` + +`scripts/check-phase16.3.sh` does not require `TestPhase10Coverage`, so the phase gate can pass while `go test ./modules/cabana` (and the project “green at every commit” rule) fails. CSRF behavior for the new routes is covered dynamically by `TestPhase10CSRF`; the coverage golden is the regression. + +**Fix:** Add the two account-token routes to `want`, update the failure message to “32 besides login” (or count from `want`), and optionally add `TestPhase10Coverage` to `CABANA_RUN` / `CABANA_REQUIRE` in `scripts/check-phase16.3.sh`. + +```go +want := []string{ + "DELETE /account/avatar", + "DELETE /account/tokens/{id}", + // ... + "POST /account/avatar", + "POST /account/tokens", + "POST /auth/login", + // ... +} +``` + +### WR-02: Create dialog Submit enabled with zero scopes selected + +**File:** `admin/src/components/account/ApiTokenCreateDialog.vue:63-64` +**Issue:** `canSubmit` only requires that the picker finished loading with a non-empty catalog; it does not require `checked` to contain at least one scope. Users can click Create with nothing selected, hit a 422, then get focus moved to scopes. Server validation is correct; the client control still presents an enabled primary action for an invalid form. + +**Fix:** Include a selection check (and keep busy/loading/failure gates): + +```ts +const canSubmit = computed( + () => + !scopesLoading.value && + !scopesFailed.value && + scopes.value.length > 0 && + selectedScopes().length > 0 && + !busy.value, +) +``` + +(Or derive from `checked.value.size` / `starChecked` equivalently.) + +## Info + +### IN-01: last_used_at in-process debounce stores success before the DB write + +**File:** `modules/cabana/api_tokens.go:277-289` +**Issue:** `stampLastUsed` writes `lastStamp` before `StampLastUsed` returns. A failed UPDATE suppresses retries for up to a minute in that process (SQL also gates by minute; only the in-memory fast path is affected). +**Fix:** Store into `lastStamp` only after a nil error, or delete the map entry on failure. + +### IN-02: Create-dialog scope checkboxes lack associated accessible names beyond the wrapping label + +**File:** `admin/src/components/account/ApiTokenCreateDialog.vue:491-517` +**Issue:** Scope rows use a wrapping `