From 76a21f3b5e7934786470052ebbb06b25e88c2f74 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Sun, 8 Nov 2026 20:12:54 -0500 Subject: [PATCH 01/10] refactor(schemas): declare the entity and action pairs the audit log records AUDITED_ACTIONS is the one list of pairs the API writes, so the logger and the audit log filters can be derived from it instead of from the two enums, which also hold values nothing writes. Co-Authored-By: Claude Opus 5.5 --- .../schemas/src/audit/__tests__/audit.test.ts | 11 ++++++++++- packages/schemas/src/audit/audit.ts | 15 +++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/packages/schemas/src/audit/__tests__/audit.test.ts b/packages/schemas/src/audit/__tests__/audit.test.ts index b068881c4..5d7928ff2 100644 --- a/packages/schemas/src/audit/__tests__/audit.test.ts +++ b/packages/schemas/src/audit/__tests__/audit.test.ts @@ -1,6 +1,15 @@ import { describe, expect, it } from 'vitest'; -import { $AuditLogsQueryParams, AUDIT_LOGS_MAX_PAGE_SIZE, AUDIT_LOGS_PAGE_SIZE } from '../audit.js'; +import { $AuditLogsQueryParams, AUDIT_LOGS_MAX_PAGE_SIZE, AUDIT_LOGS_PAGE_SIZE, AUDITED_ACTIONS } from '../audit.js'; + +describe('AUDITED_ACTIONS', () => { + it.each(Object.entries(AUDITED_ACTIONS))( + 'should declare each action written on %s once, so a filter menu cannot offer it twice', + (_entity, actions) => { + expect(new Set(actions).size).toBe(actions.length); + } + ); +}); describe('$AuditLogsQueryParams', () => { it('should read the page and limit from their query string form', () => { diff --git a/packages/schemas/src/audit/audit.ts b/packages/schemas/src/audit/audit.ts index fc3e9054e..25bcd561d 100644 --- a/packages/schemas/src/audit/audit.ts +++ b/packages/schemas/src/audit/audit.ts @@ -18,6 +18,21 @@ export const $AuditLogEntity = z.enum([ 'USER' ]); +export type AuditedEntity = keyof typeof AUDITED_ACTIONS; + +export type AuditedAction = (typeof AUDITED_ACTIONS)[TEntity][number]; + +/** + * The (entity, action) pairs the API writes. `AuditLogger.log` accepts no other pair, and the audit + * log page offers no other filter. The two enums above are wider, because they also parse what the + * stored rows hold. + */ +export const AUDITED_ACTIONS = { + ASSIGNMENT: ['CREATE', 'SEND_EMAIL', 'UPDATE'], + INSTRUMENT: ['ARCHIVE', 'UNARCHIVE'], + USER: ['LOGIN', 'UPDATE'] +} as const satisfies { [TEntity in $AuditLogEntity]?: readonly $AuditLogAction[] }; + /** The number of audit logs shown per page, matching the page size used by the client's data table. */ export const AUDIT_LOGS_PAGE_SIZE = 10; From 37db3c5025619715541a029bd3fff3223ce81128 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Sun, 8 Nov 2026 20:19:34 -0500 Subject: [PATCH 02/10] refactor(api): type AuditLogger.log against the declared audit pairs A log call whose entity and action are not in AUDITED_ACTIONS no longer compiles, so a new event cannot be written without the audit log filters learning about it. Co-Authored-By: Claude Opus 5.5 --- apps/api/src/audit/__tests__/audit.logger.test.ts | 6 ++++++ apps/api/src/audit/audit.logger.ts | 8 ++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/apps/api/src/audit/__tests__/audit.logger.test.ts b/apps/api/src/audit/__tests__/audit.logger.test.ts index 2e33768d5..619c2181c 100644 --- a/apps/api/src/audit/__tests__/audit.logger.test.ts +++ b/apps/api/src/audit/__tests__/audit.logger.test.ts @@ -46,6 +46,12 @@ 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' }); diff --git a/apps/api/src/audit/audit.logger.ts b/apps/api/src/audit/audit.logger.ts index 6c02030ff..5909eab25 100644 --- a/apps/api/src/audit/audit.logger.ts +++ b/apps/api/src/audit/audit.logger.ts @@ -1,15 +1,15 @@ 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'; @Injectable() export class AuditLogger { constructor(@InjectModel('AuditLog') private readonly auditLogModel: Model<'AuditLog'>) {} - async log( - action: $AuditLogAction, - entity: $AuditLogEntity, + async log( + action: AuditedAction, + entity: TEntity, params: { groupId: null | string; metadata?: { [key: string]: string }; userId: string } ): Promise { await this.auditLogModel.create({ From 14fdb1878054ff140e4909dbf05ecf70b8b39252 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Sun, 8 Nov 2026 20:21:21 -0500 Subject: [PATCH 03/10] fix(web): offer only the audit log filters something writes The Entity and Action menus are derived from AUDITED_ACTIONS instead of listing every enum value, and each is narrowed to what the other's choice allows, so no filter combination can match an entry nothing ever writes. Session no longer appears, since nothing logs it. Co-Authored-By: Claude Opus 5.5 --- .../_app/admin/audit/__tests__/logs.test.tsx | 44 ++++++++++++++----- apps/web/src/routes/_app/admin/audit/logs.tsx | 33 +++++++------- .../src/pages/_app/admin/audit/logs.page.ts | 7 ++- testing/src/specs/admin-audit-logs.spec.ts | 23 ++++++++-- 4 files changed, 77 insertions(+), 30 deletions(-) diff --git a/apps/web/src/routes/_app/admin/audit/__tests__/logs.test.tsx b/apps/web/src/routes/_app/admin/audit/__tests__/logs.test.tsx index 293be35ee..68592904f 100644 --- a/apps/web/src/routes/_app/admin/audit/__tests__/logs.test.tsx +++ b/apps/web/src/routes/_app/admin/audit/__tests__/logs.test.tsx @@ -42,6 +42,10 @@ const mocks = vi.hoisted(() => ({ users: [] as User[] })); +vi.mock('@opendatacapture/schemas/audit', async (importOriginal) => ({ + ...(await importOriginal()), + AUDITED_ACTIONS: { ASSIGNMENT: ['CREATE', 'SEND_EMAIL'], USER: ['LOGIN', 'UPDATE'] } +})); vi.mock('@/hooks/useSetupStateQuery', () => ({ useSetupStateQuery: () => ({ data: { tableColorMode: 'tags' } }) })); vi.mock('@tanstack/react-router', async (importOriginal) => ({ ...(await importOriginal()), @@ -115,7 +119,7 @@ const userFixture = (id: string, username: string, groupIds: string[]): User => const logFixture = (overrides: Partial<$AuditLog> = {}): $AuditLog => ({ action: 'SEND_EMAIL', - entity: 'INSTRUMENT_RECORD', + entity: 'ASSIGNMENT', group: { name: 'Depression Clinic' }, id: 'log-1', timestamp: TIMESTAMP, @@ -277,30 +281,50 @@ describe('Route', () => { expect(navigatedSearch()).toEqual({ action: 'LOGIN', userId: 'user-2' }); }); - it('should offer every action under its translated name', () => { + it('should offer only the actions something writes, under their translated names', () => { renderPage(); expect(filterOptions('audit-logs-filter-action')).toEqual([ 'Any Action', 'Create', - 'Delete', 'Update', - 'Archive', - 'Unarchive', 'Login', 'Send Email' ]); }); + it('should offer only the actions written on the chosen entity, so no combination can match nothing', () => { + mocks.search = { entity: 'USER' }; + renderPage(); + expect(filterOptions('audit-logs-filter-action')).toEqual(['Any Action', 'Update', 'Login']); + }); + + it('should offer no action for an entity nothing writes, which an old link can still name', () => { + mocks.search = { entity: 'SESSION' }; + renderPage(); + expect(filterOptions('audit-logs-filter-action')).toEqual(['Any Action']); + }); + it('should filter by the chosen action', () => { renderPage(); chooseFilter('audit-logs-filter-action', 'Send Email'); expect(navigatedSearch()).toEqual({ action: 'SEND_EMAIL' }); }); + it('should offer only the entities something writes, so an entity that is never logged is not offered', () => { + renderPage(); + expect(filterOptions('audit-logs-filter-entity')).toEqual(['Any Entity', 'Assignment', 'User']); + }); + + it('should offer only the entities the chosen action is written on, so no combination can match nothing', () => { + mocks.search = { action: 'SEND_EMAIL' }; + renderPage(); + expect(filterOptions('audit-logs-filter-entity')).toEqual(['Any Entity', 'Assignment']); + }); + it('should filter by the chosen entity', () => { renderPage(); - chooseFilter('audit-logs-filter-entity', 'Instrument Record'); - expect(navigatedSearch()).toEqual({ entity: 'INSTRUMENT_RECORD' }); + chooseFilter('audit-logs-filter-entity', 'User'); + expect(navigatedSearch()).toEqual({ entity: 'USER' }); }); it('should hide the clear filters button while no filter is set', () => { @@ -343,7 +367,7 @@ describe('Route', () => { it('should show who acted, in which group, and the translated action and entity', () => { renderPage(); - expect(cellTexts().slice(1)).toEqual(['jane', 'Depression Clinic', 'Send Email', 'Instrument Record']); + expect(cellTexts().slice(1)).toEqual(['jane', 'Depression Clinic', 'Send Email', 'Assignment']); }); it('should tag the action with its color', () => { @@ -354,8 +378,8 @@ describe('Route', () => { it('should leave the entity as plain text', () => { renderPage(); - expect(screen.getByText('Instrument Record').querySelector('[data-testid="color-tag-dot"]')).toBeNull(); - expect(screen.getByText('Instrument Record').closest('[data-testid="color-tag-label"]')).toBeNull(); + expect(screen.getByText('Assignment').querySelector('[data-testid="color-tag-dot"]')).toBeNull(); + expect(screen.getByText('Assignment').closest('[data-testid="color-tag-label"]')).toBeNull(); }); it('should show N/A for a log with no user or group', () => { diff --git a/apps/web/src/routes/_app/admin/audit/logs.tsx b/apps/web/src/routes/_app/admin/audit/logs.tsx index de78ff129..5381c2c67 100644 --- a/apps/web/src/routes/_app/admin/audit/logs.tsx +++ b/apps/web/src/routes/_app/admin/audit/logs.tsx @@ -3,13 +3,13 @@ import React, { Fragment, useCallback, useMemo, useState } from 'react'; import { snakeToCamelCase, toLowerCase } from '@douglasneuroinformatics/libjs'; import { Button, DataTable, DropdownMenu, Heading, TanstackTable } from '@douglasneuroinformatics/libui/components'; import { useDownload, useTheme, useTranslation } from '@douglasneuroinformatics/libui/hooks'; -import { $AuditLogsQuerySearchParams } from '@opendatacapture/schemas/audit'; -import type { - $AuditLog, +import { $AuditLogAction, $AuditLogEntity, - $AuditLogsQuerySearchParams as AuditLogsSearchParams + $AuditLogsQuerySearchParams, + AUDITED_ACTIONS } from '@opendatacapture/schemas/audit'; +import type { $AuditLog, $AuditLogsQuerySearchParams as AuditLogsSearchParams } from '@opendatacapture/schemas/audit'; import { createFileRoute } from '@tanstack/react-router'; import { ArrowDownIcon, ArrowUpIcon, CalendarIcon, DownloadIcon, XIcon } from 'lucide-react'; @@ -30,17 +30,15 @@ const INITIAL_PAGE = 1; const INITIAL_SORT_ORDER: SortOrder = 'desc'; -const ACTIONS: $AuditLogAction[] = ['CREATE', 'DELETE', 'UPDATE', 'ARCHIVE', 'UNARCHIVE', 'LOGIN', 'SEND_EMAIL']; +const ACTIONS_BY_ENTITY: { readonly [TEntity in $AuditLogEntity]?: readonly $AuditLogAction[] } = AUDITED_ACTIONS; -const ENTITIES: $AuditLogEntity[] = [ - 'ASSIGNMENT', - 'GROUP', - 'INSTRUMENT', - 'INSTRUMENT_RECORD', - 'SESSION', - 'SUBJECT', - 'USER' -]; +const isAudited = (entity: $AuditLogEntity, action: $AuditLogAction) => { + return ACTIONS_BY_ENTITY[entity]?.includes(action) ?? false; +}; + +const ENTITIES = $AuditLogEntity.options.filter((entity) => ACTIONS_BY_ENTITY[entity] !== undefined).sort(); + +const ACTIONS = $AuditLogAction.options.filter((action) => ENTITIES.some((entity) => isAudited(entity, action))); // The sort direction is rendered from the value the rows were actually fetched with, rather than from // `column.getIsSorted()`: in server mode the table mounts with no sorting state of its own, so the @@ -116,6 +114,9 @@ const AuditLogsToolbar = () => { }; const availableUsers = search.groupId ? users.filter((user) => user.groupIds.includes(search.groupId!)) : users; + const { action: selectedAction, entity: selectedEntity } = search; + const availableActions = selectedEntity ? ACTIONS.filter((action) => isAudited(selectedEntity, action)) : ACTIONS; + const availableEntities = selectedAction ? ENTITIES.filter((entity) => isAudited(entity, selectedAction)) : ENTITIES; const hasFilters = Object.values(search).some((value) => value !== undefined); const localize = (value: $AuditLogAction | $AuditLogEntity) => t(`common.${snakeToCamelCase(toLowerCase(value))}`); @@ -146,7 +147,7 @@ const AuditLogsToolbar = () => { allLabel={t({ en: 'Any Action', es: 'Cualquier acción', fr: 'Toute action' })} data-testid="audit-logs-filter-action" label={t('common.action')} - options={ACTIONS.map((action) => ({ label: localize(action), value: action }))} + options={availableActions.map((action) => ({ label: localize(action), value: action }))} value={search.action} onValueChange={(action) => setSearch({ action })} /> @@ -154,7 +155,7 @@ const AuditLogsToolbar = () => { allLabel={t({ en: 'Any Entity', es: 'Cualquier entidad', fr: 'Toute entité' })} data-testid="audit-logs-filter-entity" label={t('common.entity')} - options={ENTITIES.map((entity) => ({ label: localize(entity), value: entity }))} + options={availableEntities.map((entity) => ({ label: localize(entity), value: entity }))} value={search.entity} onValueChange={(entity) => setSearch({ entity })} /> diff --git a/testing/src/pages/_app/admin/audit/logs.page.ts b/testing/src/pages/_app/admin/audit/logs.page.ts index 7ec80ab99..b48199799 100644 --- a/testing/src/pages/_app/admin/audit/logs.page.ts +++ b/testing/src/pages/_app/admin/audit/logs.page.ts @@ -29,6 +29,11 @@ export class AuditLogsPage extends AppPage { /** Opens one filter's menu and picks an option; the menu closes itself once the option is chosen. */ async filterBy(filter: AuditLogsFilter, optionLabel: string) { await this.filterButton(filter).click(); - await this.$ref.getByRole('menuitemradio', { exact: true, name: optionLabel }).click(); + await this.filterOption(optionLabel).click(); + } + + /** An option of whichever filter menu is open. */ + filterOption(optionLabel: string) { + return this.$ref.getByRole('menuitemradio', { exact: true, name: optionLabel }); } } diff --git a/testing/src/specs/admin-audit-logs.spec.ts b/testing/src/specs/admin-audit-logs.spec.ts index f93ce7dce..501706c2c 100644 --- a/testing/src/specs/admin-audit-logs.spec.ts +++ b/testing/src/specs/admin-audit-logs.spec.ts @@ -32,10 +32,27 @@ test.describe('admin audit logs', () => { test('should show the chosen option on its filter button and put it in the URL', async ({ getPageModel }) => { const auditLogsPage = await getPageModel('/admin/audit/logs'); - await auditLogsPage.filterBy('entity', 'Session'); + await auditLogsPage.filterBy('entity', 'Instrument'); - await expect(auditLogsPage.filterButton('entity')).toContainText('Session'); - await expect(auditLogsPage.$ref).toHaveURL(/entity=SESSION/); + await expect(auditLogsPage.filterButton('entity')).toContainText('Instrument'); + await expect(auditLogsPage.$ref).toHaveURL(/entity=INSTRUMENT(&|$)/); + }); + + test('should not offer an entity nothing writes to the log', async ({ getPageModel }) => { + const auditLogsPage = await getPageModel('/admin/audit/logs'); + await auditLogsPage.filterButton('entity').click(); + + await expect(auditLogsPage.filterOption('User')).toBeVisible(); + await expect(auditLogsPage.filterOption('Session')).toBeHidden(); + }); + + test('should offer only the actions written on the chosen entity', async ({ getPageModel }) => { + const auditLogsPage = await getPageModel('/admin/audit/logs'); + await auditLogsPage.filterBy('entity', 'Assignment'); + await auditLogsPage.filterButton('action').click(); + + await expect(auditLogsPage.filterOption('Send Email')).toBeVisible(); + await expect(auditLogsPage.filterOption('Login')).toBeHidden(); }); test('should clear every filter at once', async ({ getPageModel }) => { From 6f1db10adb7a527454494a96c7f40e587e56b9f2 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Sun, 8 Nov 2026 20:21:53 -0500 Subject: [PATCH 04/10] docs: point the audit logging notes at AUDITED_ACTIONS The architecture doc and the audit log spec both listed which calls write an entry, and both had gone stale. They now name the map instead. Co-Authored-By: Claude Opus 5.5 --- .agents/docs/architecture/auth-and-permissions.md | 10 ++++++---- testing/src/specs/admin-audit-logs.spec.ts | 6 ++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/.agents/docs/architecture/auth-and-permissions.md b/.agents/docs/architecture/auth-and-permissions.md index 72ebd8abf..8bac0b289 100644 --- a/.agents/docs/architecture/auth-and-permissions.md +++ b/.agents/docs/architecture/auth-and-permissions.md @@ -207,10 +207,12 @@ 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. - **`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 diff --git a/testing/src/specs/admin-audit-logs.spec.ts b/testing/src/specs/admin-audit-logs.spec.ts index 501706c2c..d5f12fffc 100644 --- a/testing/src/specs/admin-audit-logs.spec.ts +++ b/testing/src/specs/admin-audit-logs.spec.ts @@ -12,10 +12,8 @@ test.describe('admin audit logs', () => { await expect(auditLogsPage.dataTable).toBeVisible(); }); - // Only `Login` (via `auth.service.ts`) and assignment actions (via `assignments.service.ts`) - // actually call `AuditLogger.log`; creating a group, user, instrument, subject etc. is not audited - // despite the Entity filter offering those options -- see #1473. Login is therefore the - // only auditable action this spec can reliably trigger and verify. + // `AUDITED_ACTIONS` (`packages/schemas/src/audit/audit.ts`) lists every pair the API writes, and + // the filters below offer exactly those. test('should record a login and find it through the action filter', async ({ apiRequestContext, getPageModel }) => { // `getPageModel('ADMIN')` reuses a token cached once per worker (see `roleAccount` in fixtures.ts), // so it would not itself produce a fresh entry here. Logging in directly does. From aa7f1a55ab5d01ba37965db979820c8fb20cc86e Mon Sep 17 00:00:00 2001 From: joshunrau Date: Sun, 8 Nov 2026 20:30:57 -0500 Subject: [PATCH 05/10] feat(api): record user creation, updates and permission changes in the audit log An admin creating a user, editing one, or replacing their permissions, and a user editing their own account, now each write an entry. An update names the fields it changed and never their values, and one that changed nothing writes no entry. Archiving and unarchiving a user are recorded as ARCHIVE and UNARCHIVE instead of UPDATE, as they are for instruments. User creation is logged in the controller, because setup and demo seeding call the same service method with no one acting. Co-Authored-By: Claude Opus 5.5 --- .../docs/architecture/auth-and-permissions.md | 8 +- .../src/audit/__tests__/audit.logger.test.ts | 18 +++ .../src/audit/__tests__/audit.utils.test.ts | 47 +++++++ apps/api/src/audit/audit.logger.ts | 22 ++- apps/api/src/audit/audit.utils.ts | 25 ++++ .../users/__tests__/users.controller.test.ts | 61 +++++--- .../src/users/__tests__/users.service.test.ts | 133 ++++++++++++++---- apps/api/src/users/users.controller.ts | 15 +- apps/api/src/users/users.service.ts | 92 +++++++----- packages/schemas/src/audit/audit.ts | 2 +- testing/src/specs/admin-audit-logs.spec.ts | 62 ++++++++ testing/src/specs/mail.spec.ts | 2 +- testing/src/support/api-client.ts | 39 ++++- 13 files changed, 437 insertions(+), 89 deletions(-) create mode 100644 apps/api/src/audit/__tests__/audit.utils.test.ts create mode 100644 apps/api/src/audit/audit.utils.ts diff --git a/.agents/docs/architecture/auth-and-permissions.md b/.agents/docs/architecture/auth-and-permissions.md index 8bac0b289..c24717cfb 100644 --- a/.agents/docs/architecture/auth-and-permissions.md +++ b/.agents/docs/architecture/auth-and-permissions.md @@ -212,7 +212,13 @@ add it to the `$GroupScopableSubjectName` exclusion. 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. + 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 diff --git a/apps/api/src/audit/__tests__/audit.logger.test.ts b/apps/api/src/audit/__tests__/audit.logger.test.ts index 619c2181c..0cf97ea79 100644 --- a/apps/api/src/audit/__tests__/audit.logger.test.ts +++ b/apps/api/src/audit/__tests__/audit.logger.test.ts @@ -58,4 +58,22 @@ describe('AuditLogger', () => { 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(); + }); + }); }); diff --git a/apps/api/src/audit/__tests__/audit.utils.test.ts b/apps/api/src/audit/__tests__/audit.utils.test.ts new file mode 100644 index 000000000..9f0ca3ac4 --- /dev/null +++ b/apps/api/src/audit/__tests__/audit.utils.test.ts @@ -0,0 +1,47 @@ +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']); + }); +}); diff --git a/apps/api/src/audit/audit.logger.ts b/apps/api/src/audit/audit.logger.ts index 5909eab25..450313e77 100644 --- a/apps/api/src/audit/audit.logger.ts +++ b/apps/api/src/audit/audit.logger.ts @@ -3,6 +3,12 @@ import type { Model } from '@douglasneuroinformatics/libnest'; import { Injectable } from '@nestjs/common'; 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 : never; +}[AuditedEntity]; + @Injectable() export class AuditLogger { constructor(@InjectModel('AuditLog') private readonly auditLogModel: Model<'AuditLog'>) {} @@ -10,7 +16,7 @@ export class AuditLogger { async log( action: AuditedAction, entity: TEntity, - params: { groupId: null | string; metadata?: { [key: string]: string }; userId: string } + params: AuditLogParams ): Promise { 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 { + if (fields.length === 0) { + return; + } + await this.log('UPDATE', entity, { + ...params, + metadata: { ...params.metadata, fields: fields.toSorted().join(',') } + }); + } } diff --git a/apps/api/src/audit/audit.utils.ts b/apps/api/src/audit/audit.utils.ts new file mode 100644 index 000000000..1781c8ce7 --- /dev/null +++ b/apps/api/src/audit/audit.utils.ts @@ -0,0 +1,25 @@ +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; +}; + +/** The names of the fields that differ between a row before and after an update, sorted. */ +export function changedFields(before: object, after: object): string[] { + const beforeValues = new Map(Object.entries(before)); + const afterValues = new Map(Object.entries(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(); +} diff --git a/apps/api/src/users/__tests__/users.controller.test.ts b/apps/api/src/users/__tests__/users.controller.test.ts index 4cbcf66ba..765f32772 100644 --- a/apps/api/src/users/__tests__/users.controller.test.ts +++ b/apps/api/src/users/__tests__/users.controller.test.ts @@ -10,6 +10,7 @@ import type { Group } from '@opendatacapture/schemas/group'; import type { BasePermissionLevel } from '@opendatacapture/schemas/user'; import { beforeEach, describe, expect, it } from 'vitest'; +import { AuditLogger } from '@/audit/audit.logger'; import { AbilityFactory } from '@/auth/ability.factory'; import { createAppAbility } from '@/auth/ability.utils'; import { ROUTE_ACCESS_METADATA_KEY } from '@/core/decorators/route-access.decorator'; @@ -48,6 +49,7 @@ const createUserData = { describe('UsersController', () => { let usersController: UsersController; let usersService: MockedInstance; + let auditLogger: MockedInstance; let groupsService: MockedInstance; let mailService: MockedInstance; @@ -56,12 +58,14 @@ describe('UsersController', () => { controllers: [UsersController], providers: [ MockFactory.createForService(UsersService), + MockFactory.createForService(AuditLogger), MockFactory.createForService(GroupsService), MockFactory.createForService(MailService) ] }).compile(); usersController = moduleRef.get(UsersController); usersService = moduleRef.get(UsersService); + auditLogger = moduleRef.get(AuditLogger); groupsService = moduleRef.get(GroupsService); mailService = moduleRef.get(MailService); }); @@ -72,7 +76,7 @@ describe('UsersController', () => { describe('create', () => { beforeEach(() => { - usersService.create.mockResolvedValue({ id: 'user-1', username: createUserData.username }); + usersService.create.mockResolvedValue({ id: 'user-2', username: createUserData.username }); groupsService.findById.mockImplementation((id: string, _options?: unknown) => { return Promise.resolve({ id, name: `Name of ${id}` }); }); @@ -84,14 +88,14 @@ describe('UsersController', () => { }); it('returns the created user together with the welcome email result', async () => { - await expect(usersController.create(createUserData, ability)).resolves.toMatchObject({ - id: 'user-1', + await expect(usersController.create(createUserData, currentUser)).resolves.toMatchObject({ + id: 'user-2', welcomeEmail: { status: 'SENT' } }); }); it('sends the welcome email with the login url derived from the request origin', async () => { - await usersController.create(createUserData, ability, 'https://odc.example.org'); + await usersController.create(createUserData, currentUser, 'https://odc.example.org'); expect(mailService.sendNewUserEmail.mock.lastCall?.[0]).toMatchObject({ email: createUserData.email, url: 'https://odc.example.org/auth/login', @@ -100,25 +104,25 @@ describe('UsersController', () => { }); it('never hands the password to the welcome email', async () => { - await usersController.create(createUserData, ability, 'https://odc.example.org'); + await usersController.create(createUserData, currentUser, 'https://odc.example.org'); expect(mailService.sendNewUserEmail.mock.lastCall?.[0]).not.toHaveProperty('password'); }); it('sends an empty url when the origin header is absent', async () => { - await usersController.create(createUserData, ability); + await usersController.create(createUserData, currentUser); expect(mailService.sendNewUserEmail.mock.lastCall?.[0]).toMatchObject({ url: '' }); }); // The controller forwards whatever was asked for and leaves the default to the mail service. it('passes the language query param through to sendNewUserEmail', async () => { - await usersController.create(createUserData, ability); + await usersController.create(createUserData, currentUser); expect(mailService.sendNewUserEmail.mock.lastCall?.[0]).toMatchObject({ language: undefined }); - await usersController.create(createUserData, ability, undefined, 'fr'); + await usersController.create(createUserData, currentUser, undefined, 'fr'); expect(mailService.sendNewUserEmail.mock.lastCall?.[0]).toMatchObject({ language: 'fr' }); }); it('joins the names of the resolved groups', async () => { - await usersController.create(createUserData, ability); + await usersController.create(createUserData, currentUser); expect(mailService.sendNewUserEmail.mock.lastCall?.[0]).toMatchObject({ group: 'Name of group-1, Name of group-2' }); @@ -127,7 +131,7 @@ describe('UsersController', () => { // Without this, dropping `{ ability }` from the service calls would leave every other test // green while the lookups silently read across every group. it('scopes both the create and the group lookups to the caller ability', async () => { - await usersController.create(createUserData, ability); + await usersController.create(createUserData, currentUser); expect(usersService.create).toHaveBeenCalledWith(createUserData, { ability }); expect(groupsService.findById).toHaveBeenCalledWith('group-1', { ability }); expect(groupsService.findById).toHaveBeenCalledWith('group-2', { ability }); @@ -139,18 +143,33 @@ describe('UsersController', () => { ? Promise.reject(new NotFoundException()) : Promise.resolve({ id, name: `Name of ${id}` }); }); - await usersController.create(createUserData, ability); + await usersController.create(createUserData, currentUser); expect(mailService.sendNewUserEmail.mock.lastCall?.[0]).toMatchObject({ group: 'Name of group-2' }); }); it('propagates a group lookup failure that is not a permission denial', async () => { groupsService.findById.mockRejectedValue(new Error('connection lost')); - await expect(usersController.create(createUserData, ability)).rejects.toThrow('connection lost'); + await expect(usersController.create(createUserData, currentUser)).rejects.toThrow('connection lost'); expect(mailService.sendNewUserEmail).not.toHaveBeenCalled(); }); + it('records who created the user, naming the new account, since the service records nothing', async () => { + await usersController.create(createUserData, currentUser); + expect(auditLogger.log).toHaveBeenCalledWith('CREATE', 'USER', { + groupId: null, + metadata: { targetUserId: 'user-2' }, + userId: currentUser.id + }); + }); + + it('records no creation when the service refuses it', async () => { + usersService.create.mockRejectedValue(new Error('conflict')); + await expect(usersController.create(createUserData, currentUser)).rejects.toThrow('conflict'); + expect(auditLogger.log).not.toHaveBeenCalled(); + }); + it('passes an empty group name when the user has no groups', async () => { - await usersController.create({ ...createUserData, groupIds: [] }, ability); + await usersController.create({ ...createUserData, groupIds: [] }, currentUser); expect(groupsService.findById).not.toHaveBeenCalled(); expect(mailService.sendNewUserEmail.mock.lastCall?.[0]).toMatchObject({ group: '' }); }); @@ -219,22 +238,26 @@ describe('UsersController', () => { }); describe('updatePermissions', () => { - it('should forward custom grants with the caller ability, so the write is scoped', async () => { + it('should forward custom grants with the current user, so the write is scoped and attributed', async () => { const permissions: Permissions = [{ action: 'read', groupId: 'group-1', subject: 'Subject' }]; - await usersController.updatePermissions('user-1', { kind: 'custom', permissions }, ability); + await usersController.updatePermissions('user-1', { kind: 'custom', permissions }, currentUser); expect(usersService.updatePermissions).toHaveBeenCalledWith( 'user-1', { kind: 'custom', permissions }, - { ability } + currentUser ); }); - it('should forward a permission set with the caller ability, so the write is scoped', async () => { - await usersController.updatePermissions('user-1', { kind: 'permissionSet', permissionSetId: 'set-1' }, ability); + it('should forward a permission set with the current user, so the write is scoped and attributed', async () => { + await usersController.updatePermissions( + 'user-1', + { kind: 'permissionSet', permissionSetId: 'set-1' }, + currentUser + ); expect(usersService.updatePermissions).toHaveBeenCalledWith( 'user-1', { kind: 'permissionSet', permissionSetId: 'set-1' }, - { ability } + currentUser ); }); }); diff --git a/apps/api/src/users/__tests__/users.service.test.ts b/apps/api/src/users/__tests__/users.service.test.ts index 7ab09402d..2fb30bb82 100644 --- a/apps/api/src/users/__tests__/users.service.test.ts +++ b/apps/api/src/users/__tests__/users.service.test.ts @@ -38,6 +38,7 @@ const baseUser = { describe('UsersService', () => { let usersService: UsersService; let userModel: MockedInstance>; + let auditLogger: MockedInstance; let cryptoService: MockedInstance; let groupsService: MockedInstance; let permissionSetsService: MockedInstance; @@ -56,6 +57,7 @@ describe('UsersService', () => { }).compile(); userModel = moduleRef.get(getModelToken('User')); usersService = moduleRef.get(UsersService); + auditLogger = moduleRef.get(AuditLogger); cryptoService = moduleRef.get(CryptoService); groupsService = moduleRef.get(GroupsService); permissionSetsService = moduleRef.get(PermissionSetsService); @@ -268,17 +270,19 @@ describe('UsersService', () => { }); describe('updateById', () => { + const storedUser = { + additionalPermissions: [ + { action: 'read', groupId: 'group-1', subject: 'Subject' }, + { action: 'read', groupId: 'group-2', subject: 'Subject' }, + { action: 'create', groupId: null, subject: 'Instrument' } + ], + groupIds: ['group-1', 'group-2'], + id: 'user-1', + username: 'jane.doe' + }; + beforeEach(() => { - userModel.findFirst.mockResolvedValue({ - additionalPermissions: [ - { action: 'read', groupId: 'group-1', subject: 'Subject' }, - { action: 'read', groupId: 'group-2', subject: 'Subject' }, - { action: 'create', groupId: null, subject: 'Instrument' } - ], - groupIds: ['group-1', 'group-2'], - id: 'user-1', - username: 'jane.doe' - }); + userModel.findFirst.mockResolvedValue(storedUser); userModel.update.mockResolvedValue({}); }); @@ -333,7 +337,27 @@ describe('UsersService', () => { await expect( usersService.updateById('user-1', { password: 'New.Name', username: 'new.name' }, admin) ).rejects.toMatchObject({ response: { code: 'PASSWORD_MATCHES_USERNAME' } }); - expect(userModel.findFirst).not.toHaveBeenCalled(); + }); + + it('should record the fields the update changed, naming the user it changed', async () => { + userModel.update.mockResolvedValue({ ...storedUser, groupIds: ['group-2', 'group-1'], username: 'janet.doe' }); + await usersService.updateById('user-1', { groupIds: ['group-2', 'group-1'], username: 'janet.doe' }, admin); + expect(auditLogger.logUpdate).toHaveBeenCalledWith('USER', ['username'], { + groupId: null, + metadata: { targetUserId: 'user-1' }, + userId: admin.id + }); + }); + + it('should name a new password as changed, though neither row carries it', async () => { + userModel.update.mockResolvedValue(storedUser); + await usersService.updateById('user-1', { password: 'jf8&Kd0!mZq2wLx' }, admin); + expect(auditLogger.logUpdate.mock.lastCall?.[1]).toEqual(['password']); + }); + + it('should record no update that the service refused', async () => { + await expect(usersService.updateById(admin.id, { disabled: true }, admin)).rejects.toThrow(ForbiddenException); + expect(auditLogger.logUpdate).not.toHaveBeenCalled(); }); it('should remove the permission set of a user promoted to administrator, who already holds every permission', async () => { @@ -373,6 +397,15 @@ describe('UsersService', () => { expect(userModel.update.mock.lastCall?.[0].data.archivedAt).toBeInstanceOf(Date); expect(userModel.update.mock.lastCall?.[0].where).toMatchObject({ id: 'user-1' }); }); + + it('should record the archive as an archive, so the Archive filter finds it beside instruments', async () => { + await usersService.archiveById('user-1', admin); + expect(auditLogger.log).toHaveBeenCalledWith('ARCHIVE', 'USER', { + groupId: null, + metadata: { targetUserId: 'user-1' }, + userId: admin.id + }); + }); }); describe('unarchiveById', () => { @@ -389,6 +422,15 @@ describe('UsersService', () => { expect(userModel.update.mock.lastCall?.[0].data).toMatchObject({ archivedAt: null }); expect(userModel.update.mock.lastCall?.[0].where).toMatchObject({ id: 'user-1' }); }); + + it('should record the restore as an unarchive, so the Unarchive filter finds it', async () => { + await usersService.unarchiveById('user-1', admin); + expect(auditLogger.log).toHaveBeenCalledWith('UNARCHIVE', 'USER', { + groupId: null, + metadata: { targetUserId: 'user-1' }, + userId: admin.id + }); + }); }); describe('updatePermissions', () => { @@ -400,7 +442,7 @@ describe('UsersService', () => { describe('custom grants', () => { it('should write a grant confined to a group the user belongs to', async () => { const permissions: Permissions = [{ action: 'read', groupId: 'group-1', subject: 'Subject' }]; - await usersService.updatePermissions('user-1', { kind: 'custom', permissions }); + await usersService.updatePermissions('user-1', { kind: 'custom', permissions }, admin); expect(userModel.update.mock.lastCall?.[0]).toMatchObject({ data: { additionalPermissions: permissions }, where: { id: 'user-1' } @@ -408,13 +450,13 @@ describe('UsersService', () => { }); it('should remove the permission set in the same write, so the user never holds both', async () => { - await usersService.updatePermissions('user-1', { kind: 'custom', permissions: [] }); + await usersService.updatePermissions('user-1', { kind: 'custom', permissions: [] }, admin); expect(userModel.update.mock.lastCall?.[0].data.permissionSet).toEqual({ disconnect: true }); }); it('should reject a grant confined to a group the user does not belong to, before writing anything', async () => { const permissions: Permissions = [{ action: 'read', groupId: 'group-2', subject: 'Subject' }]; - await expect(usersService.updatePermissions('user-1', { kind: 'custom', permissions })).rejects.toThrow( + await expect(usersService.updatePermissions('user-1', { kind: 'custom', permissions }, admin)).rejects.toThrow( BadRequestException ); expect(userModel.update).not.toHaveBeenCalled(); @@ -422,7 +464,7 @@ describe('UsersService', () => { it("should accept an unscoped grant whatever the user's groups", async () => { const permissions: Permissions = [{ action: 'create', groupId: null, subject: 'Instrument' }]; - await usersService.updatePermissions('user-1', { kind: 'custom', permissions }); + await usersService.updatePermissions('user-1', { kind: 'custom', permissions }, admin); expect(userModel.update.mock.lastCall?.[0]).toMatchObject({ data: { additionalPermissions: permissions } }); }); }); @@ -431,7 +473,7 @@ describe('UsersService', () => { const data = { kind: 'permissionSet', permissionSetId: 'set-1' } as const; it('should link the set and empty the custom grants in the same write, so the user never holds both', async () => { - await usersService.updatePermissions('user-1', data); + await usersService.updatePermissions('user-1', data, admin); expect(userModel.update.mock.lastCall?.[0]).toMatchObject({ data: { additionalPermissions: [], permissionSet: { connect: { id: 'set-1' } } }, where: { id: 'user-1' } @@ -439,28 +481,28 @@ describe('UsersService', () => { }); it('should look up the set with the caller ability', async () => { - await usersService.updatePermissions('user-1', data, { ability: admin.ability }); + await usersService.updatePermissions('user-1', data, admin); expect(permissionSetsService.findById).toHaveBeenCalledWith('set-1', { ability: admin.ability }); }); it('should propagate a set lookup that finds nothing, before writing anything', async () => { permissionSetsService.findById.mockRejectedValue(new NotFoundException()); - await expect(usersService.updatePermissions('user-1', data)).rejects.toThrow(NotFoundException); + await expect(usersService.updatePermissions('user-1', data, admin)).rejects.toThrow(NotFoundException); expect(userModel.update).not.toHaveBeenCalled(); }); it('should refuse a set for an administrator, who already holds every permission', async () => { userModel.findFirst.mockResolvedValue({ basePermissionLevel: 'ADMIN', groupIds: [], id: 'user-1' }); - await expect(usersService.updatePermissions('user-1', data)).rejects.toThrow(BadRequestException); + await expect(usersService.updatePermissions('user-1', data, admin)).rejects.toThrow(BadRequestException); expect(userModel.update).not.toHaveBeenCalled(); }); }); it('should throw when the user cannot be found, rather than creating one', async () => { userModel.findFirst.mockResolvedValue(null); - await expect(usersService.updatePermissions('user-1', { kind: 'custom', permissions: [] })).rejects.toThrow( - NotFoundException - ); + await expect( + usersService.updatePermissions('user-1', { kind: 'custom', permissions: [] }, admin) + ).rejects.toThrow(NotFoundException); expect(userModel.update).not.toHaveBeenCalled(); }); @@ -471,11 +513,35 @@ describe('UsersService', () => { const ability = createAppAbility([ { action: 'manage', conditions: { groupIds: { hasSome: ['group-1'] } }, subject: 'User' } ]); - await usersService.updatePermissions('user-1', data, { ability }); + await usersService.updatePermissions('user-1', data, { ...admin, ability }); expect(userModel.update.mock.lastCall?.[0]).toMatchObject({ where: { AND: [accessibleQuery(ability, 'update', 'User')], id: 'user-1' } }); }); + + it('should record the permission fields the write changed, naming the user it changed', async () => { + userModel.update.mockResolvedValue({ + additionalPermissions: [], + basePermissionLevel: 'STANDARD', + groupIds: ['group-1'], + id: 'user-1', + permissionSetId: 'set-1' + }); + await usersService.updatePermissions('user-1', { kind: 'permissionSet', permissionSetId: 'set-1' }, admin); + expect(auditLogger.logUpdate).toHaveBeenCalledWith('USER', ['additionalPermissions', 'permissionSetId'], { + groupId: null, + metadata: { targetUserId: 'user-1' }, + userId: admin.id + }); + }); + + it('should record no permission write that the service refused', async () => { + const permissions: Permissions = [{ action: 'read', groupId: 'group-2', subject: 'Subject' }]; + await expect(usersService.updatePermissions('user-1', { kind: 'custom', permissions }, admin)).rejects.toThrow( + BadRequestException + ); + expect(auditLogger.logUpdate).not.toHaveBeenCalled(); + }); }); describe('updateSelfById', () => { @@ -504,8 +570,27 @@ describe('UsersService', () => { expect(userModel.update).not.toHaveBeenCalled(); }); + it('should read the account it compares against by id alone, since a group manager in no group cannot read themselves', async () => { + const ability = createAppAbility([ + { action: 'read', conditions: { groupIds: { hasSome: [] } }, subject: 'User' } + ]); + await usersService.updateSelfById('user-1', { firstName: 'Janet' }, { ...currentUser, ability }); + expect(userModel.findFirst.mock.calls[0]?.[0]?.where).toStrictEqual({ AND: [{}], id: 'user-1' }); + }); + + it('should record the fields the user changed on their own account, never their values', async () => { + userModel.findFirst.mockResolvedValue({ firstName: 'Jane', id: 'user-1', mustResetPassword: true }); + userModel.update.mockResolvedValue({ firstName: 'Janet', id: 'user-1', mustResetPassword: false }); + await usersService.updateSelfById('user-1', { firstName: 'Janet', password: 'jf8&Kd0!mZq2wLx' }, currentUser); + expect(auditLogger.logUpdate).toHaveBeenCalledWith('USER', ['firstName', 'mustResetPassword', 'password'], { + groupId: null, + metadata: { targetUserId: 'user-1' }, + userId: 'user-1' + }); + }); + it('should throw when the account disappears before its current password can be compared', async () => { - userModel.findFirst.mockResolvedValue(null); + userModel.findFirst.mockResolvedValueOnce({ id: 'user-1' }).mockResolvedValueOnce(null); await expect(usersService.updateSelfById('user-1', { password: 'jf8&Kd0!mZq2wLx' }, currentUser)).rejects.toThrow( NotFoundException ); diff --git a/apps/api/src/users/users.controller.ts b/apps/api/src/users/users.controller.ts index 0e0287b40..86a6948e2 100644 --- a/apps/api/src/users/users.controller.ts +++ b/apps/api/src/users/users.controller.ts @@ -11,6 +11,7 @@ import { } from '@opendatacapture/schemas/user'; import type { CreateUserResponse } from '@opendatacapture/schemas/user'; +import { AuditLogger } from '@/audit/audit.logger'; import type { AppAbility } from '@/auth/auth.types'; import { ADMIN_ONLY, RouteAccess } from '@/core/decorators/route-access.decorator'; import { GroupsService } from '@/groups/groups.service'; @@ -28,6 +29,7 @@ import { UsersService } from './users.service'; export class UsersController { constructor( private readonly usersService: UsersService, + private readonly auditLogger: AuditLogger, private readonly groupsService: GroupsService, private readonly mailService: MailService ) {} @@ -51,11 +53,18 @@ export class UsersController { @RouteAccess(ADMIN_ONLY) async create( @Body() user: $CreateUserData, - @CurrentUser('ability') ability: AppAbility, + @CurrentUser() currentUser: RequestUser, @Headers('origin') origin?: string, @Query('language', new ParseSchemaPipe({ schema: $Language.optional() })) language?: Language ): Promise { + const { ability } = currentUser; const created = await this.usersService.create(user, { ability }); + // Logged here rather than in the service, which setup and demo seeding also call with no one acting. + await this.auditLogger.log('CREATE', 'USER', { + groupId: null, + metadata: { targetUserId: created.id }, + userId: currentUser.id + }); // Attempt to send the welcome email. The result is always returned (even when // mail is disabled or the user has no email) so the client can fall back to a // copy-pasteable message; the client ignores it entirely when mail is off. @@ -105,9 +114,9 @@ export class UsersController { updatePermissions( @Param('id') id: string, @Body() data: $UpdateUserPermissionsData, - @CurrentUser('ability') ability: AppAbility + @CurrentUser() currentUser: RequestUser ) { - return this.usersService.updatePermissions(id, data, { ability }); + return this.usersService.updatePermissions(id, data, currentUser); } @ApiOperation({ summary: 'Self Update User' }) diff --git a/apps/api/src/users/users.service.ts b/apps/api/src/users/users.service.ts index ff696c8ea..bf9b7425f 100644 --- a/apps/api/src/users/users.service.ts +++ b/apps/api/src/users/users.service.ts @@ -16,14 +16,18 @@ import { $UpdateUserPermissionsData } from '@opendatacapture/schemas/user'; import type { PasswordErrorCode } from '@opendatacapture/schemas/user'; +import type { Prisma } from '@prisma/client'; import { pwnedPassword } from 'hibp'; import { AuditLogger } from '@/audit/audit.logger'; +import { changedFields } from '@/audit/audit.utils'; import { accessibleQuery } from '@/auth/ability.utils'; import type { EntityOperationOptions } from '@/core/types'; import { GroupsService } from '@/groups/groups.service'; import { PermissionSetsService } from '@/permission-sets/permission-sets.service'; +type UserRow = Awaited>; + @Injectable() export class UsersService { private readonly logger = new Logger(UsersService.name); @@ -47,9 +51,9 @@ export class UsersService { }, where: { AND: [accessibleQuery(currentUser.ability, 'update', 'User')], id } }); - await this.auditLogger.log('UPDATE', 'USER', { + await this.auditLogger.log('ARCHIVE', 'USER', { groupId: null, - metadata: { action: 'archive', targetUserId: id }, + metadata: { targetUserId: id }, userId: currentUser.id }); return user; @@ -190,9 +194,9 @@ export class UsersService { }, where: { AND: [accessibleQuery(currentUser.ability, 'update', 'User')], id } }); - await this.auditLogger.log('UPDATE', 'USER', { + await this.auditLogger.log('UNARCHIVE', 'USER', { groupId: null, - metadata: { action: 'unarchive', targetUserId: id }, + metadata: { targetUserId: id }, userId: currentUser.id }); return user; @@ -206,16 +210,16 @@ export class UsersService { if (id === currentUser.id && (data.disabled || isDemotion)) { throw new ForbiddenException('You may not remove your own administrator access'); } + const current = await this.findById(id, { ability }); let hashedPassword: string | undefined; if (password) { - const username = data.username ?? (await this.findById(id, { ability })).username; - await this.validatePassword(password, username); + await this.validatePassword(password, data.username ?? current.username); hashedPassword = await this.cryptoService.hashPassword(password); } - return this.userModel.update({ + const user = await this.userModel.update({ data: { ...data, - additionalPermissions: groupIds ? await this.permissionsWithinGroups(id, groupIds, { ability }) : undefined, + additionalPermissions: groupIds ? this.permissionsWithinGroups(current, groupIds) : undefined, groups: { set: groupIds?.map((id) => ({ id })) }, @@ -229,6 +233,8 @@ export class UsersService { }, where: { AND: [accessibleQuery(ability, 'update', 'User')], id } }); + await this.logUserUpdate(current, user, { password, userId: currentUser.id }); + return user; } /** @@ -237,37 +243,36 @@ export class UsersService { * custom grant confined to a group is accepted only when the user belongs to that group, so what * the web client offers and what this accepts are one rule. */ - async updatePermissions(id: string, data: $UpdateUserPermissionsData, { ability }: EntityOperationOptions = {}) { - const user = await this.findById(id, { ability }); - const where = { AND: [accessibleQuery(ability, 'update', 'User')], id }; + async updatePermissions(id: string, data: $UpdateUserPermissionsData, currentUser: RequestUser) { + const { ability } = currentUser; + const current = await this.findById(id, { ability }); + let update: Prisma.UserUpdateInput; if (data.kind === 'permissionSet') { - if (user.basePermissionLevel === 'ADMIN') { + if (current.basePermissionLevel === 'ADMIN') { throw new BadRequestException('Administrators hold every permission, so they cannot hold a permission set'); } await this.permissionSetsService.findById(data.permissionSetId, { ability }); - return this.userModel.update({ - data: { additionalPermissions: [], permissionSet: { connect: { id: data.permissionSetId } } }, - omit: { - hashedPassword: true - }, - where - }); - } - const foreignGroupIds = data.permissions - .map(({ groupId }) => groupId) - .filter((groupId): groupId is string => groupId !== null && !user.groupIds.includes(groupId)); - if (foreignGroupIds.length) { - throw new BadRequestException( - `Permissions may only be scoped to a group the user belongs to: ${foreignGroupIds.join(', ')}` - ); + update = { additionalPermissions: [], permissionSet: { connect: { id: data.permissionSetId } } }; + } else { + const foreignGroupIds = data.permissions + .map(({ groupId }) => groupId) + .filter((groupId): groupId is string => groupId !== null && !current.groupIds.includes(groupId)); + if (foreignGroupIds.length) { + throw new BadRequestException( + `Permissions may only be scoped to a group the user belongs to: ${foreignGroupIds.join(', ')}` + ); + } + update = { additionalPermissions: data.permissions, permissionSet: { disconnect: true } }; } - return this.userModel.update({ - data: { additionalPermissions: data.permissions, permissionSet: { disconnect: true } }, + const user = await this.userModel.update({ + data: update, omit: { hashedPassword: true }, - where + where: { AND: [accessibleQuery(ability, 'update', 'User')], id } }); + await this.logUserUpdate(current, user, { userId: currentUser.id }); + return user; } async updateSelfById(id: string, { password, ...data }: $SelfUpdateUserData, currentUser: RequestUser) { @@ -277,6 +282,9 @@ export class UsersService { const { dateOfBirth, email, firstName, lastName, phoneNumber, sex } = data; + // Unscoped, like the write below, because the id is the caller's own: a group manager in no + // group holds no rule that lets them read themselves. + const current = await this.findById(id); let hashedPassword: string | undefined; if (password) { await this.validatePassword(password, currentUser.username); @@ -284,7 +292,7 @@ export class UsersService { hashedPassword = await this.cryptoService.hashPassword(password); } - return this.userModel.update({ + const user = await this.userModel.update({ data: { dateOfBirth, email, @@ -302,6 +310,25 @@ export class UsersService { }, where: { id } }); + await this.logUserUpdate(current, user, { password, userId: currentUser.id }); + return user; + } + + /** + * The password is stored only as a hash, which neither row carries, so a new one is named apart + * from what the two rows show. + */ + private async logUserUpdate( + before: UserRow, + after: UserRow, + { password, userId }: { password?: string; userId: string } + ): Promise { + const fields = changedFields(before, after); + await this.auditLogger.logUpdate('USER', password ? [...fields, 'password'] : fields, { + groupId: null, + metadata: { targetUserId: after.id }, + userId + }); } /** @@ -316,8 +343,7 @@ export class UsersService { * The user's stored grants minus those confined to a group outside `groupIds`. A grant scoped to a * group the user is leaving would otherwise keep reading that group's rows. */ - private async permissionsWithinGroups(id: string, groupIds: string[], options: EntityOperationOptions) { - const { additionalPermissions } = await this.findById(id, options); + private permissionsWithinGroups({ additionalPermissions }: UserRow, groupIds: string[]) { return additionalPermissions.filter(({ groupId }) => groupId === null || groupIds.includes(groupId)); } diff --git a/packages/schemas/src/audit/audit.ts b/packages/schemas/src/audit/audit.ts index 25bcd561d..6f8ec0c88 100644 --- a/packages/schemas/src/audit/audit.ts +++ b/packages/schemas/src/audit/audit.ts @@ -30,7 +30,7 @@ export type AuditedAction = (type export const AUDITED_ACTIONS = { ASSIGNMENT: ['CREATE', 'SEND_EMAIL', 'UPDATE'], INSTRUMENT: ['ARCHIVE', 'UNARCHIVE'], - USER: ['LOGIN', 'UPDATE'] + USER: ['ARCHIVE', 'CREATE', 'LOGIN', 'UNARCHIVE', 'UPDATE'] } as const satisfies { [TEntity in $AuditLogEntity]?: readonly $AuditLogAction[] }; /** The number of audit logs shown per page, matching the page size used by the client's data table. */ diff --git a/testing/src/specs/admin-audit-logs.spec.ts b/testing/src/specs/admin-audit-logs.spec.ts index d5f12fffc..3e5db302f 100644 --- a/testing/src/specs/admin-audit-logs.spec.ts +++ b/testing/src/specs/admin-audit-logs.spec.ts @@ -1,3 +1,5 @@ +import type { $AuditLog } from '@opendatacapture/schemas/audit'; + import { ApiClient } from '../support/api-client'; import { ADMIN } from '../support/constants'; import { expect, test } from '../support/fixtures'; @@ -118,3 +120,63 @@ test.describe('admin audit logs fetching', () => { expect(auditLogsRequests).toHaveLength(2); }); }); + +/** The entry among `entries` whose metadata names `value` under `key`, which is how an entry says what it acted on. */ +function entryNaming(entries: $AuditLog[], key: string, value: string) { + return entries.find(({ metadata }) => metadata?.[key] === value); +} + +test.describe('admin audit log coverage', () => { + test.describe('users', () => { + test('should record who created a user', async ({ api }) => { + const { user } = await api.createUser(); + + const entries = await api.findAuditLogs({ action: 'CREATE', entity: 'USER' }); + expect(entryNaming(entries, 'targetUserId', user.id)?.user?.username).toBe(ADMIN.username); + }); + + test('should record which fields an update changed, and not their values', async ({ api }) => { + const { user } = await api.createUser(); + await api.updateUser(user.id, { firstName: 'Renamed', lastName: user.lastName }); + + const entry = entryNaming(await api.findAuditLogs({ action: 'UPDATE', entity: 'USER' }), 'targetUserId', user.id); + expect(entry?.metadata?.fields).toBe('firstName'); + expect(JSON.stringify(entry)).not.toContain('Renamed'); + }); + + test('should record a change to the permissions a user holds', async ({ api }) => { + const { user } = await api.createUser(); + await api.setUserPermissions(user.id, [{ action: 'read', groupId: null, subject: 'Subject' }]); + + const entry = entryNaming(await api.findAuditLogs({ action: 'UPDATE', entity: 'USER' }), 'targetUserId', user.id); + expect(entry?.metadata?.fields).toBe('additionalPermissions'); + }); + + test('should record a user updating their own account, as that user', async ({ api, apiRequestContext }) => { + const { credentials, user } = await api.createUser(); + const self = new ApiClient(apiRequestContext, await ApiClient.login(apiRequestContext, credentials)); + await self.updateSelf(user.id, { firstName: 'Renamed' }); + + const entry = entryNaming(await api.findAuditLogs({ action: 'UPDATE', entity: 'USER' }), 'targetUserId', user.id); + expect(entry?.metadata?.fields).toBe('firstName'); + expect(entry?.user?.username).toBe(credentials.username); + }); + + test('should record archiving a user as an archive', async ({ api }) => { + const { user } = await api.createUser(); + await api.archiveUser(user.id); + + const entries = await api.findAuditLogs({ action: 'ARCHIVE', entity: 'USER' }); + expect(entryNaming(entries, 'targetUserId', user.id)).toBeDefined(); + }); + + test('should record unarchiving a user as an unarchive', async ({ api }) => { + const { user } = await api.createUser(); + await api.archiveUser(user.id); + await api.unarchiveUser(user.id); + + const entries = await api.findAuditLogs({ action: 'UNARCHIVE', entity: 'USER' }); + expect(entryNaming(entries, 'targetUserId', user.id)).toBeDefined(); + }); + }); +}); diff --git a/testing/src/specs/mail.spec.ts b/testing/src/specs/mail.spec.ts index d336e032f..31931070d 100644 --- a/testing/src/specs/mail.spec.ts +++ b/testing/src/specs/mail.spec.ts @@ -356,7 +356,7 @@ test.describe('mail delivery', () => { const result = await api.sendAssignmentEmail(assignment.id, { language: 'en', recipient }); expect(result.status).toBe('FAILED'); - const entries = await api.findAuditLogs('SEND_EMAIL'); + const entries = await api.findAuditLogs({ action: 'SEND_EMAIL', entity: 'ASSIGNMENT' }); const entry = entries.find(({ metadata }) => metadata?.assignmentId === assignment.id); expect(entry?.metadata?.recipient).toBe('p***@example.org'); expect(JSON.stringify(entries)).not.toContain(recipient); diff --git a/testing/src/support/api-client.ts b/testing/src/support/api-client.ts index bf3331af8..42311c701 100644 --- a/testing/src/support/api-client.ts +++ b/testing/src/support/api-client.ts @@ -1,6 +1,6 @@ import type { $CreateAssignmentData, Assignment } from '@opendatacapture/schemas/assignment'; import { AUDIT_LOGS_MAX_PAGE_SIZE } from '@opendatacapture/schemas/audit'; -import type { $AuditLog, $AuditLogAction, $AuditLogsPage } from '@opendatacapture/schemas/audit'; +import type { $AuditLog, $AuditLogsPage, AuditedAction, AuditedEntity } from '@opendatacapture/schemas/audit'; import type { $LoginCredentials } from '@opendatacapture/schemas/auth'; import type { Language, Permissions } from '@opendatacapture/schemas/core'; import type { $CreateGroupData, Group } from '@opendatacapture/schemas/group'; @@ -19,7 +19,13 @@ import type { } from '@opendatacapture/schemas/permission-set'; import type { $CreateSessionData, Session, SessionType } from '@opendatacapture/schemas/session'; import type { $CreateSubjectData } from '@opendatacapture/schemas/subject'; -import type { $CreateUserData, $UpdateUserData, $UpdateUserPermissionsData, User } from '@opendatacapture/schemas/user'; +import type { + $CreateUserData, + $SelfUpdateUserData, + $UpdateUserData, + $UpdateUserPermissionsData, + User +} from '@opendatacapture/schemas/user'; import type { APIRequestContext } from '@playwright/test'; import { E2E_MAIL_CONFIG, SEEDED_USER_PASSWORD } from './constants'; @@ -229,15 +235,21 @@ export class ApiClient { ); } - /** The newest audit entries for one action, as the audit log page and its JSON download read them. */ - async findAuditLogs(action: $AuditLogAction): Promise<$AuditLog[]> { + /** The newest audit entries for one action on one entity, as the audit log page and its JSON download read them. */ + async findAuditLogs({ + action, + entity + }: { + action: AuditedAction; + entity: TEntity; + }): Promise<$AuditLog[]> { const { data } = await this.expectJson<$AuditLogsPage>( this.request.get(`${API}/audit/logs`, { headers: this.authHeaders, - params: { action, limit: AUDIT_LOGS_MAX_PAGE_SIZE } + params: { action, entity, limit: AUDIT_LOGS_MAX_PAGE_SIZE } }), 200, - `find the '${action}' audit logs` + `find the '${action}' '${entity}' audit logs` ); return data; } @@ -352,6 +364,21 @@ export class ApiClient { return this.putUserPermissions(id, { kind: 'permissionSet', permissionSetId }); } + async unarchiveUser(id: string): Promise { + await this.expectOk( + this.request.patch(`${API}/users/${id}/unarchive`, { headers: this.authHeaders }), + `unarchive user '${id}'` + ); + } + + /** Updates the account this client is signed in as, through the route the profile page uses. */ + async updateSelf(id: string, data: $SelfUpdateUserData): Promise { + await this.expectOk( + this.request.patch(`${API}/users/self-update/${id}`, { data, headers: this.authHeaders }), + `update own account '${id}'` + ); + } + /** Updates a user's profile fields, which is everything except their permissions. */ async updateUser(id: string, data: Partial<$UpdateUserData>): Promise { const response = await this.request.patch(`${API}/users/${id}`, { data, headers: this.authHeaders }); From 2c584f3df7ad9706d1a2381f3cb35f3fc66e96f5 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Sun, 8 Nov 2026 20:33:37 -0500 Subject: [PATCH 06/10] feat(api): record assignment deletion in the audit log Deleting assignments in bulk writes one DELETE entry per assignment that was actually removed, in that assignment's group, naming it. An assignment that failed to delete is not recorded. The route now forwards the request user, so the service can attribute the entries. Co-Authored-By: Claude Opus 5.5 --- .../__tests__/assignments.controller.test.ts | 10 ++--- .../__tests__/assignments.service.test.ts | 41 +++++++++++++++---- .../src/assignments/assignments.controller.ts | 4 +- .../src/assignments/assignments.service.ts | 19 ++++++--- packages/schemas/src/audit/audit.ts | 2 +- testing/src/specs/admin-audit-logs.spec.ts | 16 ++++++++ testing/src/support/api-client.ts | 15 ++++++- 7 files changed, 82 insertions(+), 25 deletions(-) diff --git a/apps/api/src/assignments/__tests__/assignments.controller.test.ts b/apps/api/src/assignments/__tests__/assignments.controller.test.ts index fb8d392e0..22dd25a53 100644 --- a/apps/api/src/assignments/__tests__/assignments.controller.test.ts +++ b/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); }); }); diff --git a/apps/api/src/assignments/__tests__/assignments.service.test.ts b/apps/api/src/assignments/__tests__/assignments.service.test.ts index 8bd4c1f27..e5f2016f1 100644 --- a/apps/api/src/assignments/__tests__/assignments.service.test.ts +++ b/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(); }); }); diff --git a/apps/api/src/assignments/assignments.controller.ts b/apps/api/src/assignments/assignments.controller.ts index eb218ccde..36913f83c 100644 --- a/apps/api/src/assignments/assignments.controller.ts +++ b/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 { - return this.assignmentsService.deleteBulk(data.ids, { ability }); + return this.assignmentsService.deleteBulk(data.ids, currentUser); } @ApiOperation({ summary: 'Get All Assignments' }) diff --git a/apps/api/src/assignments/assignments.service.ts b/apps/api/src/assignments/assignments.service.ts index a6e6e491b..9118b23cb 100644 --- a/apps/api/src/assignments/assignments.service.ts +++ b/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( diff --git a/packages/schemas/src/audit/audit.ts b/packages/schemas/src/audit/audit.ts index 6f8ec0c88..347f9148d 100644 --- a/packages/schemas/src/audit/audit.ts +++ b/packages/schemas/src/audit/audit.ts @@ -28,7 +28,7 @@ export type AuditedAction = (type * stored rows hold. */ export const AUDITED_ACTIONS = { - ASSIGNMENT: ['CREATE', 'SEND_EMAIL', 'UPDATE'], + ASSIGNMENT: ['CREATE', 'DELETE', 'SEND_EMAIL', 'UPDATE'], INSTRUMENT: ['ARCHIVE', 'UNARCHIVE'], USER: ['ARCHIVE', 'CREATE', 'LOGIN', 'UNARCHIVE', 'UPDATE'] } as const satisfies { [TEntity in $AuditLogEntity]?: readonly $AuditLogAction[] }; diff --git a/testing/src/specs/admin-audit-logs.spec.ts b/testing/src/specs/admin-audit-logs.spec.ts index 3e5db302f..23ddefea9 100644 --- a/testing/src/specs/admin-audit-logs.spec.ts +++ b/testing/src/specs/admin-audit-logs.spec.ts @@ -179,4 +179,20 @@ test.describe('admin audit log coverage', () => { expect(entryNaming(entries, 'targetUserId', user.id)).toBeDefined(); }); }); + + test.describe('assignments', () => { + test('should record each deleted assignment in its group', async ({ api }) => { + const group = await api.createGroup(); + const assignment = await api.createAssignment({ + expiresAt: new Date(Date.now() + 86_400_000), + groupId: group.id, + instrumentId: await api.findInstrumentId('FORM'), + subjectId: await api.createSubject(group.id) + }); + await api.deleteAssignments([assignment.id]); + + const entries = await api.findAuditLogs({ action: 'DELETE', entity: 'ASSIGNMENT' }); + expect(entryNaming(entries, 'assignmentId', assignment.id)?.group?.name).toBe(group.name); + }); + }); }); diff --git a/testing/src/support/api-client.ts b/testing/src/support/api-client.ts index 42311c701..cb86a77e8 100644 --- a/testing/src/support/api-client.ts +++ b/testing/src/support/api-client.ts @@ -1,4 +1,8 @@ -import type { $CreateAssignmentData, Assignment } from '@opendatacapture/schemas/assignment'; +import type { + $CreateAssignmentData, + Assignment, + DeleteBulkAssignmentsResult +} from '@opendatacapture/schemas/assignment'; import { AUDIT_LOGS_MAX_PAGE_SIZE } from '@opendatacapture/schemas/audit'; import type { $AuditLog, $AuditLogsPage, AuditedAction, AuditedEntity } from '@opendatacapture/schemas/audit'; import type { $LoginCredentials } from '@opendatacapture/schemas/auth'; @@ -213,6 +217,15 @@ export class ApiClient { return { credentials: { password, username }, user }; } + /** Deletes assignments through the bulk route the web app's assignment table uses. */ + async deleteAssignments(ids: string[]): Promise { + return this.expectJson( + this.request.post(`${API}/assignments/bulk/delete`, { data: { ids }, headers: this.authHeaders }), + 200, + 'delete assignments' + ); + } + /** * The languages the instance currently offers. Read rather than assumed because * `admin-settings.spec.ts` turns one on and off, in a worker of its own. From 6cca28c81bac3bcbb2da3beffb318d101f7f5412 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Sun, 8 Nov 2026 20:35:24 -0500 Subject: [PATCH 07/10] feat(api): record series creation and deletion in the audit log Creating a series and deleting one each write an entry in the group that owns the series, naming it by id and title, as archiving one already did. A request that only reports a duplicate creates nothing and records nothing. Co-Authored-By: Claude Opus 5.5 --- .../__tests__/instruments.service.test.ts | 74 ++++++++++++++++++- .../src/instruments/instruments.service.ts | 10 +++ packages/schemas/src/audit/audit.ts | 2 +- testing/src/specs/admin-audit-logs.spec.ts | 24 ++++++ testing/src/support/api-client.ts | 8 ++ 5 files changed, 115 insertions(+), 3 deletions(-) diff --git a/apps/api/src/instruments/__tests__/instruments.service.test.ts b/apps/api/src/instruments/__tests__/instruments.service.test.ts index 84ae054e8..a0b726935 100644 --- a/apps/api/src/instruments/__tests__/instruments.service.test.ts +++ b/apps/api/src/instruments/__tests__/instruments.service.test.ts @@ -291,6 +291,46 @@ describe('InstrumentsService', () => { expect(result).toEqual({ instrumentId: 'fresh-id', outcome: 'created' }); }); + it('records who created the series, in the group that owns it, naming it', async () => { + vi.spyOn(instrumentsService, 'find').mockResolvedValue([]); + vi.spyOn(instrumentsService, 'create').mockResolvedValue({ id: 'fresh-id' } as any); + + await instrumentsService.createSeries( + { + details: { title: ' Totally New ' }, + groupId: 'group-1', + items: [{ edition: 1, name: 'FORM_C' }], + language: 'en' + }, + adminUser + ); + + expect(auditLogger.log).toHaveBeenCalledExactlyOnceWith('CREATE', 'INSTRUMENT', { + groupId: 'group-1', + metadata: { instrumentId: 'fresh-id', title: 'Totally New' }, + userId: adminUser.id + }); + }); + + it('records nothing when it only reports a duplicate, since no series was created', async () => { + vi.spyOn(instrumentsService, 'find').mockResolvedValue([existingSeries]); + + await instrumentsService.createSeries( + { + details: { title: 'My New Series' }, + groupId: 'group-1', + items: [ + { edition: 1, name: 'FORM_A' }, + { edition: 1, name: 'FORM_B' } + ], + language: 'en' + }, + adminUser + ); + + expect(auditLogger.log).not.toHaveBeenCalled(); + }); + it('generates a valid, unilingual series source payload with selected items in order', async () => { vi.spyOn(instrumentsService, 'find').mockResolvedValue([]); vi.spyOn(instrumentsService, 'create').mockResolvedValue({ id: 'generated-id' } as any); @@ -867,7 +907,7 @@ describe('InstrumentsService', () => { instrumentModel.findFirst.mockResolvedValue({ bundle: '__BUNDLE__', id: 'target' }); virtualizationService.eval.mockResolvedValue({ isErr: () => false, - value: { content: { items: [] }, kind: 'SERIES' } + value: { content: { items: [] }, details: { title: 'Target Series' }, kind: 'SERIES' } } as any); instrumentRecordModel.count.mockResolvedValue(0); groupModel.findMany.mockResolvedValue([{ accessibleInstrumentIds: ['other', 'target'], id: 'g1' }]); @@ -886,7 +926,7 @@ describe('InstrumentsService', () => { instrumentModel.findFirst.mockResolvedValue({ bundle: '__BUNDLE__', id: 'target' }); virtualizationService.eval.mockResolvedValue({ isErr: () => false, - value: { content: { items: [] }, kind: 'SERIES' } + value: { content: { items: [] }, details: { title: 'Target Series' }, kind: 'SERIES' } } as any); instrumentRecordModel.count.mockResolvedValue(0); groupModel.findMany.mockResolvedValue([]); @@ -897,6 +937,36 @@ describe('InstrumentsService', () => { expect(instanceCache.has('target')).toBe(false); }); + it('records who deleted the series, in the group that owned it, naming it', async () => { + instrumentModel.findFirst.mockResolvedValue({ bundle: '__BUNDLE__', id: 'target', seriesGroupId: 'group-1' }); + virtualizationService.eval.mockResolvedValue({ + isErr: () => false, + value: { content: { items: [] }, details: { title: { en: 'Target Series' } }, kind: 'SERIES' } + } as any); + instrumentRecordModel.count.mockResolvedValue(0); + groupModel.findMany.mockResolvedValue([]); + + await instrumentsService.deleteById('target', adminUser); + + expect(auditLogger.log).toHaveBeenCalledExactlyOnceWith('DELETE', 'INSTRUMENT', { + groupId: 'group-1', + metadata: { instrumentId: 'target', title: 'Target Series' }, + userId: adminUser.id + }); + }); + + it('records no deletion when the delete is refused', async () => { + instrumentModel.findFirst.mockResolvedValue({ bundle: '__BUNDLE__', id: 'target' }); + virtualizationService.eval.mockResolvedValue({ + isErr: () => false, + value: { content: { items: [] }, details: { title: 'Target Series' }, kind: 'SERIES' } + } as any); + instrumentRecordModel.count.mockResolvedValue(3); + + await expect(instrumentsService.deleteById('target', adminUser)).rejects.toThrow(ForbiddenException); + expect(auditLogger.log).not.toHaveBeenCalled(); + }); + it('does not evict the cached instance when the delete is refused', async () => { instrumentModel.findFirst.mockResolvedValue({ bundle: '__BUNDLE__', id: 'scalar' }); virtualizationService.eval.mockResolvedValue({ diff --git a/apps/api/src/instruments/instruments.service.ts b/apps/api/src/instruments/instruments.service.ts index 9e06aa95d..629aa091f 100644 --- a/apps/api/src/instruments/instruments.service.ts +++ b/apps/api/src/instruments/instruments.service.ts @@ -197,6 +197,11 @@ export class InstrumentsService { }); const generatedBundle = await bundle({ inputs: [{ content: source, name: 'index.ts' }], minify: true }); const created = await this.create({ bundle: generatedBundle }, { seriesGroupId: groupId }); + await this.auditLogger.log('CREATE', 'INSTRUMENT', { + groupId, + metadata: { instrumentId: created.id, title: this.describeTitle(title) }, + userId: currentUser.id + }); return { instrumentId: created.id, outcome: 'created' }; } @@ -265,6 +270,11 @@ export class InstrumentsService { // evaluated to check the kind. Leaving it behind would keep a deleted instrument resident for the // lifetime of the process, and would shadow the bundle of any future instrument issued the same id. this.virtualizationService.context.instruments.delete(id); + await this.auditLogger.log('DELETE', 'INSTRUMENT', { + groupId: instrument.seriesGroupId, + metadata: { instrumentId: id, title: this.describeTitle(instance.details.title) }, + userId: currentUser.id + }); return { id }; } diff --git a/packages/schemas/src/audit/audit.ts b/packages/schemas/src/audit/audit.ts index 347f9148d..fdc6310be 100644 --- a/packages/schemas/src/audit/audit.ts +++ b/packages/schemas/src/audit/audit.ts @@ -29,7 +29,7 @@ export type AuditedAction = (type */ export const AUDITED_ACTIONS = { ASSIGNMENT: ['CREATE', 'DELETE', 'SEND_EMAIL', 'UPDATE'], - INSTRUMENT: ['ARCHIVE', 'UNARCHIVE'], + INSTRUMENT: ['ARCHIVE', 'CREATE', 'DELETE', 'UNARCHIVE'], USER: ['ARCHIVE', 'CREATE', 'LOGIN', 'UNARCHIVE', 'UPDATE'] } as const satisfies { [TEntity in $AuditLogEntity]?: readonly $AuditLogAction[] }; diff --git a/testing/src/specs/admin-audit-logs.spec.ts b/testing/src/specs/admin-audit-logs.spec.ts index 23ddefea9..f19c6ee14 100644 --- a/testing/src/specs/admin-audit-logs.spec.ts +++ b/testing/src/specs/admin-audit-logs.spec.ts @@ -195,4 +195,28 @@ test.describe('admin audit log coverage', () => { expect(entryNaming(entries, 'assignmentId', assignment.id)?.group?.name).toBe(group.name); }); }); + + test.describe('series', () => { + test('should record who created a series, in the group that owns it', async ({ api, uniqueId }) => { + const group = await api.createGroup(); + const seriesId = await api.createSeries(group.id, `Audited Series ${uniqueId}`); + + const entry = entryNaming( + await api.findAuditLogs({ action: 'CREATE', entity: 'INSTRUMENT' }), + 'instrumentId', + seriesId + ); + expect(entry?.group?.name).toBe(group.name); + expect(entry?.metadata?.title).toBe(`Audited Series ${uniqueId}`); + }); + + test('should record who deleted a series', async ({ api, uniqueId }) => { + const group = await api.createGroup(); + const seriesId = await api.createSeries(group.id, `Deleted Series ${uniqueId}`); + await api.deleteSeries(seriesId); + + const entries = await api.findAuditLogs({ action: 'DELETE', entity: 'INSTRUMENT' }); + expect(entryNaming(entries, 'instrumentId', seriesId)?.metadata?.title).toBe(`Deleted Series ${uniqueId}`); + }); + }); }); diff --git a/testing/src/support/api-client.ts b/testing/src/support/api-client.ts index cb86a77e8..c94027e10 100644 --- a/testing/src/support/api-client.ts +++ b/testing/src/support/api-client.ts @@ -226,6 +226,14 @@ export class ApiClient { ); } + /** Deletes a series through the route the web app's instrument management uses. */ + async deleteSeries(id: string): Promise { + await this.expectOk( + this.request.delete(`${API}/instruments/${id}`, { headers: this.authHeaders }), + `delete series '${id}'` + ); + } + /** * The languages the instance currently offers. Read rather than assumed because * `admin-settings.spec.ts` turns one on and off, in a worker of its own. From e3989ee20207d4529b9b50cba145a8033005de81 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Sun, 8 Nov 2026 20:38:08 -0500 Subject: [PATCH 08/10] feat(api): record subject deletion in the audit log Deleting a subject writes one entry, carrying whether the delete was forced and how many records it removed, rather than one per cascaded row. A subject can belong to several groups and the route names none, so the entry names no group. It is written in the controller once the service call returns, leaving the delete and its transaction as they were. Co-Authored-By: Claude Opus 5.5 --- .../__tests__/subjects.controller.test.ts | 45 +++++++++++++++++-- .../__tests__/subjects.service.test.ts | 14 ++++++ apps/api/src/subjects/subjects.controller.ts | 24 ++++++++-- apps/api/src/subjects/subjects.service.ts | 7 +++ packages/schemas/src/audit/audit.ts | 1 + testing/src/specs/admin-audit-logs.spec.ts | 20 ++++++++- testing/src/support/api-client.ts | 11 +++++ 7 files changed, 113 insertions(+), 9 deletions(-) diff --git a/apps/api/src/subjects/__tests__/subjects.controller.test.ts b/apps/api/src/subjects/__tests__/subjects.controller.test.ts index 4da5ec400..1292a37ef 100644 --- a/apps/api/src/subjects/__tests__/subjects.controller.test.ts +++ b/apps/api/src/subjects/__tests__/subjects.controller.test.ts @@ -1,10 +1,12 @@ -import type { ParseSchemaPipe } from '@douglasneuroinformatics/libnest'; +import type { ParseSchemaPipe, RequestUser } from '@douglasneuroinformatics/libnest'; import { MockFactory } from '@douglasneuroinformatics/libnest/testing'; import type { MockedInstance } from '@douglasneuroinformatics/libnest/testing'; +import { NotFoundException } from '@nestjs/common'; import { ROUTE_ARGS_METADATA } from '@nestjs/common/constants'; import { Test } from '@nestjs/testing'; import { beforeEach, describe, expect, it } from 'vitest'; +import { AuditLogger } from '@/audit/audit.logger'; import { createAppAbility } from '@/auth/ability.utils'; import { SubjectsController } from '../subjects.controller'; @@ -12,6 +14,8 @@ import { SubjectsService } from '../subjects.service'; const ability = createAppAbility([{ action: 'manage', subject: 'Subject' }]); +const currentUser = { ability, id: 'user-1' } as RequestUser; + /** The pipe the `deleteById` handler applies to its `:id` route parameter. */ function getDeleteIdPipe(): ParseSchemaPipe { const args = Reflect.getMetadata(ROUTE_ARGS_METADATA, SubjectsController, 'deleteById') as { @@ -23,29 +27,62 @@ function getDeleteIdPipe(): ParseSchemaPipe { describe('SubjectsController', () => { let subjectsController: SubjectsController; let subjectsService: MockedInstance; + let auditLogger: MockedInstance; beforeEach(async () => { const moduleRef = await Test.createTestingModule({ controllers: [SubjectsController], - providers: [MockFactory.createForService(SubjectsService)] + providers: [MockFactory.createForService(SubjectsService), MockFactory.createForService(AuditLogger)] }).compile(); subjectsController = moduleRef.get(SubjectsController); subjectsService = moduleRef.get(SubjectsService); + auditLogger = moduleRef.get(AuditLogger); }); it('should delete a subject within what the caller may delete, forcing when asked', async () => { subjectsService.deleteById.mockResolvedValue({ id: 'subject-1' }); - await expect(subjectsController.deleteById('subject-1', true, ability)).resolves.toEqual({ id: 'subject-1' }); + await expect(subjectsController.deleteById('subject-1', true, currentUser)).resolves.toEqual({ id: 'subject-1' }); expect(subjectsService.deleteById).toHaveBeenCalledWith('subject-1', { ability, force: true }); }); it('should leave force unset when the query omits it, so a subject with records is not deleted by default', async () => { - await subjectsController.deleteById('subject-1', undefined, ability); + await subjectsController.deleteById('subject-1', undefined, currentUser); expect(subjectsService.deleteById).toHaveBeenCalledWith('subject-1', { ability, force: undefined }); }); + it('should record a forced delete once, with the number of records it removed, rather than once per record', async () => { + subjectsService.countDeletableRecords.mockResolvedValue(3); + + await subjectsController.deleteById('subject-1', true, currentUser); + + expect(subjectsService.countDeletableRecords).toHaveBeenCalledWith('subject-1', { ability }); + expect(auditLogger.log).toHaveBeenCalledExactlyOnceWith('DELETE', 'SUBJECT', { + groupId: null, + metadata: { force: 'true', recordCount: '3', subjectId: 'subject-1' }, + userId: currentUser.id + }); + }); + + it('should record an unforced delete as removing no records, without reading any', async () => { + await subjectsController.deleteById('subject-1', undefined, currentUser); + + expect(subjectsService.countDeletableRecords).not.toHaveBeenCalled(); + expect(auditLogger.log.mock.lastCall?.[2].metadata).toEqual({ + force: 'false', + recordCount: '0', + subjectId: 'subject-1' + }); + }); + + it('should record no delete that the service refused', async () => { + subjectsService.deleteById.mockRejectedValue(new NotFoundException()); + + await expect(subjectsController.deleteById('subject-1', true, currentUser)).rejects.toThrow(NotFoundException); + expect(auditLogger.log).not.toHaveBeenCalled(); + }); + it('should list the subjects of the requested group within what the caller may read', async () => { const subjects = [{ id: 'subject-1' }]; subjectsService.find.mockResolvedValue(subjects); diff --git a/apps/api/src/subjects/__tests__/subjects.service.test.ts b/apps/api/src/subjects/__tests__/subjects.service.test.ts index 15f2c5b88..1479bbbda 100644 --- a/apps/api/src/subjects/__tests__/subjects.service.test.ts +++ b/apps/api/src/subjects/__tests__/subjects.service.test.ts @@ -37,6 +37,7 @@ describe('SubjectsService', () => { useValue: { $transaction: vi.fn(), instrumentRecord: { + count: vi.fn(), deleteMany: vi.fn(), findMany: vi.fn().mockResolvedValue([]), groupBy: vi.fn() @@ -283,6 +284,19 @@ describe('SubjectsService', () => { }); }); + describe('countDeletableRecords', () => { + it("should count the subject's records the caller may delete, the ones a forced delete removes", async () => { + const ability = createAppAbility([ + { action: 'delete', conditions: { groupId: { in: ['group-1'] } }, subject: 'InstrumentRecord' } + ]); + prismaClient.instrumentRecord.count.mockResolvedValueOnce(2); + await expect(subjectsService.countDeletableRecords('123', { ability })).resolves.toBe(2); + expect(prismaClient.instrumentRecord.count).toHaveBeenCalledExactlyOnceWith({ + where: { AND: [accessibleQuery(ability, 'delete', 'InstrumentRecord')], subjectId: '123' } + }); + }); + }); + describe('deleteById', () => { it('should delete the subject via the subject model and not call $transaction, if force is falsy', async () => { subjectModel.findFirst.mockResolvedValueOnce({ id: '123' }); diff --git a/apps/api/src/subjects/subjects.controller.ts b/apps/api/src/subjects/subjects.controller.ts index ce6227b68..63796df31 100644 --- a/apps/api/src/subjects/subjects.controller.ts +++ b/apps/api/src/subjects/subjects.controller.ts @@ -1,8 +1,10 @@ import { $BooleanLike } from '@douglasneuroinformatics/libjs'; import { ApiOperation, CurrentUser, ParseSchemaPipe, ValidObjectIdPipe } from '@douglasneuroinformatics/libnest'; +import type { RequestUser } from '@douglasneuroinformatics/libnest'; import { Controller, Delete, Get, Param, Query } from '@nestjs/common'; import z from 'zod/v4'; +import { AuditLogger } from '@/audit/audit.logger'; import type { AppAbility } from '@/auth/auth.types'; import { RouteAccess } from '@/core/decorators/route-access.decorator'; @@ -10,18 +12,32 @@ import { SubjectsService } from './subjects.service'; @Controller('subjects') export class SubjectsController { - constructor(private readonly subjectsService: SubjectsService) {} + constructor( + private readonly subjectsService: SubjectsService, + private readonly auditLogger: AuditLogger + ) {} @ApiOperation({ summary: 'Delete Subject' }) @Delete(':id') @RouteAccess({ action: 'delete', subject: 'Subject' }) - deleteById( + async deleteById( @Param('id', new ParseSchemaPipe({ schema: z.string().transform((value) => decodeURIComponent(value)) })) id: string, @Query('force', new ParseSchemaPipe({ isOptional: true, schema: $BooleanLike })) force: boolean | undefined, - @CurrentUser('ability') ability: AppAbility + @CurrentUser() currentUser: RequestUser ) { - return this.subjectsService.deleteById(id, { ability, force }); + const { ability } = currentUser; + // Counted before the delete removes them. Only a forced delete removes any, and only it may + // read them: a caller allowed to delete a subject need not hold any rule on its records. + const recordCount = force ? await this.subjectsService.countDeletableRecords(id, { ability }) : 0; + const result = await this.subjectsService.deleteById(id, { ability, force }); + // A subject can belong to several groups and the route names none, so the entry names no group. + await this.auditLogger.log('DELETE', 'SUBJECT', { + groupId: null, + metadata: { force: String(force === true), recordCount: String(recordCount), subjectId: id }, + userId: currentUser.id + }); + return result; } @ApiOperation({ summary: 'Get All Subjects' }) diff --git a/apps/api/src/subjects/subjects.service.ts b/apps/api/src/subjects/subjects.service.ts index 382e64d1d..40e8f23b1 100644 --- a/apps/api/src/subjects/subjects.service.ts +++ b/apps/api/src/subjects/subjects.service.ts @@ -61,6 +61,13 @@ export class SubjectsService { }); } + /** How many records a forced delete of the subject removes with it, with the same scope it removes them in. */ + async countDeletableRecords(id: string, { ability }: EntityOperationOptions = {}) { + return this.prismaClient.instrumentRecord.count({ + where: { AND: [accessibleQuery(ability, 'delete', 'InstrumentRecord')], subjectId: id } + }); + } + async create({ id, ...data diff --git a/packages/schemas/src/audit/audit.ts b/packages/schemas/src/audit/audit.ts index fdc6310be..1970b9755 100644 --- a/packages/schemas/src/audit/audit.ts +++ b/packages/schemas/src/audit/audit.ts @@ -30,6 +30,7 @@ export type AuditedAction = (type export const AUDITED_ACTIONS = { ASSIGNMENT: ['CREATE', 'DELETE', 'SEND_EMAIL', 'UPDATE'], INSTRUMENT: ['ARCHIVE', 'CREATE', 'DELETE', 'UNARCHIVE'], + SUBJECT: ['DELETE'], USER: ['ARCHIVE', 'CREATE', 'LOGIN', 'UNARCHIVE', 'UPDATE'] } as const satisfies { [TEntity in $AuditLogEntity]?: readonly $AuditLogAction[] }; diff --git a/testing/src/specs/admin-audit-logs.spec.ts b/testing/src/specs/admin-audit-logs.spec.ts index f19c6ee14..36a64d45f 100644 --- a/testing/src/specs/admin-audit-logs.spec.ts +++ b/testing/src/specs/admin-audit-logs.spec.ts @@ -1,7 +1,7 @@ import type { $AuditLog } from '@opendatacapture/schemas/audit'; import { ApiClient } from '../support/api-client'; -import { ADMIN } from '../support/constants'; +import { ADMIN, HAPPINESS_RECORD } from '../support/constants'; import { expect, test } from '../support/fixtures'; test.describe('admin audit logs', () => { @@ -121,6 +121,8 @@ test.describe('admin audit logs fetching', () => { }); }); +const HAPPINESS = 'DNP_HAPPINESS_QUESTIONNAIRE'; + /** The entry among `entries` whose metadata names `value` under `key`, which is how an entry says what it acted on. */ function entryNaming(entries: $AuditLog[], key: string, value: string) { return entries.find(({ metadata }) => metadata?.[key] === value); @@ -219,4 +221,20 @@ test.describe('admin audit log coverage', () => { expect(entryNaming(entries, 'instrumentId', seriesId)?.metadata?.title).toBe(`Deleted Series ${uniqueId}`); }); }); + + test.describe('subjects', () => { + test('should record a forced delete once, with the number of records it removed', async ({ api, uniqueId }) => { + const group = await api.createGroup(); + const session = await api.createSession(group.id, { id: `deleted-${uniqueId}` }); + const instrumentId = await api.findInstrumentIdByName(HAPPINESS); + await api.createRecord(group.id, instrumentId, session, HAPPINESS_RECORD); + await api.createRecord(group.id, instrumentId, session, HAPPINESS_RECORD); + await api.deleteSubject(session.subjectId, { force: true }); + + const entries = await api.findAuditLogs({ action: 'DELETE', entity: 'SUBJECT' }); + const matching = entries.filter(({ metadata }) => metadata?.subjectId === session.subjectId); + expect(matching).toHaveLength(1); + expect(matching[0]?.metadata).toMatchObject({ force: 'true', recordCount: '2' }); + }); + }); }); diff --git a/testing/src/support/api-client.ts b/testing/src/support/api-client.ts index c94027e10..e3ff226a6 100644 --- a/testing/src/support/api-client.ts +++ b/testing/src/support/api-client.ts @@ -234,6 +234,17 @@ export class ApiClient { ); } + /** Deletes a subject over the API, which no page of the web app does; `force` also removes its records. */ + async deleteSubject(id: string, { force }: { force: boolean }): Promise { + await this.expectOk( + this.request.delete(`${API}/subjects/${encodeURIComponent(id)}`, { + headers: this.authHeaders, + params: { force } + }), + `delete subject '${id}'` + ); + } + /** * The languages the instance currently offers. Read rather than assumed because * `admin-settings.spec.ts` turns one on and off, in a worker of its own. From addfff5bc9985a68293a7b0c6d8ddbbf2a986bc9 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Sun, 8 Nov 2026 20:42:24 -0500 Subject: [PATCH 09/10] feat(api): record instrument record updates and deletions in the audit log Updating a record writes an entry naming the data fields the update changed, never their values, in the record's group; an update that changed nothing writes none. Deleting a record writes an entry naming the record, its instrument and its subject. The delete is logged in the controller, because the gateway synchronizer calls the same service method to roll back. changedFields now refuses a value with no fields, since record data is typed as any JSON value. Co-Authored-By: Claude Opus 5.5 --- .../src/audit/__tests__/audit.utils.test.ts | 5 ++ apps/api/src/audit/audit.utils.ts | 18 +++-- .../instrument-records.controller.test.ts | 39 ++++++++--- .../instrument-records.service.test.ts | 67 +++++++++++++++---- .../instrument-records.controller.ts | 21 ++++-- .../instrument-records.service.ts | 20 ++++-- packages/schemas/src/audit/audit.ts | 1 + testing/src/specs/admin-audit-logs.spec.ts | 36 ++++++++++ testing/src/support/api-client.ts | 17 +++++ 9 files changed, 191 insertions(+), 33 deletions(-) diff --git a/apps/api/src/audit/__tests__/audit.utils.test.ts b/apps/api/src/audit/__tests__/audit.utils.test.ts index 9f0ca3ac4..d4ba5e6e3 100644 --- a/apps/api/src/audit/__tests__/audit.utils.test.ts +++ b/apps/api/src/audit/__tests__/audit.utils.test.ts @@ -44,4 +44,9 @@ describe('changedFields', () => { 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); + }); }); diff --git a/apps/api/src/audit/audit.utils.ts b/apps/api/src/audit/audit.utils.ts index 1781c8ce7..0f12ef3e8 100644 --- a/apps/api/src/audit/audit.utils.ts +++ b/apps/api/src/audit/audit.utils.ts @@ -12,10 +12,20 @@ const compareStringListsAsSets = (value: unknown, other: unknown): boolean | und return undefined; }; -/** The names of the fields that differ between a row before and after an update, sorted. */ -export function changedFields(before: object, after: object): string[] { - const beforeValues = new Map(Object.entries(before)); - const afterValues = new Map(Object.entries(after)); +const fieldValues = (value: unknown): Map => { + 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'); diff --git a/apps/api/src/instrument-records/__tests__/instrument-records.controller.test.ts b/apps/api/src/instrument-records/__tests__/instrument-records.controller.test.ts index a2421057b..c1f7ecb1e 100644 --- a/apps/api/src/instrument-records/__tests__/instrument-records.controller.test.ts +++ b/apps/api/src/instrument-records/__tests__/instrument-records.controller.test.ts @@ -1,11 +1,12 @@ -import type { ParseSchemaPipe } from '@douglasneuroinformatics/libnest'; +import type { ParseSchemaPipe, RequestUser } from '@douglasneuroinformatics/libnest'; import { MockFactory } from '@douglasneuroinformatics/libnest/testing'; import type { MockedInstance } from '@douglasneuroinformatics/libnest/testing'; -import { BadRequestException } from '@nestjs/common'; +import { BadRequestException, NotFoundException } from '@nestjs/common'; import { ROUTE_ARGS_METADATA } from '@nestjs/common/constants'; import { Test } from '@nestjs/testing'; import { beforeEach, describe, expect, it } from 'vitest'; +import { AuditLogger } from '@/audit/audit.logger'; import { createAppAbility } from '@/auth/ability.utils'; import { InstrumentRecordsController } from '../instrument-records.controller'; @@ -13,6 +14,10 @@ import { InstrumentRecordsService } from '../instrument-records.service'; const ability = createAppAbility([{ action: 'manage', subject: 'all' }]); +const currentUser = { ability, id: 'user-1' } as RequestUser; + +const deletedRecord = { groupId: 'group-1', id: 'record-1', instrumentId: 'instrument-1', subjectId: 'subject-1' }; + /** The pipe the `find` handler applies to its `minDate` query parameter. */ function getMinDatePipe(): ParseSchemaPipe { const args = Reflect.getMetadata(ROUTE_ARGS_METADATA, InstrumentRecordsController, 'find') as { @@ -24,14 +29,16 @@ function getMinDatePipe(): ParseSchemaPipe { describe('InstrumentRecordsController', () => { let instrumentRecordsController: InstrumentRecordsController; let instrumentRecordsService: MockedInstance; + let auditLogger: MockedInstance; beforeEach(async () => { const moduleRef = await Test.createTestingModule({ controllers: [InstrumentRecordsController], - providers: [MockFactory.createForService(InstrumentRecordsService)] + providers: [MockFactory.createForService(InstrumentRecordsService), MockFactory.createForService(AuditLogger)] }).compile(); instrumentRecordsController = moduleRef.get(InstrumentRecordsController); instrumentRecordsService = moduleRef.get(InstrumentRecordsService); + auditLogger = moduleRef.get(AuditLogger); }); it('should create the record scoped to the caller ability', async () => { @@ -98,11 +105,27 @@ describe('InstrumentRecordsController', () => { ); it('should delete the record scoped to the caller ability and return no content', async () => { - instrumentRecordsService.deleteById.mockResolvedValueOnce(undefined); - await expect(instrumentRecordsController.deleteById('record-1', ability)).resolves.toBeUndefined(); + instrumentRecordsService.deleteById.mockResolvedValueOnce(deletedRecord); + await expect(instrumentRecordsController.deleteById('record-1', currentUser)).resolves.toBeUndefined(); expect(instrumentRecordsService.deleteById).toHaveBeenCalledWith('record-1', { ability }); }); + it('should record who deleted the record, in its group, naming what it held data on', async () => { + instrumentRecordsService.deleteById.mockResolvedValueOnce(deletedRecord); + await instrumentRecordsController.deleteById('record-1', currentUser); + expect(auditLogger.log).toHaveBeenCalledExactlyOnceWith('DELETE', 'INSTRUMENT_RECORD', { + groupId: 'group-1', + metadata: { instrumentId: 'instrument-1', recordId: 'record-1', subjectId: 'subject-1' }, + userId: currentUser.id + }); + }); + + it('should record no delete that the service refused', async () => { + instrumentRecordsService.deleteById.mockRejectedValueOnce(new NotFoundException()); + await expect(instrumentRecordsController.deleteById('record-1', currentUser)).rejects.toThrow(NotFoundException); + expect(auditLogger.log).not.toHaveBeenCalled(); + }); + it('should export the records of the requested group, scoped to the caller ability', async () => { await instrumentRecordsController.exportRecords(ability, 'group-1'); expect(instrumentRecordsService.exportRecords).toHaveBeenCalledWith({ groupId: 'group-1' }, { ability }); @@ -140,9 +163,9 @@ describe('InstrumentRecordsController', () => { expect(instrumentRecordsService.summarizeBySeries).toHaveBeenCalledWith({ groupId: 'group-1' }, { ability }); }); - it('should update the record with the unwrapped data, scoped to the caller ability', async () => { - await instrumentRecordsController.updateById('record-1', { data: { score: 2 } }, ability); - expect(instrumentRecordsService.updateById).toHaveBeenCalledWith('record-1', { score: 2 }, { ability }); + it('should update the record with the unwrapped data, as the current user so the change is scoped and recorded', async () => { + await instrumentRecordsController.updateById('record-1', { data: { score: 2 } }, currentUser); + expect(instrumentRecordsService.updateById).toHaveBeenCalledWith('record-1', { score: 2 }, currentUser); }); it('should find the record scoped to the caller ability', async () => { diff --git a/apps/api/src/instrument-records/__tests__/instrument-records.service.test.ts b/apps/api/src/instrument-records/__tests__/instrument-records.service.test.ts index c64e3a2fa..fd9371a99 100644 --- a/apps/api/src/instrument-records/__tests__/instrument-records.service.test.ts +++ b/apps/api/src/instrument-records/__tests__/instrument-records.service.test.ts @@ -1,7 +1,7 @@ import { EventEmitter } from 'node:events'; import { Worker } from 'node:worker_threads'; -import type { Model } from '@douglasneuroinformatics/libnest'; +import type { Model, RequestUser } from '@douglasneuroinformatics/libnest'; import { getModelToken, LoggingService, PRISMA_CLIENT_TOKEN } from '@douglasneuroinformatics/libnest'; import { MockFactory } from '@douglasneuroinformatics/libnest/testing'; import type { MockedInstance } from '@douglasneuroinformatics/libnest/testing'; @@ -16,6 +16,7 @@ import { Test } from '@nestjs/testing'; import { DEFAULT_GROUP_NAME } from '@opendatacapture/schemas/core'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { AuditLogger } from '@/audit/audit.logger'; import { AbilityFactory } from '@/auth/ability.factory'; import { accessibleQuery, createAppAbility } from '@/auth/ability.utils'; import { StorageService } from '@/storage/storage.service'; @@ -56,6 +57,7 @@ function useFakeWorker() { } describe('InstrumentRecordsService', () => { + let auditLogger: MockedInstance; let groupsService: MockedInstance; let instrumentMeasuresService: MockedInstance; let loggingService: MockedInstance; @@ -93,6 +95,7 @@ describe('InstrumentRecordsService', () => { }) }, MockFactory.createForModelToken(getModelToken('Session')), + MockFactory.createForService(AuditLogger), MockFactory.createForService(GroupsService), MockFactory.createForService(UsersService), MockFactory.createForService(InstrumentMeasuresService), @@ -103,6 +106,7 @@ describe('InstrumentRecordsService', () => { ] }).compile(); + auditLogger = moduleRef.get(AuditLogger); groupsService = moduleRef.get(GroupsService); instrumentMeasuresService = moduleRef.get(InstrumentMeasuresService); fileModel = moduleRef.get(getModelToken('InstrumentRecordFile')); @@ -1199,8 +1203,18 @@ describe('InstrumentRecordsService', () => { }); describe('updateById', () => { + const currentUser = { + ability: createAppAbility([{ action: 'manage', subject: 'all' }]), + id: 'user-1' + } as RequestUser; + const useRecord = (data: unknown) => { - instrumentRecordModel.findFirst.mockResolvedValueOnce({ data, id: 'record-1', instrumentId: 'instrument-1' }); + instrumentRecordModel.findFirst.mockResolvedValueOnce({ + data, + groupId: 'group-1', + id: 'record-1', + instrumentId: 'instrument-1' + }); }; const useInstrument = (instance: object = {}) => { @@ -1219,7 +1233,9 @@ describe('InstrumentRecordsService', () => { it('should throw a NotFoundException when the record does not exist', async () => { instrumentRecordModel.findFirst.mockResolvedValueOnce(null); - await expect(instrumentRecordsService.updateById('record-1', {})).rejects.toBeInstanceOf(NotFoundException); + await expect(instrumentRecordsService.updateById('record-1', {}, currentUser)).rejects.toBeInstanceOf( + NotFoundException + ); expect(instrumentRecordModel.update).not.toHaveBeenCalled(); }); @@ -1227,7 +1243,7 @@ describe('InstrumentRecordsService', () => { it('should reject an object update to a record whose data is an array', async () => { useRecord([{ answer: 1 }]); - await expect(instrumentRecordsService.updateById('record-1', { answer: 2 })).rejects.toBeInstanceOf( + await expect(instrumentRecordsService.updateById('record-1', { answer: 2 }, currentUser)).rejects.toBeInstanceOf( BadRequestException ); @@ -1238,7 +1254,7 @@ describe('InstrumentRecordsService', () => { useRecord({ answer: 1, comment: 'kept' }); useInstrument(); - await instrumentRecordsService.updateById('record-1', { answer: 2 }); + await instrumentRecordsService.updateById('record-1', { answer: 2 }, currentUser); expect(instrumentRecordModel.update.mock.lastCall?.[0]).toMatchObject({ data: { computedMeasures: null, data: { answer: 2, comment: 'kept' } } @@ -1250,7 +1266,7 @@ describe('InstrumentRecordsService', () => { useRecord({ answers: [1, 2, 3] }); useInstrument(); - await instrumentRecordsService.updateById('record-1', { answers: [9] }); + await instrumentRecordsService.updateById('record-1', { answers: [9] }, currentUser); expect(instrumentRecordModel.update.mock.lastCall?.[0]).toMatchObject({ data: { data: { answers: [1, 2, 3] } } }); }); @@ -1259,7 +1275,7 @@ describe('InstrumentRecordsService', () => { useRecord([{ answer: 1, comment: 'kept' }]); useInstrument(); - await instrumentRecordsService.updateById('record-1', [{ answer: 2 }]); + await instrumentRecordsService.updateById('record-1', [{ answer: 2 }], currentUser); expect(instrumentRecordModel.update.mock.lastCall?.[0]).toMatchObject({ data: { data: [{ answer: 2, comment: 'kept' }] } @@ -1273,9 +1289,11 @@ describe('InstrumentRecordsService', () => { validationSchema: { safeParseAsync: () => Promise.resolve({ error: { issues }, success: false }) } }); - await expect(instrumentRecordsService.updateById('record-1', { answer: 'x' })).rejects.toMatchObject({ - response: { issues, message: 'Merged data does not match validation schema' } - }); + await expect(instrumentRecordsService.updateById('record-1', { answer: 'x' }, currentUser)).rejects.toMatchObject( + { + response: { issues, message: 'Merged data does not match validation schema' } + } + ); expect(instrumentRecordModel.update).not.toHaveBeenCalled(); }); @@ -1285,7 +1303,7 @@ describe('InstrumentRecordsService', () => { useInstrument({ measures }); instrumentMeasuresService.computeMeasures.mockReturnValueOnce({ score: 2 }); - await instrumentRecordsService.updateById('record-1', { answer: 2 }); + await instrumentRecordsService.updateById('record-1', { answer: 2 }, currentUser); expect(instrumentMeasuresService.computeMeasures).toHaveBeenCalledWith(measures, { answer: 2 }); expect(instrumentRecordModel.update.mock.lastCall?.[0]).toMatchObject({ @@ -1302,13 +1320,38 @@ describe('InstrumentRecordsService', () => { useRecord({ answer: 1 }); useInstrument(); - await instrumentRecordsService.updateById('record-1', { answer: 2 }, { ability }); + await instrumentRecordsService.updateById('record-1', { answer: 2 }, { ...currentUser, ability }); expect(instrumentRecordModel.update.mock.lastCall?.[0].where).toStrictEqual({ AND: [accessibleQuery(ability, 'delete', 'InstrumentRecord')], id: 'record-1' }); }); + + it("should record the data fields the update changed, in the record's group, and never their values", async () => { + useRecord({ answer: 1, comment: 'kept' }); + useInstrument(); + + await instrumentRecordsService.updateById('record-1', { answer: 2, comment: 'kept' }, currentUser); + + expect(auditLogger.logUpdate).toHaveBeenCalledExactlyOnceWith('INSTRUMENT_RECORD', ['answer'], { + groupId: 'group-1', + metadata: { recordId: 'record-1' }, + userId: currentUser.id + }); + }); + + it('should record no update the schema rejected', async () => { + useRecord({ answer: 1 }); + useInstrument({ + validationSchema: { safeParseAsync: () => Promise.resolve({ error: { issues: [] }, success: false }) } + }); + + await expect(instrumentRecordsService.updateById('record-1', { answer: 'x' }, currentUser)).rejects.toThrow( + BadRequestException + ); + expect(auditLogger.logUpdate).not.toHaveBeenCalled(); + }); }); describe('summarizeBySubject', () => { diff --git a/apps/api/src/instrument-records/instrument-records.controller.ts b/apps/api/src/instrument-records/instrument-records.controller.ts index 440bdbf20..54c4c4b41 100644 --- a/apps/api/src/instrument-records/instrument-records.controller.ts +++ b/apps/api/src/instrument-records/instrument-records.controller.ts @@ -1,6 +1,7 @@ /* eslint-disable perfectionist/sort-classes -- the single-segment @Get('linear-model') must stay above @Get(':id'), which alphabetical order would put below it, where :id would match it first */ import { ApiOperation, CurrentUser, ParseSchemaPipe, ValidObjectIdPipe } from '@douglasneuroinformatics/libnest'; +import type { RequestUser } from '@douglasneuroinformatics/libnest'; import { Body, Controller, @@ -22,6 +23,7 @@ import { $UploadInstrumentRecordsData } from '@opendatacapture/schemas/instrument-records'; +import { AuditLogger } from '@/audit/audit.logger'; import type { AppAbility } from '@/auth/auth.types'; import { RouteAccess } from '@/core/decorators/route-access.decorator'; import { MsgpackInterceptor } from '@/core/interceptors/msgpack.interceptor'; @@ -30,7 +32,10 @@ import { InstrumentRecordsService } from './instrument-records.service'; @Controller('instrument-records') export class InstrumentRecordsController { - constructor(private readonly instrumentRecordsService: InstrumentRecordsService) {} + constructor( + private readonly instrumentRecordsService: InstrumentRecordsService, + private readonly auditLogger: AuditLogger + ) {} @ApiOperation({ summary: 'Create Instrument Record' }) @Post() @@ -76,8 +81,14 @@ export class InstrumentRecordsController { @Delete(':id') @HttpCode(HttpStatus.NO_CONTENT) @RouteAccess({ action: 'delete', subject: 'InstrumentRecord' }) - async deleteById(@Param('id', ValidObjectIdPipe) id: string, @CurrentUser('ability') ability: AppAbility) { - await this.instrumentRecordsService.deleteById(id, { ability }); + async deleteById(@Param('id', ValidObjectIdPipe) id: string, @CurrentUser() currentUser: RequestUser) { + const record = await this.instrumentRecordsService.deleteById(id, { ability: currentUser.ability }); + // Logged here rather than in the service, which the gateway synchronizer also calls to roll back. + await this.auditLogger.log('DELETE', 'INSTRUMENT_RECORD', { + groupId: record.groupId, + metadata: { instrumentId: record.instrumentId, recordId: id, subjectId: record.subjectId }, + userId: currentUser.id + }); } @ApiOperation({ summary: 'Export Records' }) @@ -126,9 +137,9 @@ export class InstrumentRecordsController { updateById( @Param('id', ValidObjectIdPipe) id: string, @Body() { data }: $UpdateInstrumentRecordData, - @CurrentUser('ability') ability: AppAbility + @CurrentUser() currentUser: RequestUser ) { - return this.instrumentRecordsService.updateById(id, data, { ability }); + return this.instrumentRecordsService.updateById(id, data, currentUser); } @ApiOperation({ summary: 'Get Instrument Record' }) diff --git a/apps/api/src/instrument-records/instrument-records.service.ts b/apps/api/src/instrument-records/instrument-records.service.ts index ff655a94e..4f1f0dde9 100644 --- a/apps/api/src/instrument-records/instrument-records.service.ts +++ b/apps/api/src/instrument-records/instrument-records.service.ts @@ -4,7 +4,7 @@ import { Worker } from 'node:worker_threads'; import { replacer, reviver } from '@douglasneuroinformatics/libjs'; import { InjectModel, InjectPrismaClient, LoggingService } from '@douglasneuroinformatics/libnest'; -import type { Model } from '@douglasneuroinformatics/libnest'; +import type { Model, RequestUser } from '@douglasneuroinformatics/libnest'; import { linearRegression } from '@douglasneuroinformatics/libstats'; import { BadRequestException, @@ -28,8 +28,10 @@ import type { } from '@opendatacapture/schemas/instrument-records'; import { Prisma } from '@prisma/client'; import type { InstrumentRecord as PrismaInstrumentRecord } from '@prisma/client'; -import { isNumber, mergeWith, pickBy } from 'lodash-es'; +import { cloneDeep, isNumber, mergeWith, pickBy } from 'lodash-es'; +import { AuditLogger } from '@/audit/audit.logger'; +import { changedFields } from '@/audit/audit.utils'; import { accessibleQuery, forcedAppSubject } from '@/auth/ability.utils'; import type { AppAbility } from '@/auth/auth.types'; import type { RuntimePrismaClient } from '@/core/prisma'; @@ -74,6 +76,7 @@ export class InstrumentRecordsService { private readonly loggingService: LoggingService, @InjectModel('InstrumentRecord') private readonly instrumentRecordModel: Model<'InstrumentRecord'>, @InjectModel('Session') private readonly sessionModel: Model<'Session'>, + private readonly auditLogger: AuditLogger, private readonly groupsService: GroupsService, private readonly usersService: UsersService, private readonly instrumentMeasuresService: InstrumentMeasuresService, @@ -434,7 +437,8 @@ export class InstrumentRecordsService { })); } - async updateById(id: string, data: unknown[] | { [key: string]: unknown }, { ability }: EntityOperationOptions = {}) { + async updateById(id: string, data: unknown[] | { [key: string]: unknown }, currentUser: RequestUser) { + const { ability } = currentUser; const instrumentRecord = await this.instrumentRecordModel.findFirst({ where: { id } }); @@ -449,6 +453,8 @@ export class InstrumentRecordsService { // all records must be attached to scalar instruments const instrument = (await this.getInstrumentById(instrumentRecord.instrumentId)) as ScalarInstrument; + // `mergeWith` writes into the data it merges into, so what was stored is copied first. + const storedData = cloneDeep(instrumentRecord.data); const updatedData = mergeWith(instrumentRecord.data, data, (updatedValue: unknown, sourceValue: unknown) => { if (Array.isArray(sourceValue)) { return updatedValue; @@ -464,7 +470,7 @@ export class InstrumentRecordsService { }); } - return this.instrumentRecordModel.update({ + const updated = await this.instrumentRecordModel.update({ data: { computedMeasures: instrument.measures ? this.instrumentMeasuresService.computeMeasures(instrument.measures, parseResult.data as Json) @@ -473,6 +479,12 @@ export class InstrumentRecordsService { }, where: { AND: [accessibleQuery(ability, 'delete', 'InstrumentRecord')], id } }); + await this.auditLogger.logUpdate('INSTRUMENT_RECORD', changedFields(storedData, parseResult.data), { + groupId: instrumentRecord.groupId, + metadata: { recordId: id }, + userId: currentUser.id + }); + return updated; } async upload( diff --git a/packages/schemas/src/audit/audit.ts b/packages/schemas/src/audit/audit.ts index 1970b9755..955e4339c 100644 --- a/packages/schemas/src/audit/audit.ts +++ b/packages/schemas/src/audit/audit.ts @@ -30,6 +30,7 @@ export type AuditedAction = (type export const AUDITED_ACTIONS = { ASSIGNMENT: ['CREATE', 'DELETE', 'SEND_EMAIL', 'UPDATE'], INSTRUMENT: ['ARCHIVE', 'CREATE', 'DELETE', 'UNARCHIVE'], + INSTRUMENT_RECORD: ['DELETE', 'UPDATE'], SUBJECT: ['DELETE'], USER: ['ARCHIVE', 'CREATE', 'LOGIN', 'UNARCHIVE', 'UPDATE'] } as const satisfies { [TEntity in $AuditLogEntity]?: readonly $AuditLogAction[] }; diff --git a/testing/src/specs/admin-audit-logs.spec.ts b/testing/src/specs/admin-audit-logs.spec.ts index 36a64d45f..c0659c256 100644 --- a/testing/src/specs/admin-audit-logs.spec.ts +++ b/testing/src/specs/admin-audit-logs.spec.ts @@ -237,4 +237,40 @@ test.describe('admin audit log coverage', () => { expect(matching[0]?.metadata).toMatchObject({ force: 'true', recordCount: '2' }); }); }); + + test.describe('records', () => { + test('should record which data fields an update changed, and none of their values', async ({ api, uniqueId }) => { + const group = await api.createGroup(); + const session = await api.createSession(group.id, { id: `edited-${uniqueId}` }); + const record = await api.createRecord( + group.id, + await api.findInstrumentIdByName(HAPPINESS), + session, + HAPPINESS_RECORD + ); + await api.updateRecord(record.id, { personalLifeSatisfaction: 2 }); + + const entries = await api.findAuditLogs({ action: 'UPDATE', entity: 'INSTRUMENT_RECORD' }); + const entry = entryNaming(entries, 'recordId', record.id); + expect(entry?.metadata).toStrictEqual({ fields: 'personalLifeSatisfaction', recordId: record.id }); + expect(entry?.group?.name).toBe(group.name); + }); + + test('should record who deleted a record, in its group', async ({ api, uniqueId }) => { + const group = await api.createGroup(); + const session = await api.createSession(group.id, { id: `removed-${uniqueId}` }); + const record = await api.createRecord( + group.id, + await api.findInstrumentIdByName(HAPPINESS), + session, + HAPPINESS_RECORD + ); + await api.deleteRecord(record.id); + + const entries = await api.findAuditLogs({ action: 'DELETE', entity: 'INSTRUMENT_RECORD' }); + const entry = entryNaming(entries, 'recordId', record.id); + expect(entry?.metadata?.subjectId).toBe(session.subjectId); + expect(entry?.group?.name).toBe(group.name); + }); + }); }); diff --git a/testing/src/support/api-client.ts b/testing/src/support/api-client.ts index e3ff226a6..8e8f6fe88 100644 --- a/testing/src/support/api-client.ts +++ b/testing/src/support/api-client.ts @@ -11,6 +11,7 @@ import type { $CreateGroupData, Group } from '@opendatacapture/schemas/group'; import type { InstrumentInfo } from '@opendatacapture/schemas/instrument'; import type { $CreateInstrumentRecordData, + $UpdateInstrumentRecordData, InstrumentRecord, UploadInstrumentRecordsData } from '@opendatacapture/schemas/instrument-records'; @@ -226,6 +227,14 @@ export class ApiClient { ); } + /** Deletes a record over the API, which no page of the web app does. */ + async deleteRecord(id: string): Promise { + await this.expectOk( + this.request.delete(`${API}/instrument-records/${id}`, { headers: this.authHeaders }), + `delete record '${id}'` + ); + } + /** Deletes a series through the route the web app's instrument management uses. */ async deleteSeries(id: string): Promise { await this.expectOk( @@ -403,6 +412,14 @@ export class ApiClient { ); } + /** Merges `data` into a record's data over the API, which no page of the web app does. */ + async updateRecord(id: string, data: $UpdateInstrumentRecordData['data']): Promise { + await this.expectOk( + this.request.patch(`${API}/instrument-records/${id}`, { data: { data }, headers: this.authHeaders }), + `update record '${id}'` + ); + } + /** Updates the account this client is signed in as, through the route the profile page uses. */ async updateSelf(id: string, data: $SelfUpdateUserData): Promise { await this.expectOk( From b4175dea1a7cda583b600c40a1515747fde5e19a Mon Sep 17 00:00:00 2001 From: joshunrau Date: Sun, 8 Nov 2026 20:46:37 -0500 Subject: [PATCH 10/10] feat(api): record group creation and updates in the audit log Creating a group writes an entry in the new group, and updating one writes an entry naming the fields the update changed. An update that changed nothing writes none, so a resubmitted settings form adds no entry. Creation is logged in the controller, because demo seeding calls the same service method with no one acting. Co-Authored-By: Claude Opus 5.5 --- .../__tests__/groups.controller.test.ts | 43 +++++- .../groups/__tests__/groups.service.test.ts | 144 +++++++++++++----- apps/api/src/groups/groups.controller.ts | 18 ++- apps/api/src/groups/groups.service.ts | 34 +++-- packages/schemas/src/audit/audit.ts | 1 + testing/src/specs/admin-audit-logs.spec.ts | 30 ++++ testing/src/support/api-client.ts | 16 +- 7 files changed, 215 insertions(+), 71 deletions(-) diff --git a/apps/api/src/groups/__tests__/groups.controller.test.ts b/apps/api/src/groups/__tests__/groups.controller.test.ts index 7f1723695..dd3b58f26 100644 --- a/apps/api/src/groups/__tests__/groups.controller.test.ts +++ b/apps/api/src/groups/__tests__/groups.controller.test.ts @@ -1,12 +1,15 @@ import { LoggingService } from '@douglasneuroinformatics/libnest'; +import type { RequestUser } from '@douglasneuroinformatics/libnest'; import { MockFactory } from '@douglasneuroinformatics/libnest/testing'; import type { MockedInstance } from '@douglasneuroinformatics/libnest/testing'; +import { ConflictException } from '@nestjs/common'; import { Reflector } from '@nestjs/core'; import { Test } from '@nestjs/testing'; import type { Group } from '@opendatacapture/schemas/group'; import type { BasePermissionLevel } from '@opendatacapture/schemas/user'; import { beforeEach, describe, expect, it } from 'vitest'; +import { AuditLogger } from '@/audit/audit.logger'; import { AbilityFactory } from '@/auth/ability.factory'; import { createAppAbility } from '@/auth/ability.utils'; import { ROUTE_ACCESS_METADATA_KEY } from '@/core/decorators/route-access.decorator'; @@ -17,17 +20,21 @@ import { GroupsService } from '../groups.service'; const createAbility = () => createAppAbility([{ action: 'read', subject: 'Group' }]); +const currentUser = { ability: createAppAbility([{ action: 'manage', subject: 'all' }]), id: 'admin-1' } as RequestUser; + describe('GroupsController', () => { let groupsController: GroupsController; let groupsService: MockedInstance; + let auditLogger: MockedInstance; beforeEach(async () => { const moduleRef = await Test.createTestingModule({ controllers: [GroupsController], - providers: [MockFactory.createForService(GroupsService)] + providers: [MockFactory.createForService(GroupsService), MockFactory.createForService(AuditLogger)] }).compile(); groupsController = moduleRef.get(GroupsController); groupsService = moduleRef.get(GroupsService); + auditLogger = moduleRef.get(AuditLogger); }); it('should be defined', () => { @@ -35,10 +42,33 @@ describe('GroupsController', () => { }); describe('create', () => { + beforeEach(() => { + groupsService.create.mockResolvedValue({ id: 'group-1', name: 'Test Group' }); + }); + it('should pass the provided object to the groups service', async () => { - await groupsController.create({ name: 'Test Group', type: 'CLINICAL' }); + await groupsController.create({ name: 'Test Group', type: 'CLINICAL' }, currentUser); expect(groupsService.create.mock.lastCall?.[0]).toMatchObject({ name: 'Test Group' }); }); + + it('should record who created the group, in the new group, since the service records nothing', async () => { + await expect(groupsController.create({ name: 'Test Group', type: 'CLINICAL' }, currentUser)).resolves.toEqual({ + id: 'group-1', + name: 'Test Group' + }); + expect(auditLogger.log).toHaveBeenCalledExactlyOnceWith('CREATE', 'GROUP', { + groupId: 'group-1', + userId: currentUser.id + }); + }); + + it('should record no creation the service refused', async () => { + groupsService.create.mockRejectedValueOnce(new ConflictException()); + await expect(groupsController.create({ name: 'Test Group', type: 'CLINICAL' }, currentUser)).rejects.toThrow( + ConflictException + ); + expect(auditLogger.log).not.toHaveBeenCalled(); + }); }); describe('findAll', () => { @@ -126,15 +156,14 @@ describe('GroupsController', () => { }); describe('updateById', () => { - it("should forward the update scoped to the current user's ability", async () => { - const ability = createAbility(); - await groupsController.updateById('123', { name: 'Renamed Group' }, ability); - expect(groupsService.updateById).toHaveBeenCalledWith('123', { name: 'Renamed Group' }, { ability }); + it('should forward the update with the current user, so it is scoped and recorded', async () => { + await groupsController.updateById('123', { name: 'Renamed Group' }, currentUser); + expect(groupsService.updateById).toHaveBeenCalledWith('123', { name: 'Renamed Group' }, currentUser); }); it('should return the group updated by the groups service', async () => { groupsService.updateById.mockResolvedValueOnce({ name: 'Renamed Group' }); - await expect(groupsController.updateById('123', { name: 'Renamed Group' })).resolves.toMatchObject({ + await expect(groupsController.updateById('123', { name: 'Renamed Group' }, currentUser)).resolves.toMatchObject({ name: 'Renamed Group' }); }); diff --git a/apps/api/src/groups/__tests__/groups.service.test.ts b/apps/api/src/groups/__tests__/groups.service.test.ts index c158863ed..3d54e37aa 100644 --- a/apps/api/src/groups/__tests__/groups.service.test.ts +++ b/apps/api/src/groups/__tests__/groups.service.test.ts @@ -1,4 +1,4 @@ -import type { Model } from '@douglasneuroinformatics/libnest'; +import type { Model, RequestUser } from '@douglasneuroinformatics/libnest'; import { getModelToken } from '@douglasneuroinformatics/libnest'; import { MockFactory } from '@douglasneuroinformatics/libnest/testing'; import type { MockedInstance } from '@douglasneuroinformatics/libnest/testing'; @@ -7,10 +7,13 @@ import { Test } from '@nestjs/testing'; import { PrismaClientKnownRequestError } from '@prisma/client/runtime/library'; import { beforeEach, describe, expect, it } from 'vitest'; +import { AuditLogger } from '@/audit/audit.logger'; import { createAppAbility } from '@/auth/ability.utils'; import { GroupsService } from '../groups.service'; +const admin = { ability: createAppAbility([{ action: 'manage', subject: 'all' }]), id: 'admin-1' } as RequestUser; + /** A minimal template satisfying `$GroupEmailTemplate`, which requires renderable content. */ const template = (id: string, name: string) => ({ body: { en: 'Link {{url}}' }, @@ -21,6 +24,7 @@ const template = (id: string, name: string) => ({ describe('GroupsService', () => { let groupsService: GroupsService; + let auditLogger: MockedInstance; let groupModel: MockedInstance>; let instrumentModel: MockedInstance>; @@ -28,10 +32,12 @@ describe('GroupsService', () => { const moduleRef = await Test.createTestingModule({ providers: [ GroupsService, + MockFactory.createForService(AuditLogger), MockFactory.createForModelToken(getModelToken('Group')), MockFactory.createForModelToken(getModelToken('Instrument')) ] }).compile(); + auditLogger = moduleRef.get(AuditLogger); groupModel = moduleRef.get(getModelToken('Group')); instrumentModel = moduleRef.get(getModelToken('Instrument')); groupsService = moduleRef.get(GroupsService); @@ -110,15 +116,49 @@ describe('GroupsService', () => { }); describe('updateById', () => { + beforeEach(() => { + groupModel.update.mockResolvedValue({ name: 'Test Group', settings: {} }); + }); + + it('should record the fields the update changed, in the group it changed', async () => { + groupModel.findFirst.mockResolvedValueOnce({ + accessibleInstrumentIds: ['a', 'b'], + name: 'Old Name', + settings: {} + }); + groupModel.update.mockResolvedValueOnce({ accessibleInstrumentIds: ['b', 'a'], name: 'New Name', settings: {} }); + await groupsService.updateById('123', { name: 'New Name' }, admin); + expect(auditLogger.logUpdate).toHaveBeenCalledExactlyOnceWith('GROUP', ['name'], { + groupId: '123', + userId: admin.id + }); + }); + + it('should record no update that lost to a concurrent edit', async () => { + groupModel.findFirst.mockResolvedValueOnce({ settings: {}, updatedAt: new Date('2026-07-01T00:00:00Z') }); + groupModel.update.mockRejectedValueOnce( + new PrismaClientKnownRequestError('No record was found for an update', { + clientVersion: '6.19.3', + code: 'P2025' + }) + ); + await expect( + groupsService.updateById('123', { expectedUpdatedAt: new Date('2026-07-01T00:00:00Z') }, admin) + ).rejects.toBeInstanceOf(ConflictException); + expect(auditLogger.logUpdate).not.toHaveBeenCalled(); + }); + it('should throw a NotFoundException when no accessible group has the provided id', async () => { groupModel.findFirst.mockResolvedValueOnce(null); - await expect(groupsService.updateById('123', { name: 'Test Group' })).rejects.toBeInstanceOf(NotFoundException); + await expect(groupsService.updateById('123', { name: 'Test Group' }, admin)).rejects.toBeInstanceOf( + NotFoundException + ); expect(groupModel.update).not.toHaveBeenCalled(); }); it('should set the instrumentRepos relation from instrumentRepoIds', async () => { groupModel.findFirst.mockResolvedValueOnce({ name: 'Test Group', settings: {} }); - await groupsService.updateById('123', { instrumentRepoIds: ['repo-1', 'repo-2'] }); + await groupsService.updateById('123', { instrumentRepoIds: ['repo-1', 'repo-2'] }, admin); expect(groupModel.update.mock.lastCall?.[0]).toMatchObject({ data: { instrumentRepos: { set: [{ id: 'repo-1' }, { id: 'repo-2' }] } } }); @@ -128,7 +168,7 @@ describe('GroupsService', () => { groupModel.findFirst.mockResolvedValueOnce({ name: 'Test Group', settings: {} }); // 'deleted' was removed since the client loaded the group and must not reach the relation set. instrumentModel.findMany.mockResolvedValueOnce([{ id: 'live-1' }, { id: 'live-2' }]); - await groupsService.updateById('123', { accessibleInstrumentIds: ['live-1', 'deleted', 'live-2'] }); + await groupsService.updateById('123', { accessibleInstrumentIds: ['live-1', 'deleted', 'live-2'] }, admin); expect(instrumentModel.findMany).toHaveBeenCalledWith({ select: { id: true }, where: { @@ -143,7 +183,7 @@ describe('GroupsService', () => { it('should not throw when the name is unchanged', async () => { groupModel.findFirst.mockResolvedValueOnce({ name: 'Test Group', settings: {} }); - await groupsService.updateById('123', { name: 'Test Group' }); + await groupsService.updateById('123', { name: 'Test Group' }, admin); // The current name must not be treated as a collision with itself. expect(groupModel.exists).not.toHaveBeenCalled(); expect(groupModel.update).toHaveBeenCalled(); @@ -152,11 +192,15 @@ describe('GroupsService', () => { it('should write the email template list as a composite set', async () => { const templates = [{ body: { en: 'Link {{url}}' }, id: 'tpl-1', name: 'One', subject: { en: 'Subject' } }]; groupModel.findFirst.mockResolvedValueOnce({ settings: {}, updatedAt: new Date('2026-07-01T00:00:00Z') }); - await groupsService.updateById('123', { - activeAssignmentEmailTemplateId: 'tpl-1', - emailTemplates: templates, - expectedUpdatedAt: new Date('2026-07-01T00:00:00Z') - }); + await groupsService.updateById( + '123', + { + activeAssignmentEmailTemplateId: 'tpl-1', + emailTemplates: templates, + expectedUpdatedAt: new Date('2026-07-01T00:00:00Z') + }, + admin + ); expect(groupModel.update.mock.lastCall?.[0]).toMatchObject({ data: { emailTemplates: { set: templates } } }); }); @@ -164,10 +208,14 @@ describe('GroupsService', () => { it('should reject duplicate template ids', async () => { groupModel.findFirst.mockResolvedValueOnce({ settings: {}, updatedAt: new Date('2026-07-01T00:00:00Z') }); await expect( - groupsService.updateById('123', { - emailTemplates: [template('tpl-1', 'One'), template('tpl-1', 'Two')], - expectedUpdatedAt: new Date('2026-07-01T00:00:00Z') - }) + groupsService.updateById( + '123', + { + emailTemplates: [template('tpl-1', 'One'), template('tpl-1', 'Two')], + expectedUpdatedAt: new Date('2026-07-01T00:00:00Z') + }, + admin + ) ).rejects.toBeInstanceOf(BadRequestException); }); @@ -175,21 +223,29 @@ describe('GroupsService', () => { it('should reject an active template id that resolves to nothing', async () => { groupModel.findFirst.mockResolvedValueOnce({ settings: {}, updatedAt: new Date('2026-07-01T00:00:00Z') }); await expect( - groupsService.updateById('123', { - activeAssignmentEmailTemplateId: 'tpl-gone', - emailTemplates: [template('tpl-1', 'One')], - expectedUpdatedAt: new Date('2026-07-01T00:00:00Z') - }) + groupsService.updateById( + '123', + { + activeAssignmentEmailTemplateId: 'tpl-gone', + emailTemplates: [template('tpl-1', 'One')], + expectedUpdatedAt: new Date('2026-07-01T00:00:00Z') + }, + admin + ) ).rejects.toBeInstanceOf(BadRequestException); }); it('should accept a null active id, which selects the built-in default', async () => { groupModel.findFirst.mockResolvedValueOnce({ settings: {}, updatedAt: new Date('2026-07-01T00:00:00Z') }); - await groupsService.updateById('123', { - activeAssignmentEmailTemplateId: null, - emailTemplates: [template('tpl-1', 'One')], - expectedUpdatedAt: new Date('2026-07-01T00:00:00Z') - }); + await groupsService.updateById( + '123', + { + activeAssignmentEmailTemplateId: null, + emailTemplates: [template('tpl-1', 'One')], + expectedUpdatedAt: new Date('2026-07-01T00:00:00Z') + }, + admin + ); expect(groupModel.update).toHaveBeenCalled(); }); @@ -201,10 +257,14 @@ describe('GroupsService', () => { updatedAt: new Date('2026-07-01T00:00:00Z') }); await expect( - groupsService.updateById('123', { - emailTemplates: [template('tpl-2', 'Two')], - expectedUpdatedAt: new Date('2026-07-01T00:00:00Z') - }) + groupsService.updateById( + '123', + { + emailTemplates: [template('tpl-2', 'Two')], + expectedUpdatedAt: new Date('2026-07-01T00:00:00Z') + }, + admin + ) ).rejects.toBeInstanceOf(BadRequestException); }); @@ -213,10 +273,14 @@ describe('GroupsService', () => { it('should reject an update composed against a stale revision', async () => { groupModel.findFirst.mockResolvedValueOnce({ settings: {}, updatedAt: new Date('2026-07-02T00:00:00Z') }); await expect( - groupsService.updateById('123', { - emailTemplates: [], - expectedUpdatedAt: new Date('2026-07-01T00:00:00Z') - }) + groupsService.updateById( + '123', + { + emailTemplates: [], + expectedUpdatedAt: new Date('2026-07-01T00:00:00Z') + }, + admin + ) ).rejects.toBeInstanceOf(ConflictException); expect(groupModel.update).not.toHaveBeenCalled(); }); @@ -225,7 +289,7 @@ describe('GroupsService', () => { it('should carry the expected revision into the update where clause', async () => { const expectedUpdatedAt = new Date('2026-07-01T00:00:00Z'); groupModel.findFirst.mockResolvedValueOnce({ settings: {}, updatedAt: expectedUpdatedAt }); - await groupsService.updateById('123', { emailTemplates: [], expectedUpdatedAt }); + await groupsService.updateById('123', { emailTemplates: [], expectedUpdatedAt }, admin); expect(groupModel.update.mock.lastCall?.[0]).toMatchObject({ where: { updatedAt: expectedUpdatedAt } }); }); @@ -240,30 +304,32 @@ describe('GroupsService', () => { code: 'P2025' }) ); - await expect(groupsService.updateById('123', { emailTemplates: [], expectedUpdatedAt })).rejects.toBeInstanceOf( - ConflictException - ); + await expect( + groupsService.updateById('123', { emailTemplates: [], expectedUpdatedAt }, admin) + ).rejects.toBeInstanceOf(ConflictException); }); it('should not disguise an unrelated database error as a conflict', async () => { const expectedUpdatedAt = new Date('2026-07-01T00:00:00Z'); groupModel.findFirst.mockResolvedValueOnce({ settings: {}, updatedAt: expectedUpdatedAt }); groupModel.update.mockRejectedValueOnce(new Error('connection lost')); - await expect(groupsService.updateById('123', { emailTemplates: [], expectedUpdatedAt })).rejects.toThrow( + await expect(groupsService.updateById('123', { emailTemplates: [], expectedUpdatedAt }, admin)).rejects.toThrow( 'connection lost' ); }); it('should leave the where clause unconstrained when no revision is supplied', async () => { groupModel.findFirst.mockResolvedValueOnce({ settings: {} }); - await groupsService.updateById('123', { name: 'Test Group' }); + await groupsService.updateById('123', { name: 'Test Group' }, admin); expect(groupModel.update.mock.lastCall?.[0].where).not.toHaveProperty('updatedAt'); }); it('should throw a ConflictException when renaming to an existing name', async () => { groupModel.findFirst.mockResolvedValueOnce({ name: 'Old Name', settings: {} }); groupModel.exists.mockResolvedValueOnce(true); - await expect(groupsService.updateById('123', { name: 'Taken Name' })).rejects.toBeInstanceOf(ConflictException); + await expect(groupsService.updateById('123', { name: 'Taken Name' }, admin)).rejects.toBeInstanceOf( + ConflictException + ); expect(groupModel.exists).toHaveBeenCalledWith({ name: 'Taken Name' }); }); }); diff --git a/apps/api/src/groups/groups.controller.ts b/apps/api/src/groups/groups.controller.ts index 2f15db1c7..f3b309a41 100644 --- a/apps/api/src/groups/groups.controller.ts +++ b/apps/api/src/groups/groups.controller.ts @@ -1,7 +1,9 @@ import { ApiOperation, CurrentUser } from '@douglasneuroinformatics/libnest'; +import type { RequestUser } from '@douglasneuroinformatics/libnest'; import { Body, Controller, Delete, Get, Param, Patch, Post } from '@nestjs/common'; import { $CreateGroupData, $UpdateGroupData } from '@opendatacapture/schemas/group'; +import { AuditLogger } from '@/audit/audit.logger'; import type { AppAbility } from '@/auth/auth.types'; import { ADMIN_ONLY, RouteAccess } from '@/core/decorators/route-access.decorator'; @@ -9,7 +11,10 @@ import { GroupsService } from './groups.service'; @Controller('groups') export class GroupsController { - constructor(private readonly groupsService: GroupsService) {} + constructor( + private readonly groupsService: GroupsService, + private readonly auditLogger: AuditLogger + ) {} // Gated on `manage all` (admin alone) rather than `create Group`. A group manager holds // `manage Group` conditioned on their own groups, and the guard evaluates `@RouteAccess` against @@ -18,8 +23,11 @@ export class GroupsController { @ApiOperation({ summary: 'Create Group' }) @Post() @RouteAccess(ADMIN_ONLY) - create(@Body() data: $CreateGroupData) { - return this.groupsService.create(data); + async create(@Body() data: $CreateGroupData, @CurrentUser() currentUser: RequestUser) { + const group = await this.groupsService.create(data); + // Logged here rather than in the service, which demo seeding also calls with no one acting. + await this.auditLogger.log('CREATE', 'GROUP', { groupId: group.id, userId: currentUser.id }); + return group; } @ApiOperation({ summary: 'Delete Group' }) @@ -46,7 +54,7 @@ export class GroupsController { @ApiOperation({ summary: 'Update Group' }) @Patch(':id') @RouteAccess({ action: 'update', subject: 'Group' }) - updateById(@Param('id') id: string, @Body() update: $UpdateGroupData, @CurrentUser('ability') ability?: AppAbility) { - return this.groupsService.updateById(id, update, { ability }); + updateById(@Param('id') id: string, @Body() update: $UpdateGroupData, @CurrentUser() currentUser: RequestUser) { + return this.groupsService.updateById(id, update, currentUser); } } diff --git a/apps/api/src/groups/groups.service.ts b/apps/api/src/groups/groups.service.ts index 293b8b1b0..061361d2b 100644 --- a/apps/api/src/groups/groups.service.ts +++ b/apps/api/src/groups/groups.service.ts @@ -1,11 +1,13 @@ import { InjectModel } from '@douglasneuroinformatics/libnest'; -import type { Model } from '@douglasneuroinformatics/libnest'; +import type { Model, RequestUser } from '@douglasneuroinformatics/libnest'; import { BadRequestException, ConflictException, Injectable, NotFoundException } from '@nestjs/common'; import { $CreateGroupData, $UpdateGroupData } from '@opendatacapture/schemas/group'; import type { GroupEmailTemplate } from '@opendatacapture/schemas/group'; import type { Prisma } from '@prisma/client'; import { PrismaClientKnownRequestError } from '@prisma/client/runtime/library'; +import { AuditLogger } from '@/audit/audit.logger'; +import { changedFields } from '@/audit/audit.utils'; import { accessibleQuery } from '@/auth/ability.utils'; import type { EntityOperationOptions } from '@/core/types'; @@ -13,7 +15,8 @@ import type { EntityOperationOptions } from '@/core/types'; export class GroupsService { constructor( @InjectModel('Group') private readonly groupModel: Model<'Group'>, - @InjectModel('Instrument') private readonly instrumentModel: Model<'Instrument'> + @InjectModel('Instrument') private readonly instrumentModel: Model<'Instrument'>, + private readonly auditLogger: AuditLogger ) {} async create({ name, settings, type, ...data }: $CreateGroupData) { @@ -83,8 +86,9 @@ export class GroupsService { settings, ...data }: $UpdateGroupData, - { ability }: EntityOperationOptions = {} + currentUser: RequestUser ) { + const { ability } = currentUser; const where: Prisma.GroupWhereInput = { AND: [accessibleQuery(ability, 'update', 'Group')], id }; const group = await this.groupModel.findFirst({ where }); if (!group) { @@ -121,8 +125,8 @@ export class GroupsService { validInstrumentIds = existingInstruments.map(({ id }) => id); } - try { - return await this.groupModel.update({ + const updated = await this.groupModel + .update({ data: { accessibleInstruments: validInstrumentIds ? { @@ -143,16 +147,18 @@ export class GroupsService { ...data }, where: { AND: [accessibleQuery(ability, 'update', 'Group')], id, ...revisionGuard } + }) + .catch((err: unknown) => { + // P2025 is "no record matched the where clause". The findFirst above already matched on id + // and ability, so the revision guard is the only filter that can have started failing — + // i.e. a concurrent edit. Anything else is a real fault and has to surface. + if (revisionGuard.updatedAt && err instanceof PrismaClientKnownRequestError && err.code === 'P2025') { + throw new ConflictException(`Group with ID '${id}' has been modified since it was loaded`); + } + throw err; }); - } catch (err) { - // P2025 is "no record matched the where clause". The findFirst above already matched on id - // and ability, so the revision guard is the only filter that can have started failing — - // i.e. a concurrent edit. Anything else is a real fault and has to surface. - if (revisionGuard.updatedAt && err instanceof PrismaClientKnownRequestError && err.code === 'P2025') { - throw new ConflictException(`Group with ID '${id}' has been modified since it was loaded`); - } - throw err; - } + await this.auditLogger.logUpdate('GROUP', changedFields(group, updated), { groupId: id, userId: currentUser.id }); + return updated; } /** diff --git a/packages/schemas/src/audit/audit.ts b/packages/schemas/src/audit/audit.ts index 955e4339c..7f522a762 100644 --- a/packages/schemas/src/audit/audit.ts +++ b/packages/schemas/src/audit/audit.ts @@ -29,6 +29,7 @@ export type AuditedAction = (type */ export const AUDITED_ACTIONS = { ASSIGNMENT: ['CREATE', 'DELETE', 'SEND_EMAIL', 'UPDATE'], + GROUP: ['CREATE', 'UPDATE'], INSTRUMENT: ['ARCHIVE', 'CREATE', 'DELETE', 'UNARCHIVE'], INSTRUMENT_RECORD: ['DELETE', 'UPDATE'], SUBJECT: ['DELETE'], diff --git a/testing/src/specs/admin-audit-logs.spec.ts b/testing/src/specs/admin-audit-logs.spec.ts index c0659c256..98a359d4f 100644 --- a/testing/src/specs/admin-audit-logs.spec.ts +++ b/testing/src/specs/admin-audit-logs.spec.ts @@ -273,4 +273,34 @@ test.describe('admin audit log coverage', () => { expect(entry?.group?.name).toBe(group.name); }); }); + + test.describe('groups', () => { + test.use({ actingRole: 'ADMIN' }); + + test('should show who created a group under its own group, entity and action filters', async ({ + api, + getPageModel + }) => { + const group = await api.createGroup(); + + const auditLogsPage = await getPageModel('/admin/audit/logs'); + await auditLogsPage.filterBy('group', group.name); + await auditLogsPage.filterBy('entity', 'Group'); + await auditLogsPage.filterBy('action', 'Create'); + + const rows = auditLogsPage.dataTable.getByTestId('data-table-row'); + await expect(rows).toHaveCount(1); + await expect(rows.first()).toContainText(ADMIN.username); + }); + + test('should record which fields a group update changed', async ({ api, uniqueId }) => { + const group = await api.createGroup(); + const name = `Renamed Group ${uniqueId}`; + await api.updateGroup(group.id, { name }); + + const entries = await api.findAuditLogs({ action: 'UPDATE', entity: 'GROUP' }); + const changes = entries.filter((entry) => entry.group?.name === name).map(({ metadata }) => metadata?.fields); + expect(changes).toContain('name'); + }); + }); }); diff --git a/testing/src/support/api-client.ts b/testing/src/support/api-client.ts index 8e8f6fe88..c74d1dc5b 100644 --- a/testing/src/support/api-client.ts +++ b/testing/src/support/api-client.ts @@ -7,7 +7,7 @@ import { AUDIT_LOGS_MAX_PAGE_SIZE } from '@opendatacapture/schemas/audit'; import type { $AuditLog, $AuditLogsPage, AuditedAction, AuditedEntity } from '@opendatacapture/schemas/audit'; import type { $LoginCredentials } from '@opendatacapture/schemas/auth'; import type { Language, Permissions } from '@opendatacapture/schemas/core'; -import type { $CreateGroupData, Group } from '@opendatacapture/schemas/group'; +import type { $CreateGroupData, $UpdateGroupData, Group } from '@opendatacapture/schemas/group'; import type { InstrumentInfo } from '@opendatacapture/schemas/instrument'; import type { $CreateInstrumentRecordData, @@ -87,11 +87,7 @@ export class ApiClient { 201, 'create group' ); - const accessibleInstrumentIds = await this.getAccessibleInstrumentIds(); - await this.expectOk( - this.request.patch(`${API}/groups/${group.id}`, { data: { accessibleInstrumentIds }, headers: this.authHeaders }), - 'grant instrument access' - ); + await this.updateGroup(group.id, { accessibleInstrumentIds: await this.getAccessibleInstrumentIds() }); return group; } @@ -412,6 +408,14 @@ export class ApiClient { ); } + /** Updates a group through the route the web app's group management uses. */ + async updateGroup(id: string, data: $UpdateGroupData): Promise { + await this.expectOk( + this.request.patch(`${API}/groups/${id}`, { data, headers: this.authHeaders }), + `update group '${id}'` + ); + } + /** Merges `data` into a record's data over the API, which no page of the web app does. */ async updateRecord(id: string, data: $UpdateInstrumentRecordData['data']): Promise { await this.expectOk(