mirror of
https://github.com/coder/coder.git
synced 2026-10-31 19:11:55 +08:00
Adds an audit record for administrative events on the deployment-wide
chat instruction settings (system prompt, the include-default toggle,
and the plan-mode instructions), per CODAGT-719 and operator decision
D5. Each endpoint records under a stable identity: resource type
`chat_instruction_settings`, a fixed resource ID and a human-readable
target ("System prompt", "Plan mode instructions"), so two changes to
one setting share an ID and history-by-setting works. A real change
exports a Write entry with the old-to-new text visible; a
value-identical PUT still upserts and still returns 204 but records
nothing.
Attempts are recorded, not only transitions. Identity is assigned before
the authorization check, so a denied PUT exports a 403 row with an empty
diff (no request content reaches it), a validation failure exports a 400
row, and a write failure exports a 500 row, each with an empty diff; an
operator can tell "nothing changed" from "something changed and capture
degraded" by the status code.
The write path stays authoritative. The advisory lock and, on plan-mode,
the transaction exist only to serve change-detection; if any of that
machinery fails (lock, begin, commit, rollback), the handler runs main's
idempotent write path directly and derives the response from it, so a
member-visible failure of audit-only infrastructure can never replace
main's successful response. Accepted consequence: when the lock cannot
be taken, two concurrent identical writes can produce two rows instead
of one. That is audit degradation, which is allowed; changing a member's
response is not. Write failures keep the exact response the endpoint
produced before this wiring (transaction error for the system prompt,
which was always transactional; the raw write error for plan mode, which
was not), and the full transaction error is logged so rollback failures
cannot vanish.
<details>
<summary>CODAGT-66 plan entry: S1 (verbatim)</summary>
**S1 `feat: audit chat system instructions changes`** (CODAGT-719; base:
main)
- Struct: `database.ChatSystemPromptSettings{ID uuid.UUID; SystemPrompt
string; IncludeDefaultSystemPrompt bool; PlanModeInstructions string}`
in `coderd/database/types.go` (ticket-sketched shape; one struct, both
endpoints).
- Registration: union entry (diff.go), table.go entry (`id`
ActionIgnore, other three ActionTrack), `AuditActionMap` Write-only;
four request.go cases (`ResourceTarget` "", `ResourceID` from struct,
`ResourceType` new enum value `chat_system_prompt_settings`,
`ResourceRequiresOrgID` false with the "Artificial ID / deployment
singleton" comment convention).
- Migration: `ALTER TYPE resource_type ADD VALUE IF NOT EXISTS
'chat_system_prompt_settings';` comment-only no-op down (000558 shape);
number picked at push per the numbering constraint.
- codersdk: constant + prose `FriendlyString` ("chat system prompt
settings"); `TestAuditDBEnumsCovered` forces both. `coderd/audit.go`
presentation switches: rely on safe defaults (no link, generic
description); no FE changes (filter label falls back to capitalized
value; acceptable per precedent).
- Wiring `putChatSystemPrompt` and `putChatPlanModeInstructions`:
InitRequest with Action Write; artificial `ID: uuid.New()` on `New` only
when a change is detected; no-op suppression by leaving both aReq sides
unset (nil resource IDs skip the log, request.go skip rule); the write
path itself stays byte-identical (upserts still run unconditionally).
- `putChatSystemPrompt` (writes two keys conditionally in one existing
tx): inside that tx, read the pair via `GetChatSystemPromptConfig` for
`Old`, perform the conditional writes exactly as today, then RE-READ the
pair for `New`. The re-read is load-bearing:
`include_default_system_prompt` is computed from the toggle row AND the
prompt, so a prompt-only write can flip the effective value without the
request carrying the pointer. `PlanModeInstructions` stays zero on both
sides.
- `putChatPlanModeInstructions` (no tx exists today): wrap its
read-upsert in `InTx` (behavior-preserving: same single write);
`Old`/`New` populate only `PlanModeInstructions`; the two system-prompt
fields stay zero on both sides; no cross-key reads.
- Change detection compares the populated payload fields only (never the
artificial ID).
- Tests: handler-level coderdtest with `audit.NewMock()` asserting Write
entry on change and NO entry on a value-identical PUT, for both
endpoints (this also exercises `ResourceRequiresOrgID` end to end); the
fallback-flip case (no explicit include-default row, nonempty prompt set
to empty, effective boolean flips: entry emitted with the boolean diff);
diff assertions (old->new prompt text tracked, not secret) in
`enterprise/audit/diff_internal_test.go`; `TestAuditableResources`
passes by construction.
- Bookkeeping at PR open: correct CODAGT-719's no-op premise ("matches
the existing 204-on-unchanged behavior" does not exist on main;
suppression is new, write path unchanged).
- Review focus: Old capture and the New re-read inside the tx (three of
four existing singletons never set Old; do not copy them; and the
computed include-default value makes a naive New construction wrong);
the skip-on-no-op mechanism; prompt text deliberately visible in diffs.
</details>
Note: the plan excerpt above predates operator decision D5 (2026-07-30),
which this PR implements: the resource type is
`chat_instruction_settings` (not `chat_system_prompt_settings`), each
setting carries a stable ID and a display-name target (not a per-write
artificial ID and an empty target), no-op suppression runs through
`InitRequestWithCancel` (not the nil-ID skip), and attempts (denied,
failed, capture-degraded) record rows with real statuses and empty
diffs. Ticket bookkeeping for CODAGT-719 was corrected on Linear at
kickoff: the ticket's "matches the existing 204-on-unchanged behavior"
premise does not exist on main; suppression is new, and the write path
is unchanged.
> 🤖 This PR was created with the help of Coder Agents, and _will be_
reviewed by a human. 🏂🏻
---------
Co-authored-by: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com>
164 lines
5.3 KiB
Go
164 lines
5.3 KiB
Go
package audit
|
|
|
|
import (
|
|
"context"
|
|
"slices"
|
|
"sync"
|
|
"testing"
|
|
|
|
"github.com/google/uuid"
|
|
|
|
"github.com/coder/coder/v2/coderd/database"
|
|
)
|
|
|
|
type Auditor interface {
|
|
Export(ctx context.Context, alog database.AuditLog) error
|
|
diff(old, newVal any) Map
|
|
}
|
|
|
|
type AdditionalFields struct {
|
|
WorkspaceName string `json:"workspace_name"`
|
|
BuildNumber string `json:"build_number"`
|
|
BuildReason database.BuildReason `json:"build_reason"`
|
|
WorkspaceOwner string `json:"workspace_owner"`
|
|
WorkspaceID uuid.UUID `json:"workspace_id"`
|
|
}
|
|
|
|
func NewNop() Auditor {
|
|
return nop{}
|
|
}
|
|
|
|
type nop struct{}
|
|
|
|
func (nop) Export(context.Context, database.AuditLog) error {
|
|
return nil
|
|
}
|
|
|
|
func (nop) diff(any, any) Map {
|
|
return Map{}
|
|
}
|
|
|
|
func NewMock() *MockAuditor {
|
|
return &MockAuditor{}
|
|
}
|
|
|
|
// NewMockWithDiffFn returns a MockAuditor whose entries carry diffs computed
|
|
// by the supplied function instead of the default empty diff. Tests use it to
|
|
// pin the Old/New pair a handler captured, which the empty diff cannot
|
|
// express. The function is test-supplied comparison logic; the mock never
|
|
// calls the production differ.
|
|
func NewMockWithDiffFn(fn func(old, newVal any) Map) *MockAuditor {
|
|
return &MockAuditor{diffFn: fn}
|
|
}
|
|
|
|
type MockAuditor struct {
|
|
mutex sync.Mutex
|
|
auditLogs []database.AuditLog
|
|
diffFn func(old, newVal any) Map
|
|
}
|
|
|
|
// ResetLogs removes all audit logs from the mock auditor.
|
|
// This is helpful for testing to get a clean slate.
|
|
func (a *MockAuditor) ResetLogs() {
|
|
a.mutex.Lock()
|
|
defer a.mutex.Unlock()
|
|
a.auditLogs = make([]database.AuditLog, 0)
|
|
}
|
|
|
|
func (a *MockAuditor) AuditLogs() []database.AuditLog {
|
|
a.mutex.Lock()
|
|
defer a.mutex.Unlock()
|
|
logs := make([]database.AuditLog, len(a.auditLogs))
|
|
copy(logs, a.auditLogs)
|
|
return logs
|
|
}
|
|
|
|
func (a *MockAuditor) Export(_ context.Context, alog database.AuditLog) error {
|
|
a.mutex.Lock()
|
|
defer a.mutex.Unlock()
|
|
a.auditLogs = append(a.auditLogs, alog)
|
|
return nil
|
|
}
|
|
|
|
func (a *MockAuditor) diff(old, newVal any) Map {
|
|
a.mutex.Lock()
|
|
defer a.mutex.Unlock()
|
|
if a.diffFn == nil {
|
|
return Map{}
|
|
}
|
|
return a.diffFn(old, newVal)
|
|
}
|
|
|
|
// Contains returns true if, for each non-zero-valued field in expected,
|
|
// there exists a corresponding audit log in the mock auditor that matches
|
|
// the expected values. Returns false otherwise.
|
|
func (a *MockAuditor) Contains(t testing.TB, expected database.AuditLog) bool {
|
|
a.mutex.Lock()
|
|
defer a.mutex.Unlock()
|
|
for idx, al := range a.auditLogs {
|
|
if expected.ID != uuid.Nil && al.ID != expected.ID {
|
|
t.Logf("audit log %d: expected ID %s, got %s", idx+1, expected.ID, al.ID)
|
|
continue
|
|
}
|
|
if !expected.Time.IsZero() && expected.Time != al.Time {
|
|
t.Logf("audit log %d: expected Time %s, got %s", idx+1, expected.Time, al.Time)
|
|
continue
|
|
}
|
|
if expected.UserID != uuid.Nil && al.UserID != expected.UserID {
|
|
t.Logf("audit log %d: expected UserID %s, got %s", idx+1, expected.UserID, al.UserID)
|
|
continue
|
|
}
|
|
if expected.OrganizationID != uuid.Nil && al.OrganizationID != expected.OrganizationID {
|
|
t.Logf("audit log %d: expected OrganizationID %s, got %s", idx+1, expected.OrganizationID, al.OrganizationID)
|
|
continue
|
|
}
|
|
if expected.Ip.Valid && al.Ip.IPNet.String() != expected.Ip.IPNet.String() {
|
|
t.Logf("audit log %d: expected Ip %s, got %s", idx+1, expected.Ip.IPNet, al.Ip.IPNet)
|
|
continue
|
|
}
|
|
if expected.UserAgent.Valid && al.UserAgent.String != expected.UserAgent.String {
|
|
t.Logf("audit log %d: expected UserAgent %s, got %s", idx+1, expected.UserAgent.String, al.UserAgent.String)
|
|
continue
|
|
}
|
|
if expected.ResourceType != "" && expected.ResourceType != al.ResourceType {
|
|
t.Logf("audit log %d: expected ResourceType %s, got %s", idx+1, expected.ResourceType, al.ResourceType)
|
|
continue
|
|
}
|
|
if expected.ResourceID != uuid.Nil && expected.ResourceID != al.ResourceID {
|
|
t.Logf("audit log %d: expected ResourceID %s, got %s", idx+1, expected.ResourceID, al.ResourceID)
|
|
continue
|
|
}
|
|
if expected.ResourceTarget != "" && expected.ResourceTarget != al.ResourceTarget {
|
|
t.Logf("audit log %d: expected ResourceTarget %s, got %s", idx+1, expected.ResourceTarget, al.ResourceTarget)
|
|
continue
|
|
}
|
|
if expected.Action != "" && expected.Action != al.Action {
|
|
t.Logf("audit log %d: expected Action %s, got %s", idx+1, expected.Action, al.Action)
|
|
continue
|
|
}
|
|
if len(expected.Diff) > 0 && slices.Compare(expected.Diff, al.Diff) != 0 {
|
|
t.Logf("audit log %d: expected Diff %s, got %s", idx+1, string(expected.Diff), string(al.Diff))
|
|
continue
|
|
}
|
|
if expected.StatusCode != 0 && expected.StatusCode != al.StatusCode {
|
|
t.Logf("audit log %d: expected StatusCode %d, got %d", idx+1, expected.StatusCode, al.StatusCode)
|
|
continue
|
|
}
|
|
if len(expected.AdditionalFields) > 0 && slices.Compare(expected.AdditionalFields, al.AdditionalFields) != 0 {
|
|
t.Logf("audit log %d: expected AdditionalFields %s, got %s", idx+1, string(expected.AdditionalFields), string(al.AdditionalFields))
|
|
continue
|
|
}
|
|
if expected.RequestID != uuid.Nil && expected.RequestID != al.RequestID {
|
|
t.Logf("audit log %d: expected RequestID %s, got %s", idx+1, expected.RequestID, al.RequestID)
|
|
continue
|
|
}
|
|
if expected.ResourceIcon != "" && expected.ResourceIcon != al.ResourceIcon {
|
|
t.Logf("audit log %d: expected ResourceIcon %s, got %s", idx+1, expected.ResourceIcon, al.ResourceIcon)
|
|
continue
|
|
}
|
|
return true
|
|
}
|
|
|
|
return false
|
|
}
|