From b0201b93ce66a4c3a082258b554749801d941ae1 Mon Sep 17 00:00:00 2001 From: thomasbeaudry Date: Wed, 2 Sep 2026 13:42:55 -0400 Subject: [PATCH 1/3] feat(api,gateway): add all-or-nothing bulk remote assignments Backend for group-scoped bulk remote assignments (#1500), following the review on that issue: - N subjects x M timepoints: every (instrument, expiry) pair applies to every selected subject. - All-or-nothing. Preflight throws with every issue attached rather than reporting per-row outcomes, and create re-runs the same checks so a conflict appearing between review and submit cannot slip through. Any failure deletes every row staged by the call. - One gateway request per batch, carrying each instrument bundle once rather than once per assignment. - Conflicts block by default and are waived only by an explicit allowDuplicates. Extracts a dynamic future-date schema: z.coerce.date().min(new Date()) froze the bound at module load, so a long-running process kept accepting expiries that had since passed. Adds the gateway vitest project, which did not exist. Co-Authored-By: Claude Opus 5 --- .../__tests__/assignments.service.spec.ts | 231 ++++++++++++++++++ .../src/assignments/assignments.controller.ts | 28 ++- .../src/assignments/assignments.service.ts | 218 ++++++++++++++++- .../assignments/dto/bulk-assignment.dto.ts | 38 +++ apps/api/src/gateway/gateway.service.ts | 38 +++ apps/gateway/package.json | 1 + .../src/routers/__tests__/api.router.test.ts | 59 +++++ apps/gateway/src/routers/api.router.ts | 42 +++- apps/gateway/tsconfig.json | 2 +- apps/gateway/vitest.config.ts | 20 ++ .../schemas/src/assignment/assignment.test.ts | 144 +++++++++++ packages/schemas/src/assignment/assignment.ts | 200 +++++++++++++-- 12 files changed, 995 insertions(+), 26 deletions(-) create mode 100644 apps/api/src/assignments/__tests__/assignments.service.spec.ts create mode 100644 apps/api/src/assignments/dto/bulk-assignment.dto.ts create mode 100644 apps/gateway/src/routers/__tests__/api.router.test.ts create mode 100644 apps/gateway/vitest.config.ts create mode 100644 packages/schemas/src/assignment/assignment.test.ts diff --git a/apps/api/src/assignments/__tests__/assignments.service.spec.ts b/apps/api/src/assignments/__tests__/assignments.service.spec.ts new file mode 100644 index 000000000..1c39a5ba9 --- /dev/null +++ b/apps/api/src/assignments/__tests__/assignments.service.spec.ts @@ -0,0 +1,231 @@ +import { ConfigService, getModelToken, LoggingService } from '@douglasneuroinformatics/libnest'; +import type { Model } from '@douglasneuroinformatics/libnest'; +import { MockFactory } from '@douglasneuroinformatics/libnest/testing'; +import type { MockedInstance } from '@douglasneuroinformatics/libnest/testing'; +import { ForbiddenException, NotFoundException, UnprocessableEntityException } from '@nestjs/common'; +import { Test } from '@nestjs/testing'; +import type { BulkAssignmentFailure } from '@opendatacapture/schemas/assignment'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { AuditLogger } from '@/audit/audit.logger'; +import { createAppAbility } from '@/auth/ability.utils'; +import { GatewayService } from '@/gateway/gateway.service'; + +import { AssignmentsService } from '../assignments.service'; + +const GROUP_ID = 'group-1'; + +const futureDate = () => new Date(Date.now() + 86_400_000); + +/** + * A real ability rather than a stub: `accessibleQuery` calls into CASL's `accessibleBy`, so a + * hand-rolled `can` would not survive contact with it. Permitting everything keeps each test on the + * service's own group/subject/instrument scoping rather than on CASL itself. + */ +const permissiveUser = () => + ({ + ability: createAppAbility([{ action: 'manage', subject: 'all' }]), + id: 'user-1' + }) as any; + +/** Can read (so the group resolves) but cannot create an assignment. */ +const readOnlyUser = () => + ({ + ability: createAppAbility([{ action: 'read', subject: 'all' }]), + id: 'user-1' + }) as any; + +const request = (overrides: { [key: string]: any } = {}) => ({ + allowDuplicates: false, + groupId: GROUP_ID, + subjectIds: ['subject-1', 'subject-2'], + timepoints: [{ expiresAt: futureDate(), instrumentId: 'instrument-1' }], + ...overrides +}); + +/** The refusal body attached to an UnprocessableEntityException. */ +const failureOf = async (promise: Promise): Promise => { + try { + await promise; + } catch (err) { + return (err as UnprocessableEntityException).getResponse() as BulkAssignmentFailure; + } + throw new Error('Expected the operation to be refused, but it resolved'); +}; + +describe('AssignmentsService', () => { + let assignmentsService: AssignmentsService; + let assignmentModel: MockedInstance>; + let groupModel: MockedInstance>; + let subjectModel: MockedInstance>; + let auditLogger: MockedInstance; + let gatewayService: MockedInstance; + + beforeEach(async () => { + const moduleRef = await Test.createTestingModule({ + providers: [ + AssignmentsService, + MockFactory.createForModelToken(getModelToken('Assignment')), + MockFactory.createForModelToken(getModelToken('Group')), + MockFactory.createForModelToken(getModelToken('Subject')), + { provide: AuditLogger, useValue: { log: vi.fn() } }, + { provide: ConfigService, useValue: { get: () => 3500, getOrThrow: () => ({ origin: 'https://x' }) } }, + { provide: GatewayService, useValue: { createRemoteAssignments: vi.fn() } }, + { provide: LoggingService, useValue: { error: vi.fn() } } + ] + }).compile(); + + assignmentModel = moduleRef.get(getModelToken('Assignment')); + groupModel = moduleRef.get(getModelToken('Group')); + subjectModel = moduleRef.get(getModelToken('Subject')); + auditLogger = moduleRef.get(AuditLogger); + gatewayService = moduleRef.get(GatewayService); + assignmentsService = moduleRef.get(AssignmentsService); + + groupModel.findFirst.mockResolvedValue({ accessibleInstrumentIds: ['instrument-1', 'instrument-2'], id: GROUP_ID }); + subjectModel.findMany.mockResolvedValue([{ id: 'subject-1' }, { id: 'subject-2' }]); + assignmentModel.findMany.mockResolvedValue([]); + assignmentModel.create.mockImplementation(({ data }: any) => + Promise.resolve({ ...data, instrumentId: 'instrument-1' }) + ); + assignmentModel.deleteMany.mockResolvedValue({ count: 0 }); + }); + + describe('bulkPreflight', () => { + it('should report one assignment per subject per timepoint, since every timepoint applies to every subject', async () => { + const result = await assignmentsService.bulkPreflight( + request({ + subjectIds: ['subject-1', 'subject-2'], + timepoints: [ + { expiresAt: futureDate(), instrumentId: 'instrument-1' }, + { expiresAt: futureDate(), instrumentId: 'instrument-2' } + ] + }), + permissiveUser() + ); + expect(result).toEqual({ assignmentCount: 4, subjectCount: 2, timepointCount: 2 }); + }); + + it('should scope the group query by the caller ability, so an unreadable group is not found', async () => { + groupModel.findFirst.mockResolvedValueOnce(null); + await expect(assignmentsService.bulkPreflight(request(), permissiveUser())).rejects.toBeInstanceOf( + NotFoundException + ); + }); + + it('should refuse a caller who cannot create assignments for the resolved group', async () => { + await expect(assignmentsService.bulkPreflight(request(), readOnlyUser())).rejects.toBeInstanceOf( + ForbiddenException + ); + }); + + it('should refuse an instrument the group has not opted into, since existing is not the same as assignable', async () => { + const failure = await failureOf( + assignmentsService.bulkPreflight( + request({ timepoints: [{ expiresAt: futureDate(), instrumentId: 'instrument-other' }] }), + permissiveUser() + ) + ); + expect(failure.issues).toContainEqual({ instrumentIds: ['instrument-other'], kind: 'INSTRUMENT_UNAVAILABLE' }); + }); + + it('should restrict subjects to the selected group and the caller ability', async () => { + await assignmentsService.bulkPreflight(request(), permissiveUser()); + expect(subjectModel.findMany.mock.lastCall?.[0]).toMatchObject({ + where: { groupIds: { has: GROUP_ID }, id: { in: ['subject-1', 'subject-2'] } } + }); + }); + + it('should report a subject outside the group as unavailable without revealing whether it exists', async () => { + subjectModel.findMany.mockResolvedValueOnce([{ id: 'subject-1' }]); + const failure = await failureOf(assignmentsService.bulkPreflight(request(), permissiveUser())); + expect(failure.issues).toContainEqual({ kind: 'SUBJECT_UNAVAILABLE', subjectIds: ['subject-2'] }); + }); + + it('should report an outstanding unexpired assignment as a conflict', async () => { + assignmentModel.findMany.mockResolvedValueOnce([{ instrumentId: 'instrument-1', subjectId: 'subject-1' }]); + const failure = await failureOf(assignmentsService.bulkPreflight(request(), permissiveUser())); + expect(failure.issues).toContainEqual({ + conflicts: [{ instrumentId: 'instrument-1', subjectId: 'subject-1' }], + kind: 'CONFLICT' + }); + }); + + it('should scope the conflict query to this group, instrument, subjects, and live assignments only', async () => { + await assignmentsService.bulkPreflight(request(), permissiveUser()); + expect(assignmentModel.findMany.mock.lastCall?.[0]).toMatchObject({ + where: { + groupId: GROUP_ID, + instrumentId: { in: ['instrument-1'] }, + status: 'OUTSTANDING', + subjectId: { in: ['subject-1', 'subject-2'] } + } + }); + }); + + it('should not look for conflicts when the caller has already accepted duplicates', async () => { + await assignmentsService.bulkPreflight(request({ allowDuplicates: true }), permissiveUser()); + expect(assignmentModel.findMany).not.toHaveBeenCalled(); + }); + }); + + describe('createBulk', () => { + it('should create one assignment per subject per timepoint', async () => { + const assignments = await assignmentsService.createBulk( + request({ + timepoints: [ + { expiresAt: futureDate(), instrumentId: 'instrument-1' }, + { expiresAt: futureDate(), instrumentId: 'instrument-2' } + ] + }), + permissiveUser() + ); + expect(assignments).toHaveLength(4); + expect(assignmentModel.create).toHaveBeenCalledTimes(4); + }); + + it('should send the whole batch to the gateway in a single call, so one bundle is fetched per instrument', async () => { + await assignmentsService.createBulk(request(), permissiveUser()); + expect(gatewayService.createRemoteAssignments).toHaveBeenCalledTimes(1); + expect(gatewayService.createRemoteAssignments.mock.lastCall?.[0]).toHaveLength(2); + }); + + it('should re-run the conflict check at create time, closing the race between review and submit', async () => { + assignmentModel.findMany.mockResolvedValueOnce([{ instrumentId: 'instrument-1', subjectId: 'subject-1' }]); + await expect(assignmentsService.createBulk(request(), permissiveUser())).rejects.toBeInstanceOf( + UnprocessableEntityException + ); + expect(assignmentModel.create).not.toHaveBeenCalled(); + expect(gatewayService.createRemoteAssignments).not.toHaveBeenCalled(); + }); + + it('should create despite a conflict when the caller explicitly allowed duplicates', async () => { + const assignments = await assignmentsService.createBulk(request({ allowDuplicates: true }), permissiveUser()); + expect(assignments).toHaveLength(2); + }); + + it('should delete every staged row when the gateway rejects the batch, leaving nothing behind', async () => { + gatewayService.createRemoteAssignments.mockRejectedValueOnce(new Error('gateway down')); + await expect(assignmentsService.createBulk(request(), permissiveUser())).rejects.toThrow(); + expect(assignmentModel.deleteMany).toHaveBeenCalledTimes(1); + expect(assignmentModel.deleteMany.mock.lastCall?.[0]).toMatchObject({ where: { id: { in: expect.any(Array) } } }); + expect(assignmentModel.deleteMany.mock.lastCall?.[0].where.id.in).toHaveLength(2); + }); + + it('should not record an audit entry when the batch failed, since nothing was created', async () => { + gatewayService.createRemoteAssignments.mockRejectedValueOnce(new Error('gateway down')); + await expect(assignmentsService.createBulk(request(), permissiveUser())).rejects.toThrow(); + expect(auditLogger.log).not.toHaveBeenCalled(); + }); + + it('should record one audit entry carrying the bulk counts, using the existing CREATE action', async () => { + await assignmentsService.createBulk(request(), permissiveUser()); + expect(auditLogger.log).toHaveBeenCalledTimes(1); + expect(auditLogger.log.mock.lastCall).toMatchObject([ + 'CREATE', + 'ASSIGNMENT', + { groupId: GROUP_ID, metadata: { createdCount: '2', mode: 'BULK', requestedCount: '2' } } + ]); + }); + }); +}); diff --git a/apps/api/src/assignments/assignments.controller.ts b/apps/api/src/assignments/assignments.controller.ts index 9d53d7f9e..f945d6e41 100644 --- a/apps/api/src/assignments/assignments.controller.ts +++ b/apps/api/src/assignments/assignments.controller.ts @@ -3,7 +3,7 @@ import type { RequestUser } from '@douglasneuroinformatics/libnest'; import { BadRequestException, Body, Controller, Get, Param, Patch, Post, Query } from '@nestjs/common'; import { ApiOperation } from '@nestjs/swagger'; import { Throttle } from '@nestjs/throttler'; -import type { Assignment } from '@opendatacapture/schemas/assignment'; +import type { Assignment, BulkAssignmentPreflightResult } from '@opendatacapture/schemas/assignment'; import { DEFAULT_ASSIGNMENT_EMAIL_TEMPLATE } from '@opendatacapture/schemas/mail'; import type { EmailDeliveryResult, MailTemplate } from '@opendatacapture/schemas/mail'; @@ -15,6 +15,7 @@ import { GroupsService } from '@/groups/groups.service'; import { MailService } from '@/mail/mail.service'; import { AssignmentsService } from './assignments.service'; +import { BulkAssignmentPreflightDto, CreateBulkAssignmentsDto } from './dto/bulk-assignment.dto'; import { CreateAssignmentDto } from './dto/create-assignment.dto'; import { SendAssignmentEmailDto } from './dto/send-assignment-email.dto'; import { UpdateAssignmentDto } from './dto/update-assignment.dto'; @@ -28,6 +29,24 @@ export class AssignmentsController { private readonly mailService: MailService ) {} + // Both `bulk` routes must stay above `:id/email` and `:id`, or `bulk` is captured as an assignment + // id. `perfectionist/sort-classes` happens to preserve that today because `bulkPreflight` and + // `createBulk` sort ahead of `sendEmail` and `updateById` — renaming any of them could silently + // reverse it, so check the emitted order rather than assuming. + // + // `create Assignment` is the action these perform; the group, instrument and subject scoping that + // `RouteAccess` cannot express is enforced in the service, which also checks `read Subject` through + // `accessibleQuery` before any subject is used. + @ApiOperation({ summary: 'Validate a Bulk Assignment Request' }) + @Post('bulk/preflight') + @RouteAccess({ action: 'create', subject: 'Assignment' }) + bulkPreflight( + @Body() data: BulkAssignmentPreflightDto, + @CurrentUser() currentUser: RequestUser + ): Promise { + return this.assignmentsService.bulkPreflight(data, currentUser); + } + @ApiOperation({ summary: 'Create Assignment' }) @Post() @RouteAccess({ action: 'create', subject: 'Assignment' }) @@ -35,6 +54,13 @@ export class AssignmentsController { return this.assignmentsService.create(data, currentUser); } + @ApiOperation({ summary: 'Create Assignments in Bulk' }) + @Post('bulk') + @RouteAccess({ action: 'create', subject: 'Assignment' }) + createBulk(@Body() data: CreateBulkAssignmentsDto, @CurrentUser() currentUser: RequestUser): Promise { + return this.assignmentsService.createBulk(data, currentUser); + } + @ApiOperation({ summary: 'Get All Assignments' }) @Get() @RouteAccess({ action: 'read', subject: 'Assignment' }) diff --git a/apps/api/src/assignments/assignments.service.ts b/apps/api/src/assignments/assignments.service.ts index 2695e4496..9b5218c1b 100644 --- a/apps/api/src/assignments/assignments.service.ts +++ b/apps/api/src/assignments/assignments.service.ts @@ -1,27 +1,54 @@ import crypto from 'node:crypto'; +import type { webcrypto } from 'node:crypto'; import { HybridCrypto } from '@douglasneuroinformatics/libcrypto'; -import { ConfigService, InjectModel } from '@douglasneuroinformatics/libnest'; +import { ConfigService, InjectModel, LoggingService } from '@douglasneuroinformatics/libnest'; import type { Model, RequestUser } from '@douglasneuroinformatics/libnest'; -import { Injectable, NotFoundException } from '@nestjs/common'; -import type { Assignment, UpdateAssignmentData } from '@opendatacapture/schemas/assignment'; +import { ForbiddenException, Injectable, NotFoundException, UnprocessableEntityException } from '@nestjs/common'; +import type { + Assignment, + BulkAssignmentFailure, + BulkAssignmentIssue, + BulkAssignmentPreflightData, + BulkAssignmentPreflightResult, + CreateBulkAssignmentsData, + UpdateAssignmentData +} from '@opendatacapture/schemas/assignment'; import { AuditLogger } from '@/audit/audit.logger'; -import { accessibleQuery } from '@/auth/ability.utils'; +import { accessibleQuery, forcedAppSubject } from '@/auth/ability.utils'; import type { EntityOperationOptions } from '@/core/types'; import { GatewayService } from '@/gateway/gateway.service'; import { CreateAssignmentDto } from './dto/create-assignment.dto'; +/** + * How many assignments a batch prepares at once. Key generation is CPU-bound, so an unbounded + * `Promise.all` over 500 subjects would hold the event loop for the whole batch and stall every + * other request the process is serving. + */ +const BULK_KEYPAIR_CONCURRENCY = 16; + +function chunk(values: T[], size: number): T[][] { + const chunks: T[][] = []; + for (let i = 0; i < values.length; i += size) { + chunks.push(values.slice(i, i + size)); + } + return chunks; +} + @Injectable() export class AssignmentsService { private readonly assignmentBaseUrl: string; constructor( @InjectModel('Assignment') private readonly assignmentModel: Model<'Assignment'>, + @InjectModel('Group') private readonly groupModel: Model<'Group'>, + @InjectModel('Subject') private readonly subjectModel: Model<'Subject'>, configService: ConfigService, private readonly auditLogger: AuditLogger, - private readonly gatewayService: GatewayService + private readonly gatewayService: GatewayService, + private readonly loggingService: LoggingService ) { if (configService.get('NODE_ENV') === 'production') { const siteAddress = configService.getOrThrow('GATEWAY_SITE_ADDRESS'); @@ -32,6 +59,21 @@ export class AssignmentsService { } } + /** + * Validate a whole batch without writing anything, so the client can show the user exactly what + * is wrong before they commit. Returns the shape of the operation when it is clear, and throws + * with every issue attached when it is not — a bulk operation is all-or-nothing, so there is no + * such thing as a partially acceptable request. + */ + async bulkPreflight(data: BulkAssignmentPreflightData, currentUser: RequestUser) { + const { subjectIds, timepoints } = await this.resolveBulkRequest(data, currentUser); + return { + assignmentCount: subjectIds.length * timepoints.length, + subjectCount: subjectIds.length, + timepointCount: timepoints.length + } satisfies BulkAssignmentPreflightResult; + } + async create( { expiresAt, groupId, instrumentId, subjectId }: CreateAssignmentDto, currentUser: RequestUser @@ -77,6 +119,47 @@ export class AssignmentsService { return assignment; } + /** + * Create one assignment per subject per timepoint, or none at all. + * + * The checks preflight ran are repeated here rather than trusted: preflight is advisory and a + * conflicting assignment can appear between the user reviewing the batch and submitting it. If + * anything fails — validation, staging, or the gateway — every row staged by this call is deleted + * and the caller is told what was wrong, leaving the instance exactly as it was. + */ + async createBulk(data: CreateBulkAssignmentsData, currentUser: RequestUser): Promise { + const { groupId, subjectIds, timepoints } = await this.resolveBulkRequest(data, currentUser); + + const staged: { assignment: Assignment; publicKey: webcrypto.CryptoKey }[] = []; + try { + // Bounded concurrency: key generation is CPU-bound, and a 500-subject batch spawning every + // keypair at once would starve the event loop for the rest of the process. + for (const batch of chunk( + timepoints.flatMap(({ expiresAt, instrumentId }) => + subjectIds.map((subjectId) => ({ expiresAt, instrumentId, subjectId })) + ), + BULK_KEYPAIR_CONCURRENCY + )) { + staged.push(...(await Promise.all(batch.map((row) => this.stageAssignment({ ...row, groupId }))))); + } + await this.gatewayService.createRemoteAssignments(staged); + } catch (err) { + await this.discardStagedAssignments(staged); + throw err; + } + + await this.auditLogger.log('CREATE', 'ASSIGNMENT', { + groupId, + metadata: { + createdCount: String(staged.length), + mode: 'BULK', + requestedCount: String(subjectIds.length * timepoints.length) + }, + userId: currentUser.id + }); + return staged.map(({ assignment }) => assignment); + } + async find( { subjectId }: { subjectId?: string } = {}, { ability }: EntityOperationOptions = {} @@ -121,4 +204,129 @@ export class AssignmentsService { } }); } + + /** Remove rows staged by a failed batch. Best effort — a cleanup failure must not mask the cause. */ + private async discardStagedAssignments(staged: { assignment: Assignment }[]): Promise { + if (staged.length === 0) { + return; + } + try { + await this.assignmentModel.deleteMany({ + where: { id: { in: staged.map(({ assignment }) => assignment.id) } } + }); + } catch (err) { + this.loggingService.error({ + error: err, + message: 'ERROR: Failed to roll back staged bulk assignments' + }); + } + } + + /** + * Every authorization and validity check a bulk operation depends on, in one place so preflight + * and create cannot drift apart. Throws with all issues attached; returns the resolved request + * when there are none. + */ + private async resolveBulkRequest( + { allowDuplicates, groupId, subjectIds, timepoints }: BulkAssignmentPreflightData, + { ability, id: userId }: RequestUser + ) { + // A group the caller cannot read is indistinguishable from one that does not exist. + const group = await this.groupModel.findFirst({ + where: { AND: [accessibleQuery(ability, 'read', 'Group')], id: groupId } + }); + if (!group) { + throw new NotFoundException(`Failed to find group with ID: ${groupId}`); + } + if (!ability.can('create', forcedAppSubject('Assignment', { groupId }))) { + throw new ForbiddenException('Insufficient permissions to create assignments for this group'); + } + + const issues: BulkAssignmentIssue[] = []; + + // The group's own opt-in list is the authority: an instrument existing is not permission to + // assign it here. + const accessibleInstrumentIds = new Set(group.accessibleInstrumentIds); + const instrumentIds = timepoints.map(({ instrumentId }) => instrumentId); + const unavailableInstrumentIds = instrumentIds.filter((id) => !accessibleInstrumentIds.has(id)); + if (unavailableInstrumentIds.length > 0) { + issues.push({ instrumentIds: unavailableInstrumentIds, kind: 'INSTRUMENT_UNAVAILABLE' }); + } + + const availableSubjects = await this.subjectModel.findMany({ + select: { id: true }, + where: { + AND: [accessibleQuery(ability, 'read', 'Subject')], + groupIds: { has: groupId }, + id: { in: subjectIds } + } + }); + const availableSubjectIds = new Set(availableSubjects.map(({ id }) => id)); + const unavailableSubjectIds = subjectIds.filter((id) => !availableSubjectIds.has(id)); + if (unavailableSubjectIds.length > 0) { + issues.push({ kind: 'SUBJECT_UNAVAILABLE', subjectIds: unavailableSubjectIds }); + } + + // A conflict is an assignment this group already has outstanding and unexpired for the same + // subject and instrument — reassigning would give the participant two live links to the same + // instrument. The caller may accept that, but only by saying so explicitly. + if (!allowDuplicates && unavailableInstrumentIds.length === 0 && unavailableSubjectIds.length === 0) { + const existing = await this.assignmentModel.findMany({ + select: { instrumentId: true, subjectId: true }, + where: { + expiresAt: { gt: new Date() }, + groupId, + instrumentId: { in: instrumentIds }, + status: 'OUTSTANDING', + subjectId: { in: subjectIds } + } + }); + if (existing.length > 0) { + issues.push({ + conflicts: existing.map(({ instrumentId, subjectId }) => ({ instrumentId, subjectId })), + kind: 'CONFLICT' + }); + } + } + + if (issues.length > 0) { + throw new UnprocessableEntityException({ + code: 'BULK_ASSIGNMENT_REFUSED', + issues + } satisfies BulkAssignmentFailure); + } + return { groupId, subjectIds, timepoints, userId }; + } + + /** Create the Mongo row and keypair for a single assignment within a batch. */ + private async stageAssignment({ + expiresAt, + groupId, + instrumentId, + subjectId + }: { + expiresAt: Date; + groupId: string; + instrumentId: string; + subjectId: string; + }): Promise<{ assignment: Assignment; publicKey: webcrypto.CryptoKey }> { + const { privateKey, publicKey } = await HybridCrypto.generateKeyPair(); + const id = crypto.randomUUID(); + const assignment = await this.assignmentModel.create({ + data: { + encryptionKeyPair: { + privateKey: Buffer.from(await HybridCrypto.serializePrivateKey(privateKey)), + publicKey: Buffer.from(await HybridCrypto.serializePublicKey(publicKey)) + }, + expiresAt, + group: { connect: { id: groupId } }, + id, + instrument: { connect: { id: instrumentId } }, + status: 'OUTSTANDING', + subject: { connect: { id: subjectId } }, + url: `${this.assignmentBaseUrl}/assignments/${id}` + } + }); + return { assignment, publicKey }; + } } diff --git a/apps/api/src/assignments/dto/bulk-assignment.dto.ts b/apps/api/src/assignments/dto/bulk-assignment.dto.ts new file mode 100644 index 000000000..b06d621bc --- /dev/null +++ b/apps/api/src/assignments/dto/bulk-assignment.dto.ts @@ -0,0 +1,38 @@ +import { ValidationSchema } from '@douglasneuroinformatics/libnest'; +import { ApiProperty } from '@nestjs/swagger'; +import { $BulkAssignmentPreflightData, $CreateBulkAssignmentsData } from '@opendatacapture/schemas/assignment'; +import type { + BulkAssignmentPreflightData, + BulkAssignmentTimepoint, + CreateBulkAssignmentsData +} from '@opendatacapture/schemas/assignment'; + +@ValidationSchema($BulkAssignmentPreflightData) +export class BulkAssignmentPreflightDto implements BulkAssignmentPreflightData { + @ApiProperty() + allowDuplicates: boolean; + + @ApiProperty() + groupId: string; + + @ApiProperty() + subjectIds: string[]; + + @ApiProperty() + timepoints: BulkAssignmentTimepoint[]; +} + +@ValidationSchema($CreateBulkAssignmentsData) +export class CreateBulkAssignmentsDto implements CreateBulkAssignmentsData { + @ApiProperty() + allowDuplicates: boolean; + + @ApiProperty() + groupId: string; + + @ApiProperty() + subjectIds: string[]; + + @ApiProperty() + timepoints: BulkAssignmentTimepoint[]; +} diff --git a/apps/api/src/gateway/gateway.service.ts b/apps/api/src/gateway/gateway.service.ts index 6f40e6940..ecaf04ba6 100644 --- a/apps/api/src/gateway/gateway.service.ts +++ b/apps/api/src/gateway/gateway.service.ts @@ -8,6 +8,7 @@ import { $MutateAssignmentResponseBody, $RemoteAssignment } from '@opendatacaptu import type { Assignment, CreateRemoteAssignmentInputData, + CreateRemoteAssignmentsInputData, MutateAssignmentResponseBody, RemoteAssignment } from '@opendatacapture/schemas/assignment'; @@ -46,6 +47,43 @@ export class GatewayService { return $MutateAssignmentResponseBody.parseAsync(response.data); } + /** + * Send a whole batch to the gateway in one request. + * + * Each distinct instrument's bundle is fetched once and sent once, no matter how many assignments + * reference it — a batch is a handful of instruments across hundreds of subjects, so fetching per + * assignment would re-read and re-transmit the same compiled bundle hundreds of times. + * + * The gateway writes the batch in a transaction, so this either persists in full or not at all. + */ + async createRemoteAssignments( + entries: { assignment: Assignment; publicKey: webcrypto.CryptoKey }[] + ): Promise { + const instrumentIds = [...new Set(entries.map(({ assignment }) => assignment.instrumentId))]; + const instruments = await Promise.all( + instrumentIds.map(async (instrumentId) => ({ + instrumentContainer: await this.instrumentsService.findBundleById(instrumentId), + instrumentId + })) + ); + const assignments = await Promise.all( + entries.map(async ({ assignment, publicKey }) => ({ + ...assignment, + publicKey: Array.from(await HybridCrypto.serializePublicKey(publicKey)) + })) + ); + const response = await this.httpService.axiosRef.post(`/api/assignments/bulk`, { + assignments, + instruments + } satisfies CreateRemoteAssignmentsInputData); + if (response.status !== HttpStatus.CREATED) { + throw new BadGatewayException(`Unexpected Status Code From Gateway: ${response.status}`, { + cause: response.statusText + }); + } + return $MutateAssignmentResponseBody.parseAsync(response.data); + } + async deleteRemoteAssignment(id: string): Promise { const response = await this.httpService.axiosRef.delete(`/api/assignments/${id}`); if (response.status !== HttpStatus.OK) { diff --git a/apps/gateway/package.json b/apps/gateway/package.json index 297ce32fa..c0bf1658f 100644 --- a/apps/gateway/package.json +++ b/apps/gateway/package.json @@ -14,6 +14,7 @@ "dev:test": "NODE_ENV=test env-cmd -f ../../.env tsx scripts/dev.ts && env-cmd -f ../../.env node dist/main.js", "format": "prettier --write src", "lint": "tsc && eslint --fix src", + "test": "env-cmd -f ../../.env vitest", "start": "NODE_ENV=production env-cmd -f ../../.env node dist/main.js" }, "dependencies": { diff --git a/apps/gateway/src/routers/__tests__/api.router.test.ts b/apps/gateway/src/routers/__tests__/api.router.test.ts new file mode 100644 index 000000000..dbcb91ce8 --- /dev/null +++ b/apps/gateway/src/routers/__tests__/api.router.test.ts @@ -0,0 +1,59 @@ +import { $CreateRemoteAssignmentsData } from '@opendatacapture/schemas/assignment'; +import { describe, expect, it } from 'vitest'; + +const instrumentContainer = { + bundle: 'export default {}', + id: 'instrument-1', + kind: 'FORM' +}; + +const assignment = { + completedAt: null, + createdAt: new Date().toISOString(), + expiresAt: new Date(Date.now() + 86_400_000).toISOString(), + groupId: 'group-1', + id: 'assignment-1', + instrumentId: 'instrument-1', + publicKey: [1, 2, 3], + status: 'OUTSTANDING', + subjectId: 'subject-1', + url: 'http://localhost:3500/assignments/assignment-1' +}; + +describe('$CreateRemoteAssignmentsData', () => { + it('should accept a batch carrying one container for many assignments, so a bundle is not repeated per row', () => { + const result = $CreateRemoteAssignmentsData.safeParse({ + assignments: [ + assignment, + { ...assignment, id: 'assignment-2', subjectId: 'subject-2' }, + { ...assignment, id: 'assignment-3', subjectId: 'subject-3' } + ], + instruments: [{ instrumentContainer, instrumentId: 'instrument-1' }] + }); + expect(result.success).toBe(true); + expect(result.data?.instruments).toHaveLength(1); + expect(result.data?.assignments).toHaveLength(3); + }); + + it('should reject a batch with no assignments, so an empty request is not a silent no-op', () => { + const result = $CreateRemoteAssignmentsData.safeParse({ + assignments: [], + instruments: [{ instrumentContainer, instrumentId: 'instrument-1' }] + }); + expect(result.success).toBe(false); + }); + + it('should reject a batch with no instruments, since every assignment resolves its bundle by id', () => { + const result = $CreateRemoteAssignmentsData.safeParse({ assignments: [assignment], instruments: [] }); + expect(result.success).toBe(false); + }); + + it('should reject an assignment that names no instrument, which could not be paired to a container', () => { + const { instrumentId: _instrumentId, ...withoutInstrument } = assignment; + const result = $CreateRemoteAssignmentsData.safeParse({ + assignments: [withoutInstrument], + instruments: [{ instrumentContainer, instrumentId: 'instrument-1' }] + }); + expect(result.success).toBe(false); + }); +}); diff --git a/apps/gateway/src/routers/api.router.ts b/apps/gateway/src/routers/api.router.ts index 6b64db71e..a6e5bc9ae 100644 --- a/apps/gateway/src/routers/api.router.ts +++ b/apps/gateway/src/routers/api.router.ts @@ -1,5 +1,9 @@ import { HybridCrypto } from '@douglasneuroinformatics/libcrypto'; -import { $CreateRemoteAssignmentData, $UpdateRemoteAssignmentData } from '@opendatacapture/schemas/assignment'; +import { + $CreateRemoteAssignmentData, + $CreateRemoteAssignmentsData, + $UpdateRemoteAssignmentData +} from '@opendatacapture/schemas/assignment'; import type { AssignmentStatus, MutateAssignmentResponseBody, @@ -62,6 +66,42 @@ router.post( }) ); +// Declared before `/assignments/:id` so `bulk` is not captured as an id by the routes below. +router.post( + '/assignments/bulk', + ah(async (req, res) => { + const result = await $CreateRemoteAssignmentsData.safeParseAsync(req.body); + if (!result.success) { + logger.error(result.error.issues); + throw new HttpException(400, 'Bad Request'); + } + const { assignments, instruments } = result.data; + + const containerByInstrumentId = new Map( + instruments.map(({ instrumentContainer, instrumentId }) => [instrumentId, instrumentContainer]) + ); + // Resolve every bundle before writing anything: a batch referencing an instrument the caller + // did not send is a malformed request, not a partially valid one. + const records = assignments.map(({ instrumentId, publicKey, ...assignment }) => { + const instrumentContainer = containerByInstrumentId.get(instrumentId); + if (!instrumentContainer) { + throw new HttpException(400, `Missing instrument container for assignment: ${assignment.id}`); + } + return { + ...assignment, + rawPublicKey: Buffer.from(publicKey), + targetStringified: JSON.stringify(instrumentContainer) + }; + }); + + // All-or-nothing: the core API deletes its own staged rows when this call fails, so a batch + // that half-succeeded here would leave assignment links live with no record on the other side. + await prisma.$transaction(records.map((data) => prisma.remoteAssignmentModel.create({ data }))); + + res.status(201).send({ success: true } satisfies MutateAssignmentResponseBody); + }) +); + router.patch( '/assignments/:id', ah(async (req, res) => { diff --git a/apps/gateway/tsconfig.json b/apps/gateway/tsconfig.json index e8c8e921e..744b086d6 100644 --- a/apps/gateway/tsconfig.json +++ b/apps/gateway/tsconfig.json @@ -6,5 +6,5 @@ "/runtime/v1/*": ["../../runtime/v1/dist/*"] } }, - "include": ["scripts/*", "src/**/*", "vite.config.ts"] + "include": ["scripts/*", "src/**/*", "vite.config.ts", "vitest.config.ts"] } diff --git a/apps/gateway/vitest.config.ts b/apps/gateway/vitest.config.ts new file mode 100644 index 000000000..3eb07a276 --- /dev/null +++ b/apps/gateway/vitest.config.ts @@ -0,0 +1,20 @@ +import path from 'path'; + +import { defineProject, mergeConfig } from 'vitest/config'; + +import baseConfig from '../../vitest.config'; + +export default mergeConfig( + baseConfig, + defineProject({ + resolve: { + alias: { + '@': path.resolve(import.meta.dirname, 'src') + } + }, + test: { + name: 'gateway', + root: import.meta.dirname + } + }) +); diff --git a/packages/schemas/src/assignment/assignment.test.ts b/packages/schemas/src/assignment/assignment.test.ts new file mode 100644 index 000000000..2271b1eb4 --- /dev/null +++ b/packages/schemas/src/assignment/assignment.test.ts @@ -0,0 +1,144 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import { + $BulkAssignmentFailure, + $BulkAssignmentIssue, + $BulkAssignmentPreflightData, + $CreateAssignmentData, + $CreateBulkAssignmentsData, + BULK_ASSIGNMENT_MAX_SUBJECTS +} from './assignment.js'; + +const futureDate = () => new Date(Date.now() + 86_400_000); + +const baseRequest = { + groupId: 'group-1', + subjectIds: ['subject-1', 'subject-2'], + timepoints: [{ expiresAt: futureDate(), instrumentId: 'instrument-1' }] +}; + +const subjectIds = (count: number) => Array.from({ length: count }, (_, index) => `subject-${index}`); + +afterEach(() => { + vi.useRealTimers(); +}); + +describe('$CreateAssignmentData', () => { + it('should reject an expiry in the past', () => { + expect( + $CreateAssignmentData.safeParse({ + expiresAt: new Date(Date.now() - 1000), + instrumentId: 'instrument-1', + subjectId: 'subject-1' + }).success + ).toBe(false); + }); + + it('should compare expiry against the current time rather than when the module was loaded, so a long-running process does not keep accepting a boundary that has since passed', () => { + const expiresAt = futureDate(); + expect($CreateAssignmentData.safeParse({ expiresAt, instrumentId: 'i', subjectId: 's' }).success).toBe(true); + + // Advance well past that expiry: a schema that captured `new Date()` at import time would still + // accept it, because its lower bound never moves. + vi.useFakeTimers(); + vi.setSystemTime(new Date(expiresAt.getTime() + 86_400_000)); + expect($CreateAssignmentData.safeParse({ expiresAt, instrumentId: 'i', subjectId: 's' }).success).toBe(false); + }); +}); + +describe('$BulkAssignmentPreflightData', () => { + it('should accept a well-formed request and default allowDuplicates to false, so a conflict is never waived implicitly', () => { + const result = $BulkAssignmentPreflightData.safeParse(baseRequest); + expect(result.success).toBe(true); + expect(result.data?.allowDuplicates).toBe(false); + }); + + it(`should accept exactly ${BULK_ASSIGNMENT_MAX_SUBJECTS} subjects`, () => { + const result = $BulkAssignmentPreflightData.safeParse({ + ...baseRequest, + subjectIds: subjectIds(BULK_ASSIGNMENT_MAX_SUBJECTS) + }); + expect(result.success).toBe(true); + }); + + it(`should reject ${BULK_ASSIGNMENT_MAX_SUBJECTS + 1} subjects`, () => { + const result = $BulkAssignmentPreflightData.safeParse({ + ...baseRequest, + subjectIds: subjectIds(BULK_ASSIGNMENT_MAX_SUBJECTS + 1) + }); + expect(result.success).toBe(false); + }); + + it('should reject duplicate subject ids, which would assign the same person twice in one batch', () => { + const result = $BulkAssignmentPreflightData.safeParse({ + ...baseRequest, + subjectIds: ['subject-1', 'subject-1'] + }); + expect(result.success).toBe(false); + }); + + it('should reject the same instrument at two timepoints, which would give one subject two live links to it', () => { + const result = $BulkAssignmentPreflightData.safeParse({ + ...baseRequest, + timepoints: [ + { expiresAt: futureDate(), instrumentId: 'instrument-1' }, + { expiresAt: futureDate(), instrumentId: 'instrument-1' } + ] + }); + expect(result.success).toBe(false); + }); + + it('should reject an empty subject list', () => { + expect($BulkAssignmentPreflightData.safeParse({ ...baseRequest, subjectIds: [] }).success).toBe(false); + }); + + it('should reject an empty timepoint list', () => { + expect($BulkAssignmentPreflightData.safeParse({ ...baseRequest, timepoints: [] }).success).toBe(false); + }); +}); + +describe('$CreateBulkAssignmentsData', () => { + it('should reject a timepoint whose expiry is in the past', () => { + const result = $CreateBulkAssignmentsData.safeParse({ + ...baseRequest, + timepoints: [{ expiresAt: new Date(Date.now() - 1000), instrumentId: 'instrument-1' }] + }); + expect(result.success).toBe(false); + }); +}); + +describe('$BulkAssignmentIssue', () => { + it('should parse every variant, so a client can exhaustively narrow on kind', () => { + const variants = [ + { conflicts: [{ instrumentId: 'instrument-1', subjectId: 'subject-1' }], kind: 'CONFLICT' }, + { instrumentIds: ['instrument-1'], kind: 'INSTRUMENT_UNAVAILABLE' }, + { kind: 'SUBJECT_UNAVAILABLE', subjectIds: ['subject-1'] } + ]; + for (const variant of variants) { + expect($BulkAssignmentIssue.safeParse(variant).success).toBe(true); + } + }); + + it('should reject an unknown kind', () => { + expect($BulkAssignmentIssue.safeParse({ kind: 'SOMETHING_ELSE' }).success).toBe(false); + }); + + it('should reject an issue variant carrying no detail, which would tell the user nothing', () => { + expect($BulkAssignmentIssue.safeParse({ conflicts: [], kind: 'CONFLICT' }).success).toBe(false); + expect($BulkAssignmentIssue.safeParse({ kind: 'SUBJECT_UNAVAILABLE', subjectIds: [] }).success).toBe(false); + }); +}); + +describe('$BulkAssignmentFailure', () => { + it('should parse a refusal carrying its issues', () => { + const result = $BulkAssignmentFailure.safeParse({ + code: 'BULK_ASSIGNMENT_REFUSED', + issues: [{ kind: 'SUBJECT_UNAVAILABLE', subjectIds: ['subject-1'] }] + }); + expect(result.success).toBe(true); + }); + + it('should reject a refusal with no issues, since the user could not be told what to fix', () => { + expect($BulkAssignmentFailure.safeParse({ code: 'BULK_ASSIGNMENT_REFUSED', issues: [] }).success).toBe(false); + }); +}); diff --git a/packages/schemas/src/assignment/assignment.ts b/packages/schemas/src/assignment/assignment.ts index 98d09cded..3054e7263 100644 --- a/packages/schemas/src/assignment/assignment.ts +++ b/packages/schemas/src/assignment/assignment.ts @@ -3,18 +3,18 @@ import { z } from 'zod/v4'; import { $BaseModel, $Json } from '../core/core.js'; import { $InstrumentBundleContainer } from '../instrument/instrument.base.js'; -export const $AssignmentStatus = z.enum(['CANCELED', 'COMPLETE', 'EXPIRED', 'OUTSTANDING']); +const $AssignmentStatus = z.enum(['CANCELED', 'COMPLETE', 'EXPIRED', 'OUTSTANDING']); -export type AssignmentStatus = z.infer; +type AssignmentStatus = z.infer; /** Fallback validity period (in days) for a new remote assignment when the instance has not configured one. */ -export const DEFAULT_ASSIGNMENT_DURATION_DAYS = 365; +const DEFAULT_ASSIGNMENT_DURATION_DAYS = 365; /** * An self-contained object representing an assignment. */ -export type Assignment = z.infer; -export const $Assignment = $BaseModel.extend({ +type Assignment = z.infer; +const $Assignment = $BaseModel.extend({ completedAt: z.coerce.date().nullable(), expiresAt: z.coerce.date(), groupId: z.string().min(1).nullish(), @@ -24,41 +24,205 @@ export const $Assignment = $BaseModel.extend({ url: z.string().url() }); -export type RemoteAssignment = z.infer; -export const $RemoteAssignment = $Assignment.omit({ instrumentId: true, updatedAt: true }).extend({ +type RemoteAssignment = z.infer; +const $RemoteAssignment = $Assignment.omit({ instrumentId: true, updatedAt: true }).extend({ encryptedData: z.string().nullable(), symmetricKey: z.string().nullable() }); +/** The largest number of subjects one bulk operation may target. */ +const BULK_ASSIGNMENT_MAX_SUBJECTS = 500; + +/** + * An expiry that must still be in the future *when the request is validated*. + * + * `z.date().min(new Date())` would freeze the comparison at the moment this module is imported, + * which in a long-running API process is whenever it booted — so a stale boundary would keep + * accepting expiries that have since passed. The check has to read the clock per parse. + */ +const $FutureDate = z.coerce.date().refine((value) => value.getTime() > Date.now(), { + message: 'Expiry must be in the future' +}); + +const $UniqueStrings = z + .array(z.string().min(1)) + .min(1) + .refine((values) => new Set(values).size === values.length, { message: 'Values must be unique' }); + /** The DTO transferred from the web client to the core API when creating an assignment */ -export type CreateAssignmentData = z.infer; -export const $CreateAssignmentData = z.object({ - expiresAt: z.coerce.date().min(new Date()), +type CreateAssignmentData = z.infer; +const $CreateAssignmentData = z.object({ + expiresAt: $FutureDate, groupId: z.string().nullish(), instrumentId: z.string(), subjectId: z.string() }); +/** + * One instrument and the expiry it is assigned with. A bulk operation carries a list of these and + * applies every one of them to every selected subject, so N subjects and M timepoints create N * M + * assignments. + */ +type BulkAssignmentTimepoint = z.infer; +const $BulkAssignmentTimepoint = z.object({ + expiresAt: $FutureDate, + instrumentId: z.string().min(1) +}); + +/** + * Shared by preflight and create so the client cannot validate against one shape and submit + * another. `allowDuplicates` is the caller's explicit acknowledgement of the conflicts preflight + * reported; without it a conflict fails the whole operation. + */ +const $BulkAssignmentRequestBase = z.object({ + allowDuplicates: z.boolean().default(false), + groupId: z.string().min(1), + subjectIds: $UniqueStrings.max(BULK_ASSIGNMENT_MAX_SUBJECTS), + timepoints: z + .array($BulkAssignmentTimepoint) + .min(1) + .refine((values) => new Set(values.map(({ instrumentId }) => instrumentId)).size === values.length, { + message: 'Each instrument may only be assigned once' + }) +}); + +type BulkAssignmentPreflightData = z.infer; +const $BulkAssignmentPreflightData = $BulkAssignmentRequestBase; + +type CreateBulkAssignmentsData = z.infer; +const $CreateBulkAssignmentsData = $BulkAssignmentRequestBase; + +/** + * Why a bulk operation was refused. `SUBJECT_UNAVAILABLE` deliberately does not distinguish a + * subject that does not exist from one outside the selected group — telling them apart would let a + * caller probe for subjects in groups they cannot read. + */ +type BulkAssignmentIssue = z.infer; +const $BulkAssignmentIssue = z.discriminatedUnion('kind', [ + z.object({ + conflicts: z + .array( + z.object({ + instrumentId: z.string().min(1), + subjectId: z.string().min(1) + }) + ) + .min(1), + kind: z.literal('CONFLICT') + }), + z.object({ + instrumentIds: $UniqueStrings, + kind: z.literal('INSTRUMENT_UNAVAILABLE') + }), + z.object({ + kind: z.literal('SUBJECT_UNAVAILABLE'), + subjectIds: $UniqueStrings + }) +]); + +/** + * The body returned with a non-2xx status from either bulk route. A bulk operation is + * all-or-nothing: whenever this is returned, nothing was created and nothing was changed. + */ +type BulkAssignmentFailure = z.infer; +const $BulkAssignmentFailure = z.object({ + code: z.literal('BULK_ASSIGNMENT_REFUSED'), + issues: z.array($BulkAssignmentIssue).min(1) +}); + +type BulkAssignmentPreflightResult = z.infer; +const $BulkAssignmentPreflightResult = z.object({ + assignmentCount: z.number().int().nonnegative(), + subjectCount: z.number().int().nonnegative(), + timepointCount: z.number().int().nonnegative() +}); + /** The DTO transferred from the core API to the external gateway when creating an assignment. */ -export type CreateRemoteAssignmentInputData = z.input; -export const $CreateRemoteAssignmentData = $RemoteAssignment.omit({ encryptedData: true, symmetricKey: true }).extend({ +type CreateRemoteAssignmentInputData = z.input; +const $CreateRemoteAssignmentData = $RemoteAssignment.omit({ encryptedData: true, symmetricKey: true }).extend({ instrumentContainer: $InstrumentBundleContainer, publicKey: $Uint8ArrayLike }); -export type MutateAssignmentResponseBody = z.infer; -export const $MutateAssignmentResponseBody = z.object({ +/** + * The DTO transferred from the core API to the external gateway when creating a batch. + * + * Bundles are carried in `instruments` and referenced by id from each assignment, rather than + * inlined per assignment as the single-assignment DTO does: a batch applies a handful of + * instruments to hundreds of subjects, so inlining would repeat every compiled bundle hundreds of + * times over the wire. + */ +type CreateRemoteAssignmentsInputData = z.input; +const $CreateRemoteAssignmentsData = z.object({ + assignments: z + .array( + $RemoteAssignment.omit({ encryptedData: true, symmetricKey: true }).extend({ + instrumentId: z.string().min(1), + publicKey: $Uint8ArrayLike + }) + ) + .min(1), + instruments: z + .array( + z.object({ + instrumentContainer: $InstrumentBundleContainer, + instrumentId: z.string().min(1) + }) + ) + .min(1) +}); + +type MutateAssignmentResponseBody = z.infer; +const $MutateAssignmentResponseBody = z.object({ success: z.boolean() }); -export type UpdateAssignmentData = z.infer; -export const $UpdateAssignmentData = z.object({ +type UpdateAssignmentData = z.infer; +const $UpdateAssignmentData = z.object({ status: $AssignmentStatus }); -export type UpdateRemoteAssignmentData = z.infer; -export const $UpdateRemoteAssignmentData = z.object({ +type UpdateRemoteAssignmentData = z.infer; +const $UpdateRemoteAssignmentData = z.object({ data: $Json.optional(), kind: z.enum(['SERIES', 'SCALAR']), status: z.literal('COMPLETE').optional() }); + +export type { + Assignment, + AssignmentStatus, + BulkAssignmentFailure, + BulkAssignmentIssue, + BulkAssignmentPreflightData, + BulkAssignmentPreflightResult, + BulkAssignmentTimepoint, + CreateAssignmentData, + CreateBulkAssignmentsData, + CreateRemoteAssignmentInputData, + CreateRemoteAssignmentsInputData, + MutateAssignmentResponseBody, + RemoteAssignment, + UpdateAssignmentData, + UpdateRemoteAssignmentData +}; + +export { + $Assignment, + $AssignmentStatus, + $BulkAssignmentFailure, + $BulkAssignmentIssue, + $BulkAssignmentPreflightData, + $BulkAssignmentPreflightResult, + $BulkAssignmentTimepoint, + $CreateAssignmentData, + $CreateBulkAssignmentsData, + $CreateRemoteAssignmentData, + $CreateRemoteAssignmentsData, + $MutateAssignmentResponseBody, + $RemoteAssignment, + $UpdateAssignmentData, + $UpdateRemoteAssignmentData, + BULK_ASSIGNMENT_MAX_SUBJECTS, + DEFAULT_ASSIGNMENT_DURATION_DAYS +}; From 88648a3bbad7fda50c39af6d4a697e10d00e33fc Mon Sep 17 00:00:00 2001 From: thomasbeaudry Date: Thu, 3 Sep 2026 00:31:42 -0400 Subject: [PATCH 2/3] fix(api): keep the encryption keypair out of bulk responses A staged assignment was returned straight from the model, so the private key that decrypts it was serialized into the HTTP response and posted to the gateway. Assignment does not declare the field, but structural typing does not remove it at runtime, and the gateway's schema only discards it after it has crossed the wire. Also answers preflight with 200 rather than the 201 a POST defaults to, since it creates nothing. Co-Authored-By: Claude Opus 5 --- .../__tests__/assignments.service.spec.ts | 12 ++++++++++++ .../api/src/assignments/assignments.controller.ts | 15 ++++++++++++++- apps/api/src/assignments/assignments.service.ts | 8 +++++++- 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/apps/api/src/assignments/__tests__/assignments.service.spec.ts b/apps/api/src/assignments/__tests__/assignments.service.spec.ts index 1c39a5ba9..5e7774637 100644 --- a/apps/api/src/assignments/__tests__/assignments.service.spec.ts +++ b/apps/api/src/assignments/__tests__/assignments.service.spec.ts @@ -184,6 +184,18 @@ describe('AssignmentsService', () => { expect(assignmentModel.create).toHaveBeenCalledTimes(4); }); + it('should never return or transmit the encryption keypair, which would hand out the private key', async () => { + assignmentModel.create.mockImplementation(({ data }: any) => + Promise.resolve({ ...data, encryptionKeyPair: { privateKey: 'SECRET', publicKey: 'PUB' } }) + ); + const assignments = await assignmentsService.createBulk(request(), permissiveUser()); + + expect(assignments.every((assignment) => !('encryptionKeyPair' in assignment))).toBe(true); + const sent = gatewayService.createRemoteAssignments.mock.lastCall?.[0] as { assignment: object }[]; + expect(sent.every(({ assignment }) => !('encryptionKeyPair' in assignment))).toBe(true); + expect(JSON.stringify(sent)).not.toContain('SECRET'); + }); + it('should send the whole batch to the gateway in a single call, so one bundle is fetched per instrument', async () => { await assignmentsService.createBulk(request(), permissiveUser()); expect(gatewayService.createRemoteAssignments).toHaveBeenCalledTimes(1); diff --git a/apps/api/src/assignments/assignments.controller.ts b/apps/api/src/assignments/assignments.controller.ts index f945d6e41..ce51ad6db 100644 --- a/apps/api/src/assignments/assignments.controller.ts +++ b/apps/api/src/assignments/assignments.controller.ts @@ -1,6 +1,17 @@ import { CurrentUser } from '@douglasneuroinformatics/libnest'; import type { RequestUser } from '@douglasneuroinformatics/libnest'; -import { BadRequestException, Body, Controller, Get, Param, Patch, Post, Query } from '@nestjs/common'; +import { + BadRequestException, + Body, + Controller, + Get, + HttpCode, + HttpStatus, + Param, + Patch, + Post, + Query +} from '@nestjs/common'; import { ApiOperation } from '@nestjs/swagger'; import { Throttle } from '@nestjs/throttler'; import type { Assignment, BulkAssignmentPreflightResult } from '@opendatacapture/schemas/assignment'; @@ -38,6 +49,8 @@ export class AssignmentsController { // `RouteAccess` cannot express is enforced in the service, which also checks `read Subject` through // `accessibleQuery` before any subject is used. @ApiOperation({ summary: 'Validate a Bulk Assignment Request' }) + // Creates nothing, so it answers 200 rather than the 201 a POST defaults to in Nest. + @HttpCode(HttpStatus.OK) @Post('bulk/preflight') @RouteAccess({ action: 'create', subject: 'Assignment' }) bulkPreflight( diff --git a/apps/api/src/assignments/assignments.service.ts b/apps/api/src/assignments/assignments.service.ts index 9b5218c1b..9644430ba 100644 --- a/apps/api/src/assignments/assignments.service.ts +++ b/apps/api/src/assignments/assignments.service.ts @@ -327,6 +327,12 @@ export class AssignmentsService { url: `${this.assignmentBaseUrl}/assignments/${id}` } }); - return { assignment, publicKey }; + // Drop the keypair before this row travels anywhere. `Assignment` does not declare it, but the + // model carries it, and structural typing does not remove a field at runtime: left in, the + // private key that decrypts this assignment would be serialized into the HTTP response and + // posted to the gateway. The gateway's own schema discards it, but only after it crossed the + // wire. + const { encryptionKeyPair: _encryptionKeyPair, ...withoutKeyPair } = assignment; + return { assignment: withoutKeyPair, publicKey }; } } From c88b7b3781b5a55bafc75f20c9ae44832d012f09 Mon Sep 17 00:00:00 2001 From: thomasbeaudry Date: Thu, 3 Sep 2026 00:47:47 -0400 Subject: [PATCH 3/3] test(e2e): cover bulk remote assignments end to end Drives the API rather than the UI, so it needs no page object and no regenerated route tree. Covers what the unit tier structurally cannot: real Mongo scoping, the real gateway round trip, and the all-or-nothing guarantee across both. Verified to fail without the behaviour it asserts: breaking the conflict query's status filter turns the repeat preflight from 422 into 200 and fails the suite. Also drops a dead userId binding found while reviewing. Co-Authored-By: Claude Opus 5 --- .../src/assignments/assignments.service.ts | 4 +- .../src/specs/bulk-remote-assignments.spec.ts | 180 ++++++++++++++++++ 2 files changed, 182 insertions(+), 2 deletions(-) create mode 100644 testing/src/specs/bulk-remote-assignments.spec.ts diff --git a/apps/api/src/assignments/assignments.service.ts b/apps/api/src/assignments/assignments.service.ts index 9644430ba..a0f308217 100644 --- a/apps/api/src/assignments/assignments.service.ts +++ b/apps/api/src/assignments/assignments.service.ts @@ -229,7 +229,7 @@ export class AssignmentsService { */ private async resolveBulkRequest( { allowDuplicates, groupId, subjectIds, timepoints }: BulkAssignmentPreflightData, - { ability, id: userId }: RequestUser + { ability }: RequestUser ) { // A group the caller cannot read is indistinguishable from one that does not exist. const group = await this.groupModel.findFirst({ @@ -295,7 +295,7 @@ export class AssignmentsService { issues } satisfies BulkAssignmentFailure); } - return { groupId, subjectIds, timepoints, userId }; + return { groupId, subjectIds, timepoints }; } /** Create the Mongo row and keypair for a single assignment within a batch. */ diff --git a/testing/src/specs/bulk-remote-assignments.spec.ts b/testing/src/specs/bulk-remote-assignments.spec.ts new file mode 100644 index 000000000..737fc0505 --- /dev/null +++ b/testing/src/specs/bulk-remote-assignments.spec.ts @@ -0,0 +1,180 @@ +import { BULK_ASSIGNMENT_MAX_SUBJECTS } from '@opendatacapture/schemas/assignment'; +import type { Assignment, BulkAssignmentFailure } from '@opendatacapture/schemas/assignment'; + +import { expect, test } from '../support/fixtures'; + +/** + * Bulk remote assignments, driven through the API rather than the UI. + * + * This is the tier that exercises the parts unit tests cannot: real Mongo scoping, the real gateway + * round trip, and the all-or-nothing guarantee across both. The wizard has its own coverage. + */ + +const API = '/api/v1'; + +const futureIso = (days: number) => new Date(Date.now() + days * 86_400_000).toISOString(); + +test.describe('bulk remote assignments', () => { + /** + * A group with its own subjects. `api.createGroup` grants the group every instrument, and each + * subject is linked to the group by creating a session for it — the same path the app uses. + */ + const seedGroup = async ( + { adminToken, api, apiRequestContext }: { adminToken: string; api: any; apiRequestContext: any }, + uniqueId: string, + subjectCount: number + ) => { + const group = await api.createGroup({ name: `Bulk${uniqueId}` }); + const subjectIds: string[] = []; + for (let index = 0; index < subjectCount; index++) { + const id = `bulk_${uniqueId}_${index}`; + const response = await apiRequestContext.post(`${API}/sessions`, { + data: { + date: new Date().toISOString(), + groupId: group.id, + subjectData: { id }, + type: 'IN_PERSON' + }, + headers: { Authorization: `Bearer ${adminToken}` } + }); + expect(response.status(), await response.text()).toBe(201); + subjectIds.push(id); + } + return { group, subjectIds }; + }; + + const instrumentIdsFor = async (apiRequestContext: any, adminToken: string, count: number) => { + const response = await apiRequestContext.get(`${API}/instruments/info`, { + headers: { Authorization: `Bearer ${adminToken}` } + }); + const info = (await response.json()) as { id: string; kind: string }[]; + return info + .filter(({ kind }) => kind === 'FORM' || kind === 'INTERACTIVE') + .slice(0, count) + .map(({ id }) => id); + }; + + const post = (apiRequestContext: any, adminToken: string, path: string, data: unknown) => + apiRequestContext.post(`${API}${path}`, { data, headers: { Authorization: `Bearer ${adminToken}` } }); + + test('should create one assignment per subject per timepoint, and refuse a repeat as a conflict', async ({ + adminToken, + api, + apiRequestContext, + uniqueId + }) => { + const { group, subjectIds } = await seedGroup({ adminToken, api, apiRequestContext }, uniqueId, 2); + const [instrumentA, instrumentB] = await instrumentIdsFor(apiRequestContext, adminToken, 2); + const request = { + groupId: group.id, + subjectIds, + timepoints: [ + { expiresAt: futureIso(30), instrumentId: instrumentA }, + { expiresAt: futureIso(60), instrumentId: instrumentB } + ] + }; + + const preflight = await post(apiRequestContext, adminToken, '/assignments/bulk/preflight', request); + expect(preflight.status()).toBe(200); + expect(await preflight.json()).toMatchObject({ assignmentCount: 4, subjectCount: 2, timepointCount: 2 }); + + const created = await post(apiRequestContext, adminToken, '/assignments/bulk', request); + expect(created.status()).toBe(201); + const assignments = (await created.json()) as Assignment[]; + expect(assignments).toHaveLength(4); + + // The private key that decrypts an assignment must never reach a client. + expect(JSON.stringify(assignments)).not.toContain('encryptionKeyPair'); + + // Every assignment really exists, and is scoped to this group. + for (const subjectId of subjectIds) { + const response = await apiRequestContext.get(`${API}/assignments?subjectId=${subjectId}`, { + headers: { Authorization: `Bearer ${adminToken}` } + }); + const existing = (await response.json()) as Assignment[]; + expect(existing.filter((assignment) => assignment.groupId === group.id)).toHaveLength(2); + } + + // Re-running the same batch is now a conflict for every pair, and creates nothing. + const repeat = await post(apiRequestContext, adminToken, '/assignments/bulk/preflight', request); + expect(repeat.status()).toBe(422); + const failure = (await repeat.json()) as BulkAssignmentFailure; + const conflict = failure.issues.find((issue) => issue.kind === 'CONFLICT'); + expect(conflict?.kind === 'CONFLICT' && conflict.conflicts).toHaveLength(4); + + // The caller may accept the duplicates, but only by saying so. + const waived = await post(apiRequestContext, adminToken, '/assignments/bulk/preflight', { + ...request, + allowDuplicates: true + }); + expect(waived.status()).toBe(200); + }); + + test('should report a subject from another group as unavailable, without creating anything', async ({ + adminToken, + api, + apiRequestContext, + uniqueId + }) => { + const { group, subjectIds } = await seedGroup({ adminToken, api, apiRequestContext }, uniqueId, 1); + const other = await seedGroup({ adminToken, api, apiRequestContext }, `${uniqueId}x`, 1); + const [instrumentId] = await instrumentIdsFor(apiRequestContext, adminToken, 1); + + const response = await post(apiRequestContext, adminToken, '/assignments/bulk', { + groupId: group.id, + subjectIds: [...subjectIds, ...other.subjectIds], + timepoints: [{ expiresAt: futureIso(30), instrumentId }] + }); + expect(response.status()).toBe(422); + const failure = (await response.json()) as BulkAssignmentFailure; + const issue = failure.issues.find(({ kind }) => kind === 'SUBJECT_UNAVAILABLE'); + expect(issue?.kind === 'SUBJECT_UNAVAILABLE' && issue.subjectIds).toEqual(other.subjectIds); + + // All-or-nothing: the subject that *was* eligible must not have been assigned either. + const assignments = await apiRequestContext.get(`${API}/assignments?subjectId=${subjectIds[0]}`, { + headers: { Authorization: `Bearer ${adminToken}` } + }); + expect((await assignments.json()) as Assignment[]).toHaveLength(0); + }); + + test('should refuse an instrument the group has not been granted', async ({ + adminToken, + api, + apiRequestContext, + uniqueId + }) => { + const { group, subjectIds } = await seedGroup({ adminToken, api, apiRequestContext }, uniqueId, 1); + const response = await post(apiRequestContext, adminToken, '/assignments/bulk/preflight', { + groupId: group.id, + subjectIds, + timepoints: [{ expiresAt: futureIso(30), instrumentId: 'not-an-instrument' }] + }); + expect(response.status()).toBe(422); + const failure = (await response.json()) as BulkAssignmentFailure; + expect(failure.issues.some(({ kind }) => kind === 'INSTRUMENT_UNAVAILABLE')).toBe(true); + }); + + test('should reject an expiry in the past and a batch beyond the subject limit', async ({ + adminToken, + api, + apiRequestContext, + uniqueId + }) => { + const { group, subjectIds } = await seedGroup({ adminToken, api, apiRequestContext }, uniqueId, 1); + const [instrumentId] = await instrumentIdsFor(apiRequestContext, adminToken, 1); + + const expired = await post(apiRequestContext, adminToken, '/assignments/bulk', { + groupId: group.id, + subjectIds, + timepoints: [{ expiresAt: new Date(Date.now() - 86_400_000).toISOString(), instrumentId }] + }); + expect(expired.status()).toBe(400); + + const tooMany = await post(apiRequestContext, adminToken, '/assignments/bulk/preflight', { + groupId: group.id, + subjectIds: Array.from({ length: BULK_ASSIGNMENT_MAX_SUBJECTS + 1 }, (_, index) => `s-${index}`), + timepoints: [{ expiresAt: futureIso(30), instrumentId }] + }); + expect(tooMany.status()).toBe(400); + }); +});