node_access does not respect uid

I am working on an SSE feed module for Drupal and have run into the same problem. There does not seem to be a way for one user (even the main admin or a module) to generate content for another user (even the anonymous user) because there is no obvious way to determine what will be visible to another user. This seems to mean that content can only be generated on the fly when the user viewing it is waiting. This can be a problem for cached feeds and such like.

Is there a "become" function where a module can change the current user to another user temporarily? Would it be safe to muck with the $user global variable and then restore it later?

Can some code (or directions) be provided to help following this possible bug? Is this a core problem?
Seems so abstract that I'll bet that if this is a bug it will be present in HEAD.

I'm having this problem as well. I didn't notice until I utilized simple_access for a category being managed through subscriptions.module (http://drupal.org/node/13502). Folks are getting subscription emails on nodes for which they don't have 'view' access, even though there's a check in subscriptions for just that.

Got it! If the user has admin privileges and they trigger the node_access function, *they* (the admin users) are the reason that it is ignoring individual user privileges. Here's why:

<?php
function node_access($op, $node = NULL, $uid = NULL) {
  // Convert the node to an object if necessary:
  if ($op != 'create') {
    $node = (object)$node;
  }
  // If the node is in a restricted format, disallow editing.
  if ($op == 'update' && !filter_access($node->format)) {
    return FALSE;
  }

  if (user_access('administer nodes')) {
    return TRUE;
  }

  if (!user_access('access content')) {
    return FALSE;
  }
?>

See how it returns TRUE if the user triggering the function has "administer nodes" permissions? It's not even taking into account what the permissions are of the $uid that's been passed in. One short fix, and...

<?php
function node_access($op, $node = NULL, $uid = NULL) {
  // Convert the node to an object if necessary:
  if ($op != 'create') {
    $node = (object)$node;
  }
  // If the node is in a restricted format, disallow editing.
  if ($op == 'update' && !filter_access($node->format)) {
    return FALSE;
  }

  if (!$uid && user_access('administer nodes')) {
    return TRUE;
  }

  if (!$uid && !user_access('access content')) {
    return FALSE;
  }
?>

This seems to have a logical basis, and so far (from what I've seen), the corrections are having the intended effect.

Should that be:

!isset($uid)

rather than

!$uid

There. IMO Moshe should review this before commit.

So Moshe looked at the issue and asked what about all the hook_access implementation and he is right.

i did some testing and all still works fine ... not sure when we broke this but i'm sure happy to have it fixed.

We never broke this -- this never worked! The history of this $uid is that node_access_grants (removed in 4.6) once had a $uid parameter thus Gerhard added $uid to node_access in http://drupal.org/10884 . But it was never implemented properly, it was only passed on to node_access_grants

So. An alternative to fixing hook_access deep into the code freeze is to remove $uid from node_access by applying the attached patch and apply the above fix in the Drupal 6 cycle. Those who need this functionality will need to change users -- yuck. But, they needed to change users in the past, so actually -- no change.

It's up to the powers above me to decide where they thaw the code freeze -- at node_access or at hook_access.

Committed chx' last patch. If it has been broken for that long, we can do another release without it.

#7 could be candidate for Drupal 6.

The patch in #7 does still apply to HEAD

this is a reroll of #7. it had 1 junk failed in node_content_access().

i tested and the patch works as advertised. this is actually a really important change. without this, modules have to impersonate users before sending them an email notification, for example. thats the only way for a module to check if recipient has view access for the node.

How come a $uid get into node_access? I don't see any $uid variable used in node_access neither at api.drupal.org, neither my latest cvs HEAD checkouts.

patchin #14 fails on book module since book_access() no longer exists.

re-roll without the book module part - also, Gabor makes an astute observation that the $uid param was removed from node_access() in a previous patch. So, this adds it back

I tried to look up the blame logs to find a reason why this was actually removed, but it is not behaving like I expected it to :] It would be good to see the issue. Anyway, It would be inconsistent to have $uid here, and $account at other hook_access() functions.

uid was part of the function since the beginning but it had a bug which was discovered very late in the release cycle (5.x i think). so rather than fix the bug and possibly create more breakage, we decided to remove the feature since it never worked ... so, lets get it back in.

The 4.7.x versions of node_access still has $uid as a param, so I just went back to that.

Also, here it is the really is the user ID, not the user object - so the difference in variable names is correct. See in the patch:

+  $account = isset($uid) ? user_load(array('uid' => $uid)) : $GLOBALS['user'];

Yes, I understand there is a difference now, but why should there be a difference? As far as I see, it looks confusing/inconsistent.

@Gabor - are you suggesting that the 3rd param should be a user object rather than a user ID? Your comment is still not making sense to me.

I did not take sides, because it matters less, how it is made consistent, but it matters a lot more to have it consistent. If the user id is enough for all places, then lets just have the user id. If the user object is required at some, then have that. If that is too complicated to have the user object for some calls, then we might need to think about this more, but then otherwise go with the simplest *and* more consistent solution.

Excuse me if this direction was not clear.

since the calls to user_access() require a full user object - if we are going to insist on consistency, it makes more sense to pass in the loaded object.

ok - node_access() takes $account now - anyone want to test?

If the SQL in node_access does not change -- or at least the grant invoke -- this can't be right.

chx - you are correct - function node_access_grants() now has $uid as a param, but that is not passed in, and we ought to pass in the already-loaded user object.

ok, I think this one is more thorough.

Patch still applies with some fuzz. This reroll removes the fuzz.

I tested the patch while running the node_access_example module and all is well. Then I ran the following fragment by passing different nids and got the expected results.

return node_access('view', node_load(1), $GLOBALS['user']);

Generally looks good as far as I see. But

- there is a small whitespace issue in the phpdoc added to node_access()
- we already global $user in node_access (do we use it somewhere there?), so why use $GLOBALS['user'] later on in the function?

Fixes both problems mentioned by Gabor.

Because this question was not answered, now I looked into node_access() and it does seem to use $user to give permission to the author to view a node even if not published yet. That should be affected by the $account param, right?

fixed ... good catch.

Thanks, committed.

(patches that are already committed will come back failed, since you'd be patching HEAD twice)

I added a note to update docs about this since some modules like project_issue and og can now check access before sending email notifications (for example).

@RobinMonks - it would be a great if the Tester could check the project status before running a test and posting irrelevant results (i.e. issue is in fixed state).