Extensible Node Access/Authorisation Capability (simple patch!)

See http://drupal.org/patch to make a proper patch. Thanks!

Features go against HEAD only.

I've been trying to resolve the issue of taxnonomy access and og working in concert. I've been discussing this here: http://drupal.org/node/122712 and here http://drupal.org/node/122385. One way discussion so far.

As briefly as I can: If a node belongs to a group, and has a vocabulary assigned to it, a user can access the node if he either belongs to the group or has access to the vocabulary term. It shouldn't work like this. The user should BOTH belong to the group AND have access to the vocabulary term before given permission to access the content.

What I figured out in my head was that I needed to be able to ask, at the point where taxonomy access deterrmines a users access to content, if this content belongs to a group. If so, then I need to check that access first. If the user doesn't have permissions in the group the node is in, then access should be denied regardless of whether he has the access to the vocabulary term. This is og and taxonomy access working together, at least in my mind.

I say all that to ask: Will you modification help me accomplish this? If so, could you tell me how? I will figure out the logic with respect to the group thing, but how do I replace the current permissions checking with your update?

Thanks!

You know what? I took the time to study the code and api, and I understand what you're doing. Quite elegant, actually. All I need is an example of:

function _mymodule_check_access($node,$arg1)

I'd just like to see how you check permissions for a user on a node without using node_access.

Thanks!

Thanks. I actually have it working now. I wanted to apply both OG and TAC permissions to nodes, and that is now working. Thanks so much for the snippet.

Final question: It controls view/update/delete access beautifully. However, users can still see (list) content that they don't have access to. I read somewhere some time ago that list permission is not handled by node_access.

How have you handled the listing issue?

Again, thanks!

p.s. Code I created (sample is from nodeapi section of OG module. og_node_access simply returns TRUE or FALSE):

	case 'access':

		$access = FALSE;
		// Does user have OG permission for this node?
			$og_access = og_node_access($teaser, $node);
		// Does user have TAC permission for this node?
			$tac_access = taxonomy_access_node_access($teaser, $node);

		if ($og_access === TRUE && $tac_access == FALSE) $access = FALSE;
		if ($og_access === FALSE) $access = FALSE;
		if ($og_access === TRUE && $tac_access == TRUE) $access = TRUE;
		
		return $access;
			 
	break;

Really cool modification you made. Both the OG and TAC maintaiiners said what I wanted to do wasn't possible.

Thanks to your snippet and some creative thinking, I've finally got the permissions scheme I was after working. In short, nodes now respect both TAC and OG permissions on view, update, create, delete and list. That is to say that if a node is in an OG group, and has a TAC vocabulary term assigned to it, a user must both be a member of the group and have access to the term to view the node.

What I found out was that I was able to use your snippet to build the view/update/create/delete permission access. However, your snippet could not be used at all for listing nodes. This function is handled by node_db_rewrite_sql() (including _node_access_join() and _node_access_where()). You basically have to tell this function what restrictions to add to the node list sql, which by default reads from the node_access table: (i.e., "select * from node inner join node_access on node_access.nid = node.nid").

My next question is theoretical: Is there a way to get this functionality without modifying the node.module?

In other words, is there a way when node_db_rewrite_sql() is called to have it executed from another module or template other than the node.module? That would leave your patch as the only update to the node.module required to get these permission changes done.

FYI: there's a lot more discussion about a duplicate suggestion over at http://drupal.org/node/143075.
There's also an arguably cleaner patch with simpler code and better comments to do the same thing:
http://drupal.org/node/143075#comment-249008 (#24)...

I'm still in favor of this change, but I fear it's not going to make it into D6, which would be quite a shame...

Thanks for the info on http://drupal.org/files/issues/nodeapi_access_4.patch.

Just a technical point: When using the Extensible Node Access/Authorisation Capability patch, I was simply returning TRUE, FALSE or NULL from hook_nodeapi('access'). With the cleaner code, I noticed this:

+  if (!empty($access) && is_array($access)) {
+    // If any values were true, return true; otherwise, return false.
+    return (bool) array_filter($access);
+  }

It appears to be looking for an array. Does this mean I supposed to return something other than TRUE, FALSE or NULL with this new patch?

Again, thanks. I agree: A shame if it doesn't make it into D6.

It appears to be looking for an array. Does this mean I supposed to return something other than TRUE, FALSE or NULL with this new patch?

Look at the code in modules/node/node.module for node_invoke_nodeapi(). Every module in your system can implement hook_nodeapi(), which is the point of a hook. ;) Every module has a chance to return its own value for hook_nodeapi('access'). Therefore, when you invoke the hook, you get an array of values representing what any module that implements the hook returned. So, the call site has to be prepared to handle an array, even though each hook just returns a single bool.

Thank you very much for that explanation. Understood!

Theoretical question:

If this patch doesn't make it into D6, do you think it's possible to override node_access entirely? That is, is it possible to create a module that will have it's own node_access code (that includes the patch) and that would be called every time "node_access" is invoked instead of function node_access in the node.module?

Said in another way, do you think it's possible to execute a "my_module_node_access" function (in my_module.module) instead of the "node_access" function (in node.module) every time "node_access" is called in Drupal code?

I understand that there is hook_access that would be called within the "node_access" function for modules to return access permissions for their own node types, but what I'm talking about is replacing the "node_access" function altogether without modifying the core code.

Just thought I would ask.

This will have to wait for D7.

I have another approach to the same issue, which needs to be re-worked a little for D6 and then submitted as a D7 patch.

http://drupal.org/node/191375#comment-635244

I like the nodeapi solution, but I'm not sure it replaces the need for subselects in order to have (grant1 AND grant2) logic instead of (grant1 OR grant2) logic.

I also discovered that access check logic was needed in three distinct places:

node_access()
_node_access_where_sql()
node_access_view_all_nodes()

One slight advantage to this approach: Far fewer modules use hook_node_grants() as compared to hook_nodeapi(), so we save a lot of elements in the call stack.

gosh, we already had a battle royale over this issue at http://drupal.org/node/143075. must we do it again? anyway, i oppose this patch for all the reasons listed there.

@moshe

I have submitted the alternate patch to 7.x.

http://drupal.org/node/196922

Patch applies cleanly (with some offset) to HEAD. Everything appears to work correctly, and all the Node simpletests pass without issue.

237 passes, 0 fails, 0 exceptions

RTBC!

Robin

How does this get RTBC over Moshe's objections in #18?

We also need to review in light of http://groups.drupal.org/node/14419. The plan for D7 -- chx reminds us -- is to kill the $op parameter. So we need to refactor as hook_nodeapi_access().

I don't know drupal well, so my comments may be off the course.

What about a similar idea for user_access()? Add a hook in user_access(), so a module can use the hook to define an ad hoc rule that allows one user to fully control another user node. Right now, pretty much admin is the only one that can manage any users (and all users). I want something like a parent can manage his/her family members' accounts (the whole CRUD).

For my project, I am experimenting adding a hook in user_access(), so it hooks into my foo_user_access(...). Can someone please tell me if this is absolutely the wrong approach?

$op is dead. moshe's concerns still not dealt with. This would also need to come with it's own test.

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