diff --git a/packages/agent/src/audit-trail/README.md b/packages/agent/src/audit-trail/README.md index 70d4b9f0d3..5c6c64ad1f 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -86,6 +86,7 @@ The `forest.audit_logs` table has one row per audited change: | `operation` | `create` / `update` / `delete` / `action` / `action_failed` | | `collection` | audited collection name | | `record_id` | packed record id (primary keys joined with `\|`); `null` for a `create` row still `pending` (the record's id isn't assigned yet) | +| `previous_record_id` | the id the row was filed under before a confirmed `update`, set whether or not the key moved; `null` on every other operation and on any row written before this column existed. Internal: never served to a client | | `user_id` | id of the Forest user who made the change | | `user_first_name` | that user's first name, denormalized at write time | | `user_last_name` | that user's last name, denormalized at write time | @@ -382,7 +383,14 @@ across pages rather than looping. Feed the two values back verbatim; don't synth must always add at least the last row's id to the exclusions, so a cursor that would come back unchanged ends the walk instead of repeating the page forever. +**Authors.** On the first page only (no `before`), `meta` also carries `availableUsers`: the +distinct authors matching the active filters across the collections queried, independent of the +cursor, in the per-record route's shape. Later pages omit the key rather than send `[]`, so a client +keeps the list it already saw. + A custom `AuditStore` that doesn't implement `listTimeline` simply doesn't get this route mounted. +One that implements `listTimeline` without `listTimelineUsers` serves the rows with no +`availableUsers`. ### `GET /forest/_audit-trail/correlation/{correlationKey}` diff --git a/packages/agent/src/audit-trail/instrument.ts b/packages/agent/src/audit-trail/instrument.ts index 79572c28f5..7921b0e69c 100644 --- a/packages/agent/src/audit-trail/instrument.ts +++ b/packages/agent/src/audit-trail/instrument.ts @@ -480,6 +480,10 @@ function instrumentCollection( return recorder.confirm(pendingId, { operation: 'update', recordId: toPackedRecordId(updated, primaryKeys), + // Written whether or not the key moved. Only a value here lets the route trust that the + // previous side's id is known: a null has to keep meaning "this row predates the column", + // or an old row whose key did move would be judged by the id it moved to. + previousRecordId: toPackedRecordId(record, primaryKeys), previousValues: redactValues(previousValues, redactedFields), newValues: redactValues(newValues, redactedFields), }); diff --git a/packages/agent/src/audit-trail/migrations.ts b/packages/agent/src/audit-trail/migrations.ts index da82101bc6..13ae940765 100644 --- a/packages/agent/src/audit-trail/migrations.ts +++ b/packages/agent/src/audit-trail/migrations.ts @@ -35,6 +35,12 @@ function qualifiedMigrationName002(schema: string | undefined, tableName: string : `${tableName}:002-index-timestamp-id`; } +function qualifiedMigrationName003(schema: string | undefined, tableName: string): string { + return schema + ? `${schema}.${tableName}:003-add-previous-record-id` + : `${tableName}:003-add-previous-record-id`; +} + // Every table name claimed by an audit-trail store configured in this process, keyed by // `schema\0name` — both its own data table and the migration table Umzug derives from it. // A second store's data table can otherwise land on the exact name a first store's migration @@ -240,6 +246,44 @@ function buildMigrations(schema: string | undefined, tableName: string) { ); }, }, + { + // Its own migration rather than a column added to 001: the table has shipped, so a database + // out there has already recorded 001 as applied and would never see the edit. + name: qualifiedMigrationName003(schema, tableName), + up: async ({ context }: { context: MigrationContext }) => { + const table = { tableName: context.tableName, schema: context.schema }; + const existing = await columnNames(context.queryInterface, table, context.transaction); + + // Idempotent for the same reason 001 is: a process losing a concurrent-boot race retries. + if (existing.has('previous_record_id')) return; + + try { + await context.queryInterface.addColumn( + table, + 'previous_record_id', + { type: DataTypes.TEXT, allowNull: true }, + { transaction: context.transaction }, + ); + } catch (error) { + // Without Postgres's advisory lock, another agent booting at the same moment can add the + // column between the check above and this statement. Its duplicate-column error then + // means the work is done. Anything else, or a column still missing, is a real failure. + const added = await columnNames(context.queryInterface, table, context.transaction).then( + columns => columns.has('previous_record_id'), + () => false, + ); + + if (!added) throw error; + } + }, + down: async ({ context }: { context: MigrationContext }) => { + await context.queryInterface.removeColumn( + { tableName: context.tableName, schema: context.schema }, + 'previous_record_id', + { transaction: context.transaction }, + ); + }, + }, ]; } diff --git a/packages/agent/src/audit-trail/sql-store.ts b/packages/agent/src/audit-trail/sql-store.ts index 69e93846fc..9459be3d8f 100644 --- a/packages/agent/src/audit-trail/sql-store.ts +++ b/packages/agent/src/audit-trail/sql-store.ts @@ -5,6 +5,7 @@ import type { AuditStorageOptions, AuditStore, AuditTimelineQuery, + AuditUserSummary, PendingAuditRecord, } from './types'; import type { Model, ModelStatic } from 'sequelize'; @@ -38,6 +39,9 @@ export function defineAuditLogModel( // migration creates the column as such — this must match. Nullable: a pending create's row // has no id yet, since the record doesn't exist until the write resolves. recordId: { type: DataTypes.TEXT, allowNull: true }, + // Set on every confirmed update, so a null distinguishes a row older than the column from + // one whose key held still. TEXT for the same reason as `recordId`. + previousRecordId: { type: DataTypes.TEXT, allowNull: true }, userId: { type: DataTypes.INTEGER, allowNull: true }, // Denormalised from the caller at write time — who acted then, not who holds that id today. userFirstName: { type: DataTypes.TEXT, allowNull: true }, @@ -79,6 +83,9 @@ export function toRow( operation: record.operation, collection: record.collection, recordId: record.recordId, + // Optional at insert, but kept when given, as the in-memory store keeps it: a later `confirm` + // that omits it must not read as a row older than the column. + previousRecordId: record.previousRecordId ?? null, userId: record.userId, userFirstName: record.userFirstName, userLastName: record.userLastName, @@ -243,7 +250,7 @@ function buildTimelineWhereClause( startTimestamp, endTimestamp, search, - }: AuditTimelineQuery, + }: Omit, sequelize: Sequelize, ): Record { const where: Record = { collection: { [Op.in]: collections } }; @@ -269,6 +276,38 @@ function buildTimelineWhereClause( return where; } +async function listAuthors( + model: ModelStatic, + where: Record, +): Promise { + // MAX() rather than a bare column: grouping by `user_id` alone is invalid in strict SQL + // unless every selected column is either grouped or aggregated. + const rows = (await model.findAll({ + where, + attributes: [ + 'userId', + [Sequelize.fn('MAX', Sequelize.col('user_first_name')), 'userFirstName'], + [Sequelize.fn('MAX', Sequelize.col('user_last_name')), 'userLastName'], + [Sequelize.fn('MAX', Sequelize.col('user_email')), 'userEmail'], + ], + group: ['userId'], + raw: true, + transaction: null, + })) as unknown as Array<{ + userId: number; + userFirstName: string | null; + userLastName: string | null; + userEmail: string | null; + }>; + + return rows.map(row => ({ + id: row.userId, + firstName: row.userFirstName ?? null, + lastName: row.userLastName ?? null, + email: row.userEmail ?? null, + })); +} + export function fromRow(row: Model): AuditRecord { const plain = row.get({ plain: true }) as Record; const { timestamp } = plain; @@ -279,6 +318,7 @@ export function fromRow(row: Model): AuditRecord { operation: plain.operation as AuditRecord['operation'], collection: plain.collection as string, recordId: (plain.recordId as string) ?? null, + previousRecordId: (plain.previousRecordId as string) ?? null, userId: plain.userId as number, userFirstName: (plain.userFirstName as string) ?? null, userLastName: (plain.userLastName as string) ?? null, @@ -420,32 +460,14 @@ export function createSqlAuditStore(options: AuditStorageOptions): { async listDistinctUsers(query) { const { model, connection } = await init(); - // MAX() rather than a bare column: grouping by `user_id` alone is invalid in strict SQL - // unless every selected column is either grouped or aggregated. - const rows = (await model.findAll({ - where: buildHistoryWhereClause(query as AuditHistoryQuery, connection), - attributes: [ - 'userId', - [Sequelize.fn('MAX', Sequelize.col('user_first_name')), 'userFirstName'], - [Sequelize.fn('MAX', Sequelize.col('user_last_name')), 'userLastName'], - [Sequelize.fn('MAX', Sequelize.col('user_email')), 'userEmail'], - ], - group: ['userId'], - raw: true, - transaction: null, - })) as unknown as Array<{ - userId: number; - userFirstName: string | null; - userLastName: string | null; - userEmail: string | null; - }>; - - return rows.map(row => ({ - id: row.userId, - firstName: row.userFirstName ?? null, - lastName: row.userLastName ?? null, - email: row.userEmail ?? null, - })); + return listAuthors(model, buildHistoryWhereClause(query as AuditHistoryQuery, connection)); + }, + async listTimelineUsers(query) { + if (!query.collections.length) return []; + + const { model, connection } = await init(); + + return listAuthors(model, buildTimelineWhereClause(query, connection)); }, async listByCorrelation({ collection, recordId, correlationKey }) { const { model } = await init(); diff --git a/packages/agent/src/audit-trail/types.ts b/packages/agent/src/audit-trail/types.ts index 75c9b1c468..8abdd32190 100644 --- a/packages/agent/src/audit-trail/types.ts +++ b/packages/agent/src/audit-trail/types.ts @@ -11,6 +11,13 @@ export type AuditRecord = { collection: string; /** Null for a pending create — the record's primary key isn't assigned yet. */ recordId: string | null; + /** + * The id this record was filed under before a confirmed update, written whether or not the key + * moved. Null therefore means "written before this column existed", not "the key held still": a + * row from an earlier agent cannot claim its id answers for the previous side of an update. + * Internal — it is how the agent follows a record across a rename, never served to a client. + */ + previousRecordId: string | null; userId: number; /** Denormalised from the caller at write time: who acted then, not who holds that id today. */ userFirstName: string | null; @@ -30,13 +37,18 @@ export type AuditRecord = { }; /** The subset known before the write runs, when the pending row is first inserted. */ -export type PendingAuditRecord = Omit; +export type PendingAuditRecord = Omit & + Partial>; -/** What `confirm` updates once the write (or action) has resolved. */ +/** + * What `confirm` updates once the write (or action) has resolved. `previousRecordId` is optional + * because only an update has a previous side to file under an id of its own. + */ export type AuditRecordConfirmation = Pick< AuditRecord, 'operation' | 'recordId' | 'previousValues' | 'newValues' ->; +> & + Partial>; export type AuditHistoryQuery = { collection: string; @@ -140,6 +152,14 @@ export interface AuditStore { * written before this existed simply doesn't serve the project-level route. */ listTimeline?(query: AuditTimelineQuery): AuditRecord[] | Promise; + /** + * Distinct authors matching the timeline's filters, independent of its cursor. Optional like + * `listTimeline`: without it the route serves the rows but no author list. An empty + * `collections` must match nothing. + */ + listTimelineUsers?( + query: Omit, + ): AuditUserSummary[] | Promise; /** Distinct authors matching the query filters, independent of pagination. */ listDistinctUsers( query: Omit, diff --git a/packages/agent/src/audit-trail/withhold.ts b/packages/agent/src/audit-trail/withhold.ts index 1bea817644..4d89222ae8 100644 --- a/packages/agent/src/audit-trail/withhold.ts +++ b/packages/agent/src/audit-trail/withhold.ts @@ -195,22 +195,40 @@ export default function withholdOutsidePermissionScope( return entries.map(entry => { if (entry.operation === 'action' || entry.operation === 'action_failed') return entry; - // Decoded once per row rather than once per side: the two sides read the same id, and a row - // whose id no longer decodes should say so once. - const decoded = decodePrimaryKeys(entry.recordId, withholding.collection, withholding.logger); - - const side = (values: Record, idAnswersForSide: IdAnswersForSide) => + const side = ( + values: Record, + decoded: ReturnType, + idAnswersForSide: IdAnswersForSide, + ) => permissionScopeAccepts(answerableSnapshot(values, decoded, idAnswersForSide), withholding) ? values ?? {} : {}; + // An update that recorded where it came from is judged side by side: the previous state + // against the id it was filed under then, the new state against the id it ended up with. A row + // without that column is older than it, so its id still answers for the new side alone. + // `?? null` first: the column is optional on the write types, so an absent one has to read as + // unknown exactly like a null, or a row that never recorded it would answer from the id it + // ended up with. + const previousRecordId = entry.previousRecordId ?? null; + const previousIsKnown = entry.operation !== 'update' || previousRecordId !== null; + + // Decoded once per distinct id, not once per side: a row whose id no longer decodes should say + // so once, and the two sides read the same id unless the key actually moved. + const decoded = decodePrimaryKeys(entry.recordId, withholding.collection, withholding.logger); + const decodedPrevious = + previousRecordId === null + ? decoded + : decodePrimaryKeys(previousRecordId, withholding.collection, withholding.logger); + + // A pending update is filed under the id the record had before the write, which says nothing + // about the state it was moving to, so the new side answers only with what it captured. + const newIsKnown = entry.status !== 'pending'; + return { ...entry, - // The row is filed under the identity the record ended up with, so its id answers for the - // new side of an update and for a create or a delete — never for what an update moved away - // from. - previousValues: side(entry.previousValues, entry.operation !== 'update'), - newValues: side(entry.newValues, true), + previousValues: side(entry.previousValues, decodedPrevious, previousIsKnown), + newValues: side(entry.newValues, decoded, newIsKnown), }; }); } diff --git a/packages/agent/src/routes/access/audit-trail-timeline.ts b/packages/agent/src/routes/access/audit-trail-timeline.ts index d4aa88aa0f..199225bc4e 100644 --- a/packages/agent/src/routes/access/audit-trail-timeline.ts +++ b/packages/agent/src/routes/access/audit-trail-timeline.ts @@ -54,16 +54,26 @@ export default class AuditTrailTimelineRoute extends BaseRoute { const collections = await this.readableCollections(context); const { store } = this.options.auditTrail; + // Authors on the first page only, like the per-record route and Forest's activity-logs route: + // they do not change along the walk, and later pages omit the key rather than send `[]`. // One row over the page: the only way to know whether a further page exists without a count. - const fetched = collections.length - ? await store.listTimeline({ ...filters, ...cursor, collections, limit: limit + 1 }) - : []; + const [fetched, availableUsers] = await Promise.all([ + collections.length + ? store.listTimeline({ ...filters, ...cursor, collections, limit: limit + 1 }) + : [], + !cursor.before && store.listTimelineUsers + ? store.listTimelineUsers({ ...filters, collections }) + : undefined, + ]); const page = fetched.slice(0, limit); context.response.body = { - data: page, + // `previousRecordId` stays out: it is how the agent follows a record across a rename, not + // something a client reads. + data: page.map(({ previousRecordId, ...served }) => served), meta: { cursor: fetched.length > limit ? AuditTrailTimelineRoute.nextCursor(page, cursor) : null, + ...(availableUsers && { availableUsers }), }, }; } diff --git a/packages/agent/src/routes/access/audit-trail.ts b/packages/agent/src/routes/access/audit-trail.ts index 456ee5e331..94edab757a 100644 --- a/packages/agent/src/routes/access/audit-trail.ts +++ b/packages/agent/src/routes/access/audit-trail.ts @@ -106,7 +106,9 @@ export default class AuditTrailRoute extends CollectionRoute { }); context.response.body = { - data: matched.page, + // `previousRecordId` stays out: it is how the agent follows a record across a rename, not + // something a client reads. + data: matched.page.map(({ previousRecordId, ...served }) => served), meta: { count: matched.count, ...(isFirstFetch && { availableUsers: [...matched.authors.values()] }), @@ -165,7 +167,7 @@ export default class AuditTrailRoute extends CollectionRoute { permissionScope && gone ? this.withhold(rawData, permissionScope, context) : rawData; context.response.body = { - data, + data: data.map(({ previousRecordId, ...served }) => served), meta: { count, ...(availableUsers && { availableUsers }) }, }; } diff --git a/packages/agent/test/audit-trail/in-memory-store.test.ts b/packages/agent/test/audit-trail/in-memory-store.test.ts index 286dd4f60f..b219f14d79 100644 --- a/packages/agent/test/audit-trail/in-memory-store.test.ts +++ b/packages/agent/test/audit-trail/in-memory-store.test.ts @@ -9,6 +9,7 @@ const record = ( operation: 'update', collection: 'accounts', recordId: '1', + previousRecordId: null, userId: 1, userFirstName: null, userLastName: null, diff --git a/packages/agent/test/audit-trail/in-memory-store.ts b/packages/agent/test/audit-trail/in-memory-store.ts index 2e06bbc95e..1df86706ea 100644 --- a/packages/agent/test/audit-trail/in-memory-store.ts +++ b/packages/agent/test/audit-trail/in-memory-store.ts @@ -31,7 +31,7 @@ export default class InMemoryAuditStore implements AuditStore { async insertPending(record: PendingAuditRecord): Promise { const id = this.nextId; this.nextId += 1; - this.records.push({ ...record, id, status: 'pending' }); + this.records.push({ previousRecordId: null, ...record, id, status: 'pending' }); return id; } @@ -112,10 +112,19 @@ export default class InMemoryAuditStore implements AuditStore { .slice(0, limit); } + listTimelineUsers( + query: Omit, + ): AuditUserSummary[] { + return InMemoryAuditStore.authorsOf(this.listTimeline({ ...query, limit: Infinity })); + } + listDistinctUsers( query: Omit, ): AuditUserSummary[] { - const matches = this.matching({ ...query, order: 'asc' }); + return InMemoryAuditStore.authorsOf(this.matching({ ...query, order: 'asc' })); + } + + private static authorsOf(matches: AuditRecord[]): AuditUserSummary[] { const byUser = new Map(); for (const record of matches) { diff --git a/packages/agent/test/audit-trail/instrument.test.ts b/packages/agent/test/audit-trail/instrument.test.ts index 9c7d2128c5..9f721469a4 100644 --- a/packages/agent/test/audit-trail/instrument.test.ts +++ b/packages/agent/test/audit-trail/instrument.test.ts @@ -702,12 +702,32 @@ describe('auditTrail plugin', () => { expect(sink).toHaveBeenCalledWith( expect.objectContaining({ recordId: 'new-slug', + // What the route needs to judge the previous side by the identity it actually had. + previousRecordId: 'old-slug', previousValues: { slug: 'old-slug' }, newValues: { slug: 'new-slug' }, }), ); }); + it('records where a row came from even when the key held still', async () => { + const sink = jest.fn(); + const accounts = fakeCollection('accounts', [{ id: 1, name: 'Acme', amount: 10 }]); + register([accounts], { sink }); + + await runUpdate(accounts, { + caller: makeCaller(), + patch: { name: 'Acme Inc' }, + after: [{ id: 1, name: 'Acme Inc', amount: 10 }], + }); + + // Not only on a move: a null has to keep meaning "written before this column existed", or a + // row from an older agent whose key did move would be judged by the id it moved to. + expect(sink).toHaveBeenCalledWith( + expect.objectContaining({ recordId: '1', previousRecordId: '1' }), + ); + }); + it('still records an update that changes a field the update was itself filtered on', async () => { const sink = jest.fn(); const accounts = fakeCollection('accounts', [ diff --git a/packages/agent/test/audit-trail/migrations.test.ts b/packages/agent/test/audit-trail/migrations.test.ts index 10051ec769..cee5cd0a10 100644 --- a/packages/agent/test/audit-trail/migrations.test.ts +++ b/packages/agent/test/audit-trail/migrations.test.ts @@ -43,6 +43,7 @@ describe('runAuditMigrations (sqlite)', () => { 'id', 'new_values', 'operation', + 'previous_record_id', 'previous_values', 'record_id', 'status', @@ -71,6 +72,7 @@ describe('runAuditMigrations (sqlite)', () => { expect(applied).toEqual([ { name: 'forest.audit_logs:001-create-audit-logs' }, { name: 'forest.audit_logs:002-index-timestamp-id' }, + { name: 'forest.audit_logs:003-add-previous-record-id' }, ]); await sequelize.close(); @@ -85,6 +87,7 @@ describe('runAuditMigrations (sqlite)', () => { expect(applied).toEqual([ { name: 'audit_logs:001-create-audit-logs' }, { name: 'audit_logs:002-index-timestamp-id' }, + { name: 'audit_logs:003-add-previous-record-id' }, ]); await sequelize.close(); @@ -169,7 +172,7 @@ describe('runAuditMigrations (sqlite)', () => { ).resolves.toBeUndefined(); const [applied] = await sequelize.query('SELECT name FROM "audit_logs_migration"'); - expect(applied).toHaveLength(2); + expect(applied).toHaveLength(3); await sequelize.close(); }); @@ -255,7 +258,7 @@ describe('runAuditMigrations (sqlite)', () => { const umzug = buildUmzug(sequelize, { tableName: 'audit_logs' }); await umzug.up(); - await umzug.down(); + await umzug.down({ step: 2 }); expect(await indexOf(sequelize)).toBeUndefined(); @@ -263,6 +266,49 @@ describe('runAuditMigrations (sqlite)', () => { }); }); + it('treats previous_record_id as added when a concurrent boot added it after the check', async () => { + const sequelize = new Sequelize('sqlite::memory:', { logging: false }); + await runAuditMigrations(sequelize, { tableName: 'audit_logs' }); + await sequelize.query( + `DELETE FROM "audit_logs_migration" WHERE name = 'audit_logs:003-add-previous-record-id'`, + ); + // The check reads the table before the other boot's column lands, so this one still adds it. + const queryInterface = sequelize.getQueryInterface(); + const describeTable = queryInterface.describeTable.bind(queryInterface); + jest.spyOn(queryInterface, 'describeTable').mockImplementationOnce(async (...args) => { + const { previous_record_id: lateColumn, ...columns } = await describeTable(...args); + + return columns; + }); + + await expect( + runAuditMigrations(sequelize, { tableName: 'audit_logs' }), + ).resolves.toBeUndefined(); + + const [applied] = await sequelize.query('SELECT name FROM "audit_logs_migration"'); + expect(applied).toHaveLength(3); + + await sequelize.close(); + }); + + it('still fails migration 003 when adding the column fails and the column is missing', async () => { + const sequelize = new Sequelize('sqlite::memory:', { logging: false }); + await runAuditMigrations(sequelize, { tableName: 'audit_logs' }); + await sequelize.query('ALTER TABLE "audit_logs" DROP COLUMN "previous_record_id"'); + await sequelize.query( + `DELETE FROM "audit_logs_migration" WHERE name = 'audit_logs:003-add-previous-record-id'`, + ); + jest + .spyOn(sequelize.getQueryInterface(), 'addColumn') + .mockRejectedValueOnce(new Error('disk full')); + + await expect(runAuditMigrations(sequelize, { tableName: 'audit_logs' })).rejects.toThrow( + 'disk full', + ); + + await sequelize.close(); + }); + it('rejects a pre-existing table sharing the name that is missing audit-trail columns', async () => { const sequelize = new Sequelize('sqlite::memory:', { logging: false }); await sequelize.getQueryInterface().createTable('audit_logs', { diff --git a/packages/agent/test/audit-trail/sql-store.test.ts b/packages/agent/test/audit-trail/sql-store.test.ts index bb0a56f615..c13d2b0fab 100644 --- a/packages/agent/test/audit-trail/sql-store.test.ts +++ b/packages/agent/test/audit-trail/sql-store.test.ts @@ -51,6 +51,7 @@ describe('toRow', () => { operation: 'update', collection: 'accounts', recordId: '1', + previousRecordId: null, userId: 42, userFirstName: 'Jane', userLastName: 'Doe', @@ -62,6 +63,14 @@ describe('toRow', () => { status: 'done', }); }); + + it('keeps a previousRecordId given at insert, like the in-memory store', () => { + const status: AuditStatus = 'pending'; + + expect(toRow({ ...record(), previousRecordId: '7', status })).toEqual( + expect.objectContaining({ previousRecordId: '7' }), + ); + }); }); describe('ensureAuditStorage', () => { @@ -964,6 +973,37 @@ describe('createSqlAuditStore (sqlite round-trip)', () => { await close(); }); + + describe('listTimelineUsers', () => { + it('returns the distinct authors on the allowed collections, under the timeline filters', async () => { + const { store, close } = createSqlAuditStore({ connectionString: 'sqlite::memory:' }); + + await seed(store, record({ collection: 'accounts', userId: 1, userFirstName: 'Jane' })); + await seed(store, record({ collection: 'books', userId: 1, userFirstName: 'Jane' })); + await seed(store, record({ collection: 'books', userId: 2, operation: 'delete' })); + await seed(store, record({ collection: 'secrets', userId: 3, userFirstName: 'Hidden' })); + + const users = await store.listTimelineUsers({ + collections: ['accounts', 'books'], + operations: ['update'], + }); + + expect(users).toEqual([ + { id: 1, firstName: 'Jane', lastName: 'Doe', email: 'jane.doe@forest.dev' }, + ]); + + await close(); + }); + + it('matches nothing when no collection is allowed', async () => { + const { store, close } = createSqlAuditStore({ connectionString: 'sqlite::memory:' }); + await seed(store, record({ collection: 'accounts', userId: 1 })); + + expect(await store.listTimelineUsers({ collections: [] })).toEqual([]); + + await close(); + }); + }); }); describe('fieldsChangedCondition', () => { diff --git a/packages/agent/test/routes/access/audit-trail-timeline.test.ts b/packages/agent/test/routes/access/audit-trail-timeline.test.ts index 934c3ce00a..b849260bae 100644 --- a/packages/agent/test/routes/access/audit-trail-timeline.test.ts +++ b/packages/agent/test/routes/access/audit-trail-timeline.test.ts @@ -18,7 +18,7 @@ describe('AuditTrailTimelineRoute', () => { ...patch, }); - const setup = (timeline: unknown[] = []) => { + const setup = (timeline: unknown[] = [], users?: unknown[]) => { const services = factories.forestAdminHttpDriverServices.build(); const dataSource = factories.dataSource.buildWithCollections([ factories.collection.build({ @@ -34,7 +34,10 @@ describe('AuditTrailTimelineRoute', () => { }), }), ]); - const store = { listTimeline: jest.fn().mockResolvedValue(timeline) }; + const store = { + listTimeline: jest.fn().mockResolvedValue(timeline), + ...(users && { listTimelineUsers: jest.fn().mockResolvedValue(users) }), + }; const options = factories.forestAdminHttpDriverOptions.build({ auditTrail: { connectionString: 'sqlite::memory:', store } as never, }); @@ -145,6 +148,59 @@ describe('AuditTrailTimelineRoute', () => { expect(store.listTimeline).not.toHaveBeenCalled(); }); + test('never serves the id a record moved from, which is internal', async () => { + const { route } = setup([row(1, { previousRecordId: '1' })]); + const context = contextWith(); + + await route.handleTimeline(context); + + expect((context.response.body as { data: object[] }).data[0]).not.toHaveProperty( + 'previousRecordId', + ); + }); + + describe('available users', () => { + const jane = { id: 1, firstName: 'Jane', lastName: 'Doe', email: 'jane@forest.dev' }; + + test('announces the authors on the first page, with the filters and readable collections', async () => { + const { store, route } = setup([row(1)], [jane]); + const context = contextWith({ userIds: '1', operations: 'update' }); + + await route.handleTimeline(context); + + expect(store.listTimelineUsers).toHaveBeenCalledWith( + expect.objectContaining({ + collections: ['books', 'authors'], + userIds: [1], + operations: ['update'], + }), + ); + expect((context.response.body as { meta: object }).meta).toEqual({ + cursor: null, + availableUsers: [jane], + }); + }); + + test('omits the key on a later page rather than sending an empty list', async () => { + const { store, route } = setup([row(1)], [jane]); + const context = contextWith({ before: '2026-01-05T00:00:00.000Z' }); + + await route.handleTimeline(context); + + expect(store.listTimelineUsers).not.toHaveBeenCalled(); + expect((context.response.body as { meta: object }).meta).not.toHaveProperty('availableUsers'); + }); + + test('omits the key when the store cannot list the authors', async () => { + const { route } = setup([row(1)]); + const context = contextWith(); + + await route.handleTimeline(context); + + expect((context.response.body as { meta: object }).meta).not.toHaveProperty('availableUsers'); + }); + }); + describe('cursor', () => { test('returns no cursor when the page is the last one', async () => { const { route } = setup([row(2), row(1)]); diff --git a/packages/agent/test/routes/access/audit-trail.test.ts b/packages/agent/test/routes/access/audit-trail.test.ts index 0b3e4b61f7..bd737cdf4e 100644 --- a/packages/agent/test/routes/access/audit-trail.test.ts +++ b/packages/agent/test/routes/access/audit-trail.test.ts @@ -1628,6 +1628,65 @@ describe('AuditTrailRoute', () => { ]); }); + // What PRD-1321 buys back: with the id the row was filed under before the move recorded, the + // previous side is judged by that id instead of being withheld for want of an answer. + test('judges a moved key against the id the previous side carried', async () => { + const data = await historyUnder(new ConditionTreeLeaf('id', 'Equal', 2), [ + { + operation: 'update', + recordId: '9', + previousRecordId: '2', + previousValues: { id: REDACTED, secret: 'was in scope then' }, + newValues: { id: REDACTED, secret: 'out of scope now' }, + }, + ]); + + expect(data).toEqual([ + { + operation: 'update', + recordId: '9', + previousValues: { id: REDACTED, secret: 'was in scope then' }, + newValues: {}, + }, + ]); + }); + + test("never serves the id a move came from, which is the agent's own bookkeeping", async () => { + const data = await historyUnder(new ConditionTreeLeaf('id', 'Equal', 2), [ + { + operation: 'update', + recordId: '9', + previousRecordId: '2', + previousValues: { ownerId: 2 }, + newValues: { ownerId: 2 }, + }, + ]); + + expect(data[0]).not.toHaveProperty('previousRecordId'); + }); + + test("withholds a pending update's new side, which its id cannot speak for", async () => { + const data = await historyUnder(new ConditionTreeLeaf('id', 'Equal', 2), [ + { + operation: 'update', + recordId: '2', + status: 'pending', + previousValues: { id: 2, secret: 'before' }, + newValues: { id: REDACTED, secret: 'after' }, + }, + ]); + + expect(data).toEqual([ + { + operation: 'update', + recordId: '2', + status: 'pending', + previousValues: { id: 2, secret: 'before' }, + newValues: {}, + }, + ]); + }); + test('withholds the values when the scope names an inherited property of the snapshot', async () => { const data = await historyUnder(new ConditionTreeLeaf('toString', 'NotEqual', 'private'), [ { operation: 'delete', recordId: '2', previousValues: { ownerId: 1, secret: 'shh' } },