fix: scope record edits and repo re-adds by grant, and refuse remote file assignments

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';
@@ -13,6 +18,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 +88,10 @@ describe('AssignmentsService', () => {
   let assignmentsService: AssignmentsService;
   let assignmentModel: MockedInstance<Model<'Assignment'>>;
   let groupModel: MockedInstance<Model<'Group'>>;
-  let instrumentModel: MockedInstance<Model<'Instrument'>>;
   let subjectModel: MockedInstance<Model<'Subject'>>;
   let auditLogger: MockedInstance<AuditLogger>;
   let gatewayService: MockedInstance<GatewayService>;
+  let instrumentsService: MockedInstance<InstrumentsService>;
   let loggingService: MockedInstance<LoggingService>;
 
   /** Compiles a fresh service against `config`, since the participant link base is fixed at construction. */
@@ -95,7 +101,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 +112,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 +181,31 @@ 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 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: [
+              { expiresAt: futureDate(), instrumentId: 'instrument-1' },
+              { expiresAt: futureDate(), instrumentId: 'instrument-other' }
+            ]
+          }),
+          permissiveUser()
+        )
+      );
+      expect(failure.issues).toContainEqual({ instrumentIds: ['instrument-other'], kind: 'INSTRUMENT_UNAVAILABLE' });
+    });
+
     it('should restrict subjects to the selected group and the caller ability', async () => {
       await assignmentsService.bulkPreflight(request(), permissiveUser());
       expect(subjectModel.findMany.mock.lastCall?.[0]).toMatchObject({
@@ -251,6 +277,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 +328,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 +437,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);

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 {}

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<string[]> {
-    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,15 @@ 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.filter((id) => accessibleInstrumentIds.has(id)))
+    );
     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' });

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,15 +1221,17 @@ 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();
     });
 
     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,30 +1291,55 @@ 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({
         data: { computedMeasures: { score: 2 } }
       });
     });
 
-    // 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();
 
       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', () => {