golem15/summercms
4
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(19.6.1): add code review report

Browse Source
This commit is contained in:
Jakub Zych
2026-10-22 12:36:02 +02:00
parent a4cfd628ce
commit d5da300e24
1 changed files with 224 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

224
.planning/phases/19.6.1-backend-administrators-screen/19.6.1-REVIEW.md Normal file
View File

@@ -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_