chore: audit custom role changes

coder · Emyrk · Jun 4, 2024 · Jun 5, 2024 · Jun 5, 2024 · Jun 5, 2024
commit b873065d25ec7936926cf9abec5a0dea5855f47d
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/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -1187,11 +1187,7 @@ func (q *FakeQuerier) CustomRoles(_ context.Context, arg database.CustomRolesPar
 		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())
-				}
-				return strings.EqualFold(s, roleName)
+				return strings.EqualFold(s, role.Name)
 			}) {
 				continue
 			}
@@ -8405,6 +8401,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/000215_org_custom_role_audit.down.sql b/coderd/database/migrations/000215_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/000215_org_custom_role_audit.up.sql b/coderd/database/migrations/000215_org_custom_role_audit.up.sql
@@ -0,0 +1,6 @@
+-- (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
@@ -8,10 +8,7 @@ WHERE
   -- 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 [])
+    name ILIKE ANY(@lookup_roles :: text [])
     ELSE true
   END
   -- Org scoping filter, to only fetch site wide roles

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
 	}

diff --git a/codersdk/audit.go b/codersdk/audit.go
@@ -30,6 +30,7 @@ const (
 	ResourceTypeOAuth2ProviderApp ResourceType = "oauth2_provider_app"
 	// nolint:gosec // This is not a secret.
 	ResourceTypeOAuth2ProviderAppSecret ResourceType = "oauth2_provider_app_secret"
+	ResourceTypeCustomRole              ResourceType = "custom_role"
 )
 
 func (r ResourceType) FriendlyString() string {
@@ -66,6 +67,8 @@ func (r ResourceType) FriendlyString() string {
 		return "oauth2 app"
 	case ResourceTypeOAuth2ProviderAppSecret:
 		return "oauth2 app secret"
+	case ResourceTypeCustomRole:
+		return "custom role"
 	default:
 		return "unknown"
 	}

diff --git a/enterprise/audit/diff.go b/enterprise/audit/diff.go
@@ -144,6 +144,19 @@ func convertDiffType(left, right any) (newLeft, newRight any, changed bool) {
 		return leftInt64Ptr, rightInt64Ptr, true
 	case database.TemplateACL:
 		return fmt.Sprintf("%+v", left), fmt.Sprintf("%+v", right), true
+	case database.CustomRolePermissions:
+		leftArr := make([]string, 0)
+		rightArr := make([]string, 0)
+		for _, p := range typedLeft {
+			leftArr = append(leftArr, p.String())
+		}
+		for _, p := range right.(database.CustomRolePermissions) {
+			rightArr = append(rightArr, p.String())
+		}
+
+		// String representation is much easier to visually inspect
+		//typedRight := right.(database.CustomRolePermission)
+		return leftArr, rightArr, true
 	default:
 		return left, right, false
 	}

diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go
@@ -50,6 +50,18 @@ type Table map[string]map[string]Action
 var AuditableResources = auditMap(auditableResourcesTypes)
 
 var auditableResourcesTypes = map[any]map[string]Action{
+	&database.CustomRole{}: {
+		"name":             ActionTrack,
+		"display_name":     ActionTrack,
+		"site_permissions": ActionTrack,
+		"org_permissions":  ActionTrack,
+		"user_permissions": ActionTrack,
+		"organization_id":  ActionTrack,
+
+		"id":         ActionIgnore,
+		"created_at": ActionIgnore,
+		"updated_at": ActionIgnore,
+	},
 	&database.GitSSHKey{}: {
 		"user_id":     ActionTrack,
 		"created_at":  ActionIgnore, // Never changes, but is implicit and not helpful in a diff.

diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go
@@ -746,7 +746,7 @@ func (api *API) updateEntitlements(ctx context.Context) error {
 	}
 
 	if initial, changed, enabled := featureChanged(codersdk.FeatureCustomRoles); shouldUpdate(initial, changed, enabled) {
-		var handler coderd.CustomRoleHandler = &enterpriseCustomRoleHandler{Enabled: enabled}
+		var handler coderd.CustomRoleHandler = &enterpriseCustomRoleHandler{API: api, Enabled: enabled}
 		api.AGPL.CustomRoleHandler.Store(&handler)
 	}
 

diff --git a/enterprise/coderd/roles.go b/enterprise/coderd/roles.go
@@ -7,6 +7,7 @@ import (
 
 	"github.com/google/uuid"
 
+	"github.com/coder/coder/v2/coderd/audit"
 	"github.com/coder/coder/v2/coderd/database"
 	"github.com/coder/coder/v2/coderd/database/db2sdk"
 	"github.com/coder/coder/v2/coderd/httpapi"
@@ -15,17 +16,31 @@ import (
 )
 
 type enterpriseCustomRoleHandler struct {
+	API     *API
 	Enabled bool
 }
 
-func (h enterpriseCustomRoleHandler) PatchOrganizationRole(ctx context.Context, db database.Store, rw http.ResponseWriter, orgID uuid.UUID, role codersdk.Role) (codersdk.Role, bool) {
+func (h enterpriseCustomRoleHandler) PatchOrganizationRole(ctx context.Context, rw http.ResponseWriter, r *http.Request, orgID uuid.UUID, role codersdk.Role) (codersdk.Role, bool) {
 	if !h.Enabled {
 		httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
 			Message: "Custom roles are not enabled",
 		})
 		return codersdk.Role{}, false
 	}
 
+	var (
+		db                = h.API.Database
+		auditor           = h.API.AGPL.Auditor.Load()
+		aReq, commitAudit = audit.InitRequest[database.CustomRole](rw, &audit.RequestParams{
+			Audit:          *auditor,
+			Log:            h.API.Logger,
+			Request:        r,
+			Action:         database.AuditActionWrite,
+			OrganizationID: orgID,
+		})
+	)
+	defer commitAudit()
+
 	if err := httpapi.NameValid(role.Name); err != nil {
 		httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
 			Message: "Invalid role name",
@@ -59,6 +74,21 @@ func (h enterpriseCustomRoleHandler) PatchOrganizationRole(ctx context.Context,
 		return codersdk.Role{}, false
 	}
 
+	originalRoles, err := db.CustomRoles(ctx, database.CustomRolesParams{
+		LookupRoles:     []string{role.Name},
+		ExcludeOrgRoles: false,
+		OrganizationID:  orgID,
+	})
+	// If it is a 404 (not found) error, ignore it.
+	if err != nil && !httpapi.Is404Error(err) {
+		httpapi.InternalServerError(rw, err)
+		return codersdk.Role{}, false
+	}
+	if len(originalRoles) == 1 {
+		// For auditing changes to a role.
+		aReq.Old = originalRoles[1]
+	}
+
 	inserted, err := db.UpsertCustomRole(ctx, database.UpsertCustomRoleParams{
 		Name:        role.Name,
 		DisplayName: role.DisplayName,
@@ -81,6 +111,7 @@ func (h enterpriseCustomRoleHandler) PatchOrganizationRole(ctx context.Context,
 		})
 		return codersdk.Role{}, false
 	}
+	aReq.New = inserted
 
 	return db2sdk.Role(inserted), true
 }

diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts
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;