From 6969fc7562845bf515e9bf2cf05cd3724718c21d Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 28 May 2026 18:37:57 +0100 Subject: [PATCH] fix: improve chat audit log descriptions and diff rendering (#25728) Chat ACL audit diffs rendered as `[object Object]` because the diff viewer called `.toString()` on object values. Common chat operations (archive, share) showed generic "updated chat" descriptions instead of semantic ones. Add `chatAuditLogDescription` to derive semantic descriptions from the audit diff for successful chat writes: "archived/unarchived chat" for archive toggles, "updated sharing for chat" for ACL-only changes. Extract diff value formatting into `formatAuditDiffValue`, which renders object values as deterministic compact JSON with sorted keys, fixing the `[object Object]` rendering for chat ACLs and any other object-valued fields. The previous `determineIdPSyncMappingDiff` workaround for IdP sync mappings was removed because the generic formatting handles it. Closes CODAGT-513 > Generated by Coder Agents on behalf of @johnstcn (cherry picked from commit 7ea0eff94ee81152023c3924b9bc1442ee73d189) --- coderd/audit.go | 56 ++++++++++ coderd/audit_internal_test.go | 103 ++++++++++++++++++ .../AuditLogRow/AuditLogDiff/AuditLogDiff.tsx | 47 ++------ .../AuditLogDiff/auditUtils.test.ts | 69 +++++++++++- .../AuditLogRow/AuditLogDiff/auditUtils.ts | 81 ++++++++++---- .../AuditLogRow/AuditLogRow.stories.tsx | 85 +++++++++++++++ .../AuditPage/AuditLogRow/AuditLogRow.tsx | 13 +-- 7 files changed, 381 insertions(+), 73 deletions(-) diff --git a/coderd/audit.go b/coderd/audit.go index 661019d063..a58168567f 100644 --- a/coderd/audit.go +++ b/coderd/audit.go @@ -303,6 +303,12 @@ func auditLogDescription(alog database.GetAuditLogsOffsetRow) string { _, _ = b.WriteString("{user} ") } + // Chat write operations get semantic descriptions derived from the diff. + if desc, ok := chatAuditLogDescription(alog); ok { + _, _ = b.WriteString(desc) + return b.String() + } + switch { case alog.AuditLog.StatusCode == int32(http.StatusSeeOther): _, _ = b.WriteString("was redirected attempting to ") @@ -345,6 +351,56 @@ func auditLogDescription(alog database.GetAuditLogsOffsetRow) string { return b.String() } +// chatAuditLogDescription returns a description for successful chat write +// operations based on the diff contents. It returns false for non-chat +// resources, non-write actions, or error/redirect status codes, letting +// the caller fall through to the generic description. +func chatAuditLogDescription(alog database.GetAuditLogsOffsetRow) (string, bool) { + if alog.AuditLog.ResourceType != database.ResourceTypeChat || + alog.AuditLog.Action != database.AuditActionWrite || + alog.AuditLog.StatusCode >= 400 || + alog.AuditLog.StatusCode == int32(http.StatusSeeOther) { + return "", false + } + + var diff codersdk.AuditDiff + if err := json.Unmarshal(alog.AuditLog.Diff, &diff); err != nil { + return "", false + } + + // Single "archived" field: archive or unarchive. + if len(diff) == 1 { + if field, ok := diff["archived"]; ok { + oldVal, oldOK := field.Old.(bool) + newVal, newOK := field.New.(bool) + if oldOK && newOK { + if !oldVal && newVal { + return "archived chat {target}", true + } + if oldVal && !newVal { + return "unarchived chat {target}", true + } + } + } + } + + // All fields are ACL changes: sharing update. + if len(diff) > 0 { + aclOnly := true + for field := range diff { + if field != "user_acl" && field != "group_acl" { + aclOnly = false + break + } + } + if aclOnly { + return "updated sharing for chat {target}", true + } + } + + return "", false +} + func (api *API) auditLogIsResourceDeleted(ctx context.Context, alog database.GetAuditLogsOffsetRow) bool { switch alog.AuditLog.ResourceType { case database.ResourceTypeTemplate: diff --git a/coderd/audit_internal_test.go b/coderd/audit_internal_test.go index cc7fddf3e0..640690cff9 100644 --- a/coderd/audit_internal_test.go +++ b/coderd/audit_internal_test.go @@ -3,6 +3,7 @@ package coderd import ( "context" "database/sql" + "encoding/json" "testing" "github.com/google/uuid" @@ -14,6 +15,7 @@ import ( "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbmock" + "github.com/coder/coder/v2/codersdk" ) func TestAuditLogIsResourceDeleted(t *testing.T) { @@ -111,6 +113,91 @@ func TestAuditLogDescription(t *testing.T) { }, want: "{user} deleted the git ssh key", }, + { + name: "chat_archived", + alog: chatAuditLogRow(t, codersdk.AuditDiff{ + "archived": {Old: false, New: true}, + }), + want: "{user} archived chat {target}", + }, + { + name: "chat_unarchived", + alog: chatAuditLogRow(t, codersdk.AuditDiff{ + "archived": {Old: true, New: false}, + }), + want: "{user} unarchived chat {target}", + }, + { + name: "chat_sharing_user_acl", + alog: chatAuditLogRow(t, codersdk.AuditDiff{ + "user_acl": {Old: map[string]any{}, New: map[string]any{"user-1": map[string]any{"permissions": []string{"read"}}}}, + }), + want: "{user} updated sharing for chat {target}", + }, + { + name: "chat_sharing_group_acl", + alog: chatAuditLogRow(t, codersdk.AuditDiff{ + "group_acl": {Old: map[string]any{}, New: map[string]any{"group-1": map[string]any{"permissions": []string{"read"}}}}, + }), + want: "{user} updated sharing for chat {target}", + }, + { + name: "chat_sharing_both_acls", + alog: chatAuditLogRow(t, codersdk.AuditDiff{ + "user_acl": {Old: map[string]any{}, New: map[string]any{"user-1": map[string]any{"permissions": []string{"read"}}}}, + "group_acl": {Old: map[string]any{}, New: map[string]any{"group-1": map[string]any{"permissions": []string{"read"}}}}, + }), + want: "{user} updated sharing for chat {target}", + }, + { + name: "chat_mixed_diff_falls_through", + alog: chatAuditLogRow(t, codersdk.AuditDiff{ + "archived": {Old: false, New: true}, + "pin_order": {Old: 1, New: 0}, + }), + want: "{user} updated chat {target}", + }, + { + name: "chat_acl_with_extra_field_falls_through", + alog: chatAuditLogRow(t, codersdk.AuditDiff{ + "user_acl": {Old: map[string]any{}, New: map[string]any{}}, + "pin_order": {Old: 1, New: 0}, + }), + want: "{user} updated chat {target}", + }, + { + name: "chat_failed_write_no_override", + alog: func() database.GetAuditLogsOffsetRow { + row := chatAuditLogRow(t, codersdk.AuditDiff{ + "archived": {Old: false, New: true}, + }) + row.AuditLog.StatusCode = 400 + return row + }(), + want: "{user} unsuccessfully attempted to write chat {target}", + }, + { + name: "chat_redirect_no_override", + alog: func() database.GetAuditLogsOffsetRow { + row := chatAuditLogRow(t, codersdk.AuditDiff{ + "archived": {Old: false, New: true}, + }) + row.AuditLog.StatusCode = 303 + return row + }(), + want: "{user} was redirected attempting to write chat {target}", + }, + { + name: "chat_non_write_action_no_override", + alog: func() database.GetAuditLogsOffsetRow { + row := chatAuditLogRow(t, codersdk.AuditDiff{ + "user_acl": {Old: map[string]any{}, New: map[string]any{"user-1": map[string]any{"permissions": []string{"read"}}}}, + }) + row.AuditLog.Action = database.AuditActionCreate + return row + }(), + want: "{user} created chat {target}", + }, } // nolint: paralleltest // no longer need to reinitialize loop vars in go 1.22 for _, tc := range testCases { @@ -121,3 +208,19 @@ func TestAuditLogDescription(t *testing.T) { }) } } + +// chatAuditLogRow builds a GetAuditLogsOffsetRow for a successful chat write +// with the given diff, suitable for testing auditLogDescription. +func chatAuditLogRow(t *testing.T, diff codersdk.AuditDiff) database.GetAuditLogsOffsetRow { + t.Helper() + rawDiff, err := json.Marshal(diff) + require.NoError(t, err) + return database.GetAuditLogsOffsetRow{ + AuditLog: database.AuditLog{ + Action: database.AuditActionWrite, + StatusCode: 200, + ResourceType: database.ResourceTypeChat, + Diff: rawDiff, + }, + } +} diff --git a/site/src/pages/AuditPage/AuditLogRow/AuditLogDiff/AuditLogDiff.tsx b/site/src/pages/AuditPage/AuditLogRow/AuditLogDiff/AuditLogDiff.tsx index 7565a144e2..d3e035c33d 100644 --- a/site/src/pages/AuditPage/AuditLogRow/AuditLogDiff/AuditLogDiff.tsx +++ b/site/src/pages/AuditPage/AuditLogRow/AuditLogDiff/AuditLogDiff.tsx @@ -1,43 +1,6 @@ import type { FC } from "react"; import type { AuditDiff } from "#/api/typesGenerated"; - -const getDiffValue = (value: unknown): string => { - if (typeof value === "string") { - return `"${value}"`; - } - - if (isTimeObject(value)) { - if (!value.Valid) { - return "null"; - } - - return new Date(value.Time).toLocaleString(); - } - - if (Array.isArray(value)) { - const values = value.map((v) => getDiffValue(v)); - return `[${values.join(", ")}]`; - } - - if (value === null || value === undefined) { - return "null"; - } - - return String(value); -}; - -const isTimeObject = ( - value: unknown, -): value is { Time: string; Valid: boolean } => { - return ( - value !== null && - typeof value === "object" && - "Time" in value && - typeof value.Time === "string" && - "Valid" in value && - typeof value.Valid === "boolean" - ); -}; +import { formatAuditDiffValue } from "./auditUtils"; interface AuditLogDiffProps { diff: AuditDiff; @@ -58,7 +21,9 @@ export const AuditLogDiff: FC = ({ diff }) => {
{attrName}:{" "} - {valueDiff.secret ? "••••••••" : getDiffValue(valueDiff.old)} + {valueDiff.secret + ? "••••••••" + : formatAuditDiffValue(valueDiff.old)}
@@ -74,7 +39,9 @@ export const AuditLogDiff: FC = ({ diff }) => {
{attrName}:{" "} - {valueDiff.secret ? "••••••••" : getDiffValue(valueDiff.new)} + {valueDiff.secret + ? "••••••••" + : formatAuditDiffValue(valueDiff.new)}
diff --git a/site/src/pages/AuditPage/AuditLogRow/AuditLogDiff/auditUtils.test.ts b/site/src/pages/AuditPage/AuditLogRow/AuditLogDiff/auditUtils.test.ts index 12c6cabb41..74cca5a2c6 100644 --- a/site/src/pages/AuditPage/AuditLogRow/AuditLogDiff/auditUtils.test.ts +++ b/site/src/pages/AuditPage/AuditLogRow/AuditLogDiff/auditUtils.test.ts @@ -1,4 +1,4 @@ -import { determineGroupDiff } from "./auditUtils"; +import { determineGroupDiff, formatAuditDiffValue } from "./auditUtils"; const auditDiffForNewGroup = { id: { @@ -120,3 +120,70 @@ describe("determineAuditDiff", () => { expect(determineGroupDiff(AuditDiffForDeletedGroup)).toEqual(result); }); }); + +describe("formatAuditDiffValue", () => { + it.each([ + { name: "string", value: "hello", expected: '"hello"' }, + { + name: "string containing double quotes", + value: 'he said "hello"', + expected: '"he said \\"hello\\""', + }, + { + name: "array of primitives", + value: ["admin", "auditor"], + expected: '["admin", "auditor"]', + }, + { name: "boolean true", value: true, expected: "true" }, + { name: "boolean false", value: false, expected: "false" }, + { name: "number", value: 42, expected: "42" }, + { name: "null", value: null, expected: "null" }, + { name: "undefined", value: undefined, expected: "null" }, + { + name: "invalid SQL time", + value: { Time: "0001-01-01T00:00:00Z", Valid: false }, + expected: "null", + }, + ])("preserves current behavior for $name", ({ value, expected }) => { + expect(formatAuditDiffValue(value)).toBe(expected); + }); + + it("preserves current behavior for valid SQL time objects", () => { + const value = { Time: "2024-10-22T09:03:23.961702Z", Valid: true }; + + expect(formatAuditDiffValue(value)).toBe( + new Date(value.Time).toLocaleString(), + ); + }); + + it("formats plain objects as deterministic compact JSON", () => { + expect( + formatAuditDiffValue({ + z: ["read"], + a: { permissions: ["read"] }, + }), + ).toBe('{"a":{"permissions":["read"]},"z":["read"]}'); + }); + + it("formats chat ACL objects as deterministic compact JSON", () => { + expect( + formatAuditDiffValue({ + "user-2": { permissions: ["read"] }, + "user-1": { permissions: ["read"] }, + }), + ).toBe( + '{"user-1":{"permissions":["read"]},"user-2":{"permissions":["read"]}}', + ); + }); + + it("formats arrays containing objects without object string coercion", () => { + expect( + formatAuditDiffValue([ + { user_id: "user-2", permissions: ["read"] }, + { permissions: ["read"], user_id: "user-1" }, + ]), + ).toBe( + '[{"permissions":["read"],"user_id":"user-2"}, {"permissions":["read"],"user_id":"user-1"}]', + ); + }); +}); diff --git a/site/src/pages/AuditPage/AuditLogRow/AuditLogDiff/auditUtils.ts b/site/src/pages/AuditPage/AuditLogRow/AuditLogDiff/auditUtils.ts index 7e9033841c..881463d46e 100644 --- a/site/src/pages/AuditPage/AuditLogRow/AuditLogDiff/auditUtils.ts +++ b/site/src/pages/AuditPage/AuditLogRow/AuditLogDiff/auditUtils.ts @@ -25,27 +25,68 @@ export const determineGroupDiff = (auditLogDiff: AuditDiff): AuditDiff => { }; /** - * - * @param auditLogDiff - * @returns a diff with the 'mappings' as a JSON string. Otherwise, it is [Object object] + * Formats an audit diff value for display. Strings are quoted, nullish values + * become "null", SQL time objects are localized, arrays are recursed, and plain + * objects are serialized as compact JSON with sorted keys. */ -export const determineIdPSyncMappingDiff = ( - auditLogDiff: AuditDiff, -): AuditDiff => { - const old = auditLogDiff.mapping?.old as Record | undefined; - const new_ = auditLogDiff.mapping?.new as - | Record - | undefined; - if (!old || !new_) { - return auditLogDiff; +export const formatAuditDiffValue = (value: unknown): string => { + if (typeof value === "string") { + return JSON.stringify(value); } - return { - ...auditLogDiff, - mapping: { - old: JSON.stringify(old), - new: JSON.stringify(new_), - secret: auditLogDiff.mapping?.secret, - }, - }; + if (isTimeObject(value)) { + if (!value.Valid) { + return "null"; + } + + return new Date(value.Time).toLocaleString(); + } + + if (Array.isArray(value)) { + const values = value.map((v) => formatAuditDiffValue(v)); + return `[${values.join(", ")}]`; + } + + if (value === null || value === undefined) { + return "null"; + } + + if (isPlainObject(value)) { + return JSON.stringify(sortObjectKeys(value)); + } + + return String(value); +}; + +const isTimeObject = ( + value: unknown, +): value is { Time: string; Valid: boolean } => { + return ( + value !== null && + typeof value === "object" && + "Time" in value && + typeof value.Time === "string" && + "Valid" in value && + typeof value.Valid === "boolean" + ); +}; + +const isPlainObject = (value: unknown): value is Record => { + return Object.prototype.toString.call(value) === "[object Object]"; +}; + +const sortObjectKeys = (value: unknown): unknown => { + if (Array.isArray(value)) { + return value.map(sortObjectKeys); + } + + if (!isPlainObject(value)) { + return value; + } + + const sorted: Record = {}; + for (const key of Object.keys(value).sort()) { + sorted[key] = sortObjectKeys(value[key]); + } + return sorted; }; diff --git a/site/src/pages/AuditPage/AuditLogRow/AuditLogRow.stories.tsx b/site/src/pages/AuditPage/AuditLogRow/AuditLogRow.stories.tsx index 2d4fd6e28e..d84ef16697 100644 --- a/site/src/pages/AuditPage/AuditLogRow/AuditLogRow.stories.tsx +++ b/site/src/pages/AuditPage/AuditLogRow/AuditLogRow.stories.tsx @@ -1,4 +1,5 @@ import type { Meta, StoryObj } from "@storybook/react-vite"; +import type { AuditLog } from "#/api/typesGenerated"; import { Table, TableBody } from "#/components/Table/Table"; import { chromatic } from "#/testHelpers/chromatic"; import { @@ -187,3 +188,87 @@ export const WithConnectionType: Story = { }, }, }; + +const MockChatAuditLog: AuditLog = { + ...MockAuditLog, + resource_type: "chat", + resource_id: "c542b43f-4375-421a-a7e0-b39187e35131", + resource_target: "c542b43f", + resource_icon: "", + resource_link: "/agents/c542b43f-4375-421a-a7e0-b39187e35131", + description: "{user} updated chat {target}", + additional_fields: {}, +}; + +export const WithChatACLDiff: Story = { + parameters: { chromatic }, + args: { + auditLog: { + ...MockChatAuditLog, + id: "1d718c45-5dfb-4f24-9546-4f61fa8e3402", + action: "write", + description: "{user} updated sharing for chat {target}", + diff: { + user_acl: { + old: {}, + new: { + "9a68e35d-bf3a-43bd-8e68-130df721cc71": { + permissions: ["read"], + }, + }, + secret: false, + }, + group_acl: { + old: {}, + new: { + "6d130d81-017e-44ff-8fca-3a38623dcb14": { + permissions: ["read"], + }, + }, + secret: false, + }, + }, + }, + defaultIsDiffOpen: true, + }, +}; + +export const WithArchivedChatDescription: Story = { + args: { + auditLog: { + ...MockChatAuditLog, + id: "57329396-084a-4074-9930-385a7eed858a", + action: "write", + description: "{user} archived chat {target}", + diff: { + archived: { + old: false, + new: true, + secret: false, + }, + }, + }, + }, +}; + +export const WithUpdatedChatSharingDescription: Story = { + args: { + auditLog: { + ...MockChatAuditLog, + id: "8f26cabf-8867-4d2f-942d-77e759a16c1c", + action: "write", + description: "{user} updated sharing for chat {target}", + diff: { + user_acl: { + old: {}, + new: { + "9a68e35d-bf3a-43bd-8e68-130df721cc71": { + permissions: ["read"], + }, + }, + secret: false, + }, + }, + }, + }, +}; diff --git a/site/src/pages/AuditPage/AuditLogRow/AuditLogRow.tsx b/site/src/pages/AuditPage/AuditLogRow/AuditLogRow.tsx index ea0ccc37a8..448aa393af 100644 --- a/site/src/pages/AuditPage/AuditLogRow/AuditLogRow.tsx +++ b/site/src/pages/AuditPage/AuditLogRow/AuditLogRow.tsx @@ -22,10 +22,7 @@ import { cn } from "#/utils/cn"; import { buildReasonLabels } from "#/utils/workspace"; import { AuditLogDescription } from "./AuditLogDescription/AuditLogDescription"; import { AuditLogDiff } from "./AuditLogDiff/AuditLogDiff"; -import { - determineGroupDiff, - determineIdPSyncMappingDiff, -} from "./AuditLogDiff/auditUtils"; +import { determineGroupDiff } from "./AuditLogDiff/auditUtils"; interface AuditLogRowProps { auditLog: AuditLog; @@ -53,14 +50,6 @@ export const AuditLogRow: FC = ({ auditDiff = determineGroupDiff(auditLog.diff); } - if ( - auditLog.resource_type === "idp_sync_settings_organization" || - auditLog.resource_type === "idp_sync_settings_group" || - auditLog.resource_type === "idp_sync_settings_role" - ) { - auditDiff = determineIdPSyncMappingDiff(auditLog.diff); - } - const toggle = () => { if (shouldDisplayDiff) { setIsDiffOpen((v) => !v);