From 1e4ef01b19fb17969fe09d802a66c863da6f892c Mon Sep 17 00:00:00 2001 From: waritctd <148909369+waritctd@users.noreply.github.com> Date: Thu, 22 Oct 2026 15:51:32 +0700 Subject: [PATCH 1/4] test(sales): ticket-route deposit draft refuses a non-owning sales rep POST /api/tickets/{id}/deposit-notice/draft reaches DepositNoticeService.createDraftFromQuotation, which checks role and ticket status but not ownership, unlike update/issue. These tests pin the refusal (403, no deposit_notice row) and the owner positive control, against real Postgres and in the mock. Red until the guard lands. Co-Authored-By: Claude Opus 5.5 --- .../DealQuotationOutcomeIntegrationTest.java | 22 +++++++++++++++++++ .../src/api/mockApi.depositNotices.test.js | 14 ++++++++++++ 2 files changed, 36 insertions(+) diff --git a/backend/src/test/java/th/co/glr/hr/dealquotation/DealQuotationOutcomeIntegrationTest.java b/backend/src/test/java/th/co/glr/hr/dealquotation/DealQuotationOutcomeIntegrationTest.java index 08b841ab7..a190a4367 100644 --- a/backend/src/test/java/th/co/glr/hr/dealquotation/DealQuotationOutcomeIntegrationTest.java +++ b/backend/src/test/java/th/co/glr/hr/dealquotation/DealQuotationOutcomeIntegrationTest.java @@ -326,6 +326,28 @@ void createDepositNoticeFromQuotation_newEngine_nonOwnerSales_cannotReach() { Map.of("id", ticketId), Long.class)).isZero(); } + /** The ticket route (POST /api/tickets/{id}/deposit-notice/draft) had no owner check, unlike + * update/issue: another rep could draft on this deal and read its lines, prices and customer. */ + @Test + void createDraft_ticketRoute_byNonOwningSales_refusedAndWritesNothing() { + DealQuotationDto accepted = acceptedNewOriginQuotation(PricingRequestRecipient.DESIGNER, new BigDecimal("10")); + orderConfirmation.confirmOrder(accepted.pricingRequestId(), + new OrderConfirmationRequests.ConfirmOrderRequest(UUID.randomUUID().toString()), salesActor); + var emptyRequest = new DepositNoticeDraftRequest(null, null, null, null, null, null, null, null); + + assertThatThrownBy(() -> depositNoticeService.createDraft(ticketId, emptyRequest, otherSalesActor)) + .isInstanceOf(ApiException.class) + .hasFieldOrPropertyWithValue("status", HttpStatus.FORBIDDEN) + .hasMessageContaining("ไม่มีสิทธิ์เข้าถึงรายการนี้"); + assertThat(jdbc.queryForObject("SELECT COUNT(*) FROM sales.deposit_notice WHERE ticket_id = :id", + Map.of("id", ticketId), Long.class)).isZero(); + + // Positive control: the owner still drafts on the same deal. + assertThat(depositNoticeService.createDraft(ticketId, emptyRequest, salesActor).ticketId()).isEqualTo(ticketId); + assertThat(jdbc.queryForObject("SELECT COUNT(*) FROM sales.deposit_notice WHERE ticket_id = :id", + Map.of("id", ticketId), Long.class)).isEqualTo(1L); + } + /** An open revision draft outranks the accepted quotation by id: recordOutcome has no * open-revision guard, so "latest for this pricing request" is not "accepted". */ @Test diff --git a/frontend/src/api/mockApi.depositNotices.test.js b/frontend/src/api/mockApi.depositNotices.test.js index bc1a8e747..65e6c2b0f 100644 --- a/frontend/src/api/mockApi.depositNotices.test.js +++ b/frontend/src/api/mockApi.depositNotices.test.js @@ -252,6 +252,20 @@ describe('mockApi.pricingRequests.createDepositNoticeFromQuotation — new-engin expect(pick(fromTicket.items)).toEqual(pick(fromPr.items)); }); + // Mirrors DepositNoticeService.createDraftFromQuotation's owner guard. sales2@glr.co.th is a + // second seeded sales rep; the drive above runs as the first one, who owns the deal. + it('ticket-level createDraft refuses a sales rep who does not own the deal and creates nothing', async () => { + const { ticketId } = await driveTicketToNewEngineAcceptedQuotation(); + await api.auth.login({ email: 'sales2@glr.co.th', password: 'demo1234' }); + await expect(api.depositNotices.createDraft(ticketId, {})) + .rejects.toMatchObject({ status: 403, message: 'ไม่มีสิทธิ์เข้าถึงรายการนี้' }); + + await api.auth.login({ role: 'sales' }); + expect((await api.depositNotices.listByTicket(ticketId)).depositNotices).toHaveLength(0); + const { depositNotice } = await api.depositNotices.createDraft(ticketId, {}); + expect(depositNotice.ticketId).toBe(ticketId); + }); + it('still 409s while the new-engine quotation is only ISSUED (customer has not accepted)', async () => { const { ticketId, prId } = await driveTicketToNewEngineIssuedQuotation(); await expect(api.pricingRequests.createDepositNoticeFromQuotation(prId, {})) From 416dc3b1a113f7166a578c0e33a048fc5da01312 Mon Sep 17 00:00:00 2001 From: waritctd <148909369+waritctd@users.noreply.github.com> Date: Thu, 22 Oct 2026 15:51:33 +0700 Subject: [PATCH 2/4] fix(sales): owner check on the ticket-route deposit-notice draft createDraftFromQuotation now calls requireTicketOwner after requireApprovedTicket, the same owner-only guard update/issue use. Any sales user could previously draft on another rep's deal, read its quotation lines, prices, percent and customer, and plant the owner's working draft. The pricing-request route already ran requireOwner before delegating, so it behaves the same. The mock's depositNotices.createDraft mirrors requireRole(sales) + owner. Co-Authored-By: Claude Opus 5.5 --- .../main/java/th/co/glr/hr/deposit/DepositNoticeService.java | 1 + frontend/src/api/mockApi.js | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/backend/src/main/java/th/co/glr/hr/deposit/DepositNoticeService.java b/backend/src/main/java/th/co/glr/hr/deposit/DepositNoticeService.java index b2fa63925..90eb46e41 100644 --- a/backend/src/main/java/th/co/glr/hr/deposit/DepositNoticeService.java +++ b/backend/src/main/java/th/co/glr/hr/deposit/DepositNoticeService.java @@ -103,6 +103,7 @@ public DepositNoticeDto createDraftFromQuotation(long ticketId, DepositNoticeDra Integer sourceDepositPercent, UserPrincipal actor) { requireRole(actor, SALES_ROLES); TicketSummaryDto s = requireApprovedTicket(ticketId, actor); + requireTicketOwner(ticketId, actor); requireActiveLifecycle(s); // Auto-populate items from approved ticket items (legacy) or, failing that, the diff --git a/frontend/src/api/mockApi.js b/frontend/src/api/mockApi.js index d17302ee8..df7885fc2 100644 --- a/frontend/src/api/mockApi.js +++ b/frontend/src/api/mockApi.js @@ -13265,8 +13265,10 @@ export const api = { }, async createDraft(ticketId, payload) { - requireSession(); + // Mirrors DepositNoticeService.createDraftFromQuotation: requireRole(SALES_ROLES) + requireTicketOwner. + const user = hasRole('sales'); const ticket = findTicketRaw(Number(ticketId)); + if (ticket?.createdById !== user.id) fail('ไม่มีสิทธิ์เข้าถึงรายการนี้', 403); // Phase 1 lifecycle gate (mirrors DepositNoticeService.createDraft). requireActive(ticket); if (!['approved', 'quotation_issued', 'document_issued'].includes(ticket.status)) fail('ดีลต้องได้รับการอนุมัติก่อน', 409); From 4bec63dd15ed208f42d8332dbed54ee1a6125f67 Mon Sep 17 00:00:00 2001 From: waritctd <148909369+waritctd@users.noreply.github.com> Date: Thu, 22 Oct 2026 18:50:24 +0700 Subject: [PATCH 3/4] test(sales): ticket-route deposit draft checks the owner before the deal status; mock pins the sales-only role gate Review notes on #1171: a non-owning rep must get 403 on an unapproved deal too (not the 409 that leaks its status), and the mock's new role gate had no test. Co-Authored-By: Claude Fable 5.1 --- .../DealQuotationOutcomeIntegrationTest.java | 8 +++++++- frontend/src/api/mockApi.depositNotices.test.js | 4 ++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/backend/src/test/java/th/co/glr/hr/dealquotation/DealQuotationOutcomeIntegrationTest.java b/backend/src/test/java/th/co/glr/hr/dealquotation/DealQuotationOutcomeIntegrationTest.java index a190a4367..1815e4db0 100644 --- a/backend/src/test/java/th/co/glr/hr/dealquotation/DealQuotationOutcomeIntegrationTest.java +++ b/backend/src/test/java/th/co/glr/hr/dealquotation/DealQuotationOutcomeIntegrationTest.java @@ -330,10 +330,16 @@ void createDepositNoticeFromQuotation_newEngine_nonOwnerSales_cannotReach() { * update/issue: another rep could draft on this deal and read its lines, prices and customer. */ @Test void createDraft_ticketRoute_byNonOwningSales_refusedAndWritesNothing() { + var emptyRequest = new DepositNoticeDraftRequest(null, null, null, null, null, null, null, null); + // Owner before status: a non-owner must not learn whether another rep's deal is approved yet + // (403 here, not the 409 the owner would get on this still-unapproved ticket). + assertThatThrownBy(() -> depositNoticeService.createDraft(ticketId, emptyRequest, otherSalesActor)) + .isInstanceOf(ApiException.class) + .hasFieldOrPropertyWithValue("status", HttpStatus.FORBIDDEN); + DealQuotationDto accepted = acceptedNewOriginQuotation(PricingRequestRecipient.DESIGNER, new BigDecimal("10")); orderConfirmation.confirmOrder(accepted.pricingRequestId(), new OrderConfirmationRequests.ConfirmOrderRequest(UUID.randomUUID().toString()), salesActor); - var emptyRequest = new DepositNoticeDraftRequest(null, null, null, null, null, null, null, null); assertThatThrownBy(() -> depositNoticeService.createDraft(ticketId, emptyRequest, otherSalesActor)) .isInstanceOf(ApiException.class) diff --git a/frontend/src/api/mockApi.depositNotices.test.js b/frontend/src/api/mockApi.depositNotices.test.js index 65e6c2b0f..9e497e2a7 100644 --- a/frontend/src/api/mockApi.depositNotices.test.js +++ b/frontend/src/api/mockApi.depositNotices.test.js @@ -259,6 +259,10 @@ describe('mockApi.pricingRequests.createDepositNoticeFromQuotation — new-engin await api.auth.login({ email: 'sales2@glr.co.th', password: 'demo1234' }); await expect(api.depositNotices.createDraft(ticketId, {})) .rejects.toMatchObject({ status: 403, message: 'ไม่มีสิทธิ์เข้าถึงรายการนี้' }); + // Role gate (SALES_ROLES = sales only) runs before the ticket lookup, as in Java: a non-sales + // caller gets 403 even for a ticket that does not exist, never the 404. + await api.auth.login({ role: 'ceo' }); + await expect(api.depositNotices.createDraft(999999, {})).rejects.toMatchObject({ status: 403 }); await api.auth.login({ role: 'sales' }); expect((await api.depositNotices.listByTicket(ticketId)).depositNotices).toHaveLength(0); From 93a7ed4e46f48671947301e7d7dbe8a2be84fa7b Mon Sep 17 00:00:00 2001 From: waritctd <148909369+waritctd@users.noreply.github.com> Date: Thu, 22 Oct 2026 18:57:31 +0700 Subject: [PATCH 4/4] fix(sales): check the deposit-draft owner before the deal status Review note on #1171: with the status check first, a non-owning rep learned whether another rep's deal was approved (409) before being refused (403). Owner first, like the pricing-request route and RemainingInvoiceService. Mock: drop the optional chaining that could never fire (findTicketRaw already 404s). Co-Authored-By: Claude Fable 5.1 --- .../main/java/th/co/glr/hr/deposit/DepositNoticeService.java | 2 +- frontend/src/api/mockApi.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/src/main/java/th/co/glr/hr/deposit/DepositNoticeService.java b/backend/src/main/java/th/co/glr/hr/deposit/DepositNoticeService.java index 90eb46e41..da2d1c40a 100644 --- a/backend/src/main/java/th/co/glr/hr/deposit/DepositNoticeService.java +++ b/backend/src/main/java/th/co/glr/hr/deposit/DepositNoticeService.java @@ -102,8 +102,8 @@ public DepositNoticeDto createDraft(long ticketId, DepositNoticeDraftRequest req public DepositNoticeDto createDraftFromQuotation(long ticketId, DepositNoticeDraftRequest req, Integer sourceDepositPercent, UserPrincipal actor) { requireRole(actor, SALES_ROLES); - TicketSummaryDto s = requireApprovedTicket(ticketId, actor); requireTicketOwner(ticketId, actor); + TicketSummaryDto s = requireApprovedTicket(ticketId, actor); requireActiveLifecycle(s); // Auto-populate items from approved ticket items (legacy) or, failing that, the diff --git a/frontend/src/api/mockApi.js b/frontend/src/api/mockApi.js index df7885fc2..6e76f3ec5 100644 --- a/frontend/src/api/mockApi.js +++ b/frontend/src/api/mockApi.js @@ -13268,7 +13268,7 @@ export const api = { // Mirrors DepositNoticeService.createDraftFromQuotation: requireRole(SALES_ROLES) + requireTicketOwner. const user = hasRole('sales'); const ticket = findTicketRaw(Number(ticketId)); - if (ticket?.createdById !== user.id) fail('ไม่มีสิทธิ์เข้าถึงรายการนี้', 403); + if (ticket.createdById !== user.id) fail('ไม่มีสิทธิ์เข้าถึงรายการนี้', 403); // Phase 1 lifecycle gate (mirrors DepositNoticeService.createDraft). requireActive(ticket); if (!['approved', 'quotation_issued', 'document_issued'].includes(ticket.status)) fail('ดีลต้องได้รับการอนุมัติก่อน', 409);