coder · Emyrk · Jun 4, 2024 · Jun 5, 2024 · Jun 5, 2024
diff --git a/coderd/audit/diff.go b/coderd/audit/diff.go
@@ -21,7 +21,8 @@ type Auditable interface {
 		database.AuditOAuthConvertState |
 		database.HealthSettings |
 		database.OAuth2ProviderApp |
-		database.OAuth2ProviderAppSecret
+		database.OAuth2ProviderAppSecret |
+		database.CustomRole
 }
 
 // Map is a map of changed fields in an audited resource. It maps field names to

diff --git a/coderd/audit/request.go b/coderd/audit/request.go
@@ -103,6 +103,8 @@ func ResourceTarget[T Auditable](tgt T) string {
 		return typed.Name
 	case database.OAuth2ProviderAppSecret:
 		return typed.DisplaySecret
+	case database.CustomRole:
+		return typed.Name
 	default:
 		panic(fmt.Sprintf("unknown resource %T for ResourceTarget", tgt))
 	}
@@ -140,6 +142,8 @@ func ResourceID[T Auditable](tgt T) uuid.UUID {
 		return typed.ID
 	case database.OAuth2ProviderAppSecret:
 		return typed.ID
+	case database.CustomRole:
+		return typed.ID
 	default:
 		panic(fmt.Sprintf("unknown resource %T for ResourceID", tgt))
 	}
@@ -175,6 +179,8 @@ func ResourceType[T Auditable](tgt T) database.ResourceType {
 		return database.ResourceTypeOauth2ProviderApp
 	case database.OAuth2ProviderAppSecret:
 		return database.ResourceTypeOauth2ProviderAppSecret
+	case database.CustomRole:
+		return database.ResourceTypeCustomRole
 	default:
 		panic(fmt.Sprintf("unknown resource %T for ResourceType", typed))
 	}
@@ -211,6 +217,8 @@ func ResourceRequiresOrgID[T Auditable]() bool {
 		return false
 	case database.OAuth2ProviderAppSecret:
 		return false
+	case database.CustomRole:
+		return true
 	default:
 		panic(fmt.Sprintf("unknown resource %T for ResourceRequiresOrgID", tgt))
 	}

diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go
@@ -758,6 +758,8 @@ func createAnotherUserRetry(t testing.TB, client *codersdk.Client, organizationI
 			roleName, _, err = rbac.RoleSplit(roleName)
 			require.NoError(t, err, "split org role name")
 			if ok {
+				roleName, _, err = rbac.RoleSplit(roleName)
+				require.NoError(t, err, "split rolename")
 				orgRoles[orgID] = append(orgRoles[orgID], roleName)
 			} else {
 				siteRoles = append(siteRoles, roleName)

diff --git a/coderd/database/dbauthz/customroles_test.go b/coderd/database/dbauthz/customroles_test.go
@@ -38,7 +38,7 @@ func TestUpsertCustomRoles(t *testing.T) {
 		Name:        "can-assign",
 		DisplayName: "",
 		Site: rbac.Permissions(map[string][]policy.Action{
-			rbac.ResourceAssignRole.Type: {policy.ActionCreate},
+			rbac.ResourceAssignRole.Type: {policy.ActionRead, policy.ActionCreate},
 		}),
 	}
 
@@ -243,6 +243,19 @@ func TestUpsertCustomRoles(t *testing.T) {
 				require.ErrorContains(t, err, tc.errorContains)
 			} else {
 				require.NoError(t, err)
+
+				roles, err := az.CustomRoles(ctx, database.CustomRolesParams{
+					LookupRoles: []database.NameOrganizationPair{
+						{
+							Name:           "test-role",
+							OrganizationID: tc.organizationID.UUID,
+						},
+					},
+					ExcludeOrgRoles: false,
+					OrganizationID:  uuid.UUID{},
+				})
+				require.NoError(t, err)
+				require.Len(t, roles, 1)
 			}
 		})
 	}

diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -1186,12 +1186,16 @@ func (q *FakeQuerier) CustomRoles(_ context.Context, arg database.CustomRolesPar
 	for _, role := range q.data.customRoles {
 		role := role
 		if len(arg.LookupRoles) > 0 {
-			if !slices.ContainsFunc(arg.LookupRoles, func(s string) bool {
-				roleName := rbac.RoleName(role.Name, "")
-				if role.OrganizationID.UUID != uuid.Nil {
-					roleName = rbac.RoleName(role.Name, role.OrganizationID.UUID.String())
+			if !slices.ContainsFunc(arg.LookupRoles, func(s database.NameOrganizationPair) bool {
+				if !strings.EqualFold(s.Name, role.Name) {
+					return false
 				}
-				return strings.EqualFold(s, roleName)
+
+				if s.OrganizationID == uuid.Nil {
+					return true
+				}
+
+				return s.OrganizationID == arg.OrganizationID
 			}) {
 				continue
 			}
@@ -8405,6 +8409,7 @@ func (q *FakeQuerier) UpsertCustomRole(_ context.Context, arg database.UpsertCus
 	}
 
 	role := database.CustomRole{
+		ID:              uuid.New(),
 		Name:            arg.Name,
 		DisplayName:     arg.DisplayName,
 		OrganizationID:  arg.OrganizationID,

diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql
diff --git a/coderd/database/migrations/000216_org_custom_role_audit.down.sql b/coderd/database/migrations/000216_org_custom_role_audit.down.sql
@@ -0,0 +1,2 @@
+DROP INDEX idx_custom_roles_id;
+ALTER TABLE custom_roles DROP COLUMN id;
diff --git a/coderd/database/migrations/000216_org_custom_role_audit.up.sql b/coderd/database/migrations/000216_org_custom_role_audit.up.sql
@@ -0,0 +1,9 @@
+-- A role does not need to belong to an organization
+ALTER TABLE custom_roles ALTER COLUMN organization_id DROP NOT NULL;
+
+-- (name) is the primary key, this column is almost exclusively for auditing.
+ALTER TABLE custom_roles ADD COLUMN id uuid DEFAULT gen_random_uuid() NOT NULL;
+
+-- Ensure unique uuids.
+CREATE INDEX idx_custom_roles_id ON custom_roles (id);
+ALTER TYPE resource_type ADD VALUE IF NOT EXISTS 'custom_role';
diff --git a/coderd/database/models.go b/coderd/database/models.go
diff --git a/coderd/database/querier.go b/coderd/database/querier.go
diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go
diff --git a/coderd/database/queries/roles.sql b/coderd/database/queries/roles.sql
@@ -7,11 +7,8 @@ WHERE
   true
   -- Lookup roles filter expects the role names to be in the rbac package
   -- format. Eg: name[:<organization_id>]
-  AND CASE WHEN array_length(@lookup_roles :: text[], 1) > 0  THEN
-	-- Case insensitive lookup with org_id appended (if non-null).
-    -- This will return just the name if org_id is null. It'll append
-    -- the org_id if not null
-	concat(name, NULLIF(concat(':', organization_id), ':')) ILIKE ANY(@lookup_roles :: text [])
+  AND CASE WHEN array_length(@lookup_roles :: name_organization_pair_list[], 1) > 0  THEN
+	(name, organization_id) ILIKE ANY (@lookup_roles::name_organization_pair_list[])
     ELSE true
   END
   -- Org scoping filter, to only fetch site wide roles

diff --git a/coderd/database/sqlc.yaml b/coderd/database/sqlc.yaml
@@ -28,6 +28,9 @@ sql:
         emit_enum_valid_method: true
         emit_all_enum_values: true
         overrides:
+          - db_type: "name_organization_pair_list"
+            go_type:
+              type: "NameOrganizationPair"
           - column: "custom_roles.site_permissions"
             go_type:
               type: "CustomRolePermissions"

diff --git a/coderd/database/types.go b/coderd/database/types.go
@@ -113,6 +113,26 @@
 	return json.Marshal(m)
 }
 
+// NameOrganizationPair is used as a lookup tuple for custom role rows.
+type NameOrganizationPair struct {
+	Name string `db:"name" json:"name"`
+	// OrganizationID if unset will assume a null column value
+	OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"`
+}
+
+func (a *NameOrganizationPair) Scan(src interface{}) error {
+	return xerrors.Errorf("unexpected type %T", src)
+}
+
+func (a NameOrganizationPair) Value() (driver.Value, error) {
+	var orgID interface{} = a.OrganizationID
+	if a.OrganizationID == uuid.Nil {
+		orgID = nil
+	}
+
+	return []interface{}{a.Name, orgID}, nil
+}
+
 type CustomRolePermissions []CustomRolePermission
 
 func (a *CustomRolePermissions) Scan(src interface{}) error {

diff --git a/coderd/rbac/rolestore/rolestore.go b/coderd/rbac/rolestore/rolestore.go
@@ -69,11 +69,34 @@ func Expand(ctx context.Context, db database.Store, names []string) (rbac.Roles,
 	}
 
 	if len(lookup) > 0 {
+		// The set of roles coming in are formatted as 'rolename[:<org_id>]'.
+		// In the database, org roles are scoped with an organization column.
+		lookupArgs := make([]database.NameOrganizationPair, 0, len(lookup))
+		for _, name := range lookup {
+			roleName, orgID, err := rbac.RoleSplit(name)
+			if err != nil {
+				continue
+			}
+
+			parsedOrgID := uuid.Nil // Default to no org ID
+			if orgID != "" {
+				parsedOrgID, err = uuid.Parse(orgID)
+				if err != nil {
+					continue
+				}
+			}
+
+			lookupArgs = append(lookupArgs, database.NameOrganizationPair{
+				Name:           roleName,
+				OrganizationID: parsedOrgID,
+			})
+		}
+
 		// If some roles are missing from the database, they are omitted from
 		// the expansion. These roles are no-ops. Should we raise some kind of
 		// warning when this happens?
 		dbroles, err := db.CustomRoles(ctx, database.CustomRolesParams{
-			LookupRoles:     lookup,
+			LookupRoles:     lookupArgs,
 			ExcludeOrgRoles: false,
 			OrganizationID:  uuid.Nil,
 		})

diff --git a/coderd/roles.go b/coderd/roles.go
@@ -20,12 +20,12 @@ import (
 // roles. Ideally only included in the enterprise package, but the routes are
 // intermixed with AGPL endpoints.
 type CustomRoleHandler interface {
-	PatchOrganizationRole(ctx context.Context, db database.Store, rw http.ResponseWriter, orgID uuid.UUID, role codersdk.Role) (codersdk.Role, bool)
+	PatchOrganizationRole(ctx context.Context, rw http.ResponseWriter, r *http.Request, orgID uuid.UUID, role codersdk.Role) (codersdk.Role, bool)
 }
 
 type agplCustomRoleHandler struct{}
 
-func (agplCustomRoleHandler) PatchOrganizationRole(ctx context.Context, _ database.Store, rw http.ResponseWriter, _ uuid.UUID, _ codersdk.Role) (codersdk.Role, bool) {
+func (agplCustomRoleHandler) PatchOrganizationRole(ctx context.Context, rw http.ResponseWriter, _ *http.Request, _ uuid.UUID, _ codersdk.Role) (codersdk.Role, bool) {
 	httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
 		Message: "Creating and updating custom roles is an Enterprise feature. Contact sales!",
 	})
@@ -54,7 +54,7 @@ func (api *API) patchOrgRoles(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	updated, ok := handler.PatchOrganizationRole(ctx, api.Database, rw, organization.ID, req)
+	updated, ok := handler.PatchOrganizationRole(ctx, rw, r, organization.ID, req)
 	if !ok {
 		return
 	}
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		DROP INDEX idx_custom_roles_id;
		ALTER TABLE custom_roles DROP COLUMN id;