anonymous disappears from node_access table, gets access denied for all nodes

I've also experienced this problem with drupal 6.4

When posting a story (or comment?) Drupal issues INSERT INTO node_revisions ... query. If it fails due to duplicate key Drupal catches the error, but handles it in a peculiar way: at first it logs it (INSERT INTO watchdog ...), and then does DELETE FROM node_access WHERE nid = 0. This command renders the site inaccessible to anyone but root user. For root user it shows the normal site and for others it displays a page shown when Drupal is just installed and has no contents: "Welcome to your new Drupal site bla-bla-bla."

Well, it is true that INSERT INTO node_revisions ... should never fail. However it may do so in MySQL: http://bugs.mysql.com/bug.php?id=41984. And surely, DELETE FROM node_access WHERE nid = 0 does not seem a right thing to do regardless. It seems that Drupal site cannot exist without this row.

Attached excerpt from the mysql query log showing the problem.

Has anyone found a fix for this? I've just done the same upgrade and am having the same problem with anonymous access. It's driving me nuts!

Rebuilding user permissions on Post Settings sorted this for me

I think the option to rebuild the permissions table is only available on the "post settings" page if the content access module is installed.

I have also ran across this problem, and after looking at the code and a bit of testing, I highly doubt that rebuilding permissions is a permanent solution. My problem arises in this manner: I have two content types, one uses workflow and workflow_access to restrict edits, the second does not use a workflow at all. Here's the first piece of the puzzle from node.module

<?php
function node_access_acquire_grants($node) {
  $grants = module_invoke_all('node_access_records', $node);
  if (empty($grants)) {
    $grants[] = array('realm' => 'all', 'gid' => 0, 'grant_view' => 1, 'grant_update' => 0, 'grant_delete' => 0);
  }
  else {
    // retain grants by highest priority
    $grant_by_priority = array();
    foreach ($grants as $g) {
      $grant_by_priority[intval($g['priority'])][] = $g;
    }
    krsort($grant_by_priority);
    $grants = array_shift($grant_by_priority);
  }

  node_access_write_grants($node, $grants);
}
?>

It appears that the default view permission is only granted if no other modules are granting permissions. Since workflow_access is giving grants sometimes the default view grant is not given. A look at node_access_write_grants() shows the death blow for the default view grant

<?php
function node_access_write_grants($node, $grants, $realm = NULL, $delete = TRUE) {
  if ($delete) {
    $query = 'DELETE FROM {node_access} WHERE nid = %d';
    if ($realm) {
      $query .= " AND realm in ('%s', 'all')";
    }
    db_query($query, $node->nid, $realm);
  }
...
?>

So it looks like delete is always gonna be true in this invocation and regardless of whether realm is set or not, the all grant gets deleted. What I haven't traced down yet is why rebuilding permissions restores the grant to it's proper state. I do know, however, that subsequent edits will break the permission again. I'm not really sure the correct approach to solving this problem, that's why I haven't supplied a patch. I think the default behavior of Drupal is confusing and leaves a lot of corner cases for access control modules to cover. I'm going to look over the rebuild permissions code and see if I can find any insight there, then I'll check back with some possible solutions.

After further investigation, I found that I had a misbehaving access control module. The behavior of Drupal core is correct and matches the documentation. My opinion is that some module developers do not understand how the node_access system works and are creating modules with incorrect assumptions about it.

I can replicate the issue when node saving fails for issues like unsupported characters. I am reading from csv file and creating nodes for each line.

Code:

$importNode = new StdClass();
$importNode->uid = $user->uid;
$importNode->name = (isset($user->name) ? $user->name : '');
$importNode->type = 'employee';
...
node_save($importNode);

if node_save fails, that is $importNode->nid =0, in db log I see a call "delete from node_access where nid=0".
When node_save is successfull, in db log I see a call "delete from node_access where nid=165" (or what ever the node id is)

So I guess, there should be a check that nid>0 when deleting from node_access table, as 0 is a default value for integer and unfortunatelly a special value for node_access table.

I was led here by plj in #124137-32: Access denied to anonymous users (was: Node access settings). Some of the posters there may be experiencing this issue here.

I'm not sure what exactly can fail in node_save() that will lead to $node->nid remaining 0, but it's certainly true that the node_access_acquire_grants($node); call near the end of node_save() will cause problems if $node->nid==0.

node_save() could well be made more robust, but I'm not sure there's a consensus that that's the way to go. We could also argue that whoever causes node_save() to fail needs to clean up their act...

Here's a simple patch based on salvis' investigation. I agree that in the ideal case we would figure out why this is happening. On the site where I encounter it the most likely cause is nodes being imported by feeds module. The problem comes up irregularly which makes it really hard to debug, of course, and based on the discussions here that seems fairly common.

So I propose that we just make node_save more robust. The comment is trickier here than the code.

Thanks for picking this up, greggles!

Do your comment lines go beyond col 80?

Can a node that won't load be displayed at all? If we don't node_access_acquire_grants(), the node becomes inaccessible to everyone except user 1 and users with the 'administer nodes' permission. This should probably be mentioned in the comment.

Should we write a watchdog entry? I imagine that these nodes can cause all sorts of trouble (for search, for example), and the admin should be made aware of them. This might also lead to further investigation and create some pressure on the modules that cause this problem in the first place.

If we don't have a nid how can we watchdog anything useful?

True, especially because we can't add new strings. Also, it might make more sense to display a message immediately, so that it appears in the proper context.

There's "The post could not be saved." — can we do something with that?

Since we know that something is wrong, it would be good to provide some kind of indication of that fact and give the site maintainer a fighting chance. But I don't have any good ideas...

I'm falling into the problem described here...
Attached the greggles patch revisited in the git way, I hope it will pass the testing.
I also hope it will be inserted into the 6.21 version.

This is still an issue. One of our sites had this grant removed after a poorly timed mysql error. This patch looks great to me, and doesn't cause any ill effects I can see. Could we still get this in?

Looking at the patch again the only complaint I have is that it could be better to use if (!empty($node->nid)) but I'm not sure if that provides real benefit.

Comments still extend beyond col 80.

I think if $node->nid is empty then we were unable to save the node — we didn't get a nid.

  drupal_set_message(t('The post could not be saved.'), 'error');

might be appropriate.

Not sure whether $node->nid is set, so !empty() would probably be better.

I'd rather take inspiration from this thread http://drupal.org/node/124137#comment-1991686. We should put the check into node_access_write_grants() itsself as third party modules, e. g. like workflow_access or apachesolr might call this function directly. Further there should be error message written to the screen and the watchdog (as wished in comment #19).

@gnindl:

I'd rather take inspiration from this thread http://drupal.org/node/124137#comment-1991686. We should put the check into node_access_write_grants() itsself as third party modules, e. g. like workflow_access or apachesolr might call this function directly.

You can take your inspiration from wherever you want, but a.d.o clearly says for D7 that no contrib should call node_access_write_grants() directly. This applies to D6 just the same. The fact that it's not mentioned explicitly for D6 is a documentation bug.

Any contrib that calls node_access_write_grants() directly is abusing the node access system and is incapable of successfully cooperating with other node access modules.

You can take your inspiration from wherever you want, but a.d.o clearly says for D7 that no contrib should call node_access_write_grants() directly. This applies to D6 just the same. The fact that it's not mentioned explicitly for D6 is a documentation bug.

Well if that's a rule we should stick to that. Unfortunately there's nothing preventing you from calling this function directly. This would be different if we had access control modifiers, like "protected", with a proper OO design. So you can bitch around this as much as you like, but we let a potential door open for abuse.

But OK, let's imagine all community members stick to that rule and read the documentation which I may quote here:

Note: Don't call this function directly from a contributed module. Call node_access_acquire_grants() instead.

Unfortunately in D6 the function node_access_acquire_grants() doesn't check the node object as well! Therefore I recommend to check the parameter for all possible use cases, not just for node save operations. Introducing a parameter check before running a SQL DELETE statement sounds like common sense.

Unfortunately in D6 the function node_access_acquire_grants() doesn't check the node object as well!

That's what #16 tries to fix. While we're touching the file, we should fix the documentation error (#22) as well.

Passing a $node with $node->nid==0 is a programming error, not a user error. This has been discussed elsewhere: We don't display error messages to the user for programming errors, and we certainly don't introduce new strings in D6 anymore.

(Not being able to "wipe out" existing grants is not an error, as there may (rightfully) not be any for a given node. Also, you have trailing spaces and non-conforming comments.)

I'm not sure it makes sense to pursue this at all, unless you're willing to start with D8 and work your way backwards through D7 to D6.

I am experiencing this as well and would like to to know if this will be committed.

Clean up of patch #26:
- Still check in node_access_write_grants() because modules like workflow_access call it directly. It's a fact and the modules and the code have been there for years.
- Write to watchdog only: it's admin stuff.
- Some coding standard fixes.

I don't care if it get committed directly in D6. I experienced this problem and want to share a patch. The bug makes the whole site unusable, so it's at least major.

Improvement of patch #26: Actually the node id should be checked if it's an integer value (>0), otherwise empty strings or the value 0 might wipe out the entry. This way deleting node id 0 in node_access table is made impossible.

Amended patch in comment #27, added is_int() check.