Skip to content

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jan 27, 2023
Merged

chore: update Audit docs to include Audit Actions #5887

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2521976
chore: update Audit docs to include Audit Actions
Kira-Pilot Jan 26, 2023
671b41f
regenerated audit docs
Kira-Pilot Jan 26, 2023
987c774
adjusted check_enterprise_imports.sh
Kira-Pilot Jan 27, 2023
4516686
PR feedback
Kira-Pilot Jan 27, 2023
a5a686c
changing script back for now as CI faiiling
Kira-Pilot Jan 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
chore: update Audit docs to include Audit Actions
  • Loading branch information
@Kira-Pilot
Kira-Pilot committed Jan 26, 2023
commit 2521976f629db977a85cdc09f2b12c78c9b4872d
18 changes: 18 additions & 0 deletions enterprise/audit/table.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,26 @@ import (
"reflect"

"github.com/coder/coder/coderd/database"
"github.com/coder/coder/codersdk"
)

// This mapping creates a relationship between an Auditable Resource
// and the Audit Actions we track for that resource.
// 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{
Copy link
Member

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{}?

Copy link
Member Author

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.

Copy link
Member

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.

"GitSSHKey": {string(codersdk.AuditActionCreate)},
"OrganizationMember": {},
"Organization": {},
"Template": {string(codersdk.AuditActionWrite), string(codersdk.AuditActionDelete)},
"TemplateVersion": {string(codersdk.AuditActionCreate), string(codersdk.AuditActionWrite)},
"User": {string(codersdk.AuditActionCreate), string(codersdk.AuditActionWrite), string(codersdk.AuditActionDelete)},
"Workspace": {string(codersdk.AuditActionCreate), string(codersdk.AuditActionWrite), string(codersdk.AuditActionDelete)},
"WorkspaceBuild": {string(codersdk.AuditActionStart), string(codersdk.AuditActionStop)},
"AuditableGroup": {string(codersdk.AuditActionCreate), string(codersdk.AuditActionWrite), string(codersdk.AuditActionDelete)},
}

Copy link
Member Author

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.

Copy link
Contributor

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!

Copy link
Member

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.

Copy link
Member Author

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.

type Action string

const (
Expand Down
3 changes: 2 additions & 1 deletion scripts/auditdocgen/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>")
Copy link
Member

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>?

Copy link
Member Author

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!


// We must sort the field names to ensure sub-table ordering
sortedFieldNames := sortKeys(auditableResourcesMap[resourceName])
Expand Down