Skip to content

Commit 8d53c0e

Browse files
coadlerkylecarbs
coadler
authored and
kylecarbs
committed
feat: add audit diffing for all user editable types (#1413)
1 parent bb0d626 commit 8d53c0e

File tree

5 files changed

+296
-41
lines changed

5 files changed

+296
-41
lines changed

‎coderd/audit/diff.go

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
11
package audit
22

33
import (
4+
"database/sql"
45
"fmt"
56
"reflect"
7+
8+
"github.com/google/uuid"
69
)
710

811
// TODO: this might need to be in the database package.
@@ -64,6 +67,11 @@ func diffValues[T any](left, right T, table Table) Map {
6467
continue
6568
}
6669

70+
// coerce struct types that would produce bad diffs.
71+
if leftI, rightI, ok = convertDiffType(leftI, rightI); ok {
72+
leftF, rightF = reflect.ValueOf(leftI), reflect.ValueOf(rightI)
73+
}
74+
6775
// If the field is a pointer, dereference it. Nil pointers are coerced
6876
// to the zero value of their underlying type.
6977
if leftF.Kind() == reflect.Ptr && rightF.Kind() == reflect.Ptr {
@@ -90,6 +98,36 @@ func diffValues[T any](left, right T, table Table) Map {
9098
return baseDiff
9199
}
92100

101+
// convertDiffType converts external struct types to primitive types.
102+
//nolint:forcetypeassert
103+
func convertDiffType(left, right any) (newLeft, newRight any, changed bool) {
104+
switch typed := left.(type) {
105+
case uuid.UUID:
106+
return typed.String(), right.(uuid.UUID).String(), true
107+
108+
case uuid.NullUUID:
109+
leftStr, _ := typed.MarshalText()
110+
rightStr, _ := right.(uuid.NullUUID).MarshalText()
111+
return string(leftStr), string(rightStr), true
112+
113+
case sql.NullString:
114+
leftStr := typed.String
115+
if !typed.Valid {
116+
leftStr = "null"
117+
}
118+
119+
rightStr := right.(sql.NullString).String
120+
if !right.(sql.NullString).Valid {
121+
rightStr = "null"
122+
}
123+
124+
return leftStr, rightStr, true
125+
126+
default:
127+
return left, right, false
128+
}
129+
}
130+
93131
// derefPointer deferences a reflect.Value that is a pointer to its underlying
94132
// value. It dereferences recursively until it finds a non-pointer value. If the
95133
// pointer is nil, it will be coerced to the zero value of the underlying type.

‎coderd/audit/diff_test.go

Lines changed: 194 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,12 @@
11
package audit_test
22

33
import (
4+
"database/sql"
5+
"reflect"
46
"testing"
7+
"time"
58

9+
"github.com/google/uuid"
610
"github.com/stretchr/testify/require"
711

812
"github.com/coder/coder/coderd/audit"
@@ -12,30 +16,193 @@ import (
1216
func TestDiff(t *testing.T) {
1317
t.Parallel()
1418

15-
t.Run("Normal", func(t *testing.T) {
16-
t.Parallel()
17-
18-
runDiffTests(t, []diffTest[database.User]{
19-
{
20-
name: "LeftEmpty",
21-
left: audit.Empty[database.User](), right: database.User{Username: "colin", Email: "colin@coder.com"},
22-
exp: audit.Map{
23-
"email": "colin@coder.com",
24-
},
25-
},
26-
{
27-
name: "RightEmpty",
28-
left: database.User{Username: "colin", Email: "colin@coder.com"}, right: audit.Empty[database.User](),
29-
exp: audit.Map{
30-
"email": "",
31-
},
32-
},
33-
{
34-
name: "NoChange",
35-
left: audit.Empty[database.User](), right: audit.Empty[database.User](),
36-
exp: audit.Map{},
19+
runDiffTests(t, []diffTest[database.GitSSHKey]{
20+
{
21+
name: "Create",
22+
left: audit.Empty[database.GitSSHKey](),
23+
right: database.GitSSHKey{
24+
UserID: uuid.UUID{1},
25+
CreatedAt: time.Now(),
26+
UpdatedAt: time.Now(),
27+
PrivateKey: "a very secret private key",
28+
PublicKey: "a very public public key",
3729
},
38-
})
30+
exp: audit.Map{
31+
"user_id": uuid.UUID{1}.String(),
32+
"private_key": "",
33+
"public_key": "a very public public key",
34+
},
35+
},
36+
})
37+
38+
runDiffTests(t, []diffTest[database.OrganizationMember]{
39+
{
40+
name: "Create",
41+
left: audit.Empty[database.OrganizationMember](),
42+
right: database.OrganizationMember{
43+
UserID: uuid.UUID{1},
44+
OrganizationID: uuid.UUID{2},
45+
CreatedAt: time.Now(),
46+
UpdatedAt: time.Now(),
47+
Roles: []string{"auditor"},
48+
},
49+
exp: audit.Map{
50+
"user_id": uuid.UUID{1}.String(),
51+
"organization_id": uuid.UUID{2}.String(),
52+
"roles": []string{"auditor"},
53+
},
54+
},
55+
})
56+
57+
runDiffTests(t, []diffTest[database.Organization]{
58+
{
59+
name: "Create",
60+
left: audit.Empty[database.Organization](),
61+
right: database.Organization{
62+
ID: uuid.UUID{1},
63+
Name: "rust developers",
64+
Description: "an organization for rust developers",
65+
CreatedAt: time.Now(),
66+
UpdatedAt: time.Now(),
67+
},
68+
exp: audit.Map{
69+
"id": uuid.UUID{1}.String(),
70+
"name": "rust developers",
71+
"description": "an organization for rust developers",
72+
},
73+
},
74+
})
75+
76+
runDiffTests(t, []diffTest[database.Template]{
77+
{
78+
name: "Create",
79+
left: audit.Empty[database.Template](),
80+
right: database.Template{
81+
ID: uuid.UUID{1},
82+
CreatedAt: time.Now(),
83+
UpdatedAt: time.Now(),
84+
OrganizationID: uuid.UUID{2},
85+
Deleted: false,
86+
Name: "rust",
87+
Provisioner: database.ProvisionerTypeTerraform,
88+
ActiveVersionID: uuid.UUID{3},
89+
},
90+
exp: audit.Map{
91+
"id": uuid.UUID{1}.String(),
92+
"organization_id": uuid.UUID{2}.String(),
93+
"name": "rust",
94+
"provisioner": database.ProvisionerTypeTerraform,
95+
"active_version_id": uuid.UUID{3}.String(),
96+
},
97+
},
98+
})
99+
100+
runDiffTests(t, []diffTest[database.TemplateVersion]{
101+
{
102+
name: "Create",
103+
left: audit.Empty[database.TemplateVersion](),
104+
right: database.TemplateVersion{
105+
ID: uuid.UUID{1},
106+
TemplateID: uuid.NullUUID{UUID: uuid.UUID{2}, Valid: true},
107+
CreatedAt: time.Now(),
108+
UpdatedAt: time.Now(),
109+
OrganizationID: uuid.UUID{3},
110+
Name: "rust",
111+
},
112+
exp: audit.Map{
113+
"id": uuid.UUID{1}.String(),
114+
"template_id": uuid.UUID{2}.String(),
115+
"organization_id": uuid.UUID{3}.String(),
116+
"name": "rust",
117+
},
118+
},
119+
{
120+
name: "CreateNullTemplateID",
121+
left: audit.Empty[database.TemplateVersion](),
122+
right: database.TemplateVersion{
123+
ID: uuid.UUID{1},
124+
TemplateID: uuid.NullUUID{},
125+
CreatedAt: time.Now(),
126+
UpdatedAt: time.Now(),
127+
OrganizationID: uuid.UUID{3},
128+
Name: "rust",
129+
},
130+
exp: audit.Map{
131+
"id": uuid.UUID{1}.String(),
132+
"organization_id": uuid.UUID{3}.String(),
133+
"name": "rust",
134+
},
135+
},
136+
})
137+
138+
runDiffTests(t, []diffTest[database.User]{
139+
{
140+
name: "Create",
141+
left: audit.Empty[database.User](),
142+
right: database.User{
143+
ID: uuid.UUID{1},
144+
Email: "colin@coder.com",
145+
Username: "colin",
146+
HashedPassword: []byte("hunter2ButHashed"),
147+
CreatedAt: time.Now(),
148+
UpdatedAt: time.Now(),
149+
Status: database.UserStatusActive,
150+
RBACRoles: []string{"omega admin"},
151+
},
152+
exp: audit.Map{
153+
"id": uuid.UUID{1}.String(),
154+
"email": "colin@coder.com",
155+
"username": "colin",
156+
"hashed_password": ([]byte)(nil),
157+
"status": database.UserStatusActive,
158+
"rbac_roles": []string{"omega admin"},
159+
},
160+
},
161+
})
162+
163+
runDiffTests(t, []diffTest[database.Workspace]{
164+
{
165+
name: "Create",
166+
left: audit.Empty[database.Workspace](),
167+
right: database.Workspace{
168+
ID: uuid.UUID{1},
169+
CreatedAt: time.Now(),
170+
UpdatedAt: time.Now(),
171+
OwnerID: uuid.UUID{2},
172+
TemplateID: uuid.UUID{3},
173+
Name: "rust workspace",
174+
AutostartSchedule: sql.NullString{String: "0 12 * * 1-5", Valid: true},
175+
AutostopSchedule: sql.NullString{String: "0 2 * * 2-6", Valid: true},
176+
},
177+
exp: audit.Map{
178+
"id": uuid.UUID{1}.String(),
179+
"owner_id": uuid.UUID{2}.String(),
180+
"template_id": uuid.UUID{3}.String(),
181+
"name": "rust workspace",
182+
"autostart_schedule": "0 12 * * 1-5",
183+
"autostop_schedule": "0 2 * * 2-6",
184+
},
185+
},
186+
{
187+
name: "NullSchedules",
188+
left: audit.Empty[database.Workspace](),
189+
right: database.Workspace{
190+
ID: uuid.UUID{1},
191+
CreatedAt: time.Now(),
192+
UpdatedAt: time.Now(),
193+
OwnerID: uuid.UUID{2},
194+
TemplateID: uuid.UUID{3},
195+
Name: "rust workspace",
196+
AutostartSchedule: sql.NullString{},
197+
AutostopSchedule: sql.NullString{},
198+
},
199+
exp: audit.Map{
200+
"id": uuid.UUID{1}.String(),
201+
"owner_id": uuid.UUID{2}.String(),
202+
"template_id": uuid.UUID{3}.String(),
203+
"name": "rust workspace",
204+
},
205+
},
39206
})
40207
}
41208

@@ -48,8 +215,11 @@ type diffTest[T audit.Auditable] struct {
48215
func runDiffTests[T audit.Auditable](t *testing.T, tests []diffTest[T]) {
49216
t.Helper()
50217

218+
var typ T
219+
typName := reflect.TypeOf(typ).Name()
220+
51221
for _, test := range tests {
52-
t.Run(test.name, func(t *testing.T) {
222+
t.Run(typName+"/"+test.name, func(t *testing.T) {
53223
require.Equal(t,
54224
test.exp,
55225
audit.Diff(test.left, test.right),

‎coderd/audit/table.go

Lines changed: 60 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,12 @@ import (
1010
// auditable types. If you want to audit a new type, first define it in
1111
// AuditableResources, then add it to this interface.
1212
type Auditable interface {
13-
database.User |
13+
database.GitSSHKey |
14+
database.OrganizationMember |
15+
database.Organization |
16+
database.Template |
17+
database.TemplateVersion |
18+
database.User |
1419
database.Workspace
1520
}
1621

@@ -34,26 +39,68 @@ type Table map[string]map[string]Action
3439
// AuditableResources contains a definitive list of all auditable resources and
3540
// which fields are auditable.
3641
var AuditableResources = auditMap(map[any]map[string]Action{
42+
&database.GitSSHKey{}: {
43+
"user_id": ActionTrack,
44+
"created_at": ActionIgnore, // Never changes, but is implicit and not helpful in a diff.
45+
"updated_at": ActionIgnore, // Changes, but is implicit and not helpful in a diff.
46+
"private_key": ActionSecret, // We don't want to expose private keys in diffs.
47+
"public_key": ActionTrack, // Public keys are ok to expose in a diff.
48+
},
49+
&database.OrganizationMember{}: {
50+
"user_id": ActionTrack,
51+
"organization_id": ActionTrack,
52+
"created_at": ActionIgnore, // Never changes, but is implicit and not helpful in a diff.
53+
"updated_at": ActionIgnore, // Changes, but is implicit and not helpful in a diff.
54+
"roles": ActionTrack,
55+
},
56+
&database.Organization{}: {
57+
"id": ActionTrack,
58+
"name": ActionTrack,
59+
"description": ActionTrack,
60+
"created_at": ActionIgnore, // Never changes, but is implicit and not helpful in a diff.
61+
"updated_at": ActionIgnore, // Changes, but is implicit and not helpful in a diff.
62+
},
63+
&database.Template{}: {
64+
"id": ActionTrack,
65+
"created_at": ActionIgnore, // Never changes, but is implicit and not helpful in a diff.
66+
"updated_at": ActionIgnore, // Changes, but is implicit and not helpful in a diff.
67+
"organization_id": ActionTrack,
68+
"deleted": ActionIgnore, // Changes, but is implicit when a delete event is fired.
69+
"name": ActionTrack,
70+
"provisioner": ActionTrack,
71+
"active_version_id": ActionTrack,
72+
},
73+
&database.TemplateVersion{}: {
74+
"id": ActionTrack,
75+
"template_id": ActionTrack,
76+
"organization_id": ActionTrack,
77+
"created_at": ActionIgnore, // Never changes, but is implicit and not helpful in a diff.
78+
"updated_at": ActionIgnore, // Changes, but is implicit and not helpful in a diff.
79+
"name": ActionTrack,
80+
"description": ActionTrack,
81+
"job_id": ActionIgnore, // Not helpful in a diff because jobs aren't tracked in audit logs.
82+
},
3783
&database.User{}: {
38-
"id": ActionIgnore, // Never changes.
39-
"email": ActionTrack, // A user can edit their email.
40-
"username": ActionIgnore, // A user cannot change their username.
41-
"hashed_password": ActionSecret, // A user can change their own password.
84+
"id": ActionTrack,
85+
"email": ActionTrack,
86+
"username": ActionTrack,
87+
"hashed_password": ActionSecret, // Do not expose a users hashed password.
4288
"created_at": ActionIgnore, // Never changes.
4389
"updated_at": ActionIgnore, // Changes, but is implicit and not helpful in a diff.
44-
"status": ActionTrack, // A user can update another user status
45-
"rbac_roles": ActionTrack, // A user's roles are mutable
90+
"status": ActionTrack,
91+
"rbac_roles": ActionTrack,
4692
},
4793
&database.Workspace{}: {
48-
"id": ActionIgnore, // Never changes.
94+
"id": ActionTrack,
4995
"created_at": ActionIgnore, // Never changes.
5096
"updated_at": ActionIgnore, // Changes, but is implicit and not helpful in a diff.
51-
"owner_id": ActionIgnore, // We don't allow workspaces to change ownership.
52-
"template_id": ActionIgnore, // We don't allow workspaces to change templates.
97+
"owner_id": ActionTrack,
98+
"organization_id": ActionTrack,
99+
"template_id": ActionTrack,
53100
"deleted": ActionIgnore, // Changes, but is implicit when a delete event is fired.
54-
"name": ActionIgnore, // We don't allow workspaces to change names.
55-
"autostart_schedule": ActionTrack, // Autostart schedules are directly editable by users.
56-
"autostop_schedule": ActionTrack, // Autostart schedules are directly editable by users.
101+
"name": ActionTrack,
102+
"autostart_schedule": ActionTrack,
103+
"autostop_schedule": ActionTrack,
57104
},
58105
})
59106

0 commit comments

Comments
 (0)