diff --git a/.agents/docs/architecture/auth-and-permissions.md b/.agents/docs/architecture/auth-and-permissions.md index e67b57f18..4723d5275 100644 --- a/.agents/docs/architecture/auth-and-permissions.md +++ b/.agents/docs/architecture/auth-and-permissions.md @@ -70,6 +70,16 @@ available in this codebase. Check the `where` clause of every query you add or t that the controller actually forwards `@CurrentUser('ability')` — `EntityOperationOptions.ability` is optional, so a controller that simply never passes it compiles and runs. +**A defined ability holding no rule for the queried subject does not produce a deny-all filter — +CASL's `accessibleBy` throws `ForbiddenError`.** That error is not an `HttpException`, so the +global exception filter renders it as a 500. It is reachable whenever the route guard names a +different subject than the service queries: a STANDARD user passes a `read Instrument` guard but +holds no rule on `InstrumentRecord`, so a record query made on their behalf throws. The throw is +deliberate — `accessibleQuery` is kept as-is rather than returning a deny-all filter — so where +such a caller is legitimate, guard the query with `ability.can(action, subject)` and return the +empty result instead; `findInstrumentIdsBySubject` in `src/instruments/instruments.service.ts` is +the reference example. + One call site uses `{ ...accessibleQuery(...), id: subjectId }` instead of `AND: [...]` (`src/subjects/subjects.service.ts`). Spreading merges keys and will silently lose a condition if the generated query and the literal share one. Prefer `AND`. diff --git a/apps/api/AGENTS.md b/apps/api/AGENTS.md index 1b81f53f0..f3d1ac5b0 100644 --- a/apps/api/AGENTS.md +++ b/apps/api/AGENTS.md @@ -83,6 +83,14 @@ restriction**, which reads clinical data across every group. Nothing catches thi eslint, not a passing test suite. It is the single highest-severity mistake available in this codebase, so verify the `where` clause of any query you add or edit. +The defined case has the opposite failure mode: **an ability holding no rule at all for the queried +subject makes `accessibleQuery` throw CASL's `ForbiddenError`**, which is not an `HttpException` +and so surfaces as a 500. This is reachable whenever a route's guard names one subject but the +service queries another — a STANDARD user passes `read Instrument` yet holds no `read` rule on +`InstrumentRecord`. That behaviour is deliberate and stays; where such a caller is legitimate, +short-circuit before the query with `ability.can(action, subject)` and return the empty result — +see `findInstrumentIdsBySubject` in `src/instruments/instruments.service.ts`. + Granting a new `action`/`subject` pair means editing `src/auth/ability.factory.ts` and adding tests for **both** the allow and the deny case — see `src/auth/__tests__/ability.factory.test.ts`. diff --git a/apps/api/src/instruments/__tests__/instruments.service.spec.ts b/apps/api/src/instruments/__tests__/instruments.service.spec.ts index b15da3da6..eb6d878c5 100644 --- a/apps/api/src/instruments/__tests__/instruments.service.spec.ts +++ b/apps/api/src/instruments/__tests__/instruments.service.spec.ts @@ -9,6 +9,9 @@ import type { SeriesInstrument } from '@opendatacapture/runtime-core'; import type { WithID } from '@opendatacapture/schemas/core'; import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { AbilityFactory } from '@/auth/ability.factory'; +import { accessibleQuery, createAppAbility } from '@/auth/ability.utils'; + import { InstrumentsService } from '../instruments.service'; import type { InstrumentVirtualizationContext } from '../instruments.service'; @@ -644,4 +647,79 @@ describe('InstrumentsService', () => { ]); }); }); + + describe('find', () => { + 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 () => { + await instrumentsService.find(); + expect(instrumentRecordModel.findMany).not.toHaveBeenCalled(); + }); + + // A `records: { some: ... }` relation filter compiles to a $lookup that materialises every + // record belonging to an instrument, which mongodb aborts past 100 MiB. The subject's own + // records are queried instead, so the work is bounded by the subject rather than the instrument. + it('should resolve the subject filter against records rather than joining from instruments', async () => { + instrumentRecordModel.findMany.mockResolvedValueOnce([{ instrumentId: 'id-1' }, { instrumentId: 'id-2' }] as any); + + await instrumentsService.find({ subjectId: 'subject-1' }); + + expect(instrumentRecordModel.findMany).toHaveBeenCalledWith({ + distinct: ['instrumentId'], + select: { instrumentId: true }, + where: { AND: [{}, { subjectId: 'subject-1' }] } + }); + expect(instrumentModel.findMany.mock.lastCall?.[0]).toMatchObject({ + where: { AND: expect.arrayContaining([{ id: { in: ['id-1', 'id-2'] } }]) } + }); + expect(JSON.stringify(instrumentModel.findMany.mock.lastCall?.[0])).not.toContain('records'); + }); + + it('should constrain the record lookup to what the caller may read, so the filter cannot be resolved from other groups records', async () => { + // Conditions are what make this meaningful: an unconditional read rule yields `{}`, which is + // indistinguishable from the ability never having been applied. + const ability = createAppAbility([ + { action: 'read', conditions: { groupId: { in: ['group-1'] } }, subject: 'InstrumentRecord' }, + { action: 'read', subject: 'Instrument' } + ]); + + await instrumentsService.find({ subjectId: 'subject-1' }, { ability }); + + const [call] = instrumentRecordModel.findMany.mock.lastCall as [{ where: { AND: unknown[] } }]; + expect(call.where.AND[0]).toStrictEqual(accessibleQuery(ability, 'read', 'InstrumentRecord')); + }); + + // Built through the factory rather than by hand: a hand-built ability tends to include a + // `read InstrumentRecord` rule, and it is the absence of one that breaks. A STANDARD user holds + // `create` but not `read`, and this route is gated on `read Instrument`, which they do hold. + it('should resolve for a caller who may read no records, rather than failing the request', async () => { + const abilityFactory = new AbilityFactory(MockFactory.createMock(LoggingService) as unknown as LoggingService); + const ability = abilityFactory.createForPayload({ + basePermissionLevel: 'STANDARD', + groups: [{ id: 'group-1' }], + id: 'user-1' + } as any); + + await expect(instrumentsService.find({ subjectId: 'subject-1' }, { ability })).resolves.toStrictEqual([]); + + expect(instrumentRecordModel.findMany).not.toHaveBeenCalled(); + expect(instrumentModel.findMany.mock.lastCall?.[0]).toMatchObject({ + where: { AND: expect.arrayContaining([{ id: { in: [] } }]) } + }); + }); + + it('should return nothing when the subject has no records', async () => { + instrumentRecordModel.findMany.mockResolvedValueOnce([]); + + await instrumentsService.find({ subjectId: 'subject-with-no-records' }); + + expect(instrumentModel.findMany.mock.lastCall?.[0]).toMatchObject({ + where: { AND: expect.arrayContaining([{ id: { in: [] } }]) } + }); + }); + }); }); diff --git a/apps/api/src/instruments/instruments.service.ts b/apps/api/src/instruments/instruments.service.ts index d81c84423..6be89fdcc 100644 --- a/apps/api/src/instruments/instruments.service.ts +++ b/apps/api/src/instruments/instruments.service.ts @@ -36,6 +36,7 @@ import type { import { pick } from 'lodash-es'; import { accessibleQuery } from '@/auth/ability.utils'; +import type { AppAbility } from '@/auth/auth.types'; import type { EntityOperationOptions } from '@/core/types'; import { CreateInstrumentDto } from './dto/create-instrument.dto'; @@ -261,18 +262,20 @@ export class InstrumentsService { { ability }: EntityOperationOptions = {}, groupIds?: string[] ): Promise>[]> { + // Resolved against InstrumentRecord rather than expressed as a `records: { some: ... }` relation + // filter. On mongodb prisma compiles that filter into a $lookup which materialises *every* record + // belonging to an instrument into one array before applying the predicate, and mongodb caps a + // single document's $lookup output at 100 MiB. A busy instrument therefore fails the whole query + // outright once it passes that threshold. Querying the child collection is bounded by the + // subject's own records instead of the instrument's. + const subjectInstrumentIds = query.subjectId + ? await this.findInstrumentIdsBySubject(query.subjectId, ability) + : null; + const instruments = await this.instrumentModel.findMany({ where: { AND: [ - { - records: query.subjectId - ? { - some: { - subjectId: query.subjectId - } - } - : undefined - }, + subjectInstrumentIds ? { id: { in: subjectInstrumentIds } } : {}, query.seriesGroupId ? { OR: [ @@ -520,6 +523,29 @@ export class InstrumentsService { >; } + /** + * The ids of the instruments the subject has at least one record for. + * + * Scoped to the records the caller may read, so the filter cannot be resolved from records outside + * their groups — the ids feed straight into the instrument query, so an unscoped lookup would + * disclose which instruments a subject has been administered elsewhere. + */ + private async findInstrumentIdsBySubject(subjectId: string, ability?: AppAbility): Promise { + // `accessibleQuery` throws rather than returning a restrictive filter when the ability holds no + // rule for the subject at all, and a STANDARD user holds `create` but not `read` on + // InstrumentRecord. This route is gated on `read Instrument`, which they do hold, so they reach + // here; no readable records means no instruments qualify. + if (ability && !ability.can('read', 'InstrumentRecord')) { + return []; + } + const records = await this.instrumentRecordModel.findMany({ + distinct: ['instrumentId'], + select: { instrumentId: true }, + where: { AND: [accessibleQuery(ability, 'read', 'InstrumentRecord'), { subjectId }] } + }); + return records.map((record) => record.instrumentId); + } + /** * The title of an existing series instrument that contains the exact same ordered sequence of items as * `items`, or `null` when none exists. Used to warn before creating a redundant series. diff --git a/testing/src/specs/authorization.spec.ts b/testing/src/specs/authorization.spec.ts index 90ceae4a3..ad2f0ec76 100644 --- a/testing/src/specs/authorization.spec.ts +++ b/testing/src/specs/authorization.spec.ts @@ -39,5 +39,44 @@ test.describe('authorization', () => { await page.goto('/session/remote-assignment'); await expect(page).toHaveURL('/session/start-session'); }); + + // `GET /v1/instruments/info` is gated on `read Instrument`, which a standard user holds, but + // resolving `subjectId` reads instrument records, which they do not. Scoping that lookup to the + // caller must answer with an empty list rather than failing the request. + // + // The defect this endpoint was changed for -- a mongodb $lookup exceeding its 100 MiB per-document + // ceiling once one instrument holds ~182,000 records -- cannot be reproduced at this tier. That + // gap is deliberate; the evidence for it is in the PR, measured against a live instance. + test('should answer the subject-filtered instrument list for a caller who may read no records', async ({ + apiRequestContext, + roleAccount + }) => { + const { accessToken } = await roleAccount('STANDARD'); + + const response = await apiRequestContext.get('/api/v1/instruments/info?subjectId=any-subject', { + headers: { Authorization: `Bearer ${accessToken}` } + }); + + expect(response.status()).toBe(200); + expect(await response.json()).toStrictEqual([]); + }); + }); + + // The populated case -- a group manager reaching /datahub/$subjectId/table and picking an + // instrument from the list -- is covered end to end by `instrument-completion.spec.ts`, which + // administers one first so the list has something in it. This case only pins the contract for a + // subject with no visible records: an empty list, not an error. + test('should answer the subject-filtered instrument list for a group manager rather than erroring', async ({ + apiRequestContext, + roleAccount + }) => { + const { accessToken } = await roleAccount('GROUP_MANAGER'); + + const response = await apiRequestContext.get('/api/v1/instruments/info?subjectId=any-subject', { + headers: { Authorization: `Bearer ${accessToken}` } + }); + + expect(response.status()).toBe(200); + expect(await response.json()).toStrictEqual([]); }); });