- Notifications
You must be signed in to change notification settings - Fork927
feat: Auditing group members as part of group resource#5730
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
shouldn't theaReq.New beemptyUsers and theaReq.Old have the members?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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) | ||
| ifpatchedMembersErr != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oops! Will fix. What convention do you like for naming errors? Just call themerr until there's a scoping conflict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Unless you need multiple errors in scope at once, I would always just name themerr!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could you pull this out into a util function, maybe with a unit test?
Uh oh!
There was an error while loading.Please reload this page.
resolves#4736
We now audit group members as part of group changes:
