From 6b68fbbf18acc94f595e66ab8d163fa22c7b2268 Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Wed, 18 Jan 2023 15:13:39 -0500 Subject: [PATCH] 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 * Update coderd/database/modelmethods.go Co-authored-by: Steven Masley * fetching group members in group.delete * passing through right error * broke out into util function and added tests Co-authored-by: Steven Masley --- coderd/audit.go | 2 + coderd/audit/diff.go | 4 +- coderd/audit/request.go | 10 +- coderd/database/modelmethods.go | 29 +++++ enterprise/audit/table.go | 15 ++- enterprise/coderd/groups.go | 34 +++-- .../components/AuditLogRow/AuditLogDiff.tsx | 5 +- .../components/AuditLogRow/AuditLogRow.tsx | 10 +- .../components/AuditLogRow/auditUtils.test.ts | 122 ++++++++++++++++++ site/src/components/AuditLogRow/auditUtils.ts | 26 ++++ 10 files changed, 230 insertions(+), 27 deletions(-) create mode 100644 site/src/components/AuditLogRow/auditUtils.test.ts create mode 100644 site/src/components/AuditLogRow/auditUtils.ts diff --git a/coderd/audit.go b/coderd/audit.go index d8b764c8ea..185776c2ba 100644 --- a/coderd/audit.go +++ b/coderd/audit.go @@ -464,6 +464,8 @@ func resourceTypeFromString(resourceTypeString string) string { return resourceTypeString case codersdk.ResourceTypeAPIKey: return resourceTypeString + case codersdk.ResourceTypeGroup: + return resourceTypeString } return "" } diff --git a/coderd/audit/diff.go b/coderd/audit/diff.go index 44d5014fb0..9dc1b3ad8b 100644 --- a/coderd/audit/diff.go +++ b/coderd/audit/diff.go @@ -16,8 +16,8 @@ type Auditable interface { database.User | database.Workspace | database.GitSSHKey | - database.Group | - database.WorkspaceBuild + database.WorkspaceBuild | + database.AuditableGroup } // 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 index 2bbd93c371..4a389836b9 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -64,8 +64,8 @@ func ResourceTarget[T Auditable](tgt T) string { return "" case database.GitSSHKey: return typed.PublicKey - case database.Group: - return typed.Name + case database.AuditableGroup: + return typed.Group.Name default: panic(fmt.Sprintf("unknown resource %T", tgt)) } @@ -87,8 +87,8 @@ func ResourceID[T Auditable](tgt T) uuid.UUID { return typed.ID case database.GitSSHKey: return typed.UserID - case database.Group: - return typed.ID + case database.AuditableGroup: + return typed.Group.ID default: panic(fmt.Sprintf("unknown resource %T", tgt)) } @@ -110,7 +110,7 @@ func ResourceType[T Auditable](tgt T) database.ResourceType { return database.ResourceTypeWorkspaceBuild case database.GitSSHKey: return database.ResourceTypeGitSshKey - case database.Group: + case database.AuditableGroup: return database.ResourceTypeGroup default: panic(fmt.Sprintf("unknown resource %T", tgt)) diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index 5257bec1c3..9c17691d00 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -1,9 +1,38 @@ package database import ( + "sort" + "github.com/coder/coder/coderd/rbac" ) +type AuditableGroup struct { + Group + Members []GroupMember `json:"members"` +} + +// Auditable returns an object that can be used in audit logs. +// Covers both group and group member changes. +func (g Group) Auditable(users []User) AuditableGroup { + members := make([]GroupMember, 0, len(users)) + for _, u := range users { + members = append(members, GroupMember{ + UserID: u.ID, + GroupID: g.ID, + }) + } + + // consistent ordering + sort.Slice(members, func(i, j int) bool { + return members[i].UserID.String() < members[j].UserID.String() + }) + + return AuditableGroup{ + Group: g, + Members: members, + } +} + const AllUsersGroup = "Everyone" func (s APIKeyScope) ToRBAC() rbac.Scope { diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go index 342a80f2c7..cf1f08b647 100644 --- a/enterprise/audit/table.go +++ b/enterprise/audit/table.go @@ -105,13 +105,6 @@ var AuditableResources = auditMap(map[any]map[string]Action{ "ttl": ActionTrack, "last_used_at": ActionIgnore, }, - &database.Group{}: { - "id": ActionTrack, - "name": ActionTrack, - "organization_id": ActionIgnore, // Never changes. - "avatar_url": ActionTrack, - "quota_allowance": ActionTrack, - }, // We don't show any diff for the WorkspaceBuild resource &database.WorkspaceBuild{}: { "id": ActionIgnore, @@ -128,6 +121,14 @@ var AuditableResources = auditMap(map[any]map[string]Action{ "reason": ActionIgnore, "daily_cost": ActionIgnore, }, + &database.AuditableGroup{}: { + "id": ActionTrack, + "name": ActionTrack, + "organization_id": ActionIgnore, // Never changes. + "avatar_url": ActionTrack, + "quota_allowance": ActionTrack, + "members": ActionTrack, + }, }) // auditMap converts a map of struct pointers to a map of struct names as diff --git a/enterprise/coderd/groups.go b/enterprise/coderd/groups.go index 42148aa1cb..7ca5b8e225 100644 --- a/enterprise/coderd/groups.go +++ b/enterprise/coderd/groups.go @@ -32,7 +32,7 @@ func (api *API) postGroupByOrganization(rw http.ResponseWriter, r *http.Request) ctx = r.Context() org = httpmw.OrganizationParam(r) auditor = api.AGPL.Auditor.Load() - aReq, commitAudit = audit.InitRequest[database.Group](rw, &audit.RequestParams{ + aReq, commitAudit = audit.InitRequest[database.AuditableGroup](rw, &audit.RequestParams{ Audit: *auditor, Log: api.Logger, Request: r, @@ -75,7 +75,9 @@ func (api *API) postGroupByOrganization(rw http.ResponseWriter, r *http.Request) httpapi.InternalServerError(rw, err) return } - aReq.New = group + + var emptyUsers []database.User + aReq.New = group.Auditable(emptyUsers) httpapi.Write(ctx, rw, http.StatusCreated, convertGroup(group, nil)) } @@ -93,7 +95,7 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) { ctx = r.Context() group = httpmw.GroupParam(r) auditor = api.AGPL.Auditor.Load() - aReq, commitAudit = audit.InitRequest[database.Group](rw, &audit.RequestParams{ + aReq, commitAudit = audit.InitRequest[database.AuditableGroup](rw, &audit.RequestParams{ Audit: *auditor, Log: api.Logger, Request: r, @@ -101,7 +103,14 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) { }) ) defer commitAudit() - aReq.Old = group + + currentMembers, currentMembersErr := api.Database.GetGroupMembers(ctx, group.ID) + if currentMembersErr != nil { + httpapi.InternalServerError(rw, currentMembersErr) + return + } + + aReq.Old = group.Auditable(currentMembers) if !api.Authorize(r, rbac.ActionUpdate, group) { http.NotFound(rw, r) @@ -233,15 +242,15 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) { return } - members, err := api.Database.GetGroupMembers(ctx, group.ID) + patchedMembers, err := api.Database.GetGroupMembers(ctx, group.ID) if err != nil { httpapi.InternalServerError(rw, err) return } - aReq.New = group + aReq.New = group.Auditable(patchedMembers) - httpapi.Write(ctx, rw, http.StatusOK, convertGroup(group, members)) + httpapi.Write(ctx, rw, http.StatusOK, convertGroup(group, patchedMembers)) } // @Summary Delete group by name @@ -257,7 +266,7 @@ func (api *API) deleteGroup(rw http.ResponseWriter, r *http.Request) { ctx = r.Context() group = httpmw.GroupParam(r) auditor = api.AGPL.Auditor.Load() - aReq, commitAudit = audit.InitRequest[database.Group](rw, &audit.RequestParams{ + aReq, commitAudit = audit.InitRequest[database.AuditableGroup](rw, &audit.RequestParams{ Audit: *auditor, Log: api.Logger, Request: r, @@ -265,7 +274,14 @@ func (api *API) deleteGroup(rw http.ResponseWriter, r *http.Request) { }) ) defer commitAudit() - aReq.Old = group + + groupMembers, getMembersErr := api.Database.GetGroupMembers(ctx, group.ID) + if getMembersErr != nil { + httpapi.InternalServerError(rw, getMembersErr) + return + } + + aReq.Old = group.Auditable(groupMembers) if !api.Authorize(r, rbac.ActionDelete, group) { httpapi.ResourceNotFound(rw) diff --git a/site/src/components/AuditLogRow/AuditLogDiff.tsx b/site/src/components/AuditLogRow/AuditLogDiff.tsx index dcd69aa45c..ef3629e5b0 100644 --- a/site/src/components/AuditLogRow/AuditLogDiff.tsx +++ b/site/src/components/AuditLogRow/AuditLogDiff.tsx @@ -3,6 +3,7 @@ import { AuditLog } from "api/typesGenerated" import { colors } from "theme/colors" import { MONOSPACE_FONT_FAMILY } from "theme/constants" import { combineClasses } from "util/combineClasses" +import { FC } from "react" const getDiffValue = (value: unknown): string => { if (typeof value === "string") { @@ -21,9 +22,7 @@ const getDiffValue = (value: unknown): string => { return value.toString() } -export const AuditLogDiff: React.FC<{ diff: AuditLog["diff"] }> = ({ - diff, -}) => { +export const AuditLogDiff: FC<{ diff: AuditLog["diff"] }> = ({ diff }) => { const styles = useStyles() const diffEntries = Object.entries(diff) diff --git a/site/src/components/AuditLogRow/AuditLogRow.tsx b/site/src/components/AuditLogRow/AuditLogRow.tsx index 991f0e5303..10719bbe34 100644 --- a/site/src/components/AuditLogRow/AuditLogRow.tsx +++ b/site/src/components/AuditLogRow/AuditLogRow.tsx @@ -16,6 +16,7 @@ import userAgentParser from "ua-parser-js" import { AuditLogDiff } from "./AuditLogDiff" import i18next from "i18next" import { AuditLogDescription } from "./AuditLogDescription" +import { determineGroupDiff } from "./auditUtils" const httpStatusColor = (httpStatus: number): PaletteIndex => { if (httpStatus >= 300 && httpStatus < 500) { @@ -49,6 +50,13 @@ export const AuditLogRow: React.FC = ({ ? `${browser.name} ${browser.version}` : t("auditLog:table.logRow.notAvailable") + let auditDiff = auditLog.diff + + // groups have nested diffs (group members) + if (auditLog.resource_type === "group") { + auditDiff = determineGroupDiff(auditLog.diff) + } + const toggle = () => { if (shouldDisplayDiff) { setIsDiffOpen((v) => !v) @@ -153,7 +161,7 @@ export const AuditLogRow: React.FC = ({ {shouldDisplayDiff && ( - + )} diff --git a/site/src/components/AuditLogRow/auditUtils.test.ts b/site/src/components/AuditLogRow/auditUtils.test.ts new file mode 100644 index 0000000000..957204f9bc --- /dev/null +++ b/site/src/components/AuditLogRow/auditUtils.test.ts @@ -0,0 +1,122 @@ +import { determineGroupDiff } from "./auditUtils" + +const auditDiffForNewGroup = { + id: { + old: "", + new: "e22e0eb9-625a-468b-b962-269b19473789", + secret: false, + }, + members: { + new: [], + secret: false, + }, + name: { + old: "", + new: "another-test-group", + secret: false, + }, +} + +const auditDiffForAddedGroupMember = { + members: { + old: [], + new: [ + { + group_id: "e22e0eb9-625a-468b-b962-269b19473789", + user_id: "cea4c2b0-6373-4858-b26a-df3cbfce8845", + }, + ], + secret: false, + }, +} + +const auditDiffForRemovedGroupMember = { + members: { + old: [ + { + group_id: "25793395-b093-4a3c-a473-9ecf9b243478", + user_id: "84d1cd5a-17e1-4022-898c-52e64256e737", + }, + { + group_id: "25793395-b093-4a3c-a473-9ecf9b243478", + user_id: "cea4c2b0-6373-4858-b26a-df3cbfce8845", + }, + ], + new: [ + { + group_id: "25793395-b093-4a3c-a473-9ecf9b243478", + user_id: "84d1cd5a-17e1-4022-898c-52e64256e737", + }, + ], + secret: false, + }, +} + +const AuditDiffForDeletedGroup = { + id: { + old: "25793395-b093-4a3c-a473-9ecf9b243478", + new: "", + secret: false, + }, + members: { + old: [ + { + group_id: "25793395-b093-4a3c-a473-9ecf9b243478", + user_id: "84d1cd5a-17e1-4022-898c-52e64256e737", + }, + ], + secret: false, + }, + name: { + old: "test-group", + new: "", + secret: false, + }, +} + +describe("determineAuditDiff", () => { + it("auditDiffForNewGroup", () => { + // there should be no change as members are not added when a group is created + expect(determineGroupDiff(auditDiffForNewGroup)).toEqual( + auditDiffForNewGroup, + ) + }) + + it("auditDiffForAddedGroupMember", () => { + const result = { + members: { + ...auditDiffForAddedGroupMember.members, + new: ["cea4c2b0-6373-4858-b26a-df3cbfce8845"], + }, + } + + expect(determineGroupDiff(auditDiffForAddedGroupMember)).toEqual(result) + }) + + it("auditDiffForRemovedGroupMember", () => { + const result = { + members: { + ...auditDiffForRemovedGroupMember.members, + old: [ + "84d1cd5a-17e1-4022-898c-52e64256e737", + "cea4c2b0-6373-4858-b26a-df3cbfce8845", + ], + new: ["84d1cd5a-17e1-4022-898c-52e64256e737"], + }, + } + + expect(determineGroupDiff(auditDiffForRemovedGroupMember)).toEqual(result) + }) + + it("AuditDiffForDeletedGroup", () => { + const result = { + ...AuditDiffForDeletedGroup, + members: { + ...AuditDiffForDeletedGroup.members, + old: ["84d1cd5a-17e1-4022-898c-52e64256e737"], + }, + } + + expect(determineGroupDiff(AuditDiffForDeletedGroup)).toEqual(result) + }) +}) diff --git a/site/src/components/AuditLogRow/auditUtils.ts b/site/src/components/AuditLogRow/auditUtils.ts new file mode 100644 index 0000000000..99dc7286be --- /dev/null +++ b/site/src/components/AuditLogRow/auditUtils.ts @@ -0,0 +1,26 @@ +import { AuditDiff } from "api/typesGenerated" + +interface GroupMember { + user_id: string + group_id: string +} + +/** + * + * @param auditLogDiff + * @returns a diff with the 'members' key flattened to be an array of user_ids + */ +export const determineGroupDiff = (auditLogDiff: AuditDiff): AuditDiff => { + return { + ...auditLogDiff, + members: { + old: auditLogDiff.members.old?.map( + (groupMember: GroupMember) => groupMember.user_id, + ), + new: auditLogDiff.members.new?.map( + (groupMember: GroupMember) => groupMember.user_id, + ), + secret: auditLogDiff.members.secret, + }, + } +}