A GHOP participant, cwgordon7 showed me http://drupal.org/node/198587 which got me thinking "uh, what does the current system do"?

  • The access check is a sorry mess. This is a particular gem:
    function node_revision_delete($node, $revision) {
  if (user_access('administer nodes')) {
    if (node_access('delete', $node)) {

    node_access has a shortcut for node administers returning a TRUE. I have added a nice and logical access check here: first of all, the node nid and vid passed to node_load should be legal numbers (I checked and node_load uses a %d for vid, no security problem from passing in vid raw). Then we check for at least two revisions. Next, if the user is a node admin, we grant access. Finally, if it's a view or an update and the user has the necessary permission and can view/update both the revision in question and the current revision then we grant access.

  • Te code does not utilize the new menu system at all, even worse is the node_revisions code reminiscent of pre-Drupal 4.5 or whatever times.
  • Theme system loads the wrong node.

So, at the end of the day we have bugs fixed and a good amount of ugly code removed.

CommentFileSizeAuthor
#15 node_revision_problem.patch12.15 KBchx
#13 node_revision_problem.patch11.09 KBchx
#11 node_revision_problem.patch11.86 KBchx
#10 node_revision_problem.patch11.91 KBchx
#9 node_revision_problem.patch11.98 KBchx
#6 node_revision_problem.patch9.92 KBchx
#4 node_revision_problem.patch9.92 KBchx
#3 node_revision_problem.patch9.95 KBchx
#1 node_revision_problem.patch9.44 KBchx
node_revision_problem.patch9.42 KBchx

Comments

chx’s picture

chx commented
StatusFileSize
new node_revision_problem.patch9.44 KB

Better code for theme.inc

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

Made a new revision of a node. Then tried to delete the old revision (with two total):

Delete text takes me to:

node/%252Frevisions

Page not found

notice: Undefined property: stdClass::$vid (on a bunch of lines in node.module and node.pages.inc)

Deletion failed. You tried to delete the current revision.
chx’s picture

chx commented
Status: Needs work » Needs review
StatusFileSize
new node_revision_problem.patch9.95 KB

Well, there were numerous problems with that patch, I hope it's working well now. What's most important that we have found a menu bug! An additional goodie is that now the load arguments key is documented because even I got confused about it for a second :)

chx’s picture

chx commented
StatusFileSize
new node_revision_problem.patch9.92 KB

Further reading the patch, the menu fix is wrong, it should be is_int and not is_numeric.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

Now I get this on node/1/revisions/12/delete

Access denied
notice: Trying to get property of non-object > user.module
warning: array_fill() [function.array-fill]: Number of elements must be positive > database.inc
warning: implode() [function.implode]: Bad arguments > database.inc
warning: array_keys() [function.array-keys]: The first argument should be an array > user.module
user warning: You have an error in your SQL syntax;check the manual that corresponds to your MySQL server version for the right syntax to use near ')' at line 1 query: SELECT p.perm FROM role r INNER JOIN permission p ON p.rid = r.rid WHERE r.rid IN () > user.module
warning: Illegal offset type in isset or empty > user.module

Still a bit buggy :)

chx’s picture

chx commented
Status: Needs work » Needs review
StatusFileSize
new node_revision_problem.patch9.92 KB

I forgot to rebuild my menu after the last round of changes, silly me! I also forgot how access callback is inherited...

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

All works fine now.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Needs review

- The _is_node flag seems to be very hackish to me. We don't do this anywhere, and I don't think we should do it now. Instead of the arg() and node_load() code, you have 1.5 times as much code for the same stuff with a hackish flag.

- Was it ever supported that you can return different access values from your hook_access() implementation depending on the node revision? AFAIR this is supposed to work based on nid not on nid,vid. This looks like 'intereting use of hook_access()'.

- Revert gets the confirm form called from the menu, but delete does not?

- Good: Heaps of access checks centralized :)

chx’s picture

chx commented
StatusFileSize
new node_revision_problem.patch11.98 KB
  • I might have a bit more code in theme.inc but the old code is broken, period.
  • I separated out the check for the revisioned node and surely we need this check, I do not think you should be able to revert to a revision which you can't edit because the body format is something you can't access.
  • I have added a permission to delete revisions. If there is one to view and one to revert then why not for deletions?
  • Delete had other checks but sure, I can centralize that check too, it's not much code even. You can only get to that page by typing it in anyways, there is no link, so we do not need to provide a message and a redirect, we just need to stop deleting/reverting the wrong revision, so a 403 will do.
  • Menu now stores back the unserialized load functions. I checked both callers of _menu_load_objects (_menu_translate and _menu_link_translate) they take their item by reference, so it is fine. Also, load_functions are not used in any other place. I added a tiny piece of logic in case someone calls this function twice for the same item.
chx’s picture

chx commented
StatusFileSize
new node_revision_problem.patch11.91 KB

Wait, there is some superflous code in the access check, if the node load fails, then you get a 404 anyways and the access check is not called.

chx’s picture

chx commented
StatusFileSize
new node_revision_problem.patch11.86 KB

Simplified the access control a tiny little bit.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

Everything I tested worked except the delete revision permission for authenticated users - made a user, gave them the permission (and edit own etc.) - no delete link shows up and 403 if I browse manually to node/2/revisions/8/delete.

Same behaviour with #9, #10 and #11 fwiw.

Otherwise confirm pages are back etc. etc.

chx’s picture

chx commented
Status: Needs work » Needs review
StatusFileSize
new node_revision_problem.patch11.09 KB

I gave delete own story content, delete revisions and view revisions to my user and I can see the delete link and perform a deletion.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

This version works. Delete revision is dependent on having permission to delete your own, or any, of the content type you're deleting a revision from, which seems sensible.

I don't think we do any harm to add this permission at this stage of the release cycle, and it makes managing this workflow a lot more sane. If it is too late, it's not my decision - so RTBC.

chx’s picture

chx commented
StatusFileSize
new node_revision_problem.patch12.15 KB

Gabor asked me to keep the menu gory details in menu.inc. Surely I can do it... and provide a very useful function as a side effect.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

Now this looks way better. Thanks, committed.

Please update the upgrade guide with info on menu_get_object(), this becomes one of the gems of the new menu system :)

catch’s picture

catch
he/him
Primary language English
commented
Title: Node revisions are simply neglected » Document menu_get_object()
Component: node system » documentation
Category: bug » task
Priority: Critical » Normal
Status: Fixed » Active

Changing status for the upgrade guide.

pwolanin’s picture

pwolanin commented

subscribe

chx’s picture

chx commented
Status: Active » Fixed
Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)

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