node revisions should only be viewable by admins

+1

I would also like to see the extra permissions added if possible:

'view node revisions' - just for viewing node revisions

'administer node revisions' - to delete node revisions or set them as default revision (i.e. current revision) for a node

I think this patch is important because e.g. if a revision is made because sensitive information was included in a past revision, such as a phone number, plain text email address (someone stupid didn't know about spammers or the contact form) then these should definitely NOT be accessible, and it would often be desirable to create a new revision to a node rather than to edit the current copy without creating a revision.

I disagree. I see revisions as a means of updating a document for the users. If a user has read the document, then sees that a revision has been made, I'd want to provide him with some sort of diff that shows him exactly what, as opposed to making him read the whole blasted thing over and over again. In essence, I want a a wiki diff between revisions (for example). Putting this permission in place would restrict my ability to do that.

Note to self: actually read the whole report before commenting.

-1 to JUST this patch. +1 to view/set revisions.

good point #2 morbis, but the revisions tab isn't accessible to normal users anyway... they only get to see the revisions if they know the URL to get there (?revision=x in 4.6; /revisions/x in head)

but i agree that its definitely better with the extra permissions

Please don't lump it in with administer nodes, please make a new permission.

Yes, please make this a separate permission.

In actuality, the use cases for viewing revisions probably needs a permission *per node type*.

E.g. a site uses "story" for front page content. Editors some times revise stories, but don't want those revisions public. The same site uses book pages like a wiki. It gives all users create/edit/maintain book privileges, and additionally wants them to be able to see revisions.

So, if we don't enable viewing revisions for node type, I would hope for a way to override this at the node type/module level -- so a custom wiki.module could automatically add a "view revisions" permission.

Revisions as a separate module might very well make more sense. Sites can choose to run with them enabled or not. Might make it easier to eventually do file versioning if we can hook into a module.

+1 for the permission(s), -1 for moving the revisions to a new module at this point.

I haven't tested the patch, but "delete revision" concerns me - it's the style of a wiki to NEVER delete a previous version of a document - to always have a permanent record of its manifestations. Annoyingly enough, I woudn't +1 these new permissions until "delete" became its own perm as well, separate from edit.

Sounds good to me!

Having node module set revision permissions for all other node modules contradicts the interface of the permissions screen. The current interface suggests that if I want to set revision permissions for books, I would look underneath book heading, not node.

Interface issues aside, if node module is going to set revision permissions for all other node mods why stop there? Why not "create $type", "edit own $type", etc?

Using the latest node.module listed above and CVS as of yesterday, when an authenticated user is given view and revert permissions for stories, I get the following error when attempting to review the revisions tab for a story created by that user or by another user:

user error: Unknown column 'grant_edit' in 'where clause'
query: SELECT COUNT(*) FROM node_access WHERE (nid = 0 OR nid = 2) AND CONCAT(realm, gid) IN ('all0') AND grant_edit >= 1 in /home/cyber/public_html/drupal-cvs/includes/database.mysql.inc on line 99.

When I try to set one as active, I get

warning: Cannot modify header information - headers already sent by (output started at /home/cyber/public_html/drupal-cvs/includes/common.inc:460) in /home/cyber/public_html/drupal-cvs/includes/common.inc on line 292.

And the Drupal page display is doubled with that same error. one page with "Access denied" and the other with "Page not found."

I also get some similar errors. I am using cvs from yesterday and the latest node.module (node_3, I believe) from this thread.

Here is my set up: three users
1. first user = site admin
2. authenticated user -- has view/revert permissions on all node types
3. contrib user (in created role "contrib") -- has administer node permission, no view/revert privileges

The site admin and contrib user can use view/edit/revert between various revisions with no problems.

When user 2 (authenticated user) attempts to view a post that has been revised, the following error occurs:

user error: Unknown column 'grant_edit' in 'where clause'
query: SELECT COUNT(*) FROM node_access WHERE (nid = 0 OR nid = 6) AND CONCAT(realm, gid) IN ('all0') AND grant_edit >= 1 in C:\apachefriends\xampp\htdocs\drupalcvs\includes\database.mysql.inc on line 99.

When the same user attempts to revert to an earlier version, the same error occurs.

And, I also get the same doubled display as Charlie -- Access denied on top, Page not found on bottom.

So as I see it, this adds:
revert revisions for book
revert revisions for page
revert revisions for story
view revisions for book
view revisions for page
view revisions for story

...to permissions for a node. To be like a wiki, I would also like to see:
revert revisions for own book
revert revisions for own page
revert revisions for own story
view revisions for own book
view revisions for own page
view revisions for own story

That will give someone who can post something the ability to view/revert their own work, but not anyone elses. Or am I missing something that already gives me that?

Thanks!

I think there should be one global permission "view revisions", exported by the node system. That is easiest for now; we can refine this later.
There might be some problems I'm overseeing though.

Perhas we could have a global permission, and then a permodule permission?

Kind of like themes, there's a global/general theme, and then each user can overwrite what theme they want. I think it would be nice if there was a global permission about revisions, and then each module could have their own node revision permsissions that supersceded the global ones. Just a thought.

First off, I do very much like the idea of this patch, not all users should be able to see revisions, so +1 on that :-)

As to the issue of per-module revisions, I don't think auto-generating these permissions for each node-type is good like killes mentioned. However, I don't think *just* a global setting is good enough either.

I think the best solution would be to have a global node setting 'view revisions, revert revisions' etc. This should work for all node-types.

However, each module that defines content-types should have some sort of override to this, perhaps a hook_revisions() or similar where they can set their own 'view revisions, reveret revisions' for *their own* content-type they define.

That way, we get the default global behavior which should be good in about 90% of the cases, but then for those content-types that need different permissions (maybe everyone should see revisions like a wiki mentioned above), this global setting can be overridden.

Compromise at it's best :-)

Well the access control seems to work, however I'm getting some weird results. I allowed anonymous users the ability to 'view' revisions.
However, if I try to view older revisions things go crazy and I get a 'page not found' along with an 'access denied'. However, I can view the current revision no prob, just can't review older ones.

Additionally, while logged in as uid=1, everything works great.

Two bugs:

1. If you make an older revision active, the *last* revision is always copied instead of the revision that was supposed to be active.

2. Anonymous users cannot view older revisions even if they have the permission to do so, they just get access denied when trying to view one of these revisions. This is happening if they have the following permissions for node: 'access content', 'revert revisions', 'view revisions'

Problem found, line 1918 or so:

+ if ((user_access('view revisions') || user_access('administer nodes')) && node_access($node, 'view')) {

node_access call is backwards.

This patch fixes the two issues. Thanks for testing!

Patch still seems to copying the *last* revision instead of the one I want to make active (say the 1st revision of 3, the 3rd is always copied).

Ok that is working now, it is copying the correct revision, however anonymous users with those permissions cannot revert revisions now, access denied, even if they have the 'view' and 'revert' permissions.

If you remove this check in node_revision_rollback: && node_access('update', $node) it works.

This works when I have given anonymous users the folllowing node permissions: 'access content', 'revert revisions', 'view revisions' ... seems that 'update' permission only comes when you can administer a node, which in the case, I don't want to happen, just want anonymous users to view and revert revisions (wiki like).

Right but when I remove it everything works as expected, otherwise there is no way to be able to let anon users revert revisions even if you give them the permission. This probably would happen to any other roles like this too.

I have thoroughly tested this patch and it is indeed working now quite well I think it is ready for a commit.

However, I will say, revisions to me is still not that intuitive. For example, I would like to make Drupal work more like a wiki, so I allow anonymous users the ability to create and revert revisions, along with view nodes, and then allow them to create and edit their own pages. Great, but without the 'adminster nodes' permission, there is no way for them to ever select 'create new revision' when creating a new page. And if I as an admin created a bunch of pages, there is no way for these users to 'revert' my permissions since I am the owner of those nodes.

Perhaps, with the permission 'revert revisions' the option 'create new revision' should be shown? Also, perhaps 'revert revision' should be more global? Perhaps not, but to me, it's not clear how one would go about doing either of these and still keep things locked down with the 'adminster nodes' permission.

However, this patch is certainly a much needed step in the right direction, and what I'm bringing up is more usability issues that another patch would likely need to address. Unless of course, I am missing something painstakingly obvious, but I hope not :-)

Right something simple yes would work, above was just an example thinking out loud, someone not familiar with Drupal that is :-)

Committed to HEAD. Thanks.