search

Sylius / ShopApiPlugin

Shop API for Sylius.
https://sylius.com
129 stars 89 forks source link

CompleteOrderRequest should check for customer object #598

Closed kayue closed 5 years ago

kayue commented 5 years ago

In the CompleteOrderAction, the email field is optional, and when checkout as a guest and no email is provided, the customer object won't be assigned to the cart.

When customer object is missing it will cause problems later on CustomerOrderAddressesSaver

kayue commented 5 years ago

The CompleteOrderAction doesn't perform any validation, this behavior should be change. We should perform inventory availability and order details check in this step.

https://github.com/Sylius/ShopApiPlugin/blob/master/src/Controller/Checkout/CompleteOrderAction.php#L49

mamazu commented 5 years ago

The thing with the email address on checkout is a little more complicated. There has been a merge request to fix this behaviour as far as I know. https://github.com/Sylius/ShopApiPlugin/pull/365#issue-242013792 That was the issue.

And about the availability, yes this should absolutely be validated.

kayue commented 5 years ago

That PR was merged a year ago, so I believe it doesn't cover my scenario, which is:

The fix can be simple, just make email field a required field if customer object does not exist in the complete order step. Merging #599 would be the first step.

mamazu commented 5 years ago

The thing should be fixed and covered by tests. The reason the email address is not required is the following. If the customer is logged in then the email is taken from the logged in user (and therefore not required). The issue we faced was that a customer could try to checkout as guest and then in the last step fill in the email of an existing customer and order in their name (which is obviously a huge security issue). See this test: https://github.com/Sylius/ShopApiPlugin/blob/c476d1d2a7584c41eaffdc0e4b8c537a5491586b/tests/Controller/Checkout/CompleteOrderApiTest.php#L215

For your case we indeed don't have a test but when given the email then the AssignCustomerToCartHandler should handle the creation of the user as the CustomerProvider does it.

kayue commented 5 years ago

I understand everything you said and I know why it wasn't required.

You are correct when email is given then everything is fine. We just need to handle the case where no email is given, a proper error message should return. Thank you for your time.