docs(09): add code review fix report
This commit is contained in:
Jakub Zych
@@ -0,0 +1,208 @@
|
||||
---
|
||||
phase: 09-backend-admin-authentication-and-schema-pipeline
|
||||
fixed_at: 2026-10-01T21:50:00+02:00
|
||||
review_path: .planning/phases/09-backend-admin-authentication-and-schema-pipeline/09-REVIEW.md
|
||||
iteration: 1
|
||||
findings_in_scope: 20
|
||||
fixed: 19
|
||||
already_fixed: 1
|
||||
skipped: 0
|
||||
status: all_fixed
|
||||
---
|
||||
|
||||
# Phase 9: Code Review Fix Report
|
||||
|
||||
**Fixed at:** 2026-10-01
|
||||
**Source review:** `.planning/phases/09-backend-admin-authentication-and-schema-pipeline/09-REVIEW.md`
|
||||
**Iteration:** 1
|
||||
**Scope:** critical_warning (CR-01 and WR-01 to WR-19; the 12 Info findings were out of scope)
|
||||
|
||||
**Summary:**
|
||||
- Findings in scope: 20
|
||||
- Fixed: 19
|
||||
- Already fixed before this run: 1 (CR-01)
|
||||
- Skipped: 0
|
||||
|
||||
The review predates Phase 10.1, 10.2 and 11, so every finding was re-checked against HEAD before it was touched. Paths in the review (`cabana/...`, `bouncer/...`) now live under `modules/` in `summercms.go`. Application code lives in the sibling repo `fonoteka.go`. Both repos were edited and committed separately. No co-author trailers were added.
|
||||
|
||||
Verification ran in the main checkout (`workflow.use_worktrees` is `false`), not in an isolated worktree, so the numbers below are reproducible from the working tree.
|
||||
|
||||
## Fixed Issues
|
||||
|
||||
### CR-01: Writable numeric fields cannot be saved; bad field values return 500 instead of 422
|
||||
|
||||
**Status:** already fixed, no change made
|
||||
**Evidence:** Phase 10.1 commits `c3efbc3` (fill numeric model fields from JSON numbers) and `e60e697` (answer a value that does not fit its column with a 422). `TestCRUDFillTypeIsValidation` in `modules/cabana/crud_test.go` pins it.
|
||||
|
||||
### WR-01: Required-permission wildcards never match, and multiple required codes use ALL rather than Winter's ANY
|
||||
|
||||
**Repo:** summercms.go
|
||||
**Commit:** `b4b8b5d`
|
||||
**Files modified:** `modules/cabana/contracts.go`, `modules/cabana/permissions_test.go`, `modules/cabana/README.md`, `docs/backend/users-and-permissions.md`
|
||||
**Applied fix:** `cabana.Allows` now ports Winter's `hasAnyAccess`/`hasPermission`: ANY of the listed codes, a trailing `.*` or leading `*` on the required code is met by any grant that matches it (excluding the bare prefix, as Winter does), and a grant ending in `.*` still covers every code under it. A 17-case table test pins both sides. Docs and README describe the new semantics.
|
||||
**Status:** fixed: requires human verification (behavioural change from ALL to ANY for multi-code requirements)
|
||||
|
||||
### WR-02: Navigation shows a denied parent item, including its label and target controller
|
||||
|
||||
**Repo:** summercms.go
|
||||
**Commit:** `28aa073`
|
||||
**Files modified:** `modules/cabana/navigation.go`, `modules/cabana/permissions_test.go`, `docs/backend/users-and-permissions.md`
|
||||
**Applied fix:** Like Winter's `NavigationManager`, a main item the principal may not open is dropped whatever its children allow. An allowed parent whose own controller the principal cannot open is repointed to its first openable child (or to an empty target), so the menu never links to a 403 page.
|
||||
**Status:** fixed: requires human verification (the repoint rule goes slightly beyond Winter, which keeps the fixed parent URL)
|
||||
|
||||
### WR-03: Create, update and delete are not gated by the compiled list and form declarations
|
||||
|
||||
**Repo:** summercms.go
|
||||
**Commit:** `f2ab93f`
|
||||
**Files modified:** `modules/cabana/registry.go`, `modules/cabana/http.go`, `modules/cabana/crud_lifecycle_test.go`, `modules/cabana/crud_test.go`, `modules/cabana/relation_field_test.go`, `modules/cabana/security_coverage_test.go`, `modules/cabana/README.md`, `docs/backend/admin-controllers.md`
|
||||
**Applied fix:** `CompiledController.operationDeclared` decides what the server accepts: create needs a form and `create` in `toolbar.buttons`; update and single-record delete need a form (the SPA's form screen carries the delete button, as in Winter, so single delete is not tied to the list toolbar); bulk delete needs `delete` in `toolbar.buttons`, which already requires `showCheckboxes`. A write that is not declared answers 403 `forbidden`. Test fixtures that exercised writes were given the toolbar they implicitly assumed.
|
||||
**Status:** fixed: requires human verification (interpretation of "declared" for single-record delete, see above)
|
||||
|
||||
### WR-04: A form field that is `required` but limited by `context` makes every create fail
|
||||
|
||||
**Repo:** summercms.go
|
||||
**Commit:** `b808408`
|
||||
**Files modified:** `modules/cabana/crud.go`, `modules/cabana/crud_test.go`
|
||||
**Applied fix:** `mergedRules` takes the operation and applies the form-level `required` only when `contextAllows` holds. The test fails without the fix.
|
||||
**Status:** fixed: requires human verification
|
||||
|
||||
### WR-05: Relation link and unlink ignore the panel's toolbarButtons
|
||||
|
||||
**Repo:** summercms.go
|
||||
**Commit:** `9bca815`
|
||||
**Files modified:** `modules/cabana/http.go`, `modules/cabana/permissions_test.go`, `docs/backend/relation-manager.md`
|
||||
**Applied fix:** `relationMutation` answers 403 `forbidden` unless the relation's `view.toolbarButtons` contains the requested action (the SPA reads `view.toolbarButtons` only). An unknown relation still gets the service's 404.
|
||||
|
||||
### WR-06: A relation list without an explicit `sort` fails when the first column is not sortable
|
||||
|
||||
**Repo:** summercms.go
|
||||
**Commit:** `889250c`
|
||||
**Files modified:** `modules/cabana/relation.go`, `modules/cabana/relation_test.go`
|
||||
**Applied fix:** With no `sort` the first sortable column is used, or none (primary key order). Only an explicitly requested sort is validated.
|
||||
**Status:** fixed: requires human verification
|
||||
|
||||
### WR-07: Relation column order depends on a fixed 16-space YAML indentation
|
||||
|
||||
**Repo:** summercms.go
|
||||
**Commit:** `b026aec`
|
||||
**Files modified:** `modules/cabana/relation.go`, `modules/cabana/schema.go`, `modules/cabana/relation_test.go`
|
||||
**Applied fix:** `config_relation.yaml` is decoded with `yaml.UseOrderedMap()` (new `decodeStrictOrdered`), so nested maps stay ordered through the per-entry re-marshal; `orderRelationColumns` is deleted. Tests cover 2-space and 4-space files and different orders in the view and manage panels; the 2-space test fails without the fix.
|
||||
|
||||
### WR-08: List search returns 500 for non-text searchable columns, and the scaffold creates one
|
||||
|
||||
**Repo:** summercms.go
|
||||
**Commit:** `8b48eea`
|
||||
**Files modified:** `modules/cabana/query.go`, `modules/cabana/query_test.go`
|
||||
**Applied fix:** Search compiles to `LOWER(CAST(col AS TEXT)) LIKE ? ESCAPE '\'`. A real-PostgreSQL test searches an integer, a timestamp, a switch and a text column. The `make:admin-controller` stub keeps `id: searchable: true`: with the cast it no longer errors, and removing it would make every search a 422 because the toolbar search would have no searchable column.
|
||||
|
||||
### WR-09: Scaffolded admin controllers have no permissions and no record source
|
||||
|
||||
**Repo:** summercms.go
|
||||
**Commit:** `20a79c5`
|
||||
**Files modified:** `internal/build/stubs/artifacts.tmpl`, `internal/build/artifact.go`, `internal/build/build_test.go`, `docs/console/scaffolding.md`, `docs/backend/admin-controllers.md`
|
||||
**Applied fix:** The generated controller now implements `pact.AdminRecordSource` (`NewRecord` returns `nil` with a TODO, so the list and writes answer 500 until the model is returned) and `pact.AdminPermissioned` with `<plugin>.access_<name>`. Until the plugin declares that permission, `cabana.Activate` refuses to start with an unknown-permission error naming the controller, so the scaffold can never be open to every administrator. IN-12 (the "DO NOT EDIT" header) was deliberately not changed: `docs/setup/porting-a-plugin.md` documents that the header must stay because `registry.gen.go` is rebuilt from files that carry it.
|
||||
|
||||
### WR-10: Cabana silently reuses any guard already registered under the name "backend"
|
||||
|
||||
**Repo:** summercms.go
|
||||
**Commits:** `3f47616` (fail boot on a foreign guard), `2d96adf` (follow-up)
|
||||
**Files modified:** `modules/cabana/http.go`, `modules/cabana/backend_guard_collision_test.go`, `modules/bouncer/registry.go`, `modules/bouncer/registry_test.go`, `modules/bouncer/README.md`, `modules/cabana/README.md`, `docs/backend/users-and-permissions.md`
|
||||
**Applied fix:** `Activate` registers its own guard and fails boot, naming the owner, when another plugin already owns `backend`. The first commit also failed when cabana itself had registered the guard on an earlier `Activate` of the same application, which the Fonoteka assembly tests do (`surf.Assemble` then `surf.BuildRouter`). The follow-up adds `bouncer.Registry.Owner` and keeps cabana's own guard in that case. The intermediate commit `3f47616` therefore passed the framework suite but broke three Fonoteka tests; the follow-up restores them.
|
||||
|
||||
### WR-11: Login identifier can resolve to the wrong admin; admin:create does not prevent login/email collisions
|
||||
|
||||
**Repo:** summercms.go
|
||||
**Commit:** `331351a`
|
||||
**Files modified:** `modules/cabana/auth.go`, `modules/cabana/commands.go`, `modules/cabana/auth_test.go`, `modules/cabana/commands_test.go`, `docs/backend/users-and-permissions.md`
|
||||
**Applied fix:** `findBackendLogin` fetches up to two matches and treats ambiguity as not found, so the request takes the same dummy bcrypt path and answers a plain 401. `admin:create` checks `login IN (login, email)` and `lower(email) IN (lower(login), email)`. Tests cover the ambiguous login (both admins locked out by that identifier, each still able to sign in by the other field) and three collision cases.
|
||||
**Not done:** the suggested unique index on `lower(email)`. See "Decisions needed" below.
|
||||
**Status:** fixed: requires human verification
|
||||
|
||||
### WR-12: The dummy hash cost is fixed at 10 while real hashes use the configured cost (timing oracle)
|
||||
|
||||
**Repo:** summercms.go
|
||||
**Commit:** `eb8c727`
|
||||
**Files modified:** `modules/cabana/auth.go`, `modules/cabana/auth_internal_test.go`
|
||||
**Applied fix:** The unknown-login hash is built lazily per service cost from `admin.password.bcrypt_cost` (cached per cost, so copying a `service` value stays `go vet` clean).
|
||||
|
||||
### WR-13: Admin passwords are passed as command-line flags
|
||||
|
||||
**Repo:** summercms.go
|
||||
**Commit:** `c9bb149`
|
||||
**Files modified:** `modules/cabana/commands.go`, `modules/cabana/commands_test.go`, `modules/cabana/README.md`, `docs/backend/users-and-permissions.md`, `docs/console/setup-and-maintenance.md`
|
||||
**Applied fix:** Without `--password`, both commands read the password through `bonfire.Output.Secret`: a hidden prompt on a terminal, one line from stdin otherwise, so scripts pipe it. `--password` is kept because the docs and tests use it, but it prints a deprecation warning that never echoes the value. Documented examples now use the prompt and a piped example. There is no second "confirm password" prompt, because `bonfire.Output` cannot tell a terminal from a pipe and a second read would break piped input.
|
||||
|
||||
### WR-14: Logout cannot revoke a token whose access lifetime has expired but whose refresh window is still open
|
||||
|
||||
**Repos:** summercms.go and fonoteka.go
|
||||
**Commits:** `299d220` (summercms.go), `c45fb99` (fonoteka.go)
|
||||
**Files modified:** `modules/bouncer/refresh.go`, `modules/bouncer/refresh_test.go`, `modules/bouncer/README.md`, `modules/cabana/auth.go`, `modules/cabana/http.go`, `modules/cabana/auth_test.go`, `modules/cabana/security_coverage_test.go`, `modules/cabana/README.md`, `docs/backend/users-and-permissions.md`; fonoteka.go `plugins/golem15/fonoteka/admin_phase09_security_test.go`
|
||||
**Applied fix:** New `bouncer.VerifyRefreshableClaimsAudience` verifies signature and audience with `exp` unchecked and accepts the token while `iat + refresh_ttl` is open. Logout is mounted outside the backend guard (still behind the CSRF header check), verifies the token with that function, blacklists its jti, and expires the cookie on every outcome including a 401. The route inventories in both repos list logout as public, with the reason. The test mints an expired token, logs it out, and shows refresh then fails.
|
||||
**Status:** fixed: requires human verification
|
||||
|
||||
### WR-15: The editors pivot `granted_by` stores a backend_users id in a frontend-user column
|
||||
|
||||
**Repo:** fonoteka.go
|
||||
**Commit:** `b925dd6`
|
||||
**Files modified:** `plugins/golem15/fonoteka/controllers/collections_admin_controller.go`, `plugins/golem15/fonoteka/admin_collections_test.go`
|
||||
**Applied fix:** An admin link leaves `granted_by` NULL (the column is nullable `INTEGER`, the model field is `*uint`, so no schema or migration change is involved) and `granted_by` is no longer a hook-writable pivot column. The existing test, which asserted a non-nil value, now asserts NULL. The other option in the review (resolve the backend email to a frontend user id) was not chosen: it needs a database lookup inside the hook and ties the audit column to a heuristic mapping.
|
||||
|
||||
### WR-16: Reflection helpers skip embedded structs and fall back to case-insensitive Go field names
|
||||
|
||||
**Repo:** summercms.go
|
||||
**Commit:** `629fac4`
|
||||
**Files modified:** `modules/cabana/model_fields.go` (new), `modules/cabana/model_fields_test.go` (new), `modules/cabana/http.go`, `modules/cabana/crud.go`, `modules/cabana/query.go`, `modules/cabana/list_schema.go`, `modules/cabana/filter_schema.go`, `modules/cabana/relation_field.go`, `docs/backend/admin-controllers.md`
|
||||
**Applied fix:** One `modelFields` helper flattens anonymous structs (not `time.Time`, Scanner or Valuer types) with index paths, and `fieldByColumn`, `modelColumns`, `primaryColumn`, `castPK`, the list and filter model contracts and `structFieldByColumn` use it. A field with an explicit `column:` tag is matched by that tag alone; an untagged field matches its Go name case-insensitively or GORM's default column name. `primaryColumn` reads `primarykey` case-insensitively (gorm.Model writes it lower-case) and returns the snake-case column. The review's "use GORM's parsed schema" option was not taken, to avoid changing which columns existing models expose.
|
||||
**Status:** fixed: requires human verification
|
||||
|
||||
### WR-17: User-level `backend_users.permissions` is ignored, so Winter denies are lost at cutover
|
||||
|
||||
**Repo:** summercms.go
|
||||
**Commit:** `8479def`
|
||||
**Files modified:** `modules/cabana/auth.go`, `modules/cabana/auth_test.go`, `modules/cabana/README.md`, `docs/backend/users-and-permissions.md`
|
||||
**Applied fix:** `BackendUsers.FindByID` overlays the user's own permissions on the role's and the code-declared role grants: the user's value for a code replaces the role's and only `1` grants, so `-1` and `0` remove a role grant and `1` adds one. This follows Winter's `getMergedPermissions`, which compares codes exactly, so denying `acme.a` does not take it back from a role that grants `acme.*`; the review asked for denies to cover wildcard matches too, but that would diverge from Winter, and the port rule is to match Winter. A `-1` on the wildcard key itself removes the wildcard. Seven cases are tested on real PostgreSQL.
|
||||
**Status:** fixed: requires human verification (the wildcard-deny nuance)
|
||||
|
||||
### WR-18: The phase gate's zero-test check applies per invocation, not per package
|
||||
|
||||
**Repo:** summercms.go
|
||||
**Commit:** `bbdc7a6`
|
||||
**Files modified:** `scripts/check-phase9.sh`
|
||||
**Applied fix:** `phase9_detect` tracks passing tests per `Package` and refuses when any package in the stream has none. `--self-test` gained a positive two-package case and a negative case (one package with a pass, one with only a package-level pass). `--security` and `--openapi` were run against the real suites and pass.
|
||||
|
||||
### WR-19: Plugin hooks and scopes query outside the CRUD transaction
|
||||
|
||||
**Repos:** summercms.go and fonoteka.go
|
||||
**Commits:** `4ae272e` (summercms.go), `e62f4fc` (fonoteka.go)
|
||||
**Files modified:** `modules/cabana/tx_context.go` (new), `modules/cabana/crud.go`, `modules/cabana/relation.go`, `modules/cabana/crud_lifecycle_test.go`, `modules/cabana/README.md`, `docs/backend/admin-controllers.md`; fonoteka.go `plugins/golem15/fonoteka/controllers/albums_admin_controller.go`, `plugins/golem15/fonoteka/controllers/collections_admin_controller.go`, `plugins/golem15/fonoteka/controllers/request_db.go` (new), `plugins/golem15/fonoteka/admin_albums_test.go`
|
||||
**Applied fix:** cabana puts the write's transaction on the context inside every `lagoon.Transaction` callback and exports `cabana.TxFromContext(ctx)`. The hook and scope signatures are unchanged, so no plugin contract breaks. The Fonoteka album and collection hooks read through it via a small `requestDB` helper and fall back to the pool outside a transaction (the list). The new Fonoteka test limits the pool to one connection and creates and updates an album; it hangs (and fails after 20 s) without the fix.
|
||||
|
||||
## Decisions needed
|
||||
|
||||
- **WR-11, unique index on `lower(email)` (not applied).** The suggestion adds a migration to `modules/lagoon/backend_admin_migrations.go`. Winter's `backend_users` has plain unique `login` and `email`; a functional unique index could make the migration fail on a copied table that holds two emails differing only in case, which D-01 says must copy straight in. The login-time ambiguity check and the `admin:create` check already close the practical hole, and an index would only add race protection. Decide whether to add the index, and whether to dedupe copied rows first.
|
||||
- **WR-03, single-record delete.** It is allowed whenever a form exists, not only when the list toolbar has `delete`, because the SPA's form screen always shows its own delete button (as in Winter). If delete should follow the toolbar too, the form schema needs a new field so the SPA can hide that button; that changes the admin API shape and OpenAPI.
|
||||
- **WR-17, deny versus wildcard.** Winter's merge does not let an exact-code deny remove a wildcard grant. If SummerCMS should be stricter than Winter here, say so and `applyUserPermissions` plus its tests change.
|
||||
|
||||
## Test results
|
||||
|
||||
All run in the main checkout after the last commit; both working trees are clean.
|
||||
|
||||
| Command | Where | Result |
|
||||
|---------|-------|--------|
|
||||
| `go vet ./...` | `summercms.go` | clean |
|
||||
| `go test ./...` | `summercms.go` | all packages ok |
|
||||
| `scripts/check-phase9.sh --self-test` | `summercms.go` | passed |
|
||||
| `scripts/check-phase9.sh --security` and `--openapi` | `summercms.go` | passed |
|
||||
| `go run ./cmd/summer docs:build --check`, `go test ./cmd/summer -run TestDocsTree` | `summercms.go` | no problems, ok |
|
||||
| `go vet ./...` and `go test ./...` | `fonoteka.go` root | ok (root, `parity`) |
|
||||
| `go vet ./...` and `go test ./...` | `fonoteka.go/plugins/golem15/fonoteka` and `.../user` | ok |
|
||||
|
||||
`go test ./...` from the `fonoteka.go` workspace root does not descend into the plugin modules, so they were run from their own directories as well.
|
||||
|
||||
`examples/hello` (a separate workspace module outside the root `./...`) fails `TestTypedItemRoute` with "config http.body_limits.default_bytes is required". It fails the same way at the pre-fix baseline `1a878b3`, so it is unrelated to this run.
|
||||
|
||||
---
|
||||
|
||||
_Fixed: 2026-10-01_
|
||||
_Fixer: Claude (gsd-code-fixer)_
|
||||
_Iteration: 1_
|
||||
Reference in New Issue
Block a user