fix: narrowing a privileged custom role gives its grants to the tenant's members access the host was never asked about

README.md

@@ -110,8 +110,12 @@ conditional ones reach, to check one by one.
   scope's path keeps from changing grants there, can neither change what its grants give nor
   delete it, nor widen it where a deny there, or where a role listing it, or listing one that does,
   is granted, removes a permission it would give or the power to grant it, or the label of a scope
-  such a role is granted on refuses it. Renaming a role or rewriting its description changes no
-  grant, so needs only the power to define it.
+  such a role is granted on refuses it. A grant widens by what it gives through its principal, as
+  checks read it: one to the tenant's members of a privileged role, or to the public of a role not
+  marked `Public`, gives nothing, so widening the role asks nothing about it, and a redefinition
+  that has it give something, as narrowing the role so that it is no longer privileged does, widens
+  it, with what was added to the role while it gave nothing. Renaming a role or rewriting its
+  description changes no grant, so needs only the power to define it.
   `DeleteRole` removes one with its grants; `Roles` lists the built-in and custom roles a scope type
   takes. Custom roles follow the catalog: one gives only what the catalog still has, and is
   privileged when it gives what only privileged roles do, or changes access.
@@ -304,10 +308,13 @@ conditional ones reach, to check one by one.
   `rbac.ErrNotFound`, or an error wrapping it: a change that only takes access away there, as a
   member leaving a group with access on it or the group itself, or joining one a deny there names, a
   place in the group made to end sooner, `DeleteGroup`, or `DeleteRole` deleting, or `SetRole`
-  narrowing, a role granted on it, reads it as unlabeled, and any other change fails with the error.
+  narrowing so that no grant of it there gives more, a role granted on it, reads it as unlabeled,
+  and any other change fails with the error.
 - **Privileged roles:** a role the catalog marks `Privileged`, or one that changes access, is
   privileged: it is never granted to the tenant's members, and gives nothing through them, as
-  after a catalog change.
+  after a catalog change. Their grants of it give what it gives once it no longer is: `SetRole`
+  checks a redefinition doing so as widening them, but a catalog change doing so is the host's, and
+  asks nothing.
 - **Public access:** the zero `Subject` is anonymous and holds only what is granted to the public.
   Only a role marked `Public` can be granted to the public, and a grant to the public gives
   nothing once its role no longer is.

authority_test.go

@@ -2069,8 +2069,11 @@ func TestARemovalReadsAScopeTheHostNoLongerHasAsUnlabeled(t *testing.T) {
 		}
 	})
 	ctx := t.Context()
+	// custom.priv is privileged, as it gives root, and granted to the members on gone, as a catalog
+	// change would leave it.
 	if _, err := h.pool.Exec(ctx, `INSERT INTO rbac_roles (tenant, id, permissions, resource_types) VALUES ('t1', 'custom.viewer', '{cluster:view}', '{cluster}'),
-		('t1', 'custom.ops', '{cluster:view,cluster:login}', '{cluster}')`); err != nil {
+		('t1', 'custom.ops', '{cluster:view,cluster:login}', '{cluster}'), ('t1', 'custom.priv', '{cluster:view,cluster:root}', '{cluster}');
+		INSERT INTO rbac_grants (tenant, scope_type, scope_id, principal_kind, principal_id, role) VALUES ('t1', 'cluster', 'gone', 'members', 't1', 'custom.priv')`); err != nil {
 		t.Fatal(err)
 	}
 	gone, down := rbac.Scope{Type: "cluster", ID: "gone"}, rbac.Scope{Type: "cluster", ID: "down"}
@@ -2105,6 +2108,12 @@ func TestARemovalReadsAScopeTheHostNoLongerHasAsUnlabeled(t *testing.T) {
 	if err := h.authz.SetRole(ctx, admin, "t1", ops); !errors.Is(err, rbac.ErrNotFound) {
 		t.Errorf("widening it: %v", err)
 	}
+	// Narrowed so that it is no longer privileged, a role gives the members' grant there what it
+	// gives then, so the host must read the scope for that too.
+	priv := rbac.RoleDefinition{ID: "custom.priv", Permissions: []rbac.Permission{view}, ResourceTypes: []string{"cluster"}}
+	if err := h.authz.SetRole(ctx, admin, "t1", priv); !errors.Is(err, rbac.ErrNotFound) {
+		t.Errorf("narrowing a role the members hold there so that it is no longer privileged: %v", err)
+	}
 	// So with a group the host no longer has: its members are removed, and their places made to end
 	// sooner, but none added, nor any place made to last longer.
 	if err := h.authz.ChangeMembers(ctx, admin, "t1", []rbac.Member{{Group: "lost", Principal: identity("v")}}, nil); !errors.Is(err, rbac.ErrNotFound) {

docs/architecture.md

@@ -465,6 +465,13 @@ sequenceDiagram
 - **Custom roles.** `SetRole` redefines a role in place for every grant of it, so it asks
   `ValidateGrant` about every grant and eligibility the new definition widens, before its
   transaction, and under the tenant's lock refuses with `ErrConflict` if those changed meanwhile.
+  A grant widens by what it gives through its principal, as checks read it
+  (`redefinition.widensTo`): one to the tenant's members of a privileged role, or to the public of
+  a role not marked `Public`, gives nothing, and widens once the role gives it something, however
+  the role changed, as narrowing it so that it is no longer privileged does. On each scope a grant
+  that widens is on, the denies on its path that remove from the actor a permission the role will
+  give refuse the change, as they would refuse `Apply` granting it there; on the others, only those
+  removing its power to change grants there do.
 - **Eligibilities.** `SetEligibility` lets a principal `Activate` a role for a while, perhaps
   only with approvers or a `requires` condition. An activation is a grant that ends no later than
   its eligibility expires, or its holder's membership or place in the group the eligibility names

membership_test.go

@@ -4065,6 +4065,74 @@ func TestWideningAPrivilegedRoleAsksNothingOfGrantsThatGiveNothing(t *testing.T)
 	}
 }
 
+// A role the catalog makes privileged gives the tenant's members nothing through a grant to them,
+// so widening it asks the host nothing about that grant. Narrowing it so that it is no longer
+// privileged gives them what it gives then, what was added while it gave them nothing too, so the
+// host is asked about that grant, as the role will be, and refusing it refuses the narrowing.
+func TestNarrowingARoleSoItIsNoLongerPrivilegedAsksAboutTheMembersGrant(t *testing.T) {
+	viewer := rbac.RoleDefinition{ID: "cluster.viewer", Permissions: []rbac.Permission{view}, ResourceTypes: []string{"cluster"}}
+	h := setup(t, func(c *rbac.Config) { c.Catalog = append(slices.Clone(c.Catalog), viewer) })
+	ctx := t.Context()
+	admin := roleAdmin(t, h)
+	x := rbac.RoleDefinition{ID: "custom.x", Permissions: []rbac.Permission{login}, ResourceTypes: []string{"cluster"}}
+	if err := h.authz.SetRole(ctx, admin, "t1", x); err != nil {
+		t.Fatal(err)
+	}
+	h.seed(t, grant(rbac.TenantRoot("t1"), x.ID, rbac.Members, "t1"))
+	member := h.as(t, "m")
+	// The catalog makes logging in privileged, so x is, and the members' grant gives nothing.
+	cfg := h.cfg
+	cfg.Catalog = slices.Clone(cfg.Catalog)
+	for i, r := range cfg.Catalog {
+		if r.ID == "cluster.login" {
+			cfg.Catalog[i].Privileged = true
+		}
+	}
+	var asked []rbac.GrantInput
+	refuseMembers := true
+	cfg.ValidateGrant = func(_ context.Context, _ rbac.DB, in rbac.GrantInput) error {
+		asked = append(asked, in)
+		if refuseMembers && in.Binding.Principal.Kind == rbac.Members {
+			return fmt.Errorf("%w: not through the members", rbac.ErrInvalid)
+		}
+		return nil
+	}
+	authz, err := rbac.New(h.pool, cfg)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := authz.Authorize(ctx, member, login, cluster); !errors.Is(err, rbac.ErrDenied) {
+		t.Fatalf("a privileged role through the members: %v", err)
+	}
+	x.Permissions = []rbac.Permission{login, view}
+	if err := authz.SetRole(ctx, admin, "t1", x); err != nil || len(asked) != 0 {
+		t.Fatalf("widening a privileged role the members hold: %v, asked %+v", err, asked)
+	}
+	// Narrowed to what a role that isn't privileged gives, the members' grant would give every
+	// member view on every cluster.
+	x.Permissions = []rbac.Permission{view}
+	err = authz.SetRole(ctx, admin, "t1", x)
+	var ie *rbac.InvalidError
+	if !errors.As(err, &ie) || ie.Binding.Principal != rbac.TenantMembers("t1") {
+		t.Fatalf("narrowing a role the members hold so that it is no longer privileged: %v", err)
+	}
+	if len(asked) != 1 || asked[0].Via != rbac.ViaRedefinition || asked[0].Scope != rbac.TenantRoot("t1") ||
+		asked[0].Role.Privileged || !slices.Equal(asked[0].Role.Permissions, x.Permissions) || asked[0].Actor.Principal != admin.Principal {
+		t.Fatalf("asked = %+v", asked)
+	}
+	if err := authz.Authorize(ctx, member, view, cluster); !errors.Is(err, rbac.ErrDenied) {
+		t.Fatalf("a refused narrowing gave the members view: %v", err)
+	}
+	// Once the host lets the members hold it, it gives them what it gives.
+	refuseMembers, asked = false, nil
+	if err := authz.SetRole(ctx, admin, "t1", x); err != nil || len(asked) != 1 {
+		t.Fatalf("narrowing it once the host lets the members hold it: %v, asked %+v", err, asked)
+	}
+	if err := authz.Authorize(ctx, member, view, cluster); err != nil {
+		t.Fatalf("the members' grant of a role no longer privileged: %v", err)
+	}
+}
+
 func TestAPrivilegedRoleForTheMembersIsRefusedBeforeTheHostIsAsked(t *testing.T) {
 	var asked []rbac.GrantInput
 	h := setup(t, func(c *rbac.Config) {

rbac_test.go

@@ -7576,6 +7576,86 @@ func TestADenyBelowTheTenantStopsARedefinitionHandingOnWhatItRemoves(t *testing.
 	}
 }
 
+// A grant to the tenant's members of a privileged role gives nothing, so narrowing the role so that
+// it no longer is gives them what it gives, as granting it to them would: a deny on the workspace
+// that grant is on, which keeps x from handing on what it gives there, stops it, as it stops Apply
+// there. A deny where only a grant to an identity is, which gives no more, stops nothing.
+func TestADenyWhereTheMembersHoldARoleNoLongerPrivilegedStopsNarrowingIt(t *testing.T) {
+	h, guardian := guarded(t)
+	ctx := t.Context()
+	x, r := definer(t, h)
+	member := h.as(t, "m")
+	elsewhere := rbac.Resource{Tenant: "t1", Type: "cluster", ID: "c2"}
+	// Privileged, as it gives root, and granted to the members, as a catalog change would leave it.
+	if _, err := h.pool.Exec(ctx, `INSERT INTO rbac_roles (tenant, id, permissions, resource_types) VALUES ('t1', 'custom.op', '{cluster:view,cluster:root}', '{cluster}');
+		INSERT INTO rbac_grants (tenant, scope_type, scope_id, principal_kind, principal_id, role) VALUES
+		  ('t1', 'workspace', 'w1', 'members', 't1', 'custom.op'), ('t1', 'cluster', 'c2', 'user', 'y', 'custom.op')`); err != nil {
+		t.Fatal(err)
+	}
+	noView := rbac.Deny{ID: "no-view", Permissions: []rbac.Permission{view}, Principals: []rbac.Principal{x.Principal}}
+	if err := h.authz.SetDeny(ctx, guardian, workspace, noView); err != nil {
+		t.Fatal(err)
+	}
+	if err := h.authz.Apply(ctx, x, workspace, rbac.Change{Grant: []rbac.Binding{{Role: r.ID, Principal: rbac.TenantMembers("t1")}}}); !errors.Is(err, rbac.ErrDenied) {
+		t.Fatalf("granting view to the members under the deny: %v", err)
+	}
+	narrowed := rbac.RoleDefinition{ID: "custom.op", Permissions: []rbac.Permission{view}, ResourceTypes: []string{"cluster"}}
+	var denied *rbac.DeniedError
+	if err := h.authz.SetRole(ctx, x, "t1", narrowed); !errors.As(err, &denied) || denied.Scope != workspace.Scope() || allowed(t, h, member, view, cluster) {
+		t.Fatalf("narrowing a role the members hold under the deny so that it is no longer privileged: %v", err)
+	}
+	if err := h.authz.DeleteDeny(ctx, guardian, workspace, noView.ID); err != nil {
+		t.Fatal(err)
+	}
+	if err := h.authz.SetDeny(ctx, guardian, elsewhere, noView); err != nil {
+		t.Fatal(err)
+	}
+	if err := h.authz.SetRole(ctx, x, "t1", narrowed); err != nil || !allowed(t, h, member, view, cluster) {
+		t.Fatalf("narrowing it under a deny only where a grant to an identity is: %v", err)
+	}
+}
+
+// What the narrowing checked of the denies where the members' grant is before its transaction it
+// checks again under the tenant's lock, so a deny set there while the host is asked about that
+// grant stops it. A deny elsewhere binds x, so the workspace was read before the transaction.
+func TestADenySetWhileANarrowingTheMembersGainByWaitsStopsIt(t *testing.T) {
+	var meanwhile func(context.Context) error
+	h, guardian := guarded(t, func(c *rbac.Config) {
+		validate := c.ValidateGrant
+		c.ValidateGrant = func(ctx context.Context, q rbac.DB, in rbac.GrantInput) error {
+			if do := meanwhile; do != nil && in.Via == rbac.ViaRedefinition {
+				meanwhile = nil
+				if err := do(ctx); err != nil {
+					return err
+				}
+			}
+			return validate(ctx, q, in)
+		}
+	})
+	ctx := t.Context()
+	x, _ := definer(t, h)
+	member := h.as(t, "m")
+	if _, err := h.pool.Exec(ctx, `INSERT INTO rbac_roles (tenant, id, permissions, resource_types) VALUES ('t1', 'custom.op', '{cluster:view,cluster:root}', '{cluster}');
+		INSERT INTO rbac_grants (tenant, scope_type, scope_id, principal_kind, principal_id, role) VALUES ('t1', 'workspace', 'w1', 'members', 't1', 'custom.op')`); err != nil {
+		t.Fatal(err)
+	}
+	elsewhere := rbac.Resource{Tenant: "t1", Type: "cluster", ID: "c2"}
+	if err := h.authz.SetDeny(ctx, guardian, elsewhere, rbac.Deny{ID: "no-root", Permissions: []rbac.Permission{root}, Principals: []rbac.Principal{x.Principal}}); err != nil {
+		t.Fatal(err)
+	}
+	meanwhile = func(ctx context.Context) error {
+		return h.authz.SetDeny(ctx, guardian, workspace, rbac.Deny{ID: "no-view", Permissions: []rbac.Permission{view}, Principals: []rbac.Principal{x.Principal}})
+	}
+	narrowed := rbac.RoleDefinition{ID: "custom.op", Permissions: []rbac.Permission{view}, ResourceTypes: []string{"cluster"}}
+	var denied *rbac.DeniedError
+	if err := h.authz.SetRole(ctx, x, "t1", narrowed); !errors.As(err, &denied) || denied.Scope != workspace.Scope() || allowed(t, h, member, view, cluster) {
+		t.Fatalf("narrowing a role the members hold while a deny is set where they hold it: %v", err)
+	}
+	if meanwhile != nil {
+		t.Fatal("the deny should have been set meanwhile")
+	}
+}
+
 // A deny on a container, or on the resource itself, that removes x's power to change grants there
 // stops x changing what a role granted there gives, or revoking it by deleting the role, as it
 // stops Apply there.