From 65ec79865b3a370b5cacfe461956bff821763ec5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 16 May 2024 09:49:55 -0500 Subject: [PATCH] chore: verify validity of built in rbac roles Also some xerrors linting cleanup --- coderd/rbac/object.go | 7 +++-- coderd/rbac/roles.go | 55 ++++++++++++++++++++++++++++++++++++++- coderd/rbac/roles_test.go | 21 +++++++++++++++ scripts/rbacgen/main.go | 10 ++++--- 4 files changed, 84 insertions(+), 9 deletions(-) diff --git a/coderd/rbac/object.go b/coderd/rbac/object.go index 30a74e4f825dd..dfd8ab6b55b23 100644 --- a/coderd/rbac/object.go +++ b/coderd/rbac/object.go @@ -1,9 +1,8 @@ package rbac import ( - "fmt" - "github.com/google/uuid" + "golang.org/x/xerrors" "github.com/coder/coder/v2/coderd/rbac/policy" ) @@ -36,10 +35,10 @@ type Object struct { func (z Object) ValidAction(action policy.Action) error { perms, ok := policy.RBACPermissions[z.Type] if !ok { - return fmt.Errorf("invalid type %q", z.Type) + return xerrors.Errorf("invalid type %q", z.Type) } if _, ok := perms.Actions[action]; !ok { - return fmt.Errorf("invalid action %q for type %q", action, z.Type) + return xerrors.Errorf("invalid action %q for type %q", action, z.Type) } return nil diff --git a/coderd/rbac/roles.go b/coderd/rbac/roles.go index cee365d06624c..fbac8ddf5379d 100644 --- a/coderd/rbac/roles.go +++ b/coderd/rbac/roles.go @@ -1,6 +1,7 @@ package rbac import ( + "errors" "sort" "strings" @@ -369,6 +370,30 @@ type Permission struct { Action policy.Action `json:"action"` } +func (perm Permission) Valid() error { + if perm.ResourceType == policy.WildcardSymbol { + // Wildcard is tricky to check. Just allow it. + return nil + } + + resource, ok := policy.RBACPermissions[perm.ResourceType] + if !ok { + return xerrors.Errorf("invalid resource type %q", perm.ResourceType) + } + + // Wildcard action is always valid + if perm.Action == policy.WildcardSymbol { + return nil + } + + _, ok = resource.Actions[perm.Action] + if !ok { + return xerrors.Errorf("invalid action %q for resource %q", perm.Action, perm.ResourceType) + } + + return nil +} + // Role is a set of permissions at multiple levels: // - Site level permissions apply EVERYWHERE // - Org level permissions apply to EVERYTHING in a given ORG @@ -393,6 +418,34 @@ type Role struct { cachedRegoValue ast.Value } +// Valid will check all it's permissions and ensure they are all correct +// according to the policy. This verifies every action specified make sense +// for the given resource. +func (role Role) Valid() error { + var errs []error + for _, perm := range role.Site { + if err := perm.Valid(); err != nil { + errs = append(errs, xerrors.Errorf("site: %w", err)) + } + } + + for orgID, permissions := range role.Org { + for _, perm := range permissions { + if err := perm.Valid(); err != nil { + errs = append(errs, xerrors.Errorf("org=%q: %w", orgID, err)) + } + } + } + + for _, perm := range role.User { + if err := perm.Valid(); err != nil { + errs = append(errs, xerrors.Errorf("user: %w", err)) + } + } + + return errors.Join(errs...) +} + type Roles []Role func (roles Roles) Expand() ([]Role, error) { @@ -402,7 +455,7 @@ func (roles Roles) Expand() ([]Role, error) { func (roles Roles) Names() []string { names := make([]string, 0, len(roles)) for _, r := range roles { - return append(names, r.Name) + names = append(names, r.Name) } return names } diff --git a/coderd/rbac/roles_test.go b/coderd/rbac/roles_test.go index 44ef83b74cd20..fe589449b8884 100644 --- a/coderd/rbac/roles_test.go +++ b/coderd/rbac/roles_test.go @@ -20,6 +20,27 @@ type authSubject struct { Actor rbac.Subject } +// TestBuiltInRoles makes sure our built-in roles are valid by our own policy +// rules. If this is incorrect, that is a mistake. +func TestBuiltInRoles(t *testing.T) { + t.Parallel() + for _, r := range rbac.SiteRoles() { + r := r + t.Run(r.Name, func(t *testing.T) { + t.Parallel() + require.NoError(t, r.Valid(), "invalid role") + }) + } + + for _, r := range rbac.OrganizationRoles(uuid.New()) { + r := r + t.Run(r.Name, func(t *testing.T) { + t.Parallel() + require.NoError(t, r.Valid(), "invalid role") + }) + } +} + //nolint:tparallel,paralleltest func TestOwnerExec(t *testing.T) { owner := rbac.Subject{ diff --git a/scripts/rbacgen/main.go b/scripts/rbacgen/main.go index 38f13434c77e4..1eb186c1b5ce4 100644 --- a/scripts/rbacgen/main.go +++ b/scripts/rbacgen/main.go @@ -16,6 +16,8 @@ import ( "slices" "strings" + "golang.org/x/xerrors" + "github.com/coder/coder/v2/coderd/rbac/policy" ) @@ -148,7 +150,7 @@ func generateRbacObjects(templateSource string) ([]byte, error) { // Parse the policy.go file for the action enums f, err := parser.ParseFile(token.NewFileSet(), "./coderd/rbac/policy/policy.go", nil, parser.ParseComments) if err != nil { - return nil, fmt.Errorf("parsing policy.go: %w", err) + return nil, xerrors.Errorf("parsing policy.go: %w", err) } actionMap := fileActions(f) actionList := make([]ActionDetails, 0) @@ -176,14 +178,14 @@ func generateRbacObjects(templateSource string) ([]byte, error) { x++ v, ok := actionMap[string(action)] if !ok { - errorList = append(errorList, fmt.Errorf("action value %q does not have a constant a matching enum constant", action)) + errorList = append(errorList, xerrors.Errorf("action value %q does not have a constant a matching enum constant", action)) } return v }, "concat": func(strs ...string) string { return strings.Join(strs, "") }, }).Parse(templateSource) if err != nil { - return nil, fmt.Errorf("parse template: %w", err) + return nil, xerrors.Errorf("parse template: %w", err) } // Convert to sorted list for autogen consistency. @@ -203,7 +205,7 @@ func generateRbacObjects(templateSource string) ([]byte, error) { err = tpl.Execute(&out, list) if err != nil { - return nil, fmt.Errorf("execute template: %w", err) + return nil, xerrors.Errorf("execute template: %w", err) } if len(errorList) > 0 {