node_access_view_all_nodes() is never invoked

In regards to #2, the API for node_access_view_all_nodes() shows that it is used once. (Edit: On a second reading I realize I am incorrect.)

I don't understand what the issue is here. What sort of functionality isn't working?

subscribing for reference

The relevant bit of code disappeared in #701744: node_query_node_access_alter assumes $op, assumes $user, and will only work if alias is "n". In particular see comment number 28.

Again, I don't think there is a bug here. The proposed patch in #2 short circuits some complicated logic below -- at worst it would change access behavior and at best it might be a speed improvement.

This seems rather reasonable, and doesn't break the tests that were put in for the other patch...

Should this also be checking the value of $type, i.e., $type == 'node'?

Yeah, that 'type' thing is new, since the patch from the other issue, I believe.

And yes, I think a test would be a good idea. You'd need to make a test class that would cause the view all nodes thing to return true. Since the previous patch broke this behavior, a test would ensure that future patches don't break the behavior.

Also, in this patch, you need to update http://api.drupal.org/api/drupal/modules--node--node.module/group/node_a... so it explains how to bypass this so that node_access_view_all_nodes() will return TRUE. This information should probably also be added to the node access hooks doc?

You do not need to check node or entity. That argument is only about the base table which is the field data table for entity. If you do not need to add anything access-wise, then you do not need to. Leaving at CNW for #13 and #14

There is not much point in calling node_access_view_all_nodes() if the base table is not node, though. It would certainly be more efficient to put that into the if() rather than just failing to find something in all those queries?

But actually, shouldn't that check be in http://api.drupal.org/api/drupal/modules--node--node.module/function/nod... -- I mean, why is it calling a node query alter function at all if the base table isn't 'node'?

We need a test for this.

Also, since this appears to be security related, I believe this makes it a beta blocker, too...

I may have time to look at this later today.

The first assert statement

     // Check that node_access_view_all_nodes() allows node access modules
    // to override their own access rules on the 'view' operation.
    $this->assertFalse(node_access_view_all_nodes(), t('Node access returned false for node_access_view_all_nodes().')); 

is failing not because node_test.module is not loaded, but instead because {node_access} has records in it.

In tracing through what happens, in node_access_view_all_nodes()
if (!module_implements('node_grants')) {
evaluates to FALSE, so the access query below runs.

At this time the content of {node_access} is:

mysql> select * from simpletest846636node_access;
+-----+-----+--------------------+------------+--------------+--------------+
| nid | gid | realm              | grant_view | grant_update | grant_delete |
+-----+-----+--------------------+------------+--------------+--------------+
|   0 |   0 | all                |          1 |            0 |            0 |
|   1 |   1 | test_article_realm |          1 |            0 |            0 |
|   2 |   1 | test_page_realm    |          1 |            0 |            0 |
|   3 |   1 | test_page_realm    |          1 |            0 |            0 |
|   4 |   2 | test_alter_realm   |          1 |            0 |            0 |
+-----+-----+--------------------+------------+--------------+--------------+
5 rows in set (0.00 sec)

and so node_access_view_all_nodes then returns TRUE.

One first problem is that we'll need to call drupal_static_reset('node_access_view_all_nodes') before each call to node_access_view_all_nodes().

This test might need to create its own special test module that will pass the view_all_nodes() tests, and not break any of the old tests?

After poking around, the problem seems to be that node_access_grants('view') is returning only the default 'all' => array(0) array.

The problem is not that node_test_node_grants isn't returning 'node_access_all' => array(0), but instead is that

 /**
 * Implements hook_node_grants_alter().
 */
function node_test_node_grants_alter(&$grants, $account, $op) {
  // Return an empty array of grants to prove that we can alter by reference.
  $grants = array();
}

is emptying the array.

Right. That test class was written with very specific goals. You'll probably be better off writing your own for testing this issue.

For reference, here is a patch that passes the node tests on my computer, however I agree with the others that another test class should be written.

In this patch I've moved the testing routines to a new unit test class. I'm leaving at needs work, because the unit test doesn't successfully capture the actual issue that launched this discussion.

That is, the tests do not check that we ever call node_access_view_all_nodes() from _node_query_node_access_alter.

Unfortunately I don't think I know enough to capture that behavior in a unit test.

Does anybody know why all of the patches aren't getting sent off for testing?

You have to set the status to "needs review" to cause the test bot to launch tests. Doing that now.

Now setting back to "needs work", see above - the proposed patch is not recommended by the creator.

@agentrickard, in regards to #29:

The easiest way I've found to capture data within simpletest is to have mysql log everything

bfroehle@sonoma:~$ cat /etc/mysql/conf.d/log_everything.cnf
[mysqld]
log=/tmp/mysql.log

and follow-up with a liberal use of sudo tail -f /tmp/mysql.log.

Another option is to insert a die(); statement which will kill the testing routine in a specific location. Lastly $this->debug(print_r($variable,1)); is useful for probing variables at different stages of the test.

@agentrickard, in regards to #30:

Perhaps this test should be tossed into the NodeQueryAlter test case then? This may be a better fit than the previous addition to NodeAccessRecordsUnitTest.

I don't have a sense for exactly how the node access code is structured, so it's hard for me to suggest a specific test structuring (or even which tests to run). Do you have any examples of code that doesn't function without the original patch in #1?

#25: 963656-node_access_view_all_nodes-is-never-invoked.patch queued for re-testing.

#26: 963656-node_access_view_all_nodes-with-new-unit-test.patch queued for re-testing.

@agentrickard The latter part of what you suggest in #35 is essentially the patch in #25.

As discussed with agentrickard in IRC, we present a patch which does the following:

1. Calls node_access_view_all_nodes() from _node_query_node_access_alter(). See comments #7 and #8 for a discussion of why this is necessary.
2. Improves the documentation of node_access_view_all_nodes() to describe its intended use, as per comment #14.
3. Provides a unit test which fails if node_access_view_all_nodes() is not called from _node_query_node_access_alter(), as per comment #13.

Code looks good, a couple of text edits though:

+++ modules/node/node.test
@@ -1801,6 +1801,63 @@ class NodeQueryAlter extends DrupalWebTestCase {
+    // Ensure that adding the record to node_access keeps the ¶
+    // noAccessUser without view prileges

Remove extra whitespace after "the". "privileges" is misspelled. Sentences need a period at the end. The sentence is also phrased awkwardly. Perhaps, "Test that the noAccessUser still doesn't have the 'view' privilege after adding the node_access record."

+++ modules/node/node.test
@@ -1801,6 +1801,63 @@ class NodeQueryAlter extends DrupalWebTestCase {
+    // Tell hook_node_grants to now return a node_access_all privilege,
+    // thus giving our noAccessUser view access

Needs a period at the end.

#39 + language updates from carlos8f in #40. (Thanks for catching my misspellings and typos!)

One more comment edit:

By design, this function checks..

This could just be "Checks..", the rest is redundant.

I'm not very familiar with node access internals but patch looks sane.

I'm going to defer to agentrickard to fix the documentation for node_access_view_all_nodes(), since he understands exactly what it does better than I do.

The doc for node_access_view_all_nodes() might also want to mention something like:

This function allows other modules to replicate this core behavior by providing
their own conditional grant for nid 0. For example, hook_node_grants() can
return the following array to give the 'view' privilege to all nodes:
@code
if ($op == 'view') {
  $grants['example_realm'] = array(0);
}
@endcode

@see hook_node_grants()

...to make it more clear to developers how to interface with it.

This includes the @code example and some revised text.

This looks good, id should be ID though, or possibly nid.

Leaving at CNR, as far as I'm concerned this is good to go otherwise.

'node ID 0' looked weird, so i went with changing 'node id 0' to 'nid = 0'.

I don't see anything else to complain about, the actual code change is tiny, comes with tests, so marking RTBC.

#1: 963656-view-all-nodes.patch queued for re-testing.

Awesome work, folks!

The only thing I'd suggest is that the extra documentation added here probably more belongs in hook_node_grants (which developers will naturally look up when trying to figure out the node access system) rather than here (which they won't). But I'm not going to hold up a critical bug fix (with tests!) over it. :P

Committed #48 to HEAD!

Could we get a quick follow-up that moves the docs location? I would've just done it myself, but figured it best a node access maintainer chime in on the request.

.

+ * For this feature to work, your module's nid = 0 row must be written to the
+ * {node_access} table after node_access_rebuild(). Note that this check only
+ * applies to the 'view' operation.

That sounds like your module needs to write something to the database. Please rephrase in terms of what should be returned from which hook, because I don't think we want node access modules writing to this table directly?

  * Checks to see whether any module grants 'view' for nid = 0. The node module
- * provides this record if no node access modules are enabled. Other modules
- * can replicate this behavior by providing their own conditional grant for
- * nid = 0. For example, hook_node_grants() can return the following array to
- * give the 'view' privilege to all nodes:
- * @code
- * if ($op == 'view') {
- *   $grants['example_realm'] = array(0);
- * }
- * @endcode
+ * provides this record if no node access modules are enabled. 

I think it's OK to remove this from this function, but maybe a link to the hook doc where you're putting it now would be a good idea?

Sorry, I just saw this.

Could someone detail exactly what queries we have added (when node access modules are present). I'm not so sure that this was a deliberate feature in D5/D6 and bringing it back should be avoided if it involves a multi table query.

x-post

@moshe what this does is avoids joining the main query with {node_access} if a global view grant exists, and it does a COUNT(*) query on {node_access} (statically cached) to determine if that's the case.

#218066: Prevent cross posting from reverting metadata fields

If this is a deliberate feature of the API, I would have expected a way to insert a nid = 0 record into {node_access} via a hook that node_access_rebuild calls.

I expected the same as @bfroehle. It seems pretty awkward writing a placeholder row to {node_access} *and* returning array(0) with the hook. I would think (although I'm not an expert on the grants system) only one or the other should be necessary.

Well, if that's the case, probably the right thing to do is to fix the documentation so it explicitly says "you need to do this manually" or something like that. If you're sure...

The point of documentation is to make things clear, not to make things difficult or obscure.

This documentation seems contradictory:

+ * If no node access modules are present, the core node module will provide
+ * an 'all' grant for nid = 0. This grant allows 'view' access to all published
+ * nodes. When a node access module is enabled, this record is removed from
+ * {node_access}. Node access modules can replicate this behavior by providing
+ * their own conditional grant for nid = 0. For example, hook_node_grants() can
+ * return the following array to give the 'view' privilege to all nodes:
+ * @code
+ * if ($op == 'view') {
+ *   $grants['example_realm'] = array(0);
+ * }
+ * @endcode
+ * For this feature to work, your module's nid = 0 row must be written to the
+ * {node_access} table after node_access_rebuild(). Note that this check only
+ * applies to the 'view' operation.

The first paragraph implies you can make the nid=0 grant using hook_node_grants(). Then the second paragraph says you have to put a row into {node_access}. I find this confusing, and apparently (see @62) others do too. Are you really sure that both have to be done? It seems like hook_node_access_records or hook_node_grants should take care of this.

And again, please link from where you took this documentation out in node.module to this hook doc, so someone can find the documentation there.

Oh... I see, these paragraphs are correct, but they're still *extremely* confusing and I think they need to be rewritten.

I've taken a stab at rewriting the documentation for this feature. This needs review from both a clarity perspective and an accuracy perspective, since I do not claim to be an expert in this particular matter.

Happy BAD Camp everybody.

A few more grammar adjustments.

+++ modules/node/node.moduleundefined
@@ -3007,16 +3007,13 @@ function node_access_grants($op, $account = NULL) {
+ * Note that this function does not allow a user to view all nodes. Instead, it
+ * removes the default JOIN from the {node} to the {node_access} table
+ * provided by node_query_node_access_alter() when querying for lists of
+ * nodes. Other rules enforced by the node_access() function are still applied.

I still don't quite understand this paragraph.

How about:

* Beware! The function name may lead to confusion.  The function returning
 * TRUE does not automatically allow a user to view all nodes. Instead, it
 * only removes the default JOIN from the {node} to the {node_access} table
 * provided by node_query_node_access_alter() when querying for lists of nodes.
 * In particular, other rules enforced by the node_access() function are still
 * applied.

And since I just woke up, I guess I attached a patch which does not actually match the text of my comment in #73. Fixed here.

Er, rename the function instead if a wall of text explaining its shortcomings?

+ *
+ * Beware that node_access_rebuild() function will erase any node ID 0 entry
+ * when it is called, as there is currently no hook to generate node ID 0
+ * grants.  Module developers are responsible for ensuring these gloabl view
+ * grants are restored after node_access_rebuild() is called.

It would be good to clue the developer in about how they would do this.

What happened to the code changes and/or tests in this latest patch? I'm confused...

@jhodgdon in #76: The code changes and tests already were committed by webchick in #51. At this point we're just working on fixing up the documentation so this 'very dark corner of the API' as agentrickard called it in #65 is a little less dark.

@moshe in #75: That seems like a good solution. Or perhaps I am totally mis-understanding the point of the function --- this is not my area of expertise.

oh, sorry! I forgot (and am a bit out of it today).

I'm going to take a stab at this. I don't think the current patch explains things quite right...

I think this whole function is incorrect, besides the fact that the documentation is confusing or wrong.

Take a look at:
http://api.drupal.org/api/drupal/modules--node--node.module/function/nod...

It calls node_access_grants('view') to figure out which realms/gids apply to the current $user, even though http://api.drupal.org/api/drupal/modules--node--node.module/function/_no...
allows the query to pass in a particular $account, which is used for the other permissions checks.

So I think that node_access_view_all_nodes() function needs a rewrite. It needs to have a $account parameter, and if it's going to cache its information, it needs to cache on a per-account basis.

The patch in #51 that was committed is wrong.... or am I missing something?

Bumping to critical again, possible problem with the commit. This permission (view all nodes) is very general, so I'm not certain how big of a bug this is. But node_access_view_all_nodes() implicitly does only operate on the current user, which does not respect the query's account in $query->getMetaData('account'). This might be why it was left out in the DBTNG refactor. Let's get more eyes on this to determine how big of a problem this is. If it's a problem, we need another API change.

To get the ball rolling, here's a patch which adds an $account to node_access_view_all_nodes(). The result is statically cached only if $user == $account.

Needs review to trigger the test bot.

Back to needs work. At a minimum, the patch in #83 leaves out all of the changes to the comments as discussed between #53 and #79.

The caching should be done for all $account->uid, in an array, I think. Otherwise, this function is a big performance hit.

jhodgdon: Do we cache for all users anywhere else? What if this somehow gets called for every user in the database?

Looking back at the D6 code, there is no regression I can see. node_access_view_all_nodes() is part of the archaic node_db_rewrite_sql(), and uses $GLOBALS['user']. Respecting $account can't be a bad thing though, although I'm not sure what real difference it makes if all we're checking for is a global view grant.

Alternatively we could choose only to invoke node_access_view_all_nodes() from _node_query_node_access_alter() if $user->uid == $account->uid. I'm not sure what effect this would have on the contributed node access modules.

As far as the caching goes, the problem is that if someone is doing a node query and has node access modules, and the node query is against an account other than the global $user, this function could be called several times... or maybe not, but if it isn't called several times, why bother with caching at all?

My point is, either cache for all the calls, or remove the caching.

Deciding not to call node_access_view_all_nodes if the $account is wrong is not the right answer. That would break the desired functionality.

This patch caches all results in an array keyed by $account->uid.

I also updated the unit test to ensure node_access_view_all_nodes is receiving the proper $account and not using the global $user.

Again, still need to work in revised documentation in #53 to #79.

#90 looks good. The bug itself is minor, just an oversight (or technical limitation?) that has been in core from the early days. Marked critical only because the API change should be in before RC. If #90 looks good to go, we can get that in, demote back to normal, and work on the documentation as a follow-up.

The patch's new doxygen block is not formatted correctly -- needs a blank line between the @param and the @return. I also don't like what was written for the @param much:

+ * @param $account
+ *   The user object for the user performing the operation. If omitted, the
+ *   current user is used.

How about "The user object for the user whose access is being checked", rather than "performing the operation"?

But since we're back to patching code, and we do plan to fix up ALL the doc after the code is OK, I wouldn't hold up the patch for this -- just add it to the rest of the doc fixup.

I've rolled another patch which attempts to combine the documentation and fix. (963656-patch-and-documentation.patch).

The additional changes since #90 are in 963656-diff-since-90.patch.

When you do interdiffs, just call them something.txt, so that the test bot doesn't choke on them.

I still think the doc needs some love and attention, because it's very confusing as it is... I may have time later today to take a stab at it.

Rerolled the patch. Didn't review or anything, just trying to make it pass for now.

+++ modules/node/node.api.php
@@ -143,6 +143,43 @@
+ * has no effect for other other operations like edit and delete.  If no node

Doube word (other).

+++ modules/node/node.module
@@ -3036,40 +3037,47 @@ function node_access_grants($op, $account = NULL) {
+  global $user;
+  if (!$account) {
+    $account = $user;
+  }

Minor, but couldn't/shouldn't global $user be inside the if-statement?

Powered by Dreditor.

I cleaned up the documentation a bit in the previous patch, and hopefully it's a bit clearer. Didn't touch the code.

And by the way, I'm generally in favor of putting globals at the top of the file rather than inside some if statement. It's easier to find them that way, and avoids the possibility that they might be needed again and be confused with a local variable elsewhere in the function.

For globals I personally prefer either the top of the function, but even better than that is just using $GLOBALS['user']; - for D8 I want to file a code style issue to make that consistent.

#100: 963656-100.patch queued for re-testing.

Not sure why this just failed, especially since it only differs from a previous passed test in comments only. Queued for retesting.

Also, I'm supportive of @jhodgdon's documentation changes. I think we've kicked around this issue enough, at least once it passes the test bot.

Okay, #100 passed this time. No idea why the testbot coughed a bit previously. I'm happy with the patch in #100. +1 RTBC.

I'm +1 on RTBC too.

Also note that this is a (needed) API change from the currently committed code (tagging).

Sorry, but unless someone can make the case for adding $account to node_access_view_all_nodes() being some sort of critical security regression, it's too late for this change. AFAIK that's been hard-coded since like Drupal 3.

Documentation improvements look good though.

@webchick why set this to needs work?

This is just a simple improvement, now that we have the $account stored on the query's meta we can use it in the access check (unlike D6 and earlier). I don't see why it's too late.

I think this qualifies as major at least; it would prevent queries run under the anonymous global $user (cron, or in some cases drush) from having proper node access, only showing itself when you have a node access module installed. In that light it is not "bugs that affect one piece of functionality" (normal) and rather "issues which have significant repercussions but do not render the whole system unusable" (major).

Not having $account is a security bug, since the node access query alter (the ONLY place this function is invoked) has an account associated with it that's not necessarily $user.

In D6, http://api.drupal.org/api/drupal/modules--node--node.module/function/nod... didn't have any way to specify the account.

Ah, ok. That makes sense, thanks. Sorry, it read more like "oh, while I was at it I found this!" and of course we're way past being able to commit patches like that now. :)

However, seems to no longer apply.

So.. does this mean we need to then verify $user->uid == $account->uid in _node_query_node_access_alter()? Otherwise this is, possibly, a security issue?

#100: 963656-100.patch queued for re-testing.

#100 was committed accidentally with the node type cache patch: http://drupal.org/cvs?commit=453544. Since #100 was RTBC, I am tentatively setting this to fixed.

OK, so does this break backward compatibility? Does it need to be announced? If so, please give a description of the impact on people.

@rfay, this should have a very minimal impact API wise, does not affect backward compatibility, and simply fixes the bug that if $account "meta" was set on a node access query, we use that instead of $GLOBALS['user'] when checking for a global view grant. node_access_view_all_nodes() works the same way as it did in D6, but now takes an optional $account argument.