From d14bad9a52c9f07aaa21bdd1f37c76b99b522b1b Mon Sep 17 00:00:00 2001 From: joshunrau Date: Sun, 8 Nov 2026 21:20:27 -0500 Subject: [PATCH 01/10] fix(api): scope instrument record updates by update rules PATCH /v1/instrument-records/:id is gated on `update InstrumentRecord`, but InstrumentRecordsService.updateById looked the record up with no scope at all and then scoped the write by the caller's `delete` rules. Both now use the caller's `update` rules, and the ability is a required parameter, so a record outside the caller's scope is a 404 before the payload is merged or validated. Admins hold `manage all` and are unaffected. No base level below admin holds `update` or `delete` on records, so this changes access only for users granted those actions through custom permissions or a permission set: - A user granted `update InstrumentRecord` without `delete` can now edit records within that grant. Before, every such request failed with a 500. - A user whose `update` and `delete` grants name different groups now edits records in the groups their `update` grant names, and can no longer edit records in groups only their `delete` grant names. - A payload sent for a record outside the caller's scope now answers 404 instead of being merged and validated against it, so a 400 no longer reveals that the id exists. Admins should review such grants before upgrading. Closes #1633 Co-Authored-By: Claude Opus 5.5 --- .../instrument-records.service.test.ts | 59 ++++++++++++---- .../instrument-records.service.ts | 13 ++-- testing/src/specs/authorization.spec.ts | 69 +++++++++++++++++++ testing/src/support/api-client.ts | 9 +++ 4 files changed, 131 insertions(+), 19 deletions(-) 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..99ccdf259 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 @@ -1199,6 +1199,8 @@ describe('InstrumentRecordsService', () => { }); describe('updateById', () => { + const ability = createAppAbility([{ action: 'update', subject: 'InstrumentRecord' }]); + const useRecord = (data: unknown) => { instrumentRecordModel.findFirst.mockResolvedValueOnce({ data, id: 'record-1', instrumentId: 'instrument-1' }); }; @@ -1219,7 +1221,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', {}, { ability })).rejects.toBeInstanceOf( + NotFoundException + ); expect(instrumentRecordModel.update).not.toHaveBeenCalled(); }); @@ -1227,7 +1231,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 }, { ability })).rejects.toBeInstanceOf( BadRequestException ); @@ -1238,7 +1242,7 @@ describe('InstrumentRecordsService', () => { useRecord({ answer: 1, comment: 'kept' }); useInstrument(); - await instrumentRecordsService.updateById('record-1', { answer: 2 }); + await instrumentRecordsService.updateById('record-1', { answer: 2 }, { ability }); expect(instrumentRecordModel.update.mock.lastCall?.[0]).toMatchObject({ data: { computedMeasures: null, data: { answer: 2, comment: 'kept' } } @@ -1250,7 +1254,7 @@ describe('InstrumentRecordsService', () => { useRecord({ answers: [1, 2, 3] }); useInstrument(); - await instrumentRecordsService.updateById('record-1', { answers: [9] }); + await instrumentRecordsService.updateById('record-1', { answers: [9] }, { ability }); expect(instrumentRecordModel.update.mock.lastCall?.[0]).toMatchObject({ data: { data: { answers: [1, 2, 3] } } }); }); @@ -1259,7 +1263,7 @@ describe('InstrumentRecordsService', () => { useRecord([{ answer: 1, comment: 'kept' }]); useInstrument(); - await instrumentRecordsService.updateById('record-1', [{ answer: 2 }]); + await instrumentRecordsService.updateById('record-1', [{ answer: 2 }], { ability }); expect(instrumentRecordModel.update.mock.lastCall?.[0]).toMatchObject({ data: { data: [{ answer: 2, comment: 'kept' }] } @@ -1273,9 +1277,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' }, { ability })).rejects.toMatchObject( + { + response: { issues, message: 'Merged data does not match validation schema' } + } + ); expect(instrumentRecordModel.update).not.toHaveBeenCalled(); }); @@ -1285,7 +1291,7 @@ describe('InstrumentRecordsService', () => { useInstrument({ measures }); instrumentMeasuresService.computeMeasures.mockReturnValueOnce({ score: 2 }); - await instrumentRecordsService.updateById('record-1', { answer: 2 }); + await instrumentRecordsService.updateById('record-1', { answer: 2 }, { ability }); expect(instrumentMeasuresService.computeMeasures).toHaveBeenCalledWith(measures, { answer: 2 }); expect(instrumentRecordModel.update.mock.lastCall?.[0]).toMatchObject({ @@ -1293,11 +1299,10 @@ describe('InstrumentRecordsService', () => { }); }); - // Pins a reported defect: the PATCH route is gated on update, but the write is scoped by delete rules. - // Expected to change when the source is fixed. - it("should scope the write by the caller's delete rules", async () => { + it("should scope the write by the caller's update rules, which the route is gated on, not their delete rules", async () => { const ability = createAppAbility([ - { action: 'delete', conditions: { groupId: 'group-1' }, subject: 'InstrumentRecord' } + { action: 'update', conditions: { groupId: 'group-1' }, subject: 'InstrumentRecord' }, + { action: 'delete', conditions: { groupId: 'group-2' }, subject: 'InstrumentRecord' } ]); useRecord({ answer: 1 }); useInstrument(); @@ -1305,10 +1310,36 @@ describe('InstrumentRecordsService', () => { await instrumentRecordsService.updateById('record-1', { answer: 2 }, { ability }); expect(instrumentRecordModel.update.mock.lastCall?.[0].where).toStrictEqual({ - AND: [accessibleQuery(ability, 'delete', 'InstrumentRecord')], + AND: [accessibleQuery(ability, 'update', 'InstrumentRecord')], id: 'record-1' }); }); + + it('should let a caller holding an update rule but no delete rule write the record', async () => { + useRecord({ answer: 1 }); + useInstrument(); + + await expect(instrumentRecordsService.updateById('record-1', { answer: 2 }, { ability })).resolves.toStrictEqual({ + id: 'record-1' + }); + }); + + it("should look the record up within the caller's update scope, so one outside it is not found before the payload is merged or validated", async () => { + instrumentRecordModel.findFirst.mockResolvedValueOnce(null); + const ability = createAppAbility([ + { action: 'update', conditions: { groupId: 'other-group' }, subject: 'InstrumentRecord' } + ]); + + await expect( + instrumentRecordsService.updateById('record-1', { answer: 'x' }, { ability }) + ).rejects.toBeInstanceOf(NotFoundException); + + expect(instrumentRecordModel.findFirst).toHaveBeenCalledWith({ + where: { AND: [accessibleQuery(ability, 'update', 'InstrumentRecord')], id: 'record-1' } + }); + expect(instrumentsService.findById).not.toHaveBeenCalled(); + expect(instrumentRecordModel.update).not.toHaveBeenCalled(); + }); }); describe('summarizeBySubject', () => { diff --git a/apps/api/src/instrument-records/instrument-records.service.ts b/apps/api/src/instrument-records/instrument-records.service.ts index ff655a94e..0f6e68509 100644 --- a/apps/api/src/instrument-records/instrument-records.service.ts +++ b/apps/api/src/instrument-records/instrument-records.service.ts @@ -434,10 +434,13 @@ export class InstrumentRecordsService { })); } - async updateById(id: string, data: unknown[] | { [key: string]: unknown }, { ability }: EntityOperationOptions = {}) { - const instrumentRecord = await this.instrumentRecordModel.findFirst({ - where: { id } - }); + async updateById( + id: string, + data: unknown[] | { [key: string]: unknown }, + { ability }: Required> + ) { + const where = { AND: [accessibleQuery(ability, 'update', 'InstrumentRecord')], id }; + const instrumentRecord = await this.instrumentRecordModel.findFirst({ where }); if (!instrumentRecord) { throw new NotFoundException(`Could not find record with ID '${id}'`); } @@ -471,7 +474,7 @@ export class InstrumentRecordsService { : null, data: parseResult.data }, - where: { AND: [accessibleQuery(ability, 'delete', 'InstrumentRecord')], id } + where }); } diff --git a/testing/src/specs/authorization.spec.ts b/testing/src/specs/authorization.spec.ts index 18d9a0bb5..cb6d788ec 100644 --- a/testing/src/specs/authorization.spec.ts +++ b/testing/src/specs/authorization.spec.ts @@ -6,6 +6,7 @@ import type { User } from '@opendatacapture/schemas/user'; import type { APIRequestContext, APIResponse } from '@playwright/test'; import { ApiClient } from '../support/api-client'; +import { HAPPINESS_RECORD } from '../support/constants'; import { expect, test } from '../support/fixtures'; import type { Role } from '../support/types'; @@ -321,6 +322,31 @@ function uploadRecord(request: APIRequestContext, token: string, data: UploadIns return request.post(`${API}/instrument-records/upload`, { data, headers: { Authorization: `Bearer ${token}` } }); } +/** + * Signs in a STANDARD member of the group granted `update InstrumentRecord` there and nothing else on + * records, since no base level below admin holds `update` or `delete` on them. + */ +async function loginRecordEditor(api: ApiClient, request: APIRequestContext, groupId: string): Promise { + const { credentials, user } = await api.createUser({ basePermissionLevel: 'STANDARD', groupIds: [groupId] }); + await api.setUserPermissions(user.id, [{ action: 'update', groupId, subject: 'InstrumentRecord' }]); + return ApiClient.login(request, credentials); +} + +/** A happiness questionnaire record for a new subject in the group, seeded as admin. */ +async function seedHappinessRecord(api: ApiClient, groupId: string, subjectId: string): Promise { + const instrumentId = await api.findInstrumentIdByName('DNP_HAPPINESS_QUESTIONNAIRE'); + const session = await api.createSession(groupId, { id: subjectId }); + return api.createRecord(groupId, instrumentId, session, HAPPINESS_RECORD); +} + +/** `PATCH /instrument-records/:id` as the holder of the given token, with the raw response. */ +function patchRecord(request: APIRequestContext, token: string, id: string, data: { [key: string]: unknown }) { + return request.patch(`${API}/instrument-records/${id}`, { + data: { data }, + headers: { Authorization: `Bearer ${token}` } + }); +} + /** `POST /assignments` as the holder of the given token, expiring tomorrow, with the raw response. */ function createAssignment(request: APIRequestContext, token: string, data: Omit<$CreateAssignmentData, 'expiresAt'>) { return request.post(`${API}/assignments`, { @@ -760,6 +786,49 @@ test.describe('server-side authorization', () => { expect(updated.additionalPermissions).toStrictEqual([{ action: 'create', groupId: null, subject: 'Instrument' }]); }); + // A grant is the only way a non-admin reaches `PATCH /instrument-records/:id`. The write used to be + // scoped by the caller's `delete` rules rather than the `update` rules the route is gated on, so a + // holder of `update` alone passed the guard and then had every write refused. + test('should let a user granted update on records, without delete, edit one in the group the grant names', async ({ + api, + apiRequestContext, + uniqueId + }) => { + const group = await api.createGroup({ name: `Editing Group ${uniqueId}` }); + const token = await loginRecordEditor(api, apiRequestContext, group.id); + const record = await seedHappinessRecord(api, group.id, `Edited${uniqueId}`); + + const response = await patchRecord(apiRequestContext, token, record.id, { personalLifeSatisfaction: 3 }); + + expect(response.status()).toBe(200); + expect((await api.findRecordById(record.id)).data).toStrictEqual({ + ...HAPPINESS_RECORD, + personalLifeSatisfaction: 3 + }); + }); + + // The lookup the payload is merged into is scoped as well. Before it was, an invalid payload for + // another group's record answered 400 where an unknown id answers 404, which reported the id real. + test('should answer an edit to a record outside the update grant exactly as for one that does not exist', async ({ + api, + apiRequestContext, + uniqueId + }) => { + const group = await api.createGroup({ name: `Editing Group ${uniqueId}` }); + const foreignGroup = await api.createGroup({ name: `Foreign Editing Group ${uniqueId}` }); + const token = await loginRecordEditor(api, apiRequestContext, group.id); + const record = await seedHappinessRecord(api, foreignGroup.id, `Foreign${uniqueId}`); + const invalid = { personalLifeSatisfaction: 'not a number' }; + + const foreign = await patchRecord(apiRequestContext, token, record.id, invalid); + const absent = await patchRecord(apiRequestContext, token, '0'.repeat(24), invalid); + + expect(foreign.status(), 'a record outside the grant must not be distinguishable from an absent one').toBe( + absent.status() + ); + expect(foreign.status()).toBe(404); + }); + // The import route answers with the rows it wrote, and used to find them by re-querying the // instrument with the request's optional `groupId` as the only filter. Omitting `groupId` therefore // returned every group's records for that instrument to the lowest role, which `create diff --git a/testing/src/support/api-client.ts b/testing/src/support/api-client.ts index bf3331af8..b2750949e 100644 --- a/testing/src/support/api-client.ts +++ b/testing/src/support/api-client.ts @@ -292,6 +292,15 @@ export class ApiClient { ); } + /** Reads a record back as admin, to check whether a write by another user actually took effect. */ + async findRecordById(id: string): Promise { + return this.expectJson( + this.request.get(`${API}/instrument-records/${id}`, { headers: this.authHeaders }), + 200, + `find instrument record '${id}'` + ); + } + /** The ids the start-session form suggests for a group: its subjects identified by a custom id. */ async findSubjectCustomIds(groupId: string): Promise { return this.expectJson( From 2b1b6f677b4a2082f704cc99d54f54f333c8f6e7 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Sun, 8 Nov 2026 20:59:10 -0500 Subject: [PATCH 02/10] fix(api): refuse remote assignment of file instruments A remote assignment has no way to deliver a patient's files: the gateway submits only the result data, so the import either writes a record that stays pending forever (storage enabled) or fails and retries on every synchronizer pass (storage disabled). Bulk upload already refuses file instruments for the same reason. POST /v1/assignments, POST /v1/assignments/bulk and the bulk preflight now report a FILE instrument, and a series with a FILE item, as INSTRUMENT_UNAVAILABLE (422). The check replaces AssignmentsService's archived-instrument lookup with InstrumentsService.findUnassignableIds, which evaluates each requested bundle (and each item of a series) and still refuses archived series. In-person sessions are unaffected. Note for administrators: remote assignments of file instruments created before this change are left in place. They can never deliver their files, so cancel any that are still outstanding by hand from the subject's Assignments tab. Records they already produced remain pending and hold no recoverable data. Refs #1708 Co-Authored-By: Claude Opus 5.5 --- .../__tests__/assignments.service.test.ts | 64 +++++++++++++++-- .../api/src/assignments/assignments.module.ts | 3 +- .../src/assignments/assignments.service.ts | 24 ++----- .../__tests__/instruments.service.test.ts | 71 +++++++++++++++++++ .../src/instruments/instruments.service.ts | 44 +++++++++++- 5 files changed, 181 insertions(+), 25 deletions(-) diff --git a/apps/api/src/assignments/__tests__/assignments.service.test.ts b/apps/api/src/assignments/__tests__/assignments.service.test.ts index 8bd4c1f27..526c07db8 100644 --- a/apps/api/src/assignments/__tests__/assignments.service.test.ts +++ b/apps/api/src/assignments/__tests__/assignments.service.test.ts @@ -13,6 +13,7 @@ import { AuditLogger } from '@/audit/audit.logger'; import { AbilityFactory } from '@/auth/ability.factory'; import { createAppAbility } from '@/auth/ability.utils'; import { GatewayService } from '@/gateway/gateway.service'; +import { InstrumentsService } from '@/instruments/instruments.service'; import { AssignmentsService } from '../assignments.service'; @@ -82,10 +83,10 @@ describe('AssignmentsService', () => { let assignmentsService: AssignmentsService; let assignmentModel: MockedInstance>; let groupModel: MockedInstance>; - let instrumentModel: MockedInstance>; let subjectModel: MockedInstance>; let auditLogger: MockedInstance; let gatewayService: MockedInstance; + let instrumentsService: MockedInstance; let loggingService: MockedInstance; /** Compiles a fresh service against `config`, since the participant link base is fixed at construction. */ @@ -95,7 +96,6 @@ describe('AssignmentsService', () => { AssignmentsService, MockFactory.createForModelToken(getModelToken('Assignment')), MockFactory.createForModelToken(getModelToken('Group')), - MockFactory.createForModelToken(getModelToken('Instrument')), MockFactory.createForModelToken(getModelToken('Subject')), { provide: AuditLogger, useValue: { log: vi.fn() } }, { provide: ConfigService, useValue: config }, @@ -107,23 +107,24 @@ describe('AssignmentsService', () => { deleteRemoteAssignment: vi.fn() } }, + { provide: InstrumentsService, useValue: { findUnassignableIds: vi.fn() } }, { provide: LoggingService, useValue: { error: vi.fn() } } ] }).compile(); assignmentModel = moduleRef.get(getModelToken('Assignment')); groupModel = moduleRef.get(getModelToken('Group')); - instrumentModel = moduleRef.get(getModelToken('Instrument')); subjectModel = moduleRef.get(getModelToken('Subject')); auditLogger = moduleRef.get(AuditLogger); gatewayService = moduleRef.get(GatewayService); + instrumentsService = moduleRef.get(InstrumentsService); loggingService = moduleRef.get(LoggingService); 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([]); - instrumentModel.findMany.mockResolvedValue([]); + instrumentsService.findUnassignableIds.mockResolvedValue([]); assignmentModel.create.mockImplementation(({ data }: any) => { return Promise.resolve({ ...data, instrumentId: 'instrument-1' }); }); @@ -175,11 +176,29 @@ describe('AssignmentsService', () => { // An archived series stays on the group's opt-in list so unarchiving restores it, so that list // alone would still admit it. it('should refuse an archived series the group has opted into, since archiving retires it from new assignments', async () => { - instrumentModel.findMany.mockResolvedValueOnce([{ id: 'instrument-1' }]); + instrumentsService.findUnassignableIds.mockResolvedValueOnce(['instrument-1']); const failure = await failureOf(assignmentsService.bulkPreflight(request(), permissiveUser())); expect(failure.issues).toContainEqual({ instrumentIds: ['instrument-1'], kind: 'INSTRUMENT_UNAVAILABLE' }); }); + it('should ask which of every requested instrument cannot be assigned remotely, whatever the group opted into', async () => { + await failureOf( + assignmentsService.bulkPreflight( + request({ + timepoints: [ + { expiresAt: futureDate(), instrumentId: 'instrument-1' }, + { expiresAt: futureDate(), instrumentId: 'instrument-other' } + ] + }), + permissiveUser() + ) + ); + expect(instrumentsService.findUnassignableIds).toHaveBeenCalledExactlyOnceWith([ + 'instrument-1', + 'instrument-other' + ]); + }); + 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({ @@ -251,6 +270,21 @@ describe('AssignmentsService', () => { expect(failure.issues).toContainEqual({ instrumentIds: ['instrument-other'], kind: 'INSTRUMENT_UNAVAILABLE' }); }); + it('should refuse to assign a file instrument, since the gateway has no way to upload its files', async () => { + instrumentsService.findUnassignableIds.mockResolvedValueOnce(['instrument-1']); + const failure = await failureOf(assignmentsService.create(data(), userAt('GROUP_MANAGER'))); + expect(failure.issues).toContainEqual({ instrumentIds: ['instrument-1'], kind: 'INSTRUMENT_UNAVAILABLE' }); + expect(gatewayService.createRemoteAssignment).not.toHaveBeenCalled(); + }); + + it('should refuse an administrator an ungrouped assignment of a file instrument, which skips the group checks', async () => { + instrumentsService.findUnassignableIds.mockResolvedValueOnce(['instrument-1']); + const failure = await failureOf(assignmentsService.create({ ...data(), groupId: undefined }, userAt('ADMIN'))); + expect(failure.issues).toContainEqual({ instrumentIds: ['instrument-1'], kind: 'INSTRUMENT_UNAVAILABLE' }); + expect(instrumentsService.findUnassignableIds).toHaveBeenCalledExactlyOnceWith(['instrument-1']); + expect(gatewayService.createRemoteAssignment).not.toHaveBeenCalled(); + }); + it('should refuse a subject outside the group, so its participant link cannot be issued elsewhere', async () => { subjectModel.findMany.mockResolvedValueOnce([]); const failure = await failureOf(assignmentsService.create(data(), userAt('GROUP_MANAGER'))); @@ -287,7 +321,7 @@ describe('AssignmentsService', () => { }); it('should refuse an administrator an ungrouped assignment of an archived series, which skips the group checks', async () => { - instrumentModel.findMany.mockResolvedValueOnce([{ id: 'instrument-1' }]); + instrumentsService.findUnassignableIds.mockResolvedValueOnce(['instrument-1']); const failure = await failureOf(assignmentsService.create({ ...data(), groupId: undefined }, userAt('ADMIN'))); expect(failure.issues).toContainEqual({ instrumentIds: ['instrument-1'], kind: 'INSTRUMENT_UNAVAILABLE' }); expect(assignmentModel.create).not.toHaveBeenCalled(); @@ -396,6 +430,24 @@ describe('AssignmentsService', () => { expect(gatewayService.createRemoteAssignments).not.toHaveBeenCalled(); }); + it('should refuse to assign a file instrument, since the gateway has no way to upload its files', async () => { + instrumentsService.findUnassignableIds.mockResolvedValueOnce(['instrument-2']); + const failure = await failureOf( + assignmentsService.createBulk( + request({ + timepoints: [ + { expiresAt: futureDate(), instrumentId: 'instrument-1' }, + { expiresAt: futureDate(), instrumentId: 'instrument-2' } + ] + }), + permissiveUser() + ) + ); + expect(failure.issues).toContainEqual({ instrumentIds: ['instrument-2'], kind: 'INSTRUMENT_UNAVAILABLE' }); + 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); diff --git a/apps/api/src/assignments/assignments.module.ts b/apps/api/src/assignments/assignments.module.ts index 122fdce46..067ebdfb1 100644 --- a/apps/api/src/assignments/assignments.module.ts +++ b/apps/api/src/assignments/assignments.module.ts @@ -2,6 +2,7 @@ import { forwardRef, Module } from '@nestjs/common'; import { GatewayModule } from '@/gateway/gateway.module'; import { GroupsModule } from '@/groups/groups.module'; +import { InstrumentsModule } from '@/instruments/instruments.module'; import { MailModule } from '@/mail/mail.module'; import { AssignmentsController } from './assignments.controller'; @@ -10,7 +11,7 @@ import { AssignmentsService } from './assignments.service'; @Module({ controllers: [AssignmentsController], exports: [AssignmentsService], - imports: [forwardRef(() => GatewayModule), GroupsModule, MailModule], + imports: [forwardRef(() => GatewayModule), GroupsModule, InstrumentsModule, MailModule], providers: [AssignmentsService] }) export class AssignmentsModule {} diff --git a/apps/api/src/assignments/assignments.service.ts b/apps/api/src/assignments/assignments.service.ts index a6e6e491b..c5379504a 100644 --- a/apps/api/src/assignments/assignments.service.ts +++ b/apps/api/src/assignments/assignments.service.ts @@ -20,6 +20,7 @@ import { AuditLogger } from '@/audit/audit.logger'; import { accessibleQuery, forcedAppSubject } from '@/auth/ability.utils'; import type { EntityOperationOptions } from '@/core/types'; import { GatewayService } from '@/gateway/gateway.service'; +import { InstrumentsService } from '@/instruments/instruments.service'; /** * How many assignments a batch prepares at once. Key generation is CPU-bound, so an unbounded @@ -43,11 +44,11 @@ export class AssignmentsService { constructor( @InjectModel('Assignment') private readonly assignmentModel: Model<'Assignment'>, @InjectModel('Group') private readonly groupModel: Model<'Group'>, - @InjectModel('Instrument') private readonly instrumentModel: Model<'Instrument'>, @InjectModel('Subject') private readonly subjectModel: Model<'Subject'>, configService: ConfigService, private readonly auditLogger: AuditLogger, private readonly gatewayService: GatewayService, + private readonly instrumentsService: InstrumentsService, private readonly loggingService: LoggingService ) { if (configService.get('NODE_ENV') === 'production') { @@ -85,7 +86,7 @@ export class AssignmentsService { ); } else if (!currentUser.ability.can('create', forcedAppSubject('Assignment', { groupId: null }))) { throw new ForbiddenException('Insufficient permissions to create an assignment outside a group'); - } else if ((await this.findArchivedInstrumentIds([instrumentId])).length > 0) { + } else if ((await this.instrumentsService.findUnassignableIds([instrumentId])).length > 0) { throw new UnprocessableEntityException({ code: 'BULK_ASSIGNMENT_REFUSED', issues: [{ instrumentIds: [instrumentId], kind: 'INSTRUMENT_UNAVAILABLE' }] @@ -260,18 +261,6 @@ export class AssignmentsService { } } - /** - * Which of the given instruments an administrator has archived. Unscoped by the caller's ability: - * it only narrows ids the caller already named, and every other check on them is made separately. - */ - private async findArchivedInstrumentIds(instrumentIds: string[]): Promise { - const archived = await this.instrumentModel.findMany({ - select: { id: true }, - where: { archivedAt: { not: null }, id: { in: instrumentIds } } - }); - return archived.map(({ id }) => id); - } - /** * Every authorization and validity check a grouped assignment depends on, in one place so * preflight, bulk create and single create cannot drift apart. Throws with all issues attached; @@ -296,12 +285,13 @@ export class AssignmentsService { // The group's own opt-in list is the authority: an instrument existing is not permission to // assign it here. An archived series stays on that list, so it can be unarchived without every - // group opting back in, and is refused separately. + // group opting back in, and is refused separately, as is a file instrument, which the group may + // still administer in person. const accessibleInstrumentIds = new Set(group.accessibleInstrumentIds); const instrumentIds = timepoints.map(({ instrumentId }) => instrumentId); - const archivedInstrumentIds = new Set(await this.findArchivedInstrumentIds(instrumentIds)); + const unassignableInstrumentIds = new Set(await this.instrumentsService.findUnassignableIds(instrumentIds)); const unavailableInstrumentIds = [ - ...new Set(instrumentIds.filter((id) => !accessibleInstrumentIds.has(id) || archivedInstrumentIds.has(id))) + ...new Set(instrumentIds.filter((id) => !accessibleInstrumentIds.has(id) || unassignableInstrumentIds.has(id))) ]; if (unavailableInstrumentIds.length > 0) { issues.push({ instrumentIds: unavailableInstrumentIds, kind: 'INSTRUMENT_UNAVAILABLE' }); diff --git a/apps/api/src/instruments/__tests__/instruments.service.test.ts b/apps/api/src/instruments/__tests__/instruments.service.test.ts index 28ae548db..21ad0aa81 100644 --- a/apps/api/src/instruments/__tests__/instruments.service.test.ts +++ b/apps/api/src/instruments/__tests__/instruments.service.test.ts @@ -1373,6 +1373,77 @@ describe('InstrumentsService', () => { }); }); + describe('findUnassignableIds', () => { + const fileInstance = { ...formInstance('FILE_A', 1), kind: 'FILE' as const }; + const instancesByBundle: { [bundle: string]: unknown } = { + 'hash:FILE_A-1:bundle': fileInstance, + 'hash:FORM_A-1:bundle': formInstance('FORM_A', 1), + 'hash:FORM_B-1:bundle': formInstance('FORM_B', 1), + 'series-1:bundle': existingSeries, + 'series-with-file:bundle': { + ...existingSeries, + content: { items: [{ edition: 1, name: 'FORM_A' }, fileInstance.internal] } + } + }; + + /** Stored rows, keyed by id, that both the requested lookup and the series item lookup read. */ + const storeInstruments = (archivedIds: string[] = []) => { + instrumentModel.findMany.mockImplementation(({ where }: any) => { + return Promise.resolve( + (where.id.in as string[]) + .filter((id) => `${id}:bundle` in instancesByBundle) + .map((id) => ({ archivedAt: archivedIds.includes(id) ? new Date() : null, bundle: `${id}:bundle`, id })) + ); + }); + }; + + beforeEach(() => { + vi.spyOn(cryptoService, 'hash').mockImplementation((value) => `hash:${value}`); + virtualizationService.eval.mockImplementation((code) => okAsync(instancesByBundle[code])); + storeInstruments(); + }); + + it('should report a file instrument, since the gateway has no way to upload its files', async () => { + await expect(instrumentsService.findUnassignableIds(['hash:FILE_A-1', 'hash:FORM_A-1'])).resolves.toEqual([ + 'hash:FILE_A-1' + ]); + }); + + it('should report a series with a file item, which the series renderer cannot show either', async () => { + await expect(instrumentsService.findUnassignableIds(['series-with-file', 'series-1'])).resolves.toEqual([ + 'series-with-file' + ]); + }); + + it('should look up the items of a series by their generated ids, since its bundle does not say their kind', async () => { + await instrumentsService.findUnassignableIds(['series-with-file']); + expect(instrumentModel.findMany.mock.lastCall?.[0]).toEqual({ + select: { bundle: true, id: true }, + where: { id: { in: ['hash:FORM_A-1', 'hash:FILE_A-1'] } } + }); + }); + + it('should report an archived instrument without evaluating its bundle', async () => { + storeInstruments(['series-1']); + await expect(instrumentsService.findUnassignableIds(['series-1'])).resolves.toEqual(['series-1']); + expect(virtualizationService.eval).not.toHaveBeenCalled(); + }); + + it('should skip an unknown id rather than throwing, so the caller refuses it under its own rule', async () => { + await expect(instrumentsService.findUnassignableIds(['not-an-instrument', 'hash:FILE_A-1'])).resolves.toEqual([ + 'hash:FILE_A-1' + ]); + }); + + it('should read only the requested instruments, and only what it needs of them', async () => { + await instrumentsService.findUnassignableIds(['hash:FORM_A-1']); + expect(instrumentModel.findMany).toHaveBeenCalledExactlyOnceWith({ + select: { archivedAt: true, bundle: true, id: true }, + where: { id: { in: ['hash:FORM_A-1'] } } + }); + }); + }); + describe('list', () => { let find: MockInstance; diff --git a/apps/api/src/instruments/instruments.service.ts b/apps/api/src/instruments/instruments.service.ts index 150f67645..c11625d98 100644 --- a/apps/api/src/instruments/instruments.service.ts +++ b/apps/api/src/instruments/instruments.service.ts @@ -9,7 +9,12 @@ import { UnprocessableEntityException } from '@nestjs/common'; import { bundle } from '@opendatacapture/instrument-bundler'; -import { getSeriesInstrumentItems, isScalarInstrument, isSeriesInstrument } from '@opendatacapture/instrument-utils'; +import { + getSeriesInstrumentItems, + isFileInstrument, + isScalarInstrument, + isSeriesInstrument +} from '@opendatacapture/instrument-utils'; import type { AnyInstrument, AnyScalarInstrument, @@ -411,6 +416,24 @@ export class InstrumentsService { })); } + /** + * Which of the given instruments cannot be assigned remotely: an archived series, a file + * instrument, whose uploads the gateway has no way to deliver, and a series with a file item, which + * the series renderer cannot show either. An unknown id is skipped, so the caller's own rule decides + * it. Unscoped by the caller's ability: it only narrows ids the caller already named, and every + * other check on them is made separately. + */ + async findUnassignableIds(ids: string[]): Promise { + const instruments = await this.instrumentModel.findMany({ + select: { archivedAt: true, bundle: true, id: true }, + where: { id: { in: ids } } + }); + const isUnassignable = await Promise.all( + instruments.map(async (instrument) => Boolean(instrument.archivedAt) || (await this.collectsFiles(instrument))) + ); + return instruments.filter((_, index) => isUnassignable[index]).map(({ id }) => id); + } + generateInstrumentId(instrument: AnyInstrument, seriesGroupId?: string) { if (isScalarInstrument(instrument)) { return this.generateScalarInstrumentId(instrument); @@ -537,6 +560,25 @@ export class InstrumentsService { >; } + /** + * A series bundle does not say what kind its items are, so each item is evaluated. An item that + * no longer exists is left to `findBundleById`, which refuses it when the assignment is sent. + */ + private async collectsFiles(instrument: Pick): Promise { + const instance = await this.getInstrumentInstance(instrument); + if (!isSeriesInstrument(instance)) { + return isFileInstrument(instance); + } + const itemIds = getSeriesInstrumentItems(instance.content).map((internal) => { + return this.generateScalarInstrumentId({ internal }); + }); + const items = await this.instrumentModel.findMany({ + select: { bundle: true, id: true }, + where: { id: { in: itemIds } } + }); + return (await this.instantiate(items)).some(isFileInstrument); + } + /** * The single source of this message. `InstrumentReposService.importInstrumentFromDir` reads the id * back out of it to associate an already-stored instrument with the repository that provides it, so From 14856b15b175f40ae12ce60cb116d7b939e9ab41 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Sun, 8 Nov 2026 20:59:26 -0500 Subject: [PATCH 03/10] fix(web): hide file instruments from remote assignment The Remote Assignment page and the group's bulk Remote Assignments wizard no longer offer file instruments, which the API now refuses to assign remotely because the gateway cannot deliver a patient's uploads. The in-person instrument list still offers them. A series with a file item is left for the API to refuse, since an item pinned to an older edition is absent from the instrument list the client has. Refs #1708 Co-Authored-By: Claude Opus 5.5 --- .../__tests__/remote-assignments.test.tsx | 13 +++++++++++- .../routes/_app/group/remote-assignments.tsx | 4 ++-- .../__tests__/remote-assignment.test.tsx | 18 +++++++++++++++- .../routes/_app/session/remote-assignment.tsx | 4 ++-- .../administrable-instruments.test.ts | 21 ++++++++++++++++++- .../src/utils/administrable-instruments.ts | 15 ++++++++++++- 6 files changed, 67 insertions(+), 8 deletions(-) diff --git a/apps/web/src/routes/_app/group/__tests__/remote-assignments.test.tsx b/apps/web/src/routes/_app/group/__tests__/remote-assignments.test.tsx index 6efafb91b..839beb8a7 100644 --- a/apps/web/src/routes/_app/group/__tests__/remote-assignments.test.tsx +++ b/apps/web/src/routes/_app/group/__tests__/remote-assignments.test.tsx @@ -20,7 +20,7 @@ type CapturedRouteOptions = { type MockState = { config: { setup: { isGatewayEnabled: boolean } }; - instrumentInfo: undefined | { details: { title: string }; id: string; kind: 'FORM' }[]; + instrumentInfo: undefined | { details: { title: string }; id: string; kind: 'FILE' | 'FORM' }[]; search: { mode?: string }; store: { currentGroup: Group | null }; }; @@ -178,6 +178,17 @@ describe('Route', () => { expect(wizardProps()).toMatchObject({ instruments: [{ id: 'instrument-1', title: 'Allowed' }] }); }); + it('should not offer the wizard a file instrument the group may administer, since the gateway cannot deliver its uploads', () => { + mocks.search = { mode: 'create' }; + mocks.store.currentGroup = { ...createGroup(), accessibleInstrumentIds: ['instrument-1', 'instrument-2'] }; + mocks.instrumentInfo = [ + { details: { title: 'Allowed' }, id: 'instrument-1', kind: 'FORM' }, + { details: { title: 'File Upload' }, id: 'instrument-2', kind: 'FILE' } + ]; + render(); + expect(wizardProps()).toMatchObject({ instruments: [{ id: 'instrument-1', title: 'Allowed' }] }); + }); + it('should offer the wizard no instruments while the instrument list is still loading', () => { mocks.search = { mode: 'create' }; render(); diff --git a/apps/web/src/routes/_app/group/remote-assignments.tsx b/apps/web/src/routes/_app/group/remote-assignments.tsx index 0f115060a..b185144cd 100644 --- a/apps/web/src/routes/_app/group/remote-assignments.tsx +++ b/apps/web/src/routes/_app/group/remote-assignments.tsx @@ -14,7 +14,7 @@ import { useInstrumentInfoQuery } from '@/hooks/useInstrumentInfoQuery'; import { setupStateQueryOptions, useSetupStateQuery } from '@/hooks/useSetupStateQuery'; import { subjectsQueryOptions, useSubjectsQuery } from '@/hooks/useSubjectsQuery'; import { useAppStore } from '@/store'; -import { selectAdministrableInstruments } from '@/utils/administrable-instruments'; +import { selectRemotelyAssignableInstruments } from '@/utils/administrable-instruments'; import { getDefaultAssignmentExpiry } from '@/utils/assignment-duration'; type Mode = 'CREATE' | 'DELETE' | 'LANDING'; @@ -80,7 +80,7 @@ const RouteComponent = () => { } // The API enforces the same rule, so an instrument missing here would be refused there anyway. - const instruments = selectAdministrableInstruments(instrumentInfoQuery.data ?? [], currentGroup).map( + const instruments = selectRemotelyAssignableInstruments(instrumentInfoQuery.data ?? [], currentGroup).map( (instrument) => ({ id: instrument.id, title: instrument.details.title }) ); diff --git a/apps/web/src/routes/_app/session/__tests__/remote-assignment.test.tsx b/apps/web/src/routes/_app/session/__tests__/remote-assignment.test.tsx index e0585e011..89f0ca3ba 100644 --- a/apps/web/src/routes/_app/session/__tests__/remote-assignment.test.tsx +++ b/apps/web/src/routes/_app/session/__tests__/remote-assignment.test.tsx @@ -9,7 +9,12 @@ import { Route } from '@/routes/_app/session/remote-assignment'; import '@/services/i18n'; -type InstrumentInfoStub = { details: { title: string }; id: string; kind: 'FORM'; supportedLanguages: string[] }; +type InstrumentInfoStub = { + details: { title: string }; + id: string; + kind: 'FILE' | 'FORM'; + supportedLanguages: string[]; +}; type MockState = { config: { setup: { isGatewayEnabled: boolean } }; @@ -155,6 +160,17 @@ describe('Route', () => { expect(screen.queryByRole('button', { name: 'Excluded' })).toBeNull(); }); + it('should not offer a file instrument the group may administer, since the gateway cannot deliver its uploads', () => { + mocks.instrumentInfo = [ + allowed, + { ...allowed, details: { title: 'File Upload' }, id: 'instrument-3', kind: 'FILE' } + ]; + mocks.store.currentGroup = { accessibleInstrumentIds: ['instrument-1', 'instrument-3'], id: 'group-1' }; + render(); + expect(screen.getByRole('button', { name: 'Allowed' })).toBeTruthy(); + expect(screen.queryByRole('button', { name: 'File Upload' })).toBeNull(); + }); + it('should show a loading fallback instead of the showcase while the instruments load', () => { mocks.instrumentInfo = undefined; render(); diff --git a/apps/web/src/routes/_app/session/remote-assignment.tsx b/apps/web/src/routes/_app/session/remote-assignment.tsx index 4e738d54b..938f841b1 100644 --- a/apps/web/src/routes/_app/session/remote-assignment.tsx +++ b/apps/web/src/routes/_app/session/remote-assignment.tsx @@ -18,7 +18,7 @@ import { useCreateAssignment } from '@/hooks/useCreateAssignment'; import { useInstrumentInfoQuery } from '@/hooks/useInstrumentInfoQuery'; import { useSetupStateQuery } from '@/hooks/useSetupStateQuery'; import { useAppStore } from '@/store'; -import { selectAdministrableInstruments } from '@/utils/administrable-instruments'; +import { selectRemotelyAssignableInstruments } from '@/utils/administrable-instruments'; import { getDefaultAssignmentExpiry } from '@/utils/assignment-duration'; import { $RedirectUrl, omittedIfBlank } from '@/utils/validation'; @@ -129,7 +129,7 @@ const RouteComponent = () => { { setSelectedInstrument(instrument); setIsCreateModalOpen(true); diff --git a/apps/web/src/utils/__tests__/administrable-instruments.test.ts b/apps/web/src/utils/__tests__/administrable-instruments.test.ts index b4aac66da..e207d0241 100644 --- a/apps/web/src/utils/__tests__/administrable-instruments.test.ts +++ b/apps/web/src/utils/__tests__/administrable-instruments.test.ts @@ -1,10 +1,11 @@ import { describe, expect, it } from 'vitest'; -import { selectAdministrableInstruments } from '../administrable-instruments'; +import { selectAdministrableInstruments, selectRemotelyAssignableInstruments } from '../administrable-instruments'; const archivedSeries = { archivedAt: new Date('2024-06-01'), id: 'archived-series', kind: 'SERIES' as const }; const activeSeries = { archivedAt: null, id: 'active-series', kind: 'SERIES' as const }; const form = { id: 'form', kind: 'FORM' as const }; +const fileUpload = { id: 'file-upload', kind: 'FILE' as const }; const ids = (instruments: { id: string }[]) => instruments.map(({ id }) => id); @@ -29,4 +30,22 @@ describe('selectAdministrableInstruments', () => { 'form' ]); }); + + it('should keep a file instrument, since an in-person session can store its uploads', () => { + expect(ids(selectAdministrableInstruments([fileUpload, form], null))).toEqual(['file-upload', 'form']); + }); +}); + +describe('selectRemotelyAssignableInstruments', () => { + it('should drop a file instrument, since the gateway has no way to deliver its uploads', () => { + const group = { accessibleInstrumentIds: ['file-upload', 'form'] }; + expect(ids(selectRemotelyAssignableInstruments([fileUpload, form], group))).toEqual(['form']); + }); + + it('should drop what an in-person session would also drop', () => { + const group = { accessibleInstrumentIds: ['archived-series', 'active-series'] }; + expect(ids(selectRemotelyAssignableInstruments([archivedSeries, activeSeries, form], group))).toEqual([ + 'active-series' + ]); + }); }); diff --git a/apps/web/src/utils/administrable-instruments.ts b/apps/web/src/utils/administrable-instruments.ts index f9fecfb6f..8daf8101f 100644 --- a/apps/web/src/utils/administrable-instruments.ts +++ b/apps/web/src/utils/administrable-instruments.ts @@ -24,4 +24,17 @@ function selectAdministrableInstruments( + instruments: TInstrument[], + currentGroup: null | { accessibleInstrumentIds: string[] } +): TInstrument[] { + return selectAdministrableInstruments(instruments, currentGroup).filter(({ kind }) => kind !== 'FILE'); +} + +export { selectAdministrableInstruments, selectRemotelyAssignableInstruments }; From b1b2a3badf7e5a9cb6ec2a4d541e26ef272205a0 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Sun, 8 Nov 2026 21:01:02 -0500 Subject: [PATCH 04/10] test(testing): cover refusing remote assignment of file instruments The Remote Assignment page no longer offers Arbitrary File Upload, which the in-person instrument list still offers, and POST /v1/assignments answers 422 INSTRUMENT_UNAVAILABLE for a file instrument, with or without a group, and for a series with a file item, creating nothing. Refs #1708 Co-Authored-By: Claude Opus 5.5 --- testing/src/specs/remote-assignment.spec.ts | 85 +++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/testing/src/specs/remote-assignment.spec.ts b/testing/src/specs/remote-assignment.spec.ts index 2ae6c534c..6c796f473 100644 --- a/testing/src/specs/remote-assignment.spec.ts +++ b/testing/src/specs/remote-assignment.spec.ts @@ -1,3 +1,6 @@ +import type { BulkAssignmentFailure } from '@opendatacapture/schemas/assignment'; + +import { AccessibleInstrumentsPage } from '../pages/_app/instruments/accessible-instruments.page'; import { RemoteAssignmentPage } from '../pages/_app/session/remote-assignment.page'; import { baseURL } from '../support/env'; import { expect, test } from '../support/fixtures'; @@ -61,6 +64,88 @@ test.describe('remote assignment', () => { await expect.poll(() => page.getByRole('menuitemcheckbox').allTextContents()).toEqual(initialOptions); }); + test('should not offer a file instrument for remote assignment, though an in-person session offers it', async ({ + getPageModel, + page, + uniqueId + }) => { + await startSession(getPageModel, `File${uniqueId}`, `Subject${uniqueId}`, 'Female'); + + await page.getByTestId('nav-button-/instruments/accessible-instruments').click(); + await page.waitForURL('**/instruments/accessible-instruments'); + await expect(new AccessibleInstrumentsPage(page).instrumentCard('Arbitrary File Upload')).toBeVisible(); + + await page.getByTestId('nav-button-/session/remote-assignment').click(); + await page.waitForURL('**/session/remote-assignment'); + const remoteAssignmentPage = new RemoteAssignmentPage(page); + await expect(remoteAssignmentPage.instrumentCard('Happiness Questionnaire')).toBeVisible(); + await expect(remoteAssignmentPage.instrumentCard('Arbitrary File Upload')).toHaveCount(0); + }); + + // `api.createGroup` grants every instrument, the file instrument included, so the refusal comes + // from the file rule rather than the group's opt-in list. + test('should refuse to assign a file instrument remotely, with or without a group, and create nothing', async ({ + adminToken, + api, + apiRequestContext + }) => { + const group = await api.createGroup(); + const subjectId = await api.createSubject(group.id); + const instrumentId = await api.findInstrumentId('FILE'); + + for (const groupId of [group.id, undefined]) { + const response = await apiRequestContext.post('/api/v1/assignments', { + data: { expiresAt: new Date(Date.now() + 86_400_000), groupId, instrumentId, subjectId }, + headers: { Authorization: `Bearer ${adminToken}` } + }); + expect(response.status()).toBe(422); + expect(((await response.json()) as BulkAssignmentFailure).issues).toContainEqual({ + instrumentIds: [instrumentId], + kind: 'INSTRUMENT_UNAVAILABLE' + }); + } + expect(await api.findAssignments(subjectId)).toHaveLength(0); + }); + + // Assigned without a group, so the group's opt-in list cannot be what refuses it. + test('should refuse to assign a series with a file item remotely', async ({ + adminToken, + api, + apiRequestContext, + uniqueId + }) => { + const headers = { Authorization: `Bearer ${adminToken}` }; + const group = await api.createGroup(); + const subjectId = await api.createSubject(group.id); + const instruments = (await (await apiRequestContext.get('/api/v1/instruments/info', { headers })).json()) as { + internal?: { edition: number; name: string }; + kind: string; + }[]; + const items = ['FORM', 'FILE'].map((kind) => instruments.find((instrument) => instrument.kind === kind)!.internal); + const series = await apiRequestContext.post('/api/v1/instruments/series', { + data: { + confirmDuplicate: true, + details: { title: `File Series ${uniqueId}` }, + groupId: group.id, + items, + language: 'en' + }, + headers + }); + expect(series.status(), await series.text()).toBe(201); + const { instrumentId } = (await series.json()) as { instrumentId: string }; + + const response = await apiRequestContext.post('/api/v1/assignments', { + data: { expiresAt: new Date(Date.now() + 86_400_000), instrumentId, subjectId }, + headers + }); + expect(response.status()).toBe(422); + expect(((await response.json()) as BulkAssignmentFailure).issues).toContainEqual({ + instrumentIds: [instrumentId], + kind: 'INSTRUMENT_UNAVAILABLE' + }); + }); + test('should create a remote assignment and display the link', async ({ getPageModel, page, uniqueId }) => { await startSession(getPageModel, `Assign${uniqueId}`, `User${uniqueId}`, 'Female'); From d5472afda0a183bd6bb71152304165c55d002207 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Mon, 9 Nov 2026 19:51:44 -0500 Subject: [PATCH 05/10] fix(api): refuse re-adding an instrument repo the caller cannot update POST /v1/instrument-repos treats the URL of a registered repository as a re-sync, storing any access token sent with it and then syncing. It did that for every caller the route admits, ignoring the group scope of their grant, so a user granted create InstrumentRepo for one group could replace the token of a private repository only another group uses, and break every later sync of it. The controller now forwards the caller's ability, and create re-syncs a registered repository only for a caller who may update it. Anyone else gets a 409 before the token is read or written. The check is ability.can on the repository's groups rather than accessibleQuery, which throws a 500 for a caller holding no update rule at all. The lookup stays by URL alone, since the URL is unique and the 409 has to say so. Who loses what: holders of a group-scoped create InstrumentRepo grant can no longer re-add, and so retoken or re-sync, a repository outside their group. Administrators lose nothing, and re-adding with a new token is still how one replaces an expired token. The earlier sync fix (d2cf1e6aa) narrowed group-scoped update InstrumentRepo grants in the same way, since POST /v1/instrument-repos/:id/sync now answers 404 for a repository outside the grant's group. Closes #1728 Co-Authored-By: Claude Opus 5.5 --- .../instrument-repos.controller.test.ts | 6 +- .../instrument-repos.service.test.ts | 58 ++++++ .../instrument-repos.controller.ts | 4 +- .../instrument-repos.service.ts | 16 +- .../suites/06-instrument-repo-readd.suite.ts | 172 ++++++++++++++++++ 5 files changed, 246 insertions(+), 10 deletions(-) create mode 100644 apps/api/test/suites/06-instrument-repo-readd.suite.ts diff --git a/apps/api/src/instrument-repos/__tests__/instrument-repos.controller.test.ts b/apps/api/src/instrument-repos/__tests__/instrument-repos.controller.test.ts index 33ac7d889..3e5629da0 100644 --- a/apps/api/src/instrument-repos/__tests__/instrument-repos.controller.test.ts +++ b/apps/api/src/instrument-repos/__tests__/instrument-repos.controller.test.ts @@ -26,11 +26,11 @@ describe('InstrumentReposController', () => { service = moduleRef.get(InstrumentReposService); }); - it('should pass the submitted repository to the service when creating one', async () => { + it("should pass the submitted repository to the service with the current user's ability, which decides a re-add", async () => { const data = { accessToken: 'token', url: 'https://github.com/owner/repo' }; service.create.mockResolvedValueOnce(repo); - await expect(controller.create(data)).resolves.toBe(repo); - expect(service.create).toHaveBeenCalledWith(data); + await expect(controller.create(data, ability)).resolves.toBe(repo); + expect(service.create).toHaveBeenCalledWith(data, { ability }); }); it("should scope the deletion to the current user's ability", async () => { diff --git a/apps/api/src/instrument-repos/__tests__/instrument-repos.service.test.ts b/apps/api/src/instrument-repos/__tests__/instrument-repos.service.test.ts index 1d284beb6..e96d9cbd1 100644 --- a/apps/api/src/instrument-repos/__tests__/instrument-repos.service.test.ts +++ b/apps/api/src/instrument-repos/__tests__/instrument-repos.service.test.ts @@ -18,6 +18,8 @@ import { bundle } from '@opendatacapture/instrument-bundler'; import JSZip from 'jszip'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { createAppAbility } from '@/auth/ability.utils'; + import { InstrumentsService } from '../../instruments/instruments.service'; import { InstrumentReposService } from '../instrument-repos.service'; @@ -297,6 +299,62 @@ describe('InstrumentReposService', () => { expect(instrumentRepoModel.update).not.toHaveBeenCalled(); }); + + describe('re-adding a registered repo as a caller', () => { + const url = 'https://github.com/owner/repo'; + const confinedTo = (groupId: string) => ({ groupIds: { hasSome: [groupId] } }); + + it('should refuse one whose update grant is confined to other groups, leaving the token untouched', async () => { + const ability = createAppAbility([ + { action: 'create', conditions: confinedTo('group-a'), subject: 'InstrumentRepo' }, + { action: 'update', conditions: confinedTo('group-a'), subject: 'InstrumentRepo' } + ]); + instrumentRepoModel.findFirst.mockResolvedValueOnce({ groupIds: ['group-b'], id: 'existing' }); + const syncSpy = vi.spyOn(internal, 'syncRepo').mockResolvedValue({ ok: true, repo: { id: 'existing' } }); + + await expect(service.create({ accessToken: 'invalid', url }, { ability })).rejects.toBeInstanceOf( + ConflictException + ); + expect(instrumentRepoModel.update).not.toHaveBeenCalled(); + expect(syncSpy).not.toHaveBeenCalled(); + }); + + it('should refuse one holding only create as a conflict, rather than the 500 of a query it has no rule for', async () => { + const ability = createAppAbility([{ action: 'create', subject: 'InstrumentRepo' }]); + instrumentRepoModel.findFirst.mockResolvedValueOnce({ groupIds: [], id: 'existing' }); + vi.spyOn(internal, 'syncRepo').mockResolvedValue({ ok: true, repo: { id: 'existing' } }); + + await expect(service.create({ accessToken: 'invalid', url }, { ability })).rejects.toBeInstanceOf( + ConflictException + ); + expect(instrumentRepoModel.update).not.toHaveBeenCalled(); + }); + + it("should re-sync for one whose update grant covers one of the repo's groups", async () => { + const ability = createAppAbility([ + { action: 'update', conditions: confinedTo('group-b'), subject: 'InstrumentRepo' } + ]); + instrumentRepoModel.findFirst.mockResolvedValueOnce({ groupIds: ['group-b', 'group-c'], id: 'existing' }); + const syncSpy = vi.spyOn(internal, 'syncRepo').mockResolvedValue({ ok: true, repo: { id: 'existing' } }); + + await service.create({ url }, { ability }); + + expect(syncSpy).toHaveBeenCalledOnce(); + }); + + it('should let an administrator replace the token, which is how an expired one is refreshed', async () => { + const ability = createAppAbility([{ action: 'manage', subject: 'all' }]); + instrumentRepoModel.findFirst.mockResolvedValueOnce({ groupIds: [], id: 'existing' }); + const syncSpy = vi.spyOn(internal, 'syncRepo').mockResolvedValue({ ok: true, repo: { id: 'existing' } }); + + await service.create({ accessToken: 'new-token', url }, { ability }); + + const accessToken = instrumentRepoModel.update.mock.lastCall?.[0].data.accessToken; + expect(typeof accessToken === 'string' && internal.decrypt(accessToken)).toBe('new-token'); + expect(instrumentRepoModel.update.mock.lastCall?.[0].where).toStrictEqual({ id: 'existing' }); + expect(syncSpy).toHaveBeenCalledOnce(); + }); + }); }); describe('sync', () => { diff --git a/apps/api/src/instrument-repos/instrument-repos.controller.ts b/apps/api/src/instrument-repos/instrument-repos.controller.ts index 12827c580..05fac775f 100644 --- a/apps/api/src/instrument-repos/instrument-repos.controller.ts +++ b/apps/api/src/instrument-repos/instrument-repos.controller.ts @@ -18,8 +18,8 @@ export class InstrumentReposController { @ApiOperation({ summary: 'Create Instrument Repo' }) @Post() @RouteAccess({ action: 'create', subject: 'InstrumentRepo' }) - create(@Body() dto: $CreateInstrumentRepoData) { - return this.instrumentReposService.create(dto); + create(@Body() dto: $CreateInstrumentRepoData, @CurrentUser('ability') ability: AppAbility) { + return this.instrumentReposService.create(dto, { ability }); } @ApiOperation({ summary: 'Delete Instrument Repo' }) diff --git a/apps/api/src/instrument-repos/instrument-repos.service.ts b/apps/api/src/instrument-repos/instrument-repos.service.ts index 3636bd91b..5c65fd574 100644 --- a/apps/api/src/instrument-repos/instrument-repos.service.ts +++ b/apps/api/src/instrument-repos/instrument-repos.service.ts @@ -26,7 +26,7 @@ import type { import JSZip from 'jszip'; import { z } from 'zod/v4'; -import { accessibleQuery } from '@/auth/ability.utils'; +import { accessibleQuery, forcedAppSubject } from '@/auth/ability.utils'; import { decryptSecret, encryptSecret } from '@/core/secret-cipher'; import type { EntityOperationOptions } from '@/core/types'; import { InstrumentsService } from '@/instruments/instruments.service'; @@ -96,15 +96,21 @@ export class InstrumentReposService implements OnModuleInit { private readonly loggingService: LoggingService ) {} - async create({ accessToken, url }: $CreateInstrumentRepoData) { + async create({ accessToken, url }: $CreateInstrumentRepoData, { ability }: EntityOperationOptions = {}) { const { owner, repoName } = this.parseGitHubUrl(url); const normalizedUrl = this.normalizeUrl(url); - // Re-adding an already-registered repo is treated as a re-sync rather than an error. If a new - // access token was supplied (e.g. the previous one expired), persist it first so the sync — and - // all future syncs — use the updated credentials. + // Re-adding a registered repo re-syncs it after persisting any new token, which is how an expired + // token is replaced. Only a caller who may update the repo gets that, so the lookup is by URL alone + // (it is unique) and anyone else is refused before the token is touched. `ability.can`, because + // `accessibleQuery` throws a 500 for a caller holding no `update` rule at all. const existing = await this.instrumentRepoModel.findFirst({ where: { url: normalizedUrl } }); if (existing) { + if (ability && !ability.can('update', forcedAppSubject('InstrumentRepo', { groupIds: existing.groupIds }))) { + throw new ConflictException( + `Repository '${normalizedUrl}' is already registered, and you do not have permission to update it` + ); + } if (accessToken) { await this.instrumentRepoModel.update({ data: { accessToken: this.encrypt(accessToken) }, diff --git a/apps/api/test/suites/06-instrument-repo-readd.suite.ts b/apps/api/test/suites/06-instrument-repo-readd.suite.ts new file mode 100644 index 000000000..1c91b114b --- /dev/null +++ b/apps/api/test/suites/06-instrument-repo-readd.suite.ts @@ -0,0 +1,172 @@ +import { ConfigService, PRISMA_CLIENT_TOKEN } from '@douglasneuroinformatics/libnest'; +import type { Permissions } from '@opendatacapture/schemas/core'; +import type { Group } from '@opendatacapture/schemas/group'; +import type { User } from '@opendatacapture/schemas/user'; +import JSZip from 'jszip'; +import { beforeAll, expect, it, onTestFinished, vi } from 'vitest'; + +import type { RuntimePrismaClient } from '@/core/prisma'; +import { decryptSecret, encryptSecret } from '@/core/secret-cipher'; + +import { defineSuite } from '../helpers'; + +const PASSWORD = 'DataCapture2025_Test'; + +type AuthHeaders = { authorization: string }; + +// Registering a repository through the API downloads it from GitHub, so each one these tests re-add is +// written directly, and every download a re-add would start is answered by a stub. +export default defineSuite('re-adding a registered instrument repository', function () { + let adminHeaders: AuthHeaders; + let ownGroup: Group; + let otherGroup: Group; + + const prisma = () => this.app.get(PRISMA_CLIENT_TOKEN); + const secretKey = () => this.app.get(ConfigService).getOrThrow('SECRET_KEY'); + + const login = async (username: string): Promise => { + const response = await this.app.inject({ + method: 'POST', + payload: { password: PASSWORD, username }, + url: '/v1/auth/login' + }); + expect(response.statusCode).toBe(200); + return { authorization: `Bearer ${response.json<{ accessToken: string }>().accessToken}` }; + }; + + const createGroup = async (name: string): Promise => { + const response = await this.app.inject({ + headers: adminHeaders, + method: 'POST', + payload: { name, type: 'RESEARCH' }, + url: '/v1/groups' + }); + expect(response.statusCode).toBe(201); + return response.json(); + }; + + /** A standard user in `ownGroup` holding exactly `permissions`, signed in. */ + const signInGrantee = async (username: string, permissions: Permissions): Promise => { + const created = await this.app.inject({ + headers: adminHeaders, + method: 'POST', + payload: { + basePermissionLevel: 'STANDARD', + firstName: 'Repo', + groupIds: [ownGroup.id], + lastName: 'Grantee', + password: PASSWORD, + username + }, + url: '/v1/users' + }); + expect(created.statusCode).toBe(201); + const granted = await this.app.inject({ + headers: adminHeaders, + method: 'PUT', + payload: { kind: 'custom', permissions }, + url: `/v1/users/${created.json().id}/permissions` + }); + expect(granted.statusCode).toBe(200); + return login(username); + }; + + const registerRepo = (repoName: string, groupIds: string[]) => { + return prisma().instrumentRepo.create({ + data: { + accessToken: encryptSecret('stored-token', secretKey()), + groupIds, + instrumentIds: [], + name: repoName, + owner: 'odc-test', + repoName, + url: `https://github.com/odc-test/${repoName}` + } + }); + }; + + const storedToken = async (id: string) => { + return (await prisma().instrumentRepo.findUniqueOrThrow({ where: { id } })).accessToken; + }; + + /** Answers every GitHub download with an empty repository, and passes any other request on. */ + const stubGitHub = async () => { + const archive = await new JSZip().generateAsync({ type: 'arraybuffer' }); + const realFetch = globalThis.fetch; + const fetchSpy = vi.spyOn(globalThis, 'fetch').mockImplementation((input, init) => { + return typeof input === 'string' && input.startsWith('https://api.github.com/') + ? Promise.resolve(new Response(archive)) + : realFetch(input, init); + }); + onTestFinished(() => fetchSpy.mockRestore()); + return fetchSpy; + }; + + const reAdd = (headers: AuthHeaders, url: string) => { + return this.app.inject({ + headers, + method: 'POST', + payload: { accessToken: 'new-token', url }, + url: '/v1/instrument-repos' + }); + }; + + beforeAll(async () => { + adminHeaders = await login('admin'); + ownGroup = await createGroup('Repo Grantee Group'); + otherGroup = await createGroup('Repo Owner Group'); + }); + + it("should refuse a user whose grants are confined to their own group with a 409, keeping another group's repository token", async () => { + const headers = await signInGrantee('repo.scoped.updater', [ + { action: 'create', groupId: ownGroup.id, subject: 'InstrumentRepo' }, + { action: 'update', groupId: ownGroup.id, subject: 'InstrumentRepo' } + ]); + const repo = await registerRepo('scoped-updater-target', [otherGroup.id]); + const fetchSpy = await stubGitHub(); + + const response = await reAdd(headers, repo.url); + + expect(response.statusCode).toBe(409); + expect(fetchSpy).not.toHaveBeenCalled(); + expect(await storedToken(repo.id)).toBe(repo.accessToken); + }); + + it('should refuse a user holding only a scoped create grant with a 409, rather than a 500', async () => { + const headers = await signInGrantee('repo.scoped.creator', [ + { action: 'create', groupId: ownGroup.id, subject: 'InstrumentRepo' } + ]); + const repo = await registerRepo('scoped-creator-target', [otherGroup.id]); + await stubGitHub(); + + const response = await reAdd(headers, repo.url); + + expect(response.statusCode).toBe(409); + expect(await storedToken(repo.id)).toBe(repo.accessToken); + }); + + it("should re-sync for a user whose update grant covers the repository's group", async () => { + const headers = await signInGrantee('repo.own.updater', [ + { action: 'create', groupId: ownGroup.id, subject: 'InstrumentRepo' }, + { action: 'update', groupId: ownGroup.id, subject: 'InstrumentRepo' } + ]); + const repo = await registerRepo('own-group-target', [ownGroup.id]); + await stubGitHub(); + + const response = await reAdd(headers, repo.url); + + expect(response.statusCode).toBe(201); + }); + + it('should let an administrator replace the token and re-sync, which is how an expired one is refreshed', async () => { + const repo = await registerRepo('admin-target', [otherGroup.id]); + const fetchSpy = await stubGitHub(); + + const response = await reAdd(adminHeaders, repo.url); + + expect(response.statusCode).toBe(201); + expect(fetchSpy).toHaveBeenCalled(); + const token = await storedToken(repo.id); + expect(token && decryptSecret(token, secretKey())).toBe('new-token'); + }); +}); From 7a121c0af7ea0d28783a7daf2aa0de62b09e3f92 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Sun, 8 Nov 2026 20:55:05 -0500 Subject: [PATCH 06/10] perf(api): read instrument bundles only for uncached instances `InstrumentsService.find` loaded every matching instrument document, bundle included, and then served most of them from the evaluated-instance cache, ignoring the bundle it had just read. The cost of each `/v1/instruments/info`, `/v1/instruments/list` and dashboard load grew with the total size of every bundle on the instance. `find` now selects only `id`, then reads `bundle` for just the ids missing from `virtualizationService.context.instruments`. A row deleted between the two queries has no instance and is dropped, rather than returned as `undefined`. Refs #1537 Co-Authored-By: Claude Opus 5.5 --- .../__tests__/instruments.service.test.ts | 53 +++++++++++++++++-- .../src/instruments/instruments.service.ts | 16 +++++- 2 files changed, 62 insertions(+), 7 deletions(-) diff --git a/apps/api/src/instruments/__tests__/instruments.service.test.ts b/apps/api/src/instruments/__tests__/instruments.service.test.ts index 21ad0aa81..2fb148815 100644 --- a/apps/api/src/instruments/__tests__/instruments.service.test.ts +++ b/apps/api/src/instruments/__tests__/instruments.service.test.ts @@ -767,6 +767,7 @@ describe('InstrumentsService', () => { await instrumentsService.find({}, {}, ['group-1']); expect(instrumentModel.findMany).toHaveBeenCalledWith({ + select: { id: true }, where: { AND: [ { records: undefined }, @@ -1088,6 +1089,12 @@ describe('InstrumentsService', () => { where: { AND: [accessibleQuery(ability, 'read', 'Instrument'), ownedSeriesFilter(['group-1'])] } }); }); + + it('should evaluate a bundle once however often it is requested, serving repeats from the cache', async () => { + await instrumentsService.findBundleById('form'); + await instrumentsService.findBundleById('form'); + expect(virtualizationService.eval).toHaveBeenCalledOnce(); + }); }); describe('updateSeriesArchive', () => { @@ -1282,16 +1289,52 @@ describe('InstrumentsService', () => { }); describe('find (evaluation)', () => { + const storedRows = [ + { bundle: 'FORM_BUNDLE', id: 'hash:FORM_A-1' }, + { bundle: 'SERIES_BUNDLE', id: 'series-1' } + ]; + beforeEach(() => { - instrumentModel.findMany.mockResolvedValue([ - { bundle: 'FORM_BUNDLE', id: 'hash:FORM_A-1' }, - { bundle: 'SERIES_BUNDLE', id: 'series-1' } - ]); + instrumentModel.findMany.mockImplementation(({ where }: any) => { + return Promise.resolve(where.id ? storedRows.filter(({ id }) => where.id.in.includes(id)) : storedRows); + }); virtualizationService.eval.mockImplementation((code) => { return okAsync(code === 'FORM_BUNDLE' ? formInstance('FORM_A', 1) : existingSeries); }); }); + it('should read the bundle only of an instrument it has not evaluated yet', async () => { + instanceCache.set('series-1', { ...existingSeries, id: 'series-1' }); + + const result = await instrumentsService.find(); + + expect(instrumentModel.findMany).toHaveBeenLastCalledWith({ + select: { bundle: true, id: true }, + where: { id: { in: ['hash:FORM_A-1'] } } + }); + expect(result.map(({ id }) => id)).toEqual(['hash:FORM_A-1', 'series-1']); + }); + + it('should not read any bundle when every instrument is already evaluated', async () => { + await instrumentsService.find(); + instrumentModel.findMany.mockClear(); + + await instrumentsService.find(); + + expect(instrumentModel.findMany).toHaveBeenCalledOnce(); + expect(instrumentModel.findMany).toHaveBeenCalledWith(expect.objectContaining({ select: { id: true } })); + }); + + it('should drop an instrument deleted before its bundle was read, rather than return an undefined instance', async () => { + instrumentModel.findMany + .mockResolvedValueOnce(storedRows.map(({ id }) => ({ id }))) + .mockResolvedValueOnce([storedRows[0]!]); + + const result = await instrumentsService.find(); + + expect(result.map(({ id }) => id)).toEqual(['hash:FORM_A-1']); + }); + it('should return only instruments of the requested kind', async () => { const result = await instrumentsService.find({ kind: 'FORM' }); expect(result.map(({ id }) => id)).toEqual(['hash:FORM_A-1']); @@ -1310,7 +1353,7 @@ describe('InstrumentsService', () => { it('should scope owned series to the named series group in preference to the caller groups', async () => { await instrumentsService.find({ seriesGroupId: 'group-2' }, {}, ['group-1']); - expect(instrumentModel.findMany.mock.lastCall?.[0]).toMatchObject({ + expect(instrumentModel.findMany.mock.calls[0]?.[0]).toMatchObject({ where: { AND: [ {}, diff --git a/apps/api/src/instruments/instruments.service.ts b/apps/api/src/instruments/instruments.service.ts index c11625d98..9774d6b3e 100644 --- a/apps/api/src/instruments/instruments.service.ts +++ b/apps/api/src/instruments/instruments.service.ts @@ -310,7 +310,8 @@ export class InstrumentsService { requestedSeriesGroup = { seriesGroupId: { in: groupIds } }; } - const instruments = await this.instrumentModel.findMany({ + const rows = await this.instrumentModel.findMany({ + select: { id: true }, where: { AND: [ subjectInstrumentIds ? { id: { in: subjectInstrumentIds } } : {}, @@ -321,7 +322,18 @@ export class InstrumentsService { ] } }); - const instances = await this.instantiate(instruments); + const cache = this.virtualizationService.context.instruments; + const uncachedIds = rows.map(({ id }) => id).filter((id) => !cache.has(id)); + if (uncachedIds.length > 0) { + // Scoped by the ids, which the query above already filtered for this caller. + const uncached = await this.instrumentModel.findMany({ + select: { bundle: true, id: true }, + where: { id: { in: uncachedIds } } + }); + await this.instantiate(uncached); + } + // A row deleted between the two queries has no bundle to evaluate, so it has no instance either. + const instances = rows.flatMap(({ id }) => cache.get(id) ?? []); if (!query.kind) { return instances as WithID>[]; } From 4005c6b8b1f2e18376d6f703227d0e8e6a52798f Mon Sep 17 00:00:00 2001 From: joshunrau Date: Mon, 9 Nov 2026 19:51:46 -0500 Subject: [PATCH 07/10] fix(api): sync a re-added instrument repo with its new token Re-adding a registered repository with a new access token stored the token, but then synced the record read before the update, so the sync the re-add started still downloaded with the old token and failed with a 502 when that token had expired. The sync now uses the updated record. Co-Authored-By: Claude Opus 5.5 --- .../__tests__/instrument-repos.service.test.ts | 11 +++++++++++ .../instrument-repos/instrument-repos.service.ts | 14 +++++++------- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/apps/api/src/instrument-repos/__tests__/instrument-repos.service.test.ts b/apps/api/src/instrument-repos/__tests__/instrument-repos.service.test.ts index e96d9cbd1..b0225f2e9 100644 --- a/apps/api/src/instrument-repos/__tests__/instrument-repos.service.test.ts +++ b/apps/api/src/instrument-repos/__tests__/instrument-repos.service.test.ts @@ -291,6 +291,17 @@ describe('InstrumentReposService', () => { ); }); + it('should re-sync with the updated token, so the sync re-adding starts does not download with the expired one', async () => { + const updated = { accessToken: 'new-ciphertext', id: 'existing' }; + instrumentRepoModel.findFirst.mockResolvedValueOnce({ accessToken: 'old-ciphertext', id: 'existing' }); + instrumentRepoModel.update.mockResolvedValueOnce(updated); + const syncSpy = vi.spyOn(internal, 'syncRepo').mockResolvedValue({ ok: true, repo: updated }); + + await service.create({ accessToken: 'new-token', url: 'https://github.com/owner/repo' }); + + expect(syncSpy).toHaveBeenCalledWith(updated); + }); + it('does not touch the stored token when re-adding without one', async () => { instrumentRepoModel.findFirst.mockResolvedValueOnce({ id: 'existing' }); vi.spyOn(internal, 'syncRepo').mockResolvedValue({ ok: true, repo: { id: 'existing' } }); diff --git a/apps/api/src/instrument-repos/instrument-repos.service.ts b/apps/api/src/instrument-repos/instrument-repos.service.ts index 5c65fd574..c8364c6ff 100644 --- a/apps/api/src/instrument-repos/instrument-repos.service.ts +++ b/apps/api/src/instrument-repos/instrument-repos.service.ts @@ -111,13 +111,13 @@ export class InstrumentReposService implements OnModuleInit { `Repository '${normalizedUrl}' is already registered, and you do not have permission to update it` ); } - if (accessToken) { - await this.instrumentRepoModel.update({ - data: { accessToken: this.encrypt(accessToken) }, - where: { id: existing.id } - }); - } - const result = await this.syncRepo(existing); + const repo = accessToken + ? await this.instrumentRepoModel.update({ + data: { accessToken: this.encrypt(accessToken) }, + where: { id: existing.id } + }) + : existing; + const result = await this.syncRepo(repo); if (!result.ok) { throw result.error; } From 5f30825983ac93b5438f292b85c0a83ce8ee4966 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Mon, 9 Nov 2026 19:58:12 -0500 Subject: [PATCH 08/10] test(gateway): serve test requests on an address no other process can share The server and router tests listened on port 0 with no host, which binds the wildcard address, and then fetched localhost or 127.0.0.1. A second process can still bind 127.0.0.1 on that same port, and the more specific bind takes the IPv4 connection. During a full unit run the API project's mongodb-memory-server can: it probes for a free port, then mongod binds it on 127.0.0.1, and mongod answers HTTP with a 200. A request was then answered without reaching the server under test. Each test server now binds 127.0.0.1 and is fetched there. A specific bind cannot be shared, so the request always reaches it. Closes #1897 Co-Authored-By: Claude Opus 5.5 --- apps/gateway/src/routers/__tests__/api.router.test.ts | 2 +- apps/gateway/src/routers/__tests__/cap.router.test.ts | 2 +- apps/gateway/src/routers/__tests__/root.router.test.ts | 2 +- apps/gateway/src/server/__tests__/server.base.test.ts | 10 +++++++--- .../src/server/__tests__/server.development.test.ts | 4 ++-- .../src/server/__tests__/server.production.test.ts | 4 ++-- 6 files changed, 14 insertions(+), 10 deletions(-) diff --git a/apps/gateway/src/routers/__tests__/api.router.test.ts b/apps/gateway/src/routers/__tests__/api.router.test.ts index e5112964e..00788060f 100644 --- a/apps/gateway/src/routers/__tests__/api.router.test.ts +++ b/apps/gateway/src/routers/__tests__/api.router.test.ts @@ -90,7 +90,7 @@ describe('apiRouter', () => { app.use('/api', apiRouter); app.use(errorHandlerMiddleware); server = await new Promise((resolve) => { - const listening = app.listen(0, () => resolve(listening)); + const listening = app.listen(0, '127.0.0.1', () => resolve(listening)); }); const address = server.address(); if (typeof address !== 'object' || address === null) { diff --git a/apps/gateway/src/routers/__tests__/cap.router.test.ts b/apps/gateway/src/routers/__tests__/cap.router.test.ts index 9a5a1c3b4..91ae46e78 100644 --- a/apps/gateway/src/routers/__tests__/cap.router.test.ts +++ b/apps/gateway/src/routers/__tests__/cap.router.test.ts @@ -50,7 +50,7 @@ describe('capRouter', () => { app.use('/api/auth', capRouter); app.use(errorHandlerMiddleware); server = await new Promise((resolve) => { - const listening = app.listen(0, () => resolve(listening)); + const listening = app.listen(0, '127.0.0.1', () => resolve(listening)); }); const address = server.address(); if (typeof address !== 'object' || address === null) { diff --git a/apps/gateway/src/routers/__tests__/root.router.test.ts b/apps/gateway/src/routers/__tests__/root.router.test.ts index 2894297f1..84af7aa16 100644 --- a/apps/gateway/src/routers/__tests__/root.router.test.ts +++ b/apps/gateway/src/routers/__tests__/root.router.test.ts @@ -54,7 +54,7 @@ describe('rootRouter', () => { }); app.use('/', rootRouter); server = await new Promise((resolve) => { - const listening = app.listen(0, () => resolve(listening)); + const listening = app.listen(0, '127.0.0.1', () => resolve(listening)); }); const address = server.address(); if (typeof address !== 'object' || address === null) { diff --git a/apps/gateway/src/server/__tests__/server.base.test.ts b/apps/gateway/src/server/__tests__/server.base.test.ts index 49da65b13..1006e47ac 100644 --- a/apps/gateway/src/server/__tests__/server.base.test.ts +++ b/apps/gateway/src/server/__tests__/server.base.test.ts @@ -68,6 +68,10 @@ class TestServer extends BaseServer { super(); } + listenOnRandomPort() { + return this.app.listen(0, '127.0.0.1'); + } + protected loadRender() { return this.renderLoader(); } @@ -88,15 +92,15 @@ class StacktraceFixingServer extends TestServer { const openServers: Server[] = []; -async function request(server: BaseServer, path: string, init?: RequestInit) { - const httpServer = server.listen(); +async function request(server: TestServer, path: string, init?: RequestInit) { + const httpServer = server.listenOnRandomPort(); openServers.push(httpServer); await once(httpServer, 'listening'); const address = httpServer.address(); if (typeof address !== 'object' || address === null) { throw new Error(`Expected the server to listen on a port, got: ${address}`); } - return fetch(`http://localhost:${address.port}${path}`, init); + return fetch(`http://127.0.0.1:${address.port}${path}`, init); } function readLogEntries(): LogEntry[] { diff --git a/apps/gateway/src/server/__tests__/server.development.test.ts b/apps/gateway/src/server/__tests__/server.development.test.ts index 305c8b440..dd8d60eac 100644 --- a/apps/gateway/src/server/__tests__/server.development.test.ts +++ b/apps/gateway/src/server/__tests__/server.development.test.ts @@ -46,7 +46,7 @@ class TestDevelopmentServer extends DevelopmentServer { } listenOnRandomPort() { - return this.app.listen(0); + return this.app.listen(0, '127.0.0.1'); } render() { @@ -97,7 +97,7 @@ describe('DevelopmentServer', () => { if (typeof address !== 'object' || address === null) { throw new Error(`Expected the server to listen on a port, got: ${address}`); } - const response = await fetch(`http://localhost:${address.port}/src/entry-client.tsx`); + const response = await fetch(`http://127.0.0.1:${address.port}/src/entry-client.tsx`); expect(await response.text()).toBe('served by vite'); }); diff --git a/apps/gateway/src/server/__tests__/server.production.test.ts b/apps/gateway/src/server/__tests__/server.production.test.ts index 6e1dac7e2..92e975985 100644 --- a/apps/gateway/src/server/__tests__/server.production.test.ts +++ b/apps/gateway/src/server/__tests__/server.production.test.ts @@ -40,7 +40,7 @@ vi.mock('../server.base', async () => { class TestProductionServer extends ProductionServer { listenOnRandomPort() { - return this.app.listen(0); + return this.app.listen(0, '127.0.0.1'); } render() { @@ -62,7 +62,7 @@ async function request(path: string, init?: RequestInit) { if (typeof address !== 'object' || address === null) { throw new Error(`Expected the server to listen on a port, got: ${address}`); } - return fetch(`http://localhost:${address.port}${path}`, init); + return fetch(`http://127.0.0.1:${address.port}${path}`, init); } describe('ProductionServer', () => { From 031f401db20a2aafd652bddd81532a2de3912b0e Mon Sep 17 00:00:00 2001 From: joshunrau Date: Mon, 9 Nov 2026 20:20:25 -0500 Subject: [PATCH 09/10] refactor(api): read only uncached bundles when checking remote assignability `findUnassignableIds` and `collectsFiles` read the bundle of every requested instrument and series item, even one already evaluated, which `find` has stopped doing. All three now resolve ids to instances through one private `findInstancesByIds`, which reads a bundle only for an instrument missing from the cache. `findUnassignableIds` reads `archivedAt` and `id` first and evaluates only the non-archived ones. Bulk assignment now asks only about instruments the group has opted into. Any other instrument is already refused, and evaluating its bundle could only fail, which would have turned the 422 into a 500. Co-Authored-By: Claude Opus 5.5 --- .../__tests__/assignments.service.test.ts | 21 ++++--- .../src/assignments/assignments.service.ts | 4 +- .../__tests__/instruments.service.test.ts | 9 ++- .../src/instruments/instruments.service.ts | 59 +++++++++---------- 4 files changed, 51 insertions(+), 42 deletions(-) diff --git a/apps/api/src/assignments/__tests__/assignments.service.test.ts b/apps/api/src/assignments/__tests__/assignments.service.test.ts index 526c07db8..ea1212535 100644 --- a/apps/api/src/assignments/__tests__/assignments.service.test.ts +++ b/apps/api/src/assignments/__tests__/assignments.service.test.ts @@ -2,7 +2,12 @@ import { ConfigService, getModelToken, LoggingService } from '@douglasneuroinfor 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 { + ForbiddenException, + InternalServerErrorException, + NotFoundException, + UnprocessableEntityException +} from '@nestjs/common'; import { Test } from '@nestjs/testing'; import type { BulkAssignmentFailure } from '@opendatacapture/schemas/assignment'; import type { Permissions } from '@opendatacapture/schemas/core'; @@ -181,8 +186,13 @@ describe('AssignmentsService', () => { expect(failure.issues).toContainEqual({ instrumentIds: ['instrument-1'], kind: 'INSTRUMENT_UNAVAILABLE' }); }); - it('should ask which of every requested instrument cannot be assigned remotely, whatever the group opted into', async () => { - await failureOf( + it('should refuse an instrument the group has not opted into without evaluating it, so a broken bundle cannot turn the refusal into a 500', async () => { + instrumentsService.findUnassignableIds.mockImplementation((ids: string[]) => { + return ids.includes('instrument-other') + ? Promise.reject(new InternalServerErrorException('Failed to evaluate instrument')) + : Promise.resolve([]); + }); + const failure = await failureOf( assignmentsService.bulkPreflight( request({ timepoints: [ @@ -193,10 +203,7 @@ describe('AssignmentsService', () => { permissiveUser() ) ); - expect(instrumentsService.findUnassignableIds).toHaveBeenCalledExactlyOnceWith([ - 'instrument-1', - 'instrument-other' - ]); + expect(failure.issues).toContainEqual({ instrumentIds: ['instrument-other'], kind: 'INSTRUMENT_UNAVAILABLE' }); }); it('should restrict subjects to the selected group and the caller ability', async () => { diff --git a/apps/api/src/assignments/assignments.service.ts b/apps/api/src/assignments/assignments.service.ts index c5379504a..e5fe2cefa 100644 --- a/apps/api/src/assignments/assignments.service.ts +++ b/apps/api/src/assignments/assignments.service.ts @@ -289,7 +289,9 @@ export class AssignmentsService { // still administer in person. const accessibleInstrumentIds = new Set(group.accessibleInstrumentIds); const instrumentIds = timepoints.map(({ instrumentId }) => instrumentId); - const unassignableInstrumentIds = new Set(await this.instrumentsService.findUnassignableIds(instrumentIds)); + const unassignableInstrumentIds = new Set( + await this.instrumentsService.findUnassignableIds(instrumentIds.filter((id) => accessibleInstrumentIds.has(id))) + ); const unavailableInstrumentIds = [ ...new Set(instrumentIds.filter((id) => !accessibleInstrumentIds.has(id) || unassignableInstrumentIds.has(id))) ]; diff --git a/apps/api/src/instruments/__tests__/instruments.service.test.ts b/apps/api/src/instruments/__tests__/instruments.service.test.ts index 2fb148815..03cb5f230 100644 --- a/apps/api/src/instruments/__tests__/instruments.service.test.ts +++ b/apps/api/src/instruments/__tests__/instruments.service.test.ts @@ -1176,7 +1176,6 @@ describe('InstrumentsService', () => { beforeEach(() => { instrumentModel.findMany.mockResolvedValue([]); instrumentRecordModel.findMany.mockResolvedValue([]); - vi.spyOn(instrumentsService as any, 'instantiate').mockResolvedValue([]); }); it('should not query records when no subject is given, so the unfiltered listing costs one query', async () => { @@ -1478,10 +1477,14 @@ describe('InstrumentsService', () => { ]); }); - it('should read only the requested instruments, and only what it needs of them', async () => { + it('should read no bundle of an instrument it has already evaluated, as find does not', async () => { await instrumentsService.findUnassignableIds(['hash:FORM_A-1']); + instrumentModel.findMany.mockClear(); + + await instrumentsService.findUnassignableIds(['hash:FORM_A-1']); + expect(instrumentModel.findMany).toHaveBeenCalledExactlyOnceWith({ - select: { archivedAt: true, bundle: true, id: true }, + select: { archivedAt: true, id: true }, where: { id: { in: ['hash:FORM_A-1'] } } }); }); diff --git a/apps/api/src/instruments/instruments.service.ts b/apps/api/src/instruments/instruments.service.ts index 9774d6b3e..bcc8a42ff 100644 --- a/apps/api/src/instruments/instruments.service.ts +++ b/apps/api/src/instruments/instruments.service.ts @@ -322,18 +322,8 @@ export class InstrumentsService { ] } }); - const cache = this.virtualizationService.context.instruments; - const uncachedIds = rows.map(({ id }) => id).filter((id) => !cache.has(id)); - if (uncachedIds.length > 0) { - // Scoped by the ids, which the query above already filtered for this caller. - const uncached = await this.instrumentModel.findMany({ - select: { bundle: true, id: true }, - where: { id: { in: uncachedIds } } - }); - await this.instantiate(uncached); - } - // A row deleted between the two queries has no bundle to evaluate, so it has no instance either. - const instances = rows.flatMap(({ id }) => cache.get(id) ?? []); + // Scoped by the ids, which the query above already filtered for this caller. + const instances = await this.findInstancesByIds(rows.map(({ id }) => id)); if (!query.kind) { return instances as WithID>[]; } @@ -437,13 +427,15 @@ export class InstrumentsService { */ async findUnassignableIds(ids: string[]): Promise { const instruments = await this.instrumentModel.findMany({ - select: { archivedAt: true, bundle: true, id: true }, + select: { archivedAt: true, id: true }, where: { id: { in: ids } } }); - const isUnassignable = await Promise.all( - instruments.map(async (instrument) => Boolean(instrument.archivedAt) || (await this.collectsFiles(instrument))) + const archivedIds = instruments.filter(({ archivedAt }) => archivedAt).map(({ id }) => id); + const instances = await this.findInstancesByIds( + instruments.filter(({ archivedAt }) => !archivedAt).map(({ id }) => id) ); - return instruments.filter((_, index) => isUnassignable[index]).map(({ id }) => id); + const collectingFiles = await Promise.all(instances.map((instance) => this.collectsFiles(instance))); + return [...archivedIds, ...instances.filter((_, index) => collectingFiles[index]).map(({ id }) => id)]; } generateInstrumentId(instrument: AnyInstrument, seriesGroupId?: string) { @@ -576,19 +568,14 @@ export class InstrumentsService { * A series bundle does not say what kind its items are, so each item is evaluated. An item that * no longer exists is left to `findBundleById`, which refuses it when the assignment is sent. */ - private async collectsFiles(instrument: Pick): Promise { - const instance = await this.getInstrumentInstance(instrument); + private async collectsFiles(instance: AnyInstrument): Promise { if (!isSeriesInstrument(instance)) { return isFileInstrument(instance); } const itemIds = getSeriesInstrumentItems(instance.content).map((internal) => { return this.generateScalarInstrumentId({ internal }); }); - const items = await this.instrumentModel.findMany({ - select: { bundle: true, id: true }, - where: { id: { in: itemIds } } - }); - return (await this.instantiate(items)).some(isFileInstrument); + return (await this.findInstancesByIds(itemIds)).some(isFileInstrument); } /** @@ -684,6 +671,24 @@ export class InstrumentsService { return Array.from(results.values()); } + /** + * Reads the bundle only of an instrument not yet evaluated. Unscoped: callers pass ids they have + * already scoped or that the caller named. + */ + private async findInstancesByIds(ids: string[]) { + const cache = this.virtualizationService.context.instruments; + const uncachedIds = ids.filter((id) => !cache.has(id)); + if (uncachedIds.length > 0) { + const uncached = await this.instrumentModel.findMany({ + select: { bundle: true, id: true }, + where: { id: { in: uncachedIds } } + }); + await Promise.all(uncached.map((instrument) => this.getInstrumentInstance(instrument))); + } + // An id with no stored row, such as one deleted since it was read, has no instance either. + return ids.flatMap((id) => cache.get(id) ?? []); + } + /** * The ids of the instruments the subject has at least one record for. * @@ -769,14 +774,6 @@ export class InstrumentsService { return JSON.stringify(items.map(({ edition, name }) => ({ edition, name }))); } - private async instantiate(instruments: Pick[]) { - return Promise.all( - instruments.map((instrument) => { - return this.getInstrumentInstance(instrument); - }) - ); - } - private instrumentExistsConflict(id: string): ConflictException { return new ConflictException(`Instrument with ID '${id}' already exists!`); } From 851e740007542b9d271383b11b634b6db423f749 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Mon, 9 Nov 2026 20:20:25 -0500 Subject: [PATCH 10/10] test(api): check that a re-added repository syncs with its new token The integration tests for re-adding a repository checked that a token was stored and that something was downloaded, which both held while the sync still used the old token. They now check the download's `Authorization` header, for the administrator and for a user whose update grant covers the repository's group. Co-Authored-By: Claude Opus 5.5 --- .../test/suites/06-instrument-repo-readd.suite.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/apps/api/test/suites/06-instrument-repo-readd.suite.ts b/apps/api/test/suites/06-instrument-repo-readd.suite.ts index 1c91b114b..408d21932 100644 --- a/apps/api/test/suites/06-instrument-repo-readd.suite.ts +++ b/apps/api/test/suites/06-instrument-repo-readd.suite.ts @@ -102,6 +102,13 @@ export default defineSuite('re-adding a registered instrument repository', funct return fetchSpy; }; + const expectSyncedWithNewToken = async (repoId: string, fetchSpy: Awaited>) => { + expect(fetchSpy).toHaveBeenCalledOnce(); + expect(fetchSpy.mock.lastCall?.[1]?.headers).toMatchObject({ Authorization: 'Bearer new-token' }); + const token = await storedToken(repoId); + expect(token && decryptSecret(token, secretKey())).toBe('new-token'); + }; + const reAdd = (headers: AuthHeaders, url: string) => { return this.app.inject({ headers, @@ -151,11 +158,12 @@ export default defineSuite('re-adding a registered instrument repository', funct { action: 'update', groupId: ownGroup.id, subject: 'InstrumentRepo' } ]); const repo = await registerRepo('own-group-target', [ownGroup.id]); - await stubGitHub(); + const fetchSpy = await stubGitHub(); const response = await reAdd(headers, repo.url); expect(response.statusCode).toBe(201); + await expectSyncedWithNewToken(repo.id, fetchSpy); }); it('should let an administrator replace the token and re-sync, which is how an expired one is refreshed', async () => { @@ -165,8 +173,6 @@ export default defineSuite('re-adding a registered instrument repository', funct const response = await reAdd(adminHeaders, repo.url); expect(response.statusCode).toBe(201); - expect(fetchSpy).toHaveBeenCalled(); - const token = await storedToken(repo.id); - expect(token && decryptSecret(token, secretKey())).toBe('new-token'); + await expectSyncedWithNewToken(repo.id, fetchSpy); }); });