Entity listing and loading does not allow for node access

subscribe

Almost did it correct from the first try ;)

+   * @param $tag
+   *   The tag to add.
+   * @return EntityFieldQuery
+   *   The called object.

One small newline needed

I am +1 on adding the ability to add tags to EntityFieldQuery.

What aspilicious is saying is that there should be a blank line between @param and @return in docblocks. The doc could be cleaned up a bit in other ways too, but it's clear enough.

As far as it being a security hole, maybe we should put something into the doc header for the class about that?

Also, there is no taxonomy tag for queries, or at least there is no taxonomy query alter function that I can see on api.drupal.org.

Anyway, here's a new patch. The only difference is documentation...

Hmmm....

Take a look at http://api.drupal.org/api/function/node_query_node_access_alter/7

In order to use the query tag, you also need meta data. So there also needs to be an addMetaData method on this query.

Anyway, here's some cleaned up doc, but the code needs work still.

See http://api.scratch.drupal.org/api/drupal/includes--database--query.inc/f... for more on addMetaData

Also for an example of usage, NodeQueryAlter::testNodeQueryAlterLowLevelWithAccess() in modules/node/node.test has an example.

Agreed with the above. I can help on the documentation part for sure.

node_load() has had no access restrictions for nearly a decade and we've not considered that a security hole. Why exactly do we now need even that to be filtered? Or am I misunderstanding how far this is going to go?

Hmmm. Are you saying that the node queries that respected node access in the past didn't go through node_load()?

Sorry, didn't mean to change the priority on you crell.

But It does need to be CNW, since if we are adding addTag we still need meta data too.

I didn't mean to change the priority either. Bah! :-)

I just talked with chx a bit in AIM. I'm fine with adding default access restriction to X_load_multiple(), just not to X_load(). It sounds like the plan is to add access restriction methods to the controllers with no defaults, and then X_load_multiple() will be hard coded with a sane set of default restriction markers (eg, node_access tag). If you want a different set, call entity_get_controller() yourself and do your own thing. That sounds good to me.

The caveat is that controllers are not designed to be disposable action objects the way SelectQuery is. That is, the controller persists from one query to the next. So what happens if you call NodeController->addTag('foo')->load(1), and then later on call NodeController->load(2), and then later call NodeController->addTag('bar')->load(3)? How do we tell that the tags/metadata/etc. are from an "old query"? The edge cases here are interesting to say the least... :-)

My best idea at the moment is for load() to end by wiping any such settings. So you could do NodeController->addTag('foo')->addTag('bar')->load(1), and then the next time you call NodeController->... it would have no tags since you already called load() once.

But then there's still the question of entity caching. I'll let someone else figure that bit of excitement out...

I'm not sure what's being suggested with node_load_multiple(), but that case is what node_access() is for, entity loading and listing are not the same thing, and I want entity_load() to not take $conditions at all in D8.

We definitely don't want to do any access control during the loading process. Access control belong in the query, not in the process of loading the objects. For example: you cannot correctly page the data if you haven't performed the access control beforehand.

This said, it seems possible to implement proper access control here. But we have to understand that the access control (that belong in the query) will have to be implemented differently for each query mechanisms.

In core, we only have to deal with field_sql_storage. This means that we need to:

• Add the <entity_name>_access tag to the SQL query generated by EntityFieldQuery::propertyQuery. Because we start the query from the base table, it should work transparently.
• Add a field_entity_access tag to the SQL query generated by field_sql_storage_field_storage_query(). This one is slightly more tricky, because in the case of field-only queries (where the entity type is not specified in the query), all the access control mechanism needs to kick-in at the same time. Each of them will have to do the access control conditionally on the etid of the row. For node_access, for example, it means having a LEFT JOIN {node_access} na ON ($base_table_alias.etid = :node_etid AND na.nid = $base_table_alias.entity_id), and having a similar check on the entity type in the condition.

It will probably not be that pretty (even if I'm convinced that we can implement that with only very minor changes to node_query_node_access_alter()), but we really need to do this.

I think all that is really being suggested is to put addTag/addMetaData capability on queries that are attempting to list entities.

The patch above adds addTag (and needs to also have addMetaData) on the field storage query.

chx (I believe) is also suggesting adding the capability of tags to the low-level entity controller's querying capabilities. He isn't suggesting adding it by default to entity_load, but just giving the entity controller class/interface the ability to have tags if someone wants to call the lower-level functions when they are doing a combined query/load using conditions.

And in both cases, the idea is also to document that access checks are not being done by default, so if you need access checks, you should use addTag.

This all looks fairly reasonable, except I'm not sure about this bit inside EntityFieldQuery::propertyQuery()

     }
+    if ($entity_type == 'node') {
+      $this->addTag('node_access');
+    }

Why automatically add the node_access tag? There are sometimes reasons for doing queries where you might not want to check node access (such as during cron runs, where user is anonymous but you might need to act on nodes anonymous wouldn't have permission to see), and with this added, it seems like you are forcing the query to add node access restrictions whether they're wanted or not. Or am I missing something?

Is the addMetaData function missing a return $this;? This should be required to be consistent with the documentation: @return EntityFieldQuery * The called object.

public function addMetaData($key, $object) {
  $this->metaData[$key] = $object;
}

should be

public function addMetaData($key, $object) {
  $this->metaData[$key] = $object;
  return $this;
}

Paddy.

addtag_efq.patch queued for re-testing.

#18: the_ugliest_sql_query_of_drupal_7.patch queued for re-testing.

#3: 860180-betterdoc.patch queued for re-testing.

tested this a bit and it works as described by DamZ and implemented by chx. i tested by enabling node_access_test module (using drush) and viewed the url: /node_access_test_page.

code and docs look good as well.

as a followup, chx mildly agreed to add a test which adds access control for users (e.g. hide all 'executive role' users) and does a user field query which applies those conditions. that would be great for testing and API documentation.

LIVE from Drupalcon CPH! I couldn't quite figure what this code was doing, so I started looking at the tests.

+++ modules/node/node.test	2010-08-15 06:27:00 +0000
@@ -1692,6 +1692,68 @@ class NodeQueryAlter extends DrupalWebTe
+        'value' => 'A' . $this->randomName(32),

+++ modules/node/tests/node_access_test.module	2010-08-15 06:27:00 +0000
@@ -100,3 +106,37 @@ function node_access_test_page() {
+    $result = $query->fieldCondition('body', 'value', 'A', 'STARTS_WITH')->execute();

This seems like it has a high probability of false-triggering when tests are expanded later, and it's also not obvious what's going on. Can we prefix with "[node_access_test]" or something?

This whole setUp() function could generally use a few comments to explain what we're setting up here and why.

+++ modules/node/node.test	2010-08-15 06:27:00 +0000
@@ -1692,6 +1692,68 @@ class NodeQueryAlter extends DrupalWebTe
+    $this->accessUser = $this->drupalCreateUser(array('access content', 'node test view'));

+++ modules/node/tests/node_access_test.module	2010-08-15 06:27:00 +0000
@@ -58,6 +58,12 @@ function node_access_test_menu() {
+    'access arguments' => array('access content'),

I don't understand where that "node test view" permission is actually used? There's no special allotment done in the node creation process... How do these tests pass at all right now?

+++ modules/node/node.test	2010-08-15 06:27:00 +0000
@@ -1692,6 +1692,68 @@ class NodeQueryAlter extends DrupalWebTe
+    // Verify that a user with access permission can see at least one node.
+
+    $this->drupalLogin($this->accessUser);
...
+    // Verify that a user with no access permission cannot see nodes.
+
+    $this->drupalLogin($this->noAccessUser);

(minor) weird stray blank lines here.

+++ modules/node/node.test	2010-08-15 06:27:00 +0000
@@ -1692,6 +1692,68 @@ class NodeQueryAlter extends DrupalWebTe
+    $this->assertText('Yes, 4 nodes', "4 nodes were found for access user");
...
+    $this->assertText('No nodes', "No nodes were found for no access user");

+++ modules/node/tests/node_access_test.module	2010-08-15 06:27:00 +0000
@@ -100,3 +106,37 @@ function node_access_test_page() {
+    if (!empty($result['node'])) {
+      $output .= '<p>Yes, ' . count($result['node']) . ' nodes</p>';
+      $output .= '<ul>';
+      foreach ($result['node'] as $nid => $v) {
+        $output .= '<li>' . $nid . '</li>';
+      }
+      $output .= '</ul>';
+    }
+    else {
+      $output .= '<p>No nodes</p>';
+    }

Is there a reason we don't just do "N nodes"? That seems like it'd simplify this code quite a bit.

+++ modules/node/node.test	2010-08-15 06:27:00 +0000
@@ -1692,6 +1692,68 @@ class NodeQueryAlter extends DrupalWebTe
+    $this->assertNoText('Exception', "No database exception");
...
+    $this->assertNoText('Exception', "No database exception");

Is that strictly necessary? Won't the testbot report if exceptions were encountered, just as it reports back if notices were encountered? If not, we should probably fix.

Powered by Dreditor.

I'll try to address webchick's issues in the patch from #25 to get this ready.

+++ modules/node/node.module	2010-08-15 06:26:49 +0000
@@ -3009,6 +3008,33 @@ function node_access_view_all_nodes() {
+ * @param $type
+ *   Either 'node' or 'entity' depending on what sort of query it is. See
+ *   node_query_node_access_alter() and node_query_entity_field_access_alter()
+ *   for more.
...
+function _node_query_node_access_alter($query, $base_table, $type) {

@@ -3037,12 +3063,24 @@ function node_query_node_access_alter(Qu
+  $condition = $type == 'node' ? $query : db_and();

So, I think this is the first place I'm lost. Under what circumstances does a function called _node_query_something something have something other than a node coming into it? I "saw" node_query_node_access_alter() and node_query_entity_field_access_alter() and that didn't help me out at all.

Can we flesh these comments out with an example of some kind? And/or, better yet, reflect it in the tests?

Powered by Dreditor.

Oops. I cross-posted. :( Sorry, dixon_!

No problem :)

I've got some major newtwork issues here in CPH. But here is the patch finally. chx improved the comments in node.module. So this patch only contains some comment improvements in node.test and some minor code style issues that webchick mentioned.

Moshe gave a review and found a double space in the comments. Now fixed.

RTBC. This was green before and all we changed was Doxygen but I guess we should still wait for green before commit.

Patch looks good, and running test locally pass.

Unintentionally set back to needs review. Network issues.

Reviewed it and it looks good. Committed to CVS HEAD. Thanks!