fix(assignments): guard the per-assignment routes like the day routes

AssignmentOpsController declared @RequirePermission('day_edit') on its
mutations but carried only JwtAuthGuard. TripAccessGuard is what reads that
decorator and what resolves :tripId, so on this controller the permission
markers were metadata nobody looked at and the trip in the URL was never
matched against the caller. Its sibling, DayAssignmentsController, has had the
pair since the migration.

The two participants handlers also went straight to the service with the id
from the path, unlike move/time/transport, which ask getAssignmentForTrip
first. They ask now too.
This commit is contained in:
Maurice
2026-08-16 21:37:36 +02:00
committed by Maurice
parent ff27624e02
commit 0f0bf50cd3
3 changed files with 110 additions and 5 deletions
@@ -113,9 +113,14 @@ export class DayAssignmentsController {
/**
* /api/trips/:tripId/assignments/:id/* — per-assignment ops (move, time,
* participants), independent of the day path. Same parity rules as above.
*
* Same guard pair as the day controller, and for the same reason: TripAccessGuard
* is what resolves :tripId and what reads @RequirePermission. The per-handler
* getAssignmentForTrip calls answer a different question — whether the assignment
* sits on the trip in the URL, not whether the caller may be on that trip.
*/
@Controller('api/trips/:tripId/assignments')
@UseGuards(JwtAuthGuard)
@UseGuards(JwtAuthGuard, TripAccessGuard)
export class AssignmentOpsController {
constructor(private readonly assignments: AssignmentsService) {}
@@ -142,6 +147,9 @@ export class AssignmentOpsController {
@Get(':id/participants')
participants(@CurrentUser() user: User, @Param('tripId') tripId: string, @Param('id') id: string) {
if (!this.assignments.getAssignmentForTrip(id, tripId)) {
throw new HttpException({ error: 'Assignment not found' }, 404);
}
return { participants: this.assignments.getParticipants(id) };
}
@@ -191,6 +199,9 @@ export class AssignmentOpsController {
@Body() body: AssignmentParticipantsDto,
@Headers('x-socket-id') socketId?: string,
) {
if (!this.assignments.getAssignmentForTrip(id, tripId)) {
throw new HttpException({ error: 'Assignment not found' }, 404);
}
const participants = this.assignments.setParticipants(id, body.user_ids);
this.assignments.broadcast(tripId, 'assignment:participants', { assignmentId: Number(id), participants }, socketId);
return { participants };
+88
View File
@@ -194,4 +194,92 @@ describe('Assignments e2e (real auth guard + temp SQLite)', () => {
expect(res.status).toBe(200);
expect(res.body.assignment).toMatchObject({ id, day_id: 4, order_index: 0 });
});
// The per-assignment controller declared @RequirePermission but not the guard
// that reads it, so the decorators were inert metadata and :tripId was never
// checked against the caller at all. The handlers that ask
// getAssignmentForTrip(id, tripId) were happy as long as the assignment sat on
// the trip in the URL — which is true for the owner's trip too.
describe('a trip the caller cannot see', () => {
const FOREIGN_TRIP = 9;
beforeEach(() => {
canAccessTrip.mockImplementation((tripId: unknown) => (Number(tripId) === 5 ? { id: 5, user_id: 1 } : undefined));
db.prepare('INSERT OR IGNORE INTO days (id, trip_id) VALUES (30, ?), (31, ?)').run(FOREIGN_TRIP, FOREIGN_TRIP);
db.prepare('INSERT OR IGNORE INTO places (id, trip_id, name) VALUES (20, ?, ?)').run(FOREIGN_TRIP, 'Their hotel');
});
const seedForeignAssignment = () =>
Number(db.prepare('INSERT INTO day_assignments (day_id, place_id, order_index) VALUES (30, 20, 0)').run().lastInsertRowid);
it('404s a move instead of reordering their itinerary', async () => {
const id = seedForeignAssignment();
const res = await request(server)
.put(`/api/trips/${FOREIGN_TRIP}/assignments/${id}/move`)
.set('Cookie', sessionCookie(1))
.send({ new_day_id: 31, order_index: 0 });
expect(res.status).toBe(404);
expect(db.prepare('SELECT day_id FROM day_assignments WHERE id = ?').get(id)).toEqual({ day_id: 30 });
});
it('404s a time change instead of rewriting their schedule', async () => {
const id = seedForeignAssignment();
const res = await request(server)
.put(`/api/trips/${FOREIGN_TRIP}/assignments/${id}/time`)
.set('Cookie', sessionCookie(1))
.send({ place_time: '23:00', end_time: null });
expect(res.status).toBe(404);
expect(db.prepare('SELECT assignment_time FROM day_assignments WHERE id = ?').get(id)).toEqual({ assignment_time: null });
});
it('404s a transport change', async () => {
const id = seedForeignAssignment();
const res = await request(server)
.put(`/api/trips/${FOREIGN_TRIP}/assignments/${id}/transport`)
.set('Cookie', sessionCookie(1))
.send({ transport_mode: 'driving' });
expect(res.status).toBe(404);
});
it('404s setting participants instead of writing to their assignment', async () => {
const id = seedForeignAssignment();
const res = await request(server)
.put(`/api/trips/${FOREIGN_TRIP}/assignments/${id}/participants`)
.set('Cookie', sessionCookie(1))
.send({ user_ids: [1] });
expect(res.status).toBe(404);
expect(db.prepare('SELECT COUNT(*) AS n FROM assignment_participants WHERE assignment_id = ?').get(id)).toEqual({ n: 0 });
});
it('404s reading participants instead of disclosing who is on it', async () => {
const id = seedForeignAssignment();
db.prepare('INSERT INTO assignment_participants (assignment_id, user_id) VALUES (?, 2)').run(id);
const res = await request(server)
.get(`/api/trips/${FOREIGN_TRIP}/assignments/${id}/participants`)
.set('Cookie', sessionCookie(1));
expect(res.status).toBe(404);
});
// The guard has to reject an assignment borrowed from elsewhere even when the
// caller is legitimately on the trip named in the URL.
it('404s an assignment that belongs to another trip than the URL says', async () => {
const id = seedForeignAssignment();
const res = await request(server)
.put(`/api/trips/5/assignments/${id}/participants`)
.set('Cookie', sessionCookie(1))
.send({ user_ids: [1] });
expect(res.status).toBe(404);
expect(db.prepare('SELECT COUNT(*) AS n FROM assignment_participants WHERE assignment_id = ?').get(id)).toEqual({ n: 0 });
});
it('403s when the caller is on the trip but lacks day_edit', async () => {
const id = seedAssignment();
checkPermission.mockReturnValue(false);
const res = await request(server)
.put(`/api/trips/5/assignments/${id}/time`)
.set('Cookie', sessionCookie(1))
.send({ place_time: '09:00', end_time: null });
expect(res.status).toBe(403);
});
});
});
@@ -78,8 +78,10 @@ describe('AssignmentOpsController (parity with the per-assignment op routes)', (
expect(reconcile).toHaveBeenCalledWith('5', 'sock');
});
it('GET /:id/participants returns participants (access-only)', () => {
const s = svc({ getParticipants: vi.fn().mockReturnValue([{ user_id: 2 }]) } as Partial<AssignmentsService>);
it('GET /:id/participants 404 when the assignment is on another trip, else returns participants (access-only)', () => {
expect(thrown(() => new AssignmentOpsController(svc({ getAssignmentForTrip: vi.fn().mockReturnValue(undefined) } as Partial<AssignmentsService>)).participants(user, '5', '9')))
.toEqual({ status: 404, body: { error: 'Assignment not found' } });
const s = svc({ getAssignmentForTrip: vi.fn().mockReturnValue({ id: 9 }), getParticipants: vi.fn().mockReturnValue([{ user_id: 2 }]) } as Partial<AssignmentsService>);
expect(new AssignmentOpsController(s).participants(user, '5', '9')).toEqual({ participants: [{ user_id: 2 }] });
});
@@ -92,9 +94,13 @@ describe('AssignmentOpsController (parity with the per-assignment op routes)', (
expect(reconcile).toHaveBeenCalledWith('5', 'sock');
});
it('PUT /:id/participants sets + broadcasts (non-array bodies are the Zod pipe\'s 400, covered in e2e)', () => {
it('PUT /:id/participants 404 on a foreign assignment, else sets + broadcasts (non-array bodies are the Zod pipe\'s 400, covered in e2e)', () => {
const setParticipants = vi.fn().mockReturnValue([{ user_id: 2 }]); const broadcast = vi.fn();
expect(new AssignmentOpsController(svc({ setParticipants, broadcast } as Partial<AssignmentsService>)).setParticipants(user, '5', '9', { user_ids: [2] }, 'sock')).toEqual({ participants: [{ user_id: 2 }] });
expect(thrown(() => new AssignmentOpsController(svc({ getAssignmentForTrip: vi.fn().mockReturnValue(undefined), setParticipants } as Partial<AssignmentsService>)).setParticipants(user, '5', '9', { user_ids: [2] })))
.toEqual({ status: 404, body: { error: 'Assignment not found' } });
expect(setParticipants).not.toHaveBeenCalled();
const s = svc({ getAssignmentForTrip: vi.fn().mockReturnValue({ id: 9 }), setParticipants, broadcast } as Partial<AssignmentsService>);
expect(new AssignmentOpsController(s).setParticipants(user, '5', '9', { user_ids: [2] }, 'sock')).toEqual({ participants: [{ user_id: 2 }] });
expect(setParticipants).toHaveBeenCalledWith('9', [2]);
expect(broadcast).toHaveBeenCalledWith('5', 'assignment:participants', { assignmentId: 9, participants: [{ user_id: 2 }] }, 'sock');
});