I'm working on a new module: http://drupal.org/project/volunteer_timeslots
And I came acoss a perplexing situation: for this module I have a custom node type representing an event is created, and in the node edit form "organizers" are entered, and saved as part of the data in a separate table as the uid values for the corresponding site users.
I set up my implementation of hook_access such that these "organizers" could edit the node where they are listed. The problem: if an organizer edits the node and removes herself from the list of organizers, the update fails because of the code below in node.module which re-checks the permission, even though the edit form should never have been presented without that permission.
It seems this may not present a problem for other modules doing access control, such as TAC, where the node_access table is not update until during the node save. Obviously I could work around this problem, but it seems like this check is unnecesary.
bug is present in 4.7 and HEAD (5.0).
function node_form_submit($form_id, $edit) {
global $user;
// Fix up the node when required:
$node = node_submit($edit);
// Prepare the node's body:
if ($node->nid) {
// Check whether the current user has the proper access rights to
// perform this operation:
if (node_access('update', $node)) {
node_save($node);
watchdog('content', t('%type: updated %title.', array('%type' => theme('placeholder', t($node->type)), '%title' => theme('placeholder', $node->title))), WATCHDOG_NOTICE, l(t('view'), 'node/'. $node->nid));
drupal_set_message(t('The %post was updated.', array ('%post' => node_get_name($node))));
}
}
Comment | File | Size | Author |
---|---|---|---|
#13 | access_check_original_node47_13.diff.txt | 1.01 KB | pwolanin |
#12 | no_access_check_in_node_form_submit_12.patch | 1.99 KB | pwolanin |
#7 | access_check_original_node47_7.diff.txt | 1.01 KB | pwolanin |
#3 | no_access_check_in_node_form_submit.patch | 1.75 KB | chx |
Comments
Comment #1
chx CreditAttribution: chx commentedI have mixed feelings on this. One, it's not really necessary any more, it is likely to be pre-fapi code and protected from direct POSTing to node/x/edit. On the other hand I am a bit afraid to remove this safeguard. Just a little bit... Moshe? Heine?
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedI feel that this is cruft code and should be removed. the access check burden is on the code displaying the form.
Comment #3
chx CreditAttribution: chx commentedOK, I talked with Heine too and we have an agreement that this not the place to access check. I looked at node_add and see a
if (isset($types[$type]) && node_access('create', $type)) {
protecting the drupal_get_form call. node_page_edit is protected by the menu system.Comment #4
pwolanin CreditAttribution: pwolanin commentedSo, if this is a bug, should we be looking at a patch for both 4,7 and 5.0, or only 5.0?
Comment #5
Heine CreditAttribution: Heine commented5.0 only, though a patch for 4.7 that changes the node_access call to operate on the existing node, not the edited one would probably be acceptable.
Comment #6
pwolanin CreditAttribution: pwolanin commentedOk, sounds easy enough. I'll see if I can roll a 4.7 patch as you suggest, with a call to node_load to get the original node.
Comment #7
pwolanin CreditAttribution: pwolanin commentedHere's a patch for 4.7 where the access check is done using the original $node object. Seems to solve the problem I initially reported.
Comment #8
Kjartan CreditAttribution: Kjartan commentedMoving to 4.7.4.
Comment #9
Kjartan CreditAttribution: Kjartan commentedSorry my bad, moving back to cvs, thats what I get for reading issues too quick.
Fails to apply against current HEAD though.
Comment #10
pwolanin CreditAttribution: pwolanin commentedThe patch in #3 should be applied to 5.x/HEAD while my patch is a more minimal one for 4.7.x. (thought the approach would work for 5.x as well).
Which patch doesn't apply to which branch?
Comment #11
Kjartan CreditAttribution: Kjartan commentedThe 5.0 patch (no_access_check_in_node_form_submit.patch by chx) fails to apply against HEAD. Haven't tested the 4.7 patch just yet as I am focusing on 5.0 issues.
Comment #12
pwolanin CreditAttribution: pwolanin commentedrerolled patch from #3 against current HEAD
Comment #13
pwolanin CreditAttribution: pwolanin commentedpatch from #7 still applies with offset. Attached patch is the same, but against current 4.7 branch.
Comment #14
Kjartan CreditAttribution: Kjartan commented5.0 patch applies fine now, and the code looks fine. Should be safe to commit based on the earlier comments thats its just cruft and not necessary.
Comment #15
drummCommitted to HEAD.
Comment #16
pwolanin CreditAttribution: pwolanin commentedchanging version to 4.7, the patch for 4.7 in #13 is different (smaller) than the patch for 5.x to minimize the changes.
Comment #17
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedapplied
Comment #18
(not verified) CreditAttribution: commented