Unpublished content visible when a Node Access module is enabled

The core check for access to personal unpublished nodes occurs before Domain Access fires. Check your permissions settings.

  // Check if authors can view their own unpublished nodes.
  if ($op == 'view' && !$node->status && user_access('view own unpublished content', $account) && $account->uid == $node->uid && $account->uid != 0) {
    $rights[$account->uid][$cid][$op] = TRUE;
    return TRUE;
  }

If this is not the case, this would seem to be a core bug in node_access().

http://api.drupal.org/api/function/node_access/7

Core bug. This is a regression from Drupal 6. The {node_access} table has no knowledge of publication status, so this query needs a condition on View, or we have to re-think how we handle this aspect of the system.

Might be that Node Access modules are required to return NODE_ACCESS_IGNORE for the 'view' op if the $node->status = 0.

Changing title.

It will work, but it likely is the wrong solution, because it won't allow Node Access modules to allow editing / deleting of unpublished nodes.

Tagging.

This patch kills #452538: Enable Node Grants for Unpublished Nodes

@chx

For edit and delete, yes. But we either have to:

a) Extend {node_access} (or $op) to include new grant states.

b) Write in an exception for 'view' in the node_grants subroutine.

You can still access unpublished nodes using hook_node_access(), since the code snippet from #1 occurs first.

Another try.

I am studying on how to write a patchtest for this.

@agentrickard please see the issue I linked. It's definitely desired that node access module control unpublished nodes.

'grant_view' is not sufficient to cover the two publication states of a node.

To do that would require an API and schema change, introducing {node_access}.grant_preview, because Node Access modules currently have nothing to query against in the {node_access} table that represents the publication state of the node.

Alternately, we could add a JOIN to the {node} table on the View state here, but doing so will not allow node access modules to control 'preview' grants.

With the API in it's current state, modules have to use hook_node_access() to enable the viewing of unpublished nodes. That happens before Node Access API is invoked.

I am shocked that no one saw this prior, although it came up during Q&A at the Node Access talk Copenhagen, when I mentioned that Node Access modules cannot control access to unpublished nodes.

Note: 'preview' == 'view_unpublished'

New patch. This changes the API so that modules can now return three values for hook_node_access_records() and hook_node_grants():

0 == no access
1 == access if node is published
2 == access if node is unpublished

This applies to all 3 $op values (view, update, delete). Good news is that all existing modules currently return 1, so we won't have accidental privilege escalation.

TODO:

1) Tests
2) Documentation

This approach cannot work because hook_node_grants() only returns the grant_ids, not their value.

Alternate patch that adds a new {node_access}.view_unpublished column.

Let me summarize where we stand. There seem to be four options.

1) Revert to D6 standards, where node access modules have no say for unpublished nodes.

2) Disallow node access modules from responding to 'view' for a _single node_ when it is unpublished. In this case, modules that want to allow this behavior have to use hook_node_access(). This is the approach in #10.

3) Rewrite both hook_node_access_records() and hook_node_grants() to account for both published and unpublished states. This is the approach started in #15.

4) Add a new grant to the node_access table and to hook_node_grants() and hook_node_access_records(). This is the approach in #17, and is actually less intrusive than it sounds, since it would not force changes in contrib.

Personally, I vote for option #2 or #4. The fact of the matter is that $op is broken, and 'view' is simply not a sufficient context for nodes that have a published/unpublished state.

chx's approach in #11 doesn't actually address the issue at hand.

Can we give a use case that’s hard to solve with current system? AFAICT, node access modules are now fully responsible for granting access. In general, they should not be granting access to unpublished nodes. Thus, they should inspect $node and not return grants if unpublished.

For example, if I enable node_access_test.module (use drush to do this), a user still can't see unpublished nodes. That’s because this module doesn't grant access to those. Seems OK to me.

In D7, node access modules have greater powers and need to use them responsibly - be more careful in write_records.

So a documentation issue then?

So the idea is to make hook_node_access_records() implementations responsible for checking for $node->status?

It seems reasonable to do so, but node_access_test.module doesn't!

So #452538: Enable Node Grants for Unpublished Nodes recommended if (!$node->status) { return; } in hook_node_access_records. This is what domain_access does. That does not seem to work too well.

node_access_test module does not test node status at all and so if you have the 'node test view' permission you can view unpublished and published nodes both.

Finally, agentrickard pointed out on IRC that hook_node_grants does not even get $node.

This together makes me think that we have a valid bug to fix not just a docs issue.

Ok, so the actual problem is that node_access_acquire_grants() grants access to a node if no grant is returned from any hook_node_access_records() implementation:

  if (empty($grants)) {
    $grants[] = array('realm' => 'all', 'gid' => 0, 'grant_view' => 1, 'grant_update' => 0, 'grant_delete' => 0);
  }

That means that node access implementations have no way to differentiate between "I don't care about this node" and "I care about this node but I don't want anyone to access it".

What I suggest is that we ask node access implementation to return a dummy "deny all" grant like this for that later case:

  $grants[] = array('realm' => 'all', 'gid' => 0, 'grant_view' => 0, 'grant_update' => 0, 'grant_delete' => 0);

In case you were wondering node_access_write_grants() only save positive grants to the database, so those dummy grants will never be saved. They will just prevent node_access_acquire_grants() from granting access to the node to everyone.

This is a beginning of a test for what DamZ suggests. It also establishes a very nice easy pattern.

That makes this mostly a documentation issue.

Here is a suggested change.

Well, that deny-all in the example is not guaranteed to trump other grants that might have been returned. I suggest you add a high priority to that grant as well.

I don't really get how this handles the 'I don't care' case. If no modules care, then the allow-all grant gets written and that allows anyone to view unpublished. I think node_access() has to change so it denies unpublished in the 'noone cares' case (even when we are using the grants system).. MW: Jibberish removed

Subscribe.

I see what DamZ is getting at in #24, but the dummy grant will not be written to the database, due to the logic in node_access_write_grants(). And setting it for gid 0 is also incorrect. It has to have gid = $node->nid.

      // Only write grants; denies are implicit.
      if ($grant['grant_view'] || $grant['grant_update'] || $grant['grant_delete']) {
        $grant['nid'] = $node->nid;
        $query->values($grant);
      }

On the other hand, chx is right and Domain Access does account for 'unpublished' nodes, though not sufficiently, since we probably do want to allow editing and deletion of unpublished nodes.

So the key would be to have core submit a deny grant for unpublished nodes that applies to all node access grants.

The attached patch works as expected when I remove this from domain_node_access_records(): (!$node->status) { return; }. I had that there because I wasn't sure what else to do, and, in fact, I want to allow edit and delete access to unpublished nodes in some cases.

This patch to core simply forces grant_view to be set based on $node->status, at the API level.

But I'm still not certain that this will be sufficient to satisfy #452538: Enable Node Grants for Unpublished Nodes, because it will force modules to use hook_node_access_records() to allow access to view unpublished nodes. (Which is still an improvement over D6). The only way to allow this at the {node_access} table level is to alter the API storage.

Setting to needs review.

@moshe -- If no modules respond, no record is written for this specific nid, and this bug only occurs when doing a direct query by nid inside the node_access() function.

    if (module_implements('node_grants')) {
      $query = db_select('node_access');
      $query->addExpression('1');
      $query->condition('grant_' . $op, 1, '>=');
      $nids = db_or()->condition('nid', $node->nid);
      if ($node->status) {
        $nids->condition('nid', 0);
      }
      $query->condition($nids);
      $query->range(0, 1);

      $grants = db_or();
      foreach (node_access_grants($op, $account) as $realm => $gids) {
        foreach ($gids as $gid) {
          $grants->condition(db_and()
            ->condition('gid', $gid)
            ->condition('realm', $realm)
          );
        }
      }
      if (count($grants) > 0) {
        $query->condition($grants);
      }
      $result =  (bool) $query
        ->execute()
        ->fetchField();
      $rights[$account->uid][$cid][$op] = $result;
      return $result;
    }

The argument for the above patch is that setting grant_view on an unpublished node is a security violation, so we cannot trust contributed modules to enforce the logic. We must do so at the API level. Modules that want to override this must explcitly do so in hook_node_access().

@agentrickard: the dummy grant will not be saved to the database, but that doesn't matter, at all.

The only non-documentation change required is:

   if (empty($grants)) {
-    $grants[] = array('realm' => 'all', 'gid' => 0, 'grant_view' => 1, 'grant_update' => 0, 'grant_delete' => 0);
+    $grants[] = array('realm' => 'all', 'gid' => 0, 'grant_view' => $node->status, 'grant_update' => 0, 'grant_delete' => 0);
   }

So, there are three use cases:

• No node access module care about the node: $grants is empty and the above rule prevents access to unpublished nodes
• A node access module cares about the node and wants unpublished nodes to be hidden: it returns something like array('realm' => 'myreal', 'gid' => 3, 'grant_view' => $node->status, 'grant_update' => 0, 'grant_delete' => 0)
• A node access module cares about the node and wants unpublished nodes to be accessible in some cases: it returns something like array('realm' => 'myreal', 'gid' => 3, 'grant_view' => 1, 'grant_update' => 0, 'grant_delete' => 0)

@DamZ

Edited post:

Hm. we'll need to test that extensively.

Original post:

No. Because saving the gid=0 data that way will break core. Because normal node listing queries (when no node access modules exist), join against that record. The realm 'all' with gid '0' applies to all nodes when no node access modules exist.  Doing that would essentially lock access to all nodes on a site unless a node access module were present.

See _node_query_node_access_alter().

The issue is that we have to account for both listing queries -- which should already be filtered by $node->status -- and individual queries -- which are not.

The only place the individual query exists is inside {node_access}.

No, the problem is still in case 3 from post #32. In the current system, a node access module does not have enough context to know whether to grant access to an unpublished node. Here's the Domain Access use case:

-- Grants are made not per user, but contextually per domain. So if a node is visible on Domain A, any user can view it on Domain A, regardless of role or permission.
-- We want editors assigned to domain B to be able to edit or delete any node on Domain B, regardless of publication status.
-- We save a grant, grant_view = $node->status, grant_update = 0, grant_delete = 0.
-- This works fine, except that we cannot grant the editor 'view' status of the node without resorting to hook_node_access_records().
-- If we try to save grant_view = 1 to allow access to unpublished nodes, then our invocation of hook_node_grants() has to be smart enough to arbitrate between the two classes of user. But it cannot be, since the grant request has no knowledge of the publication state. So we end up granting access to a class of user that we should not.

One possible solution to that is to pass node status as part of nook_node_grants().

The overall issue is that the ability to apply hook_node_grants() to unpublished nodes essentially means that we have gone from 3 publication states (view, update, delete) to 6 (view, view_unpublished, edit, edit_unpublished, delete, delete_unpublished).

No, the problem is still in case 3 from post #32. In the current system, a node access module does not have enough context to know whether to grant access to an unpublished node.

It's up to the node access module to decide if it wants to give access to unpublished nodes. Generally, most of them will not (domain access clearly will not).

[edit] ie.: you don't need any context. It's a design decision of the node access module to give access to unpublished nodes or not, and to which realm/gid pair to grant that right.

So now you're asking node access modules to create a new realm for every node, just to handle the unpublished state?

Seems like that's a worse solution.

DamZ and I hashed this out in IRC. It will be up to the node access module to use a realm to account for 'unpublished.' In most cases, including mine, this will not be done, and unpublished nodes will have grant_view = 0.

Working on documentation now.

OK, here's DamZ's patch, with appropriate documentation. Likely needs tests.

Dang it, let's try that again.

Above patch, with a working test. (Also moves and documents a test that was in the wrong place.)

The test fail is odd, fixed by changing the logic in node_access_acquire_grants(), which may be preferable anyway. Added documentation clarification at chx's suggesting in IRC.

I am wondering whether we could do a HEAD / DRUPAL-7--1 module checkout, grep for access_grants and post a critical in their queue to get the maintainer aware.

We can have Randy Fay send a note to the devel list, too.

The test above isn't right, it doesn't check the proper condition. I need to refactor that code.

Proper test that caught a foreach error when $grants is passed empty to node_access_write_grants().

New test properly leverages the existing node_test.module for node access records testing.

sending a word to the devel list about a sechole? is that enough?

Well, it's a potential security hole, but only if module developers have the kind of mental block that I had on this issue -- that is, not realizing you need a new realm to handle access to unpublished nodes. We fixed the problem in core, which was the actual security hole.

We don't normally take great strides to notify developers of such things.

This was a bit of a special case for two reasons, I think:

1) I already ported Domain Access to Drupal 7.

2) In that port, I deliberately didn't handle 'unpublished' nodes, since I hadn't figured out what to do about them.

I would expect that other modules may not have similar problems.

Notifying maintainers, though, is a separate issue. Patch still needs review.

I'm not very familar with the node access system but the new code and comments read pretty well.
This one is over 80 chars though:

+    // Check that core does not grant access to an unpublished node when an empty $grants array is returned.

Looks good. One comment on the docs:

+++ modules/node/node.api.php	27 Sep 2010 13:46:34 -0000
@@ -197,7 +237,7 @@ function hook_node_access_records($node)
-      'grant_view' => 1,
+      'grant_view' => $node->status,

IMO, this 'private' example should check $node->status and simply not return a record at all if unpublished. If this example doesn't care about unpublished (as the comment suggests), then it should stay neutral and let other modules and core sort it out.

Fixed the inline comment and added clarity to the docblock.

Ready to fly once bot says green

+++ modules/node/node.api.php	28 Sep 2010 14:27:07 -0000
@@ -194,17 +234,22 @@ function hook_node_access_records($node)
+    // Only published nodes should be viewable to all users. If we allow access
+    // blindly here, then all users could view an unpublished node.
+    if ($node->status) {
+      $grants[] = array(

Better to write: if ($node->status === NODE_PUBLISHED) {?

+++ modules/node/node.module	28 Sep 2010 14:27:10 -0000
@@ -3156,8 +3156,8 @@ function node_access_acquire_grants($nod
-  // If no grants are set, then use the default grant.
-  if (empty($grants)) {
+  // If no grants are set and the node is published, then use the default grant.
+  if (empty($grants) && !empty($node->status)) {
     $grants[] = array('realm' => 'all', 'gid' => 0, 'grant_view' => 1, 'grant_update' => 0, 'grant_delete' => 0);

!empty($node->status) means that the node is not stored in the database yet. A node that is stored in the database with status NODE_NOT_PUBLISHED would also be 'unpublished'. Sounds like the code comment might be inaccurate?

Powered by Dreditor.

I don't agree with the constant comparison - we don't do that anywhere else.

Not sure whether the other remark is valid. !empty($node->status) does not indicate whether a node is stored in the database or not.

!empty($node->status) means that the node is not stored in the database yet.

$node->status is set via the node form, or via the function calling node_save(), it has no relationship to whether the node is in the database or not. The only two checks for this are $node->is_new and $node->nid.

With the constant comparison, if we wanted to do that, it needs to be in its own code style issue, I don't see any reason to hold this up on either of these, so marking back to RTBC.

You're right. Committed to CVS HEAD. Thanks.

Subscribing