Recent patch to change array2object() into a cast bring fourth this bug:
users with create new forum topics right are forbiddent to do so.

The problem is in user.module. E.g. node/add page is handled by node_add() function. That function calls node_access('create', $type), where $type is a string, e.g. 'forum'. This is not correct, as node_access() expects node object/array there.

It's weird that it worked before, with array2object().

When node_access('create', 'forum') is called then:
1. array2object('forum') produces 'forum' (a string)
2. (object) 'forum' produces object: class stdClass { var $scalar = 'forum'; }

It worked for 1. because module_invoke(node_get_base($node), 'access', $op, $node); returned TRUE (didn't checked why).

With 2. that do not return TRUE, and the access fails.

CommentFileSizeAuthor
#9 node_access-42955_1.diff1.56 KBCvbge
#2 node_access-42955_0.diff665 bytesCvbge
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Cvbge’s picture

Cvbge CreditAttribution: Cvbge commented
Title: Users can't create new content (forum topics) - incorrect use of node_access() » Users can't create new content (e.g. forum topics) - incorrect use of node_access()

Users with 'administer nodes' can add new content, others can't.

Cvbge’s picture

Cvbge CreditAttribution: Cvbge commented
FileSize
node_access-42955_0.diff665 bytes

Here's a proposed patch

Cvbge’s picture

Cvbge CreditAttribution: Cvbge commented
Status: Active » Needs review
Cvbge’s picture

Cvbge CreditAttribution: Cvbge commented
Title: Users can't create new content (e.g. forum topics) - incorrect use of node_access() » Users can't create new content (e.g. forum topics)

In fact this bug was created by the mentioned array2object() patch...

OTOH, there are issues with accessing a string as object ($node->format) with array2object() or accessing not existing object variable with (cast)

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community

This is a bit weird fix, yet it's working and does not require much code change. Otherwise we would need to fix up the three callers of this, and these type of loops (two of them):

    foreach (node_get_types() as $type => $name) {
      if (node_access('create', $type)) {

can't really be written in a short manner.

So, I RTC'd this.

Chris Johnson’s picture

Chris Johnson CreditAttribution: Chris Johnson commented

node_access() should not be called with a string as a second argument. It should always be a node, either as an array or an object.

There are 3 places in core where it the API is used incorrectly. It seems like we should fix those 3 places (2 in node.module, 1 in blogapi.module).

There are about a dozen places in contrib/modules in fewer files where the same mistake is made.

It seems like we should just fix core and require correct usage in contrib.

Cvbge’s picture

Cvbge CreditAttribution: Cvbge commented
Status: Reviewed & tested by the community » Needs work

What about node_access_type() which would take type as second arg and have similar logic as current node_access()?

Cvbge’s picture

Cvbge CreditAttribution: Cvbge commented

OTOH the node_access_type() could only work for 'create' check, so it's better to integrate it with normal node_access() function.
node_access() needs also a doc update.

Cvbge’s picture

Cvbge CreditAttribution: Cvbge commented
Status: Needs work » Needs review
FileSize
node_access-42955_1.diff1.56 KB

Here's the patch, not tested.

Cvbge’s picture

Cvbge CreditAttribution: Cvbge commented

node_access() should not be called with a string as a second argument. It should always be a node, either as an array or an object.

It seems the docs for the node_access() function were wrong. The hook_node_access() function has the 'create' $op and string $node described.

Chris Johnson’s picture

Chris Johnson CreditAttribution: Chris Johnson commented

I'm confused. I'm not seeing any hook_node_access() function in core. What am I missing?

Cvbge’s picture

Cvbge CreditAttribution: Cvbge commented

I'm sorry, I meant hook_access.

Dries’s picture

Dries CreditAttribution: Dries commented

I used to be on the other side but now, I come to prefer option #6.
Though, I'm willing to go with the patch if that is considered the cleanest solution.

Cvbge’s picture

Cvbge CreditAttribution: Cvbge commented

I currently consider this the best solution. But if someone writes patch for the #6 way, I'll review and *maybe* I'll change my mind (if the patch is really better)

chx’s picture

chx CreditAttribution: chx commented

As create is fundamentally different from others (no nid!) I still like the current approach. How would you fix up the foreaches I quoted above?? Create a $node = array('type' =>$type) just to be able to call up node_access? Let's face it, that's not practical.

Dries’s picture

Dries CreditAttribution: Dries commented

Committed to HEAD. Thanks.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed
simon rawson’s picture

simon rawson CreditAttribution: simon rawson commented

I'm too slow! I was just about to post to say that I thought we should go for the

$node = array('type' =>$type);

solution. It is consistent, at least. Surely it is bad practice to have a function that takes an array/object normally, but takes a string some times.

At the very least the comments need improving! There is not even a mention of 'create' as a possible op.

chx’s picture

chx CreditAttribution: chx commented

Simon, we already have such functions. _node_names is one such example.

Cvbge’s picture

Cvbge CreditAttribution: Cvbge commented

$node = array('type' =>$type);

I've used this approach in my first patch, in #2. I changed it because it creates false "node" object with only type specified for the 'create' op. And the patch would be a bit bigger.

Also, the last patch contains changes that add 'create' $op description.

simon rawson’s picture

simon rawson CreditAttribution: simon rawson commented

@chx

Thanks for that. I'm learning more about Drupal every day. As a relative new comer, it is often easy to forget that Drupal is a piece of software which has evolved and, as such, sometimes things are always the way you think they might be.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Fixed » Closed (fixed)