Make commerce_checkout_access() extensible

Subscribe

Been working on this for a client while attempting to sort out #1362412: commerce_checkout_access() blocks anonymous checkout. Above patch doesn't seem to apply to the latest dev cleanly, but the approach used in node_access seems to make the most sense here:

  // We grant access to the node if both of the following conditions are met:
  // - No modules say to deny access.
  // - At least one module says to grant access.
  // If no module specified either allow or deny, we fall back to the
  // node_access table.
  $access = module_invoke_all('node_access', $node, $op, $account);
  if (in_array(NODE_ACCESS_DENY, $access, TRUE)) {
    $rights[$account->uid][$cid][$op] = FALSE;
    return FALSE;
  }
  elseif (in_array(NODE_ACCESS_ALLOW, $access, TRUE)) {
    $rights[$account->uid][$cid][$op] = TRUE;
    return TRUE;
  }
  
  // (If no one else says yea or nay, more node_access() $stuff...)

This means we'll probably want constants for COMMERCE_CHECKOUT_ACCESS_ALLOW/DENY to distinguish between a hook implementation explicitly allowing/denying access and the absence of a response (in which case the default logic should take hold, with any DENY taking precedence in the event of conflicting implementations).

Time permitting I'll whip up another patch, but does this sound like a reasonable approach?

Patch operates much in the spirit of #1, though unless I'm mistaken it seems like we really want two hooks here: hook_commerce_checkout_access() to extend order-level access and hook_commerce_checkout_page_access() to extend page-level access. Both require implementors to use the same ALLOW/DENY constants. Review and feedback welcome.

I have made a patch for PayPal WPS using the above patch (comment #4).

See: #1350264: Completion message problem with anonymous users only

Thanks for creating that patch. I needed it desperately for a custom module I developed which allows a user to impersonate another user during the checkout process . Commerce does not allow it by default.

I think #4 is headed in the right direction but I made a few changes and rolled a new patch. The changes include:

• Remove the ALLOW/DENY constants and rely on TRUE/FALSE like the patch in #1. This is consistent with other access hooks including hook_commerce_entity_access.
• Add the new hooks to commerce_checkout_hook_info() in order to allow lazy loading.
• Took a stab at adding hook documentation in commerce_checkout.api.php.

Thanks for the improvements! I actually used the constants to maintain consistency with hook_node_access and similar mechanisms in core after a bit of research, but I'm not hellbent on keeping them. It's just a matter of what we want to be consistent with.

EDIT: Looking back, do we want to handle IGNORE in a similar way?

In the current patch, ignore would be handled by returning NULL (or nothing). This is how it is handled in hook_commerce_entity_access() as well as several other access hooks outside of hook_node_access().

Ah, I see. So long as we're being consistent. :]

One more for RTBC?

The patch from #7 makes a lot of sense to me, let's do this :)

Here's a new patch based on #7 that includes moving the cart session variable checking into commerce_cart.module.

Calls to the same method looking in the same place for only slightly different things smells a bit, but that's not an issue for this patch.

Approach looks sane, though.

The double call to commerce_cart_order_session_exists() definitely smells a bit to me as well, but it's a far more pleasant smell than the one the session variable checks at the end of commerce_checkout_access() are giving off now. I think removing the double call would require changing the way the commerce_cart_order_session_* functions are implemented, which I agree is not an issue for this patch.

Totally agree, on all counts.

More eyes for RTBC?

I'll see if I can test this soon. On our site it would be preferable to allow anonymous checkout, but prevent users coming through anonymously with PayPal EC from having an access denied, etc.

Tested against the latest dev and it all looks good!

Many +1's for getting this committed. We have several sites at the moment that are overriding the whole of 'commerce_checkout_router' just to get around this problem.