commerce_checkout_access() blocks anonymous checkout [#1362412]

I'm referring to this issue: #1350264: Completion message problem with anonymous users only

How to recreate this issue?

1. Install kickstart (or a vanilla DC)
2. Install the commerce_paypal module
3. Add paypal as a payment method to the checkout process and configure a paypal sandbox
4. Checkout a cart with a product as an anonymous user.
5. The checkout process will take you to the paypal site
6. Paypal redirects you back to 'checkout/ID/payment/return/HASH'
7. Notice how you get a "Page not found" error.

So, what goes wrong here? The problem can be tracked down to commerce_checkout_access().

Breakdown:

  // First, if this order doesn't belong to the account return FALSE.
  if ($account->uid) {
    if ($account->uid != $order->uid) {
      return FALSE;
    }
  }

This part of the code seems logical (Alltough I'd rather do an if (isset($account->uid)) to make sure anonymous (0) isn't confused with a FALSE) Then again: as an anonymous user one should fall through this part of the code anyway at this point.

 elseif (empty($_SESSION['commerce_cart_completed_orders']) ||
    !in_array($order->order_id, $_SESSION['commerce_cart_completed_orders'])) {
    // Return FALSE if the order does have a uid.
    if ($order->uid) {
      return FALSE;
    }

    // And then return FALSE if the anonymous user's session doesn't specify
    // this order ID.
    if (empty($_SESSION['commerce_cart_orders']) || !in_array($order->order_id, $_SESSION['commerce_cart_orders'])) {
      return FALSE;
    }
  }

Here's were things go wrong. By default, commerce comes with a few rules which will create and associate a user account with orders checked out by anonymous users. An order doesn't necessary end up in the 'commerce_cart_completed_orders' session array: the order might need to go through some sort of post processing and such. In case of the PayPal module: the order id is registered in 'commerce_cart_orders'. The problem are these three lines:

    // Return FALSE if the order does have a uid.
    if ($order->uid) {
      return FALSE;
    }

So, when the order is assigned to an account, but hasn't been processed fully by a payment method for whatever reason, and you are checking it out as an anonomyous user: you'll end up on an 'page not found' error... which is actually a disguised 403 page (safety through obfuscation).

How to solve this?

Well, on the project I'm working on, merely disabling the "Assign an anonymous order to a pre-existing user" and "Create a new account for an anonymous order" sufficed to get PayPal working. But, I'm sure there are enough use cases where registering a user account based on the emailaddress is a must.

I've seen #946954: Make commerce_checkout_access() extensible, so I know this part of the commerce code is susceptible to a lot of change. Guess there is no easy fix atm?

Comment	File	Size	Author
#6	1362412.checkout_access.patch	783 bytes	rszrama
#6

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

svendecabooter

Dutch

Gent

CreditAttribution: svendecabooter commented 2 January 2012 at 15:19

Category:

task

» bug

I'm encountering the same problem with the Commerce Ogone module when "Direct HTTP server-to-server request" is enabled.
This is pretty annoying behavior.

Comment #2

davidwatson CreditAttribution: davidwatson commented 17 January 2012 at 22:19

Just submitted a patch in #946954: Make commerce_checkout_access() extensible not too long ago; other modules like Commerce PayPal or Commerce Ogone could theoretically use the hooks provided to explicitly allow or deny access. I'm hoping the sooner we address that issue the sooner we can address this one for good, so any feedback on the patch would be much appreciated!

Comment #3

Tommy Kaneko CreditAttribution: Tommy Kaneko commented 24 January 2012 at 11:30

Using the patch on #946954: Make commerce_checkout_access() extensible (comment #4), I have created a patch for PayPal WPS to get over the anonymous checkout problem:
#1350264: Completion message problem with anonymous users only

Comment #4

davidwatson CreditAttribution: davidwatson commented 25 January 2012 at 02:59

Status:

Active

» Closed (duplicate)

Marking this as a duplicate of the other two issues named above, as that's where the actual work seems to be happening.

Comment #5

rszrama CreditAttribution: rszrama commented 25 January 2012 at 17:31

Status:

Closed (duplicate)

» Fixed

Let's leave this open, as checkout router extensibility represents a new feature while this is more of a bug in existing behavior. Also, I can't reproduce this locally ... because it depends on the receipt of payment notification. Naturally. : P

What happens is the IPN comes back from PayPal before the customer returns and triggers the checkout completion rules (this is a recent change as of Drupal Commerce 1.1). We have to do this to cover the cases where the customer doesn't return (PayPal doesn't do it automatically and gives a couple other links to distract the customer). However, looking over the function you've called out, I don't actually see the point in bailing out of the elseif if the order belongs to a user. We've already confirmed the order is represented in the user's session, so it shouldn't matter how the order has been updated since then... we know that user originated the order, so it's safe to show them the completion page.

So, I'm just going to remove that code:

 elseif (empty($_SESSION['commerce_cart_completed_orders']) ||
    !in_array($order->order_id, $_SESSION['commerce_cart_completed_orders'])) {
    // Return FALSE if the order does have a uid.
    if ($order->uid) {
      return FALSE;
    }

    // And then return FALSE if the anonymous user's session doesn't specify
    // this order ID.
    if (empty($_SESSION['commerce_cart_orders']) || !in_array($order->order_id, $_SESSION['commerce_cart_orders'])) {
      return FALSE;
    }
  }

will become:

 elseif (empty($_SESSION['commerce_cart_completed_orders']) ||
    !in_array($order->order_id, $_SESSION['commerce_cart_completed_orders'])) {
    // And then return FALSE if the anonymous user's session doesn't specify
    // this order ID.
    if (empty($_SESSION['commerce_cart_orders']) || !in_array($order->order_id, $_SESSION['commerce_cart_orders'])) {
      return FALSE;
    }
  }

I can't see any problems here; that was a duplicate check, because if an anonymous user got here and the cart in question wasn't in their cart orders array, they'd get a FALSE anyways. If it was in their carts array and somehow got assigned to another user (that it wasn't supposed to), then there's a deeper issue at play that has nothing to do with this access check.

Comment #6

rszrama CreditAttribution: rszrama commented 25 January 2012 at 17:33

Status:

Fixed

» Needs review

File	Size
1362412.checkout_access.patch	783 bytes

Sorry, meant to attach the patch and review. ; )

Comment #7

davidwatson CreditAttribution: davidwatson commented 25 January 2012 at 19:09

Makes sense, Ryan; didn't mean to jump the gun. :]

Rationale and code changes (all 783 bytes) all look sane to me, I'll review this on a test site later so we can RTBC.

Comment #8

25 January 2012 at 20:03

Status:

Needs review

» Needs work

The last submitted patch, 1362412.checkout_access.patch, failed testing.

Comment #9

rszrama CreditAttribution: rszrama commented 25 January 2012 at 20:25

Status:

Needs work

» Needs review

#6: 1362412.checkout_access.patch queued for re-testing.

Comment #10

25 January 2012 at 20:28

Status:

Needs review

» Needs work

The last submitted patch, 1362412.checkout_access.patch, failed testing.

Comment #11

rszrama CreditAttribution: rszrama commented 25 January 2012 at 23:07

Status:

Needs work

» Fixed

Went ahead with this for inclusion in the 1.2 once I realized the tests were failing to apply because I already made the commit in conjunction with a commit to fix failing product tests.