I was having trouble accessing nodes inside the nodequeue and it led me to discover that, from what I'm seeing right now, the access check for nodes is wrong. If the user doesn't have "administer nodes" permission, you forcefully restrict all unpublished nodes. There are many conditions and variables involved in node access; even with unpublished nodes.

Here's the query that's being ran:

SELECT DISTINCT n.nid AS nid
FROM 
{node} n
LEFT OUTER JOIN {nodequeue_nodes} nq ON nq.nid = n.nid
WHERE  (nq.sqid = :db_condition_placeholder_0) AND( (n.status = :db_condition_placeholder_1) OR (n.uid = :db_condition_placeholder_2) )
ORDER BY nq.position ASC

I don't see any joins on the node_access table.

For instance, I'm using Workbench Moderation, and certain roles are able to see unpublished nodes so they can moderate them. When they view the node queue, the node titles are excluded, but the other action links are there.

Perhaps a call to node_access() should be used for each node?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mstef’s picture

Status: Active » Needs review
FileSize
2.19 KB

How's this?

ezra-g’s picture

Why not just remove:

  if (!user_access('administer nodes', $account)) {
    $query->condition(db_or()->condition('n.status', 1)->condition('n.uid', $account->uid)); 
  }
mstef’s picture

Then you miss out on all this good (/important) stuff:

http://api.drupal.org/api/drupal/modules%21node%21node.module/function/n...

mstef’s picture

Looking at that query, I don't see any "access"-checking other than the published filter.

mstef’s picture

Ah shoot.. screwed up that patch..new one coming

mstef’s picture

mstef’s picture

I'm starting to think this may even be a security issue..

Status: Needs review » Needs work

The last submitted patch, nodequeue-proper_node_access_check-1871816-6.patch, failed testing.

ezra-g’s picture

Looking at that query, I don't see any "access"-checking other than the published filter.

That's what

->addTag('node_access')

is for.

In general, if you suspect you've found a security issue, please follow the process for reporting a security issue.

mstef’s picture

I saw the addTag but after printing the query, nothing for node access is added. Also, it's not doing what it is suppose to do, which is why I opened this issue.

mstef’s picture

Status: Needs work » Needs review

Odd reason the test failed. Lets try again.

mstef’s picture

Same patch as #6, for another run at the tests.

  • fizk committed 0579045 on 7.x-2.x
    Issue #1871816 by mstef: Node access checking is wrong
    
fizk’s picture

Issue summary: View changes
Status: Needs review » Fixed

Committed, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.