- Notifications
You must be signed in to change notification settings - Fork926
clean up#5357
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
clean up#5357
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| } | ||
| } else { | ||
| logger.Warn(logCtx, "marshal group member diff", slog.Error(err)) | ||
| } |
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.
Is there a better way to log here so that I don't have to pass throughlogCtx andlogger?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What if instead of usingdatabase.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This might take some work adding support for slices in the diff code.
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What if instead of usingdatabase.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The context should normally just be calledctx, and the context + logger should always be the first and second fields.
| funcaddGroupMemberDiff(logCtx context.Context,diffMap,groupMemberLists json.RawMessage,logger slog.Logger)Map { | |
| funcaddGroupMemberDiff(ctx context.Context,logger slog.Logger,diffMap,groupMemberLists json.RawMessage)Map { |
Closing this for now as we discuss two other solutions:
|
Uh oh!
There was an error while loading.Please reload this page.
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?