diff --git a/apps/api/src/assignments/__tests__/assignments.service.test.ts b/apps/api/src/assignments/__tests__/assignments.service.test.ts index 8bd4c1f27..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'; @@ -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>; 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 +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); 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..e5fe2cefa 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,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' }); 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/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..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 @@ -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'; @@ -289,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' } }); @@ -297,6 +310,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..c8364c6ff 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,22 +96,28 @@ 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 (accessToken) { - await this.instrumentRepoModel.update({ - data: { accessToken: this.encrypt(accessToken) }, - where: { id: existing.id } - }); + 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` + ); } - 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; } diff --git a/apps/api/src/instruments/__tests__/instruments.service.test.ts b/apps/api/src/instruments/__tests__/instruments.service.test.ts index 28ae548db..03cb5f230 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', () => { @@ -1169,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 () => { @@ -1282,16 +1288,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 +1352,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: [ {}, @@ -1373,6 +1415,81 @@ 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 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, 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..bcc8a42ff 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, @@ -305,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 } } : {}, @@ -316,7 +322,8 @@ export class InstrumentsService { ] } }); - const instances = await this.instantiate(instruments); + // 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>[]; } @@ -411,6 +418,26 @@ 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, id: true }, + where: { id: { in: ids } } + }); + const archivedIds = instruments.filter(({ archivedAt }) => archivedAt).map(({ id }) => id); + const instances = await this.findInstancesByIds( + instruments.filter(({ archivedAt }) => !archivedAt).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) { if (isScalarInstrument(instrument)) { return this.generateScalarInstrumentId(instrument); @@ -537,6 +564,20 @@ 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(instance: AnyInstrument): Promise { + if (!isSeriesInstrument(instance)) { + return isFileInstrument(instance); + } + const itemIds = getSeriesInstrumentItems(instance.content).map((internal) => { + return this.generateScalarInstrumentId({ internal }); + }); + return (await this.findInstancesByIds(itemIds)).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 @@ -630,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. * @@ -715,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!`); } 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..408d21932 --- /dev/null +++ b/apps/api/test/suites/06-instrument-repo-readd.suite.ts @@ -0,0 +1,178 @@ +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 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, + 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]); + 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 () => { + const repo = await registerRepo('admin-target', [otherGroup.id]); + const fetchSpy = await stubGitHub(); + + const response = await reAdd(adminHeaders, repo.url); + + expect(response.statusCode).toBe(201); + await expectSyncedWithNewToken(repo.id, fetchSpy); + }); +}); 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', () => { 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 }; 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/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'); 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(