fix(main): enforce payment permission eligibility in pre-verified order status completion (#1207)

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.

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.

main/routes/orders.ts

@@ -2302,23 +2302,69 @@ router.post(
   },
 );
 
-interface OrderStatusBillEvaluationRow {
-  id?: number | string;
-  total?: number | string | null;
-  round_off?: number | string | null;
-  paid_amount?: number | string | null;
-  refunded_cents?: number | string | null;
-  balance?: number | string | null;
-  payment_status?: string | null;
-  payment_details?: unknown;
-}
+function evaluateOrderStatusPaymentBlock(
+  db: ReturnType<typeof getDatabase>,
+  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));
 
-function evaluateOrderPaymentCompletion(
-  order: { total?: number | string | null; round_off?: number | string | null } | null | undefined,
-  bills: OrderStatusBillEvaluationRow[],
-  minorFactor = 100,
-) {
-  const evaluatedBills = bills.map((b) => {
+  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);
@@ -2378,8 +2424,8 @@ function evaluateOrderPaymentCompletion(
     };
   });
 
-  const orderTotalMinor = Math.max(0, Math.round(Number(order?.total || 0) * minorFactor));
-  const orderRoundOffMinor = Math.round(Number(order?.round_off || 0) * minorFactor);
+  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);
@@ -2395,22 +2441,20 @@ function evaluateOrderPaymentCompletion(
     payableBills.reduce((sum, b) => sum + b.unpaidBalanceMinor, 0) + unbilledMinor;
   const totalCollectedMinor = evaluatedBills.reduce((sum, b) => sum + b.effectiveCollectedMinor, 0);
 
-  const requiresPaymentOverride =
+  const paymentBlocksCompletion =
     totalOutstandingMinor > 0 ||
     (orderTotalMinor > 0 && payableBills.length === 0) ||
     payableBills.some((b) => !b.isSettled);
 
   return {
+    minorFactor,
+    roundCurrency,
     evaluatedBills,
-    payableBills,
     orderTotalMinor,
-    orderRoundOffMinor,
-    orderUnroundedTotalMinor,
-    effectiveOrderTargetMinor,
-    unbilledMinor,
+    payableBills,
     totalOutstandingMinor,
     totalCollectedMinor,
-    requiresPaymentOverride,
+    paymentBlocksCompletion,
   };
 }
 
@@ -2455,6 +2499,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
@@ -2473,7 +2526,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 = {
@@ -2555,41 +2609,22 @@ 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 {
+          minorFactor,
+          roundCurrency,
           evaluatedBills,
-          payableBills,
           orderTotalMinor,
+          payableBills,
           totalOutstandingMinor,
           totalCollectedMinor,
-          requiresPaymentOverride,
-        } = evaluateOrderPaymentCompletion(currentOrder, billRows, minorFactor);
-
-        const paymentBlocksCompletion = status === 'completed' && requiresPaymentOverride;
+          paymentBlocksCompletion: rawPaymentBlocksCompletion,
+        } = evaluateOrderStatusPaymentBlock(
+          db,
+          req.params.id,
+          currentOrder.total,
+          currentOrder.round_off,
+        );
+        const paymentBlocksCompletion = status === 'completed' && rawPaymentBlocksCompletion;
 
         const requireKitchenDelivered = db
           .prepare(

tests/manager-pin-verification.test.ts

@@ -316,6 +316,74 @@ async function run() {
     'PIN-only void records matching manager 1 in approved_by',
   );
 
+  // Test 11: 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`);