Remove hook_access

I second that the current node_access() function is somewhat confusing. Some modules call node_access('view') but no core module implements the 'view' hook.

$ grep -r "node_access('view'" *
modules/comment.module:          'callback' => 'comment_reply', 'access' => node_access('view', $node), 'type' => MENU_CALLBACK);
modules/node.module:          'access' => node_access('view', $node),
modules/node.module:        if (node_access('view', $edit)) {

When it is not implemented, you'd expect access to be denied, yet they are allowed to access the node. Furthermore, if you do implement the hook, how does it stack with the node-level permissions at the database level?

yes, this is a bit confusing. fully database defined permissions sounds like a good idea.

hook_access('view') only has affect when you are viewing a single node.

I'm not a fan of this idea. I like that I can have permissions based on the node type and control them programatically. hook_access is useful. The node_access database table is a useful addition to it. I don't see the problem having both.

I don't think the database overhead will be too bad. There will be one query per node_load()

That one query could return any number of rows. Which then have to be iterated and compared against the result of hook_node_grants (which could result in many more database queries).

Furthermore, an example of a permission easy to implement via hook_access, yet difficult with node_access table is "edit own pages".

When it is not implemented, you'd expect access to be denied, yet they are allowed to access the node

One quirk regarding the node_access table is it comes by default with view grants for nid==0 and grantid==0. This allows everyone to view everything by default.

I'm also wondering: without hook_access how you would check for 'create' permission.

@yodadex - _create_ would be a new column in node_access.

if admin wanted 'edit own', drupal would insert the propoer grants in node_access table for each node. so in this case, node_access API would be in use for a lot more drupal sites. personally, i don;t see that as a problem. node access queries were not optimized in the old days but now they are quite fast. see groups.drupal.org for example. anyway, this is open for discussion and a solvable problem even without forcing node_access API.

Wow, I'm surprised to see this thread revived!

@yodadex - _create_ would be a new column in node_access.

Create is unlike view, update, delete because it works not on existing nodes, but on node types. It would probably require a new database table.

Personally, I really like the flexibility that comes with allowing code to determine whether access is granted or not. Consider the case of "this content is for subscribers only today, but after 1 year it is viewable by the general public". This is easily handled by code checking the creation date of a node. Less easily handled by a cron job updating the node_access table.

I hate this idea.

1) Updating the node_access table on a large database takes a long time, so doing large-scale changes to access permissions sucks.

2) for operations other than 'view' there is NO value-add in the database whatsoever; it is just an impediment. It creates a bunch of extra data and creates all kinds of problems with synchronization.

The only reason the node_access table has any value at all is because of db_rewrite_sql and that is an iffy value-add. node_db_rewrite_sql screws up queries by adding DISTINCT whether or not you want it, and it adds a performance hit to *every* query for nodes. Moving MORE toward node_access just increases the performance LOSS by having more sites require the node_access table.

Well, we are at a crossroads. We have not yet heard any alternative for node listings besides node_access table. I think no good alternatives will emerge. Thus, it makes sense to embrace it as proposed here, instead of http://drupal.org/node/143075. Note that this patch achieves what http://drupal.org/node/143075 wants; give all modules a vote in whether a node may be accessed or not. Modules will have to implement hook_node_access_records() instead of hook_access(). No big deal.

I'm not totally convinced I'm right, because these are non trivial issues. I could be convinced otherwise. But for now, this issue looks like the right direction to me.

Enabling the node_access table (which is not always enabled) is an immediate performance hit the system, which is unnecessary. That, right there, IMO, is the biggest reason we should have alternatives so that when the use of the node_access table is not necessary, we can avoid it. While I agree that the use of the node_access table is sometimes necessary, I think it is important to be able to use it when other avenues exist.

By having a single module that uses the node_access table, DISTINCT is added to all of your node queries, plus a relatively complicated join against a composite field. This is not free. On large, busy systems, this can and will have a performance impact.

Using the node access system is not easy, nor straightforward. We've tried to make the API as simple as possible, but pre-calculating the access for all possibilities in advance is non-intuitive, flat out. It makes complicated access heirarchies difficult and data intensive. The number of users out there who understand node_access well is small, and there is a REASON for that. It's not due to poor documentation, nor is it due to unwillingness. It's due to an obtuse method that is clever but is NOT easy.

Maintaining the node_access table is hard. If I have a complicated structure, and I end up with 30 records per node in a million node database, look at the amount of data I'm required to put in my database. Think about the complexity in indexing that; how difficult it is to update that data when my access needs need to change.

Embracing node_access as the ONLY way to control access to nodes is, IMO, stupid. It creates a performance drain as well as increases the difficulty of writing code, and the ONLY justification for it is because there should only be one way to control access? Does this really make sense?

Using Views and custom filters, I can bypass the node_access table for 'view' types, and the node_access table should *never* have been used for 'update' and 'delete' types. You never need those on a multi-node basis, and so they add a LOT of unnecessary weight.

We're trying to make Drupal more performant, not less.

I definitely agree with merlinofchaos. At the college where I work, we implemented a custom module that provides granular node access rules for a hierarchical category system. In order to do it, we essentially had to ignore the current access hooks and hack into the node_access() function. That's because the ability for a given user to access a particular node can't be calculated using a single SQL query, and storing the access permissions for each of 30,000 users on each of several thousand nodes would get very costly, very quickly.

In my opinion, it's time to rethink the node_access() function. Yes, try to keep backward compatibility with existing modules, but also provide some way for a given module to say, "I insist that the user [has/doesn't have] access to the node you're asking about, so ignore all other modules".

Moved over to: #537862: Convert hook_access() to hook_node_access() for more flexibility