Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Currently the order refresh functionality doesn't care if an order is locked due to being in the payment step, meaning a locked order could still be refreshed which would be catastrophic if a customer is in the process of completing his payment in an offsite payment method, but there are other situations where this could lead to similar problems as well.
How to reproduce such a situation on a default Commerce install:
1) Be a user with administrative access to the order admin.
2) You add a product costing X to your cart and proceed to the offsite payment terminal, but you don't complete the payment yet.
3) In the meantime the product's price has now changed to Y, maybe from a promotion ending or due to someone changing it.
4) You open a new browser tab where you go to the order admin and look at your cart order. This triggers a refresh which updates the product price for the product in your cart from X to Y and thus also changes the order total.
5) Now you return to your original browser tab and complete your payment.
6) This causes the amount registered in the payment terminal to be X, but the amount registered in Drupal both as the order total and on the payment transaction to be Y, which would be really bad.
This is obviously a bit convoluted, but if you change the order refresh mode for your order type(s) to "Refresh a draft order when it is loaded regardless of who it belongs to." then this is infinitely easier to reproduce and could be reproduced for any user just by an administrator looking at a cart, taking out a report or whatever else.
We encountered something similar to this in a production setting which is how we discovered this in the first place. The situation we encountered involved the customer never returning to the Drupal site after completing his payment in the offsite payment terminal, due to en error either on our end or on the other end, and thus the order was never placed and due to this payment method creating the transaction in the onReturn() method this was never done either. The order was left in the draft state, locked, in the payment checkout step and with no payment transaction for a long time before the client reported it to us. Normally we can just fix these orders manually, but during this time something had triggered a refresh for this order and the order total was no longer the same as the amount registered in the payment terminal so the payment could no longer be captured.
Either way this is reproducible, can lead to serious problems, and the way I see it there are no disadvantages to just bailing out of the order refresh code if the loaded order is locked.
Patch to follow.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | locked_orders_should-3047357-9.patch | 1.36 KB | mglaman |
| |||
| #6 | skip_refresh_when_locked_merge_needsrefresh_and_should_refresh-3047357-6.patch | 4.06 KB | davenelson |
| |||
| #2 | skip_refresh_of_locked_orders-3047357-2.patch | 603 bytes | TwiiK |
| |||
Comments
Comment #2
TwiiK CreditAttribution: TwiiK at Ny Media AS commentedPatch attached.
Also, while looking at this code I don't understand why the methods to determine if an order should be refreshed or not are split between shouldRefresh() and needsRefresh(), they both seem to do the same thing and one calls the other and none of them are used separately in any other capacity. I can see the comment explaining why shouldRefresh() is there, but I don't agree, especially now that I've added another check to check if an order is locked to needsRefresh(). I feel the additional check in shouldRefresh() could just as well be merged into needsRefresh() and the shouldRefresh() method removed altogether. I could include such a change in my patch, but I feel it should be discussed first. It would probably also lead to a few tests needed to be updated which I'm not proficient with, but I could give it a stab.
Comment #3
TwiiK CreditAttribution: TwiiK at Ny Media AS commentedComment #4
bojanz CreditAttribution: bojanz at Centarro commentedI agree. The split feels like a historical oddity. It's also odd that $order_type is loaded in both methods, guessing that happened later. Not sure how easy it will be to merge them though, since the flow is opposite (allow if VS deny if)
Comment #5
davenelson CreditAttribution: davenelson at Acro Commerce commentedHey there. I'm at the Drupalcon sprint and can take a crack at merging the needsRefresh() and shouldRefresh() methods.
I just want to confirm I'm properly following the discussion, after merging the two methods needsRefresh() would also do the "Ensure the order is only refreshed for its customer, when configured so." check. In other words, when an order type's refresh mode is `REFRESH_CUSTOMER`, the order can only be refreshed when loaded by its customer and never need a refresh when loaded by another user.
To accomplish this could we just add the check happening in shouldRefresh() after the new check for if the order is locked? eg bail if the customer ID doesn't match the current user ID?
A thought on removing the shouldRefresh() method - rather than removing the method altogether, would it make sense to deprecate it and have it call needsRefresh()? I'm new to Commerce 2 and not sure how likely a call to shouldRefresh() would be in contrib or custom projects, but deprecating the method would help with keeping things backwards compatible.
Comment #6
davenelson CreditAttribution: davenelson at Acro Commerce commentedPatch attached for merging needsRefresh and shouldRefresh
testNeedsRefresh() has been updated to include the assertions from shouldRefresh and run the existing assertions in REFRESH_ALWAYS refresh mode to get past the new check against current user. Also added an assertion for the locked order scenario.
Comment #7
TwiiK CreditAttribution: TwiiK at Ny Media AS commentedI haven't tried the patch in #6, but just wanted to say that we've been running with the patch in #2 on all our sites ever since I made the original issue, but for some reason the composer patcher failed to apply the patch on a site for me just now and immediately I ran into issues due to the order processor processing old locked draft orders.
Debugging the issues led me back around to this issue again... :p
Comment #8
mglamanI'll be giving this a review tomorrow to commit.
Comment #9
mglamanDiscussed with @jsacksick and we want to avoid messing with
needsRefreshandshouldRefresh. We should stick with patch #2.Even if an order needs to be refreshed, this allows someone to decorate the order refresh service and prevent refresh. This stems from requests in 1.x to allow preventing order refresh with custom logic.
This is more explicit. "Shoulds" invokes "Needs" to determine if it should be. But someone could decorate the service and method to add additional logic.
Comment #10
mglamanHere is the patch in #2 with rests.
Comment #12
mglaman🥳 Committed. Thanks, everyone.