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

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

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 {
+		view := navigationView(ctx, tr, item, children)
-			continue
+		view.Controller = r.openableTarget(principal, item, children)
-		}
+		navigation = append(navigation, view)
-		navigation = append(navigation, navigationView(ctx, tr, item, children))
 	}
 	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{}

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)
+	}
+}