Revert Revision does not retain Simple Access groups

yup, this is easily reproducible on a clean test site using the most recent 5.x-compatible code. haven't tried in 4.7.x. maybe i'll have a chance to debug and fix this, but just wanted to bump it, and confirm that it's still a critical bug.

This thread may be related an issue I've just experienced with simple_access interacting with image.

image has a mechanism to rebuild thumbnails, and if I do that then my access controls are lost in the same way as described above - nodes become visible to all.

I added some quick debug messages - when a node_save is triggered (not sure if that gets called when a revision is made...) then node_access_records is invoked for all modules.

The simple_access_node_access_records function attempts to loop through the node and retrieve all the access settings. If nothing is found then the defaults are restored (there is also a bug on line 164 - grant_update is assigned twice - should grant_delete?)

In the case of image module it calls node_save to complete its update, but because the access records weren't previously loaded they are overwritten with the defaults again.

Does reverting a node revision cause a similar behaviour?

I have done some investigation to see if I can work out what is going on. The problem seems to be that simple_access_node_access_records() expects $node->simple_access to be set. If it is, it applies the grants it finds, if it isn't, it sets defaults.

When a node is reverted (or an image thumbnail regenerated) then simple_access_node_access_records() is called but $node->simple_access isn't defined because a form wasn't used. Therefore access is reset to the default settings.

The attached patch adds some code to simple_access_node_access_records() so that if $node->simple_access isn't set then it loads the grants from the database and arranges them in the same format as the form would have provided. The module then thinks a form was used and retains the existing grant details.

I have given this some limited testing on my local system (two user groups define, view, edit and delete options enabled), and have created/reverted revisions and regenerated image thumbnails. Permissions seem to be retained.

I've attached the patch in case anyone wants to try it.

Attached is a more extensive patch. I noticed the menus and titles weren't in "Drupal 5" sentence case, so I fixed that while I was in the module. I also noticed that the tabs to view nodes with edit and delete settings weren't appearing on the Administer > Content management > Simple access page.

This patch fixes the revision permissions being lost AND the menus.

Stuart, thanks for the patch. however, please keep the menu changes out of it, since it's easier to review, test and commit patches that only try to do one thing. for example, if it was just the menu title stuff, i could commit that immediately, with little testing. however, the solution to the bug here requires some more thought and testing. let's just keep this issue focused on 1 thing. please submit a new issue about the menu titles with a patch that just fixes that, and it's much more likely to get fixed quickly.

thanks again for your work debugging this. sounds like you're on the right track.

No prob! I'm still getting the hang of generating patches etc, especially in the case where there are a number of un-related changes. I wasn't sure if there was a way to create a patch that was just the menu fixes, hence the double post :-)

The first attached patch only fixes the revisions problem.

I was also thinking about this some more last night, and reading some of the other posts. I'm not overly familiar with the node access system, but the example node access module in the api documentation implements a separate table for tracking access, but simple_access directly modifies the node access table. Is that the right way to do things? I noticed other posts again simple_access saying that it wasn't working with other access control modules - is that part of the problem?

This current patch node applies, and with the new structure it is no longer needed.