feat: record admin changes in the audit log and offer only the filters it writes

.agents/docs/architecture/auth-and-permissions.md

@@ -207,10 +207,18 @@ add it to the `$GroupScopableSubjectName` exclusion.
 
 ## Adjacent things that are not the permission system
 
-- **Audit logging is not automatic.** `AuditLogger` (`src/audit/audit.logger.ts`) is injected into
-  exactly two services and produces exactly three log calls: `CREATE`/`UPDATE` on `ASSIGNMENT`
-  (`src/assignments/assignments.service.ts`) and `LOGIN` on `USER` (`src/auth/auth.service.ts`).
-  Nothing else in the API writes an audit log. Do not assume a new mutation is recorded.
+- **Audit logging is not automatic.** A mutation is recorded only where it calls
+  `AuditLogger.log` (`src/audit/audit.logger.ts`), so do not assume a new one is. The pairs that are
+  written are `AUDITED_ACTIONS` in `packages/schemas/src/audit/audit.ts`: `log` is typed against it,
+  so a call naming an undeclared pair does not compile, and the web audit log page derives its
+  Entity and Action filters from it. The Prisma `AuditLogAction` and `AuditLogEntity` enums are
+  wider and do not say what is written. The log records what a signed-in user did, so an entry is
+  written where that user is known: in the controller when the service method is also called by
+  setup, demo seeding, repository sync or the gateway synchronizer, which act for no one
+  (`UsersController.create`). An update goes through `AuditLogger.logUpdate` with `changedFields`
+  (`src/audit/audit.utils.ts`), which records the names of the fields it changed and never their
+  values, because every admin reads the log; an update that changed nothing writes no entry.
+  Entries are written after the change, not in its transaction.
 - **`user.hashedPassword` is globally omitted** by the Prisma client constructed in
   `src/core/prisma.ts`. `UsersService.findByUsername` re-enables it via
   `{ includeHashedPassword: true }`, which is how login reads it. Individual `UsersService` methods

apps/api/src/assignments/__tests__/assignments.controller.test.ts

@@ -89,15 +89,11 @@ describe('AssignmentsController', () => {
   });
 
   describe('deleteBulk', () => {
-    it('should forward the requested ids with the caller ability, so the service can scope the deletion', async () => {
+    it('should forward the requested ids with the current user, so the service can scope and record the deletion', async () => {
       const result = { deletedCount: 2, failedIds: [] };
       assignmentsService.deleteBulk.mockResolvedValueOnce(result);
-      await expect(assignmentsController.deleteBulk({ ids: ['a-1', 'a-2'] }, currentUser.ability)).resolves.toBe(
-        result
-      );
-      expect(assignmentsService.deleteBulk).toHaveBeenCalledExactlyOnceWith(['a-1', 'a-2'], {
-        ability: currentUser.ability
-      });
+      await expect(assignmentsController.deleteBulk({ ids: ['a-1', 'a-2'] }, currentUser)).resolves.toBe(result);
+      expect(assignmentsService.deleteBulk).toHaveBeenCalledExactlyOnceWith(['a-1', 'a-2'], currentUser);
     });
   });
 

apps/api/src/assignments/__tests__/assignments.service.test.ts

@@ -11,7 +11,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest';
 
 import { AuditLogger } from '@/audit/audit.logger';
 import { AbilityFactory } from '@/auth/ability.factory';
-import { createAppAbility } from '@/auth/ability.utils';
+import { accessibleQuery, createAppAbility } from '@/auth/ability.utils';
 import { GatewayService } from '@/gateway/gateway.service';
 
 import { AssignmentsService } from '../assignments.service';
@@ -460,7 +460,7 @@ describe('AssignmentsService', () => {
         { id: 'a-2', status: 'OUTSTANDING' }
       ]);
       assignmentModel.delete.mockResolvedValue({});
-      const result = await assignmentsService.deleteBulk(['a-1', 'a-2'], { ability: permissiveUser().ability });
+      const result = await assignmentsService.deleteBulk(['a-1', 'a-2'], permissiveUser());
       expect(result).toEqual({ deletedCount: 2, failedIds: [] });
       expect(gatewayService.deleteRemoteAssignment).toHaveBeenCalledTimes(2);
       expect(assignmentModel.delete).toHaveBeenCalledTimes(2);
@@ -469,7 +469,7 @@ describe('AssignmentsService', () => {
     it('should skip the gateway call for expired assignments, since their links are already dead', async () => {
       assignmentModel.findMany.mockResolvedValueOnce([{ id: 'a-1', status: 'EXPIRED' }]);
       assignmentModel.delete.mockResolvedValue({});
-      const result = await assignmentsService.deleteBulk(['a-1'], { ability: permissiveUser().ability });
+      const result = await assignmentsService.deleteBulk(['a-1'], permissiveUser());
       expect(result).toEqual({ deletedCount: 1, failedIds: [] });
       expect(gatewayService.deleteRemoteAssignment).not.toHaveBeenCalled();
       expect(assignmentModel.delete).toHaveBeenCalledTimes(1);
@@ -482,14 +482,14 @@ describe('AssignmentsService', () => {
       ]);
       gatewayService.deleteRemoteAssignment.mockRejectedValueOnce(new Error('gateway down'));
       assignmentModel.delete.mockResolvedValue({});
-      const result = await assignmentsService.deleteBulk(['a-1', 'a-2'], { ability: permissiveUser().ability });
+      const result = await assignmentsService.deleteBulk(['a-1', 'a-2'], permissiveUser());
       expect(result.deletedCount).toBe(1);
       expect(result.failedIds).toEqual(['a-1']);
     });
 
     it('should only find assignments with OUTSTANDING or EXPIRED status', async () => {
       assignmentModel.findMany.mockResolvedValueOnce([]);
-      await assignmentsService.deleteBulk(['a-1'], { ability: permissiveUser().ability });
+      await assignmentsService.deleteBulk(['a-1'], permissiveUser());
       expect(assignmentModel.findMany.mock.lastCall?.[0]).toMatchObject({
         where: { id: { in: ['a-1'] }, status: { in: ['OUTSTANDING', 'EXPIRED'] } }
       });
@@ -499,17 +499,40 @@ describe('AssignmentsService', () => {
       const failure = new Error('gateway down');
       assignmentModel.findMany.mockResolvedValueOnce([{ id: 'a-1', status: 'OUTSTANDING' }]);
       gatewayService.deleteRemoteAssignment.mockRejectedValueOnce(failure);
-      await assignmentsService.deleteBulk(['a-1'], { ability: permissiveUser().ability });
+      await assignmentsService.deleteBulk(['a-1'], permissiveUser());
       expect(loggingService.error).toHaveBeenCalledExactlyOnceWith({
         error: failure,
         message: 'Failed to delete assignment a-1'
       });
     });
 
-    it('should apply no ability restriction for an internal caller that passes none', async () => {
+    it('should scope the assignments it deletes to the caller ability', async () => {
+      const user = userAt('GROUP_MANAGER');
       assignmentModel.findMany.mockResolvedValueOnce([]);
-      await assignmentsService.deleteBulk(['a-1']);
-      expect(assignmentModel.findMany.mock.lastCall?.[0]?.where?.AND).toStrictEqual([{}]);
+      await assignmentsService.deleteBulk(['a-1'], user);
+      expect(assignmentModel.findMany.mock.lastCall?.[0]?.where?.AND).toStrictEqual([
+        accessibleQuery(user.ability, 'delete', 'Assignment')
+      ]);
+    });
+
+    it('should record each deleted assignment in its own group, so the group filter finds it', async () => {
+      assignmentModel.findMany.mockResolvedValueOnce([
+        { groupId: GROUP_ID, id: 'a-1', status: 'EXPIRED' },
+        { groupId: null, id: 'a-2', status: 'EXPIRED' }
+      ]);
+      assignmentModel.delete.mockResolvedValue({});
+      await assignmentsService.deleteBulk(['a-1', 'a-2'], permissiveUser());
+      expect(auditLogger.log.mock.calls).toEqual([
+        ['DELETE', 'ASSIGNMENT', { groupId: GROUP_ID, metadata: { assignmentId: 'a-1' }, userId: 'user-1' }],
+        ['DELETE', 'ASSIGNMENT', { groupId: null, metadata: { assignmentId: 'a-2' }, userId: 'user-1' }]
+      ]);
+    });
+
+    it('should record no deletion for an assignment that failed to delete', async () => {
+      assignmentModel.findMany.mockResolvedValueOnce([{ groupId: GROUP_ID, id: 'a-1', status: 'OUTSTANDING' }]);
+      gatewayService.deleteRemoteAssignment.mockRejectedValueOnce(new Error('gateway down'));
+      await assignmentsService.deleteBulk(['a-1'], permissiveUser());
+      expect(auditLogger.log).not.toHaveBeenCalled();
     });
   });
 

apps/api/src/assignments/assignments.controller.ts

@@ -91,9 +91,9 @@ export class AssignmentsController {
   @RouteAccess({ action: 'delete', subject: 'Assignment' })
   deleteBulk(
     @Body() data: $DeleteBulkAssignmentsData,
-    @CurrentUser('ability') ability?: AppAbility
+    @CurrentUser() currentUser: RequestUser
   ): Promise<DeleteBulkAssignmentsResult> {
-    return this.assignmentsService.deleteBulk(data.ids, { ability });
+    return this.assignmentsService.deleteBulk(data.ids, currentUser);
   }
 
   @ApiOperation({ summary: 'Get All Assignments' })

apps/api/src/assignments/assignments.service.ts

@@ -153,16 +153,16 @@ export class AssignmentsService {
     return staged.map(({ assignment }) => assignment);
   }
 
-  async deleteBulk(ids: string[], { ability }: EntityOperationOptions = {}) {
+  async deleteBulk(ids: string[], currentUser: RequestUser) {
     const assignments = await this.assignmentModel.findMany({
       where: {
-        AND: [accessibleQuery(ability, 'delete', 'Assignment')],
+        AND: [accessibleQuery(currentUser.ability, 'delete', 'Assignment')],
         id: { in: ids },
         status: { in: ['OUTSTANDING', 'EXPIRED'] }
       }
     });
 
-    const deletedIds: string[] = [];
+    const deleted: typeof assignments = [];
     const failedIds: string[] = [];
 
     for (const assignment of assignments) {
@@ -171,7 +171,7 @@ export class AssignmentsService {
           await this.gatewayService.deleteRemoteAssignment(assignment.id);
         }
         await this.assignmentModel.delete({ where: { id: assignment.id } });
-        deletedIds.push(assignment.id);
+        deleted.push(assignment);
       } catch (err) {
         this.loggingService.error({
           error: err,
@@ -181,7 +181,16 @@ export class AssignmentsService {
       }
     }
 
-    return { deletedCount: deletedIds.length, failedIds };
+    // One entry per assignment, as for an emailed link, so the log says which ones were deleted.
+    for (const { groupId, id } of deleted) {
+      await this.auditLogger.log('DELETE', 'ASSIGNMENT', {
+        groupId: groupId ?? null,
+        metadata: { assignmentId: id },
+        userId: currentUser.id
+      });
+    }
+
+    return { deletedCount: deleted.length, failedIds };
   }
 
   async find(

apps/api/src/audit/__tests__/audit.logger.test.ts

@@ -46,10 +46,34 @@ describe('AuditLogger', () => {
       expect(auditLogModel.create.mock.lastCall?.[0]?.data.metadata).toStrictEqual({ mode: 'BULK' });
     });
 
+    it('should refuse at compile time a pair AUDITED_ACTIONS does not declare, so the filters cannot miss it', async () => {
+      // @ts-expect-error -- nothing sends an email about a user, so the pair is not declared
+      await auditLogger.log('SEND_EMAIL', 'USER', { groupId: null, userId: 'user-1' });
+      expect(auditLogModel.create).toHaveBeenCalledOnce();
+    });
+
     it('should timestamp the entry when it is written, so the log can be ordered by time', async () => {
       const before = Date.now();
       await auditLogger.log('LOGIN', 'USER', { groupId: null, userId: 'user-1' });
       expect(auditLogModel.create.mock.lastCall?.[0]?.data.timestamp).toBeGreaterThanOrEqual(before);
     });
   });
+
+  describe('logUpdate', () => {
+    it('should record the names of the changed fields, sorted, beside the metadata it is given', async () => {
+      await auditLogger.logUpdate('USER', ['lastName', 'email'], {
+        groupId: null,
+        metadata: { targetUserId: 'user-2' },
+        userId: 'user-1'
+      });
+      expect(auditLogModel.create.mock.lastCall?.[0]).toMatchObject({
+        data: { action: 'UPDATE', entity: 'USER', metadata: { fields: 'email,lastName', targetUserId: 'user-2' } }
+      });
+    });
+
+    it('should record nothing for an update that changed nothing, so a resubmitted form adds no entry', async () => {
+      await auditLogger.logUpdate('USER', [], { groupId: null, userId: 'user-1' });
+      expect(auditLogModel.create).not.toHaveBeenCalled();
+    });
+  });
 });

apps/api/src/audit/__tests__/audit.utils.test.ts

@@ -0,0 +1,52 @@
+import { describe, expect, it } from 'vitest';
+
+import { changedFields } from '../audit.utils';
+
+describe('changedFields', () => {
+  it('should name only the fields whose value changed, so a resubmitted form reports what was edited', () => {
+    expect(changedFields({ firstName: 'Jane', lastName: 'Doe' }, { firstName: 'Janet', lastName: 'Doe' })).toEqual([
+      'firstName'
+    ]);
+  });
+
+  it('should name a field the update added or removed', () => {
+    expect(changedFields({ email: 'jane@example.org' }, { phoneNumber: '5555555' })).toEqual(['email', 'phoneNumber']);
+  });
+
+  it('should sort the names, so two entries for the same edit read the same', () => {
+    expect(changedFields({ zip: '1' }, { city: 'Montreal' })).toEqual(['city', 'zip']);
+  });
+
+  it('should compare nested values by content, so a rewritten but equal object is not a change', () => {
+    expect(changedFields({ settings: { minimumAge: 18 } }, { settings: { minimumAge: 18 } })).toEqual([]);
+  });
+
+  it('should compare dates by their time, so a re-read date is not a change', () => {
+    expect(changedFields({ dateOfBirth: new Date(0) }, { dateOfBirth: new Date(0) })).toEqual([]);
+  });
+
+  it('should treat a reordered list of ids as unchanged, since the web resubmits them in its own order', () => {
+    expect(changedFields({ groupIds: ['a', 'b'] }, { groupIds: ['b', 'a'] })).toEqual([]);
+  });
+
+  it('should name a list of ids that gained or lost one', () => {
+    expect(changedFields({ groupIds: ['a', 'b'] }, { groupIds: ['a'] })).toEqual(['groupIds']);
+  });
+
+  it('should keep the order of a list of objects significant', () => {
+    expect(changedFields({ rules: [{ id: 1 }, { id: 2 }] }, { rules: [{ id: 2 }, { id: 1 }] })).toEqual(['rules']);
+  });
+
+  it('should leave out updatedAt, which every write changes', () => {
+    expect(changedFields({ updatedAt: new Date(0) }, { updatedAt: new Date(1) })).toEqual([]);
+  });
+
+  it('should name the indices of a list that changed, so array-shaped record data is described too', () => {
+    expect(changedFields([{ score: 1 }, { score: 2 }], [{ score: 1 }, { score: 3 }])).toEqual(['1']);
+  });
+
+  it.each([null, 'text', 7])('should refuse to compare %j, which has no fields to name', (value) => {
+    expect(() => changedFields(value, {})).toThrow(TypeError);
+    expect(() => changedFields({}, value)).toThrow(TypeError);
+  });
+});

apps/api/src/audit/audit.logger.ts

@@ -1,16 +1,22 @@
 import { InjectModel } from '@douglasneuroinformatics/libnest';
 import type { Model } from '@douglasneuroinformatics/libnest';
 import { Injectable } from '@nestjs/common';
-import type { $AuditLogAction, $AuditLogEntity } from '@opendatacapture/schemas/audit';
+import type { AuditedAction, AuditedEntity } from '@opendatacapture/schemas/audit';
+
+type AuditLogParams = { groupId: null | string; metadata?: { [key: string]: string }; userId: string };
+
+type UpdateAuditedEntity = {
+  [TEntity in AuditedEntity]: 'UPDATE' extends AuditedAction<TEntity> ? TEntity : never;
+}[AuditedEntity];
 
 @Injectable()
 export class AuditLogger {
   constructor(@InjectModel('AuditLog') private readonly auditLogModel: Model<'AuditLog'>) {}
 
-  async log(
-    action: $AuditLogAction,
-    entity: $AuditLogEntity,
-    params: { groupId: null | string; metadata?: { [key: string]: string }; userId: string }
+  async log<TEntity extends AuditedEntity>(
+    action: AuditedAction<TEntity>,
+    entity: TEntity,
+    params: AuditLogParams
   ): Promise<void> {
     await this.auditLogModel.create({
       data: {
@@ -27,4 +33,18 @@ export class AuditLogger {
       }
     });
   }
+
+  /**
+   * Record an update by the names of the fields it changed, never their values: every admin reads
+   * the log, and a record's values are clinical data. An update that changed nothing is not recorded.
+   */
+  async logUpdate(entity: UpdateAuditedEntity, fields: string[], params: AuditLogParams): Promise<void> {
+    if (fields.length === 0) {
+      return;
+    }
+    await this.log('UPDATE', entity, {
+      ...params,
+      metadata: { ...params.metadata, fields: fields.toSorted().join(',') }
+    });
+  }
 }

apps/api/src/audit/audit.utils.ts

@@ -0,0 +1,35 @@
+import { isEqual, isEqualWith } from 'lodash-es';
+
+const isStringList = (value: unknown): value is string[] => {
+  return Array.isArray(value) && value.every((item) => typeof item === 'string');
+};
+
+/** A list of strings is a list of ids here, and one that is only reordered has not changed. */
+const compareStringListsAsSets = (value: unknown, other: unknown): boolean | undefined => {
+  if (isStringList(value) && isStringList(other)) {
+    return isEqual(value.toSorted(), other.toSorted());
+  }
+  return undefined;
+};
+
+const fieldValues = (value: unknown): Map<string, unknown> => {
+  if (typeof value !== 'object' || value === null) {
+    throw new TypeError(`Cannot name the changed fields of ${String(value)}, which has no fields`);
+  }
+  return new Map(Object.entries(value));
+};
+
+/**
+ * The names of the fields that differ between a row, or a record's data, before and after an
+ * update, sorted.
+ */
+export function changedFields(before: unknown, after: unknown): string[] {
+  const beforeValues = fieldValues(before);
+  const afterValues = fieldValues(after);
+  const fields = new Set([...beforeValues.keys(), ...afterValues.keys()]);
+  // Every write sets it, so it would name every update as a change.
+  fields.delete('updatedAt');
+  return [...fields]
+    .filter((field) => !isEqualWith(beforeValues.get(field), afterValues.get(field), compareStringListsAsSets))
+    .sort();
+}