From 194be12133f02e45103123ef4346a6dc5aec41fd Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 16 May 2024 12:07:44 -0500 Subject: [PATCH] chore: verify validity of built in rbac roles (#13296) Verifies our built in roles are valid according to our policy.go. Working on custom roles requires the dynamic roles to adhere to these rules. Feels fair the built in ones do too. --- 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 30a74e4f82..dfd8ab6b55 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 cee365d066..fbac8ddf53 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 44ef83b74c..fe589449b8 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 38f13434c7..1eb186c1b5 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 {