From 8479defe53a258708b9040c2c9dd76f10544bd26 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Thu, 1 Oct 2026 21:33:07 +0200 Subject: [PATCH] fix(09): WR-17 merge an admin's own permissions over the role's, honouring denies --- docs/backend/users-and-permissions.md | 2 + modules/cabana/README.md | 2 +- modules/cabana/auth.go | 35 +++++++++++++++ modules/cabana/auth_test.go | 62 +++++++++++++++++++++++++++ 4 files changed, 100 insertions(+), 1 deletion(-) diff --git a/docs/backend/users-and-permissions.md b/docs/backend/users-and-permissions.md index a08d9e4..801a08b 100644 --- a/docs/backend/users-and-permissions.md +++ b/docs/backend/users-and-permissions.md @@ -66,6 +66,8 @@ func (p *BlogPlugin) Navigation() []pact.NavigationItem { A controller's `pact.AdminPermissioned.RequiredPermissions` are checked before any schema is served or query runs, and navigation and settings entries are filtered by the permissions they name, so an administrator sees only what they may open: a main menu item the administrator may not open is dropped even when one of its side-menu entries would pass, and an allowed item that links to a controller the administrator cannot open links to its first openable side-menu entry instead. `cabana.Allows` is the check and follows Winter's `hasAnyAccess`: superusers pass, an administrator needs any one of the listed codes, and an empty requirement list allows any signed-in administrator. Wildcards match on both sides: a grant ending in `.*` covers every code with that prefix, and a required code such as `acme.blog.*` is met by any grant under `acme.blog.`. The last lines of the activation example on [Admin controllers](admin-controllers.md) show it. +An administrator's grants are the role's `permissions` merged with the administrator's own `backend_users.permissions`, the way WinterCMS merges them: the administrator's value for a code replaces the role's, and only `1` grants. A `-1` (or `0`) on the administrator therefore removes a permission the role grants, so rows copied from a WinterCMS database keep their denies. As in WinterCMS the merge compares codes exactly, so denying `acme.blog.access_posts` does not take it back from a role that grants `acme.blog.*`. + Actions registered through `pact.HasAdminActions` may name extra permissions, checked on top of the controller's. ## Managing administrators diff --git a/modules/cabana/README.md b/modules/cabana/README.md index 01fe594..5bae324 100644 --- a/modules/cabana/README.md +++ b/modules/cabana/README.md @@ -19,7 +19,7 @@ Schema-driven admin backend that compiles WinterCMS-style YAML list, form, filte - Toolbar actions: `toolbar.buttons` in `config_list.yaml` lists the built-in `create` and `delete` next to names the controller registers through `pact.HasAdminActions`. Registered actions share one namespace with widget actions, `create` and `delete` are reserved, and each toolbar action needs a label. The list schema's `toolbarActions` carries only the actions the requesting administrator may run, with localized labels; an unknown name fails boot. - Server-rendered partials: `headerPartial: ` in `config_list.yaml` (a strip above the list) and `type: partial` with `path: ` in `fields.yaml` render the template `{ConfigDir}/_.htm` with `html/template` against a view model from the controller's `pact.AdminPartialData`. The result reaches the SPA as an allowlisted node tree, never as an HTML string. A missing or unparsable template, a free-form path or a controller without `pact.AdminPartialData` fails boot. - Singleton settings screens declared with `pact.HasSettings`, read and saved by `cabana.SettingsService`. -- Backend navigation (`pact.HasNavigation`) and permissions (`pact.HasPermissions`), filtered per user by `cabana.Registry.Metadata`. `cabana.Allows` implements the permission check with Winter's `hasAnyAccess` semantics: superusers pass, a principal needs any one of the listed codes, and wildcards match on both sides (a grant ending in `.*` covers every code with that prefix, and a required code ending in `.*` is met by any grant under it). +- Backend navigation (`pact.HasNavigation`) and permissions (`pact.HasPermissions`), filtered per user by `cabana.Registry.Metadata`. An administrator's own `backend_users.permissions` are merged over the role's as in Winter (a `-1` denies a code the role grants). `cabana.Allows` implements the permission check with Winter's `hasAnyAccess` semantics: superusers pass, a principal needs any one of the listed codes, and wildcards match on both sides (a grant ending in `.*` covers every code with that prefix, and a required code ending in `.*` is met by any grant under it). - Admin authentication against WinterCMS's `backend_users` and `backend_user_roles` tables (`cabana.BackendUser`, `cabana.BackendUserRole`, `cabana.BackendUsers`): a JWT guard registered in [bouncer](../bouncer/README.md) as `backend` (a guard another plugin already registered under that name fails `cabana.Activate`), login throttling, token refresh and revocation, and two transports. API clients use a Bearer token; the SPA sends `X-Requested-With: XMLHttpRequest` and receives the token in the HttpOnly, SameSite=Strict cookie named by `cabana.AdminCookieName`. Cookie-authenticated requests that change state must carry that header, which blocks cross-site request forgery. - A consistent JSON envelope for every response: `cabana.WriteData`, `cabana.WriteError` and `cabana.WriteErrorDetails`, typed for documentation as `cabana.Envelope`, `cabana.ListEnvelope`, `cabana.RecordEnvelope` and `cabana.ErrorEnvelope`. A body that cannot be encoded is logged and answered with the generic 500 envelope, never a success status with a truncated body. - OpenAPI documentation: `cabana.AdminList`, `cabana.AdminCreate` and the other `Admin*` functions have empty bodies and exist only to carry the swag annotations of each admin route. diff --git a/modules/cabana/auth.go b/modules/cabana/auth.go index 38d5028..1de236d 100644 --- a/modules/cabana/auth.go +++ b/modules/cabana/auth.go @@ -61,9 +61,44 @@ func (p BackendUsers) FindByID(ctx context.Context, id uint) (*bouncer.Principal } principal.PermissionGrants[code] = true } + // The administrator's own permissions are applied last, over the role's + // and the code-declared role grants, as Winter's getMergedPermissions does. + principal.PermissionGrants = applyUserPermissions(principal.PermissionGrants, user.Permissions) return principal, nil } +// applyUserPermissions overlays an administrator's own backend_users.permissions +// onto the grants that come from the role, the way Winter's +// User::getMergedPermissions does: the user's value for a code replaces the +// role's, and only a value of 1 grants. A user-level -1 (or 0) therefore removes +// a permission the role grants, and a user-level 1 adds one the role does not. +// Winter compares codes exactly while merging, so a deny of one code never +// removes a wildcard grant such as "acme.*"; it removes that exact code. +func applyUserPermissions(grants map[string]bool, raw string) map[string]bool { + raw = strings.TrimSpace(raw) + if raw == "" || raw == "{}" || raw == "null" { + return grants + } + var decoded map[string]any + if err := json.Unmarshal([]byte(raw), &decoded); err != nil { + return grants + } + for code, value := range decoded { + if truthyGrant(value) { + if grants == nil { + grants = map[string]bool{} + } + grants[code] = true + continue + } + delete(grants, code) + } + if len(grants) == 0 { + return nil + } + return grants +} + func principalFrom(user BackendUser) *bouncer.Principal { principal := &bouncer.Principal{ ID: user.ID, diff --git a/modules/cabana/auth_test.go b/modules/cabana/auth_test.go index ce443f2..00e9bec 100644 --- a/modules/cabana/auth_test.go +++ b/modules/cabana/auth_test.go @@ -630,3 +630,65 @@ func TestAdminLogoutRevokesExpiredRefreshableToken(t *testing.T) { } } } + +// TestBackendUserPermissionsOverrideRole pins WR-17: an administrator's own +// backend_users.permissions are merged over the role's the way Winter merges +// them. A user-level -1 removes a permission the role grants and a user-level 1 +// adds one the role lacks, so a row copied from WinterCMS keeps its denies. +func TestBackendUserPermissionsOverrideRole(t *testing.T) { + gdb := adminGorm(t) + role := cabana.BackendUserRole{Name: "WR-17 role", Code: "wr17", Permissions: `{"wr17.a":1,"wr17.b":1,"wr17.wild.*":1}`} + if err := gdb.Create(&role).Error; err != nil { + t.Fatal(err) + } + cases := []struct { + name string + perms string + roleP *uint + want map[string]bool + }{ + {name: "no user level permissions", perms: "", roleP: &role.ID, want: map[string]bool{"wr17.a": true, "wr17.b": true, "wr17.wild.*": true}}, + {name: "empty object", perms: "{}", roleP: &role.ID, want: map[string]bool{"wr17.a": true, "wr17.b": true, "wr17.wild.*": true}}, + {name: "deny removes a role grant and a grant adds one", perms: `{"wr17.b":-1,"wr17.c":1}`, roleP: &role.ID, want: map[string]bool{"wr17.a": true, "wr17.c": true, "wr17.wild.*": true}}, + {name: "zero is not a grant", perms: `{"wr17.a":0}`, roleP: &role.ID, want: map[string]bool{"wr17.b": true, "wr17.wild.*": true}}, + {name: "deny of a wildcard grant", perms: `{"wr17.wild.*":-1}`, roleP: &role.ID, want: map[string]bool{"wr17.a": true, "wr17.b": true}}, + {name: "no role", perms: `{"wr17.only":1}`, roleP: nil, want: map[string]bool{"wr17.only": true}}, + {name: "malformed JSON is ignored", perms: `not json`, roleP: &role.ID, want: map[string]bool{"wr17.a": true, "wr17.b": true, "wr17.wild.*": true}}, + } + for i, tc := range cases { + hash, err := bouncer.HashPassword(4, "x") + if err != nil { + t.Fatal(err) + } + user := cabana.BackendUser{ + Login: "wr17-" + strconv.Itoa(i), Email: "wr17-" + strconv.Itoa(i) + "@example.test", Password: hash, + IsActivated: true, RoleID: tc.roleP, Permissions: tc.perms, + } + if err := gdb.Create(&user).Error; err != nil { + t.Fatal(err) + } + principal, err := (cabana.BackendUsers{DB: gdb}).FindByID(context.Background(), user.ID) + if err != nil || principal == nil { + t.Fatalf("%s: FindByID = %v, %v", tc.name, principal, err) + } + got := map[string]bool{} + for code, on := range principal.PermissionGrants { + if on { + got[code] = true + } + } + if len(got) != len(tc.want) { + t.Fatalf("%s: grants = %v, want %v", tc.name, got, tc.want) + } + for code := range tc.want { + if !got[code] { + t.Fatalf("%s: grants = %v, want %v", tc.name, got, tc.want) + } + } + } + // The check the guard feeds: the denied code no longer passes Allows. + denied := &bouncer.Principal{Backend: true, PermissionGrants: map[string]bool{"wr17.a": true}} + if cabana.Allows(denied, []string{"wr17.b"}) { + t.Fatal("a denied code passed Allows") + } +}