node_add needs a sanity check [#484060]

After a bit of playing with some of my own node access work, it's come to my attention that node_add (and node_delete) make their own checks to node access irregardless of what the menu system that controls access to them has to say about the matter. In short if you were to menu_alter a particular node type to allow anonymous users to create it, node_add would still deny them access to do so unless the corresponding perm were associated with anonymous users. This becomes rather ugly when we look at a situation that might break outside the drupal norm e.g. per grant id permissions. Namely a user may create nodes of type x when on node 119, but not when on node 118 (think OG). In this situation, with the way it works currently, a site administrator would have to simply turn on "create" access to all roles for all affected content types and pray he had his access settings correct.

If we remove the secondary access check on node_add/node_delete then we're essentially entrusting our future to the menu system. I feel this is probably a better method than being locked into what we have right now.

This issue definitely affects #345475: node_delete checks for access before deleting.

Eclipse

Comment	File	Size	Author
#4	node_add.patch	811 bytes	EclipseGc
#4

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

agentrickard

he/him

English

Georgia (US)

CreditAttribution: agentrickard commented 7 June 2009 at 15:03

This is pretty weird. AFAIK, node_add() should only be:


/**
 * Present a node submission form.
 */
function node_add($type) {
  global $user;
  $node = array('uid' => $user->uid, 'name' => (isset($user->name) ? $user->name : ''), 'type' => $type, 'language' => '');

  drupal_set_title(t('Create @name', array('@name' => $types[$type]->name)), PASS_THROUGH);
  $output = drupal_get_form($type . '_node_form', $node);
  return $output;
}

Since the _node_add_access() check handles all the other parts. I have no idea why the docblock says " or a set of links to such forms."

Comment #2

sun

German

Karlsruhe

CreditAttribution: sun commented 11 June 2009 at 03:32

+1 - makes absolutely sense.

Comment #3

pwolanin CreditAttribution: pwolanin commented 17 June 2009 at 22:26

This bit me too, wondering if I filed an issue before?

Comment #4

EclipseGc CreditAttribution: EclipseGc commented 17 October 2009 at 18:20

Assigned:	Unassigned	» EclipseGc
Category:	feature	» bug
Status:	Active	» Needs review

File	Size
node_add.patch	811 bytes

Sorry it took so long to get to this. Quick justification for getting this in:

1.) node_delete() is already modified in this way per #345475: node_delete checks for access before deleting && http://api.drupal.org/api/function/node_delete_multiple/7
2.) _node_add_access() already checks this and is the access callback for the menu item in question
3.) this will make the system more flexible for developers who'd like to determine for themselves whether a user can add a node or not, giving rise to some much more powerful permission/gid based systems.

I can't get my system to pass tests in a clean environment, but hopefully the bot will like it :-D

Eclipse

Comment #5

EclipseGc CreditAttribution: EclipseGc commented 17 October 2009 at 18:50

Sun asked me to details this issue a little more for those who it has not bitten:

The menu system is already equipped with the ability to check various access callback. node/add does this via the _node_add_access() function which checks to see if the user who's attempting to access this page has the permission to do so. This is all we really need as it allows module developers to hijack node/add via hook_menu_alter() and put their own access callback there that runs custom code (and in most cases probably defaults back down to _node_add_access() functionality if their cases aren't met).

Currently this is impossible as node_add() literally does the same check that _node_add_access() is doing again preventing contrib modules from setting their own access callbacks with any meaningful success. This fails spectacularly if you try it currently heh... Modules that need this functionality are required to provide their own copy of node_add() minus this particular portion of the code.

This patch will simply remove the second check that cannot be overridden, and will free contrib modules up to utilize core code instead of having to provide their own copies of the same function. I hope this clarifies the issue at hand.

Eclipse

Comment #6

sun

German

Karlsruhe

CreditAttribution: sun commented 17 October 2009 at 19:29

Status:

Needs review

» Reviewed & tested by the community

Thanks!

Comment #7

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 2 November 2009 at 04:46

Version:

7.x-dev

» 6.x-dev

Thanks for the nice summary @ #5. I agree that removing this duplicate access check makes sense, and is consistent with what we do everywhere else in core. Probably a hold-over from a much earlier version of Drupal.

Committed to HEAD. Marking down to 6.x for consideration. Still applies with some fuzz.

Comment #8

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 2 November 2009 at 04:46

Priority:

Critical

» Normal

Also, while this is annoying, it's not critical. Drupal works fine without this patch.

Comment #9

EclipseGc CreditAttribution: EclipseGc commented 3 November 2009 at 02:53

Hmmm, I don't recall marking it critical... sorry 'bout that!

Comment #10

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 6 November 2009 at 08:21

I've asked the security team to look at this issue. It is possible that certain contrib modules rely on this function checking access when reused, so while I agree in principle that the access check should be removed, I'm not 100% convinced that we should do it in a point release.