Fix query access control on relationships (comments)

Hm, this is applicable to any node access module. I checked with dereine and merlinofchaos. The Views relationship handler might be able to take care of this automatically.

I also added a note to the TAC troubleshooting handbook page.

What about this?

Commited to 7.x-3.x after some advice from davereid. There are modules which has two access tags: commentrss.

This patch didn't worked that well. Reverted

This time the patch should work better.

Patch works as expected against Domain Access 7.x.3.

SELECT DISTINCT node_comment.title AS node_comment_title, node_comment.nid AS node_comment_nid, comment.changed AS comment_changed, comment.subject AS comment_subject, comment.cid AS cid, comment.nid AS comment_nid, 'comment' AS field_data_comment_body_comment_entity_type FROM comment comment LEFT JOIN node node_comment ON comment.nid = node_comment.nid INNER JOIN node_access na ON na.nid = comment.nid WHERE (( (node_comment.status = 1 OR (node_comment.uid = 0 AND 0 <> 0 AND 0 = 1) OR 0 = 1) ))AND(( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '0') AND (na.realm = 'domain_site') )OR( (na.gid = '2') AND (na.realm = 'domain_id') ))AND (na.grant_view >= '1') ORDER BY comment_changed DESC LIMIT 5 OFFSET 0

Thanks for the review!

Commited to 7.x-3.x

This broke a lot of things.

The intention is good, but you cannot do it this way: Drupal doesn't allow you to define which base table(s) a tag applies to. By default, only one table is going to ever be used per tag.

What we do in Entity Reference, and is a bit of a hack, is to alter the query ourselves giving it only the new tag and the base table:

    // Save the old tags and metadata.
    // For some reason, those are public.
    $old_tags = $query->alterTags;
    $old_metadata = $query->alterMetaData;

    $query->alterTags = array($tag => TRUE);
    $query->alterMetaData['base_table'] = $base_table;
    drupal_alter(array('query', 'query_' . $tag), $query);

    // Restore the tags and metadata.
    $query->alterTags = $old_tags;
    $query->alterMetaData = $old_metadata;

Views is going to need to go this way if we want to properly do access control across relationships, or the current patch needs to be reverted (see #1323366: Query access fails in certain cases on post-RC1 Views for an example of the breakage the current implementation causes).

Here is the same idea applied to Views.

I also moved the tagging of the base query from views_plugin_query_default::execute() to view::build(), as the previous patch introduced a generic notion of query tags.

Tested, works for Commerce.

@agentrickard
Could you check if Domain Access still works with the latest patch?

Retitling and promoting, since I don't think Views 7.x-3.0-RC2 should go out without this fixed.

+++ b/includes/view.incundefined
@@ -801,6 +801,15 @@ class view extends views_db_object {
     }
 
+    // Add the access control tag to the query.
+    if (empty($this->query->options['disable_sql_rewrite'])) {
+      $base_table_data = views_fetch_data($this->base_table);
+      if (isset($base_table_data['table']['base']['access query tag'])) {
+        $access_tag = $base_table_data['table']['base']['access query tag'];
+        $this->query->add_tag($access_tag);
+      }

Mh, $this->query->options is part of the query_default object so using this in the main view class seems to be kind of hacky.

+++ b/plugins/views_plugin_query_default.incundefined
@@ -1009,8 +1009,11 @@ class views_plugin_query_default extends views_plugin_query {
+  function add_tag($tag, $base_table = NULL) {
+    $this->tags[] = array(
+      'tag' => $tag,
+      'base_table' => $base_table,

But sure it makes sense to add a add_tag function.

The views_plugin_query_default::add_tag() method has been added by the previous patch. views_handler_relationship using it makes it more general then just the default query plugin, isn't it?

Maybe we should add a stub implementation in views_plugin_query, and move the 'disable_sql_rewrite' check to the add_tag method itself?

The views_plugin_query_default::add_tag() method has been added by the previous patch. views_handler_relationship using it makes it more general then just the default query plugin, isn't it?

Ups :) Sure it makes sense to move them up.

and move the 'disable_sql_rewrite' check to the add_tag method itself?

I'm not sure about it. Why shouldn't you be allowed to add some other kind of tags to the query, Can't they by used for something else then rewriting query, perhaps logging?

For the record, the current patch fails with Domain Access. The query alter tag is not applied.

Here is a new patch which fixes the above issue and some code-style stuff.
@agentrickard
Can you describe how this fails with DA and how to reproduce the problem. Maybe we can help here.

Here is a patch which should not fail anymore. $alias vs $base_table was the problem.

According to dereine, this is a release blocker for a stable version of Views.

The previous version of the patch did not apply the {node_access} JOIN when used with the default "recent comments" view. This new patch does, albeit with an error.

Notice: Undefined index: base_table in views_plugin_query_default->build() (line 1311 of /Applications/MAMP/htdocs/drupal-domain73/sites/all/modules/views/plugins/views_plugin_query_default.inc).

Odd, actually. because this part of the patch seems to fail on that View:

    // Alter the queries.
    foreach ($this->tags as $tag) {
      $base_table = $tag['base_table'];
      $this->execute_alter_hooks($view->build_info['query'], $tag['tag'], $base_table);
      $this->execute_alter_hooks($view->build_info['count_query'], $tag['tag'], $base_table);
    }

$this->tags is an array that contains 'tag' and 'table_alias', but not 'base_table'.

Too many patches

dereine found an issue with build_join() that prevented the query from running properly. This version is working on Domain Access for both the 'recent comments' and 'tracker' views.

However, I get the same results with the following code for execute_alter_hooks():

  /**
   * Manually alter a SelectQuery.
   *
   * Core doesn't allow defining a different base table for every tag,
   * making it impossible to property perform access control across
   * relationships. We run every tag one by one, mimicking the alter process
   * performed in SelectQuery::preExecute().
   */
  protected function execute_alter_hooks(SelectQueryInterface $query, $tag, $base_table = NULL) {
    drupal_alter(array('query', 'query_' . $tag), $query);
  }

So I wonder just what the more complex version is trying to do.

Please react on this issue, because it doesn't seem to be that this is a real blocker.
So maybe there will be another RC without this patch, sorry.

#24 looks ok to me.

Just to answer the question:

So I wonder just what the more complex version is trying to do.

This version is meant to be able to apply several tags across more then just one relationship. Ie. it will apply the proper access control when you add a third relationship to the mix (for example: node -> comment -> author of comment or comment -> parent comment -> parent comment will all work properly).

And yes, fixing this properly is definitely very high priority for me.

Okay, I just diffed the latest patches. Only concerned about this part:

diff --git a/handlers/views_handler_relationship.inc b/handlers/views_handler_relationship.inc
-index bcab8b9..2344536 100644
+index bcab8b9..be80fe6 100644
 --- a/handlers/views_handler_relationship.inc
 +++ b/handlers/views_handler_relationship.inc
 @@ -137,7 +137,7 @@ class views_handler_relationship extends views_handler {
@@ -7,10 +7,23 @@
      if (empty($this->query->options['disable_sql_rewrite']) && isset($table_data['table']['base']['access query tag'])) {
        $access_tag = $table_data['table']['base']['access query tag'];
 -      $this->query->add_tag($access_tag);
-+      $this->query->add_tag($access_tag, $alias);
++      $this->query->add_tag($access_tag, $this->definition['base']);
      }
    }
  
+diff --git a/includes/handlers.inc b/includes/handlers.inc

Will this work if the table gets joined in twice?

Well at the end the only thing which will be set to the query is base_table + the access tag.

The patch uses $this->tags[] = array(...); so there will be a tag foreach relationship and so a query-altering foreach relationship.
So this seems to be fine.

This is a small test module for manual testing: it implements an access control strategy that:

• shows only the nodes whose title starts with "a"
• shows only the comments whose subject starts with "b"

Great thanks! Anyway as multiple people now have tested it commited to 7.x-3.x
A small step for access, but a big step for the release.

So the committed patch is definitely not correct. It issues a 'base_table' metadata that is not aliased, and as a consequence doesn't match a table of the query.

Here is a the export of a view that I'm using to test this. If the patch was correct, I would result in the following query:

SELECT node.created AS node_created, node.nid AS nid
FROM 
{node} node
LEFT JOIN {comment} comment_node ON node.nid = comment_node.nid
LEFT JOIN {node} node_comment ON comment_node.nid = node_comment.nid
WHERE (( (node.status = '1') ))AND (node.title LIKE 'a%' ESCAPE '\\') AND (comment_node.subject LIKE 'b%' ESCAPE '\\') AND (node_comment.title LIKE 'a%' ESCAPE '\\') 
ORDER BY node_created DESC
LIMIT 10 OFFSET 0

This fixed the mentioned view for me.

Okay this one works for me as well

function build_join($select_query, $table, $view_query) {
     if (empty($this->definition['table formula'])) {
-      $right_table = "{" . $this->table . "}";
+      $right_table = $this->table;
     }

This was the actual bug fix, which made this working.

I knew #28 smelled like trouble :P

Great job, guys!

Based on @damz research reverted this patch. Damz told that this might be a bug of entity_reference/commerce

Sad panda :(
So what happened? Why was the whole patch rolled back?

Related: #1276450: Views results empty for unprivileged user when using Relationship: Content: Referenced Product, which I've set to 1.1 blocker for now - perhaps it just needs documentation. But I think a lot of people probably don't understand the issues involved and will have odd things happening to their production views.

Just a random patch which allows to not add a access tag.

#39 is not going to work: core doesn't support picking which joined tables are affected by access control and which are not. It merely apply the access control to all the tables that have the same name as the 'base_table'.

Well this patch would allow some people to get the same behavior as RC1.

This issue is now not about the initial problem anymore... see #36

Who can give me a clue how to solve my problem ?
#1354550: Problems when user don't have "Bypass content access control" permission

I see here 1000 patches and as I see in the discussion 99% of them don't work.

I have the same problem as in #1354550: Problems when user don't have "Bypass content access control" permission. I applied the patch #39, ticked the checkbox on the relationship settings form but as predicted in #40 it did not deprive the query of the node_access inner joins. They stay even if I comment these lines out from views_handler_relationship.inc (around line 147):

    if (empty($this->query->options['disable_sql_rewrite']) && isset($table_data['table']['base']['access query tag'])) {
      $access_tag = $table_data['table']['base']['access query tag'];
      $this->query->add_tag($access_tag);
    }

surprised to see this is still an issue... seems like its the root of #1276450: Views results empty for unprivileged user when using Relationship: Content: Referenced Product

This is still a problem in 7.x-3.3 as well; it'd be nice to see this problem fixed. I had to remove relationships from a View to make the View accessible to anonymous users -- I'd prefer not to have to do that.

• Closed #1354550: Problems when user don't have "Bypass content access control" permission as a duplicate of this issue.
• #1349080: node_access filters out accessible nodes when node is left joined may also be a duplicate.
• #1409640: Taxonomy terms on node relationship potentially joins on wrong table may also be a duplicate.

As posted on #1349080: node_access filters out accessible nodes when node is left joined, I spent some time in a debugger and tracked it down to a core bug in Node module, when any node_access is applied to the query.

#1409640: Taxonomy terms on node relationship potentially joins on wrong table looks like a different bug.

Closing this, it feels like the remaining problems are being handled in core via #1349080.

Sorry, but no, this is not fixed.

#1349080: node_access filters out accessible nodes when node is left joined is about showing more nodes, not less comments. Besides, #1349080-332: node_access filters out accessible nodes when node is left joined for D7 has been sitting there for 3 years now, with no hope of being committed, so this definitely remains an open issue with Views for D7.

@salvis: Thanks for clarifying that.

So I think what we need here is to follow a test-driven approach - start with writing some test coverage that shows the problem, then we can fix the root problem.