From b67b7e28f20087acd2a2701c9f1955cd4b68730a Mon Sep 17 00:00:00 2001 From: khaira777 <777gurkirat@gmail.com> Date: Sun, 18 Oct 2026 13:59:34 -0400 Subject: [PATCH 1/2] fix(auth): enforce payment permission eligibility in pre-verified order status completion (#1207) --- main/routes/orders.ts | 301 +++++++++++++++---------- tests/manager-pin-verification.test.ts | 58 +++++ 2 files changed, 245 insertions(+), 114 deletions(-) diff --git a/main/routes/orders.ts b/main/routes/orders.ts index 098f1e30b..c32c69759 100644 --- a/main/routes/orders.ts +++ b/main/routes/orders.ts @@ -2299,6 +2299,167 @@ router.post( }, ); +function evaluateOrderStatusPaymentBlock( + db: ReturnType, + orderId: string | number | unknown, + orderTotal: unknown, + orderRoundOff: unknown, +): { + minorFactor: number; + roundCurrency: (amount: number) => number; + evaluatedBills: Array<{ + id: string; + paymentStatus: string; + totalMinor: number; + unroundedTotalMinor: number; + recordedPaidMinor: number; + effectiveCollectedMinor: number; + hasCollectedFunds: boolean; + isPayable: boolean; + unpaidBalanceMinor: number; + isSettled: boolean; + }>; + orderTotalMinor: number; + payableBills: Array<{ + id: string; + paymentStatus: string; + totalMinor: number; + unroundedTotalMinor: number; + recordedPaidMinor: number; + effectiveCollectedMinor: number; + hasCollectedFunds: boolean; + isPayable: boolean; + unpaidBalanceMinor: number; + isSettled: boolean; + }>; + totalOutstandingMinor: number; + totalCollectedMinor: number; + paymentBlocksCompletion: boolean; +} { + const tenantCurrency = getTenantCurrency(); + const minorFactor = getCurrencyMinorUnitFactor(tenantCurrency); + const fractionDigits = getCurrencyFractionDigits(tenantCurrency); + const roundCurrency = (amount: number): number => + fractionDigits === 0 ? Math.round(amount) : Number(amount.toFixed(fractionDigits)); + + const billRows = db + .prepare(` + SELECT b.*, + COALESCE((SELECT SUM(r.amount_cents) FROM refunds r WHERE r.bill_id = b.id), 0) AS refunded_cents + FROM bills b + WHERE b.order_id = ? + ORDER BY b.id ASC + `) + .all(String(orderId)) as Array<{ + id: string; + total?: number | null; + round_off?: number | null; + paid_amount?: number | null; + refunded_cents?: number | null; + balance?: number | null; + payment_details?: unknown; + payment_status?: string | null; + }>; + + const evaluatedBills = billRows.map((b) => { + const totalMinor = Math.max(0, Math.round(Number(b.total || 0) * minorFactor)); + const roundOffMinor = Math.round(Number(b.round_off || 0) * minorFactor); + const unroundedTotalMinor = Math.max(0, totalMinor - roundOffMinor); + const paidMinor = Math.max(0, Math.round(Number(b.paid_amount || 0) * minorFactor)); + const refundedMinor = Math.max(0, Math.round(Number(b.refunded_cents || 0))); + const fallbackBalance = Math.max(0, Number(b.total || 0) - Number(b.paid_amount || 0)); + const storedBalanceMinor = Math.max( + 0, + Math.round(Number(b.balance ?? fallbackBalance) * minorFactor), + ); + let parsedPaymentDetails: Array<{ amount?: unknown }> = []; + if (typeof b.payment_details === 'string' && b.payment_details.trim()) { + try { + const parsed = JSON.parse(b.payment_details); + if (Array.isArray(parsed)) parsedPaymentDetails = parsed; + } catch { + parsedPaymentDetails = []; + } + } else if (Array.isArray(b.payment_details)) { + parsedPaymentDetails = b.payment_details as Array<{ amount?: unknown }>; + } + const detailsPaidMinor = parsedPaymentDetails.reduce( + (sum, entry) => sum + Math.max(0, Math.round(Number(entry?.amount || 0) * minorFactor)), + 0, + ); + const recordedPaidMinor = Math.max(paidMinor, detailsPaidMinor); + const netCollectedMinor = Math.max(0, recordedPaidMinor - refundedMinor); + const paymentStatus = String(b.payment_status || ''); + const effectiveCollectedMinor = Math.max( + netCollectedMinor, + paymentStatus === 'paid' ? Math.max(0, totalMinor - refundedMinor) : 0, + ); + const hasCollectedFunds = + effectiveCollectedMinor > 0 || + (recordedPaidMinor > 0 && paymentStatus !== 'refunded') || + paymentStatus === 'partial' || + paymentStatus === 'partially_refunded' || + (paymentStatus === 'paid' && (totalMinor > 0 || parsedPaymentDetails.length > 0)); + const isPayable = totalMinor > 0 || storedBalanceMinor > 0; + const unpaidBalanceMinor = isPayable + ? Math.max(storedBalanceMinor, Math.max(0, totalMinor - recordedPaidMinor)) + : 0; + const isSettled = + !isPayable || + (paymentStatus === 'paid' && + unpaidBalanceMinor === 0 && + recordedPaidMinor >= totalMinor); + return { + id: b.id, + paymentStatus, + totalMinor, + unroundedTotalMinor, + recordedPaidMinor, + effectiveCollectedMinor, + hasCollectedFunds, + isPayable, + unpaidBalanceMinor, + isSettled, + }; + }); + + const orderTotalMinor = Math.max(0, Math.round(Number(orderTotal || 0) * minorFactor)); + const orderRoundOffMinor = Math.round(Number(orderRoundOff || 0) * minorFactor); + const orderUnroundedTotalMinor = Math.max(0, orderTotalMinor - orderRoundOffMinor); + const effectiveOrderTargetMinor = Math.min(orderTotalMinor, orderUnroundedTotalMinor); + const payableBills = evaluatedBills.filter((b) => b.isPayable); + const billedCoverageMinor = evaluatedBills.reduce( + (sum, b) => sum + Math.max(b.totalMinor, b.unroundedTotalMinor), + 0, + ); + const unbilledMinor = + payableBills.length === 0 + ? orderTotalMinor + : Math.max(0, effectiveOrderTargetMinor - billedCoverageMinor); + const totalOutstandingMinor = + payableBills.reduce((sum, b) => sum + b.unpaidBalanceMinor, 0) + unbilledMinor; + const totalCollectedMinor = evaluatedBills.reduce( + (sum, b) => sum + b.effectiveCollectedMinor, + 0, + ); + + const paymentBlocksCompletion = + totalOutstandingMinor > 0 || + (orderTotalMinor > 0 && payableBills.length === 0) || + payableBills.some((b) => !b.isSettled); + + return { + minorFactor, + roundCurrency, + evaluatedBills, + orderTotalMinor, + payableBills, + totalOutstandingMinor, + totalCollectedMinor, + paymentBlocksCompletion, + }; +} + router.patch( '/:id/status', orderWriteRateLimit, @@ -2340,6 +2501,15 @@ router.patch( return res.status(429).json({ error: 'Too many PIN attempts. Try again in 15 minutes.' }); } if (/^\d{4,6}$/.test(override_pin)) { + const preVerificationRequiresPayment = + status === 'completed' && + evaluateOrderStatusPaymentBlock( + db, + req.params.id, + (order as any).total, + (order as any).round_off, + ).paymentBlocksCompletion; + const explicitApproverId = req.body.approver_id || req.body.manager_id || req.body.user_id; const candidateUsers = explicitApproverId @@ -2358,7 +2528,8 @@ router.patch( if ( hasRole(candidate.role, ROLE_ACCESS.ownerManager) && hasPermission(candidate.id, 'orders.status.update') && - (status !== 'cancelled' || hasPermission(candidate.id, 'orders.item.cancel')) + (status !== 'cancelled' || hasPermission(candidate.id, 'orders.item.cancel')) && + (!preVerificationRequiresPayment || hasPermission(candidate.id, 'payments.take')) ) { if (await verifyPinAsync(candidate.pin_hash, override_pin)) { preVerifiedStatusApprover = { @@ -2440,121 +2611,23 @@ router.patch( const statusOrder = ['pending', 'preparing', 'ready', 'served', 'completed']; const currentStatusIndex = statusOrder.indexOf(currentOrder.status); - const tenantCurrency = getTenantCurrency(); - const minorFactor = getCurrencyMinorUnitFactor(tenantCurrency); - const fractionDigits = getCurrencyFractionDigits(tenantCurrency); - const roundCurrency = (amount: number): number => - fractionDigits === 0 ? Math.round(amount) : Number(amount.toFixed(fractionDigits)); - - const billRows = db - .prepare(` - SELECT b.*, - COALESCE((SELECT SUM(r.amount_cents) FROM refunds r WHERE r.bill_id = b.id), 0) AS refunded_cents - FROM bills b - WHERE b.order_id = ? - ORDER BY b.id ASC - `) - .all(req.params.id) as Array<{ - id: string; - total?: number | null; - round_off?: number | null; - paid_amount?: number | null; - refunded_cents?: number | null; - balance?: number | null; - payment_details?: unknown; - payment_status?: string | null; - }>; - - const evaluatedBills = billRows.map((b) => { - const totalMinor = Math.max(0, Math.round(Number(b.total || 0) * minorFactor)); - const roundOffMinor = Math.round(Number(b.round_off || 0) * minorFactor); - const unroundedTotalMinor = Math.max(0, totalMinor - roundOffMinor); - const paidMinor = Math.max(0, Math.round(Number(b.paid_amount || 0) * minorFactor)); - const refundedMinor = Math.max(0, Math.round(Number(b.refunded_cents || 0))); - const fallbackBalance = Math.max(0, Number(b.total || 0) - Number(b.paid_amount || 0)); - const storedBalanceMinor = Math.max( - 0, - Math.round(Number(b.balance ?? fallbackBalance) * minorFactor), - ); - let parsedPaymentDetails: Array<{ amount?: unknown }> = []; - if (typeof b.payment_details === 'string' && b.payment_details.trim()) { - try { - const parsed = JSON.parse(b.payment_details); - if (Array.isArray(parsed)) parsedPaymentDetails = parsed; - } catch { - parsedPaymentDetails = []; - } - } else if (Array.isArray(b.payment_details)) { - parsedPaymentDetails = b.payment_details as Array<{ amount?: unknown }>; - } - const detailsPaidMinor = parsedPaymentDetails.reduce( - (sum, entry) => sum + Math.max(0, Math.round(Number(entry?.amount || 0) * minorFactor)), - 0, - ); - const recordedPaidMinor = Math.max(paidMinor, detailsPaidMinor); - const netCollectedMinor = Math.max(0, recordedPaidMinor - refundedMinor); - const paymentStatus = String(b.payment_status || ''); - const effectiveCollectedMinor = Math.max( - netCollectedMinor, - paymentStatus === 'paid' ? Math.max(0, totalMinor - refundedMinor) : 0, - ); - const hasCollectedFunds = - effectiveCollectedMinor > 0 || - (recordedPaidMinor > 0 && paymentStatus !== 'refunded') || - paymentStatus === 'partial' || - paymentStatus === 'partially_refunded' || - (paymentStatus === 'paid' && (totalMinor > 0 || parsedPaymentDetails.length > 0)); - const isPayable = totalMinor > 0 || storedBalanceMinor > 0; - const unpaidBalanceMinor = isPayable - ? Math.max(storedBalanceMinor, Math.max(0, totalMinor - recordedPaidMinor)) - : 0; - const isSettled = - !isPayable || - (paymentStatus === 'paid' && - unpaidBalanceMinor === 0 && - recordedPaidMinor >= totalMinor); - return { - id: b.id, - paymentStatus, - totalMinor, - unroundedTotalMinor, - recordedPaidMinor, - effectiveCollectedMinor, - hasCollectedFunds, - isPayable, - unpaidBalanceMinor, - isSettled, - }; - }); - - const orderTotalMinor = Math.max( - 0, - Math.round(Number(currentOrder.total || 0) * minorFactor), - ); - const orderRoundOffMinor = Math.round(Number(currentOrder.round_off || 0) * minorFactor); - const orderUnroundedTotalMinor = Math.max(0, orderTotalMinor - orderRoundOffMinor); - const effectiveOrderTargetMinor = Math.min(orderTotalMinor, orderUnroundedTotalMinor); - const payableBills = evaluatedBills.filter((b) => b.isPayable); - const billedCoverageMinor = evaluatedBills.reduce( - (sum, b) => sum + Math.max(b.totalMinor, b.unroundedTotalMinor), - 0, - ); - const unbilledMinor = - payableBills.length === 0 - ? orderTotalMinor - : Math.max(0, effectiveOrderTargetMinor - billedCoverageMinor); - const totalOutstandingMinor = - payableBills.reduce((sum, b) => sum + b.unpaidBalanceMinor, 0) + unbilledMinor; - const totalCollectedMinor = evaluatedBills.reduce( - (sum, b) => sum + b.effectiveCollectedMinor, - 0, + const { + minorFactor, + roundCurrency, + evaluatedBills, + orderTotalMinor, + payableBills, + totalOutstandingMinor, + totalCollectedMinor, + paymentBlocksCompletion: rawPaymentBlocksCompletion, + } = evaluateOrderStatusPaymentBlock( + db, + req.params.id, + currentOrder.total, + currentOrder.round_off, ); - const paymentBlocksCompletion = - status === 'completed' && - (totalOutstandingMinor > 0 || - (orderTotalMinor > 0 && payableBills.length === 0) || - payableBills.some((b) => !b.isSettled)); + status === 'completed' && rawPaymentBlocksCompletion; const requireKitchenDelivered = db .prepare( diff --git a/tests/manager-pin-verification.test.ts b/tests/manager-pin-verification.test.ts index 86501898b..e8b6f8b76 100644 --- a/tests/manager-pin-verification.test.ts +++ b/tests/manager-pin-verification.test.ts @@ -316,6 +316,64 @@ async function run() { 'PIN-only void records matching manager 1 in approved_by', ); + // Test 11 (Issue #1207): Completing unpaid order with shared PIN requires payment permission during pre-verification + // Seed two managers sharing PIN 5555: + // mgr-shared-nopay comes first alphabetically, but has payments.take denied + // mgr-shared-pay has default manager permissions including payments.take + db.prepare(` + INSERT INTO users (id, name, email, password, role, pin_hash, is_active, created_at, updated_at) + VALUES ('mgr-shared-nopay', 'Manager No Pay', 'mgrnopay@test.local', 'Pass1234!', 'manager', ?, 1, ?, ?) + `).run(bcrypt.hashSync('5555', 10), now(), now()); + db.prepare(` + INSERT INTO user_permission_overrides (user_id, permission_id, effect, updated_by, created_at, updated_at) + VALUES ('mgr-shared-nopay', 'payments.take', 'deny', 'mgr-1', ?, ?) + `).run(now(), now()); + + db.prepare(` + INSERT INTO users (id, name, email, password, role, pin_hash, is_active, created_at, updated_at) + VALUES ('mgr-shared-pay', 'Manager With Pay', 'mgrpay@test.local', 'Pass1234!', 'manager', ?, 1, ?, ?) + `).run(bcrypt.hashSync('5555', 10), now(), now()); + + const unpaidOrderRes = db.prepare(` + INSERT INTO orders (order_number, type, subtotal, tax_amount, total, status, created_at, updated_at) + VALUES ('ORD-UNPAID-001', 'dine_in', 100, 0, 100, 'pending', ?, ?) + `).run(now(), now()); + const unpaidOrderId = unpaidOrderRes.lastInsertRowid; + + db.prepare(` + INSERT INTO bills (order_id, bill_number, subtotal, tax_amount, total, paid_amount, balance, payment_status, created_at, updated_at) + VALUES (?, 'BILL-UNPAID-001', 100, 0, 100, 0, 100, 'unpaid', ?, ?) + `).run(unpaidOrderId, now(), now()); + + // Test 11a: Explicit approver who lacks payments.take fails with 403 + const res11a = await request(app) + .patch(`/api/orders/${unpaidOrderId}/status`) + .set(cashierAuth) + .send({ status: 'completed', override_pin: '5555', approver_id: 'mgr-shared-nopay' }); + assertEqual(res11a.status, 403, 'Explicit approver lacking payments.take is rejected with 403'); + resetOrderPinRateLimit(); + + // Test 11b: Fallback PIN scan skips ineligible mgr-shared-nopay and authorizes mgr-shared-pay + const res11b = await request(app) + .patch(`/api/orders/${unpaidOrderId}/status`) + .set(cashierAuth) + .send({ status: 'completed', override_pin: '5555' }); + assertEqual(res11b.status, 200, 'Fallback PIN scan authorizes eligible manager with payments.take'); + resetOrderPinRateLimit(); + + const completedOrder = db.prepare('SELECT status FROM orders WHERE id = ?').get(unpaidOrderId) as any; + assertEqual(completedOrder.status, 'completed', 'Order status updated to completed'); + + const auditLog11 = db.prepare(` + SELECT * FROM order_audit_log WHERE order_id = ? ORDER BY id DESC LIMIT 1 + `).get(unpaidOrderId) as any; + assert(!!auditLog11, 'Audit log entry created for completed unpaid order'); + assertEqual( + JSON.parse(auditLog11.details_json || '{}').approved_by, + 'mgr-shared-pay', + 'Audit log attributes approval to eligible mgr-shared-pay', + ); + const results = getResults(); if (results.failed > 0) { throw new Error(`${results.failed} test assertions failed`); From bd865dd7f2fa7a7c687d557c5a270df71fd742c6 Mon Sep 17 00:00:00 2001 From: khaira777 <777gurkirat@gmail.com> Date: Sun, 18 Oct 2026 14:12:14 -0400 Subject: [PATCH 2/2] no-mistakes(document): Document pre-verified payment permission eligibility and format changes --- .../authentication-and-authorization.md | 11 ++++++---- docs/reference/api.md | 4 +++- main/routes/orders.ts | 12 +++------- tests/manager-pin-verification.test.ts | 22 ++++++++++++++----- 4 files changed, 29 insertions(+), 20 deletions(-) diff --git a/docs/architecture/authentication-and-authorization.md b/docs/architecture/authentication-and-authorization.md index 855e82eb7..abe0680d2 100644 --- a/docs/architecture/authentication-and-authorization.md +++ b/docs/architecture/authentication-and-authorization.md @@ -135,10 +135,13 @@ deliberate context-policy role check, not a permission, because the override PIN person acting in an owner/manager capacity regardless of what their individual permission grants say. To prevent event loop starvation and transaction lock contention during CPU-intensive bcrypt evaluations, manager PINs are verified asynchronously via `verifyPinAsync` outside the transaction -(with client-IP rate limits checked first). The pre-verified approver is then atomically re-validated -inside `withTxn` against current SQLite state (active status, owner/manager role, required permissions, -and matching `pin_hash`). The check cannot be hoisted into middleware without changing the read the -decision is based on, and it is invisible to anything that inspects the route's middleware chain. +(with client-IP rate limits checked first). Candidate filtering during pre-verification enforces the +role and permissions required by the transition (including `payments.take` when completion is +blocked by unsettled payment), ensuring fallback PIN scanning only matches eligible approvers. The +pre-verified approver is then atomically re-validated inside `withTxn` against current SQLite state +(active status, owner/manager role, required permissions, and matching `pin_hash`). The check cannot +be hoisted into middleware without changing the read the decision is based on, and it is invisible to +anything that inspects the route's middleware chain. **KDS WebSocket authorization.** The KDS socket authenticates with the same bearer token but authorizes per message. See below. diff --git a/docs/reference/api.md b/docs/reference/api.md index 68bb57f59..b25ab979c 100644 --- a/docs/reference/api.md +++ b/docs/reference/api.md @@ -329,7 +329,9 @@ one. Failures without a registered tuple may retain the `{ error }` response. separate operation via `/api/refunds`) and restores product, variant, add-on, and recipe stock only for items still in `pending` status. - When `approver_id` (or `manager_id`, `user_id`) is supplied on an overridden transition, PIN verification is - restricted to that active user without falling back to scanning other managers. Every overridden `status_changed` audit log entry records + restricted to that active user without falling back to scanning other managers. When omitted, candidate managers + are evaluated, skipping users who lack the required role or permissions (including `payments.take` when completion + is blocked by unsettled payment). Every overridden `status_changed` audit log entry records `actor_user_id`, `approved_by`, `overridden_safeguards`, `bill_ids`, `outstanding_amount`, `outstanding_amounts`, and `collected_amount` (plus `undelivered_items` when overriding kitchen delivery), and never records the PIN. diff --git a/main/routes/orders.ts b/main/routes/orders.ts index c32c69759..0ee83de4d 100644 --- a/main/routes/orders.ts +++ b/main/routes/orders.ts @@ -2406,9 +2406,7 @@ function evaluateOrderStatusPaymentBlock( : 0; const isSettled = !isPayable || - (paymentStatus === 'paid' && - unpaidBalanceMinor === 0 && - recordedPaidMinor >= totalMinor); + (paymentStatus === 'paid' && unpaidBalanceMinor === 0 && recordedPaidMinor >= totalMinor); return { id: b.id, paymentStatus, @@ -2438,10 +2436,7 @@ function evaluateOrderStatusPaymentBlock( : Math.max(0, effectiveOrderTargetMinor - billedCoverageMinor); const totalOutstandingMinor = payableBills.reduce((sum, b) => sum + b.unpaidBalanceMinor, 0) + unbilledMinor; - const totalCollectedMinor = evaluatedBills.reduce( - (sum, b) => sum + b.effectiveCollectedMinor, - 0, - ); + const totalCollectedMinor = evaluatedBills.reduce((sum, b) => sum + b.effectiveCollectedMinor, 0); const paymentBlocksCompletion = totalOutstandingMinor > 0 || @@ -2626,8 +2621,7 @@ router.patch( currentOrder.total, currentOrder.round_off, ); - const paymentBlocksCompletion = - status === 'completed' && rawPaymentBlocksCompletion; + const paymentBlocksCompletion = status === 'completed' && rawPaymentBlocksCompletion; const requireKitchenDelivered = db .prepare( diff --git a/tests/manager-pin-verification.test.ts b/tests/manager-pin-verification.test.ts index e8b6f8b76..9c86cd495 100644 --- a/tests/manager-pin-verification.test.ts +++ b/tests/manager-pin-verification.test.ts @@ -334,10 +334,12 @@ async function run() { VALUES ('mgr-shared-pay', 'Manager With Pay', 'mgrpay@test.local', 'Pass1234!', 'manager', ?, 1, ?, ?) `).run(bcrypt.hashSync('5555', 10), now(), now()); - const unpaidOrderRes = db.prepare(` + const unpaidOrderRes = db + .prepare(` INSERT INTO orders (order_number, type, subtotal, tax_amount, total, status, created_at, updated_at) VALUES ('ORD-UNPAID-001', 'dine_in', 100, 0, 100, 'pending', ?, ?) - `).run(now(), now()); + `) + .run(now(), now()); const unpaidOrderId = unpaidOrderRes.lastInsertRowid; db.prepare(` @@ -358,15 +360,23 @@ async function run() { .patch(`/api/orders/${unpaidOrderId}/status`) .set(cashierAuth) .send({ status: 'completed', override_pin: '5555' }); - assertEqual(res11b.status, 200, 'Fallback PIN scan authorizes eligible manager with payments.take'); + assertEqual( + res11b.status, + 200, + 'Fallback PIN scan authorizes eligible manager with payments.take', + ); resetOrderPinRateLimit(); - const completedOrder = db.prepare('SELECT status FROM orders WHERE id = ?').get(unpaidOrderId) as any; + const completedOrder = db + .prepare('SELECT status FROM orders WHERE id = ?') + .get(unpaidOrderId) as any; assertEqual(completedOrder.status, 'completed', 'Order status updated to completed'); - const auditLog11 = db.prepare(` + const auditLog11 = db + .prepare(` SELECT * FROM order_audit_log WHERE order_id = ? ORDER BY id DESC LIMIT 1 - `).get(unpaidOrderId) as any; + `) + .get(unpaidOrderId) as any; assert(!!auditLog11, 'Audit log entry created for completed unpaid order'); assertEqual( JSON.parse(auditLog11.details_json || '{}').approved_by,