coder · Emyrk · May 16, 2024 · May 16, 2024
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 {