diff --git a/.changeset/22510-activity-actor-name.md b/.changeset/22510-activity-actor-name.md new file mode 100644 index 00000000000..5183fe03632 --- /dev/null +++ b/.changeset/22510-activity-actor-name.md @@ -0,0 +1,30 @@ +--- +'@objectstack/plugin-audit': patch +--- + +`sys_activity.actor_name` is written: a user's create, update or delete records that user's display name on its activity row, so the record History tab names who acted instead of "Unknown user" + +Clause-②: no + +`sys_activity` declares `actor_name` and lists it among its highlight fields, because activity +entries are denormalized snapshots read in time order. The audit writer filled `actor_id` and never +`actor_name`, so every row carried the user's id and no name. A renderer that reads the name, such +as the record History tab, showed "Unknown user" for every entry, for every user. + +The writer now records the name when it writes the row. The name is the title of the user's +`sys_user` row (its `name` column), read once per user and kept for 30 seconds. A predicate write +over many rows costs one `sys_user` read for its actor, not one per row. + +- **A user write.** `actor_name` is the user's display name. `actor_id` is unchanged, and the name + is read for that same id. This includes a write the system authorized on behalf of a credited + user (`attributedUserId`). +- **A user whose name cannot be read** (no row, or a blank name). `actor_name` stays empty. The + writer never puts the id in it. +- **A write with no user** (boot, migration, a scheduled job, a flow running as the system). + `actor_name` stays empty, as before. Per ADR-0118 D1 the system actor is `null`, and the UI + renders the "System" label for it. +- **An object with `enable.activities: false`.** No activity row is written and no name is read, + as before. + +Rows written before this change keep `actor_id` and no name. This change does not backfill them. +Nothing you author changes: no key, option, export or type is added or removed. diff --git a/packages/plugins/plugin-audit/src/activity-actor-name.test.ts b/packages/plugins/plugin-audit/src/activity-actor-name.test.ts new file mode 100644 index 00000000000..15e39a65c47 --- /dev/null +++ b/packages/plugins/plugin-audit/src/activity-actor-name.test.ts @@ -0,0 +1,285 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#22510] `sys_activity.actor_name` is WRITTEN — the acting user's display + * name, captured when the activity row is written. + * + * `sys_activity` declares the column (and lists it among its highlight + * fields) because its entries are "denormalized snapshots" read + * chronologically. No writer filled it: every row carried `actor_id` and a + * null `actor_name`, and the record History tab — which reads `actor_name` — + * showed "Unknown user" for every entry, for every user. + * + * Pinned through the REAL engine (`ObjectQL`), so the user travels the way it + * does in production — `ExecutionContext.userId` → the hook session — rather + * than as a hand-built hook context: + * + * 1. a user's write names that user, on create, update and delete alike; + * 2. `actor_id` is unchanged, and is the id the name was read for (control); + * 3. a user whose name cannot be read leaves `actor_name` empty — never the + * id, which would read as a plausible name; + * 4. a write with no user leaves it empty too: ADR-0118 D1 keeps the system + * actor `null`, and its "System" label is a rendering rule, not data; + * 5. the read is one per user, not one per row — `writeAudit` runs once per + * row of a predicate write, and this is its hot path. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { installAuditWriters } from './audit-writers.js'; + +const f = (name: string, type: string, extra: Record = {}) => + ({ name, label: name, type, ...extra }) as any; + +const sysAuditLog = { + name: 'sys_audit_log', label: 'Audit Log', + fields: { + id: f('id', 'text', { primaryKey: true }), action: f('action', 'text'), + user_id: f('user_id', 'text'), object_name: f('object_name', 'text'), + record_id: f('record_id', 'text'), old_value: f('old_value', 'textarea'), + new_value: f('new_value', 'textarea'), tenant_id: f('tenant_id', 'text'), + }, +}; + +/** The two actor columns the shipped `sys_activity` declares, both present. */ +const sysActivity = { + name: 'sys_activity', label: 'Activity', + fields: { + id: f('id', 'text', { primaryKey: true }), type: f('type', 'text'), + timestamp: f('timestamp', 'datetime'), summary: f('summary', 'text'), + actor_id: f('actor_id', 'text'), actor_name: f('actor_name', 'text'), + object_name: f('object_name', 'text'), record_id: f('record_id', 'text'), + record_label: f('record_label', 'text'), metadata: f('metadata', 'textarea'), + }, +}; + +/** `nameField: 'name'`, as the shipped `sys_user` declares it. */ +const sysUser = { + name: 'sys_user', label: 'User', nameField: 'name', + fields: { + id: f('id', 'text', { primaryKey: true }), name: f('name', 'text'), + email: f('email', 'email'), + }, +}; + +const bizTask = { + name: 'biz_task', label: 'Task', + fields: { id: f('id', 'text', { primaryKey: true }), title: f('title', 'text'), status: f('status', 'text') }, +}; + +/** `enable.activities: false` — this object's writes produce no activity row. */ +const bizQuiet = { + name: 'biz_quiet', label: 'Quiet', enable: { activities: false }, + fields: { id: f('id', 'text', { primaryKey: true }), title: f('title', 'text') }, +}; + +/** + * A copy-returning store that holds the caller's bound. Reads are counted by a + * spy on `find`, outside the double, so the double itself stays a plain query. + */ +function makeCountingDriver() { + const stores = new Map>>(); + const storeFor = (o: string) => { + let s = stores.get(o); + if (!s) { s = new Map(); stores.set(o, s); } + return s; + }; + let nextId = 0; + const copy = (r: T): T => (r == null ? r : JSON.parse(JSON.stringify(r))); + const matches = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k === '$and' && Array.isArray(v)) { + if (!v.every((sub) => matches(row, sub))) return false; + continue; + } + if (k.startsWith('$')) continue; + if (v && typeof v === 'object' && '$in' in v) { + if (!(v.$in as unknown[]).includes(row[k])) return false; + continue; + } + const expected = (v && typeof v === 'object' && '$eq' in v) ? v.$eq : v; + if ((row[k] ?? null) !== (expected ?? null)) return false; + } + return true; + }; + const select = (object: string, where: any, limit?: number) => { + const rows = Array.from(storeFor(object).values()).filter((r) => matches(r, where)); + // The caller's bound, after the filter, by presence. + const page = typeof limit === 'number' ? rows.slice(0, limit) : rows; + return page.map(copy); + }; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {} as any, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find(object: string, ast: any) { return select(object, ast?.where, ast?.limit); }, + async findOne(object: string, ast: any) { return select(object, ast?.where, 1)[0] ?? null; }, + async create(object: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row = { ...data, id }; + storeFor(object).set(id, row); + return copy(row); + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const cur = s.get(id); + if (!cur) return null; + const updated = { ...cur, ...data, id }; + s.set(id, updated); + return copy(updated); + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + return id && storeFor(object).has(id) ? this.update(object, id, data) : this.create(object, data); + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count(object: string, ast: any) { return (await this.find(object, ast)).length; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r))); + }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + async updateMany(object: string, ast: any, data: Record) { + const rows = await this.find(object, ast); + const s = storeFor(object); + for (const r of rows) s.set(r.id as string, { ...s.get(r.id as string), ...data, id: r.id }); + return rows.length; + }, + async deleteMany(object: string, ast: any) { + const rows = await this.find(object, ast); + for (const r of rows) storeFor(object).delete(r.id as string); + return rows.length; + }, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + const findSpy = vi.spyOn(driver, 'find'); + /** How many `find` calls have read `object` so far. */ + const readsOf = (object: string) => findSpy.mock.calls.filter(([o]) => o === object).length; + return { driver, readsOf, storeFor }; +} + +const OWNER_PACKAGE = 'com.objectstack.test.activity-actor-name'; +const ADA = 'usr_ada'; + +async function boot() { + const engine = new ObjectQL(); + const stub = makeCountingDriver(); + engine.registerDriver(stub.driver, true); + await engine.init(); + for (const o of [sysAuditLog, sysActivity, sysUser, bizTask, bizQuiet]) { + engine.registry.registerObject(o as any, OWNER_PACKAGE); + } + installAuditWriters(engine as any, 'test.audit'); + // Seeded with no context: a system write, which names nobody. + await engine.insert('sys_user', { id: ADA, name: 'Ada Lovelace', email: 'ada@example.com' }); + return { engine, ...stub }; +} + +const as = (userId: string) => ({ context: { userId } }); + +/** Every activity row written about `recordId`, in write order. */ +const activityFor = (storeFor: (o: string) => Map>, recordId: string) => + Array.from(storeFor('sys_activity').values()).filter((r) => r.record_id === recordId); + +describe('[#22510] sys_activity.actor_name — the acting user\'s name, written with the row', () => { + it('a user\'s create, update and delete each carry that user\'s name', async () => { + const { engine, storeFor } = await boot(); + + await engine.insert('biz_task', { id: 't1', title: 'Call Acme', status: 'open' }, as(ADA) as any); + await engine.update('biz_task', { status: 'done' }, { where: { id: 't1' }, ...as(ADA) } as any); + await engine.delete('biz_task', { where: { id: 't1' }, ...as(ADA) } as any); + + const rows = activityFor(storeFor, 't1'); + expect(rows.map((r) => r.type)).toEqual(['created', 'updated', 'deleted']); + for (const row of rows) { + expect(row.actor_name).toBe('Ada Lovelace'); + // Control: `actor_id` is exactly what it was — the same id the name was read for. + expect(row.actor_id).toBe(ADA); + } + }); + + it('names the human a system-authorized write is ATTRIBUTED to — the same id `actor_id` records', async () => { + const { engine, storeFor } = await boot(); + + // The better-auth envelope: authorized as the system, crediting a human + // through provenance (`attributedUserId`) rather than the session. + await engine.insert( + 'biz_task', + { id: 't2', title: 'Grade change', status: 'open' }, + { context: { isSystem: true, attributedUserId: ADA } } as any, + ); + + const [row] = activityFor(storeFor, 't2'); + expect(row.actor_id).toBe(ADA); + expect(row.actor_name).toBe('Ada Lovelace'); + }); + + it('a user whose name cannot be read leaves actor_name empty — never the id', async () => { + const { engine, storeFor } = await boot(); + await engine.insert('sys_user', { id: 'usr_blank', name: ' ', email: 'blank@example.com' }); + + // No `sys_user` row at all, and a row whose name is blank. + await engine.insert('biz_task', { id: 't3', title: 'Ghost', status: 'open' }, as('usr_ghost') as any); + await engine.insert('biz_task', { id: 't4', title: 'Blank', status: 'open' }, as('usr_blank') as any); + + const [ghost] = activityFor(storeFor, 't3'); + const [blank] = activityFor(storeFor, 't4'); + expect(ghost.actor_id).toBe('usr_ghost'); + expect(ghost.actor_name ?? null).toBeNull(); + expect(blank.actor_id).toBe('usr_blank'); + expect(blank.actor_name ?? null).toBeNull(); + }); + + it('a write with no user leaves actor_name empty (ADR-0118 D1: the system actor is null) and reads no user', async () => { + const { engine, storeFor, readsOf } = await boot(); + const before = readsOf('sys_user'); + + await engine.insert('biz_task', { id: 't5', title: 'Nightly', status: 'open' }, { context: { isSystem: true } } as any); + // A service principal on `actor` is not a user either (ADR-0118 D5: user or system). + await engine.insert( + 'biz_task', + { id: 't6', title: 'Flow', status: 'open' }, + { context: { isSystem: true, actor: 'svc:flow:nightly' } } as any, + ); + + for (const id of ['t5', 't6']) { + const [row] = activityFor(storeFor, id); + expect(row.actor_id).toBeNull(); + expect(row.actor_name ?? null).toBeNull(); + } + expect(readsOf('sys_user') - before).toBe(0); + }); +}); + +describe('[#22510] the name costs one read per user, not one per row', () => { + it('a predicate update over three rows reads sys_user ONCE, and a later write by the same user not at all', async () => { + const { engine, storeFor, readsOf } = await boot(); + for (const id of ['p1', 'p2', 'p3']) { + await engine.insert('biz_task', { id, title: id, status: 'open' }); + } + const before = readsOf('sys_user'); + + await engine.update('biz_task', { status: 'done' }, { where: { status: 'open' }, multi: true, ...as(ADA) } as any); + + const updated = Array.from(storeFor('sys_activity').values()).filter((r) => r.type === 'updated'); + expect(updated).toHaveLength(3); + for (const row of updated) expect(row.actor_name).toBe('Ada Lovelace'); + expect(readsOf('sys_user') - before).toBe(1); + + await engine.update('biz_task', { title: 'renamed' }, { where: { id: 'p1' }, ...as(ADA) } as any); + expect(readsOf('sys_user') - before).toBe(1); + }); + + it('an object with `enable.activities: false` writes no activity row and reads no user', async () => { + const { engine, storeFor, readsOf } = await boot(); + const before = readsOf('sys_user'); + + await engine.insert('biz_quiet', { id: 'q1', title: 'Quiet' }, as(ADA) as any); + + expect(activityFor(storeFor, 'q1')).toHaveLength(0); + expect(readsOf('sys_user') - before).toBe(0); + }); +}); diff --git a/packages/plugins/plugin-audit/src/audit-writers.ts b/packages/plugins/plugin-audit/src/audit-writers.ts index facf776b723..fecc7a3d630 100644 --- a/packages/plugins/plugin-audit/src/audit-writers.ts +++ b/packages/plugins/plugin-audit/src/audit-writers.ts @@ -26,6 +26,9 @@ import { SECRET_MASK, collectInternalReadFields, collectMaskedReadFields } from // picker, the search companion and the approval inbox the day an author sets // `nameField` — the same argument the SECRET_MASK import above makes. import { referenceTargetOf, resolveDisplayField } from '@objectstack/spec/data'; +// [#22510] The acting user's object, named by the registry constant rather +// than a literal — `sys_activity.actor_name` is resolved from its title. +import { SystemObjectName } from '@objectstack/spec/system'; // [#21120] The family-wide stored-metadata-body seam. `sys_metadata` / // `sys_metadata_history` rows carry a serialized metadata BODY in their // `metadata` column (a datasource body holds stored credential material), and @@ -1366,6 +1369,47 @@ export function installAuditWriters( return out.size > 0 ? out : undefined; }; + /** + * [#22510] The acting user's display name, for `sys_activity.actor_name`. + * + * The object declares that column — and lists it among its highlight fields — + * because "activity entries are denormalized snapshots" read + * chronologically (`sys-activity.object.ts`). No writer filled it, so every + * record History entry read "Unknown user" while its `actor_id` named the + * user. The name is written HERE, at write time; nothing resolves it on read. + * + * The source is `sys_user`'s title through {@link resolveLookupTitles}: the + * ADR-0079 answer for that object (`nameField: 'name'`, the profile + * display-name column the approval timeline resolves its actors from), and + * the same answer this writer already renders for a `user` reference in a + * tracked-change summary. One title resolution for a user in this file. + * + * A user whose name cannot be read — no row, a blank name, an unregistered + * `sys_user`, a failing read — answers `null`, never the id: an id in a name + * column is a plausible wrong value no reader can tell from a name (the + * class `actor-user.ts` describes), while `null` is the empty every reader + * already handles, and `actor_id` still says who acted. + * + * Cost: memoized per user for {@link ACTOR_NAME_TTL_MS}, and the PROMISE is + * what is kept, so concurrent rows share one read. A predicate write over N + * rows dispatches `writeAudit` once per row; its actor costs one `sys_user` + * read, not N — the hot path #6656 / PR #6977 took reads off. A rename shows + * on rows written after the window; rows already written keep the name they + * were written with, which is what a snapshot is. + */ + const ACTOR_NAME_TTL_MS = 30_000; + const actorNameCache = new Map; expires: number }>(); + const resolveActorName = (api: any, userId: string): Promise => { + const now = Date.now(); + const hit = actorNameCache.get(userId); + if (hit && hit.expires > now) return hit.value; + const value = resolveLookupTitles(api, new Map([[SystemObjectName.USER, new Set([userId])]])) + .then((titles) => titles?.get(SystemObjectName.USER)?.get(userId) ?? null) + .catch(() => null); + actorNameCache.set(userId, { value, expires: now + ACTOR_NAME_TTL_MS }); + return value; + }; + /** * [#6656, Option A+] The view the ledger records for one record. * @@ -1854,6 +1898,20 @@ export function installAuditWriters( } const textSources: ActivityTextSources = { summary: summarySources, record_label: labelSources }; + // `enable.activities` is an opt-OUT capability (#2707): absent block/flag + // = mirror on (spec default `true`), explicit `false` = this object's CRUD + // is not mirrored into the sys_activity timeline. This is the per-object + // lever for activity-row growth (ADR-0057). The compliance audit row is + // NOT gated — sys_audit_log capture stays unconditional. + const activitiesEnabled = getObjectDef(ctx.object)?.enable?.activities !== false; + // [#22510] The name of the user `actor_id` names — the SAME `userId`, so + // the two columns cannot disagree about who acted. Read only when an + // activity row will be written. No user ⇒ no name: ADR-0118 D1 keeps the + // system actor `null`, and rendering it as "System" is the UI's i18n rule, + // not data this writer persists. + const actorName: string | null = + activitiesEnabled && userId ? await resolveActorName(api, userId) : null; + const activityRow: Record = { type: activityType, // Explicit ISO timestamp — `defaultValue: 'NOW()'` on the column @@ -1862,6 +1920,9 @@ export function installAuditWriters( timestamp: new Date().toISOString(), summary, actor_id: userId ?? null, + // [#22510] Present only when a name resolved: an unresolved actor leaves + // the column exactly as every row before this change left it. + ...(actorName ? { actor_name: actorName } : {}), object_name: ctx.object, record_id: recordId ?? null, record_label: label, @@ -1876,12 +1937,6 @@ export function installAuditWriters( activityRow.organization_id = tenantId ?? null; } - // `enable.activities` is an opt-OUT capability (#2707): absent block/flag - // = mirror on (spec default `true`), explicit `false` = this object's CRUD - // is not mirrored into the sys_activity timeline. This is the per-object - // lever for activity-row growth (ADR-0057). The compliance audit row is - // NOT gated — sys_audit_log capture stays unconditional. - const activitiesEnabled = getObjectDef(ctx.object)?.enable?.activities !== false; const activityRowToWrite = activitiesEnabled ? activityRow : undefined; // [#21262] The ledger row is written first; the writer advances this // before the activity row, so after a throw it names the refused table.