chore: verify validity of built in rbac roles (#13296)

Emyrk · web-flow · commit 194be12133f0 · 2024-05-16T12:07:44.000-05:00
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.
diff --git 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
@@ -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
@@ -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
@@ -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 {
