"Undefined group in gnode_node_access() line 306" after Patch #55 of Parent Issue when viewing gnode if logged in without “Bypass group access control” permission

I saw Comment #61 in the Parent Issue #2317195-61: Allow more than one parent by node / subgroup by the patch author which basically said to try re-saving the Group configurations if there were any problems. So, I did. Two of the three Groups we have took a long time to re-save. After re-saving all three, I tried again to view gnodes while logged in as a Group Admin, and again got the errors.

Also, after doing the re-saving of Group configurations, editing a gnode (which apparently only User 1 can do without generating the errors) still shows “Not a group node” under “Group settings” even if it’s assigned to one or more Parent Groups:

Has anyone looked at this issue? Do we need to offer a bounty? How do we go about doing that? This is an absolute showstopper for a major project at a critical stage.

We especially need attention from @ctrlADel and @josephdpurcell as developer and refiner (respectively) of the Parent Issue patch which seems to be causing this, and of course @kristiaanvandeneynde as Project Maintainer. Thanks.

Just verified that this error happens regardless of the logged-in user unless the user is User 1 or a member of the Administrators role. This includes Group Admins as stated before, but also Group Members, Outsiders, and Users who are not a member of any Group.

If the node has been assigned to any Group(s), then the error appears on any attempt to view or edit it. If the node has not been assigned to any Group(s), then viewing and editing (where permissions permit) work just fine.

Again: this is a showstopper for a major system under development. We’re offering to pay for a resolution.

Going to Drupal 8 (which does seem to have a more robust version of Group) is not an option since other modules that this system absolutely depends on (such as IP Geolocation & Maps) have no usable D8 version, nor one in the pipeline (#2624188: [ip_geoloc] IP Geolocation Views & Mapsso far all that has been done is to convert the .info file to .yml format, and that has been the status since August 11, 2013).

It finally occurred to me to try to figure out just which Permission the Administrators had that the others lacked that the lack thereof triggered the error. I figured to start with the permissions actually implemented by Group.

Of the three default permissions plus the one created for each individual group type itself, the first two are Administrator-level. The third I had already assigned to some of the roles generating the error, so I knew that couldn’t be it.

So, to test, I assigned to a non-Admin Role the second Permission:

Bypass group access control
^{View, edit and delete all groups regardless of permission restrictions Warning: Give to trusted roles only; this permission has security implications.}

And lo! The users of that Role could now safely view gnode content without the error!

But since this is a “security implications” Permission, I’d rather not assign that to non-Admin-level Roles, and also it defeats the whole point of having group access if we bypass the access rules.

Still, though, maybe this information will help track down the underlying cause of these errors?

I may later try assigning just the other Admin-level Permission:

Configure Group module
^{Configure group types, group roles, member fields, etc. Warning: Give to trusted roles only; this permission has security implications.}

to a non-Admin Role and see what happens.

Okay, I think I finally fixed it, even though I know next to nothing about Drupal module development.

In looking over the Line 306 area of the patched …/sites/all/modules/group/modules/gnode/gnode.module, I found that it was in this function:

function gnode_node_access($node, $op, $account) {
  if (_gnode_get_mode() === GROUP_NODE_COMPLIANCE_MODE && $op !== 'create') {
    return NODE_ACCESS_IGNORE;
  }

  if (is_string($node)) {
    if ($op == 'create') {
      if (gnode_group_node_create_access($node, $account)) {
        return NODE_ACCESS_ALLOW;
      }
    }
  }

  // Make sure we are dealing with a published group node.
  if (empty($node->group) || !$node->status) {
    return NODE_ACCESS_IGNORE;
  }

  // If the user can bypass group access, he is allowed access.
  if (user_access('bypass group access', $account)) {
    return NODE_ACCESS_ALLOW;
  }

  // If the user can administer the group, he is allowed access.
  if (group_access('administer group', $group, $account)) {
    return NODE_ACCESS_ALLOW;
  }

  // Check access within all of the node's groups.
  foreach (group_entity_parent_gids($node) as $group) {
    if (group_access('administer group', $group, $account)) {
      return NODE_ACCESS_ALLOW;
    }

    switch ($op) {
      case "view":
        if (group_access("view $node->type node", $group, $account)) {
          return NODE_ACCESS_ALLOW;
        }
        break;

      case "update":
      case "delete":
        if (group_access("$op any $node->type node", $group, $account)) {
          return NODE_ACCESS_ALLOW;
        }

        if ($account->uid == $node->uid && group_access("$op own $node->type node", $group, $account)) {
          return NODE_ACCESS_ALLOW;
        }

        break;
    }
  }

  return NODE_ACCESS_DENY;
}

The problem is that it still has code (which was apparently added by the patch!) to check for single parent group access for a Group Administrator, as well as (and just before) the same code safely within a foreach right below it. The comment “// If the user can administer the group, he is allowed access.” appears twice in short succession, with nearly identical code and functionality, differing only in that the second is properly enclosed in a foreach loop since the $node->group is now an array instead of a single instance of Group.

The original version of this function had a line not far up that assigned $group to $node->group, but that line was removed in the patch without replacement. This is why this line fails: $group is undefined!

Since the functionality is duplicated below properly, all I had to do was comment out this snippet (I use three slashes instead of two for my debugging comments to help me find them later):

  // If the user can administer the group, he is allowed access.
///  if (group_access('administer group', $group, $account)) {
///    return NODE_ACCESS_ALLOW;
///  } /// ERRONEOUS with multiple parent groups! Redundant, too. See below:

This solved the problem, and so far as I know has no adverse side effects. I’ll do further tests to make sure that it maintains enforcement of proper gnode access, but it should since the same functionality properly implemented appears right below that.

If commenting that out has no adverse side-effects, then that section of code should probably be deleted entirely in the next revision of the patch.

I’m not familiar with how to go about rolling this into a new patch (as I said, I’m not yet a Drupal module developer nor maintainer, but would like to learn). So if someone could take care of this, I’d appreciate it.

Thanks, @rositis@gmail.com! It took me a few tries to successfully reverse the patch (I forgot that I had to first undo all of my own changes), but once I did and applied yours, it seems to be working fine (despite giving a “warning: 1 line adds whitespace errors.” message when I git-applied the patch).

You should submit this over at the parent thread.

Okay, I see a potential problem with this patch. It works fine for me, but it may not work fine in all situations. According to the PHP Manual, foreach will issue an error if used on a variable that isn’t an array or object.

Now here’s the thing: the former (and this) patch implement a “Group entity settings” admin tab at “…/admin/group/settings/entity”. The bottom section of this is as follows:

Entities in multiple groups

🗹 Group
🗹 Node
^{Choose if an entity can be in multiple groups. Entities in multiple groups can be updated and deleted by users with the necessary permissions in any of the groups it is in. After changing this session you must clear the site cache. It is not recommended that you go from multiple to single on a site which has entities in multiple groups without testing.}

Firstly, in the help text, the word “session” (which I boldfaced above) should probably be “setting.”

But more importantly, what do those checkboxes actually do? Does un-checking the “Node” checkbox switch back to the former, pre-patch implentation of gnodes, with $node->group returning a scalar Group instead of an Array of Groups? If so, then the seemingly redundant (and error-producing) code that I commented out and rositis@gmail.com deleted may turn out to be needed after all.

If this is how it works, then, for those who don’t want Multiple Parent Group functionality for Nodes despite having the patch installed (maybe they only want [sub]Groups to be able to have multiple parent Groups?), I strongly suspect that the code will now produce an error on the foreach loops for such sites.

What should probably be done is to enclose the whole node access testing code within a conditional (if (…) {…} else {…}) block testing on the setting of that checkbox (I’m not sure how to check for the configuration setting in code), with the if (…) {…} block enclosing the new version of the code in this patch which uses foreach loops to iterate through the $node->group array of parent groups, while the else {…} block would instead enclose the former version of the gnode code which acts on $node->group directly as a scalar Group reference.

Either that or remove the setting entirely and simply assume that Multiple Parent Groups should always be allowed, and that $node->group would always return an array, even if with only one element (or zero for that matter). Or maybe have the checkbox work by limiting the size of the array to no more than one for those sites who want to enforce having only one Parent Group for any GNode, but still have it stored as an array for consistency and compatibility with other code.

I assume that a similar issue could arise with the Subgroups functionality and the “Group” checkbox.

In reading up on the differences between Drupal 7 and 8 Entity APIs, D8 resolves this sort of thing by making everything a list. If I read and understood it right, all properties of all Entities are lists, even if it’s something like a Node’s “Title” that can logically only have one entry. D8 has constraints to enforce validations at more than just the form level, including enforcing minimum and maximum counts in the list.

I think that the D7 Group module should be written like the D8 one as much as possible, and Parent Group should be a list even if the checkbox to allow multiple Parent Groups is unchecked. That way all code to access parameters would access them as lists so there wouldn’t have to be constant checks to see if it should be a list or a scalar, thus preventing such errors.

I’ll post more details in the Parent Issue thread.