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 new file mode 100644 index 00000000..45a5f645 --- /dev/null +++ b/.planning/phases/19.6.1-backend-administrators-screen/19.6.1-REVIEW.md @@ -0,0 +1,224 @@ +--- +phase: 19.6.1-backend-administrators-screen +reviewed: 2026-10-22T10:34:44Z +depth: standard +files_reviewed: 70 +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/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 +--- + +# Phase 19.6.1: Code Review Report + +**Reviewed:** 2026-10-22T10:34:44Z +**Depth:** standard +**Files Reviewed:** 70 +**Status:** issues_found + +## 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. + +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. + +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. + +## Narrative Findings (AI reviewer) + +## Critical Issues + +### 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) + +**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. + +## 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. + +## 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. + +--- + +_Reviewed: 2026-10-22T10:34:44Z_ +_Reviewer: Claude (msd-code-reviewer)_ +_Depth: standard_