I'm using node_access in a straighforward but not common way. For the vast majority of my nodes, I want the default permission scheme to apply. However, for a few nodes I'd like to allow all users to edit. I use tac_lite to set the grant_update record for just these few nodes.

So what I'd like is a node_access table with just a few entries. The default entry which sets grant_view for node "0" (the one written in node_access_rebuild), plus one row for each of my special nodes which sets the grant_update column.

But the current logic of node_access_rebuild is that if any module provides any node_access records, the default grant_view row is not written. So the effect is that I need to add a grant_view row for each node on my site. This makes the difference between having just a handful of rows in node_access versus thousands of rows, and there is a performance penalty for that.

So this patch changes the logic in node_access_rebuild to the following:

First invoke hook_node_grants for each node.

If (no module has provided a grant_view permission for any node)
    Then write the default grant_view row.

The attached patch is against Drupal 5.x, because that's the site I'm working on. However if the spirit of the patch is acceptable, I will re-roll against 6x and 7x.

CommentFileSizeAuthor
#1 node_access_view.diff554 bytesDave Cohen
node_access_view.diff1.52 KBDave Cohen

Comments

Dave Cohen’s picture

Dave Cohen commented
Title: Improve node_access rebuild to keep node_access table small » Improve node_access_rebuild to keep node_access table small
StatusFileSize
new node_access_view.diff554 bytes

Sorry, the previous patch had an unrelated diff. Here's node_access_rebuild after the patch:

function node_access_rebuild() {
  db_query("DELETE FROM {node_access}");
  // only recalculate if site is using a node_access module
  if (count(module_implements('node_grants'))) {
    // If not in 'safe mode', increase the maximum execution time:
    if (!ini_get('safe_mode')) {
      set_time_limit(240);
    }
    $result = db_query("SELECT nid FROM {node}");
    while ($node = db_fetch_object($result)) {
      $loaded_node = node_load($node->nid, NULL, TRUE);
      // To preserve database integrity, only aquire grants if the node
      // loads successfully.
      if (!empty($loaded_node)) {
        node_access_acquire_grants($loaded_node);
      }
    }
  }

  $result = db_query("SELECT * FROM {node_access} WHERE grant_view > 0");
  if (!db_num_rows($result))
    // No modules are providing view permissions.  Add default grant.
    db_query("INSERT INTO {node_access} VALUES (0, 0, 'all', 1, 0, 0)");
  
  cache_clear_all();
}
kbahey’s picture

kbahey commented

I like this.

I have seen cases where a site is fine until a rebuild is done and then the node_access tables has a lot more rows that it originally had. This in turn causes slowness for many queries due to temporary tables being required and sifting through a lot of rows.

salvis’s picture

salvis
he/his
commented

But the current logic of node_access_rebuild is that if any module provides any node_access records, the default grant_view row is not written.

This is not correct. node.module supplies the nid/all/0/100 grant for each node that doesn't receive any other grant. IOW, *you* don't need to supply it.

What is happening is that if no node access module is active at all, node.module supplies a single 0/all/0/100 record and effectively disables the node access mechanism. If you enable any node access module, even if it doesn't supply any grants at all, then the default nid/all/0/100 grant is created by node_access_acquire_grants().

Your stated goal

So what I'd like is a node_access table with just a few entries.

is an interesting one, but I don't see how your patch would help.

Your patch could only make a difference if some module would supply at least one grant for every node (which is exactly what you want to avoid), but absolutely no view grant.

Dave Cohen’s picture

Dave Cohen commented

Salvis,

My node_access module is providing update grants, but no view grants. I want the view permissions to be controlled by the nid/all/0/100 grant. While my module is granting an additional update permission to a small number of nodes.

The difference made by my patch is that nid/all/0/100 is written if no module provides a view grant. The current logic is that nid/all/0/100 is written only if no grants are provided (view or otherwise).

I hope that explains it better.

salvis’s picture

salvis
he/his
commented

You write about node-specific nid/all/0/100 grants, but your code checks "nid-less" (*) and writes a global 0/all/0/100 grant.

The difference made by my patch is that nid/all/0/100 is written

The code in #1 write 0/all/0/100 — I don't understand...

Dave Cohen’s picture

Dave Cohen commented

I miswrote. My goal is to have 0/all/0/100 written, unless a module provides view grants. If a module provides only update or delete grants, I still want 0/all/0/100 to be written.

salvis’s picture

salvis
he/his
commented

Ok, that makes sense. You'd probably want to do

  if (db_result(db_query("SELECT (*) FROM {node_access} WHERE grant_view > 0")) { ...

In that case, the patch is incomplete, because unless you'd provide a non-view grant for each and every node (which is not what you want to do, if I understand you right), node_access_acquire_grants() supplies the nid/all/0/100 grant for the orphan nodes, and your INSERT will never get triggered.

The problem is, you can't just keep node_access_acquire_grants() from doing its thing, because you can't know whether a later node might receive a view grant until you're completely done, and if one did, you'd have to restart enumerating all your nodes from scratch. I don't think you can do that on D5 because of the memory and time-out issues.

If you can somehow guarantee, that no module will supply a view grant, then this could work nicely, but if an NA module suddenly started to supply view grants anyway, then it would need to somehow trigger a permissions rebuild to become effective.

cburschka’s picture

cburschka
they
commented
Status: Needs review » Active

Since a patch for D7 is not attached, this shouldn't be in the D7 CNR queue.

Jooblay.net’s picture

Jooblay.net commented
Issue summary: View changes

What is the status of this ticket:) Can we close this...

Status: Active » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.