Convert hook_access() to hook_node_access() for more flexibility

Great stuff - exactly as we discussed at the BoF. This lets all modules participate in the allow/deny decision, not just the "owning" content type. Previously, modules had to to use the grants system for that and who likes that (beside me).

My feedback is nitpicky - beware:

@see http://api.drupal.org/api/group/node_access/7 Replace the URL with node_access(). API.module will hyperlink.

// $Id:$. I have always done // $Id$. No colon. Not sure it matters.

+ // We grant access to the node if the following conditions are met". i would like clarify "...if either of the following conditions ..."

if (in_array(NODE_ACCESS_DENY, $access, TRUE)) {
+  	return FALSE;
+  }
+  elseif (in_array(NODE_ACCESS_ALLOW, $access, TRUE)) {
+  	return TRUE;
   }

Indentation troubles.

"node type" should be "content type"

Can we make is so that Blindly returning FALSE will throw an error instead of Blindly returning FALSE will break other node access modules? I think it would be smarter to make constants (NODE_ACCESS_ALLOW, etc) be 1,2,3 or 'allow', 'deny', 'ignore' instead of 1, 0, NULL. Force people to implement the function correctly.

I think the documentation for hook_node_access should state that NODE_ACCESS_DENY trumps NODE_ACCESS_ALLOW. And that either response trumps the node_access table.

In nodeperms_node_access(), every instance of NODE_ACCESS_DENY needs to be replaced with NODE_ACCESS_IGNORE.

Otherwise, the node_access table will not have any effect on update or delete permissions.

Conceptually, checking the permission on the permission page means "this is allowed", while not checking it means "some other module might allow this, or might not". Leaving the permission unchecked is not the same as deny.

Subscribing for now.

This is exactly what I've been wanting for the last 3 1/2 years. Thank you.

In D6, leaving a node permission unchecked means "defer to node_access table". So if you choose DENY instead of IGNORE, its a change people will have to get used to. One effect of the current system is that I can check a permission for admin roles, while leaving it unchecked for non-admin roles. That way admins always get the privilege but for other users it defers to node_access.

You could argue that users should either use nodeperms or node_access, but never mix the two. If that's your choice, just bear in mind how it is different from the current behavior that people have sites configured for. And if you're into enforcing that discipline, wouldn't you also force hook implementers to return the proper value (instead of NULL)? ;)

OK, here's my idea. It's so radical, that I don't think it's going to get much love, though. :-)

Coming from a UX perspective, I think it makes more sense to completely abstract the permissions system, so that everything currently provided using the grants table is moved in to a removable "grants" module. I don't think it's necessary to configure which node types use grants and which don't; this can be an all-or-nothing, based upon whether or not the module is active.

If the grants module is not active, there would be significantly fewer entries in admin/user/permissions. All content would be wide-open to view, but not to create/update/delete.

Any module can affect the permissions by adding its ALLOW/DENY vote about a given node. This includes the grants module, which is treated identically to all others. If any module returns a definitive ALLOW or DENY, then it takes precedence. If not, the default is TRUE for view, and FALSE for everything else.

(Stepping into flame-retardant suit)

I didn't realize the idea had been brought up before, so consider it dropped. I'm happy enough with the proposed change.

As far as the remaining questions go, I would vote for NODE_ACCESS_IGNORE===NULL and lack of permission means "ignore". Maybe there's something you can do in the UI to make this more obvious, though.

Patch removes tabs from #15, and adds a test. The test allows a user to edit the node if the title of the node is the same as the user name.

oh my, I've re-rolled something wired. Will re-roll soon.

@Crell, The test that I've added doesn't pass, although I thought it would - maybe you can see what I did wrong there... :/

So, I'm putting to patches here - one with the test and one without (just with the tabs fix).

As Crell has dismissed #153998: Access checks returning NULL is ugly., this needs to be committed, because node access is just as broken in D7 now as it was in D6 before the commit in #153998-99: Access checks returning NULL is ugly..

Returning NODE_ACCESSS_IGNORE/NULL/whatever to allow the node access table to kick in if hook_node_access() is undecided is a very important feature.

@Amitaibu: You've lost the fixes to the currently broken Blog, Forum, and Poll modules, as well as 'details' like

=== added directory 'modules/nodeperms'

and

=== modified file 'profiles/default/default.info'
--- profiles/default/default.info	2009-07-15 02:08:40 +0000
+++ profiles/default/default.info	2009-08-02 05:48:32 +0000
@@ -14,3 +14,4 @@ dependencies[] = taxonomy
 dependencies[] = dblog
 dependencies[] = search
 dependencies[] = toolbar
+dependencies[] = nodeperms

@Crell: You have a few tabs (in nodeperms_configuration() and nodeperms_node_access()) that should be removed.

Here's Crell's patch - without the tabs.

I read through this one more time. IMO, this is RTBC.

Very nice patch, +1! Just three nits:

1.

function poll_permission() {
$perms = array();
$perms += array(

2.

* Impelement hook_menu().

(removed an 'e')

3.

// If we have not configured whether or not to manage a given notnode type, assume

No other changes.

+++ modules/nodeperms/nodeperms.module	2009-08-06 03:31:53 +0000
@@ -0,0 +1,126 @@
+  // Build standard list of node permissions for this type.
+  $perms = array(
+    "create $type content" => array(
+      'title' => t('Create %type_name content', array('%type_name' => $info->name)),
+      'description' => t('Create new %type_name content.', array('%type_name' => $info->name)),
+    ),
+    "edit own $type content" => array(
+      'title' => t('Edit own %type_name content', array('%type_name' => $info->name)),
+      'description' => t('Edit %type_name content created by the user.', array('%type_name' => $info->name)),
+    ),
+    "edit any $type content" => array(
+      'title' => t('Edit any %type_name content', array('%type_name' => $info->name)),
+      'description' => t('Edit any %type_name content, regardless of its author.', array('%type_name' => $info->name)),
+    ),
+    "delete own $type content" => array(
+      'title' => t('Delete own %type_name content', array('%type_name' => $info->name)),
+      'description' => t('Delete %type_name content created by the user.', array('%type_name' => $info->name)),
+    ),
+    "delete any $type content" => array(
+      'title' => t('Delete any %type_name content', array('%type_name' => $info->name)),
+      'description' => t('Delete any %type_name content, regardless of its author.', array('%type_name' => $info->name)),
+    ),
+  );

Am I the only one that think the descriptions are not needed?

+++ modules/nodeperms/nodeperms.module	2009-08-06 03:31:53 +0000
@@ -0,0 +1,126 @@
+  // that we do want to.  That ensures that a newly created node type will have

There is an extra space here.

This review is powered by Dreditor.

In D6, node_access() used to call filter_access() to help decide whether to allow updating:

  // If the node is in a restricted format, disallow editing.
  if ($op == 'update' && !filter_access($node->format)) {
    return FALSE;
  }

Removing this was first discussed in #372743-51: Body and teaser as fields and decided in #192 and following. It was committed with the understanding that something like #91663-63: Permission of text format is not checked when editing an entity and instead reset to something a user can use. would be committed, too, but that issue is not moving.

What is the plan now?

(D6 node_access() is currently broken because it ignores $account when calling filter_access(). This is being worked on in #470840: Bug in node_access if we specify an account when check the filter_access.)

@salvis - are you sure that this issue needs to move out of rtbc for that bug. filter_access() is related gto what we are doing here but i don't think it is right to make this dependant on an unresolved issue in nearby code. or i am misunderstanding the relationship.

Ok, fine.

(Throwing "an access-denied from literally anywhere" isn't so great. I'm working on devel_node_access to explain why a given user has or hasn't CRUD rights on any given node, and I'm hoping for something better than having to try to edit to find out whether something gets thrown or not... I'm happy we were able to talk about it :-))

+1, just discovered the mess of hook_access, for a site that needs to allow anonymous users to edit newly created (by them) nodes in an ubercart cart, and found this issue, which would make the solution trivial, as opposed to the hacky method I need to solve it now (redefine the nodetype(s) as owned by my own custom module, just to catch the hook_access for it so I can deal with the special case)

This is a great little patch. Eliminates basically all of the WTF from the node access system, and makes it actually understandable to developers. Hooray!

Question: Why bother with the node_access table at all anymore after this?

+++ modules/node/node.api.php	2009-08-06 03:47:31 +0000
@@ -342,6 +342,63 @@ function hook_node_load($nodes, $types) 
+ * block access, return NODE_ACCES_IGNORE or simply return nothing.

ACCESS

+++ modules/node/node.api.php	2009-08-06 03:47:31 +0000
@@ -342,6 +342,63 @@ function hook_node_load($nodes, $types) 
+  $type = is_string($node) ? $node : (is_array($node) ? $node['type'] : $node->type);

Ew? Can we please clean up the calling code so that we always get passed in a full $node object?

+++ modules/node/node.api.php	2009-08-06 03:47:31 +0000
@@ -342,6 +342,63 @@ function hook_node_load($nodes, $types) 
+  return NODE_ACCESS_IGNORE;

How about document something like:

// Alternately, simply do not return a value from this function.

+++ modules/node/node.module	2009-08-06 03:29:45 +0000
@@ -16,6 +16,21 @@
+define('NODE_ACCESS_ALLOW', 'allow');

Why strings rather than numbers? Not really opposed, but it's different than what we normally do. I would've expected -1 (deny), 0 (neutral), and 1 (allow).

+++ modules/nodeperms/nodeperms.admin.inc	2009-08-03 02:04:54 +0000
@@ -0,0 +1,29 @@
+
+  $form = array();

Minor, but you don't need to instantiate an array if you're specifying the key when adding an index, like you are below.

+++ modules/nodeperms/nodeperms.admin.inc	2009-08-03 02:04:54 +0000
@@ -0,0 +1,29 @@
+    '#description' => t('Select which content types should use Drupal\'s permission system for controlling access. Note that you may have more than one module affecting access to the same content type.')

Please use double quotes here so you don't have to escape the apostrophe.

+++ modules/nodeperms/nodeperms.admin.inc	2009-08-03 02:04:54 +0000
--- modules/nodeperms/nodeperms.info	1970-01-01 00:00:00 +0000
+++ modules/nodeperms/nodeperms.info	2009-08-03 02:05:07 +0000
+++ modules/nodeperms/nodeperms.info	2009-08-03 02:05:07 +0000
@@ -0,0 +1,8 @@
+name = Node Permissions

Hm.

a) We don't use the word "Node" in user-facing text.
b) We don't usually abbreviate the names of things, i.e. "perms"
c) "nodeperms" is not a word. :P

Why not 'contentaccess' if we're going to smoosh two words together? And why are we so opposed to underscores, again?

+++ modules/nodeperms/nodeperms.module	2009-08-06 03:31:53 +0000
@@ -0,0 +1,126 @@
+  // Build standard list of node permissions for this type.

While we're at it, why not 'view'? That would eliminate a *HUGE* WTF for the vast majority of people installing Drupal, and would allow people not to have to resort to choosing from the absolutely confusing array of node access modules for what most would assume is basic CMS functionality. I could also see this being a 'killer feature' of Drupal 7.

+++ modules/nodeperms/nodeperms.module	2009-08-06 03:31:53 +0000
@@ -0,0 +1,126 @@
+  $configured_types = variable_get('nodeperms_types', array());

Should we statically cache this? Looks like it might be re-generated several times on a page.

+++ profiles/default/default.info	2009-08-02 05:48:32 +0000
@@ -14,3 +14,4 @@ dependencies[] = taxonomy
+dependencies[] = nodeperms

Do we want to add this to expert profile as well?

19 days to code freeze. Better review yourself.

@webchick:

Why strings rather than numbers? Not really opposed, but it's different than what we normally do. I would've expected -1 (deny), 0 (neutral), and 1 (allow).

Personally, I'd vote for -1 (neutral), 0 (deny), and 1 (allow). That way it's more analogous to a boolean.

I read through this again and am quite satisfied.

While it seems to be moving in the proper direction, I'm not really satisfied yet. It is reasonably close though.

1. I don't like the module name 'nodeperms'. Why does this need to be a separate module? I recommend that we move it back into node.module. Now that the behavior is configurable, the value of making it a separate module is really low, and just makes it more confusing for new users. It is such a small module. I personally don't think it is worth making a module.

2. Don't abbreviate permission(s) as perm(s).

3. + $type = is_string($node) ? $node : (is_array($node) ? $node['type'] : $node->type); is still pretty ugly. Can we fix our parameter handling? If not, we can leave that for a follow-up patch.

4. I didn't understand the 'Node permissions' settings at first. What does it mean not to control permissions? That everyone can create and edit the nodes? The description didn't properly educate me and I had to go and look at the code to make sense of it ... So when I install an alternative node access module, I need to go and uncheck those checkboxes? When I create a new node type, do I need to go and check the permissions or does it come with sane defaults? Also, shouldn't the node permissions settings be part of the per-content type settings? This sounds like it could be a usability regression so I'd like to see this discussed some more so we can nail the help text, the default behavior and the configuration workflow.

5. Do we need a couple of new/extra tests for this? We don't test the configuration setting, and given that this has security implications, I would suggest we write some small tests to make sure the configuration actually works.

Re 35.4, too long to type is not an excuse. If it does end up being a separate module, it should be called node_permissions

we move the configuration to per-node-type-page, then we recreate the same problem. If you don't want to use permissions for node access, you have to go to a separate page for every single node type to disable an admittedly not-inherently-obvious setting. (Does that mean we document what "node access" is on every node type page? That could be a lot of non-trivial text on an already busy page.)

On the flip side, if consolidate them into one page, we introduce inconsistencies in the interface - some things will be on the individual node type pages, while other things will be on other pages. If one thing puts on a separate page, everything should be on seaprate pages. In fact, according to your model (although exaggerating it greatly), there should be no node type admin pages - we should have pages like "names of node types, descriptions of node types." While I admit that that never should and never will happen, it's the concept that matters.

This is OT for this issue, but I personally would love us to introduce a nice hook_node_settings() or something so that these could all be aggregated and we could define a "defaults" page. Right now your only prayer is grepping through the code of modules that implement hook_form_alter() and looking for $form_id node_type. :P Ugh. :P

Crell's summary of what I said is dead on. The only situation where you need to override default behaviour, is where you 1. have a contrib or custom node access module installed 2. the core behaviour conflicts with how you want to use it. Since that's not a common scenario in the scheme of things, I think a core UI (and/or a new core module) is a bit overkill. But, we should allow the flexibility to override things like this, which is either a variable without a settings page (we already have several of these in core, it's not a new idea), or some other way to switch it off without patching node_node_access().

I agree with Dries that I'm -1 to a new module just for this. It makes things needlessly confusing for the default behaviour that probably at least 99.5% of sites are going to use, since this is all that Drupal can do now, and has been able to do since its inception. Also, another thing catch said during that conversation that I strongly agree with is "If I don't like what OG does by default in hook_node_view(), I don't make a og_hook_node_view.module".

Hidden variables for advanced features that power users and/or contrib modules with edge cases use is the standard way we have of dealing with this situation, so makes sense to re-use here IMO.

In our case, we have our own module that completely replaces all node access. If the permissions code was to remain in core in such a fashion that it could not be removed, then I would at the very least like to have a way to automatically default newly-added content types so that the permissions code does not affect them.

Otherwise, we would end up having to uncheck yet another checkbox every time we add a new content type--or wonder why permissions are acting in undesirable ways.

It's fine with me if this is controlled by a hidden variable.

+++ modules/node/node.module	2009-08-18 07:09:54 +0000
@@ -2188,28 +2202,35 @@ function node_search_validate($form, &$f
  * proper functioning of the pager system. When adding a node listing to your
  * module, be sure to use db_rewrite_sql() to add
  * the appropriate clauses to your query for access checks.

ewww. lets not advocate use of db_rewrite_sql() anymore. i know you are not changing this line but you got close to it :) ... How about "Be sure to use a db_select() and add a query tag of 'node_access'. This insures that only accessible nodes are retrieved."

After that change, this is RTBC.

On topic: nice work everyone.

Off topic: what?!? db_rewrite_sql is dead? Is there doc anywhere about how to rewrite code that relies on it?

Throwing a little more node_access developer weight behind this patch. This standardizes the access system, so that 'node' modules are no longer first-class citizens with rights that other modules can never attain. And a huge +1 to letting modules act on node 'create' permissions.

The rest we can iron out later.

One note:

Users with the "administer
+ * nodes" permission may always view and edit content through the
+ * administrative interface.

We swapped this for the 'bypass node access' permission because 'administer nodes' was too all-powerful, so the docs need at least one change.

It looks great now. Thanks for incorporating our suggestions. I committed this to CVS HEAD.

I'm marking this 'code needs work' for three reasons:
1. Let's get the upgrade instructions updated before we mark this fixed.
2. I recommend that we follow-up with a CHANGELOG.txt entry.
3. I didn't see any tests for this yet. Given that this touches security, I'd still like to see some tests.

Thanks Crell, Amitaibu, salvis et al.

Well done.

3. is covered by NodeAccessUnitTest IMO.

I shouldn't have been credited for the issue, but I'm still happy it's in :)

Tagging.

I am sleepy. Sorry. :)

Tagging Novice. Hello, Novice! We need a quick one-liner patch to add an entry to CHANGELOG.txt for this patch. Feel free to drop by #drupal if you have questions on how to do that.

I added another one for:

* Access control affects both published and unpublished nodes.

And committed to HEAD. Thanks! :D