Skip to content

Commit 6b68fbb

Browse files
Kira-PilotEmyrk
Kira-Pilot
and
Emyrk
authored
feat: Auditing group members as part of group resource (#5730)
* added AuditableGroup type * added json tags * Anonymizing gGroup struct * adding support on the FE for nested group diffs * added type for GroupMember * Update coderd/database/modelmethods.go Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com> * Update coderd/database/modelmethods.go Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com> * fetching group members in group.delete * passing through right error * broke out into util function and added tests Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>
1 parent 56b9965 commit 6b68fbb

File tree

10 files changed

+230
-27
lines changed

10 files changed

+230
-27
lines changed

‎coderd/audit.go

+2
Original file line numberDiff line numberDiff line change
@@ -464,6 +464,8 @@ func resourceTypeFromString(resourceTypeString string) string {
464464
return resourceTypeString
465465
case codersdk.ResourceTypeAPIKey:
466466
return resourceTypeString
467+
case codersdk.ResourceTypeGroup:
468+
return resourceTypeString
467469
}
468470
return ""
469471
}

‎coderd/audit/diff.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@ type Auditable interface {
1616
database.User |
1717
database.Workspace |
1818
database.GitSSHKey |
19-
database.Group |
20-
database.WorkspaceBuild
19+
database.WorkspaceBuild |
20+
database.AuditableGroup
2121
}
2222

2323
// Map is a map of changed fields in an audited resource. It maps field names to

‎coderd/audit/request.go

+5-5
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,8 @@ func ResourceTarget[T Auditable](tgt T) string {
6464
return ""
6565
case database.GitSSHKey:
6666
return typed.PublicKey
67-
case database.Group:
68-
return typed.Name
67+
case database.AuditableGroup:
68+
return typed.Group.Name
6969
default:
7070
panic(fmt.Sprintf("unknown resource %T", tgt))
7171
}
@@ -87,8 +87,8 @@ func ResourceID[T Auditable](tgt T) uuid.UUID {
8787
return typed.ID
8888
case database.GitSSHKey:
8989
return typed.UserID
90-
case database.Group:
91-
return typed.ID
90+
case database.AuditableGroup:
91+
return typed.Group.ID
9292
default:
9393
panic(fmt.Sprintf("unknown resource %T", tgt))
9494
}
@@ -110,7 +110,7 @@ func ResourceType[T Auditable](tgt T) database.ResourceType {
110110
return database.ResourceTypeWorkspaceBuild
111111
case database.GitSSHKey:
112112
return database.ResourceTypeGitSshKey
113-
case database.Group:
113+
case database.AuditableGroup:
114114
return database.ResourceTypeGroup
115115
default:
116116
panic(fmt.Sprintf("unknown resource %T", tgt))

‎coderd/database/modelmethods.go

+29
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,38 @@
11
package database
22

33
import (
4+
"sort"
5+
46
"github.com/coder/coder/coderd/rbac"
57
)
68

9+
type AuditableGroup struct {
10+
Group
11+
Members []GroupMember `json:"members"`
12+
}
13+
14+
// Auditable returns an object that can be used in audit logs.
15+
// Covers both group and group member changes.
16+
func (g Group) Auditable(users []User) AuditableGroup {
17+
members := make([]GroupMember, 0, len(users))
18+
for _, u := range users {
19+
members = append(members, GroupMember{
20+
UserID: u.ID,
21+
GroupID: g.ID,
22+
})
23+
}
24+
25+
// consistent ordering
26+
sort.Slice(members, func(i, j int) bool {
27+
return members[i].UserID.String() < members[j].UserID.String()
28+
})
29+
30+
return AuditableGroup{
31+
Group: g,
32+
Members: members,
33+
}
34+
}
35+
736
const AllUsersGroup = "Everyone"
837

938
func (s APIKeyScope) ToRBAC() rbac.Scope {

‎enterprise/audit/table.go

+8-7
Original file line numberDiff line numberDiff line change
@@ -105,13 +105,6 @@ var AuditableResources = auditMap(map[any]map[string]Action{
105105
"ttl": ActionTrack,
106106
"last_used_at": ActionIgnore,
107107
},
108-
&database.Group{}: {
109-
"id": ActionTrack,
110-
"name": ActionTrack,
111-
"organization_id": ActionIgnore, // Never changes.
112-
"avatar_url": ActionTrack,
113-
"quota_allowance": ActionTrack,
114-
},
115108
// We don't show any diff for the WorkspaceBuild resource
116109
&database.WorkspaceBuild{}: {
117110
"id": ActionIgnore,
@@ -128,6 +121,14 @@ var AuditableResources = auditMap(map[any]map[string]Action{
128121
"reason": ActionIgnore,
129122
"daily_cost": ActionIgnore,
130123
},
124+
&database.AuditableGroup{}: {
125+
"id": ActionTrack,
126+
"name": ActionTrack,
127+
"organization_id": ActionIgnore, // Never changes.
128+
"avatar_url": ActionTrack,
129+
"quota_allowance": ActionTrack,
130+
"members": ActionTrack,
131+
},
131132
})
132133

133134
// auditMap converts a map of struct pointers to a map of struct names as

‎enterprise/coderd/groups.go

+25-9
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ func (api *API) postGroupByOrganization(rw http.ResponseWriter, r *http.Request)
3232
ctx = r.Context()
3333
org = httpmw.OrganizationParam(r)
3434
auditor = api.AGPL.Auditor.Load()
35-
aReq, commitAudit = audit.InitRequest[database.Group](rw, &audit.RequestParams{
35+
aReq, commitAudit = audit.InitRequest[database.AuditableGroup](rw, &audit.RequestParams{
3636
Audit: *auditor,
3737
Log: api.Logger,
3838
Request: r,
@@ -75,7 +75,9 @@ func (api *API) postGroupByOrganization(rw http.ResponseWriter, r *http.Request)
7575
httpapi.InternalServerError(rw, err)
7676
return
7777
}
78-
aReq.New = group
78+
79+
var emptyUsers []database.User
80+
aReq.New = group.Auditable(emptyUsers)
7981

8082
httpapi.Write(ctx, rw, http.StatusCreated, convertGroup(group, nil))
8183
}
@@ -93,15 +95,22 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) {
9395
ctx = r.Context()
9496
group = httpmw.GroupParam(r)
9597
auditor = api.AGPL.Auditor.Load()
96-
aReq, commitAudit = audit.InitRequest[database.Group](rw, &audit.RequestParams{
98+
aReq, commitAudit = audit.InitRequest[database.AuditableGroup](rw, &audit.RequestParams{
9799
Audit: *auditor,
98100
Log: api.Logger,
99101
Request: r,
100102
Action: database.AuditActionWrite,
101103
})
102104
)
103105
defer commitAudit()
104-
aReq.Old = group
106+
107+
currentMembers, currentMembersErr := api.Database.GetGroupMembers(ctx, group.ID)
108+
if currentMembersErr != nil {
109+
httpapi.InternalServerError(rw, currentMembersErr)
110+
return
111+
}
112+
113+
aReq.Old = group.Auditable(currentMembers)
105114

106115
if !api.Authorize(r, rbac.ActionUpdate, group) {
107116
http.NotFound(rw, r)
@@ -233,15 +242,15 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) {
233242
return
234243
}
235244

236-
members, err := api.Database.GetGroupMembers(ctx, group.ID)
245+
patchedMembers, err := api.Database.GetGroupMembers(ctx, group.ID)
237246
if err != nil {
238247
httpapi.InternalServerError(rw, err)
239248
return
240249
}
241250

242-
aReq.New = group
251+
aReq.New = group.Auditable(patchedMembers)
243252

244-
httpapi.Write(ctx, rw, http.StatusOK, convertGroup(group, members))
253+
httpapi.Write(ctx, rw, http.StatusOK, convertGroup(group, patchedMembers))
245254
}
246255

247256
// @Summary Delete group by name
@@ -257,15 +266,22 @@ func (api *API) deleteGroup(rw http.ResponseWriter, r *http.Request) {
257266
ctx = r.Context()
258267
group = httpmw.GroupParam(r)
259268
auditor = api.AGPL.Auditor.Load()
260-
aReq, commitAudit = audit.InitRequest[database.Group](rw, &audit.RequestParams{
269+
aReq, commitAudit = audit.InitRequest[database.AuditableGroup](rw, &audit.RequestParams{
261270
Audit: *auditor,
262271
Log: api.Logger,
263272
Request: r,
264273
Action: database.AuditActionDelete,
265274
})
266275
)
267276
defer commitAudit()
268-
aReq.Old = group
277+
278+
groupMembers, getMembersErr := api.Database.GetGroupMembers(ctx, group.ID)
279+
if getMembersErr != nil {
280+
httpapi.InternalServerError(rw, getMembersErr)
281+
return
282+
}
283+
284+
aReq.Old = group.Auditable(groupMembers)
269285

270286
if !api.Authorize(r, rbac.ActionDelete, group) {
271287
httpapi.ResourceNotFound(rw)

‎site/src/components/AuditLogRow/AuditLogDiff.tsx

+2-3
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { AuditLog } from "api/typesGenerated"
33
import { colors } from "theme/colors"
44
import { MONOSPACE_FONT_FAMILY } from "theme/constants"
55
import { combineClasses } from "util/combineClasses"
6+
import { FC } from "react"
67

78
const getDiffValue = (value: unknown): string => {
89
if (typeof value === "string") {
@@ -21,9 +22,7 @@ const getDiffValue = (value: unknown): string => {
2122
return value.toString()
2223
}
2324

24-
export const AuditLogDiff: React.FC<{ diff: AuditLog["diff"] }> = ({
25-
diff,
26-
}) => {
25+
export const AuditLogDiff: FC<{ diff: AuditLog["diff"] }> = ({ diff }) => {
2726
const styles = useStyles()
2827
const diffEntries = Object.entries(diff)
2928

‎site/src/components/AuditLogRow/AuditLogRow.tsx

+9-1
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import userAgentParser from "ua-parser-js"
1616
import { AuditLogDiff } from "./AuditLogDiff"
1717
import i18next from "i18next"
1818
import { AuditLogDescription } from "./AuditLogDescription"
19+
import { determineGroupDiff } from "./auditUtils"
1920

2021
const httpStatusColor = (httpStatus: number): PaletteIndex => {
2122
if (httpStatus >= 300 && httpStatus < 500) {
@@ -49,6 +50,13 @@ export const AuditLogRow: React.FC<AuditLogRowProps> = ({
4950
? `${browser.name} ${browser.version}`
5051
: t("auditLog:table.logRow.notAvailable")
5152

53+
let auditDiff = auditLog.diff
54+
55+
// groups have nested diffs (group members)
56+
if (auditLog.resource_type === "group") {
57+
auditDiff = determineGroupDiff(auditLog.diff)
58+
}
59+
5260
const toggle = () => {
5361
if (shouldDisplayDiff) {
5462
setIsDiffOpen((v) => !v)
@@ -153,7 +161,7 @@ export const AuditLogRow: React.FC<AuditLogRowProps> = ({
153161

154162
{shouldDisplayDiff && (
155163
<Collapse in={isDiffOpen}>
156-
<AuditLogDiff diff={auditLog.diff} />
164+
<AuditLogDiff diff={auditDiff} />
157165
</Collapse>
158166
)}
159167
</TableCell>

‎site/src/components/AuditLogRow/auditUtils.test.ts

+122
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
import { determineGroupDiff } from "./auditUtils"
2+
3+
const auditDiffForNewGroup = {
4+
id: {
5+
old: "",
6+
new: "e22e0eb9-625a-468b-b962-269b19473789",
7+
secret: false,
8+
},
9+
members: {
10+
new: [],
11+
secret: false,
12+
},
13+
name: {
14+
old: "",
15+
new: "another-test-group",
16+
secret: false,
17+
},
18+
}
19+
20+
const auditDiffForAddedGroupMember = {
21+
members: {
22+
old: [],
23+
new: [
24+
{
25+
group_id: "e22e0eb9-625a-468b-b962-269b19473789",
26+
user_id: "cea4c2b0-6373-4858-b26a-df3cbfce8845",
27+
},
28+
],
29+
secret: false,
30+
},
31+
}
32+
33+
const auditDiffForRemovedGroupMember = {
34+
members: {
35+
old: [
36+
{
37+
group_id: "25793395-b093-4a3c-a473-9ecf9b243478",
38+
user_id: "84d1cd5a-17e1-4022-898c-52e64256e737",
39+
},
40+
{
41+
group_id: "25793395-b093-4a3c-a473-9ecf9b243478",
42+
user_id: "cea4c2b0-6373-4858-b26a-df3cbfce8845",
43+
},
44+
],
45+
new: [
46+
{
47+
group_id: "25793395-b093-4a3c-a473-9ecf9b243478",
48+
user_id: "84d1cd5a-17e1-4022-898c-52e64256e737",
49+
},
50+
],
51+
secret: false,
52+
},
53+
}
54+
55+
const AuditDiffForDeletedGroup = {
56+
id: {
57+
old: "25793395-b093-4a3c-a473-9ecf9b243478",
58+
new: "",
59+
secret: false,
60+
},
61+
members: {
62+
old: [
63+
{
64+
group_id: "25793395-b093-4a3c-a473-9ecf9b243478",
65+
user_id: "84d1cd5a-17e1-4022-898c-52e64256e737",
66+
},
67+
],
68+
secret: false,
69+
},
70+
name: {
71+
old: "test-group",
72+
new: "",
73+
secret: false,
74+
},
75+
}
76+
77+
describe("determineAuditDiff", () => {
78+
it("auditDiffForNewGroup", () => {
79+
// there should be no change as members are not added when a group is created
80+
expect(determineGroupDiff(auditDiffForNewGroup)).toEqual(
81+
auditDiffForNewGroup,
82+
)
83+
})
84+
85+
it("auditDiffForAddedGroupMember", () => {
86+
const result = {
87+
members: {
88+
...auditDiffForAddedGroupMember.members,
89+
new: ["cea4c2b0-6373-4858-b26a-df3cbfce8845"],
90+
},
91+
}
92+
93+
expect(determineGroupDiff(auditDiffForAddedGroupMember)).toEqual(result)
94+
})
95+
96+
it("auditDiffForRemovedGroupMember", () => {
97+
const result = {
98+
members: {
99+
...auditDiffForRemovedGroupMember.members,
100+
old: [
101+
"84d1cd5a-17e1-4022-898c-52e64256e737",
102+
"cea4c2b0-6373-4858-b26a-df3cbfce8845",
103+
],
104+
new: ["84d1cd5a-17e1-4022-898c-52e64256e737"],
105+
},
106+
}
107+
108+
expect(determineGroupDiff(auditDiffForRemovedGroupMember)).toEqual(result)
109+
})
110+
111+
it("AuditDiffForDeletedGroup", () => {
112+
const result = {
113+
...AuditDiffForDeletedGroup,
114+
members: {
115+
...AuditDiffForDeletedGroup.members,
116+
old: ["84d1cd5a-17e1-4022-898c-52e64256e737"],
117+
},
118+
}
119+
120+
expect(determineGroupDiff(AuditDiffForDeletedGroup)).toEqual(result)
121+
})
122+
})

0 commit comments

Comments
 (0)