Instrument record PATCH is scoped by delete rules instead of update rules #1633

Activity

1. joshunrau

   added

   BugType: existing behavior is wrongType: existing behavior is wrong

   Priority: HighSignificant or frequently hit user impact; target the next release or twoSignificant or frequently hit user impact; target the next release or two

   Difficulty: LowIsolated change, about 2 hours or lessIsolated change, about 2 hours or less

   Area: APIapps/api (NestJS backend, Prisma models, CASL permissions)apps/api (NestJS backend, Prisma models, CASL permissions)

   SecurityAuthentication, authorization, data exposure or a vulnerable dependencyAuthentication, authorization, data exposure or a vulnerable dependency

   on Oct 5, 2026
2. joshunrau commented on Oct 26, 2026

   

   joshunrau

   on Oct 26, 2026

   CollaboratorAuthor

   More actions

   Still reproduces on main (d86740e). updateById has not changed since f29adf1, but the lines have moved. The guard is at instrument-records.controller.ts:106, the unscoped lookup at instrument-records.service.ts:441-443 and the delete-scoped write at :477. efa9070 renamed the test file, so the pinned test is now __tests__/instrument-records.service.test.ts:1170.

   In the mixed-scope case, patching the group A record is also a 500, not a refusal. The unscoped findFirst finds the record, the delete-scoped update matches no row, and Prisma throws P2025. That is not an HttpException, so libnest's global filter answers 500. #1759 describes the same mechanism.

   Fix

   Build the where once and pass it to both queries. deleteById (:163) and AssignmentsService.updateById (assignments.service.ts:199) already do this:

   const where = { AND: [accessibleQuery(ability, 'update', 'InstrumentRecord')], id };

   With one constant, the lookup and the write cannot end up on different actions again. No ability.can short-circuit is needed. The guard (jwt-auth.guard.ts:63-65) only admits a caller who holds an update InstrumentRecord rule, so this call cannot throw.

   apps/api/AGENTS.md:102 and .agents/docs/architecture/auth-and-permissions.md:76 name only a mismatch between the guard's subject and the service's subject as the way to reach the ForbiddenError 500. Add the action mismatch to both in the same PR. #1660 rewrites that paragraph of auth-and-permissions.md, so whichever lands second has to rebase it. #1807 moves updateById to :431 but does not change it.

   An update grant also reads

   $UpdateInstrumentRecordData accepts { "data": {} }, and the route returns the stored record. After the fix, an update InstrumentRecord grant can therefore read every record in its scope. That includes a STANDARD user, who has no read InstrumentRecord rule. I'd accept this, since the 200 the issue expects implies it, and document it under "Where an ability comes from" in auth-and-permissions.md. If it is not acceptable, gate the route on both actions with an array, as summary.controller.ts:15 does, and add the read scope to the where. The guard then answers an update-only caller with 403, and no new 500 appears.

   Tests

   • Unit: when you invert the test at :1170, also assert that findFirst receives the same where as update. The model is mocked, so that argument is the only place the scope is visible. The existing null case at :1091 already covers the 404.
   • E2E: the route has no web client, so add the cases to the server-side authorization block of testing/src/specs/authorization.spec.ts, next to the grant test at :617. Put a STANDARD user in groups A and B, because a scoped grant must name one of the user's own groups. As admin, seed one DNP_HAPPINESS_QUESTIONNAIRE record in each group with api.uploadRecords and the happinessRecord helper (:300). Its data has no date or set value, so Instrument record PATCH rejects every update to a record with a date field, because stored dates are never revived #1723 does not interfere. ApiClient.uploadRecords is typed { subjectId: string }[], so widen the type to expose the record id.
     • Grant update on A and delete on B, then log in again. PATCH A should return 200 and PATCH B 404. On main they return 500 and 200.
     • Grant update on A alone. PATCH A should return 200. On main it returns 500.

   Related

   #1723 and #1639 also change updateById and the same describe('updateById') block. Land all three in one PR, with one fix(api) commit each, in this order: this issue, #1723, #1639. Keep these authorization cases in authorization.spec.ts, not in the instrument-record-update.spec.ts that #1639 proposes. #1728 is the same unscoped lookup by id, in InstrumentReposService.sync.

   _{Triage note written with Claude Code. Code references point at main @ d86740e.}
3. joshunrau

   closed this as completedin #1899on Nov 10, 2026
4. david-roper

   added a commit that references this issue on Nov 10, 2026

   fix(api): scope instrument record updates by update rules

   ...

   This issue will close once commit d14bad9 is merged into the 'main' branch.

   d14bad9