diff --git a/.planning/phases/19.6.1-backend-administrators-screen/19.6.1-REVIEW.md b/.planning/phases/19.6.1-backend-administrators-screen/19.6.1-REVIEW.md index ef861a74..acd201aa 100644 --- a/.planning/phases/19.6.1-backend-administrators-screen/19.6.1-REVIEW.md +++ b/.planning/phases/19.6.1-backend-administrators-screen/19.6.1-REVIEW.md @@ -1,98 +1,34 @@ --- phase: 19.6.1-backend-administrators-screen -reviewed: 2026-10-22T10:34:44Z +reviewed: 2026-10-22T12:51:21Z depth: standard -files_reviewed: 70 +files_reviewed: 6 files_reviewed_list: - - admin/src/api/types.ts - - admin/src/components/form/FormField.vue - - admin/src/components/form/control.ts - - admin/src/components/form/fields/PermissionEditorField.vue - admin/src/components/form/formContext.ts - - admin/src/components/form/formState.ts - - admin/src/components/form/trigger.ts - - admin/src/components/relation/RelationChildModal.vue - admin/src/views/FormView.vue - - admin/src/views/PreviewView.vue - - admin/tests/browser/record-actions-wrap.test.ts + - admin/src/components/relation/RelationChildModal.vue + - admin/src/components/form/fields/RelationField.vue + - admin/tests/form/RelationField.test.ts - admin/tests/form/FormView.test.ts - - admin/tests/form/PermissionEditorField.test.ts - - admin/tests/form/PreviewView.test.ts - - admin/tests/form/formState.test.ts - - admin/tests/form/trigger.test.ts - - admin/vitest.browser.config.ts - - docs/backend/admin-controllers.md - - docs/backend/forms.md - - docs/backend/users-and-permissions.md - - modules/cabana/README.md - - modules/cabana/action_extensions.go - - modules/cabana/action_extensions_postgres_test.go - - modules/cabana/action_extensions_test.go - - modules/cabana/admin_openapi.go - - modules/cabana/administrators_harness_test.go - - modules/cabana/administrators_lifecycle_postgres_test.go - - modules/cabana/administrators_postgres_test.go - - modules/cabana/backend_privileges.go - - modules/cabana/backend_privileges_internal_test.go - - modules/cabana/backend_roles_admin.go - - modules/cabana/backend_screens.go - - modules/cabana/backend_screens_smoke_test.go - - modules/cabana/backend_screens_test.go - - modules/cabana/backend_users_admin.go - - modules/cabana/backendscreens/controllers/roles/config_form.yaml - - modules/cabana/backendscreens/controllers/roles/config_list.yaml - - modules/cabana/backendscreens/controllers/users/_hint.htm - - modules/cabana/backendscreens/controllers/users/config_filter.yaml - - modules/cabana/backendscreens/controllers/users/config_form.yaml - - modules/cabana/backendscreens/controllers/users/config_list.yaml - - modules/cabana/backendscreens/models/user/columns.yaml - - modules/cabana/backendscreens/models/user/fields.yaml - - modules/cabana/backendscreens/models/userrole/columns.yaml - - modules/cabana/backendscreens/models/userrole/fields.yaml - - modules/cabana/contracts.go - - modules/cabana/crud.go - - modules/cabana/example_action_extension_test.go - - modules/cabana/example_permission_record_locks_test.go - - modules/cabana/export_test.go - - modules/cabana/field_dependent.go - - modules/cabana/field_permission.go - - modules/cabana/field_repeater.go - - modules/cabana/field_trigger.go - - modules/cabana/field_trigger_smoke_test.go - - modules/cabana/field_trigger_test.go - - modules/cabana/form_schema.go - - modules/cabana/http.go - - modules/cabana/record_locks_smoke_test.go - - modules/cabana/record_locks_test.go - - modules/cabana/registry.go - - modules/cabana/relation_child.go - - modules/cabana/relation_field.go - - modules/cabana/roles_postgres_test.go - - modules/cabana/schema_types.go - - modules/pact/README.md - - modules/pact/capabilities.go - - modules/phrasebook/backend/lang/en/lang.yaml - - modules/phrasebook/backend/lang/pl/lang.yaml - - scripts/check-phase19.6.1.sh findings: - critical: 1 - warning: 3 - info: 6 - total: 10 -status: issues_found + critical: 0 + warning: 0 + info: 0 + total: 0 +status: clean gap_closure: - reviewed: 2026-10-22T11:50:00Z + reviewed: 2026-10-22T12:51:21Z depth: standard - plan: "04" - files_reviewed: 7 + plan: "05" + prior_review: d7dc2a3 + files_reviewed: 6 files_reviewed_list: - - modules/cabana/backend_roles_admin.go - - modules/cabana/action_extensions.go - - modules/cabana/action_extensions_test.go - - modules/cabana/roles_postgres_test.go - - modules/cabana/action_extensions_postgres_test.go - - modules/pact/capabilities.go - - scripts/check-phase19.6.1.sh + - admin/src/components/form/formContext.ts + - admin/src/views/FormView.vue + - admin/src/components/relation/RelationChildModal.vue + - admin/src/components/form/fields/RelationField.vue + - admin/tests/form/RelationField.test.ts + - admin/tests/form/FormView.test.ts findings: critical: 0 warning: 0 @@ -104,195 +40,31 @@ gap_closure: # Phase 19.6.1: Code Review Report -**Reviewed:** 2026-10-22T10:34:44Z +**Reviewed:** 2026-10-22T12:51:21Z **Depth:** standard -**Files Reviewed:** 70 -**Status:** issues_found +**Files Reviewed:** 6 +**Status:** clean + +Wave-scoped incremental review of plan 05 (gap closure G-19.6.1-T2) after `d7dc2a3`. This file replaces the plans 01–04 review text; open findings from that pass stay on `19.6.1-REVIEW-DISPOSITION.md`. Concurrent settings-panel work and generated `modules/boardwalk/dist` were not reviewed (the dist was rebuilt in `51e1a1e`). ## Summary -The review focused on the Administrators and Roles save paths: the D-05..D-09 and D-19/D-20 privilege rules, transaction and lock correctness, GORM query scoping, and the SPA trigger and record-lock logic. +Plan 05 persists a picked relation option on the form host so a remounted `RelationField` can seed its id→label cache from `props.labels` after FormView unmounts inactive tab panels. The inject, both providers, `choose()` write, and the two named vitest cases match that contract. Inactive tabs stay unmounted (`v-show` is absent). Labels are display-only text interpolation; save still posts the id. -Most rules hold. The save pipeline runs `FormBeforeUpdate` before the permission editor and relation assignment, so the outranking guard sees the stored role and overrides. `is_activated`, `role_id`, `permissions` and `is_superuser` are protected fill keys. A non-superuser who sends `is_superuser` is answered 403 even though the field is hidden. Superusers are filtered out of every list, form, partial, record-action and bulk lookup through `scopeAdministrators`. The last-superuser guard takes the target's row lock first and the advisory lock second. Under Postgres's default READ COMMITTED isolation, it correctly serializes concurrent reductions on the form, record-action, bulk and delete paths. +Traced path: `choose()` writes `{value,label}` through `FORM_REMEMBER_LABEL` before `update:modelValue`; `rememberRelationLabel` replaces or appends by `option.value` on a new `labels` object; `FormGrid` looks up `labels[field.name]`; remount's `watch(() => props.labels, remember, { immediate: true })` fills `known`; `labelOf` then returns the name. `adopt()` still replaces labels from the server on load/save. A missing provider is a no-op. The child modal shadows the record form's provider so a tabbed child writes its own `labels` ref. -Four gaps remain in the privilege model: - -- **Roles code rename (blocker).** The Roles screen lets a non-superuser give a role a code that `pact.Permission.Roles` grants attach to. Every holder of that role, the editor included, then gains code-declared grants that are outside the D-07 ceiling. -- **Scoped tokens.** The D-07 and D-19 ceilings use the principal's full grants, not the grants a scoped personal API token actually allows. -- **Dropped override values.** Stored override values other than 1 and -1 are silently dropped on any save, which can restore grants that the override was removing. -- **Contributed actions.** Record actions that other plugins add to the Administrators screen through the D-14 seam bypass D-19 and D-09. +All reviewed files meet quality standards. No issues found. ## Narrative Findings (AI reviewer) -## Critical Issues +No Critical, Warning, or Info findings. -### CR-01: Roles screen lets a non-superuser rename or create a role onto a code with code-declared grants (D-07 ceiling bypass, self-escalation) +`FORM_REMEMBER_LABEL` is a typed `InjectionKey` next to `FIELD_SCOPE`. `rememberRelationLabel` copies the field array and the labels record (no in-place mutation), so the shallow `props.labels` watch fires. Empty (`null`) choice does not remember. Locked options return before the write, so `locked: entry.locked || undefined` is always `undefined` on this path and does not clear a stored lock. `RelationChildModal` uses the same helper and provide; its `FormGrid` already receives `:labels="labels"`. Tests use the widgets fixture (`maker` on `default`, `serial` on `tab:Details`) and the same `tab()` / `mousedown` pattern as the D-21 backstops. No consuming-application names. No new dependency. -**File:** `modules/cabana/backend_roles_admin.go:158-206` (with `backend_privileges.go:30-39`, `auth.go:55`, `registry.go:451-462`) -**Issue:** Code-declared grants (`pact.Permission.Roles`) attach to a role purely by its `code` string. `FindByID` merges `reg.rolePermissions(user.Role.Code)` into every holder's grants, and `roleGrantSet` does the same. - -`code` is fillable (`BackendUserRole.Fillable`). `FormBeforeUpdate` only refuses a code change when the **stored** role is already a system role (`c.systemRole(stored.IsSystem, stored.Code)`). `FormBeforeCreate` only checks uniqueness. Neither checks the **new** code against `reg.roleGrants`, and neither checks the new code's grants against the editor's holdings. - -Example: a non-superuser editor E holds `backend.manage_roles` and a non-system role R. Some plugin declares grants for a role code that has no row yet, for example `Roles: []string{"editor"}`. This can also happen on an imported Winter DB whose seeded `developer`/`publisher` row has a different name or code, since the seed is `ON CONFLICT (name) DO NOTHING`. E changes R's code to `editor`: -- the code passes `checkCode` (no other row uses it); -- R now carries every code-declared `editor` grant; -- E and every other holder of R gain those grants on their next request. - -None of this passes through the permission editor's `Locked` check, the role-picker ceiling (`AdminRelationLocks`) or the D-08 self-role refusal. All of those look at stored JSON and role ids, not at the code. This is exactly what D-07 forbids ("a non-superuser can't add a permission they lack to any role"). It also violates D-16: a role whose code gets code-declared grants silently becomes a system role. - -**Fix:** In both hooks, refuse a code that code-declared grants attach to unless the editor acts as a superuser and the role already had that code. At minimum, require the new code's grants to be covered: -```go -// in rolesAdmin, shared by FormBeforeCreate and FormBeforeUpdate (after reading stored) -func (c rolesAdmin) checkCodeGrants(ctx context.Context, newCode, storedCode string) error { - grants := c.registry().rolePermissions(newCode) - if len(grants) == 0 || newCode == storedCode { - return nil - } - principal, _ := bouncer.User(ctx) - if actsAsSuperuser(principal) { - return nil - } - var held map[string]bool - if principal != nil { - held = principal.PermissionGrants - } - if !grantsCovered(held, grants) { - return &ForbiddenError{Message: rolesLang + "code_grants_forbidden", - Details: map[string]any{"code": []string{rolesLang + "code_grants_forbidden"}}} - } - return nil -} -``` -A stricter option that matches D-16 is to refuse `len(grants) > 0 && newCode != storedCode` for everyone, so a role can never become a code-declared system role through the screen. Add a Postgres test that renames a held role onto a code-declared code as a non-superuser and expects 403. - -**Gap-closure (2026-10-22):** addressed by plan 04 (`refuseCodeDeclaredCode`). See the gap-closure pass below. Finding kept for history. - -## Warnings - -### WR-01: D-07/D-19 ceilings ignore a scoped personal API token's scope - -**File:** `modules/cabana/backend_users_admin.go:508-535, 545-571`; `modules/cabana/backend_privileges.go:123-146` -**Issue:** `catalogOptions`, `AdminRelationLocks` and `guardOutranked` all use `principal.PermissionGrants` as "what the editor holds". For a scoped token principal (`Scope != nil && !Scope.Full`), `Allows` only lets requests through `allowsUnderScope`, but the ceiling still uses the owner's **full** grants. `actsAsSuperuser` already refuses superuser powers to scoped tokens, so the authors treat scope as a boundary, but the non-superuser ceiling does not follow it. - -Now that `backend.manage_users`/`backend.manage_roles` are catalog permissions, a plugin may declare a token scope over them (`validatePluginTokenScope` accepts any catalog code). A token scoped only to `backend.manage_users` can then: -- create an administrator with a password and with every role or override the owner holds, or -- set the password of any non-outranking peer. - -A session as that administrator carries the owner's full grant set. A narrowly scoped (possibly leaked) token is thereby upgraded into full-grant credentials. -**Fix:** Derive the ceiling from scope ∩ grants when the principal is scope-limited, for example a helper `heldGrants(p)` that returns `p.PermissionGrants` filtered by `allowsUnderScope(p, []string{code})` when `p.Scope != nil && !p.Scope.Full`. Use it in `catalogOptions`, `AdminRelationLocks` and `guardOutranked`. Alternatively, refuse writes on the Administrators and Roles screens to any principal without a full scope. - -### WR-02: Stored override values other than 1/-1 are dropped on every save, re-granting what they removed - -**File:** `modules/cabana/backend_users_admin.go:584-590, 620-660`; `modules/cabana/field_permission.go:321-363` -**Issue:** `applyUserPermissions` (auth.go:77-100) treats every non-truthy stored override (`0`, `false`, `"0"`, `null`, any non-`1`) as **removing** the role's grant. `decodePermissionValues(raw, true)` keeps only `1` and `-1` and silently drops every other value. The permission editor then builds `next` from this decoded `stored` map and re-encodes it. - -As a result, any save of an administrator form erases those entries, including saves that do not touch permissions and saves where the code is `Locked` for the editor. On the self record it also passes `samePermissionValues`, because both sides were decoded the same way. Two consequences: -- **Third-party escalation.** Target T holds role R granting `p` with override `{"p":0}`. T is covered by editor E, who lacks `p`, so the D-19 guard passes. E saves T, for example to set T's password, and T silently regains `p`, beyond E's ceiling. -- **Self-escalation.** The same thing happens on E's own record when E saves their profile. - -Winter strips 0 on write, so the precondition needs rows written by SQL or other tooling. The guard should still not depend on that. -**Fix:** Preserve unknown stored values verbatim. Either decode non-truthy non-`-1` values as `-1`, which matches their effective meaning in `applyUserPermissions`, or carry the raw stored JSON entries for codes the submission does not change into `next` without normalizing them. Add a unit test where stored `{"p":0}` survives an unrelated save. - -### WR-03: Record actions contributed to the Administrators screen bypass D-19 (outranking) and D-09 (last superuser) - -**File:** `modules/cabana/action_extensions.go:23-70`; `modules/pact/capabilities.go:378-408` -**Issue:** D-19 refuses "running record/bulk actions on a non-superuser admin whose effective role + permissions grant more than the caller holds". D-09 protects the last superuser on "record actions". The built-in lifecycle actions call `guardOutranked`/`guardLastSuperuser` themselves. Actions merged in through `HasAdminActionExtensions`, however, run on `summercms.cabana.users` with only the target's `RequiredPermissions`, their own `Permissions` and `Applies`. The framework does not apply the outranking guard to them. - -The first planned consumer is 19.7's frontend-link action, which binds a frontend account to a backend admin that MCP then acts as. Through the seam, any `backend.manage_users` holder could link their own frontend account to an administrator who outranks them, which is the account-takeover path D-19 was written to close. Every contributor would have to re-implement unexported guards it cannot reach. -**Fix:** When compiling extensions whose target is `AdministratorsControllerID`, wrap each contributed `Run` (and `Applies`) so that `guardOutranked(ctx, reg, principal, record.(*BackendUser))` runs inside the action transaction before the contributor's `Run`. Alternatively, export a `cabana.GuardAdministratorWrite(ctx, record)` helper and document in `pact.AdminActionExtension` that it is required. Add a test with a contributed action on an outranking target that expects 403. - -**Gap-closure (2026-10-22):** D-19 half addressed by plan 04 (`wrapAdministratorsContributedRun`). D-09 (last superuser on a contributed Run) and wrapping `Applies` were out of that plan's contract and are not re-filed. Finding kept for history. - -## Info - -### IN-01: Last-superuser guard silently depends on READ COMMITTED - -**File:** `modules/cabana/backend_privileges.go:148-178` -**Issue:** The comment promises "the second one counts after the first committed". That is true only because each statement takes a fresh snapshot under Postgres's default isolation. If `lagoon.Transaction` or the pool's `default_transaction_isolation` is ever raised to REPEATABLE READ, the count runs on the snapshot taken by the earlier `SELECT ... FOR UPDATE`, and both concurrent demotions pass. -**Fix:** State the isolation assumption in the doc comment, or run the count with `SET LOCAL transaction_isolation` checks. Another option is to lock the other active superuser rows with `FOR UPDATE` instead of only counting them. - -### IN-02: Re-deleting a soft-deleted administrator reports success and bumps tokens - -**File:** `modules/cabana/backend_users_admin.go:199-232`; `modules/cabana/crud.go:1170-1185` -**Issue:** `FormExtendQuery` is unscoped, so a trashed admin loads. The scoped `tx.Delete` updates 0 rows, yet the response says `Deleted: 1` and `FormAfterDelete` rewrites `tokens_valid_after`. -**Fix:** Refuse delete on `user.DeletedAt.Valid` in `FormBeforeDelete`, or report a no-op. - -### IN-03: Role code uniqueness is a check-then-insert without a lock - -**File:** `modules/cabana/backend_roles_admin.go:190-206` -**Issue:** `backend_user_roles.code` has only a plain index. Two concurrent creates or renames to the same code both pass `checkCode`, and the comment itself notes that "code lookups fail on an ambiguous one". -**Fix:** Take a `pg_advisory_xact_lock` keyed on the code (or a fixed key) before counting, or add a unique index in a migration. - -### IN-04: Role delete counts holders without locking against concurrent assignment - -**File:** `modules/cabana/backend_roles_admin.go:214-241` -**Issue:** An administrator save that assigns the role concurrently with its delete hits the `role_id` FK, and the delete answers an opaque 500 instead of the `delete_has_users` refusal. -**Fix:** Lock the role row (`SELECT ... FOR UPDATE` on `backend_user_roles`) before counting holders. Assignments then take `FOR KEY SHARE` through the FK and serialize. - -### IN-05: `toggleOn` reads unknown switch encodings as "off" - -**File:** `modules/cabana/backend_users_admin.go:705-718, 784-797` -**Issue:** A non-SPA client that sends `"on"`, `"yes"` or `"True "` for `is_superuser` on a superuser target silently demotes the target, subject only to the last-superuser check. -**Fix:** Reject values that are not a recognised boolean encoding with a 422 on the field instead of treating them as false. - -### IN-06: Role picker on your own record is editable in the SPA but always refused - -**File:** `modules/cabana/backend_users_admin.go:504-535, 816-829` -**Issue:** D-08 refuses a change to your own role only in `FormAfterUpdate`, after the row write, and nothing marks the role field read-only for the self record. The permissions tab gets a record lock instead. The administrator only learns of the refusal from a 403 on save. -**Fix:** In `AdminRelationLocks`, when the form is the editor's own record, lock every role except the current one. Alternatively, expose a per-record read-only hint for `role`, as `AdminPermissionRecordLocks` does for permissions. +Prior findings WR-01, WR-02, and IN-01..IN-06 were out of this pass. --- -## Gap-closure pass (2026-10-22) - -**Reviewed:** 2026-10-22T11:50:00Z -**Depth:** standard -**Plan:** 04 (commits `068adaa`, `cb460b6`, `e620860`, `6bd2506`) -**Files Reviewed:** 7 -**Status:** clean - -This pass reviewed only the plan-04 gap-closure sources at HEAD. It does not re-review the original 70-file phase. - -### Gap-closure files - -- `modules/cabana/backend_roles_admin.go` (`refuseCodeDeclaredCode`, `FormBeforeCreate`, `FormBeforeUpdate`) -- `modules/cabana/action_extensions.go` (`wrapAdministratorsContributedRun`, `compileActionExtensions` wrap) -- `modules/cabana/action_extensions_test.go` -- `modules/cabana/roles_postgres_test.go` (`TestRolesCodeDeclaredCodeRefused`) -- `modules/cabana/action_extensions_postgres_test.go` (`TestActionExtensionOutranked`) -- `modules/pact/capabilities.go` (`AdminActionExtension` comment) -- `scripts/check-phase19.6.1.sh` (`DOCKER_TESTS` names) - -### Gap-closure summary - -No new Critical, Warning, Info, or Nice-to-have findings. - -`refuseCodeDeclaredCode` is called from create (stored code `""`) and update (stored code from the save transaction). It refuses when `submitted != stored` and `len(registry.rolePermissions(submitted)) > 0`, including for a superuser. Same-code saves of a system role still pass. `rolePermissions` is nil-safe. The 422 shape reuses `backend::lang.user.role.code_locked` on `code`. `guardOutranked` stays unexported. - -`compileActionExtensions` wraps a contributed `Run` only when `ext.Controller == AdministratorsControllerID`, after `checkContributedAction` has required a non-nil `Run`. The wrapper type-asserts `*BackendUser`, fails closed without calling `next` on a bad record, then calls `guardOutranked` inside `CRUDService.RecordAction`'s transaction. `lifecycleFailure` / `actionFailure` pass `*ForbiddenError` through as 403. Other controller IDs are stored unchanged. `Applies` is not wrapped, as the plan required. - -The boot smoke now supplies a superuser principal so the wrap's `actsAsSuperuser` short-circuit avoids needing `TxFromContext` in a unit test. The Postgres tests cover the HTTP paths: create/rename onto a unique code-declared code with no row (editor and superuser), a unique non-declared create, a same-code system-role save, an outranking contributed `acme_ping` (403, spy empty), built-in deactivate still 403, and a covered target still 200 inside the transaction. Both names are in `DOCKER_TESTS` and not in `SHORT_TESTS`. The `AdminActionExtension` comment states that cabana, not the contributor, applies `user.outranked`. No consuming-application names, no new dependency, YAML parser unchanged. - -Prior findings WR-01, WR-02, and IN-01..IN-06 were out of this pass. WR-03's D-09 clause (contributed Run still does not call `guardLastSuperuser`) remains as originally written; a non-superuser still cannot load a superuser row (`scopeAdministrators` → 404), so that leftover is the superuser-on-superuser path the plan did not take. - -### Gap-closure Critical Issues - -None. - -### Gap-closure Warnings - -None. - -### Gap-closure Info - -None. - -### Gap-closure Nice-to-have - -None. - ---- - -_Reviewed: 2026-10-22T10:34:44Z (original); 2026-10-22T11:50:00Z (gap-closure plan 04)_ +_Reviewed: 2026-10-22T12:51:21Z_ _Reviewer: Claude (msd-code-reviewer)_ _Depth: standard_