clean up #5357
clean up #5357
Conversation
| } | ||
| } else { | ||
| logger.Warn(logCtx, "marshal group member diff", slog.Error(err)) | ||
| } |
There was a problem hiding this comment.
Is there a better way to log here so that I don't have to pass through logCtx and logger?
There was a problem hiding this comment.
Nah, it's on purpose though. The context can potentially hold fields that the logger can use.
| HasGroupMemberChange: hasGroupMemberChange, | ||
| GroupMemberLists: wriBytes, | ||
| }) | ||
|
|
There was a problem hiding this comment.
A pain point here is that if this request fails sometime before line 115, we won't get an audit log signifying the failure. I'm not sure how to get around this, given we want to pass some computational stuff in (GroupMemberLists).
There was a problem hiding this comment.
What if instead of using database.Group here we instead made []database.GroupMember auditable? I think this might solve a lot of the jankiness of needing hardcoded stuff for group members.
There was a problem hiding this comment.
This might take some work adding support for slices in the diff code.
There was a problem hiding this comment.
@coadler I was trying to avoid having another auditable resource; however, I'm down to make the change because I agree - it would simplify a lot. In general, do you have any opinions about how many auditable resources we should have? I don't want to bloat the feature but maybe that's less of a concern than I think it is.
| HasGroupMemberChange: hasGroupMemberChange, | ||
| GroupMemberLists: wriBytes, | ||
| }) | ||
|
|
There was a problem hiding this comment.
What if instead of using database.Group here we instead made []database.GroupMember auditable? I think this might solve a lot of the jankiness of needing hardcoded stuff for group members.
| } | ||
| } else { | ||
| logger.Warn(logCtx, "marshal group member diff", slog.Error(err)) | ||
| } |
There was a problem hiding this comment.
Nah, it's on purpose though. The context can potentially hold fields that the logger can use.
|
|
||
| // Adds a 'members' key to Group resource diffs | ||
| // in order to capture the addition or removal of group members | ||
| func addGroupMemberDiff(logCtx context.Context, diff Map, groupMemberLists json.RawMessage, logger slog.Logger) Map { |
There was a problem hiding this comment.
The context should normally just be called ctx, and the context + logger should always be the first and second fields.
| func addGroupMemberDiff(logCtx context.Context, diff Map, groupMemberLists json.RawMessage, logger slog.Logger) Map { | |
| func addGroupMemberDiff(ctx context.Context, logger slog.Logger, diff Map, groupMemberLists json.RawMessage) Map { |
|
Closing this for now as we discuss two other solutions:
|
resolves #4736
Previously, we were logging audit entries for updates to the user table, but not to the user members table. This meant that if someone updated a group by adding or removing members, these changes weren't logged.
This PR adds member logging. We keep the diff for members under the Group resource as this is most intuitive for the auditor.
NOTE: unsure why the
TestAuthorizeAllEndpointstest is failing - any ideas?