fix: unify key-ops audit shape and surface per-key detail (#25534)

Adding missed commit from https://github.com/coder/coder/pull/25484

This formats the audit logs correctly

![image.png](https://app.graphite.com/user-attachments/assets/598d018b-cdf5-4a2c-8321-24ba2c650a1a.png)



<!--

If you have used AI to produce some or all of this PR, please ensure you have read our [AI Contribution guidelines](https://coder.com/docs/about/contributing/AI_CONTRIBUTING) before submitting.

-->
This commit is contained in:
Danny Kopping
2026-05-20 17:33:26 +02:00
committed by GitHub
parent cd54861e4f
commit 44b1edd4da
8 changed files with 308 additions and 11 deletions
+72 -11
View File
@@ -20,6 +20,7 @@ import (
"github.com/coder/coder/v2/coderd/database/dbauthz"
"github.com/coder/coder/v2/coderd/database/dbtime"
"github.com/coder/coder/v2/coderd/httpapi"
"github.com/coder/coder/v2/coderd/httpmw"
"github.com/coder/coder/v2/coderd/util/ptr"
"github.com/coder/coder/v2/codersdk"
)
@@ -233,6 +234,8 @@ func (api *API) aiProvidersCreate(rw http.ResponseWriter, r *http.Request) {
}
aReq.New = row
auditAIProviderKeyChanges(ctx, r, *auditor, api.Logger, aiProviderKeyChanges{Added: keys})
sdk, err := db2sdk.AIProvider(row, keys)
if err != nil {
api.Logger.Error(ctx, "convert AI provider", slog.F("provider_id", row.ID), slog.Error(err))
@@ -295,8 +298,9 @@ func (api *API) aiProvidersUpdate(rw http.ResponseWriter, r *http.Request) {
idOrName := chi.URLParam(r, "idOrName")
var (
updated database.AIProvider
keys []database.AIProviderKey
updated database.AIProvider
keys []database.AIProviderKey
keyChanges aiProviderKeyChanges
)
err := api.Database.InTx(func(tx database.Store) error {
old, err := lookupAIProvider(ctx, tx, idOrName)
@@ -354,7 +358,7 @@ func (api *API) aiProvidersUpdate(rw http.ResponseWriter, r *http.Request) {
if req.APIKeys != nil {
var ops aiProviderKeyOpsAudit
keys, ops, err = applyAIProviderKeyOps(ctx, tx, updated.ID, *req.APIKeys)
keys, ops, keyChanges, err = applyAIProviderKeyOps(ctx, tx, updated.ID, *req.APIKeys)
if err != nil {
return err
}
@@ -395,6 +399,8 @@ func (api *API) aiProvidersUpdate(rw http.ResponseWriter, r *http.Request) {
return
}
auditAIProviderKeyChanges(ctx, r, *auditor, api.Logger, keyChanges)
sdk, err := db2sdk.AIProvider(updated, keys)
if err != nil {
api.Logger.Error(ctx, "convert AI provider", slog.F("provider_id", updated.ID), slog.Error(err))
@@ -575,17 +581,70 @@ type aiProviderKeyOp struct {
Masked string `json:"masked"`
}
// aiProviderKeyChanges captures the rows added and removed by
// applyAIProviderKeyOps so the caller can emit one audit entry per
// affected key after the transaction commits.
type aiProviderKeyChanges struct {
Added []database.AIProviderKey
Removed []database.AIProviderKey
}
// auditAIProviderKeyChanges emits one audit entry per added or removed
// key, attributed to the actor on the HTTP request. Per-key entries
// keep key rotation visible in the audit log because the parent
// AIProvider audit diff is empty for key-only PATCHes (keys live in a
// separate table).
//
// APIKey is replaced with the masked rendering before the row reaches
// the audit pipeline so plaintext keys never land in the diff or any
// audit backend, independent of the api_key column's audit policy.
func auditAIProviderKeyChanges(ctx context.Context, r *http.Request, auditor audit.Auditor, log slog.Logger, changes aiProviderKeyChanges) {
if len(changes.Added) == 0 && len(changes.Removed) == 0 {
return
}
key, ok := httpmw.APIKeyOptional(r)
if !ok {
return
}
requestID, _ := httpmw.RequestIDOptional(r)
emit := func(action database.AuditAction, before, after database.AIProviderKey) {
before.APIKey = aibridgeutils.MaskSecret(before.APIKey)
after.APIKey = aibridgeutils.MaskSecret(after.APIKey)
audit.BackgroundAudit(ctx, &audit.BackgroundAuditParams[database.AIProviderKey]{
Audit: auditor,
Log: log,
UserID: key.UserID,
RequestID: requestID,
Status: http.StatusOK,
IP: r.RemoteAddr,
UserAgent: r.UserAgent(),
Action: action,
Old: before,
New: after,
})
}
for _, k := range changes.Removed {
emit(database.AuditActionDelete, k, database.AIProviderKey{})
}
for _, k := range changes.Added {
emit(database.AuditActionCreate, database.AIProviderKey{}, k)
}
}
// applyAIProviderKeyOps reconciles a provider's keys against the
// supplied mutation list inside a transaction: kept-by-ID rows stay,
// rows whose ID is absent from the list are deleted, and entries
// carrying a plaintext APIKey are inserted as new rows. Caller is
// responsible for prior validation (XOR per entry, no duplicate IDs).
// IDs that do not belong to this provider return errAIProviderKeyUnknown.
func applyAIProviderKeyOps(ctx context.Context, tx database.Store, providerID uuid.UUID, muts []codersdk.AIProviderKeyMutation) ([]database.AIProviderKey, aiProviderKeyOpsAudit, error) {
var ops aiProviderKeyOpsAudit
func applyAIProviderKeyOps(ctx context.Context, tx database.Store, providerID uuid.UUID, muts []codersdk.AIProviderKeyMutation) ([]database.AIProviderKey, aiProviderKeyOpsAudit, aiProviderKeyChanges, error) {
var (
ops aiProviderKeyOpsAudit
changes aiProviderKeyChanges
)
existing, err := tx.GetAIProviderKeysByProviderID(ctx, providerID)
if err != nil {
return nil, ops, xerrors.Errorf("load existing ai provider keys: %w", err)
return nil, ops, changes, xerrors.Errorf("load existing ai provider keys: %w", err)
}
existingByID := make(map[uuid.UUID]struct{}, len(existing))
for _, k := range existing {
@@ -598,7 +657,7 @@ func applyAIProviderKeyOps(ctx context.Context, tx database.Store, providerID uu
switch {
case m.ID != nil:
if _, ok := existingByID[*m.ID]; !ok {
return nil, ops, xerrors.Errorf("%w: %s", errAIProviderKeyUnknown, *m.ID)
return nil, ops, changes, xerrors.Errorf("%w: %s", errAIProviderKeyUnknown, *m.ID)
}
keep[*m.ID] = struct{}{}
case m.APIKey != nil:
@@ -611,25 +670,27 @@ func applyAIProviderKeyOps(ctx context.Context, tx database.Store, providerID uu
continue
}
if err := tx.DeleteAIProviderKey(ctx, k.ID); err != nil {
return nil, ops, xerrors.Errorf("delete ai provider key %s: %w", k.ID, err)
return nil, ops, changes, xerrors.Errorf("delete ai provider key %s: %w", k.ID, err)
}
ops.Removed = append(ops.Removed, aiProviderKeyOp{ID: k.ID, Masked: aibridgeutils.MaskSecret(k.APIKey)})
changes.Removed = append(changes.Removed, k)
}
added, err := insertAIProviderKeys(ctx, tx, providerID, inserts)
if err != nil {
return nil, ops, err
return nil, ops, changes, err
}
for _, k := range added {
ops.Added = append(ops.Added, aiProviderKeyOp{ID: k.ID, Masked: aibridgeutils.MaskSecret(k.APIKey)})
}
changes.Added = append(changes.Added, added...)
ops.Kept = len(keep)
out, err := tx.GetAIProviderKeysByProviderID(ctx, providerID)
if err != nil {
return nil, ops, xerrors.Errorf("reload ai provider keys: %w", err)
return nil, ops, changes, xerrors.Errorf("reload ai provider keys: %w", err)
}
return out, ops, nil
return out, ops, changes, nil
}
// errAIProviderKeyUnknown is the sentinel returned by
+65
View File
@@ -994,6 +994,52 @@ func TestAIProvidersKeyManagement(t *testing.T) {
require.Contains(t, sdkErr.Validations[0].Detail, "already referenced")
})
t.Run("PATCHPropertiesAudited", func(t *testing.T) {
t.Parallel()
auditor := audit.NewMock()
client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor})
_ = coderdtest.CreateFirstUser(t, client)
ctx := testutil.Context(t, testutil.WaitLong)
//nolint:gocritic // Owner role is the audience for this endpoint.
provider, err := client.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{
Type: codersdk.AIProviderTypeOpenAI,
Name: "keys-props-audit",
Enabled: true,
BaseURL: "https://api.openai.com/v1",
})
require.NoError(t, err)
// Reset before the update so we look only at audits produced by
// the PATCH (the create path emits its own AIProvider audit).
auditor.ResetLogs()
newDisplay := "Renamed"
newURL := "https://api.openai.com/v2"
disabled := false
_, err = client.UpdateAIProvider(ctx, provider.Name, codersdk.UpdateAIProviderRequest{
DisplayName: &newDisplay,
BaseURL: &newURL,
Enabled: &disabled,
})
require.NoError(t, err)
// The parent AIProvider audit entry fires for property-only
// PATCHes; the enterprise auditor populates the diff with the
// changed fields (display_name, base_url, enabled). The mock
// auditor used here returns an empty diff so we only assert the
// entry shape; the actual diff content is exercised by the
// enterprise audit unit tests.
var sawUpdate bool
for _, lg := range auditor.AuditLogs() {
if lg.Action == database.AuditActionWrite && lg.ResourceType == database.ResourceTypeAIProvider {
require.Equal(t, provider.ID, lg.ResourceID)
sawUpdate = true
}
}
require.True(t, sawUpdate, "expected parent AIProvider audit for property-only PATCH")
})
t.Run("PATCHKeysSurfacesOpsInAudit", func(t *testing.T) {
t.Parallel()
auditor := audit.NewMock()
@@ -1059,6 +1105,25 @@ func TestAIProvidersKeyManagement(t *testing.T) {
})
require.NoError(t, err)
require.JSONEq(t, string(expected), string(updated.AdditionalFields))
// Per-key audit entries surface the added/removed keys as their
// own log lines, so a key-only PATCH is visible even without
// frontend changes. The Create handler also emits per-key
// audits for the initial two keys, so match by ResourceID.
var sawCreate, sawDelete bool
for _, lg := range logs {
if lg.ResourceType != database.ResourceTypeAIProviderKey {
continue
}
switch {
case lg.Action == database.AuditActionCreate && lg.ResourceID == added.ID:
sawCreate = true
case lg.Action == database.AuditActionDelete && lg.ResourceID == removed.ID:
sawDelete = true
}
}
require.True(t, sawCreate, "expected create audit for added key")
require.True(t, sawDelete, "expected delete audit for removed key")
})
t.Run("MutationUnknownIDRejected", func(t *testing.T) {
+1
View File
@@ -35,6 +35,7 @@ type Auditable interface {
database.TaskTable |
database.AiSeatState |
database.AIProvider |
database.AIProviderKey |
database.Chat |
database.AuditableGroupAiBudget |
database.UserSecret |
+10
View File
@@ -136,6 +136,8 @@ func ResourceTarget[T Auditable](tgt T) string {
return "AI Seat"
case database.AIProvider:
return typed.Name
case database.AIProviderKey:
return typed.ID.String()
case database.AuditableGroupAiBudget:
return typed.GroupName
case database.Chat:
@@ -218,6 +220,8 @@ func ResourceID[T Auditable](tgt T) uuid.UUID {
return typed.UserID
case database.AIProvider:
return typed.ID
case database.AIProviderKey:
return typed.ID
case database.AuditableGroupAiBudget:
return typed.GroupID
case database.Chat:
@@ -285,6 +289,8 @@ func ResourceType[T Auditable](tgt T) database.ResourceType {
return database.ResourceTypeAiSeat
case database.AIProvider:
return database.ResourceTypeAIProvider
case database.AIProviderKey:
return database.ResourceTypeAIProviderKey
case database.AuditableGroupAiBudget:
return database.ResourceTypeGroupAiBudget
case database.Chat:
@@ -356,6 +362,10 @@ func ResourceRequiresOrgID[T Auditable]() bool {
case database.AIProvider:
// AI providers are deployment-scoped, not org-scoped.
return false
case database.AIProviderKey:
// AI provider keys inherit the deployment scope of their parent
// provider.
return false
case database.AuditableGroupAiBudget:
// Group AI budgets are org-scoped through their parent group.
return true
+1
View File
@@ -16,6 +16,7 @@ We track the following resources:
| <b>Resource<b> | | |
|-----------------------------------------------------------------|----------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| AIProvider<br><i>create, write, delete</i> | <table><thead><tr><th>Field</th><th>Tracked</th></tr></thead><tbody> | <tr><td>base_url</td><td>true</td></tr><tr><td>created_at</td><td>false</td></tr><tr><td>deleted</td><td>true</td></tr><tr><td>display_name</td><td>true</td></tr><tr><td>enabled</td><td>true</td></tr><tr><td>id</td><td>true</td></tr><tr><td>name</td><td>true</td></tr><tr><td>settings</td><td>true</td></tr><tr><td>settings_key_id</td><td>false</td></tr><tr><td>type</td><td>true</td></tr><tr><td>updated_at</td><td>false</td></tr></tbody></table> |
| AIProviderKey<br><i>create, delete</i> | <table><thead><tr><th>Field</th><th>Tracked</th></tr></thead><tbody> | <tr><td>api_key</td><td>true</td></tr><tr><td>api_key_key_id</td><td>false</td></tr><tr><td>created_at</td><td>false</td></tr><tr><td>id</td><td>true</td></tr><tr><td>provider_id</td><td>true</td></tr><tr><td>updated_at</td><td>false</td></tr></tbody></table> |
| APIKey<br><i>login, logout, register, create, write, delete</i> | <table><thead><tr><th>Field</th><th>Tracked</th></tr></thead><tbody> | <tr><td>allow_list</td><td>false</td></tr><tr><td>created_at</td><td>true</td></tr><tr><td>expires_at</td><td>true</td></tr><tr><td>hashed_secret</td><td>false</td></tr><tr><td>id</td><td>false</td></tr><tr><td>ip_address</td><td>false</td></tr><tr><td>last_used</td><td>true</td></tr><tr><td>lifetime_seconds</td><td>false</td></tr><tr><td>login_type</td><td>false</td></tr><tr><td>scopes</td><td>false</td></tr><tr><td>token_name</td><td>false</td></tr><tr><td>updated_at</td><td>false</td></tr><tr><td>user_id</td><td>true</td></tr></tbody></table> |
| AiSeatState<br><i>create</i> | <table><thead><tr><th>Field</th><th>Tracked</th></tr></thead><tbody> | <tr><td>first_used_at</td><td>true</td></tr><tr><td>last_event_description</td><td>true</td></tr><tr><td>last_event_type</td><td>true</td></tr><tr><td>last_used_at</td><td>false</td></tr><tr><td>updated_at</td><td>false</td></tr><tr><td>user_id</td><td>true</td></tr></tbody></table> |
| AuditOAuthConvertState<br><i></i> | <table><thead><tr><th>Field</th><th>Tracked</th></tr></thead><tbody> | <tr><td>created_at</td><td>true</td></tr><tr><td>expires_at</td><td>true</td></tr><tr><td>from_login_type</td><td>true</td></tr><tr><td>to_login_type</td><td>true</td></tr><tr><td>user_id</td><td>true</td></tr></tbody></table> |
+47
View File
@@ -457,6 +457,53 @@ func Test_diff(t *testing.T) {
},
},
})
runDiffTests(t, []diffTest{
{
name: "PropertyChange",
left: database.AIProvider{
ID: uuid.UUID{1},
Type: database.AiProviderTypeOpenai,
Name: "primary-openai",
DisplayName: sql.NullString{String: "Primary", Valid: true},
Enabled: true,
BaseUrl: "https://api.openai.com/v1",
},
right: database.AIProvider{
ID: uuid.UUID{1},
Type: database.AiProviderTypeOpenai,
Name: "primary-openai",
DisplayName: sql.NullString{String: "Renamed", Valid: true},
Enabled: false,
BaseUrl: "https://api.openai.com/v2",
},
exp: audit.Map{
"display_name": audit.OldNew{Old: "Primary", New: "Renamed"},
"enabled": audit.OldNew{Old: true, New: false},
"base_url": audit.OldNew{Old: "https://api.openai.com/v1", New: "https://api.openai.com/v2"},
},
},
})
runDiffTests(t, []diffTest{
{
// api_key is tracked, but callers must pre-mask before the
// row reaches the audit pipeline. The pre-masked rendering
// (sk-prefix...suffix) is what flows into the diff.
name: "PreMaskedKeyFlowsThrough",
left: audit.Empty[database.AIProviderKey](),
right: database.AIProviderKey{
ID: uuid.UUID{1},
ProviderID: uuid.UUID{2},
APIKey: "sk-a...wxyz",
},
exp: audit.Map{
"id": audit.OldNew{Old: "", New: uuid.UUID{1}.String()},
"provider_id": audit.OldNew{Old: "", New: uuid.UUID{2}.String()},
"api_key": audit.OldNew{Old: "", New: "sk-a...wxyz"},
},
},
})
}
func runDiffTests(t *testing.T, tests []diffTest) {
+8
View File
@@ -392,6 +392,14 @@ var auditableResourcesTypes = map[any]map[string]Action{
"created_at": ActionIgnore, // Implicit; not useful in a diff.
"updated_at": ActionIgnore, // Changes; not useful in a diff.
},
&database.AIProviderKey{}: {
"id": ActionTrack,
"provider_id": ActionTrack,
"api_key": ActionTrack, // Callers must pre-mask before auditing; the audit pipeline never sees plaintext.
"api_key_key_id": ActionIgnore, // dbcrypt key reference, derivable.
"created_at": ActionIgnore, // Implicit; not useful in a diff.
"updated_at": ActionIgnore, // Changes; not useful in a diff.
},
&database.TaskTable{}: {
"id": ActionTrack,
"organization_id": ActionIgnore, // Never changes.
@@ -0,0 +1,104 @@
package coderd_test
import (
"context"
"encoding/json"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/coder/coder/v2/coderd/audit"
"github.com/coder/coder/v2/coderd/coderdtest"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/dbauthz"
"github.com/coder/coder/v2/coderd/database/dbtestutil"
"github.com/coder/coder/v2/codersdk"
entaudit "github.com/coder/coder/v2/enterprise/audit"
"github.com/coder/coder/v2/enterprise/audit/backends"
"github.com/coder/coder/v2/enterprise/coderd/coderdenttest"
"github.com/coder/coder/v2/enterprise/coderd/license"
"github.com/coder/coder/v2/testutil"
)
// TestAIProviderAuditDiff exercises the full HTTP -> enterprise auditor
// -> Postgres write path for AI provider updates. The mock auditor used
// elsewhere returns an empty diff, so this is the only place that
// proves changed properties land in the audit_logs row.
func TestAIProviderAuditDiff(t *testing.T) {
t.Parallel()
db, ps := dbtestutil.NewDB(t)
auditor := entaudit.NewAuditor(
db,
entaudit.DefaultFilter,
backends.NewPostgres(db, true),
)
ownerClient, _ := coderdenttest.New(t, &coderdenttest.Options{
AuditLogging: true,
Options: &coderdtest.Options{
Database: db,
Pubsub: ps,
Auditor: auditor,
},
LicenseOptions: &coderdenttest.LicenseOptions{
Features: license.Features{
codersdk.FeatureAuditLog: 1,
},
},
})
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium)
defer cancel()
//nolint:gocritic // Owner role is the audience for this endpoint.
provider, err := ownerClient.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{
Type: codersdk.AIProviderTypeOpenAI,
Name: "audit-target",
DisplayName: "Audit Target",
Enabled: true,
BaseURL: "https://api.openai.com/v1",
})
require.NoError(t, err)
newDisplay := "Renamed"
newURL := "https://api.openai.com/v2"
disabled := false
_, err = ownerClient.UpdateAIProvider(ctx, provider.Name, codersdk.UpdateAIProviderRequest{
DisplayName: &newDisplay,
BaseURL: &newURL,
Enabled: &disabled,
})
require.NoError(t, err)
rows, err := db.GetAuditLogsOffset(
dbauthz.AsSystemRestricted(ctx),
database.GetAuditLogsOffsetParams{
ResourceType: string(database.ResourceTypeAIProvider),
LimitOpt: 10,
},
)
require.NoError(t, err)
require.Len(t, rows, 2, "expected one create and one update audit row")
// GetAuditLogsOffset returns entries sorted by time in descending order.
updateLog := rows[0].AuditLog
require.Equal(t, database.AuditActionWrite, updateLog.Action)
var updateDiff audit.Map
require.NoError(t, json.Unmarshal(updateLog.Diff, &updateDiff))
if assert.Contains(t, updateDiff, "display_name", "display_name missing from diff") {
assert.Equal(t, "Audit Target", updateDiff["display_name"].Old)
assert.Equal(t, newDisplay, updateDiff["display_name"].New)
}
if assert.Contains(t, updateDiff, "base_url", "base_url missing from diff") {
assert.Equal(t, "https://api.openai.com/v1", updateDiff["base_url"].Old)
assert.Equal(t, newURL, updateDiff["base_url"].New)
}
if assert.Contains(t, updateDiff, "enabled", "enabled missing from diff") {
assert.Equal(t, true, updateDiff["enabled"].Old)
assert.Equal(t, false, updateDiff["enabled"].New)
}
}