- 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
Uh oh!
There was an error while loading.Please reload this page.
clean up#5357
Changes fromall commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -24,6 +24,10 @@ type RequestParams struct { | ||||||
| Request *http.Request | ||||||
| Action database.AuditAction | ||||||
| AdditionalFields json.RawMessage | ||||||
| // specific to Group resource patch requests | ||||||
| HasGroupMemberChange bool | ||||||
| GroupMemberLists json.RawMessage | ||||||
| } | ||||||
| type Request[T Auditable] struct { | ||||||
| @@ -144,6 +148,12 @@ func InitRequest[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request | ||||||
| if sw.Status < 400 { | ||||||
| diff := Diff(p.Audit, req.Old, req.New) | ||||||
| // Group resource types may have group member changes. | ||||||
| // We track diffs of this nature differently as GroupMember is a distinct table. | ||||||
| if p.HasGroupMemberChange { | ||||||
| diff = addGroupMemberDiff(logCtx, diff, p.GroupMemberLists, p.Log) | ||||||
| } | ||||||
| var err error | ||||||
| diffRaw, err = json.Marshal(diff) | ||||||
| if err != nil { | ||||||
| @@ -238,3 +248,30 @@ func parseIP(ipStr string) pqtype.Inet { | ||||||
| Valid: ip != nil, | ||||||
| } | ||||||
| } | ||||||
| type GroupMemberLists struct { | ||||||
| OldGroupMembers []string | ||||||
| NewGroupMembers []string | ||||||
| } | ||||||
| // 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 commentThe reason will be displayed to describe this comment to others.Learn more. The context should normally just be called Suggested change
| ||||||
| var ( | ||||||
| groupMemberBytes = []byte(groupMemberLists) | ||||||
| members GroupMemberLists | ||||||
| err = json.Unmarshal(groupMemberBytes, &members) | ||||||
| ) | ||||||
| if err == nil { | ||||||
| diff["members"] = OldNew{ | ||||||
| Old: members.OldGroupMembers, | ||||||
| New: members.NewGroupMembers, | ||||||
| Secret: false, | ||||||
| } | ||||||
| } else { | ||||||
| logger.Warn(logCtx, "marshal group member diff", slog.Error(err)) | ||||||
| } | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 through There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||||||
| return diff | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -2,6 +2,7 @@ package coderd | ||
| import ( | ||
| "database/sql" | ||
| "encoding/json" | ||
| "fmt" | ||
| "net/http" | ||
| @@ -14,6 +15,7 @@ import ( | ||
| "github.com/coder/coder/coderd/httpapi" | ||
| "github.com/coder/coder/coderd/httpmw" | ||
| "github.com/coder/coder/coderd/rbac" | ||
| "github.com/coder/coder/coderd/util/slice" | ||
| "github.com/coder/coder/codersdk" | ||
| ) | ||
| @@ -72,16 +74,44 @@ func (api *API) postGroupByOrganization(rw http.ResponseWriter, r *http.Request) | ||
| func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) { | ||
| var ( | ||
| ctx = r.Context() | ||
| group = httpmw.GroupParam(r) | ||
| auditor = api.AGPL.Auditor.Load() | ||
| ) | ||
| var req codersdk.PatchGroupRequest | ||
| if !httpapi.Read(ctx, rw, r, &req) { | ||
| return | ||
| } | ||
| var ( | ||
| groupResourceInfo = make(map[string][]string) | ||
| hasGroupMemberChange = len(req.AddUsers) != 0 || len(req.RemoveUsers) != 0 | ||
| ) | ||
| if hasGroupMemberChange { | ||
| currentMembers, currentMembersErr := api.Database.GetGroupMembers(ctx, group.ID) | ||
| if currentMembersErr != nil { | ||
| httpapi.InternalServerError(rw, currentMembersErr) | ||
| return | ||
| } | ||
| groupResourceInfo = createMemberMap(req, groupResourceInfo, currentMembers) | ||
| } | ||
| wriBytes, marshalErr := json.Marshal(groupResourceInfo) | ||
| if marshalErr != nil { | ||
| httpapi.InternalServerError(rw, marshalErr) | ||
| } | ||
| aReq, commitAudit := audit.InitRequest[database.Group](rw, &audit.RequestParams{ | ||
| Audit: *auditor, | ||
| Log: api.Logger, | ||
| Request: r, | ||
| Action: database.AuditActionWrite, | ||
| HasGroupMemberChange: hasGroupMemberChange, | ||
| GroupMemberLists: wriBytes, | ||
| }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. What if instead of using There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. | ||
| defer commitAudit() | ||
| aReq.Old = group | ||
| @@ -90,11 +120,6 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) { | ||
| return | ||
| } | ||
| if req.Name != "" && req.Name == database.AllUsersGroup { | ||
| httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ | ||
| Message: fmt.Sprintf("%q is a reserved group name!", database.AllUsersGroup), | ||
| @@ -375,3 +400,28 @@ func convertRole(role rbac.Role) codersdk.Role { | ||
| Name: role.Name, | ||
| } | ||
| } | ||
| // Creates a list of current group member IDs | ||
| // as well as a list of the patched group member IDs | ||
| // in order to form a diff for the group resource audit log entry | ||
| func createMemberMap(req codersdk.PatchGroupRequest, groupMemberMap map[string][]string, currentMembers []database.User) map[string][]string { | ||
| var oldMemberIds []string | ||
| for _, x := range currentMembers { | ||
| oldMemberIds = append(oldMemberIds, x.ID.String()) | ||
| } | ||
| var newMemberIds []string | ||
| // Although we favor adding users over deleting them, | ||
| // users are added and deleted in separate requests on the FE, | ||
| // so we won't see duplicate IDs. | ||
| newMemberIds = append(newMemberIds, req.AddUsers...) | ||
| for _, x := range currentMembers { | ||
| if !slice.Contains(newMemberIds, x.ID.String()) && !slice.Contains(req.RemoveUsers, x.ID.String()) { | ||
| newMemberIds = append(newMemberIds, x.ID.String()) | ||
| } | ||
| } | ||
| groupMemberMap["oldGroupMembers"] = oldMemberIds | ||
| groupMemberMap["newGroupMembers"] = newMemberIds | ||
| return groupMemberMap | ||
| } | ||