commerce_entity_access_query_alter can add crazy conditions if the base table does not match the entity type

This is actually unrelated. That other issue has to do with a legitimate access control check afaik; disabling SQL rewriting on the View will solve that.

This issue seems to be bumping up against what we've documented would happen here:

http://drupalcode.org/project/commerce.git/blob/refs/heads/7.x-1.x:/comm...

Note that it basically says if the query alter function wasn't passed a base table, all we can do is guess, and we'll likely be wrong. I wonder if commerce_product_query_commerce_product_access_alter() doesn't need to pass 'commerce_product' or some alias to commerce_entity_access_query_alter() as a base table parameter.

Hah, for what it's worth, I maintain it's safe to disable SQL rewriting on Views where you know you're doing other filtering. For example, if the order would only ever join to line items that you know are on it and you've confirmed that the user has access to view the order through other filters / contextual filters, there's no harm in disabling rewriting so they have access to view any of the referenced line items or their referenced data.

Ultimately I think that's a better solution than essentially duplicating access control by leaving the check in place even though we know through other means that the user should have access to view the data.

Alrighty, I did a fair bit of digging into this this afternoon and evening. I think I turned up an unrelated bug and conceived of a more intelligent way to establish a base table, because it's obviously incorrect for a commerce_product entity access check to be using commerce_order as the base table for its rewriting. : )

Fixing the base table

If a $base_table parameter currently isn't passed in to the function, it just takes the first one from the tables array and uses it, even though it's likely incorrect. We've even documented it's incorrect, but we never made an attempt to get a nearer match.

However, the tables array actually contains the information we need to make a better guess. I've updated the comments and code to indicate that I'm looping through the tables to see if any of them match the base table of the access query's entity type, and if so I'm using that table's alias as the new $base_table. In a quick test, this had the desired effect for this particular scenario.

Fixing an unknown problem with $really_restricted

I can't remember all the details surrounding our inclusion of the $really_restricted parameter. I'm pretty sure it was something of a premature optimization; we determined that we didn't need to add any additional conditions if the user had "view any * entity of bundle *" because that would trump the more specific ownership checks. Thus if we got to the end of the bundle checks and the user only had "view any * entity of bundle *" permissions, we exited out of the function.

This means that the $conditions array was never actually added to the $query, though, because we don't get around to doing that until the end of the function. : P

I've addressed this in the same patch by simply removing $really_restricted altogether and adding some additional documentation to the creation of the $conditions OR group. Since this is an OR group, we don't need to exit out early. There's no problem in us having multiple conditions on the query with some failing if the original one passes, and it is feasible that someone might have a "view any * entity of bundle *" permission that doesn't affect a query but a "view own * entities" permission that does. That scenario would currently be unsupported by Commerce.

I'm going to get a review from Damien on this before moving forward (he may remember the context of $really_restricted), but the patch is posted for you and the test bot to test. : )

Reactivating the test bot.

Let's try just reposting the patch.

Damien clarified $really_restricted for me. While I read it above as potentially a premature optimization, I was wrong. It's a safe assumption and a useful optimization. I was missing the fact that if more than one product types were present on the site and the user only had access to view all products of one or the other type, it would fall down through this if / else statement to set $really_restricted to TRUE. But in the event there's only one product type and we detected the user has access to view any products of that type, it's completely safe to exit early - neither the product type check nor the falsifying condition will be added and the view query would pass as expected.

Regarding the first hunk, we took a look at _node_query_node_access_alter() to ensure our function matched the core behavior for determining a base table. It appears this patch gets most of the way there, though I apparently need to take into consideration that a table in the query's tables array may itself be a select query (presumably accommodating joins to the results of subqueries). Node does offer some manner of support for foreign keys, but I don't think that's essential for us to implement here. Additionally, I'm not sure node doesn't have a bug in using the table name instead of its alias name, but perhaps there's some other mechanism in the Node module for joining to the table directly that I'm unaware of. : ?

Commit: http://drupalcode.org/project/commerce.git/commitdiff/5211db5