Notice: Trying to get property 'type' of non-object in node_node_access()

[argiepiano]

Description of the bug

This notice is thrown when using entity_access to check node (and possibly other entities') "create" access for a user that doesn't have a "Bypass content access control" permission.

Steps To Reproduce

Create a user without Bypass permission. Then run the following in Devel:

$user = user_load(9); // In my case, this user has uid == 9.  To test, you must use the uid of the user you created.
dpm(entity_access('create', 'node', NULL, $user ));

Actual behavior

Backdrop throws the Notice in the subject of this issue.

Expected behavior

There shouldn't be a Notice.

Additional information

The third argument of entity_access can either be a node entity, or null (as in this case). entity_access invokes Node:createAccess which allows a null value for the $bundle (first) argument. All good so far.

The problem happens when Node:createAccess() invokes hook_node_access. At this point node_node_access() is invoked. The first argument is null. But node_node_access() doesn't know how to handle null and the notice is thrown.

I guess an important question is - what does it mean to pass null as the first argument of node_node_access()? In the example above with entity_access() (which is actually invoked by Rules), the goal was to know if a user can create any bundle of nodes. Currently there is no node permission that specifically refers to the creation of nodes of any bundle - rather they are tailored to specific bundles (e.g. create page, create post). The one that comes closest is administer content.

So, one way to fix this might be for node_node_access() to interpret a null value in $node to mean the creation of any type of content, i.e. 'administer content', and to perform a check on that and return either allow or deny. Another way may be to return NODE_ACCESS_IGNORE. But the status quo leads to the Notice.

THEN, there is another problem: lines 251 to 261 on Node:createAccess(), where $bundle (i.e. NULL) is used to set the $rights static cache array. Not sure if this is on purpose or if it's an oversight.

I will submit a PR for a potential fix following fix (1) above. But I'll appreciate any thoughts.

[indigoxela]

There shouldn't be a Notice.

I'm not sure... In function node_node_access the $node parameter isn't optional. It can be a string (then it's the node type) or an object (then it's a Node). There are two more params, which are also not optional.

The node module runs hook_node_access, but other modules could do, too. Switching the logic and returning NODE_ACCESS_ALLOW might lead to unexpected results.

[argiepiano]

I'm not sure... In function node_node_access the $node parameter isn't optional.

Yes, but.... please notice that this hook WILL, in some occasions like the one I outlined, be invoked with an empty $node parameter. So the problem has to be fixed either with node_node_access, or in the core function (Node::createAccess) that invokes it with an empty $node.

My point: yes, I can see why my fix may not be ideal, but we need to provide a fix - otherwise we'll keep seeing that Notice. 🤷🏽

[indigoxela]

@argiepiano I get the point and I don't object, but I'm not sure if this approach is the right thing in the right place.

The code in entity_access causes me some head scratching:

  elseif ($op == 'create') {
    $entity_info = entity_get_info($entity_type);

    $class = $entity_info['entity class'];
    $bundle = NULL;
    if ($entity !== NULL) {
      $bundle = $entity->bundle();
    }
    return $class::createAccess($bundle, $account);
  }

So... if the operation is "create" and the $entity is set ... huh?? When would this ever be the case?

[argiepiano]

The code in entity_access causes me some head scratching

Yes, the code is weak and it hasn't been thought out thoroughly.

So... if the operation is "create" and the $entity is set ... huh?? When would this ever be the case?

There are occasions when you want to send a newly created (unsaved) entity to entity_access to check if the user has permission to save it (the 4 operations supported are create, view, update and delete). For example, often entity add functions will create an "empty" entity without an entity id to send it to the creation form. I think in part the confusion is that we use the word 'create' access when in fact what we are checking for in those cases is ' permission to save to database a newly created entity that is currently existing in memory and will disappear after the page request ends'.

Back to the Notice, I think we could modify node_node_access() to return NODE_ACCESS_IGNORE if the $node is null, and let the person/module who wants to check for global creation access of any node type create its own hook_node_access().

And if this is the plan, we should modify the documentation of entity_access that says "If no entity is given, it will be determined whether access is allowed for all entities of the given type." as this is currently not happening in core, as you saw above.

[indigoxela]

There are occasions when you want to send a newly created (unsaved) entity to entity_access to check if the user has permission to save it

Code wise then we wouldn't end up in elseif ($op == 'create'), because the prior if (!empty($entity)) already strikes, right?

I understand that you need a solution for Rules, and you prefer to have it sooner. On the other hand it's function entity_access() doing the wrong thing here, not function node_node_access(). Correct me if I'm wrong.

It might be possible to use your PR as a temporary workaround to not further block contrib, but then we should overhaul entity_access and possibly - when that fix gets released (1.22.0?), revert the workaround in node_node_access.

@argiepiano would that work for you?

[argiepiano]

I understand that you need a solution for Rules, and you prefer to have it sooner.

I decided to use entity_plus_access() in Rules (which makes more sense since Rules is deeply integrated with Entity Plus). entity_plus_access() is the verbatim version of D7's Entity API entity_access(). I'm still checking that access checks are done correctly in Rules, but I believe this Notice won't show up. Still it'd be good to fix this at some point, as there will be other cases when this Notice will show up for others.

Code wise then we wouldn't end up in elseif ($op == 'create'), because the prior if (!empty($entity)) already strikes, right?

@indigoxela not quite. I will respond to this part later.

[indigoxela]

@indigoxela not quite. I will respond to this part later.

Sure, take your time, no need to hurry. In the meantime I decided to rather fix the base problem, than only the symptom, see issue #5474.