Files

Jakub Zych ba1aaefa01 docs(09): record approved review decisions and close UAT

2026-10-01 23:13:02 +02:00

18 KiB

Raw Blame History

phase, fixed_at, review_path, iteration, findings_in_scope, fixed, already_fixed, skipped, status

phase	fixed_at	review_path	iteration	findings_in_scope	fixed	already_fixed	skipped	status
09-backend-admin-authentication-and-schema-pipeline	2026-10-01T21:50:00+02:00	.planning/phases/09-backend-admin-authentication-and-schema-pipeline/09-REVIEW.md	1	20	19	1	0	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)

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.

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 (resolved 2026-10-01, approved by the user)

WR-11, unique index on lower(email): added in 2da8112 (summercms.go). The new migration 202610010001_backend_users_email_ci_unique creates backend_users_email_lower_unique. Rows copied from Winter are not changed automatically: when emails differ only in case, the migration refuses to run and names the clashing logins, so the operator changes or removes one and runs migrate again. Picking an admin account to drop is not something a migration should do. Tests: TestBackendAdminEmailCaseInsensitiveUnique and TestBackendAdminEmailIndexRefusesCaseDuplicates. Documented in modules/lagoon/README.md and docs/backend/users-and-permissions.md.
WR-03, single-record delete: kept as Winter. It stays allowed whenever a form exists, because the form screen's delete button matches Winter, and the admin API shape stays unchanged.
WR-17, deny versus wildcard: kept as Winter. An exact-code deny does not remove a wildcard grant, matching Winter's getMergedPermissions.

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

18 KiB Raw Blame History