Posted by chx on December 16, 2007 at 10:10pm
4 followers
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | documentation |
| Category: | task |
| Priority: | normal |
| Assigned: | chx |
| Status: | closed (fixed) |
Issue Summary
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:
<?php
function node_revision_delete($node, $revision) {
if (user_access('administer nodes')) {
if (node_access('delete', $node)) {
?>node_accesshas a shortcut for node administers returning aTRUE. 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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| node_revision_problem.patch | 9.42 KB | Ignored: Check issue status. | None | None |
Comments
#1
Better code for theme.inc
#2
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.
#3
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 argumentskey is documented because even I got confused about it for a second :)#4
Further reading the patch, the menu fix is wrong, it should be is_int and not is_numeric.
#5
Now I get this on node/1/revisions/12/delete
Access deniednotice: 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 :)
#6
I forgot to rebuild my menu after the last round of changes, silly me! I also forgot how access callback is inherited...
#7
All works fine now.
#8
- 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 :)
#9
#10
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.
#11
Simplified the access control a tiny little bit.
#12
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.
#13
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.
#14
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.
#15
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.
#16
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 :)
#17
Changing status for the upgrade guide.
#18
subscribe
#19
#20
Automatically closed -- issue fixed for two weeks with no activity.