feat(coderd/rbac): compare scopes by permission coverage (#28167)

Split out of #28045 ([PLAT-479](https://linear.app/codercom/issue/PLAT-479)) so the coverage rules can be reviewed on their own. First of three. No behavior change: both functions are added without production callers, and the OAuth2 authorize endpoint consumes them in the next PR. **ScopesCover(allowed, requested)** reports whether every permission the request grants is also granted by the allowlist. It expands both sides and compares permissions rather than names, so coder:workspaces.access covers workspace:read without ever naming it. The fail-closed rules are the part worth reviewing: - Coverage models site-level grants only. A scope carrying org or user permissions, a negative permission, or a narrowed allow list is refused on either side rather than compared on the part that is modeled. Answering "covered" from the fraction that was read would report authority that was never examined. - An unknown name on either side is an error, not a false, since a caller cannot tell "not covered" apart from "could not decide". - A wildcard request is covered only by a wildcard grant. Enumerating every workspace action that exists today does not cover workspace:*, because the wildcard also authorizes the actions added tomorrow. **CanonicalScopeName** maps the aliases IsExternalScope accepts, all and application_connect, onto the api_key_scope enum spellings. IsExternalScope answers whether a name may be requested, not how it is spelled once persisted, so anything storing a validated name has to canonicalize in between. Both sides of ScopesCover must already be canonical, which its parameter names restate at every call site. Covered by 17 coverage cases, guard cases that drive each unmodeled shape through both sides of the comparison, and an exhaustive sweep over the external catalog: coder:all covers all of it, and every catalog name covers itself. Stack: this PR, then negotiate and persist the scope, then the consent page and invalid_scope redirect. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2026-10-31 02:43:21 +08:00 · 2026-08-20 09:16:04 -07:00
parent e58a603949
commit dea3a2944a
4 changed files with 527 additions and 8 deletions
@@ -244,6 +244,15 @@ func (s Scope) Name() RoleIdentifier {
 	return s.Identifier
 }

+// ExpandScope resolves a scope name to the permissions it grants, from the
+// builtin scopes, the composite coder:* scopes, or a low-level resource:action
+// pair. The name must be canonical: the `all` and `application_connect`
+// aliases IsExternalScope accepts are not scope names here, so canonicalize
+// with CanonicalScopeName first.
+//
+// Every expansion populates Site only, with a wildcard allow list and no
+// negative permissions. ScopesCover depends on that shape and refuses a scope
+// that breaks it.
 func ExpandScope(scope ScopeName) (Scope, error) {
 	if role, ok := builtinScopes[scope]; ok {
 		return role, nil
@@ -318,3 +327,120 @@ func expandLowLevel(resource string, action policy.Action) Scope {
 		AllowIDList: []AllowListElement{{Type: policy.WildcardSymbol, ID: policy.WildcardSymbol}},
 	}
 }
+
+// ScopesCover reports whether every permission the requested scope grants is
+// also granted by at least one of the allowed scopes. It compares expanded
+// permissions, not names, so `coder:workspaces.access` covers `workspace:read`
+// and `coder:all` covers everything.
+//
+// Only a wildcard grant covers a wildcard request: `workspace:*` also
+// authorizes the actions added tomorrow, which no list of today's can.
+//
+// Both sides must already be canonical. IsExternalScope also admits the `all`
+// and `application_connect` aliases, which are not expandable names, so
+// canonicalize between validating a name and asking about its coverage.
+//
+// Coverage models site-level grants only. Anything it cannot fully compare, an
+// unknown name or a scope carrying more than site permissions, is an error on
+// either side rather than a false, since a caller cannot act on coverage
+// decided from a fraction of the authority.
+func ScopesCover(canonicalAllowed []ScopeName, canonicalRequested ScopeName) (bool, error) {
+	want, err := ExpandScope(canonicalRequested)
+	if err != nil {
+		return false, xerrors.Errorf("expand requested scope: %w", err)
+	}
+
+	grants := make([]namedScope, 0, len(canonicalAllowed))
+	for _, name := range canonicalAllowed {
+		expanded, err := ExpandScope(name)
+		if err != nil {
+			return false, xerrors.Errorf("expand allowed scope: %w", err)
+		}
+		grants = append(grants, namedScope{name: name, scope: expanded})
+	}
+
+	return scopesCoverExpanded(grants, namedScope{name: canonicalRequested, scope: want})
+}
+
+// namedScope pairs an expanded scope with the name the caller spelled, so a
+// guard error can name the scope as it was requested rather than as it expanded.
+type namedScope struct {
+	name  ScopeName
+	scope Scope
+}
+
+// scopesCoverExpanded is the comparison ScopesCover runs once both sides are
+// expanded. It is separate because every Scope ExpandScope builds satisfies
+// the guards below, so driving synthetic Scope values through this function is
+// the only way to reach them. Testing checkCoverable alone would leave
+// unverified the part that matters most: that both sides are actually checked.
+func scopesCoverExpanded(allowed []namedScope, requested namedScope) (bool, error) {
+	if err := checkCoverable(requested.scope, coverageSideRequested, requested.name); err != nil {
+		return false, err
+	}
+
+	granted := make([]Permission, 0, len(allowed)*4)
+	for _, entry := range allowed {
+		if err := checkCoverable(entry.scope, coverageSideAllowed, entry.name); err != nil {
+			return false, err
+		}
+		granted = append(granted, entry.scope.Site...)
+	}
+
+	for _, needed := range requested.scope.Site {
+		if !permissionCovered(needed, granted) {
+			return false, nil
+		}
+	}
+	return true, nil
+}
+
+// Which side of a coverage comparison a scope sits on. Both sides are held to
+// the same invariant, so the side only distinguishes the error messages.
+const (
+	coverageSideRequested = "requested"
+	coverageSideAllowed   = "allowed"
+)
+
+// checkCoverable reports an error when scope carries authority that coverage
+// cannot compare, rather than letting the comparison run on the part that is
+// modeled. Each guard names authority coverage would not otherwise read: an org
+// or user grant may itself carry a negative permission, a negative site
+// permission would read as a grant on a matching resource and action (see
+// permissionCovered), and an allow list makes the Site permissions
+// conditional, so reading them as unconditional would overstate what the scope
+// grants.
+func checkCoverable(scope Scope, side string, name ScopeName) error {
+	if len(scope.User) > 0 || len(scope.ByOrgID) > 0 {
+		return xerrors.Errorf("%s scope %q grants org or user permissions, which coverage does not model", side, name)
+	}
+	for _, perm := range scope.Site {
+		if perm.Negate {
+			return xerrors.Errorf("%s scope %q carries a negative permission, which coverage does not model", side, name)
+		}
+	}
+	if !allowListContainsAll(scope.AllowIDList) {
+		return xerrors.Errorf("%s scope %q carries a resource allow list, which coverage does not model", side, name)
+	}
+	return nil
+}
+
+// permissionCovered reports whether any granted permission subsumes needed,
+// treating the wildcard resource type and action as covering every value.
+//
+// granted must carry no negative permissions; checkCoverable refuses a scope
+// holding one before ScopesCover gets here. Skipping a negative leaves any
+// wildcard beside it free to match, so an "everything except delete" scope
+// would read as covering delete.
+func permissionCovered(needed Permission, granted []Permission) bool {
+	for _, perm := range granted {
+		if perm.ResourceType != needed.ResourceType && perm.ResourceType != policy.WildcardSymbol {
+			continue
+		}
+		if perm.Action != needed.Action && perm.Action != policy.WildcardSymbol {
+			continue
+		}
+		return true
+	}
+	return false
+}
@@ -85,13 +85,28 @@ var externalComposite = map[ScopeName]struct{}{
 	"coder:apikeys.manage_self": {},
 }

-// IsExternalScope returns true if the scope is public, including the
-// `all` and `application_connect` special scopes and the curated
-// low-level resource:action scopes.
+// scopeAliases maps the spellings accepted for backward compatibility onto the
+// names the api_key_scope enum stores. IsExternalScope accepts every key and
+// CanonicalScopeName rewrites it to its value, so the two agree by reading one
+// table rather than by keeping two switches in step. Drift between them is
+// worse in one direction than the other: a name accepted as public but not
+// rewritten is declared requestable and then fails to expand on every request
+// naming it.
+var scopeAliases = map[ScopeName]ScopeName{
+	"all":                 ScopeAll,
+	"application_connect": ScopeApplicationConnect,
+}
+
+// IsExternalScope returns true if the scope is public: the `all` and
+// `application_connect` aliases, the canonical `coder:all` and
+// `coder:application_connect`, a curated low-level resource:action scope, or a
+// curated composite `coder:*` scope.
 func IsExternalScope(name ScopeName) bool {
+	if _, ok := scopeAliases[name]; ok {
+		return true
+	}
 	switch name {
-	// Include `all` and `application_connect` for backward compatibility.
-	case "all", ScopeAll, "application_connect", ScopeApplicationConnect:
+	case ScopeAll, ScopeApplicationConnect:
 		return true
 	}
 	if _, ok := externalLowLevel[name]; ok {
@@ -104,9 +119,29 @@ func IsExternalScope(name ScopeName) bool {
 	return false
 }

-// ExternalScopeNames returns a sorted list of all public scopes, which
-// includes the `all` and `application_connect` special scopes, curated
-// low-level resource:action names, and curated composite coder:* scopes.
+// CanonicalScopeName maps the backward-compatibility aliases IsExternalScope
+// accepts onto the names the api_key_scope enum stores. Any other name is
+// returned unchanged.
+//
+// IsExternalScope answers whether a name may be requested; it does not answer
+// how that name is spelled once persisted. The aliases `all` and
+// `application_connect` are accepted but are not enum members, so a caller
+// that stores what it validated must canonicalize in between.
+func CanonicalScopeName(name ScopeName) ScopeName {
+	if canonical, ok := scopeAliases[name]; ok {
+		return canonical
+	}
+	return name
+}
+
+// ExternalScopeNames returns a sorted list of all public scopes: the canonical
+// `coder:all` and `coder:application_connect` spellings, the curated low-level
+// resource:action names, and the curated composite coder:* scopes.
+//
+// Every name returned is canonical, so the list omits the bare `all` and
+// `application_connect` aliases IsExternalScope also accepts. A caller matching
+// a client-supplied name against this list must run it through
+// CanonicalScopeName first, or reject a spelling the same package calls public.
 func ExternalScopeNames() []string {
 	names := make([]string, 0, len(externalLowLevel)+len(externalComposite)+2)
 	names = append(names, string(ScopeAll))
@@ -0,0 +1,167 @@
+package rbac
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/coder/coder/v2/coderd/rbac/policy"
+)
+
+var (
+	workspaceRead         = Permission{ResourceType: "workspace", Action: policy.ActionRead}
+	workspaceWildcard     = Permission{ResourceType: "workspace", Action: policy.WildcardSymbol}
+	workspaceDeleteNegate = Permission{ResourceType: "workspace", Action: policy.ActionDelete, Negate: true}
+)
+
+// coverableScope is the shape every ExpandScope result has: site permissions
+// only, wildcard allow list, no negatives.
+func coverableScope(perms ...Permission) Scope {
+	return Scope{
+		Role:        Role{Site: perms},
+		AllowIDList: []AllowListElement{AllowListAll()},
+	}
+}
+
+// TestScopeAliases asserts what the shared table exists to guarantee, which no
+// test outside this package can: every alias is public, resolves to a public
+// name, and resolves to one the RBAC layer can expand. A name IsExternalScope
+// calls public but ExpandScope rejects is requestable in name only, and every
+// request naming it fails. Iterating the table rather than naming the two
+// aliases means a third is covered the day it is added.
+func TestScopeAliases(t *testing.T) {
+	t.Parallel()
+
+	require.NotEmpty(t, scopeAliases)
+
+	for alias, canonical := range scopeAliases {
+		require.Truef(t, IsExternalScope(alias), "alias %q must be public", alias)
+		require.Equalf(t, canonical, CanonicalScopeName(alias), "alias %q", alias)
+
+		// An alias is a second spelling of a scope, not a scope of its own.
+		require.Truef(t, IsExternalScope(canonical), "canonical %q must be public", canonical)
+		_, err := ExpandScope(canonical)
+		require.NoErrorf(t, err, "canonical %q must expand", canonical)
+
+		// The alias itself does not expand, which is what makes
+		// canonicalization mandatory before storage or coverage rather than a
+		// tidying step callers may skip.
+		_, err = ExpandScope(alias)
+		require.Errorf(t, err, "alias %q must not expand directly", alias)
+
+		// The list a client reads offers the canonical spelling and only that
+		// one, so a caller can request a name from it and store what it
+		// requested. Listing the alias too would offer two names for one scope,
+		// one of which fails to expand once stored.
+		require.NotContainsf(t, ExternalScopeNames(), string(alias), "list must omit alias %q", alias)
+		require.Containsf(t, ExternalScopeNames(), string(canonical), "list must offer %q", canonical)
+	}
+}
+
+// TestScopesCoverGuards drives Scope values that no catalog entry produces.
+// The guards exist for authority ScopeName inputs cannot express today, so
+// ScopesCover cannot reach them and they would otherwise ship unverified.
+//
+// Each shape runs on both sides of the comparison, with the opposite side
+// coverable, so a guard consulted on only one side fails here.
+func TestScopesCoverGuards(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name    string
+		scope   Scope
+		wantErr string
+	}{
+		{
+			name:  "SitePermissionsOnly",
+			scope: coverableScope(workspaceRead),
+		},
+		{
+			name: "UserPermission",
+			scope: Scope{
+				Role: Role{
+					Site: []Permission{workspaceRead},
+					User: []Permission{workspaceRead},
+				},
+				AllowIDList: []AllowListElement{AllowListAll()},
+			},
+			wantErr: "grants org or user permissions",
+		},
+		{
+			// The case the guards were added for: a scope granting every
+			// workspace action except delete. The permission coverage reads is
+			// harmless, and the one it does not read carves delete back out, so
+			// comparing on Site alone would answer a request for
+			// workspace:delete from a wildcard the scope has already qualified.
+			name: "NegativeUserPermission",
+			scope: Scope{
+				Role: Role{
+					Site: []Permission{workspaceWildcard},
+					User: []Permission{workspaceDeleteNegate},
+				},
+				AllowIDList: []AllowListElement{AllowListAll()},
+			},
+			wantErr: "grants org or user permissions",
+		},
+		{
+			name: "OrgPermission",
+			scope: Scope{
+				Role: Role{
+					Site:    []Permission{workspaceRead},
+					ByOrgID: map[string]OrgPermissions{"00000000-0000-0000-0000-000000000001": {}},
+				},
+				AllowIDList: []AllowListElement{AllowListAll()},
+			},
+			wantErr: "grants org or user permissions",
+		},
+		{
+			name:    "NegativeSitePermission",
+			scope:   coverableScope(workspaceWildcard, workspaceDeleteNegate),
+			wantErr: "carries a negative permission",
+		},
+		{
+			name: "NarrowedAllowList",
+			scope: Scope{
+				Role:        Role{Site: []Permission{workspaceRead}},
+				AllowIDList: []AllowListElement{{Type: "workspace", ID: "00000000-0000-0000-0000-000000000002"}},
+			},
+			wantErr: "carries a resource allow list",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
+			// The opposite side always covers, so a guard error stays
+			// distinguishable from an ordinary uncovered result.
+			cleanGrant := namedScope{name: "clean_scope", scope: coverableScope(workspaceWildcard)}
+			cleanRequest := namedScope{name: "clean_scope", scope: coverableScope(workspaceRead)}
+			under := namedScope{name: "test_scope", scope: test.scope}
+
+			sides := []struct {
+				side      string
+				allowed   []namedScope
+				requested namedScope
+			}{
+				{side: coverageSideRequested, allowed: []namedScope{cleanGrant}, requested: under},
+				{side: coverageSideAllowed, allowed: []namedScope{under}, requested: cleanRequest},
+			}
+
+			for _, args := range sides {
+				side := args.side
+				got, err := scopesCoverExpanded(args.allowed, args.requested)
+				if test.wantErr == "" {
+					require.NoErrorf(t, err, "side %q", side)
+					require.Truef(t, got, "side %q", side)
+					continue
+				}
+				require.ErrorContainsf(t, err, test.wantErr, "side %q", side)
+				// The side names itself, so an operator reading the error can
+				// tell which half of the comparison was undecidable.
+				require.ErrorContainsf(t, err, side+` scope "test_scope"`, "side %q", side)
+				require.Falsef(t, got, "an undecided comparison must not report coverage, side %q", side)
+			}
+		})
+	}
+}
@@ -61,3 +61,194 @@ func TestExpandScope(t *testing.T) {
 		}
 	})
 }
+
+func TestScopesCover(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name      string
+		allowed   []rbac.ScopeName
+		requested rbac.ScopeName
+		want      bool
+		// wantErrContains names the side that could not be expanded. Asserting
+		// the side keeps rows that fail through different paths from passing
+		// on each other's errors.
+		wantErrContains string
+	}{
+		{
+			name:      "IdenticalName",
+			allowed:   []rbac.ScopeName{"workspace:read"},
+			requested: "workspace:read",
+			want:      true,
+		},
+		{
+			// Name matching alone cannot decide this: coder:workspaces.access
+			// never mentions workspace:ssh by name, but its expansion covers it.
+			name:      "CompositeCoversItsMember",
+			allowed:   []rbac.ScopeName{"coder:workspaces.access"},
+			requested: "workspace:ssh",
+			want:      true,
+		},
+		{
+			// The composite grants no permission on this resource at all.
+			name:      "CompositeDoesNotCoverUnrelatedResource",
+			allowed:   []rbac.ScopeName{"coder:workspaces.access"},
+			requested: "user_secret:delete",
+			want:      false,
+		},
+		{
+			// Same resource, action the composite does not grant. Coverage
+			// compares the pair, not the resource alone.
+			name:      "CompositeDoesNotCoverUngrantedActionOnSameResource",
+			allowed:   []rbac.ScopeName{"coder:workspaces.access"},
+			requested: "workspace:delete",
+			want:      false,
+		},
+		{
+			name:      "AllCoversEverything",
+			allowed:   []rbac.ScopeName{rbac.ScopeAll},
+			requested: "user_secret:delete",
+			want:      true,
+		},
+		{
+			name:      "NarrowScopeDoesNotCoverAll",
+			allowed:   []rbac.ScopeName{"workspace:read"},
+			requested: rbac.ScopeAll,
+			want:      false,
+		},
+		{
+			name:      "ResourceWildcardCoversOneAction",
+			allowed:   []rbac.ScopeName{"workspace:*"},
+			requested: "workspace:ssh",
+			want:      true,
+		},
+		{
+			name:      "OneActionDoesNotCoverResourceWildcard",
+			allowed:   []rbac.ScopeName{"workspace:ssh"},
+			requested: "workspace:*",
+			want:      false,
+		},
+		{
+			// A composite is covered only when every permission it expands
+			// to is granted, so a strict subset of them is not enough.
+			name:      "PartialUnionDoesNotCoverComposite",
+			allowed:   []rbac.ScopeName{"template:read", "file:create"},
+			requested: "coder:templates.build",
+			want:      false,
+		},
+		{
+			// The allowed side is a union rather than a set of independent
+			// candidates, so one composite's permissions may be drawn from
+			// several allowed entries at once.
+			name:      "UnionOfAllowedScopesCoversComposite",
+			allowed:   []rbac.ScopeName{"template:read", "file:*", "provisioner_jobs:read"},
+			requested: "coder:templates.build",
+			want:      true,
+		},
+		{
+			name:      "EmptyAllowedCoversNothing",
+			allowed:   nil,
+			requested: "workspace:read",
+			want:      false,
+		},
+		{
+			// Not a false: a caller cannot distinguish "known and not
+			// covered" from "we could not tell", so an undecidable
+			// comparison is surfaced rather than answered.
+			name:            "UnknownRequestedScopeErrors",
+			allowed:         []rbac.ScopeName{rbac.ScopeAll},
+			requested:       "not_a_real_scope",
+			wantErrContains: "expand requested scope",
+		},
+		{
+			name:            "UnknownAllowedScopeErrors",
+			allowed:         []rbac.ScopeName{"not_a_real_scope"},
+			requested:       "workspace:read",
+			wantErrContains: "expand allowed scope",
+		},
+		{
+			// The aliases IsExternalScope accepts are not expandable names,
+			// so callers must canonicalize before asking about coverage.
+			name:            "NonCanonicalAliasErrorsRequestedAll",
+			allowed:         []rbac.ScopeName{rbac.ScopeAll},
+			requested:       "all",
+			wantErrContains: "expand requested scope",
+		},
+		{
+			name:            "NonCanonicalAliasErrorsRequestedApplicationConnect",
+			allowed:         []rbac.ScopeName{rbac.ScopeAll},
+			requested:       "application_connect",
+			wantErrContains: "expand requested scope",
+		},
+		{
+			// The only row that would catch someone canonicalizing inside the
+			// allowed loop, which would silently widen what an allow list
+			// accepts without any caller asking for it.
+			name:            "NonCanonicalAliasErrorsAllowedApplicationConnect",
+			allowed:         []rbac.ScopeName{"application_connect"},
+			requested:       rbac.ScopeApplicationConnect,
+			wantErrContains: "expand allowed scope",
+		},
+		{
+			name:            "NonCanonicalAliasErrorsAllowedAll",
+			allowed:         []rbac.ScopeName{"all"},
+			requested:       rbac.ScopeAll,
+			wantErrContains: "expand allowed scope",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
+			got, err := rbac.ScopesCover(test.allowed, test.requested)
+			if test.wantErrContains != "" {
+				require.ErrorContains(t, err, test.wantErrContains)
+				require.False(t, got, "an undecided comparison must not report coverage")
+				return
+			}
+			require.NoError(t, err)
+			require.Equal(t, test.want, got)
+		})
+	}
+}
+
+// TestCanonicalScopeName pins the alias mapping. Without it the two case arms
+// could be swapped, so that requesting `all` persisted application_connect and
+// the reverse, and the suite would stay green.
+func TestCanonicalScopeName(t *testing.T) {
+	t.Parallel()
+
+	require.Equal(t, rbac.ScopeAll, rbac.CanonicalScopeName("all"))
+	require.Equal(t, rbac.ScopeApplicationConnect, rbac.CanonicalScopeName("application_connect"))
+
+	// Canonical names and low-level names are returned unchanged, so callers
+	// can canonicalize unconditionally.
+	require.Equal(t, rbac.ScopeAll, rbac.CanonicalScopeName(rbac.ScopeAll))
+	require.Equal(t, rbac.ScopeApplicationConnect, rbac.CanonicalScopeName(rbac.ScopeApplicationConnect))
+	require.Equal(t, rbac.ScopeName("workspace:read"), rbac.CanonicalScopeName("workspace:read"))
+	require.Equal(t, rbac.ScopeName("not_a_real_scope"), rbac.CanonicalScopeName("not_a_real_scope"))
+}
+
+// TestScopesCoverEveryExternalScope asserts the property the OAuth2 allowlist
+// check depends on: coder:all is a ceiling over the whole external catalog, and
+// every catalog name covers itself. A name that cannot be compared at all would
+// otherwise reject every request naming it, which is a rejection no app owner
+// could act on.
+func TestScopesCoverEveryExternalScope(t *testing.T) {
+	t.Parallel()
+
+	// The aliases inherit this without being named here: TestScopeAliases pins
+	// that each one canonicalizes to a name this list offers.
+	for _, name := range rbac.ExternalScopeNames() {
+		scope := rbac.ScopeName(name)
+
+		covered, err := rbac.ScopesCover([]rbac.ScopeName{rbac.ScopeAll}, scope)
+		require.NoErrorf(t, err, "coder:all vs %q", scope)
+		require.Truef(t, covered, "coder:all must cover %q", scope)
+
+		covered, err = rbac.ScopesCover([]rbac.ScopeName{scope}, scope)
+		require.NoErrorf(t, err, "%q vs itself", scope)
+		require.Truef(t, covered, "%q must cover itself", scope)
+	}
+}