diff --git a/coderd/rbac/scopes.go b/coderd/rbac/scopes.go index 7cbec46d741..a69628ae178 100644 --- a/coderd/rbac/scopes.go +++ b/coderd/rbac/scopes.go @@ -318,3 +318,94 @@ 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 is the semantic form +// of "is this request within this ceiling", as opposed to comparing the names +// themselves: `coder:workspaces.access` covers `workspace:read` because it +// expands to include it, and `coder:all` covers everything. +// +// Names must be canonical (see CanonicalScopeName). An unknown name on either +// side is an error rather than a false, since a caller cannot tell those apart +// safely. +// +// The comparison is deliberately asymmetric about what it ignores. Positive +// permissions on the allowed side that this does not model are dropped, which +// can only make the answer stricter. Anything on the requested side that is +// not modeled fails closed instead, because ignoring it would answer +// "covered" about authority that was never compared. +// +// Negative permissions are the exception to that asymmetry and fail closed on +// both sides. Dropping an anti-grant from the ceiling would widen it, so the +// direction that makes the rest of the allowed side safe to ignore does not +// hold for them. +func ScopesCover(allowed []ScopeName, requested ScopeName) (bool, error) { + want, err := ExpandScope(requested) + if err != nil { + return false, xerrors.Errorf("expand requested scope: %w", err) + } + // Scope expansion populates Site only, with a wildcard allow list and no + // negative permissions. These guards hold that invariant: if a future + // scope breaks it, coverage stops being decidable here and the request is + // refused rather than approved on an incomplete comparison. + if len(want.User) > 0 || len(want.ByOrgID) > 0 { + return false, xerrors.Errorf("scope %q grants org or user permissions, which coverage does not model", requested) + } + for _, perm := range want.Site { + if perm.Negate { + return false, xerrors.Errorf("scope %q carries a negative permission, which coverage does not model", requested) + } + } + if !allowListContainsAll(want.AllowIDList) { + return false, xerrors.Errorf("scope %q carries a resource allow list, which coverage does not model", requested) + } + + granted := make([]Permission, 0, len(allowed)*4) + for _, name := range allowed { + expanded, err := ExpandScope(name) + if err != nil { + return false, xerrors.Errorf("expand allowed scope %q: %w", name, err) + } + // A narrower allow list on the allowed side would make these + // permissions conditional, and treating them as unconditional would + // overstate the ceiling. + if !allowListContainsAll(expanded.AllowIDList) { + return false, xerrors.Errorf("allowed scope %q carries a resource allow list, which coverage does not model", name) + } + // A negative permission is the one thing on this side that cannot be + // dropped safely. Ignoring an unmodelled grant narrows the ceiling, + // but ignoring an anti-grant widens it: an "everything except delete" + // scope would otherwise cover a request for delete. + for _, perm := range expanded.Site { + if perm.Negate { + return false, xerrors.Errorf("allowed scope %q carries a negative permission, which coverage does not model", name) + } + } + granted = append(granted, expanded.Site...) + } + + for _, needed := range want.Site { + if !permissionCovered(needed, granted) { + return false, nil + } + } + return true, nil +} + +// permissionCovered reports whether any granted permission subsumes needed, +// treating the wildcard resource type and action as covering every value. +func permissionCovered(needed Permission, granted []Permission) bool { + for _, perm := range granted { + if perm.Negate { + continue + } + if perm.ResourceType != needed.ResourceType && perm.ResourceType != policy.WildcardSymbol { + continue + } + if perm.Action != needed.Action && perm.Action != policy.WildcardSymbol { + continue + } + return true + } + return false +} diff --git a/coderd/rbac/scopes_catalog.go b/coderd/rbac/scopes_catalog.go index 04304681a69..be129b204fe 100644 --- a/coderd/rbac/scopes_catalog.go +++ b/coderd/rbac/scopes_catalog.go @@ -104,6 +104,24 @@ func IsExternalScope(name ScopeName) bool { return false } +// 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 { + switch name { + case "all": + return ScopeAll + case "application_connect": + return ScopeApplicationConnect + } + return name +} + // 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. diff --git a/coderd/rbac/scopes_test.go b/coderd/rbac/scopes_test.go index 270f6ff0285..27a0171287f 100644 --- a/coderd/rbac/scopes_test.go +++ b/coderd/rbac/scopes_test.go @@ -61,3 +61,151 @@ func TestExpandScope(t *testing.T) { } }) } + +func TestScopesCover(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + allowed []rbac.ScopeName + requested rbac.ScopeName + want bool + wantErr bool + }{ + { + name: "IdenticalName", + allowed: []rbac.ScopeName{"workspace:read"}, + requested: "workspace:read", + want: true, + }, + { + // The case name matching cannot answer: the composite expands to + // include the requested permission, so the request is within the + // authority the composite already grants. + name: "CompositeCoversItsMember", + allowed: []rbac.ScopeName{"coder:workspaces.access"}, + requested: "workspace:ssh", + want: true, + }, + { + name: "CompositeDoesNotCoverNonMember", + allowed: []rbac.ScopeName{"coder:workspaces.access"}, + requested: "workspace:delete", + want: false, + }, + { + // Same resource, different action. Coverage compares the pair, + // not the resource alone. + name: "CompositeDoesNotCoverWiderActionOnCoveredResource", + allowed: []rbac.ScopeName{"coder:workspaces.access"}, + requested: "template:update", + 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", + wantErr: true, + }, + { + name: "UnknownAllowedScopeErrors", + allowed: []rbac.ScopeName{"not_a_real_scope"}, + requested: "workspace:read", + wantErr: true, + }, + { + // The aliases IsExternalScope accepts are not expandable names, + // so callers must canonicalize before asking about coverage. + name: "NonCanonicalAliasErrors", + allowed: []rbac.ScopeName{rbac.ScopeAll}, + requested: "all", + wantErr: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + got, err := rbac.ScopesCover(test.allowed, test.requested) + if test.wantErr { + require.Error(t, err) + require.False(t, got, "an undecided comparison must not report coverage") + return + } + require.NoError(t, err) + require.Equal(t, test.want, got) + }) + } +} + +// 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() + + for _, name := range rbac.ExternalScopeNames() { + canonical := rbac.CanonicalScopeName(rbac.ScopeName(name)) + + covered, err := rbac.ScopesCover([]rbac.ScopeName{rbac.ScopeAll}, canonical) + require.NoErrorf(t, err, "coder:all vs %q", canonical) + require.Truef(t, covered, "coder:all must cover %q", canonical) + + covered, err = rbac.ScopesCover([]rbac.ScopeName{canonical}, canonical) + require.NoErrorf(t, err, "%q vs itself", canonical) + require.Truef(t, covered, "%q must cover itself", canonical) + } +}