From 28aa073de0de41520bac76f2eab52a1c341f88d4 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Thu, 1 Oct 2026 20:59:26 +0200 Subject: [PATCH] fix(09): WR-02 drop a denied main menu item and never link it to a controller the admin cannot open --- docs/backend/users-and-permissions.md | 2 +- modules/cabana/navigation.go | 40 +++++++++++++++++-- modules/cabana/permissions_test.go | 56 +++++++++++++++++++++++++++ 3 files changed, 93 insertions(+), 5 deletions(-) diff --git a/docs/backend/users-and-permissions.md b/docs/backend/users-and-permissions.md index d24a9bd..8f88ca8 100644 --- a/docs/backend/users-and-permissions.md +++ b/docs/backend/users-and-permissions.md @@ -64,7 +64,7 @@ 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. `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. +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. Actions registered through `pact.HasAdminActions` may name extra permissions, checked on top of the controller's. diff --git a/modules/cabana/navigation.go b/modules/cabana/navigation.go index e2003fb..85f817e 100644 --- a/modules/cabana/navigation.go +++ b/modules/cabana/navigation.go @@ -40,6 +40,12 @@ func (r *Registry) Metadata(ctx context.Context, principal *bouncer.Principal, t return navigation, settings } for _, item := range r.navigation { + // Like Winter's NavigationManager, a main item the principal may not + // open is dropped whatever its children allow, so a denied parent + // never leaks its label or target controller. + if !Allows(principal, item.Permissions) { + continue + } children := make([]NavigationEntry, 0) for _, child := range item.SideMenu { if !Allows(principal, child.Permissions) { @@ -47,10 +53,9 @@ func (r *Registry) Metadata(ctx context.Context, principal *bouncer.Principal, t } children = append(children, navigationView(ctx, tr, child, nil)) } - if !Allows(principal, item.Permissions) && len(children) == 0 { - continue - } - navigation = append(navigation, navigationView(ctx, tr, item, children)) + view := navigationView(ctx, tr, item, children) + view.Controller = r.openableTarget(principal, item, children) + navigation = append(navigation, view) } sort.SliceStable(navigation, func(i, j int) bool { if navigation[i].Order == navigation[j].Order { @@ -83,6 +88,33 @@ func (r *Registry) Metadata(ctx context.Context, principal *bouncer.Principal, t return navigation, settings } +// openableTarget returns the controller a main item should link to. It is the +// item's own controller unless the principal cannot open it, in which case it +// is the first side-menu entry the principal can, so the menu never links to a +// page that answers 403. It is empty when nothing is openable. +func (r *Registry) openableTarget(principal *bouncer.Principal, item pact.NavigationItem, children []NavigationEntry) string { + if item.Controller == "" || r.canOpen(principal, item.Controller) { + return item.Controller + } + for _, child := range children { + if child.Controller != "" && r.canOpen(principal, child.Controller) { + return child.Controller + } + } + return "" +} + +// canOpen reports whether principal passes a registered controller's required +// permissions. A controller the registry does not know is not blocked here: +// the guard answers for it when it is opened. +func (r *Registry) canOpen(principal *bouncer.Principal, controller string) bool { + cc, ok := r.Get(controller) + if !ok { + return true + } + return Allows(principal, requiredOf(cc.Controller)) +} + func navigationView(ctx context.Context, tr *phrasebook.Translator, item pact.NavigationItem, children []NavigationEntry) NavigationEntry { if children == nil { children = []NavigationEntry{} diff --git a/modules/cabana/permissions_test.go b/modules/cabana/permissions_test.go index fda50de..8b220d1 100644 --- a/modules/cabana/permissions_test.go +++ b/modules/cabana/permissions_test.go @@ -1,9 +1,11 @@ package cabana import ( + "context" "testing" "git.golem15.com/golem15/summercms/modules/bouncer" + "git.golem15.com/golem15/summercms/modules/pact" ) // TestAllowsFollowsWinterHasAnyAccess pins the permission check to Winter's @@ -49,3 +51,57 @@ func TestAllowsFollowsWinterHasAnyAccess(t *testing.T) { }) } } + +type navController struct { + id string + required []string +} + +func (c navController) ID() string { return c.id } +func (navController) ModelName() string { return "Metadata" } +func (navController) ConfigDir() string { return "controllers/metadata" } +func (c navController) RequiredPermissions() []string { return c.required } + +// TestNavigationDropsDeniedParentAndRepointsTarget covers the WR-02 rules: a +// main item the principal may not open is dropped whatever its children allow, +// and an allowed parent never links to a controller the principal cannot open. +func TestNavigationDropsDeniedParentAndRepointsTarget(t *testing.T) { + reg := &Registry{ + byID: map[string]*CompiledController{ + "acme.shop.albums": {Controller: navController{"acme.shop.albums", []string{"acme.shop.access_albums"}}}, + "acme.shop.genres": {Controller: navController{"acme.shop.genres", []string{"acme.shop.access_genres"}}}, + }, + navigation: []pact.NavigationItem{ + { + Code: "shop", Label: "Shop", Controller: "acme.shop.albums", Permissions: []string{"acme.shop.*"}, + SideMenu: []pact.NavigationItem{ + {Code: "albums", Label: "Albums", Controller: "acme.shop.albums", Permissions: []string{"acme.shop.access_albums"}}, + {Code: "genres", Label: "Genres", Controller: "acme.shop.genres", Permissions: []string{"acme.shop.access_genres"}}, + }, + }, + { + Code: "locked", Label: "Locked", Controller: "acme.shop.albums", Permissions: []string{"acme.locked.access"}, + SideMenu: []pact.NavigationItem{ + {Code: "genres", Label: "Genres", Controller: "acme.shop.genres", Permissions: []string{"acme.shop.access_genres"}}, + }, + }, + }, + } + genresOnly := &bouncer.Principal{ID: 1, Backend: true, PermissionGrants: map[string]bool{"acme.shop.access_genres": true}} + nav, _ := reg.Metadata(context.Background(), genresOnly, nil) + if len(nav) != 1 || nav[0].Code != "shop" { + t.Fatalf("navigation = %#v, want only the shop item (the locked parent must be dropped)", nav) + } + if nav[0].Controller != "acme.shop.genres" { + t.Fatalf("parent controller = %q, want the first openable child", nav[0].Controller) + } + if len(nav[0].SideMenu) != 1 || nav[0].SideMenu[0].Code != "genres" { + t.Fatalf("side menu = %#v", nav[0].SideMenu) + } + + both := &bouncer.Principal{ID: 2, Backend: true, PermissionGrants: map[string]bool{"acme.shop.access_albums": true}} + nav, _ = reg.Metadata(context.Background(), both, nil) + if len(nav) != 1 || nav[0].Controller != "acme.shop.albums" { + t.Fatalf("navigation = %#v, want the parent to keep its own controller", nav) + } +}