-
Notifications
You must be signed in to change notification settings - Fork 888
feat: add workspace build start/stop to audit log #4744
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
Still trying to figure out how to get the audit log enabled on v2 dev so I have not tested it yet but it looks good to me.
if alog.ResourceType == database.ResourceTypeWorkspaceBuild { | ||
workspaceBytes := []byte(alog.AdditionalFields) | ||
var workspaceResourceInfo WorkspaceResourceInfo | ||
_ = json.Unmarshal(workspaceBytes, &workspaceResourceInfo) |
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.
Would this error be worth surfacing (even just through the logger) or is it expected that this might sometimes not contain workspace info?
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.
Sure!
@@ -278,28 +278,59 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { | |||
return | |||
} | |||
|
|||
// we only want to create audit logs for delete builds right now | |||
auditor := api.Auditor.Load() |
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.
Not really related to your PR since this was already here so feel free to ignore but I wonder if the auditor should be set up before the authorization check so we can log unauthorized request attempts.
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.
Good eye! We have a separate ticket for this!
coderd/workspacebuilds.go
Outdated
"workspaceName": workspace.Name, | ||
} | ||
|
||
wriBytes, _ := json.Marshal(workspaceResourceInfo) |
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.
Seems impossible for this to error but swallowed errors make me anxious 😨 Should we log it just in case?
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.
Yes, definitely!
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.
Tested with much success 🎉 One very minor thing that caught my eye was that the workspace name is bolded on create/delete but not update but we would need to refactor how we do the description to support that I think.
@code-asher thanks! I have a follow-up PR with that bold fix :) |
resolves #4625
This PR replaces #4636 which had a duplicate migration.
