From 37c859ec4c117030dae543cab5cfd3c2c1b4b93e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Mar 2023 13:59:42 -0600 Subject: [PATCH] chore: Ensure all audit types in ResourceTable match APGL (#6563) * chore: Ensure all audit types in ResourceTable match APGL * Implement more checks to ensure all tracked fields are present * Add unit test to ensure all types are represented in audit table * Trade compile time safety for syntax --- docs/admin/audit-logs.md | 22 ++++----- enterprise/audit/table.go | 63 ++++++++++++++++++++++--- enterprise/audit/table_internal_test.go | 55 +++++++++++++++++++++ 3 files changed, 123 insertions(+), 17 deletions(-) create mode 100644 enterprise/audit/table_internal_test.go diff --git a/docs/admin/audit-logs.md b/docs/admin/audit-logs.md index 3e06f307c2..a532ed5200 100644 --- a/docs/admin/audit-logs.md +++ b/docs/admin/audit-logs.md @@ -9,17 +9,17 @@ We track the following resources: -| Resource | | -| ----------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| APIKey
write |
FieldTracked
created_atfalse
expires_atfalse
hashed_secretfalse
idfalse
ip_addressfalse
last_usedfalse
lifetime_secondsfalse
login_typefalse
scopefalse
token_namefalse
updated_atfalse
user_idfalse
| -| Group
create, write, delete |
FieldTracked
avatar_urltrue
idtrue
memberstrue
nametrue
organization_idfalse
quota_allowancetrue
| -| GitSSHKey
create |
FieldTracked
created_atfalse
private_keytrue
public_keytrue
updated_atfalse
user_idtrue
| -| License
create, delete |
FieldTracked
exptrue
idfalse
jwtfalse
uploaded_attrue
uuidtrue
| -| Template
write, delete |
FieldTracked
active_version_idtrue
allow_user_cancel_workspace_jobstrue
created_atfalse
created_bytrue
default_ttltrue
deletedfalse
descriptiontrue
display_nametrue
group_acltrue
icontrue
idtrue
is_privatetrue
max_ttltrue
min_autostart_intervaltrue
nametrue
organization_idfalse
provisionertrue
updated_atfalse
user_acltrue
| -| TemplateVersion
create, write |
FieldTracked
created_atfalse
created_bytrue
git_auth_providersfalse
idtrue
job_idfalse
nametrue
organization_idfalse
readmetrue
template_idtrue
updated_atfalse
| -| User
create, write, delete |
FieldTracked
avatar_urlfalse
created_atfalse
deletedtrue
emailtrue
hashed_passwordtrue
idtrue
last_seen_atfalse
login_typefalse
rbac_rolestrue
statustrue
updated_atfalse
usernametrue
| -| Workspace
create, write, delete |
FieldTracked
autostart_scheduletrue
created_atfalse
deletedfalse
idtrue
last_used_atfalse
nametrue
organization_idfalse
owner_idtrue
template_idtrue
ttltrue
updated_atfalse
| -| WorkspaceBuild
start, stop |
FieldTracked
build_numberfalse
created_atfalse
daily_costfalse
deadlinefalse
idfalse
initiator_idfalse
job_idfalse
max_deadlinefalse
provisioner_statefalse
reasonfalse
template_version_idtrue
transitionfalse
updated_atfalse
workspace_idfalse
| +| Resource | | +| ----------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| APIKey
write |
FieldTracked
created_atfalse
expires_atfalse
hashed_secretfalse
idfalse
ip_addressfalse
last_usedfalse
lifetime_secondsfalse
login_typefalse
scopefalse
token_namefalse
updated_atfalse
user_idfalse
| +| Group
create, write, delete |
FieldTracked
avatar_urltrue
idtrue
memberstrue
nametrue
organization_idfalse
quota_allowancetrue
| +| GitSSHKey
create |
FieldTracked
created_atfalse
private_keytrue
public_keytrue
updated_atfalse
user_idtrue
| +| License
create, delete |
FieldTracked
exptrue
idfalse
jwtfalse
uploaded_attrue
uuidtrue
| +| Template
write, delete |
FieldTracked
active_version_idtrue
allow_user_cancel_workspace_jobstrue
created_atfalse
created_bytrue
default_ttltrue
deletedfalse
descriptiontrue
display_nametrue
group_acltrue
icontrue
idtrue
max_ttltrue
nametrue
organization_idfalse
provisionertrue
updated_atfalse
user_acltrue
| +| TemplateVersion
create, write |
FieldTracked
created_atfalse
created_bytrue
git_auth_providersfalse
idtrue
job_idfalse
nametrue
organization_idfalse
readmetrue
template_idtrue
updated_atfalse
| +| User
create, write, delete |
FieldTracked
avatar_urlfalse
created_atfalse
deletedtrue
emailtrue
hashed_passwordtrue
idtrue
last_seen_atfalse
login_typefalse
rbac_rolestrue
statustrue
updated_atfalse
usernametrue
| +| Workspace
create, write, delete |
FieldTracked
autostart_scheduletrue
created_atfalse
deletedfalse
idtrue
last_used_atfalse
nametrue
organization_idfalse
owner_idtrue
template_idtrue
ttltrue
updated_atfalse
| +| WorkspaceBuild
start, stop |
FieldTracked
build_numberfalse
created_atfalse
daily_costfalse
deadlinefalse
idfalse
initiator_idfalse
job_idfalse
max_deadlinefalse
provisioner_statefalse
reasonfalse
template_version_idtrue
transitionfalse
updated_atfalse
workspace_idfalse
| diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go index 7e4611cc2d..d6e6acbdbf 100644 --- a/enterprise/audit/table.go +++ b/enterprise/audit/table.go @@ -1,6 +1,7 @@ package audit import ( + "fmt" "reflect" "github.com/coder/coder/coderd/database" @@ -42,8 +43,11 @@ const ( type Table map[string]map[string]Action // AuditableResources contains a definitive list of all auditable resources and -// which fields are auditable. -var AuditableResources = auditMap(map[any]map[string]Action{ +// which fields are auditable. All resource types must be valid audit.Auditable +// types. +var AuditableResources = auditMap(auditableResourcesTypes) + +var auditableResourcesTypes = map[any]map[string]Action{ &database.GitSSHKey{}: { "user_id": ActionTrack, "created_at": ActionIgnore, // Never changes, but is implicit and not helpful in a diff. @@ -64,9 +68,7 @@ var AuditableResources = auditMap(map[any]map[string]Action{ "description": ActionTrack, "icon": ActionTrack, "default_ttl": ActionTrack, - "min_autostart_interval": ActionTrack, "created_by": ActionTrack, - "is_private": ActionTrack, "group_acl": ActionTrack, "user_acl": ActionTrack, "allow_user_cancel_workspace_jobs": ActionTrack, @@ -159,7 +161,7 @@ var AuditableResources = auditMap(map[any]map[string]Action{ "exp": ActionTrack, "uuid": ActionTrack, }, -}) +} // auditMap converts a map of struct pointers to a map of struct names as // strings. It's a convenience wrapper so that structs can be passed in by value @@ -168,12 +170,61 @@ func auditMap(m map[any]map[string]Action) Table { out := make(Table, len(m)) for k, v := range m { - out[structName(reflect.TypeOf(k).Elem())] = v + tableKey, tableValue := entry(k, v) + out[tableKey] = tableValue } return out } +// entry is a helper function that checks the json tags to make sure all fields +// are tracked. And no excess fields are tracked. +func entry(v any, f map[string]Action) (string, map[string]Action) { + vt := reflect.TypeOf(v) + for vt.Kind() == reflect.Ptr { + vt = vt.Elem() + } + + // This should never happen because audit.Audible only allows structs in + // its union. + if vt.Kind() != reflect.Struct { + panic(fmt.Sprintf("audit table entry value must be a struct, got %T", v)) + } + + name := structName(vt) + + // Use the flattenStructFields to recurse anonymously embedded structs + vv := reflect.ValueOf(v) + diffs, err := flattenStructFields(vv, vv) + if err != nil { + panic(fmt.Sprintf("audit table entry type %T failed to flatten", v)) + } + + fcpy := make(map[string]Action, len(f)) + for k, v := range f { + fcpy[k] = v + } + for _, d := range diffs { + jsonTag := d.FieldType.Tag.Get("json") + if jsonTag == "-" { + // This field is explicitly ignored. + continue + } + if _, ok := fcpy[jsonTag]; !ok { + panic(fmt.Sprintf("audit table entry missing action for field %q in type %q", d.FieldType.Name, name)) + } + delete(fcpy, jsonTag) + } + + // If there are any fields left in fcpy, they are extra fields that don't + // exist in the struct. Don't track them. + if len(fcpy) > 0 { + panic(fmt.Sprintf("audit table entry has extra actions for type %q: %v", name, fcpy)) + } + + return structName(vt), f +} + func (t Action) String() string { return string(t) } diff --git a/enterprise/audit/table_internal_test.go b/enterprise/audit/table_internal_test.go new file mode 100644 index 0000000000..7ba9c48598 --- /dev/null +++ b/enterprise/audit/table_internal_test.go @@ -0,0 +1,55 @@ +package audit + +import ( + "go/types" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/tools/go/packages" +) + +// TestAuditableResources ensures that all auditable resources are included in +// the Auditable interface and vice versa. +func TestAuditableResources(t *testing.T) { + t.Parallel() + + pkgs, err := packages.Load(&packages.Config{ + Mode: packages.NeedTypes, + }, "../../coderd/audit") + require.NoError(t, err) + + if len(pkgs) != 1 { + t.Fatal("expected one package") + } + auditPkg := pkgs[0] + auditableType := auditPkg.Types.Scope().Lookup("Auditable") + + // If any of these type cast fails, our Auditable interface is not what we + // expect it to be. + named, ok := auditableType.(*types.TypeName) + require.True(t, ok, "expected Auditable to be a type name") + + interfaceType, ok := named.Type().(*types.Named).Underlying().(*types.Interface) + require.True(t, ok, "expected Auditable to be an interface") + + unionType, ok := interfaceType.EmbeddedType(0).(*types.Union) + require.True(t, ok, "expected Auditable to be a union") + + found := make(map[string]bool) + // Now we check we have all the resources in the AuditableResources + for i := 0; i < unionType.Len(); i++ { + // All types come across like 'github.com/coder/coder/coderd/database.' + typeName := unionType.Term(i).Type().String() + _, ok := AuditableResources[typeName] + assert.True(t, ok, "missing resource %q from AuditableResources", typeName) + found[typeName] = true + } + + // Also check that all resources in the table are in the union. We could + // have extra resources here. + for name := range AuditableResources { + _, ok := found[name] + assert.True(t, ok, "extra resource %q found in AuditableResources", name) + } +}