I am trying to use OG-7.x-2.x in our site and am having trouble with revisioning and the publish button. OG has the grants you the ability to control the roles a user has based on their membership in a group and the group context of the node being viewed. There is another module, og_moderation, that provides the expanded permissions management for things like revisioning and publishing. This module can currently control what is shown in the edit field but is unable to control the availability of the publish and unpublish buttons that appear along the top of the page.

My solution would be a hook added to _revisioning_access_node_revision function that would allow other modules to weigh in on the access value being returned. I will add a patch to better illustrate my idea. This would enable a module like og_moderation to control the buttons as needed.

If this doesn't make sense or sounds like a bad idea, is there a different approach I should be pursuing? Thanks.

CommentFileSizeAuthor
#5 revisioning_access_node_revision-1958012-5.patch858 bytespgillis
#1 revisioning_access_node_revision-1958012-1.patch531 bytespgillis

Comments

pgillis’s picture

pgillis commented
Status: Active » Needs review
StatusFileSize
new revisioning_access_node_revision-1958012-1.patch531 bytes

The patch I promised in last post.

rdeboer’s picture

rdeboer
Location Melbourne
commented

@pgillis:

I'm not adverse to this patch.

Just checking though... you're saying the existing hook_node_access() doesn't give you enough to weigh in on the access result?

Rik

pgillis’s picture

pgillis commented

@RdeBoer

Thanks for your reply. I originally thought the same thing, the problem I have with hook_node_access is that I don't get a call to that hook with "publish revisions" or "unpublish current revision" as the $op. I only get view, update, or create. So I can't really decide a course of action from the operator.

I had also toyed with the idea that there was something that should be done differently in the function revisioning_user_node_access() because there is no longer a hook_user_node_access in D7, but I couldn't wrap my head around how the base og module modifies the user_access function's behavior.

rdeboer’s picture

rdeboer
Location Melbourne
commented

Re #1, #3

Hi Pete,

The patch needs a bit more thinking through....
module_invoke_all(...) returns an array of results, one for each hook implementation. So how do we reconcile these? What if we get one 'yes' and one 'no' ?
Also, should other modules be consulted before the node_access() call or after?

Rik

pgillis’s picture

pgillis commented
StatusFileSize
new revisioning_access_node_revision-1958012-5.patch858 bytes

Hi Rik,

I see what you mean. I am assuming we want to err on the side of denying access. I believe that's the approach of hook_node_access. I have attached another patch with some comments in the code. Does this make more sense?

Pete

rdeboer’s picture

rdeboer
Location Melbourne
commented

Hi Pete,
Getting closer...
Still wondering though... Is hook_revisioning_access_node_revision() meant to be able to "right" a wrong.
The way you've got it in your patch is saying that no matter what the outcome is so far, listen to the hook implementations.
Is that what is needed?

Or is it that the hook implementations may veto a "yes" and block access, but not grant access when it has already been denied by core's node_access?

   $access = $node_op && node_access($node_op, $node);
-
-  return $access;
+  if (!$access) {
+    return FALSE;
+  }
+  // Let other modules weigh in on the outcome.
+  // If any module denies access that is the final result.
+  $access = module_invoke_all('revisioning_access_node_revision', $revision_op, $node);
+  return !in_array(NODE_ACCESS_DENY, $access, TRUE);

What do you think?
Rik

pgillis’s picture

pgillis commented

Hi Rik,

Yes, I believe what is needed is the ability to "right" a wrong as you said. In my particular use case I am trying to enable Organic Groups permissions to potentially override access that is being denied by Revisioning based on the content's OG group context.

The problem I have with the patch you proposed in #6 is that, in my case, the hook will never be invoked because the Revisioning module has already decided against access based on the global permissions.

All that being said, I think that if a module calling the hook is explicitly denying access that would trump any other module's attempt to grant access via the hook. When there is a wrong to be righted in the hook calling modules we should err on the side of denying access.

Pete

rdeboer’s picture

rdeboer
Location Melbourne
commented

Ok Pete,
Then we'll do it pretty much like your #5 patch.

This means that a single module implementing hook_revisioning_access_node_revision will override whatever has been decided on node access so far. This is a bit of a deviation of standard Drupal behaviour, but hell, the end justifies the means.

When multiple modules implement hook_revisioning_access_node_revision a single "NO" amongst them will win over any number of "YESes", i.e. the right to veto, which is what core does.

Will check in code when I get to a dev machine.

Rik

rdeboer’s picture

rdeboer
Location Melbourne
commented
Status: Needs review » Fixed

Checked in. Available in dev snapshot.

pgillis’s picture

pgillis commented
Status: Fixed » Closed (fixed)

Looks great, thanks Rik!