diff --git a/.agents/docs/architecture/auth-and-permissions.md b/.agents/docs/architecture/auth-and-permissions.md index 72ebd8abf..c24717cfb 100644 --- a/.agents/docs/architecture/auth-and-permissions.md +++ b/.agents/docs/architecture/auth-and-permissions.md @@ -207,10 +207,18 @@ add it to the `$GroupScopableSubjectName` exclusion. ## Adjacent things that are not the permission system -- **Audit logging is not automatic.** `AuditLogger` (`src/audit/audit.logger.ts`) is injected into - exactly two services and produces exactly three log calls: `CREATE`/`UPDATE` on `ASSIGNMENT` - (`src/assignments/assignments.service.ts`) and `LOGIN` on `USER` (`src/auth/auth.service.ts`). - Nothing else in the API writes an audit log. Do not assume a new mutation is recorded. +- **Audit logging is not automatic.** A mutation is recorded only where it calls + `AuditLogger.log` (`src/audit/audit.logger.ts`), so do not assume a new one is. The pairs that are + written are `AUDITED_ACTIONS` in `packages/schemas/src/audit/audit.ts`: `log` is typed against it, + so a call naming an undeclared pair does not compile, and the web audit log page derives its + Entity and Action filters from it. The Prisma `AuditLogAction` and `AuditLogEntity` enums are + wider and do not say what is written. The log records what a signed-in user did, so an entry is + written where that user is known: in the controller when the service method is also called by + setup, demo seeding, repository sync or the gateway synchronizer, which act for no one + (`UsersController.create`). An update goes through `AuditLogger.logUpdate` with `changedFields` + (`src/audit/audit.utils.ts`), which records the names of the fields it changed and never their + values, because every admin reads the log; an update that changed nothing writes no entry. + Entries are written after the change, not in its transaction. - **`user.hashedPassword` is globally omitted** by the Prisma client constructed in `src/core/prisma.ts`. `UsersService.findByUsername` re-enables it via `{ includeHashedPassword: true }`, which is how login reads it. Individual `UsersService` methods 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/apps/api/src/audit/__tests__/audit.logger.test.ts b/apps/api/src/audit/__tests__/audit.logger.test.ts index 2e33768d5..0cf97ea79 100644 --- a/apps/api/src/audit/__tests__/audit.logger.test.ts +++ b/apps/api/src/audit/__tests__/audit.logger.test.ts @@ -46,10 +46,34 @@ describe('AuditLogger', () => { expect(auditLogModel.create.mock.lastCall?.[0]?.data.metadata).toStrictEqual({ mode: 'BULK' }); }); + it('should refuse at compile time a pair AUDITED_ACTIONS does not declare, so the filters cannot miss it', async () => { + // @ts-expect-error -- nothing sends an email about a user, so the pair is not declared + await auditLogger.log('SEND_EMAIL', 'USER', { groupId: null, userId: 'user-1' }); + expect(auditLogModel.create).toHaveBeenCalledOnce(); + }); + it('should timestamp the entry when it is written, so the log can be ordered by time', async () => { const before = Date.now(); await auditLogger.log('LOGIN', 'USER', { groupId: null, userId: 'user-1' }); expect(auditLogModel.create.mock.lastCall?.[0]?.data.timestamp).toBeGreaterThanOrEqual(before); }); }); + + describe('logUpdate', () => { + it('should record the names of the changed fields, sorted, beside the metadata it is given', async () => { + await auditLogger.logUpdate('USER', ['lastName', 'email'], { + groupId: null, + metadata: { targetUserId: 'user-2' }, + userId: 'user-1' + }); + expect(auditLogModel.create.mock.lastCall?.[0]).toMatchObject({ + data: { action: 'UPDATE', entity: 'USER', metadata: { fields: 'email,lastName', targetUserId: 'user-2' } } + }); + }); + + it('should record nothing for an update that changed nothing, so a resubmitted form adds no entry', async () => { + await auditLogger.logUpdate('USER', [], { groupId: null, userId: 'user-1' }); + expect(auditLogModel.create).not.toHaveBeenCalled(); + }); + }); }); 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..d4ba5e6e3 --- /dev/null +++ b/apps/api/src/audit/__tests__/audit.utils.test.ts @@ -0,0 +1,52 @@ +import { describe, expect, it } from 'vitest'; + +import { changedFields } from '../audit.utils'; + +describe('changedFields', () => { + it('should name only the fields whose value changed, so a resubmitted form reports what was edited', () => { + expect(changedFields({ firstName: 'Jane', lastName: 'Doe' }, { firstName: 'Janet', lastName: 'Doe' })).toEqual([ + 'firstName' + ]); + }); + + it('should name a field the update added or removed', () => { + expect(changedFields({ email: 'jane@example.org' }, { phoneNumber: '5555555' })).toEqual(['email', 'phoneNumber']); + }); + + it('should sort the names, so two entries for the same edit read the same', () => { + expect(changedFields({ zip: '1' }, { city: 'Montreal' })).toEqual(['city', 'zip']); + }); + + it('should compare nested values by content, so a rewritten but equal object is not a change', () => { + expect(changedFields({ settings: { minimumAge: 18 } }, { settings: { minimumAge: 18 } })).toEqual([]); + }); + + it('should compare dates by their time, so a re-read date is not a change', () => { + expect(changedFields({ dateOfBirth: new Date(0) }, { dateOfBirth: new Date(0) })).toEqual([]); + }); + + it('should treat a reordered list of ids as unchanged, since the web resubmits them in its own order', () => { + expect(changedFields({ groupIds: ['a', 'b'] }, { groupIds: ['b', 'a'] })).toEqual([]); + }); + + it('should name a list of ids that gained or lost one', () => { + expect(changedFields({ groupIds: ['a', 'b'] }, { groupIds: ['a'] })).toEqual(['groupIds']); + }); + + it('should keep the order of a list of objects significant', () => { + expect(changedFields({ rules: [{ id: 1 }, { id: 2 }] }, { rules: [{ id: 2 }, { id: 1 }] })).toEqual(['rules']); + }); + + it('should leave out updatedAt, which every write changes', () => { + expect(changedFields({ updatedAt: new Date(0) }, { updatedAt: new Date(1) })).toEqual([]); + }); + + it('should name the indices of a list that changed, so array-shaped record data is described too', () => { + expect(changedFields([{ score: 1 }, { score: 2 }], [{ score: 1 }, { score: 3 }])).toEqual(['1']); + }); + + it.each([null, 'text', 7])('should refuse to compare %j, which has no fields to name', (value) => { + expect(() => changedFields(value, {})).toThrow(TypeError); + expect(() => changedFields({}, value)).toThrow(TypeError); + }); +}); diff --git a/apps/api/src/audit/audit.logger.ts b/apps/api/src/audit/audit.logger.ts index 6c02030ff..450313e77 100644 --- a/apps/api/src/audit/audit.logger.ts +++ b/apps/api/src/audit/audit.logger.ts @@ -1,16 +1,22 @@ import { InjectModel } from '@douglasneuroinformatics/libnest'; import type { Model } from '@douglasneuroinformatics/libnest'; import { Injectable } from '@nestjs/common'; -import type { $AuditLogAction, $AuditLogEntity } from '@opendatacapture/schemas/audit'; +import type { AuditedAction, AuditedEntity } from '@opendatacapture/schemas/audit'; + +type AuditLogParams = { groupId: null | string; metadata?: { [key: string]: string }; userId: string }; + +type UpdateAuditedEntity = { + [TEntity in AuditedEntity]: 'UPDATE' extends AuditedAction ? TEntity : never; +}[AuditedEntity]; @Injectable() export class AuditLogger { constructor(@InjectModel('AuditLog') private readonly auditLogModel: Model<'AuditLog'>) {} - async log( - action: $AuditLogAction, - entity: $AuditLogEntity, - params: { groupId: null | string; metadata?: { [key: string]: string }; userId: string } + async log( + action: AuditedAction, + entity: TEntity, + 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..0f12ef3e8 --- /dev/null +++ b/apps/api/src/audit/audit.utils.ts @@ -0,0 +1,35 @@ +import { isEqual, isEqualWith } from 'lodash-es'; + +const isStringList = (value: unknown): value is string[] => { + return Array.isArray(value) && value.every((item) => typeof item === 'string'); +}; + +/** A list of strings is a list of ids here, and one that is only reordered has not changed. */ +const compareStringListsAsSets = (value: unknown, other: unknown): boolean | undefined => { + if (isStringList(value) && isStringList(other)) { + return isEqual(value.toSorted(), other.toSorted()); + } + return undefined; +}; + +const fieldValues = (value: unknown): Map => { + if (typeof value !== 'object' || value === null) { + throw new TypeError(`Cannot name the changed fields of ${String(value)}, which has no fields`); + } + return new Map(Object.entries(value)); +}; + +/** + * The names of the fields that differ between a row, or a record's data, before and after an + * update, sorted. + */ +export function changedFields(before: unknown, after: unknown): string[] { + const beforeValues = fieldValues(before); + const afterValues = fieldValues(after); + const fields = new Set([...beforeValues.keys(), ...afterValues.keys()]); + // Every write sets it, so it would name every update as a change. + fields.delete('updatedAt'); + return [...fields] + .filter((field) => !isEqualWith(beforeValues.get(field), afterValues.get(field), compareStringListsAsSets)) + .sort(); +} 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/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/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/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/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/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/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..7f522a762 100644 --- a/packages/schemas/src/audit/audit.ts +++ b/packages/schemas/src/audit/audit.ts @@ -18,6 +18,24 @@ 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', 'DELETE', 'SEND_EMAIL', 'UPDATE'], + GROUP: ['CREATE', '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[] }; + /** 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; 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..98a359d4f 100644 --- a/testing/src/specs/admin-audit-logs.spec.ts +++ b/testing/src/specs/admin-audit-logs.spec.ts @@ -1,5 +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', () => { @@ -12,10 +14,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. @@ -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('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.filterButton('entity')).toContainText('Session'); - await expect(auditLogsPage.$ref).toHaveURL(/entity=SESSION/); + await expect(auditLogsPage.filterOption('Send Email')).toBeVisible(); + await expect(auditLogsPage.filterOption('Login')).toBeHidden(); }); test('should clear every filter at once', async ({ getPageModel }) => { @@ -103,3 +120,187 @@ test.describe('admin audit logs fetching', () => { expect(auditLogsRequests).toHaveLength(2); }); }); + +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); +} + +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(); + }); + }); + + 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); + }); + }); + + 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}`); + }); + }); + + 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' }); + }); + }); + + 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); + }); + }); + + 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/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..c74d1dc5b 100644 --- a/testing/src/support/api-client.ts +++ b/testing/src/support/api-client.ts @@ -1,12 +1,17 @@ -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, $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'; +import type { $CreateGroupData, $UpdateGroupData, Group } from '@opendatacapture/schemas/group'; import type { InstrumentInfo } from '@opendatacapture/schemas/instrument'; import type { $CreateInstrumentRecordData, + $UpdateInstrumentRecordData, InstrumentRecord, UploadInstrumentRecordsData } from '@opendatacapture/schemas/instrument-records'; @@ -19,7 +24,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'; @@ -76,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; } @@ -207,6 +214,42 @@ 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' + ); + } + + /** 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( + this.request.delete(`${API}/instruments/${id}`, { headers: this.authHeaders }), + `delete series '${id}'` + ); + } + + /** 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. @@ -229,15 +272,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 +401,37 @@ 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 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( + 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( + 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 });