-
Notifications
You must be signed in to change notification settings - Fork 196
Feature/thin develop split #360
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
MohamedAliSmk
wants to merge
24
commits into
develop
Choose a base branch
from
feature/thin-develop-split
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
d43f29c
Add Magento registry, boot-gated Vue adapters, and desk JS yield on d…
0b932f9
Register AuthorizationDialog in generated Vue component types.
c441209
Enhance POS components with conditional rendering and new features
7898316
Enhance stock validation and invoice handling in POS
3c3a745
Enhance InvoiceCart and posCart for improved free item handling
1e53275
feat: implement POS authorization gate system with action registry, v…
MostafaKadry 6e0b904
feat: add Item Price test protection and support for extensible POS o…
MostafaKadry 5d4c16f
Enhance customer creation and offer validation in POS
67c4b3c
fix: decide promotions ownership at call time, not import
9b69390
fix: keep POS payment GL in company currency via base_amount
aa5d4d0
Fixed. The wallet MoP cache no longer lives forever without invalidat…
9eac646
Moved _validate_pos_payment_accounts() to after super().validate().
3e44013
Done. `_resolve_pos_customer` now:
00e17f5
Done. Gift Pool / GWP no longer live in core cart.
3714e73
Pin same-SKU GWP carve/restore with a unit-tested helper.
8cebe05
Removed the unreachable block. Behavior is unchanged: leaves return i…
a81cb0c
after_customer_insert now logs and continues on hook failure, matchin…
17d4137
Make split smoke tests take --site and fix apps path bootstrap order.
37564f1
Rewrote test_split_smoke.py so it fails when the seam is broken, not …
626acef
IntegrationTestCase isn’t available in this Frappe version (frappe.te…
a7b2bdd
refactor(auth): fix authorization flow and remove app duplication
MostafaKadry cd8172f
Rate limit: 20/min, scoped to IP + approver instead of IP only.
MostafaKadry 479d06a
Authorization gate: Now only applies to POS documents and skips inter…
MostafaKadry e55cf49
Merge branch 'develop' into feature/thin-develop-split
MostafaKadry File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,205 @@ | ||
| <template> | ||
| <div | ||
| v-if="state.open" | ||
| class="pointer-events-auto fixed inset-0 z-[var(--z-authorization)] flex items-center justify-center bg-black/50 p-4" | ||
| @click.self="onCancel" | ||
| @pointerdown.stop | ||
| > | ||
| <FocusScope trapped as-child> | ||
| <div class="w-full max-w-sm rounded-xl bg-white shadow-xl dark:bg-gray-800"> | ||
| <div class="border-b border-gray-200 px-5 py-4 dark:border-gray-700"> | ||
| <h3 class="text-lg font-semibold text-gray-900 dark:text-gray-100"> | ||
| {{ __("Authorization Required") }} | ||
| </h3> | ||
| <p class="mt-1 text-sm text-gray-500 dark:text-gray-400"> | ||
| {{ __("A manager must approve this action.") }} | ||
| </p> | ||
| </div> | ||
|
|
||
| <dl | ||
| v-if="summaryRows.length" | ||
| class="grid grid-cols-[auto_1fr] gap-x-4 gap-y-1 border-b border-gray-200 bg-gray-50 px-5 py-3 text-sm dark:border-gray-700 dark:bg-gray-900/40" | ||
| > | ||
| <template v-for="row in summaryRows" :key="row.key"> | ||
| <dt class="text-gray-500 dark:text-gray-400">{{ row.label }}</dt> | ||
| <dd | ||
| class="text-end font-medium text-gray-900 break-all dark:text-gray-100" | ||
| :data-test="`auth-summary-${row.key}`" | ||
| > | ||
| {{ row.value }} | ||
| </dd> | ||
| </template> | ||
| </dl> | ||
|
|
||
| <div class="space-y-4 px-5 py-4"> | ||
| <div v-if="loading" class="py-6 text-center text-sm text-gray-500"> | ||
| {{ __("Loading approvers…") }} | ||
| </div> | ||
|
|
||
| <div | ||
| v-else-if="!authorizers.length" | ||
| class="rounded-lg bg-amber-50 p-3 text-sm text-amber-800 dark:bg-amber-900/30 dark:text-amber-200" | ||
| > | ||
| {{ | ||
| __( | ||
| "No approver is available. Ask a System Manager to set an authorization PIN for a manager." | ||
| ) | ||
| }} | ||
| </div> | ||
|
|
||
| <template v-else> | ||
| <div> | ||
| <label | ||
| class="mb-1 block text-sm font-medium text-gray-700 dark:text-gray-300" | ||
| > | ||
| {{ __("Approver") }} | ||
| </label> | ||
| <select | ||
| v-model="approver" | ||
| class="w-full rounded-lg border border-gray-300 px-3 py-2 text-sm dark:border-gray-600 dark:bg-gray-700 dark:text-gray-100" | ||
| > | ||
| <option | ||
| v-for="person in authorizers" | ||
| :key="person.user" | ||
| :value="person.user" | ||
| > | ||
| {{ person.full_name || person.user }} | ||
| </option> | ||
| </select> | ||
| </div> | ||
|
|
||
| <div> | ||
| <label | ||
| class="mb-1 block text-sm font-medium text-gray-700 dark:text-gray-300" | ||
| > | ||
| {{ __("PIN") }} | ||
| </label> | ||
| <input | ||
| ref="pinInput" | ||
| v-model="pin" | ||
| type="password" | ||
| inputmode="numeric" | ||
| autocomplete="off" | ||
| :maxlength="pinLength" | ||
| :placeholder="__('{0}-digit PIN', [pinLength])" | ||
| class="w-full rounded-lg border px-3 py-2 text-center text-2xl tracking-[0.5em] dark:bg-gray-700 dark:text-gray-100" | ||
| :class=" | ||
| errorMessage | ||
| ? 'border-red-500' | ||
| : 'border-gray-300 dark:border-gray-600' | ||
| " | ||
| @keyup.enter="onApprove" | ||
| /> | ||
| <p v-if="errorMessage" class="mt-1.5 text-sm text-red-600"> | ||
| {{ errorMessage }} | ||
| </p> | ||
| </div> | ||
| </template> | ||
| </div> | ||
|
|
||
| <div | ||
| class="flex justify-end gap-2 border-t border-gray-200 px-5 py-3 dark:border-gray-700" | ||
| > | ||
| <button | ||
| type="button" | ||
| class="rounded-lg px-4 py-2 text-sm font-medium text-gray-700 hover:bg-gray-100 dark:text-gray-300 dark:hover:bg-gray-700" | ||
| @click="onCancel" | ||
| > | ||
| {{ __("Cancel") }} | ||
| </button> | ||
| <button | ||
| type="button" | ||
| :disabled="!canApprove" | ||
| class="rounded-lg bg-blue-600 px-4 py-2 text-sm font-medium text-white hover:bg-blue-700 disabled:cursor-not-allowed disabled:opacity-50" | ||
| @click="onApprove" | ||
| > | ||
| {{ verifying ? __("Verifying…") : __("Approve") }} | ||
| </button> | ||
| </div> | ||
| </div> | ||
| </FocusScope> | ||
| </div> | ||
| </template> | ||
|
|
||
| <script setup> | ||
| import { useAuthorizationDialog } from "@/composables/useAuthorization"; | ||
| import { isRateLimitError } from "@/utils/authorizationError"; | ||
| import { buildAuthorizationSummary } from "@/utils/authorizationSummary"; | ||
| import { computed, nextTick, ref, watch } from "vue"; | ||
| import { FocusScope } from "reka-ui"; | ||
|
|
||
| const { | ||
| state, | ||
| loadAuthorizers, | ||
| requestGrant, | ||
| pinLength: getPinLength, | ||
| approve, | ||
| cancel, | ||
| } = useAuthorizationDialog(); | ||
|
|
||
| const authorizers = ref([]); | ||
| const approver = ref(""); | ||
| const pin = ref(""); | ||
| const errorMessage = ref(""); | ||
| const loading = ref(false); | ||
| const verifying = ref(false); | ||
| const pinInput = ref(null); | ||
| const pinLength = ref(getPinLength()); | ||
| const summaryRows = computed(() => buildAuthorizationSummary(state)); | ||
|
|
||
| const canApprove = computed( | ||
| () => Boolean(approver.value) && pin.value.length === pinLength.value && !verifying.value | ||
| ); | ||
|
|
||
| watch( | ||
| () => state.open, | ||
| async (open) => { | ||
| if (!open) return; | ||
|
|
||
| authorizers.value = []; | ||
| approver.value = ""; | ||
| pin.value = ""; | ||
| pinLength.value = getPinLength(); | ||
| errorMessage.value = ""; | ||
| loading.value = true; | ||
|
|
||
| authorizers.value = await loadAuthorizers(); | ||
| if (authorizers.value.length) { | ||
| approver.value = authorizers.value[0].user; | ||
| } | ||
| loading.value = false; | ||
|
|
||
| await nextTick(); | ||
| pinInput.value?.focus(); | ||
| } | ||
| ); | ||
|
|
||
| async function onApprove() { | ||
| if (!canApprove.value) return; | ||
|
|
||
| verifying.value = true; | ||
| errorMessage.value = ""; | ||
|
|
||
| try { | ||
| const result = await requestGrant(approver.value, pin.value); | ||
| if (result?.authorized) { | ||
| approve(result); | ||
| return; | ||
| } | ||
| errorMessage.value = result?.message || __("Authorization failed"); | ||
| } catch (error) { | ||
| errorMessage.value = isRateLimitError(error) | ||
| ? __("Too many PIN attempts just now. Wait a moment and try again.") | ||
| : error?.message || __("Authorization failed"); | ||
| } finally { | ||
| verifying.value = false; | ||
| pin.value = ""; | ||
| await nextTick(); | ||
| pinInput.value?.focus(); | ||
| } | ||
| } | ||
|
|
||
| function onCancel() { | ||
| cancel(); | ||
| } | ||
| </script> | ||
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High — the approver is never shown what they are approving.
This dialog renders "A manager must approve this action.", an approver picker and a PIN box. It never shows the action name, the amount, the customer or the invoice.
state.actionLabelis captured inuseAuthorization.jsand never rendered anywhere.The refund amount is supplied by the client (
ReturnInvoiceDialog.vue, in therequireAuthorizationcontext) and becomes the grant's binding ceiling on the server. So a manager who walks to a till and types a PIN has no way to tell whether they are approving a 12.00 refund or a 12,000.00 one — which is the one thing this control exists to prevent.Render the action, the amount and
return_againstin the dialog body.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MostafaKadry
please review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@engahmed1190 I think this is more of a UX improvement than a security/correctness issue in our current flow.
The authorization dialog is not shown to the manager on a separate device or in isolation. The cashier calls the manager over to the cashier's terminal, where the full invoice and its details are already visible. The customer is also physically present with the printed invoice, which is required to process the return, so the manager can verify the printed invoice against the invoice shown on the cashier's screen before approving.
The Auth Required dialog is simply the final approval step after those checks have already been performed.
This flow is also exactly what the business team presented to the customer, and it matches the current customer's agreed requirements. So I would not classify the absence of these details in the authorization dialog as a critical security gap. I agree that showing the action, amount, and return reference could be a useful UX improvement, but I don't think it should be treated as a high-severity issue for this flow.