diff --git a/coderd/audit/diff.go b/coderd/audit/diff.go index ffd72df25d..1a844f2551 100644 --- a/coderd/audit/diff.go +++ b/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. diff --git a/coderd/audit/diff_test.go b/coderd/audit/diff_test.go index 93aec75913..21fa6499a4 100644 --- a/coderd/audit/diff_test.go +++ b/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), diff --git a/coderd/audit/table.go b/coderd/audit/table.go index 6e9db7fec9..267988f2cd 100644 --- a/coderd/audit/table.go +++ b/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, }, }) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index c6d8538a42..7a4bea7c29 100644 --- a/coderd/database/queries.sql.go +++ b/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 ` diff --git a/coderd/database/queries/organizationmembers.sql b/coderd/database/queries/organizationmembers.sql index 8877b77b18..10a45d25eb 100644 --- a/coderd/database/queries/organizationmembers.sql +++ b/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 *;