hook_node_access() returning NODE_ACCESS_DENY is ignored

Question

I am trying to block the current user from performing any action (view, update, delete) on a node. The user should only be able to view, update, delete the node being displayed if he is the owner of the node, or the owner of the node is a subuser of his (I am using the Subuser module).

The function below returns the correct value for the various tests I performed, but when the function returns NODE_ACCESS_DENY, the access isn't denied. The user can still see the node.

Is there something I am missing?

function mymodule_node_access($node, $op, $account) {
  $type = is_string($node) ? $node : $node->type;
  if (is_object($node)) {
    if ($node->type == 'my_content_type') {
      // Determine the current uid and, if the user doesn't have the right role, deny the access
      if (is_array($account->roles) && in_array('vip', $account->roles)) {
        $current_uid  = $account->uid;
      } elseif ( is_array($account->roles) && 
                !in_array('administrator', $account->roles)) {
        return NODE_ACCESS_DENY;
      } else {
        return NODE_ACCESS_ALLOW;
      }

      // Determine the node owner.
      $node_owner = $node->uid;

      // If the node owner is the same as the current user, allow to view the node
      if ($current_uid == $node_owner) {
        return NODE_ACCESS_ALLOW;
      }

      // If not, check if the node owner is a subuser of the current vip user
      $node_owner_profile = user_load($node_owner);
      if (isset($node_owner_profile->field_parent[LANGUAGE_NONE][0]['value'])) {
        if ($node_owner_profile->field_parent[LANGUAGE_NONE][0]['value'] == $current_uid) {
          return NODE_ACCESS_ALLOW;
        } else {
          return NODE_ACCESS_DENY;
        }
      } else {
        return NODE_ACCESS_DENY;
      }
    }
  }

  return NODE_ACCESS_IGNORE;
}

Answer 1

There is an error in the code you are showing: A variable is initialized inside a if() statement, but it is used outside that statement. $current_uid is initialized from the following code.

  if (is_array($account->roles) && in_array('vip', $account->roles)) {
    $current_uid  = $account->uid;
  }

It is then used from the following code.

  // If the node owner is the same as the current user, allow to view the node
  if ($current_uid == $node_owner) {
    return NODE_ACCESS_ALLOW;
  }

That code only works if the account object passed to the hook has the vip role, or if the node has been created from the anonymous user (the one whose ID is 0). Differently, $current_uid will be NULL.

Also, the code I show in the second snippet is not doing what the comment says. It is not allowing the current user to view the node if the node was created by that user: It is allowing that user to view, edit, update, and delete the node. The only operation that code is not allowing is creating a node of that content type, since in that case Drupal would pass the content type string in $node and the code is checking $node is an object.

To achieve what the comment is describing, the correct code is the following one.

  if ($current_uid == $node_owner && $op == 'view') {
    return NODE_ACCESS_ALLOW;
  }

I would use the following code, but see the notes after the code.

function mymodule_node_access($node, $op, $account) {
  $type = is_string($node) ? $node : $node->type;
  if (is_object($node)) {
    if ($type == 'my_content_type') {
      $current_uid = $account->uid;
      $node_owner = $node->uid;
      if (is_array($account->roles)) {
        if (!in_array('vip', $account->roles) && !in_array('administrator', $account->roles)) {
          return NODE_ACCESS_DENY;
        }
        elseif (in_array('administrator', $account->roles)) {
          return NODE_ACCESS_ALLOW;
        }
      }
      else {
        return NODE_ACCESS_IGNORE;
      }

      if ($current_uid == $node_owner && $op == 'view') {
        return NODE_ACCESS_ALLOW;
      }

      $node_owner_profile = user_load($node_owner);
      if (isset($node_owner_profile->field_parent[LANGUAGE_NONE][0]['value'])) {
        if ($node_owner_profile->field_parent[LANGUAGE_NONE][0]['value'] == $current_uid) {
          return NODE_ACCESS_ALLOW;
        } else {
          return NODE_ACCESS_DENY;
        }
      } else {
        return NODE_ACCESS_DENY;
      }
    }
  }

  return NODE_ACCESS_IGNORE;
}

Keep in mind the following notes reported in the documentation for hook_node_access(), especially the second one.

Note that not all modules will want to influence access on all node types. If your module does not want to actively grant or block access, return NODE_ACCESS_IGNORE or simply return nothing. Blindly returning FALSE will break other node access modules.

Also note that this function isn't called for node listings (e.g., RSS feeds, the default home page at path 'node', a recent content block, etc.) See Node access rights for a full explanation.

Keep also in mind that for users with the bypass node access permission, none of the hook_node_access() implementations are invoked. If the tests are done with accounts having that permission, that would explain why the user is able to act on the nodes even if the code is not giving the permission.

Instead of checking the roles of the user account, I would rather check if the user has a specific permission. hook_node_access() implementations are allowed to use user_permission().

Answer 2

My final solution is in fact a workaround. I have adapted the code as necessary and used it in template_preprocess_node The code is adding a new variable to the $variables array - called $access_allowed.

In node--content-type.tpl.php, I check that variable. If TRUE, I display the node. Otherwise, I redirect to the 403 page.

I know it's not as elegant as the solution using hook_node_access, but, for now, it works for me. If anyone finds a better solution, it would be great!

Thanks