[waritctd]

Summary

POST /api/tickets/{ticketId}/deposit-notice/draft → DepositNoticeService.createDraft → createDraftFromQuotation checked the role (sales) and the ticket status, but not ownership. update() and issue() in the same class call requireTicketOwner (requestRevision() inlines the same createdById != actor.id() comparison). So any sales user could:

• create a DRAFT deposit notice on another rep's deal;
• read that deal's quotation lines, prices, deposit percent, customer and project from the response;
• plant items that then became the owner's working draft.

Authz change, stated explicitly: createDraftFromQuotation now calls requireTicketOwner(ticketId, actor) after requireApprovedTicket. This is the same owner-only guard that update() and issue() use. It is not widened to sales_manager or ceo; requireRole(SALES_ROLES) already limits this method to sales. The guard sits in the shared method, so both callers get it:

• The ticket route (DepositNoticeController) is the bug.
• The pricing-request route (OrderConfirmationService.createDepositNoticeFromQuotation) already ran requireOwner before delegating, so its behaviour does not change. Its existing test createDepositNoticeFromQuotation_newEngine_nonOwnerSales_cannotReach still passes.

Files changed

Path	Change
backend/src/main/java/th/co/glr/hr/deposit/DepositNoticeService.java	Added requireTicketOwner(ticketId, actor) to createDraftFromQuotation (one line).
frontend/src/api/mockApi.js	depositNotices.createDraft now does hasRole('sales') plus ticket.createdById !== user.id → 403 ไม่มีสิทธิ์เข้าถึงรายการนี้. This mirrors requireRole(SALES_ROLES) + requireTicketOwner, the same way the mock's pricing-request route already does.
backend/src/test/java/th/co/glr/hr/dealquotation/DealQuotationOutcomeIntegrationTest.java	New test createDraft_ticketRoute_byNonOwningSales_refusedAndWritesNothing.
frontend/src/api/mockApi.depositNotices.test.js	New test ticket-level createDraft refuses a sales rep who does not own the deal and creates nothing.

There are two commits: the tests first (1e4ef01b), then the fix (416dc3b1).

Commands run

cd backend && ./mvnw -B -q -Dtest=DealQuotationOutcomeIntegrationTest -Dsurefire.failIfNoSpecifiedTests=false -Dtest.fork.count=1 test   # red, green, mutant, green again
cd backend && ./mvnw -B -q -Dtest='DepositNoticeServiceTest,DepositNoticeControllerTest,PaymentTrackIntegrationTest,FinanceDealIntegrationTest,OrderConfirmationIntegrationTest' -Dsurefire.failIfNoSpecifiedTests=false -Dtest.fork.count=1 test
cd frontend && npx vitest run src/api/mockApi.depositNotices.test.js   # red, green, mutant, green again
cd frontend && npx vitest run src/api/mockApi.remainingInvoiceAuthz.test.js src/api/mockApi.billingNoteAuthz.test.js src/api/mockApi.storedImportRequestRollup.test.js src/api/mockApi.priceImportCountries.test.js src/api/serverContract.test.js src/api/contract.test.js src/api/mockApi.depositNotices.test.js
cd frontend && npx eslint src/api/mockApi.js src/api/mockApi.depositNotices.test.js

Tests / build results

Integration tests ran against real Postgres through Testcontainers.

Run	Result
DealQuotationOutcomeIntegrationTest	34 tests, 0 failures, 0 errors
Other suites that call createDraft (DepositNoticeServiceTest 48, DepositNoticeControllerTest 8, PaymentTrackIntegrationTest 14, FinanceDealIntegrationTest 23, OrderConfirmationIntegrationTest 13)	all green
mockApi.depositNotices.test.js	10/10
Other mock and contract tests that reach createDraft	7 files, 61/61
eslint on the two changed frontend files	clean

Not run locally: the full mvnw verify, the full npm test, and npm run build. CI gates those.

Authz evidence

Real-service, real-DB. createDraft_ticketRoute_byNonOwningSales_refusedAndWritesNothing drives the real DepositNoticeService and repository against real Postgres (AbstractPostgresIntegrationTest). The test is written wrong-way-round: a non-owning sales rep gets 403, and SELECT COUNT(*) FROM sales.deposit_notice WHERE ticket_id = :id is 0.

Mutation-checked: with only the guard line removed (after mvnw clean), exactly this one test went red (34 tests, 1 failure, "Expecting code to raise a throwable."). Restoring the line left an empty diff, and the class was green again.

The mock test is drift protection only. It is not evidence about production authz.

Known risks

• Check order. A non-owner calling on a deal that is not yet approved still gets the 409 from requireApprovedTicket before the 403. This matches the ordering in update() and issue(), which check state before the owner. The mock checks the owner before status, so it returns 403 in that case. This is a status-only divergence, and the mock is not more permissive.
• Mock role gate added. The mock's createDraft now refuses every role except sales. Before, any logged-in role could draft in mock mode. Java's SALES_ROLES is also sales only. One known divergence, not new here: Java remaps a quotation-grant holder outside ฝ่ายขาย to role sales (fix(sales): grant holder outside ฝ่ายขาย acts as sales; ผู้ติดต่อ Phone/Email print on the quotation #1144) while the mock keeps their own role, so the mock refuses a grant holder on their own deal where Java accepts (mock stricter than Java, the safe direction; the mock's pricing-request route has the same gap). No existing mock-driven test relied on the old behaviour: the 61 tests above pass, and DepositNoticePage.test.jsx stubs the api.
• Mock is still looser elsewhere. The mock's depositNotices.update and issue still have no owner check, although the Java versions do. This is out of scope here and left as a follow-up.
• Overlap with fix(sales): the deposit notice bills what the customer's latest answered quotation printed (blank new-engine % = 30, legacy = 50, one row for lines and %) #1170. fix(sales): the deposit notice bills what the customer's latest answered quotation printed (blank new-engine % = 30, legacy = 50, one row for lines and %) #1170 (fix/deposit-null-percent-matches-pdf) adds tests to the same two test files. git merge-tree against its head shows a clean merge.

Test coverage (test-first)

Test	What it pins
DealQuotationOutcomeIntegrationTest#createDraft_ticketRoute_byNonOwningSales_refusedAndWritesNothing	Drive a deal to an accepted new-engine quotation plus confirmOrder as the owner. A second sales rep calls createDraft on the ticket route: ApiException with status FORBIDDEN and "ไม่มีสิทธิ์เข้าถึงรายการนี้", and 0 deposit_notice rows. Positive control: the owner then drafts on the same deal, and the count is 1.
mockApi.depositNotices.test.js › ticket-level createDraft refuses a sales rep who does not own the deal and creates nothing	The same scenario in the mock: sales2@glr.co.th gets 403 and listByTicket is empty. The owner still drafts. Also pins the mock's sales-only role gate: a ceo on a ticket that does not exist gets 403, not 404 (role runs before the lookup, as in Java).
(same Java test, review round)	Owner check runs before the status check: a non-owning rep on the still-unapproved fresh ticket gets 403, not the 409 that would reveal the deal's status.

Red before green (tests committed before the fix):

• Java: createDraft_ticketRoute_byNonOwningSales_refusedAndWritesNothing, java.lang.AssertionError: Expecting code to raise a throwable. at DealQuotationOutcomeIntegrationTest.java:338 (34 tests, 1 failure).
• Vitest: AssertionError: promise resolved "{ depositNotice: { id: 11, …(25) } }" instead of rejecting (1 failed, 9 passed).
• Review round (Opus review of 416dc3b, approve with notes): owner-before-status assertion red on 416dc3b with Expecting 403 FORBIDDEN … 409 CONFLICT (34 run, 1 failure), green after the reorder; mock role-gate pin red under the mutant requireSession() with expected Error: ไม่พบดีลนี้ { status: 404 } to match { status: 403 }, green restored. After the reorder: DealQuotationOutcome 34, DepositNoticeServiceTest 48, DepositNoticeControllerTest 8, OrderConfirmationIntegrationTest 13, DealQuotationConfirmOrder 31, DepositNoticeIssueGuard 3, all green (real Postgres); vitest depositNotices + contract 16/16; eslint clean.

Both failures are on the missing behaviour, not compile or fixture errors.

Deliberately not covered:

• sales_manager, ceo, import and account on this route. requireRole(SALES_ROLES) already refuses them and this PR does not change that.
• The HTTP layer. The test works at service level; the controller is a thin pass-through.
• A real-backend e2e (e2e-real) case.

🤖 Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
demo-glr	Ready	Preview	Oct 22, 2026 11:57am UTC