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))));
    }
  }
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

I 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?

moshe weitzman’s picture

I feel that this is cruft code and should be removed. the access check burden is on the code displaying the form.

chx’s picture

Status: Active » Needs review
FileSize
1.75 KB

OK, 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.

pwolanin’s picture

So, if this is a bug, should we be looking at a patch for both 4,7 and 5.0, or only 5.0?

Heine’s picture

5.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.

pwolanin’s picture

Ok, 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.

pwolanin’s picture

Here'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.

Kjartan’s picture

Version: x.y.z » 4.7.4

Moving to 4.7.4.

Kjartan’s picture

Version: 4.7.4 » x.y.z
Status: Needs review » Needs work

Sorry my bad, moving back to cvs, thats what I get for reading issues too quick.

Fails to apply against current HEAD though.

pwolanin’s picture

The 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?

Kjartan’s picture

The 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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1.99 KB

rerolled patch from #3 against current HEAD

pwolanin’s picture

patch from #7 still applies with offset. Attached patch is the same, but against current 4.7 branch.

Kjartan’s picture

Status: Needs review » Reviewed & tested by the community

5.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.

drumm’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD.

pwolanin’s picture

Version: x.y.z » 4.7.4
Status: Fixed » Reviewed & tested by the community

changing version to 4.7, the patch for 4.7 in #13 is different (smaller) than the patch for 5.x to minimize the changes.

killes@www.drop.org’s picture

Status: Reviewed & tested by the community » Fixed

applied

Anonymous’s picture

Status: Fixed » Closed (fixed)