If the fourth parameter to commerce_entity_access is not passed in, or passed in as null, then incorrect access logic occurs.
The related issue, in ctools_entity_from_field_context, has existed for about two years, so while this is certainly an improper invocation by the caller, it is not isolated.
If the fourth parameter is not passed in, the entity_get_info() call is made with an argument of null. This returns an array of arrays, rather than a singular array into $entity_info.
This renders all of the checks against $entity_info invalid, since the code assumes that the $entity_info is for a singular entity, rather than an array of entity_info(s) keyed by entity_type.
This does two things:
1) It always returns false for 'create', 'update', and 'delete' operations, when it possibly should be true. (It could also incorrectly return true, if another module implements hook_commerce_entity_access and also has issues with $entity_access being null)
2) Because of inverse logic for the 'view' operation, it always returns true.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | commerce_entity_access_incorrect_access_logic-2608754-2.patch | 530 bytes | tomtech |
Comments
Comment #2
tomtech commentedAttached is a patch that short circuits the commerce_entity_access function and always returns false if the entity_type is not specified.
Comment #3
torgospizzaWe're getting some odd results now as well, and I think the entity_access function needs to be strengthened a bit.
When I submitted some Commerce entities to Apachesolr, I got the errors:
Because the function doesn't set a default value for those parameters this warning gets thrown (and of course the function is then looking for an isset() rather than an !empty(). Obviously not a huge show-stopper but I'd prefer not to have messages like this clogging up our logs.
Attached is a patch that builds off #2 and also sets default NULL values for args 3 and 4.
Comment #4
rszrama commentedCan you link to the relevant issue in Ctools? Is it one I've already commented on in the past? (I know their lengthy release cycles have caused issues elsewhere in the past.)
Comment #5
torgospizzaI could only find this one still open, there are plenty of others that are duplicate: #2209775: Undefined index: access callback in ctools_entity_from_field_context()
Others:
#2195211: Warning after update: Missing argument 4 for entity_metadata_taxonomy_access
#2160355: Trying to get property of non-object in entity_metadata_user_access()
For us, using Panelizer and Views on a Product Display (and the product display has product reference fields, and those referenced product entities have poster images, and other data on them) suddenly none of our Product entity data was showing up in the Panelizer. Essentially the Relationship in Panelizer had vaporized. Re-adding the relationship worked, but I think that was before I applied the patch in this issue. I will test a bit more and verify that.
Looking in the logs I'm now getting (thanks to the patch in this issue) the warning that a no entity_type was passed into the function. I'll keep digging here, because we definitely need to get this fixed :)
Comment #6
tomtech commented@torgosPizza, thanks for the additional info.
@rszrama, I've added two related issues.
In ctools, this was resolved in #2195211: Warning after update: Missing argument 4 for entity_metadata_taxonomy_access.
It was the regressed with #2209775: Undefined index: access callback in ctools_entity_from_field_context().
I do think there would be other issues in this callback from ctools, though, since the signature for the entity access callback seems inconsistent.
For example:
entity_access($op, $entity_type, $entity = NULL, $account = NULL)
commerce_entity_access($op, $entity, $account, $entity_type)
entity_metadata_taxonomy_access($op, $entity = NULL, $account = NULL, $entity_type)
the base 'entity_access' method would receive the wrong arguments.
Regardless of the ctools callback, I think the bigger issue is the security concern with this method. If entity_type is not passed, you get a missing argument notice.
Many, based on the threads referenced, view this as more annoying than anything else.
But I'm a bit concerned that the function will, in some cases, return true when it should be false.
Comment #7
torgospizzaThanks @TomTech. I think what I'll do is remove and revert my patch, then apply the latest patch at #2209775: Undefined index: access callback in ctools_entity_from_field_context() which seems the way to go, unless I'm completely wrong here.
Comment #8
torgospizzaComment #9
torgospizzaConfirmed: reverted my patch (but applied patch #2) and THEN applied the patch from CTools at #2209775-24: Undefined index: access callback in ctools_entity_from_field_context() and the problem went away. Posters are now appearing as expected.
Comment #10
torgospizzaThat patch in CTools has landed, so we can probably close this, and point folks to use the latest version of CTools rather than patch Commerce.
Comment #11
rszrama commentedI wish there was a status for "waiting on dependency release". : P
Maybe we just move this to postponed or something and recommend 1.x-dev until 1.10 is released?
Comment #12
torgospizzaSounds like a plan. And yeah, a bit more granularity for these statuses is perhaps warranted :)
Comment #13
bojanz commentedWe're at 1.12 now :)