Repository navigation
fix(auth): add explicit manager selection to approval UI and kitchen override binding - #1229
Merged
Merged
Conversation
…s-server failed login counter (#1221) - Centralize PIN rate limiting and concurrency serialization with withPinVerificationLock in security middleware - Re-check isPinRateLimited inside lock so concurrent bursts reject immediately with 429 without redundant bcrypt executions - Defer resetSuccessfulLogin in KDS and Server App until after role and permission validation so 403 Forbidden does not clear the failed-login counter - Add regression tests covering 403 failure counter preservation and concurrent PIN verification bursts
…in counter preservation
…IN after delivery"}
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…1222-1229-r1 # Conflicts: # main/routes/bills.ts
# Conflicts: # docs/architecture/order-lifecycle.md # main/routes/bills.ts # tests/manager-pin-verification.test.ts
…1222-1229-r1 # Conflicts: # main/routes/bills.ts # main/routes/orders.ts
# Conflicts: # docs/reference/api.md
…1222-1229-r1 # Conflicts: # docs/architecture/authentication-and-authorization.md # main/routes/orders.ts # tests/manager-pin-verification.test.ts
# Conflicts: # tests/manager-pin-verification.test.ts
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
fix(auth): add explicit manager selection to approval UI and kitchen override binding
What Changed
ApproverSelectcomponent anduseManagerApprovershook across POS payment, prepaid checkout, and order override dialogs to explicitly select an approving manager when multiple eligible managers exist.approver_id,manager_id,user_id), binding kitchen delivery override PIN verification directly to the selected active manager and logging the approval.Risk Assessment
🚨 High: The approver loading hook calls the privileged /api/staff endpoint requiring staff.view, which returns 403 Forbidden for cashiers and servers, breaking manager selection and showing an error state across POS and order approval flows.
Testing
Live validation executed 5 end-to-end scenarios against the running product via Playwright browser flows and backend integration tests; all scenarios passed with full visual and behavioral evidence, confirming explicit manager selection in approval modals, kitchen override binding, adversarial rejection of mismatched PINs, empty-state fallback, and unblocked settlement retries after item delivery.
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
frontend/src/hooks/useManagerApprovers.ts:29-useManagerApproversattempts to fetch eligible approvers viaGET /api/staff?active=true, which requiresrequirePermission('staff.view')(main/routes/staff.ts:114), granted by default only toOWNER_MANAGER(shared/permissions.ts:415). Cashiers and servers do not havestaff.view. When non-manager staff open any screen with manager approvals, the request fails with HTTP 403, settingloadFailed: trueand leavingapproversempty. This renders an error banner ("Could not load refund approvers. Try again.") and prevents the approver dropdown from ever displaying. Furthermore, becauseapproversis empty,effectiveApproverIdevaluates toundefined, silently strippingapprover_idfrom all actions performed by cashiers and servers. Sibling sites affected:frontend/src/components/pos/PaymentModal.tsx:185(kitchen override and discount for cashiers),frontend/src/components/pos/PrepaidCheckoutModal.tsx:139(prepaid checkout discount for cashiers),frontend/src/app/(dashboard)/orders/page.tsx:230(void and cancel approvals for servers/cashiers), andfrontend/src/components/ApproverSelect.tsx:42(renders error state for all non-managers). A policy decision is needed on how non-managers should retrieve eligible approvers (e.g. creating an unprivileged endpoint like/api/staff/approversexposing only non-sensitive active manager names and IDs, or alteringstaff.viewaccess policy); authorizing this API/policy extension requires user guidance.frontend/src/app/(dashboard)/orders/page.tsx:2088- Deselecting an approver in approval modals is prevented by the fallback patternlocalApproverId || managerApprovers.selectedApproverId. When the authenticated user is a manager,managerApprovers.selectedApproverIdis pre-populated with their user ID. PassingcancelModal.approverId || managerApprovers.selectedApproverIdas the controlledvalueof<ApproverSelect>causes choosing the empty placeholder option ("") to evaluate back tomanagerApprovers.selectedApproverId, resetting the dropdown selection. Additionally, the modal submit button disabled check!(cancelModal.approverId || managerApprovers.selectedApproverId)remains false, allowing submission with the preselected manager even when the user intended to clear the field. Sibling sites with the identical binding pattern:frontend/src/app/(dashboard)/orders/page.tsx:2164(void item modal),frontend/src/app/(dashboard)/orders/page.tsx:2348(discount modal),frontend/src/components/pos/PaymentModal.tsx:1493(kitchen override),frontend/src/components/pos/PaymentModal.tsx:1765(payment discount), andfrontend/src/components/pos/PrepaidCheckoutModal.tsx:785(prepaid discount).frontend/src/hooks/useManagerApprovers.ts:9- Simplification:useManagerApproversintroduces speculative flexibility withUseManagerApproversOptions(allowedRoles?: readonly Role[]). No caller in the codebase passes options touseManagerApprovers(); all rely onROLE_ACCESS.ownerManager. The user intent strictly requires manager selection for approvals and kitchen override binding, with no requirement for dynamic role-filtered querying. The unused options interface and memoization should be removed. Sibling sites with unneeded components:main/routes/bills.ts:3421andmain/routes/bills.ts:3511redundantly chain duplicate properties on the same object ((payment as any).approver_id || ... || req.body.approver_id ...wherepayment === req.body, andbody.approver_id || ... || req.body.approver_id ...wherebody === req.body).frontend/src/components/ApproverSelect.tsx:58-ApproverSelecthardcodes refund-specific localization keys (refundApprovalSetupStaff-> "Refund approval is not configured. Ask the owner to set an Approval PIN in Staff." andrefundApproverLoadFailed-> "Could not load refund approvers. Try again.") even though it is used for kitchen delivery overrides, order item voids, order cancellations, and bill discounts, none of which are refunds. Sibling site:frontend/src/components/ApproverSelect.tsx:48(refundApproverLoadFailed). The component should accept optional custom messages or use neutral authorization strings.🔧 Fix applied.
6 issues (2 errors, 2 warnings, 2 infos) still open:
frontend/src/hooks/useManagerApprovers.ts:29-useManagerApproversattempts to fetch eligible approvers viaGET /api/staff?active=true, which requiresrequirePermission('staff.view')(main/routes/staff.ts:114), granted by default only toOWNER_MANAGER(shared/permissions.ts:415). Cashiers and servers do not havestaff.view. When non-manager staff open any screen with manager approvals, the request fails with HTTP 403, settingloadFailed: trueand leavingapproversempty. This renders an error banner ("Could not load refund approvers. Try again.") and prevents the approver dropdown from ever displaying. Furthermore, becauseapproversis empty,effectiveApproverIdevaluates toundefined, silently strippingapprover_idfrom all actions performed by cashiers and servers. Sibling sites affected:frontend/src/components/pos/PaymentModal.tsx:185(kitchen override and discount for cashiers),frontend/src/components/pos/PrepaidCheckoutModal.tsx:139(prepaid checkout discount for cashiers),frontend/src/app/(dashboard)/orders/page.tsx:230(void and cancel approvals for servers/cashiers), andfrontend/src/components/ApproverSelect.tsx:42(renders error state for all non-managers). A policy decision is needed on how non-managers should retrieve eligible approvers (e.g. creating an unprivileged endpoint like/api/staff/approversexposing only non-sensitive active manager names and IDs, or alteringstaff.viewaccess policy); authorizing this API/policy extension requires user guidance.frontend/src/hooks/useManagerApprovers.ts:9- Simplification:useManagerApproversintroduces speculative flexibility withUseManagerApproversOptions(allowedRoles?: readonly Role[]). No caller in the codebase passes options touseManagerApprovers(); all rely onROLE_ACCESS.ownerManager. The user intent strictly requires manager selection for approvals and kitchen override binding, with no requirement for dynamic role-filtered querying. The unused options interface and memoization should be removed. Sibling sites with unneeded components:main/routes/bills.ts:3421andmain/routes/bills.ts:3511redundantly chain duplicate properties on the same object ((payment as any).approver_id || ... || req.body.approver_id ...wherepayment === req.body, andbody.approver_id || ... || req.body.approver_id ...wherebody === req.body).frontend/src/components/ApproverSelect.tsx:58-ApproverSelecthardcodes refund-specific localization keys (refundApprovalSetupStaff-> "Refund approval is not configured. Ask the owner to set an Approval PIN in Staff." andrefundApproverLoadFailed-> "Could not load refund approvers. Try again.") even though it is used for kitchen delivery overrides, order item voids, order cancellations, and bill discounts, none of which are refunds. Sibling site:frontend/src/components/ApproverSelect.tsx:48(refundApproverLoadFailed). The component should accept optional custom messages or use neutral authorization strings.frontend/src/hooks/useManagerApprovers.ts:29-useManagerApproversattempts to fetch eligible approvers viaGET /api/staff?active=true, which requiresrequirePermission('staff.view')(main/routes/staff.ts:114), granted by default only toOWNER_MANAGER(shared/permissions.ts:415). Cashiers and servers do not havestaff.view. When non-manager staff open any screen with manager approvals, the request fails with HTTP 403, settingloadFailed: trueand leavingapproversempty. This renders an error banner ("Could not load refund approvers. Try again.") and prevents the approver dropdown from ever displaying. Furthermore, becauseapproversis empty,effectiveApproverIdevaluates to"", silently omittingapprover_idfrom actions performed by cashiers and servers and falling back to any matching manager PIN. Sibling sites affected:frontend/src/components/pos/PaymentModal.tsx:187(kitchen override and discount for cashiers),frontend/src/components/pos/PrepaidCheckoutModal.tsx:139(prepaid checkout discount for cashiers),frontend/src/app/(dashboard)/orders/page.tsx:232(void and cancel approvals for servers/cashiers), andfrontend/src/components/ApproverSelect.tsx:47(renders error state for all non-managers). A policy decision is needed on how non-managers should retrieve eligible approvers (e.g. creating an unprivileged endpoint like/api/staff/approversexposing only non-sensitive active manager names and IDs, or alteringstaff.viewaccess policy); authorizing this API/policy extension requires user guidance.frontend/src/hooks/useManagerApprovers.ts:9- Simplification:useManagerApproversintroduces speculative flexibility withUseManagerApproversOptions(allowedRoles?: readonly Role[]). No caller in the codebase passes options touseManagerApprovers(); all rely onROLE_ACCESS.ownerManager. The user intent strictly requires manager selection for approvals and kitchen override binding, with no requirement for dynamic role-filtered querying. The unused options interface and memoization should be removed. Sibling sites with unneeded components:main/routes/bills.ts:3421andmain/routes/bills.ts:3511redundantly chain duplicate properties on the same object ((payment as any).approver_id || ... || req.body.approver_id ...wherepayment === req.body, andbody.approver_id || ... || req.body.approver_id ...wherebody === req.body), and accept unrequested fallback aliasesmanager_idanduser_id.frontend/src/components/ApproverSelect.tsx:52- In Round 1,AUTH-APPROVER-REFUND-COPYwas addressed by addingloadFailedMessageandemptyMessageprops toApproverSelect.tsx:17-18, but the fix left behind all consumer call sites without passing either prop. As a result, the default fallback stringsrefundApproverLoadFailed("Could not load refund approvers. Try again.") andrefundApprovalSetupStaff("Refund approval is not configured. Ask the owner to set an Approval PIN in Staff.") are still displayed for non-refund operations. Remaining sibling sites left behind:frontend/src/components/pos/PaymentModal.tsx:1490(kitchen override),frontend/src/components/pos/PaymentModal.tsx:1757(payment discount),frontend/src/components/pos/PrepaidCheckoutModal.tsx:778(prepaid checkout discount),frontend/src/app/(dashboard)/orders/page.tsx:2074(cancel order modal),frontend/src/app/(dashboard)/orders/page.tsx:2152(void item modal), andfrontend/src/app/(dashboard)/orders/page.tsx:2337(order discount modal). Either neutral strings should be used as defaults inApproverSelect.tsx:52andApproverSelect.tsx:62, or custom messages should be passed at each sibling site.🔧 Fix applied.
6 issues (3 errors, 3 warnings) still open:
frontend/src/hooks/useManagerApprovers.ts:29-useManagerApproversattempts to fetch eligible approvers viaGET /api/staff?active=true, which requiresrequirePermission('staff.view')(main/routes/staff.ts:114), granted by default only toOWNER_MANAGER(shared/permissions.ts:415). Cashiers and servers do not havestaff.view. When non-manager staff open any screen with manager approvals, the request fails with HTTP 403, settingloadFailed: trueand leavingapproversempty. This renders an error banner ("Could not load refund approvers. Try again.") and prevents the approver dropdown from ever displaying. Furthermore, becauseapproversis empty,effectiveApproverIdevaluates toundefined, silently strippingapprover_idfrom all actions performed by cashiers and servers. Sibling sites affected:frontend/src/components/pos/PaymentModal.tsx:185(kitchen override and discount for cashiers),frontend/src/components/pos/PrepaidCheckoutModal.tsx:139(prepaid checkout discount for cashiers),frontend/src/app/(dashboard)/orders/page.tsx:230(void and cancel approvals for servers/cashiers), andfrontend/src/components/ApproverSelect.tsx:42(renders error state for all non-managers). A policy decision is needed on how non-managers should retrieve eligible approvers (e.g. creating an unprivileged endpoint like/api/staff/approversexposing only non-sensitive active manager names and IDs, or alteringstaff.viewaccess policy); authorizing this API/policy extension requires user guidance.frontend/src/hooks/useManagerApprovers.ts:9- Simplification:useManagerApproversintroduces speculative flexibility withUseManagerApproversOptions(allowedRoles?: readonly Role[]). No caller in the codebase passes options touseManagerApprovers(); all rely onROLE_ACCESS.ownerManager. The user intent strictly requires manager selection for approvals and kitchen override binding, with no requirement for dynamic role-filtered querying. The unused options interface and memoization should be removed. Sibling sites with unneeded components:main/routes/bills.ts:3421andmain/routes/bills.ts:3511redundantly chain duplicate properties on the same object ((payment as any).approver_id || ... || req.body.approver_id ...wherepayment === req.body, andbody.approver_id || ... || req.body.approver_id ...wherebody === req.body).frontend/src/hooks/useManagerApprovers.ts:29-useManagerApproversattempts to fetch eligible approvers viaGET /api/staff?active=true, which requiresrequirePermission('staff.view')(main/routes/staff.ts:114), granted by default only toOWNER_MANAGER(shared/permissions.ts:415). Cashiers and servers do not havestaff.view. When non-manager staff open any screen with manager approvals, the request fails with HTTP 403, settingloadFailed: trueand leavingapproversempty. This renders an error banner ("Could not load refund approvers. Try again.") and prevents the approver dropdown from ever displaying. Furthermore, becauseapproversis empty,effectiveApproverIdevaluates to"", silently omittingapprover_idfrom actions performed by cashiers and servers and falling back to any matching manager PIN. Sibling sites affected:frontend/src/components/pos/PaymentModal.tsx:187(kitchen override and discount for cashiers),frontend/src/components/pos/PrepaidCheckoutModal.tsx:139(prepaid checkout discount for cashiers),frontend/src/app/(dashboard)/orders/page.tsx:232(void and cancel approvals for servers/cashiers), andfrontend/src/components/ApproverSelect.tsx:47(renders error state for all non-managers). A policy decision is needed on how non-managers should retrieve eligible approvers (e.g. creating an unprivileged endpoint like/api/staff/approversexposing only non-sensitive active manager names and IDs, or alteringstaff.viewaccess policy); authorizing this API/policy extension requires user guidance.frontend/src/hooks/useManagerApprovers.ts:9- Simplification:useManagerApproversintroduces speculative flexibility withUseManagerApproversOptions(allowedRoles?: readonly Role[]). No caller in the codebase passes options touseManagerApprovers(); all rely onROLE_ACCESS.ownerManager. The user intent strictly requires manager selection for approvals and kitchen override binding, with no requirement for dynamic role-filtered querying. The unused options interface and memoization should be removed. Sibling sites with unneeded components:main/routes/bills.ts:3421andmain/routes/bills.ts:3511redundantly chain duplicate properties on the same object ((payment as any).approver_id || ... || req.body.approver_id ...wherepayment === req.body, andbody.approver_id || ... || req.body.approver_id ...wherebody === req.body), and accept unrequested fallback aliasesmanager_idanduser_id.frontend/src/hooks/useManagerApprovers.ts:29-useManagerApproversattempts to fetch eligible approvers viaGET /api/staff?active=true, which requiresrequirePermission('staff.view')(main/routes/staff.ts:114), granted by default only toOWNER_MANAGER(shared/permissions.ts:415). Cashiers and servers do not havestaff.view. When non-manager staff open any screen with manager approvals, the request fails with HTTP 403, settingloadFailed: trueand leavingapproversempty. This renders an error banner ("Could not load approvers. Try again.") and prevents the approver dropdown from ever displaying. Furthermore, becauseapproversis empty,effectiveApproverIdevaluates to"", silently omittingapprover_idfrom actions performed by cashiers and servers and falling back to any matching manager PIN on the backend. Sibling sites affected:frontend/src/components/pos/PaymentModal.tsx:187(kitchen override and discount for cashiers),frontend/src/components/pos/PrepaidCheckoutModal.tsx:139(prepaid checkout discount for cashiers),frontend/src/app/(dashboard)/orders/page.tsx:232(void and cancel approvals for servers/cashiers), andfrontend/src/components/ApproverSelect.tsx:47(renders error state for all non-managers). A policy decision is needed on how non-managers should retrieve eligible approvers (e.g. creating an unprivileged endpoint like/api/staff/approversexposing only non-sensitive active manager names and IDs, or alteringstaff.viewaccess policy); authorizing this API/policy extension requires user guidance.frontend/src/hooks/useManagerApprovers.ts:9- Simplification:useManagerApproversintroduces speculative flexibility withUseManagerApproversOptions(allowedRoles?: readonly Role[]). No caller in the codebase passes options touseManagerApprovers(); all rely onROLE_ACCESS.ownerManager. The user intent strictly requires manager selection for approvals and kitchen override binding, with no requirement for dynamic role-filtered querying. The unused options interface and memoization should be removed. Sibling sites with unneeded components:main/routes/bills.ts:3421andmain/routes/bills.ts:3511redundantly chain duplicate properties on the same object ((payment as any).approver_id || ... || req.body.approver_id ...wherepayment === req.body, andbody.approver_id || ... || req.body.approver_id ...wherebody === req.body), and accept unrequested fallback aliasesmanager_idanduser_id. Furthermore, acceptinguser_idas an alias forapprover_idcreates a concrete bug: if a request payload includes the acting cashier's or server'suser_id,explicitApproverIdcaptures the cashier's user ID, causingpreVerifyKitchenDeliveryPinto fail with HTTP 403 because the cashier does not have the manager/owner role, even when a valid manager override PIN is supplied. The unrequested aliases and duplicate object checks should be removed.frontend/src/components/pos/PaymentModal.tsx:2272-PaymentModaldisables the Pay button wheneverkitchenUndeliveredCount > 0andkitchenOverridePinis empty (kitchenUndeliveredCount > 0 && (!kitchenOverridePin || ...)). When kitchen items finish delivery while the payment modal remains open, cashiers are unable to retry payment without entering a manager PIN, causingfrontend/e2e/kitchen-delivery-payment-retry.spec.ts:86to fail. User decision is needed on whether cashiers should be required to provide a manager override PIN once items are flagged undelivered, or if the Pay button should remain enabled whenkitchenOverridePinis empty to allow delivery retries.frontend/e2e/approval-manager-selection.spec.ts- new test file written by agent: frontend/e2e/approval-manager-selection.spec.tsnode tests/run-electron-node-test.cjs tests/manager-pin-verification.test.tsnpm run test:kitchen-delivery-enforcementcd frontend && npx playwright test --config=playwright.config.ts --project=chromium e2e/approval-manager-selection.spec.tscd frontend && npx playwright test --config=playwright.config.ts --project=chromium -g "payment can retry without a manager PIN after kitchen delivery completes"🔧 Fix applied.
1 info still open:
frontend/e2e/approval-manager-selection.spec.ts- new test file written by agent: frontend/e2e/approval-manager-selection.spec.tsnode tests/run-electron-node-test.cjs tests/manager-pin-verification.test.tsnode tests/run-electron-node-test.cjs tests/kitchen-delivery-enforcement.test.tsnode tests/run-electron-node-test.cjs tests/manager-pin-rate-limit-bypass.test.tscd frontend && npx playwright test --config=playwright.config.ts --project=chromium e2e/approval-manager-selection.spec.tscd frontend && npx playwright test --config=playwright.config.ts --project=chromium e2e/kitchen-delivery-payment-retry.spec.ts✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.