chore: protect reserved builtin rolenames (#13571)

Emyrk · web-flow · commit 3d30c8dc6858 · 2024-06-13T15:12:37.000-05:00
Conflicting built-in and database role names makes it hard to
disambiguate
diff --git a/coderd/rbac/roles.go b/coderd/rbac/roles.go
@@ -195,6 +195,13 @@ type RoleOptions struct {
 	NoOwnerWorkspaceExec bool
 }
 
+// ReservedRoleName exists because the database should only allow unique role
+// names, but some roles are built in. So these names are reserved
+func ReservedRoleName(name string) bool {
+	_, ok := builtInRoles[name]
+	return ok
+}
+
 // ReloadBuiltinRoles loads the static roles into the builtInRoles map.
 // This can be called again with a different config to change the behavior.
 //
diff --git a/enterprise/coderd/roles.go b/enterprise/coderd/roles.go
@@ -11,6 +11,7 @@ import (
 	"github.com/coder/coder/v2/coderd/database"
 	"github.com/coder/coder/v2/coderd/database/db2sdk"
 	"github.com/coder/coder/v2/coderd/httpapi"
+	"github.com/coder/coder/v2/coderd/rbac"
 	"github.com/coder/coder/v2/coderd/rbac/policy"
 	"github.com/coder/coder/v2/codersdk"
 )
@@ -41,6 +42,16 @@ func (h enterpriseCustomRoleHandler) PatchOrganizationRole(ctx context.Context,
 	)
 	defer commitAudit()
 
+	// This check is not ideal, but we cannot enforce a unique role name in the db against
+	// the built-in role names.
+	if rbac.ReservedRoleName(role.Name) {
+		httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
+			Message: "Reserved role name",
+			Detail:  fmt.Sprintf("%q is a reserved role name, and not allowed to be used", role.Name),
+		})
+		return codersdk.Role{}, false
+	}
+
 	if err := httpapi.NameValid(role.Name); err != nil {
 		httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
 			Message: "Invalid role name",
diff --git a/enterprise/coderd/roles_test.go b/enterprise/coderd/roles_test.go
@@ -210,6 +210,34 @@ func TestCustomOrganizationRole(t *testing.T) {
 		require.ErrorContains(t, err, "Validation")
 	})
 
+	t.Run("ReservedName", func(t *testing.T) {
+		t.Parallel()
+		dv := coderdtest.DeploymentValues(t)
+		dv.Experiments = []string{string(codersdk.ExperimentCustomRoles)}
+		owner, first := coderdenttest.New(t, &coderdenttest.Options{
+			Options: &coderdtest.Options{
+				DeploymentValues: dv,
+			},
+			LicenseOptions: &coderdenttest.LicenseOptions{
+				Features: license.Features{
+					codersdk.FeatureCustomRoles: 1,
+				},
+			},
+		})
+
+		ctx := testutil.Context(t, testutil.WaitMedium)
+
+		//nolint:gocritic // owner is required for this
+		_, err := owner.PatchOrganizationRole(ctx, first.OrganizationID, codersdk.Role{
+			Name:                    "owner", // Reserved
+			DisplayName:             "Testing Purposes",
+			SitePermissions:         nil,
+			OrganizationPermissions: nil,
+			UserPermissions:         nil,
+		})
+		require.ErrorContains(t, err, "Reserved")
+	})
+
 	t.Run("MismatchedOrganizations", func(t *testing.T) {
 		t.Parallel()
 		dv := coderdtest.DeploymentValues(t)
