chore: implement audit log for custom role edits

Emyrk · Emyrk · commit fb09e72d45ca · 2024-06-06T11:44:10.000-05:00
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
@@ -244,7 +244,6 @@ func TestUpsertCustomRoles(t *testing.T) {
 			} else {
 				require.NoError(t, err)
 
-				// Verify we can fetch the role
 				roles, err := az.CustomRoles(ctx, database.CustomRolesParams{
 					LookupRoles: []database.NameOrganizationPair{
 						{
diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -8410,6 +8410,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/000217_org_custom_role_audit.down.sql b/coderd/database/migrations/000217_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/000217_org_custom_role_audit.up.sql b/coderd/database/migrations/000217_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/queries.sql.go b/coderd/database/queries.sql.go
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,26 @@ func (h enterpriseCustomRoleHandler) PatchOrganizationRole(ctx context.Context,
 		return codersdk.Role{}, false
 	}
 
+	originalRoles, err := db.CustomRoles(ctx, database.CustomRolesParams{
+		LookupRoles: []database.NameOrganizationPair{
+			{
+				Name:           role.Name,
+				OrganizationID: orgID,
+			},
+		},
+		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[0]
+	}
+
 	inserted, err := db.UpsertCustomRole(ctx, database.UpsertCustomRoleParams{
 		Name:        role.Name,
 		DisplayName: role.DisplayName,
@@ -81,6 +116,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
`@@ -21,7 +21,8 @@ type Auditable interface {`
`21`	`21`	`database.AuditOAuthConvertState \|`
`22`	`22`	`database.HealthSettings \|`
`23`	`23`	`database.OAuth2ProviderApp \|`
`24`		`- database.OAuth2ProviderAppSecret`
	`24`	`+ database.OAuth2ProviderAppSecret \|`
	`25`	`+ database.CustomRole`
`25`	`26`	`}`
`26`	`27`
`27`	`28`	`// Map is a map of changed fields in an audited resource. It maps field names to`
Original file line number	Diff line number	Diff line change
`@@ -244,7 +244,6 @@ func TestUpsertCustomRoles(t *testing.T) {`
`244`	`244`	`} else {`
`245`	`245`	`require.NoError(t, err)`
`246`	`246`
`247`		`- // Verify we can fetch the role`
`248`	`247`	`roles, err := az.CustomRoles(ctx, database.CustomRolesParams{`
`249`	`248`	`LookupRoles: []database.NameOrganizationPair{`
`250`	`249`	`{`
Original file line number	Diff line number	Diff line change
`@@ -8410,6 +8410,7 @@ func (q *FakeQuerier) UpsertCustomRole(_ context.Context, arg database.UpsertCus`
`8410`	`8410`	`}`
`8411`	`8411`
`8412`	`8412`	`role := database.CustomRole{`
	`8413`	`+ ID: uuid.New(),`
`8413`	`8414`	`Name: arg.Name,`
`8414`	`8415`	`DisplayName: arg.DisplayName,`
`8415`	`8416`	`OrganizationID: arg.OrganizationID,`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+DROP INDEX idx_custom_roles_id;`
	`2`	`+ALTER TABLE custom_roles DROP COLUMN id;`