docs(16.3): add code review report

2026-10-11 13:48:16 +02:00
parent f4395b57b7
commit 36b1bce5c5
1 changed files with 155 additions and 0 deletions
--- 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
@@ -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 `<label>` (good) but the checkbox has no `aria-describedby` linking the description text; screen-reader users get the visible label text via the label association, which is acceptable. Optional improvement: wire description ids for longer scope blurbs.
+**Fix:** Optional only — add stable ids on description spans and `aria-describedby` on the inputs if UX audit wants richer announcements.
+
+---
+
+_Reviewed: 2026-10-11T11:47:43Z_
+_Reviewer: Claude (msd-code-reviewer)_
+_Depth: standard_