-
Notifications
You must be signed in to change notification settings - Fork 888
chore: update Audit docs to include Audit Actions #5887
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
"WorkspaceBuild": {string(codersdk.AuditActionStart), string(codersdk.AuditActionStop)}, | ||
"AuditableGroup": {string(codersdk.AuditActionCreate), string(codersdk.AuditActionWrite), string(codersdk.AuditActionDelete)}, | ||
} | ||
|
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 if you can think of a better way to get this relationship rather than maintaining this map, I'm all ears.
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.
I think this is the best way currently!
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 blocking this PR, but would be beneficial as a next step:
What would make sense now is refactoring the rest of the codebase to use only this map to figure out the AuditAction
. Otherwise, it will go out of sync relatively quickly. Alternatively, we can implement a unit test that checks if the map is in-sync.
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.
@mtojek @coadler When we make an audit request for a resource, we pass an action. I could grab the action from this new map. This would be a quick update, but would touch every audit request in the code base.
Would this be too heavy-handed an association?
Otherwise, I can explore the unit test that looks at the keys in AuditableResources
and ensures there are identical keys in AuditActionMap
.
Let me know which approach seems better.
scripts/auditdocgen/main.go
Outdated
@@ -117,7 +117,8 @@ func updateAuditDoc(doc []byte, auditableResourcesMap AuditableResourcesMap) ([] | |||
buffer.WriteString("|--|-----------------|\n") | |||
|
|||
for _, resourceName := range sortedResourceNames { | |||
buffer.WriteString("|" + resourceName + "|<table><thead><tr><th>Field</th><th>Tracked</th></tr></thead><tbody>") | |||
auditActionsString := strings.Join(audit.AuditActionMap[resourceName], ", ") | |||
buffer.WriteString("|" + resourceName + "<br><i>" + auditActionsString + "|<table><thead><tr><th>Field</th><th>Tracked</th></tr></thead><tbody>") |
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.
nit: doesn't <i>
require </i>
?
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 question. It's rendering correctly, but I see it's not identified as self-closing so I'll fix!
enterprise/audit/table.go
Outdated
// It is important to maintain this mapping when adding a new Auditable Resource to the | ||
// AuditableResources map (below) as our documentation - generated in scripts/auditdocgen/main.go - | ||
// depends upon it. | ||
var AuditActionMap = map[string][]string{ |
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 you think we can just keep it as map[string][]Action{}
?
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.
@mtojek The reason I didn't do that is because then, in the script, we must range over each Action and convert it to a string, which seemed like extra computation, and since this data structure is only used for that purpose, I thought it might be better to just have it in the format that is easiest for lookup. Let me know if you think this is a suboptimal approach - I'm happy to change it.
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.
There are only a few items on this array, so I'm pretty sure that we won't notice the performance drop :) I think that's safe to adjust it.
"WorkspaceBuild": {string(codersdk.AuditActionStart), string(codersdk.AuditActionStop)}, | ||
"AuditableGroup": {string(codersdk.AuditActionCreate), string(codersdk.AuditActionWrite), string(codersdk.AuditActionDelete)}, | ||
} | ||
|
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 blocking this PR, but would be beneficial as a next step:
What would make sense now is refactoring the rest of the codebase to use only this map to figure out the AuditAction
. Otherwise, it will go out of sync relatively quickly. Alternatively, we can implement a unit test that checks if the map is in-sync.
https://coder.com/docs/v2/671b41fc5a26a848c2755f01e310ace0afc7479a/admin/audit-logs
We now show the actions that are audited for each resource in our audit docs. You can see the actions in italics below each resource cell if you check out the above link.