From 3f2d324ce21b895e8dc652bc3d745ad6cbb5a14b Mon Sep 17 00:00:00 2001 From: jmgasper Date: Wed, 12 Aug 2026 16:52:12 +1000 Subject: [PATCH] PM-5858: Require assignment before ticket closure What was broken The Support API allowed a Support Team member to close a ticket without being assigned, and its ticket responses omitted the persisted closer ID. Root cause Closure authorization checked only the Support Team role, and close and unassign decisions were not serialized on the ticket row. What was changed Require assignment to the acting support user, lock close and unassign decisions against races, and expose the stored closer ID in ticket responses. Any added/updated tests Added coverage for unassigned rejection before writes, shared ticket locking, closer persistence and mapping, and preserved close idempotency and notification behavior. --- README.md | 4 +- src/tickets/dto/ticket-response.dto.ts | 3 ++ src/tickets/tickets.controller.ts | 5 +- src/tickets/tickets.service.spec.ts | 70 ++++++++++++++++++++++++-- src/tickets/tickets.service.ts | 42 ++++++++++++++-- 5 files changed, 115 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index eaed022..6dd99e3 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,8 @@ The service mounts all operations below `/v6/support`: replies read - `POST|DELETE /v6/support/tickets/:ticketId/assignees/me` — assign or unassign the current Support Team user -- `POST /v6/support/tickets/:ticketId/close` — close a ticket as Support Team +- `POST /v6/support/tickets/:ticketId/close` — close a ticket assigned to the + current Support Team user Interactive OpenAPI documentation is served at `/v6/support/api-docs`. @@ -33,6 +34,7 @@ the role as one multi-word value. Prisma owns a dedicated PostgreSQL `support` schema. The initial migration creates tickets, chronologically ordered responses, many-to-many assignees, ticket read states, response read receipts, and a notification outbox. +Closed ticket responses include the stored closer user ID for audit display. An absent receipt means unread. List responses compare the caller's `lastReadAt` with the latest request, response, or close activity. Opening or diff --git a/src/tickets/dto/ticket-response.dto.ts b/src/tickets/dto/ticket-response.dto.ts index 83aa2d3..070fb6e 100644 --- a/src/tickets/dto/ticket-response.dto.ts +++ b/src/tickets/dto/ticket-response.dto.ts @@ -78,6 +78,9 @@ export class TicketSummaryDto { @ApiPropertyOptional({ format: 'date-time' }) closedAt?: Date; + @ApiPropertyOptional({ example: '123456' }) + closedByUserId?: string; + @ApiProperty({ format: 'date-time' }) updatedAt!: Date; diff --git a/src/tickets/tickets.controller.ts b/src/tickets/tickets.controller.ts index b917845..43002d9 100644 --- a/src/tickets/tickets.controller.ts +++ b/src/tickets/tickets.controller.ts @@ -197,7 +197,7 @@ export class TicketsController { } /** - * Closes an open ticket as a Support Team user. + * Closes an open ticket assigned to the current Support Team user. * * @param actor Authenticated Topcoder support user. * @param ticketId Ticket UUID. @@ -208,7 +208,8 @@ export class TicketsController { @ApiOperation({ summary: 'Close a resolved support ticket' }) @ApiOkResponse({ type: TicketDetailDto }) @ApiForbiddenResponse({ - description: 'The caller is not on the Support Team.', + description: + 'The caller is not on the Support Team or is not assigned to the ticket.', }) async close( @CurrentActor() actor: SupportActor, diff --git a/src/tickets/tickets.service.spec.ts b/src/tickets/tickets.service.spec.ts index e03a559..39aa0b3 100644 --- a/src/tickets/tickets.service.spec.ts +++ b/src/tickets/tickets.service.spec.ts @@ -40,6 +40,7 @@ function createTicketRecord( assignees: [], challengeId: null, closedAt: null, + closedByUserId: null, description: 'The challenge submission is unavailable.', id: 'ticket-1', memberHandle: 'member_one', @@ -67,6 +68,7 @@ function createTicketRecord( */ function createHarness() { const tx = { + $queryRaw: jest.fn().mockResolvedValue([]), responseReadReceipt: { createMany: jest.fn().mockResolvedValue({ count: 0 }), updateMany: jest.fn().mockResolvedValue({ count: 0 }), @@ -451,12 +453,18 @@ describe('TicketsService', () => { try { const { notificationOutbox, service, tx } = createHarness(); tx.supportTicket.findUnique - .mockResolvedValueOnce({ status: TicketStatus.OPEN }) + .mockResolvedValueOnce({ + assignees: [{ userId: support.userId }], + status: TicketStatus.OPEN, + }) .mockResolvedValueOnce({ memberUserId: member.userId, status: TicketStatus.CLOSED, }) - .mockResolvedValueOnce({ status: TicketStatus.OPEN }); + .mockResolvedValueOnce({ + assignees: [{ userId: support.userId }], + status: TicketStatus.OPEN, + }); tx.supportResponse.create.mockResolvedValue({ id: 'response-reopen' }); const firstClosedAt = new Date('2026-08-07T02:00:00.000Z'); @@ -472,6 +480,13 @@ describe('TicketsService', () => { jest.setSystemTime(secondClosedAt); await service.close(support, 'ticket-1'); + expect(tx.supportTicket.updateMany).toHaveBeenCalledWith({ + data: expect.objectContaining({ + closedByUserId: support.userId, + status: TicketStatus.CLOSED, + }), + where: { id: 'ticket-1', status: TicketStatus.OPEN }, + }); expect(notificationOutbox.queueTicketClosed).toHaveBeenNthCalledWith( 1, tx, @@ -518,6 +533,25 @@ describe('TicketsService', () => { expect(tx.ticketAssignee.upsert).not.toHaveBeenCalled(); }); + it('locks the ticket before unassigning to serialize against closure', async () => { + const { service, tx } = createHarness(); + tx.supportTicket.findUnique.mockResolvedValue({ + status: TicketStatus.OPEN, + }); + + await service.unassignMe(support, 'ticket-1'); + + expect(tx.$queryRaw).toHaveBeenCalledTimes(1); + const lockQuery = tx.$queryRaw.mock.calls[0][0] as { strings: string[] }; + expect(lockQuery.strings.join('')).toContain('FOR UPDATE'); + expect(tx.$queryRaw.mock.invocationCallOrder[0]).toBeLessThan( + tx.supportTicket.findUnique.mock.invocationCallOrder[0], + ); + expect(tx.ticketAssignee.deleteMany).toHaveBeenCalledWith({ + where: { ticketId: 'ticket-1', userId: support.userId }, + }); + }); + it('role-gates both assignment and ticket closure', async () => { const { db, memberDirectory, service } = createHarness(); @@ -531,19 +565,48 @@ describe('TicketsService', () => { expect(db.$transaction).not.toHaveBeenCalled(); }); + it('rejects closure by unassigned support before writing or notifying', async () => { + const { db, notificationOutbox, service, tx } = createHarness(); + tx.supportTicket.findUnique.mockResolvedValue({ + assignees: [], + status: TicketStatus.OPEN, + }); + + await expect(service.close(support, 'ticket-1')).rejects.toMatchObject({ + message: 'Assign this support ticket to yourself before closing it.', + }); + expect(tx.$queryRaw).toHaveBeenCalledTimes(1); + const lockQuery = tx.$queryRaw.mock.calls[0][0] as { strings: string[] }; + expect(lockQuery.strings.join('')).toContain('FOR UPDATE'); + expect(tx.$queryRaw.mock.invocationCallOrder[0]).toBeLessThan( + tx.supportTicket.findUnique.mock.invocationCallOrder[0], + ); + expect(tx.supportTicket.updateMany).not.toHaveBeenCalled(); + expect(tx.ticketReadState.upsert).not.toHaveBeenCalled(); + expect(notificationOutbox.queueTicketClosed).not.toHaveBeenCalled(); + expect(notificationOutbox.dispatch).not.toHaveBeenCalled(); + expect(db.supportTicket.findUnique).not.toHaveBeenCalled(); + }); + it('treats an already-closed ticket as idempotent without enqueueing again', async () => { const { db, notificationOutbox, service, tx } = createHarness(); const closedAt = new Date('2026-08-07T02:00:00.000Z'); tx.supportTicket.findUnique.mockResolvedValue({ + assignees: [], status: TicketStatus.CLOSED, }); db.supportTicket.findUnique.mockResolvedValue( - createTicketRecord({ closedAt, status: TicketStatus.CLOSED }), + createTicketRecord({ + closedAt, + closedByUserId: support.userId, + status: TicketStatus.CLOSED, + }), ); const result = await service.close(support, 'ticket-1'); expect(result.status).toBe(TicketStatus.CLOSED); + expect(result.closedByUserId).toBe(support.userId); expect(tx.supportTicket.updateMany).not.toHaveBeenCalled(); expect(notificationOutbox.queueTicketClosed).not.toHaveBeenCalled(); expect(notificationOutbox.dispatch).not.toHaveBeenCalled(); @@ -552,6 +615,7 @@ describe('TicketsService', () => { it('does not create a close event when a concurrent close transition wins', async () => { const { notificationOutbox, service, tx } = createHarness(); tx.supportTicket.findUnique.mockResolvedValue({ + assignees: [{ userId: support.userId }], status: TicketStatus.OPEN, }); tx.supportTicket.updateMany.mockResolvedValueOnce({ count: 0 }); diff --git a/src/tickets/tickets.service.ts b/src/tickets/tickets.service.ts index 3f6d288..165c49e 100644 --- a/src/tickets/tickets.service.ts +++ b/src/tickets/tickets.service.ts @@ -433,6 +433,7 @@ export class TicketsService { ): Promise { this.assertSupportTeam(actor); await this.db.$transaction(async (tx) => { + await this.lockTicket(tx, ticketId); const ticket = await tx.supportTicket.findUnique({ select: { status: true }, where: { id: ticketId }, @@ -508,7 +509,7 @@ export class TicketsService { } /** - * Closes an open ticket as the authenticated Support Team member. + * Closes an open ticket assigned to the authenticated Support Team member. * * A conditional update makes concurrent and repeated closes idempotent. Only * the transaction that changes OPEN to CLOSED queues close email and Slack @@ -517,14 +518,22 @@ export class TicketsService { * @param actor authenticated Support Team member resolving the ticket. * @param ticketId target support ticket UUID. * @returns updated full ticket detail. - * @throws ForbiddenException when the actor lacks the Support Team role. + * @throws ForbiddenException when the actor lacks the Support Team role or + * is not assigned to the ticket. * @throws NotFoundException when the ticket does not exist. */ async close(actor: SupportActor, ticketId: string): Promise { this.assertSupportTeam(actor); const notificationIds = await this.db.$transaction(async (tx) => { + await this.lockTicket(tx, ticketId); const ticket = await tx.supportTicket.findUnique({ - select: { status: true }, + select: { + assignees: { + select: { userId: true }, + where: { userId: actor.userId }, + }, + status: true, + }, where: { id: ticketId }, }); if (!ticket) { @@ -533,6 +542,11 @@ export class TicketsService { if (ticket.status === TicketStatus.CLOSED) { return []; } + if (ticket.assignees.length === 0) { + throw new ForbiddenException( + 'Assign this support ticket to yourself before closing it.', + ); + } const closedAt = new Date(); const closeUpdate = await tx.supportTicket.updateMany({ @@ -564,6 +578,26 @@ export class TicketsService { return this.getById(actor, ticketId); } + /** + * Locks a ticket row so unassignment and closure decisions cannot interleave. + * + * @param tx active Prisma transaction. + * @param ticketId target support ticket UUID. + * @returns a promise resolved after PostgreSQL acquires the row lock. + * @throws Prisma database errors. + */ + private async lockTicket( + tx: Prisma.TransactionClient, + ticketId: string, + ): Promise { + await tx.$queryRaw(Prisma.sql` + SELECT "id" + FROM "support"."support_tickets" + WHERE "id" = ${ticketId}::uuid + FOR UPDATE + `); + } + /** * Rejects access unless the actor owns the ticket or belongs to Support Team. * @@ -639,6 +673,7 @@ export class TicketsService { assignees: record.assignees.map((assignee) => this.toAssignee(assignee)), challengeId: record.challengeId ?? undefined, closedAt: record.closedAt ?? undefined, + closedByUserId: record.closedByUserId ?? undefined, description: record.description, hasUnread: !actorReadState || actorReadState.lastReadAt < latestActivityAt, @@ -678,6 +713,7 @@ export class TicketsService { assignees: record.assignees.map((assignee) => this.toAssignee(assignee)), challengeId: record.challengeId ?? undefined, closedAt: record.closedAt ?? undefined, + closedByUserId: record.closedByUserId ?? undefined, description: record.description, hasUnread: !actorReadState || actorReadState.lastReadAt < latestActivityAt,