feat: add audit diffing for all user editable types (#1413)

coderd/audit/diff.go

@@ -1,8 +1,11 @@
 package audit
 
 import (
+	"database/sql"
 	"fmt"
 	"reflect"
+
+	"github.com/google/uuid"
 )
 
 // TODO: this might need to be in the database package.
@@ -64,6 +67,11 @@ func diffValues[T any](left, right T, table Table) Map {
 			continue
 		}
 
+		// coerce struct types that would produce bad diffs.
+		if leftI, rightI, ok = convertDiffType(leftI, rightI); ok {
+			leftF, rightF = reflect.ValueOf(leftI), reflect.ValueOf(rightI)
+		}
+
 		// If the field is a pointer, dereference it. Nil pointers are coerced
 		// to the zero value of their underlying type.
 		if leftF.Kind() == reflect.Ptr && rightF.Kind() == reflect.Ptr {
@@ -90,6 +98,36 @@ func diffValues[T any](left, right T, table Table) Map {
 	return baseDiff
 }
 
+// convertDiffType converts external struct types to primitive types.
+//nolint:forcetypeassert
+func convertDiffType(left, right any) (newLeft, newRight any, changed bool) {
+	switch typed := left.(type) {
+	case uuid.UUID:
+		return typed.String(), right.(uuid.UUID).String(), true
+
+	case uuid.NullUUID:
+		leftStr, _ := typed.MarshalText()
+		rightStr, _ := right.(uuid.NullUUID).MarshalText()
+		return string(leftStr), string(rightStr), true
+
+	case sql.NullString:
+		leftStr := typed.String
+		if !typed.Valid {
+			leftStr = "null"
+		}
+
+		rightStr := right.(sql.NullString).String
+		if !right.(sql.NullString).Valid {
+			rightStr = "null"
+		}
+
+		return leftStr, rightStr, true
+
+	default:
+		return left, right, false
+	}
+}
+
 // derefPointer deferences a reflect.Value that is a pointer to its underlying
 // value. It dereferences recursively until it finds a non-pointer value. If the
 // pointer is nil, it will be coerced to the zero value of the underlying type.

coderd/audit/diff_test.go

@@ -1,8 +1,12 @@
 package audit_test
 
 import (
+	"database/sql"
+	"reflect"
 	"testing"
+	"time"
 
+	"github.com/google/uuid"
 	"github.com/stretchr/testify/require"
 
 	"github.com/coder/coder/coderd/audit"
@@ -12,30 +16,193 @@ import (
 func TestDiff(t *testing.T) {
 	t.Parallel()
 
-	t.Run("Normal", func(t *testing.T) {
-		t.Parallel()
+	runDiffTests(t, []diffTest[database.GitSSHKey]{
+		{
+			name: "Create",
+			left: audit.Empty[database.GitSSHKey](),
+			right: database.GitSSHKey{
+				UserID:     uuid.UUID{1},
+				CreatedAt:  time.Now(),
+				UpdatedAt:  time.Now(),
+				PrivateKey: "a very secret private key",
+				PublicKey:  "a very public public key",
+			},
+			exp: audit.Map{
+				"user_id":     uuid.UUID{1}.String(),
+				"private_key": "",
+				"public_key":  "a very public public key",
+			},
+		},
+	})
 
-		runDiffTests(t, []diffTest[database.User]{
-			{
-				name: "LeftEmpty",
-				left: audit.Empty[database.User](), right: database.User{Username: "colin", Email: "colin@coder.com"},
-				exp: audit.Map{
-					"email": "colin@coder.com",
-				},
+	runDiffTests(t, []diffTest[database.OrganizationMember]{
+		{
+			name: "Create",
+			left: audit.Empty[database.OrganizationMember](),
+			right: database.OrganizationMember{
+				UserID:         uuid.UUID{1},
+				OrganizationID: uuid.UUID{2},
+				CreatedAt:      time.Now(),
+				UpdatedAt:      time.Now(),
+				Roles:          []string{"auditor"},
 			},
-			{
-				name: "RightEmpty",
-				left: database.User{Username: "colin", Email: "colin@coder.com"}, right: audit.Empty[database.User](),
-				exp: audit.Map{
-					"email": "",
-				},
+			exp: audit.Map{
+				"user_id":         uuid.UUID{1}.String(),
+				"organization_id": uuid.UUID{2}.String(),
+				"roles":           []string{"auditor"},
 			},
-			{
-				name: "NoChange",
-				left: audit.Empty[database.User](), right: audit.Empty[database.User](),
-				exp: audit.Map{},
+		},
+	})
+
+	runDiffTests(t, []diffTest[database.Organization]{
+		{
+			name: "Create",
+			left: audit.Empty[database.Organization](),
+			right: database.Organization{
+				ID:          uuid.UUID{1},
+				Name:        "rust developers",
+				Description: "an organization for rust developers",
+				CreatedAt:   time.Now(),
+				UpdatedAt:   time.Now(),
 			},
-		})
+			exp: audit.Map{
+				"id":          uuid.UUID{1}.String(),
+				"name":        "rust developers",
+				"description": "an organization for rust developers",
+			},
+		},
+	})
+
+	runDiffTests(t, []diffTest[database.Template]{
+		{
+			name: "Create",
+			left: audit.Empty[database.Template](),
+			right: database.Template{
+				ID:              uuid.UUID{1},
+				CreatedAt:       time.Now(),
+				UpdatedAt:       time.Now(),
+				OrganizationID:  uuid.UUID{2},
+				Deleted:         false,
+				Name:            "rust",
+				Provisioner:     database.ProvisionerTypeTerraform,
+				ActiveVersionID: uuid.UUID{3},
+			},
+			exp: audit.Map{
+				"id":                uuid.UUID{1}.String(),
+				"organization_id":   uuid.UUID{2}.String(),
+				"name":              "rust",
+				"provisioner":       database.ProvisionerTypeTerraform,
+				"active_version_id": uuid.UUID{3}.String(),
+			},
+		},
+	})
+
+	runDiffTests(t, []diffTest[database.TemplateVersion]{
+		{
+			name: "Create",
+			left: audit.Empty[database.TemplateVersion](),
+			right: database.TemplateVersion{
+				ID:             uuid.UUID{1},
+				TemplateID:     uuid.NullUUID{UUID: uuid.UUID{2}, Valid: true},
+				CreatedAt:      time.Now(),
+				UpdatedAt:      time.Now(),
+				OrganizationID: uuid.UUID{3},
+				Name:           "rust",
+			},
+			exp: audit.Map{
+				"id":              uuid.UUID{1}.String(),
+				"template_id":     uuid.UUID{2}.String(),
+				"organization_id": uuid.UUID{3}.String(),
+				"name":            "rust",
+			},
+		},
+		{
+			name: "CreateNullTemplateID",
+			left: audit.Empty[database.TemplateVersion](),
+			right: database.TemplateVersion{
+				ID:             uuid.UUID{1},
+				TemplateID:     uuid.NullUUID{},
+				CreatedAt:      time.Now(),
+				UpdatedAt:      time.Now(),
+				OrganizationID: uuid.UUID{3},
+				Name:           "rust",
+			},
+			exp: audit.Map{
+				"id":              uuid.UUID{1}.String(),
+				"organization_id": uuid.UUID{3}.String(),
+				"name":            "rust",
+			},
+		},
+	})
+
+	runDiffTests(t, []diffTest[database.User]{
+		{
+			name: "Create",
+			left: audit.Empty[database.User](),
+			right: database.User{
+				ID:             uuid.UUID{1},
+				Email:          "colin@coder.com",
+				Username:       "colin",
+				HashedPassword: []byte("hunter2ButHashed"),
+				CreatedAt:      time.Now(),
+				UpdatedAt:      time.Now(),
+				Status:         database.UserStatusActive,
+				RBACRoles:      []string{"omega admin"},
+			},
+			exp: audit.Map{
+				"id":              uuid.UUID{1}.String(),
+				"email":           "colin@coder.com",
+				"username":        "colin",
+				"hashed_password": ([]byte)(nil),
+				"status":          database.UserStatusActive,
+				"rbac_roles":      []string{"omega admin"},
+			},
+		},
+	})
+
+	runDiffTests(t, []diffTest[database.Workspace]{
+		{
+			name: "Create",
+			left: audit.Empty[database.Workspace](),
+			right: database.Workspace{
+				ID:                uuid.UUID{1},
+				CreatedAt:         time.Now(),
+				UpdatedAt:         time.Now(),
+				OwnerID:           uuid.UUID{2},
+				TemplateID:        uuid.UUID{3},
+				Name:              "rust workspace",
+				AutostartSchedule: sql.NullString{String: "0 12 * * 1-5", Valid: true},
+				AutostopSchedule:  sql.NullString{String: "0 2 * * 2-6", Valid: true},
+			},
+			exp: audit.Map{
+				"id":                 uuid.UUID{1}.String(),
+				"owner_id":           uuid.UUID{2}.String(),
+				"template_id":        uuid.UUID{3}.String(),
+				"name":               "rust workspace",
+				"autostart_schedule": "0 12 * * 1-5",
+				"autostop_schedule":  "0 2 * * 2-6",
+			},
+		},
+		{
+			name: "NullSchedules",
+			left: audit.Empty[database.Workspace](),
+			right: database.Workspace{
+				ID:                uuid.UUID{1},
+				CreatedAt:         time.Now(),
+				UpdatedAt:         time.Now(),
+				OwnerID:           uuid.UUID{2},
+				TemplateID:        uuid.UUID{3},
+				Name:              "rust workspace",
+				AutostartSchedule: sql.NullString{},
+				AutostopSchedule:  sql.NullString{},
+			},
+			exp: audit.Map{
+				"id":          uuid.UUID{1}.String(),
+				"owner_id":    uuid.UUID{2}.String(),
+				"template_id": uuid.UUID{3}.String(),
+				"name":        "rust workspace",
+			},
+		},
 	})
 }
 
@@ -48,8 +215,11 @@ type diffTest[T audit.Auditable] struct {
 func runDiffTests[T audit.Auditable](t *testing.T, tests []diffTest[T]) {
 	t.Helper()
 
+	var typ T
+	typName := reflect.TypeOf(typ).Name()
+
 	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
+		t.Run(typName+"/"+test.name, func(t *testing.T) {
 			require.Equal(t,
 				test.exp,
 				audit.Diff(test.left, test.right),

coderd/audit/table.go

@@ -10,7 +10,12 @@ import (
 // auditable types. If you want to audit a new type, first define it in
 // AuditableResources, then add it to this interface.
 type Auditable interface {
-	database.User |
+	database.GitSSHKey |
+		database.OrganizationMember |
+		database.Organization |
+		database.Template |
+		database.TemplateVersion |
+		database.User |
 		database.Workspace
 }
 
@@ -34,26 +39,68 @@ type Table map[string]map[string]Action
 // AuditableResources contains a definitive list of all auditable resources and
 // which fields are auditable.
 var AuditableResources = auditMap(map[any]map[string]Action{
+	&database.GitSSHKey{}: {
+		"user_id":     ActionTrack,
+		"created_at":  ActionIgnore, // Never changes, but is implicit and not helpful in a diff.
+		"updated_at":  ActionIgnore, // Changes, but is implicit and not helpful in a diff.
+		"private_key": ActionSecret, // We don't want to expose private keys in diffs.
+		"public_key":  ActionTrack,  // Public keys are ok to expose in a diff.
+	},
+	&database.OrganizationMember{}: {
+		"user_id":         ActionTrack,
+		"organization_id": ActionTrack,
+		"created_at":      ActionIgnore, // Never changes, but is implicit and not helpful in a diff.
+		"updated_at":      ActionIgnore, // Changes, but is implicit and not helpful in a diff.
+		"roles":           ActionTrack,
+	},
+	&database.Organization{}: {
+		"id":          ActionTrack,
+		"name":        ActionTrack,
+		"description": ActionTrack,
+		"created_at":  ActionIgnore, // Never changes, but is implicit and not helpful in a diff.
+		"updated_at":  ActionIgnore, // Changes, but is implicit and not helpful in a diff.
+	},
+	&database.Template{}: {
+		"id":                ActionTrack,
+		"created_at":        ActionIgnore, // Never changes, but is implicit and not helpful in a diff.
+		"updated_at":        ActionIgnore, // Changes, but is implicit and not helpful in a diff.
+		"organization_id":   ActionTrack,
+		"deleted":           ActionIgnore, // Changes, but is implicit when a delete event is fired.
+		"name":              ActionTrack,
+		"provisioner":       ActionTrack,
+		"active_version_id": ActionTrack,
+	},
+	&database.TemplateVersion{}: {
+		"id":              ActionTrack,
+		"template_id":     ActionTrack,
+		"organization_id": ActionTrack,
+		"created_at":      ActionIgnore, // Never changes, but is implicit and not helpful in a diff.
+		"updated_at":      ActionIgnore, // Changes, but is implicit and not helpful in a diff.
+		"name":            ActionTrack,
+		"description":     ActionTrack,
+		"job_id":          ActionIgnore, // Not helpful in a diff because jobs aren't tracked in audit logs.
+	},
 	&database.User{}: {
-		"id":              ActionIgnore, // Never changes.
-		"email":           ActionTrack,  // A user can edit their email.
-		"username":        ActionIgnore, // A user cannot change their username.
-		"hashed_password": ActionSecret, // A user can change their own password.
+		"id":              ActionTrack,
+		"email":           ActionTrack,
+		"username":        ActionTrack,
+		"hashed_password": ActionSecret, // Do not expose a users hashed password.
 		"created_at":      ActionIgnore, // Never changes.
 		"updated_at":      ActionIgnore, // Changes, but is implicit and not helpful in a diff.
-		"status":          ActionTrack,  // A user can update another user status
-		"rbac_roles":      ActionTrack,  // A user's roles are mutable
+		"status":          ActionTrack,
+		"rbac_roles":      ActionTrack,
 	},
 	&database.Workspace{}: {
-		"id":                 ActionIgnore, // Never changes.
+		"id":                 ActionTrack,
 		"created_at":         ActionIgnore, // Never changes.
 		"updated_at":         ActionIgnore, // Changes, but is implicit and not helpful in a diff.
-		"owner_id":           ActionIgnore, // We don't allow workspaces to change ownership.
-		"template_id":        ActionIgnore, // We don't allow workspaces to change templates.
+		"owner_id":           ActionTrack,
+		"organization_id":    ActionTrack,
+		"template_id":        ActionTrack,
 		"deleted":            ActionIgnore, // Changes, but is implicit when a delete event is fired.
-		"name":               ActionIgnore, // We don't allow workspaces to change names.
-		"autostart_schedule": ActionTrack,  // Autostart schedules are directly editable by users.
-		"autostop_schedule":  ActionTrack,  // Autostart schedules are directly editable by users.
+		"name":               ActionTrack,
+		"autostart_schedule": ActionTrack,
+		"autostop_schedule":  ActionTrack,
 	},
 })
 

coderd/database/queries.sql.go

@@ -587,8 +587,8 @@ SET
 	-- Remove all duplicates from the roles.
 	roles = ARRAY(SELECT DISTINCT UNNEST($1 :: text[]))
 WHERE
-		user_id = $2
-		AND organization_id = $3
+	user_id = $2
+	AND organization_id = $3
 RETURNING user_id, organization_id, created_at, updated_at, roles
 `
 

coderd/database/queries/organizationmembers.sql

@@ -47,6 +47,6 @@ SET
 	-- Remove all duplicates from the roles.
 	roles = ARRAY(SELECT DISTINCT UNNEST(@granted_roles :: text[]))
 WHERE
-		user_id = @user_id
-		AND organization_id = @org_id
+	user_id = @user_id
+	AND organization_id = @org_id
 RETURNING *;