feat: Auditing group members as part of group resource #5730
Conversation
enterprise/coderd/groups.go
Outdated
| Audit: *auditor, | ||
| Log: api.Logger, | ||
| Request: r, | ||
| Action: database.AuditActionDelete, | ||
| }) | ||
| ) | ||
| defer commitAudit() | ||
| aReq.Old = group | ||
| var emptyUsers []database.User | ||
| aReq.Old = group.Auditable(emptyUsers) |
There was a problem hiding this comment.
shouldn't the aReq.New be emptyUsers and the aReq.Old have the members?
There was a problem hiding this comment.
Great catch @Emyrk! Updated.
Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>
Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>
…/coder into audit-group-and-members/kira-pilot
enterprise/coderd/groups.go
Outdated
| patchedMembers, patchedMembersErr := api.Database.GetGroupMembers(ctx, group.ID) | ||
| if patchedMembersErr != nil { |
There was a problem hiding this comment.
Do these errors need to be named differently? It seems like the wrong error is being passed below.
There was a problem hiding this comment.
Oops! Will fix. What convention do you like for naming errors? Just call them err until there's a scoping conflict?
There was a problem hiding this comment.
Unless you need multiple errors in scope at once, I would always just name them err!
There was a problem hiding this comment.
Sounds good! Will update now.
| // groups have nested diffs (group members) | ||
| // so we overwrite the member diff such that | ||
| // only the user_id is shown. | ||
| if (auditLog.resource_type === "group") { |
There was a problem hiding this comment.
Could you pull this out into a util function, maybe with a unit test?
resolves #4736
We now audit group members as part of group changes:
