Symptoms

When using a node access module alongside Revisioning — Content Access, Taxonomy Access Control, Workbench Access, or something custom — saving a node using the option "Create new revision and moderate" leads to Access Denied or other permissions-related problems. Editing and saving revisions other than the current one can also lead to problems.

Problem

A new revision "in moderation" is effectively a draft that is unavailable to the public. Revisioning performs most of its magic to accomplish this in when loading the node, in revisioning_node_load(). However, when a revision in moderation gets saved, it's that draft revision being passed to hook_node_access_records() instead of the current revision. Node access records are then written based on the wrong revision.

Steps to reproduce

Reproducing this is tricky because a lot of it depends on permissions being configured a certain way. I put together a small module that I hope helps show what's going on. All it does is create a new content type and writes a node access record for nodes of that type. The module writes a node access record only when the node sticky flag is enabled. The record isn't actually enforced, but the Devel Node Access block shows when it's written.

  1. Create a new site using standard install profile.
  2. Download and enable: revisioning, devel, devel_node_access
  3. Patch Revisioning with included patch to add revisioning_issue module, enable it.
  4. Rebuild permissions as Drupal suggests with "The content access permissions need to be rebuilt."
  5. Create a new Demo node at: /node/add/demo

    Enable "Create new revision, no moderation" under Revision information.
    Enable "Sticky at top of lists" under Publishing options.
  6. Save the node, then note the Devel Node Access block in the footer. A node access record has been added for the node. See attached screenshot.
  7. Edit the node.

    Enable "Create new revision and moderate" under Revision information.
    Disable "Sticky at top of lists" under Publishing options.
  8. Save the node and then go to the View Current tab.
  9. Note the Devel Node Access block. The custom node access record no longer appears. This is because it only gets written for nodes with "Sticky at top of lists" and the revision in moderation does not have that. The revision in moderation is being passed to hook_node_access_records() instead of the current revision.
CommentFileSizeAuthor
#21 revisioning_node_access_moderation-2037655-21.patch3.25 KBfbrooks
#19 revisioning_node_access_moderation-2037655-19.patch4.11 KBfbrooks
#15 revisioning_node_access_moderation-2037655-15.patch4.1 KBfbrooks
#1 2037655-1-revisioning-node-access-moderation.patch1.46 KBmilesw
revisioning_node_access_issue_screenshot.png127.94 KBmilesw
revisioning_node_access_issue_module.patch1.69 KBmilesw

Comments

milesw’s picture

milesw commented
Status: Active » Needs review
StatusFileSize
new 2037655-1-revisioning-node-access-moderation.patch1.46 KB

Somehow the current revision needs to be loaded before node_access_acquire_grants() is called.

The order of hooks invoked during node_save() looks like this:

function node_save($node) {
  ...
  // Call the node specific callback (if any). This can be
  // node_invoke($node, 'insert') or
  // node_invoke($node, 'update').
  node_invoke($node, $op);

  // Save fields.
  $function = "field_attach_$op";
  $function('node', $node);

  module_invoke_all('node_' . $op, $node);
  module_invoke_all('entity_' . $op, $node, 'node');

  // Update the node access table for this node. There's no need to delete
  // existing records if the node is new.
  $delete = $op == 'update';
  node_access_acquire_grants($node, $delete);
  ...
}

The attached patch seems to fix the situation, but feels pretty hackish. It loads current the node revision just after hook_entity_update() finishes, as the very first implementation of hook_node_access_records().

Maybe someone can come up with a cleaner solution.

milesw’s picture

milesw commented
Issue summary: View changes

Style edits

Leeteq’s picture

Leeteq commented
Priority: Normal » Major
Issue summary: View changes
rdeboer’s picture

rdeboer
Location Melbourne
commented

@milesw

So sorry this completely dropped off my radar.
I appreciate the analysis that went into this. Thanks so much for the patch!

Like you I'm not 100% about all the ins and outs and potential consequences of your solution.
Would be great to get some feedback from others having applied this patch.

Rik

milesw’s picture

milesw commented

@RdeBoer

No problem. Coming back to this now my description is quite verbose.

Just to clarify what needs fixing:

The current revision of the node needs to be the one passed to node_access_acquire_grants(), not the revision being saved.

Patch #1 accomplishes that by swapping revisions in a first implementation of hook_node_access_records().

Another option would be to swap revisions in a last implementation of hook_entity_update().

rdeboer’s picture

rdeboer
Location Melbourne
commented
Assigned: Unassigned » rdeboer
Status: Needs review » Fixed

Patch applied with attribution.
Available now in 7.x-1.x-dev and 7.x-1.7 when released.
Thanks so much Miles and sorry about the delay.
Rik

milesw’s picture

milesw commented

Thanks for getting this committed, Rik!

rdeboer’s picture

rdeboer
Location Melbourne
commented

No thank you, Miles!
Let's run with this for a while in dev. Am keen to have this tested by the community and released as part of 7.x-1.7.
Rik

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

dsrikanth’s picture

dsrikanth commented

Revisioning 7.x-1.7 breaks Content Access 7.x-1.2-beta2. Rolling back to Revisioning 7.x-1.6 seems to solve the issue.
The "Access permissions by user" block provided by Devel Node Access shows Revisioning as the reason why a content access configured node is visible to non-intended user. Does anyone else have the same issue?

milesw’s picture

milesw commented

@dsrikanth: I was able to reproduce what you described. Here is the change that's causing the problem: #2191527: Node access is incorrectly restricted

It would be helpful if you could detail your scenario there, including what's happening and what you expect to happen.

fbrooks’s picture

fbrooks commented

I wanted to bring #2241705 Redirected to currently published revision after editing unpublished revision to your attention. I believe it is being caused by this patch.

fbrooks’s picture

fbrooks commented

Miles/Rik,

I think that if there is an issue here, it is a bug in the individual node access module implementations and not in the Revisioning module.

The demo node access module that Miles provides, I would argue has a flawed hook_node_access_records implementation. The docs for hook_node_access_records states that the node argument is "the node that has just been saved." From that I would expect the node being passed in to reflect the revision that was just saved. If a hook_node_access_records is making a decision based on some other revision, it should use the nid of the node object that was passed in to identify and load the needed revision.

Frank

milesw’s picture

milesw commented

@fbrooks:

That's a fair argument.

The reason I believe this is Revisioning's responsibility is because Drupal core has no concept of "revision in moderation". With core, saving a node revision means the revision you're saving becomes the current node. Previous revisions are simply a history behind the current node. Revisioning changes this behavior, and needs to do some hackish things to accomplish that (see revisioning_node_load()).

Contrib modules expect the behavior included with Drupal core, and therefore something like Content Access can't be expected to handle revisions in moderation.

It's worth noting the Workbench Moderation module offers "revision in moderation" functionality similar to Revisioning. It implements it in a very different way. When a user saves a node revision in moderation everything happens normally, then in a PHP shutdown function it calls node_save() a second time for the "live" version of the node. This updates the node table and also allows the correct node access records to be added for the "current" node revision.

The method used by Workbench Moderation is equally hackish to what's happening here in Revisioning, and I'm not suggesting Revisioning should go that route.

fbrooks’s picture

fbrooks commented

Miles,

I see your point. You are right that it is Revisioning's fault these node access modules do not work properly; however, I would say it is not Revisioning responsibility to change to work with them. Revisioning changes the workflow of the system. When you install it, I don't think you should expect it work with a node access module, or any module for that matter, that depends on a different workflow. As a user of Revisioning, or any module that alters the workflow, that is a limitation you accept.

I would propose that the better way to deal with this issue is to make a separate module that adapts Revisioning to the other node access module you wish to use.

Frank

fbrooks’s picture

fbrooks commented
StatusFileSize
new revisioning_node_access_moderation-2037655-15.patch4.1 KB

The change introduced in this issue is also causing #2271747 Incorrect Title Used in Status Message When Saving a Revision Under Moderation With Edited Title.

I am concerned that altering the reference to the node revision under save as the patch introduced here does, is going to cause more problems in the long run.

Attached is a patch for resolving this issue without modifying the reference to the node revision under save.

I believe this approach will resolve this issue, #2271747, and both of the issues reported under #2241705

Frank

fbrooks’s picture

fbrooks commented
Status: Closed (fixed) » Needs review

The last submitted patch, 1: 2037655-1-revisioning-node-access-moderation.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 15: revisioning_node_access_moderation-2037655-15.patch, failed testing.

fbrooks’s picture

fbrooks commented
StatusFileSize
new revisioning_node_access_moderation-2037655-19.patch4.11 KB

Something was wrong with the last line of revisioning_node_access_moderation-2037655-15.patch, submitting again.

fbrooks’s picture

fbrooks commented
Status: Needs work » Needs review
fbrooks’s picture

fbrooks commented
StatusFileSize
new revisioning_node_access_moderation-2037655-21.patch3.25 KB

This is another alternative that I think is better. The use of hook_module_implements_alter in revisioning_node_access_moderation-2037655-19 really violates the api for that hook by removing entries from the array of implementations. The api for that hook is pretty clear that changing the order is really the only expected result.

The alternate approach in this patch is to use hook_node_access_records_alter to clear the grants array and rebuild it using the current node. This is clearly within the API for hook_node_access_records_alter and, I believe, it accomplishes the same result as patch 2037655-1-revisioning-node-access-moderation without modifying the reference to the node revision under save.

rdeboer’s picture

rdeboer
Location Melbourne
commented

Thanks Frank!
Commited with attribution.
Let's hope this is the last of it!
Rik

Leeteq’s picture

Leeteq commented
Status: Needs review » Fixed

- so this is in -dev now (fixed), right?

rdeboer’s picture

rdeboer
Location Melbourne
commented

Yes it is in the very latest dev.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

lostchord’s picture

lostchord commented
Status: Closed (fixed) » Active

I'm still seeing this issue with 7.x-1.9 and 7.x-1.x-dev. I have Workbench Access enabled as well. Rebuilding permissions restores the correct access after the edit.

rdeboer’s picture

rdeboer
Location Melbourne
commented

Hi @lostchord,
Thanks for sharing your workaround in #26.
Rik

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Issue tags: +Needs tests

I recommend starting with some tests - document what *should* happen, then work out how to fix it.