Performance issues with node access control

Also see https://www.drupal.org/node/2878389#comment-12089798

1. +++ b/modules/gnode/gnode.module
   @@ -109,136 +108,74 @@ function gnode_node_access(NodeInterface $node, $op, AccountInterface $account)
    function gnode_node_grants(AccountInterface $account, $op) {
   

   This doesn't seem to work. I love the idea of adding static caching. Have you tested this with custom group roles (not synchronized from the user roles)?

2. +++ b/modules/gnode/gnode.module
   @@ -269,31 +207,172 @@ function gnode_node_access_records(NodeInterface $node) {
   +        $role_id = $synchronizer->getGroupRoleId($group_type_id, $role);
   

   $role is a object, and getGroupRoleId() expects a role ID. So we need to do $synchronizer->getGroupRoleId($group_type_id, $role->id())

3. +++ b/modules/gnode/gnode.module
   @@ -269,31 +207,172 @@ function gnode_node_access_records(NodeInterface $node) {
   +        $role = GroupRole::load($role_id);
   

   We need a use statement for Drupal\group\Entity\GroupRole

4. +++ b/modules/gnode/gnode.module
   @@ -269,31 +207,172 @@ function gnode_node_access_records(NodeInterface $node) {
   +        $can_view = $role->hasPermission("view $perm_type entity") ? 1 : 0;
   

   Since $role can be empty, this could lead to an error. Probably best to add a check for $role above like:

   if (!$role) {
     continue;
   }
   

There is another issue I found trying to debug this, the subgroup patch in #2736233: Port Subgroup (ggroup) to the D8 version allows membership inheritance. An event was added to the hasPermission() method of a group to also check the permission in subgroups/supergroups. This makes it harder to change the grants to roles as you are proposing in the patch.

Not sure what to do, but just wanted to let you know so maybe we can figure out another way?

This already helps A LOT! Page load was more than 3 times as fast. Number of queries went from 1169 to 689.

Changes look good! Thanks.
There is still an issue for possible membership inheritance through subgroups. Could you maybe also take a look at this? Maybe we can come up with some solution to allow these changes while still keeping permission inheritance?

Yeah, subgroups are pretty complex. Just found out the node grants doesn't work well with subgroups in the current situation as well. The grants are only added for the groups you are a member of. Since you can inherit roles/membership for parent groups or subgroups, this is an issue.

This needs attention for sure, which might not be a problem of the node grants hook.

This looks good overall, just have a few nitpicks/suggestions.

1. +++ b/modules/gnode/gnode.module
   @@ -109,136 +109,79 @@ function gnode_node_access(NodeInterface $node, $op, AccountInterface $account)
   + * - 'gnode_anon': Given to anonymous users.
   + * - 'gnode_out': Given to group outsiders.
   + * - 'gnode_member': Grant for a member of a particular group.
   + * - 'gnode_author': Grant update or delete access to authors.
   

   Shouldn't this also mention 'gnode_out:role_id' for the synced roles, and 'gnode:role_id' for the group roles?

2. +++ b/modules/gnode/gnode.module
   @@ -109,136 +109,79 @@ function gnode_node_access(NodeInterface $node, $op, AccountInterface $account)
   +  // If the user is anonymous, they can't be part of any groups, so we can just
   +  // abort here.
   +  if ($account->isAnonymous()) {
   +    $grants[$uid]['gnode_anon'] = [0];
   +    return $grants[$uid];
   +  }
   

   Maybe the comment here should mention that we assign a generic grant ID of '0' rather than the group ID, since this permission isn't specific to a given group.

3. +++ b/modules/gnode/gnode.module
   @@ -109,136 +109,79 @@ function gnode_node_access(NodeInterface $node, $op, AccountInterface $account)
   +  // We can also abort early if they can bypass group access.
   +  if ($account->hasPermission('bypass group access')) {
   +    $grants[$uid]['gnode_bypass'] = [0];
   +    return $grants[$uid];
   +  }
   

   Maybe the comment here should mention that we assign a generic grant ID of '0' rather than the group ID, since this permission isn't specific to a given group.

4. +++ b/modules/gnode/gnode.module
   @@ -109,136 +109,79 @@ function gnode_node_access(NodeInterface $node, $op, AccountInterface $account)
   +  // You can always be considered an outsider. (Joining the group cannot give
   +  // you LESS access.)
   +  $grants[$uid]['gnode_out'] = [0];
   

   Maybe the comment here should mention that we assign a generic grant ID of '0' rather than the group ID, since this permission isn't specific to a given group.

5. +++ b/modules/gnode/gnode.module
   @@ -109,136 +109,79 @@ function gnode_node_access(NodeInterface $node, $op, AccountInterface $account)
   +  // Add the synced roles.
   +  $user_roles = $account->getRoles(TRUE);
   +  if (count($user_roles)) {
   +    foreach ($user_roles as $role) {
   +      $grants[$uid]["gnode_out:$role"] = [0];
   +    }
   

   Maybe the comment here should mention that we assign a generic grant ID of '0' rather than the group ID, since this permission isn't specific to a given group.

6. +++ b/modules/gnode/tests/src/Kernel/GroupNodeGrantsTest.php
   @@ -15,67 +19,89 @@ class GroupNodeGrantsTest extends GroupNodeAccessTestBase {
   +    $this->assertEquals(['gnode_anon' => [0]], $grants, 'Anonymous users only receive the bypass grant.');
   

   Shouldn't this state 'Anonymous users only receive the anonymous grant.' rather than '...the bypass grant.'?

7. +++ b/modules/gnode/tests/src/Kernel/GroupNodeGrantsTest.php
   @@ -15,67 +19,89 @@ class GroupNodeGrantsTest extends GroupNodeAccessTestBase {
   +  public function testRoleSyncGrants($op) {
   +    $grants = gnode_node_grants($this->account, $op);
    
   -    // Test 'delete any' permissions.
   -    $this->assertTrue(in_array($this->groupA2->id(), $grants['gnode:a']), 'A-group: Outsider can delete any A-nodes.');
   -    $this->assertTrue(in_array($this->groupB1->id(), $grants['gnode:b']), 'B-group: Member can delete any B-nodes.');
   +    // Ensure we didn't end up with the faux "anonymous" or "authenticated"
   +    // roles.
   +    $this->assertNotContains('gnode_out:' . RoleInterface::ANONYMOUS_ID, $grants, 'Anonymous outsider not given.');
   +    $this->assertNotContains('gnode_out:' . RoleInterface::AUTHENTICATED_ID, $grants, 'Authenticated outsider not given.');
   +  }
   

   Would it be feasible to simulate creation of a synced role and checking that it IS set appropriately here, in addition to checking that the faux roles are not set?

Looks good to me. Thanks @kevin.dutra!

I see there is a lot of speed after applying the patch. From 10s to 1s. And also after apply of this @https://www.drupal.org/node/2895988

Sweet, reviewing the patch now at DrupalCamp BE.

@seanB I was the room monitor for your session just now, what a coincidence :)

Re #9:

As a side note, it'd be great if there was some nicer DX around adding a group role to a membership -- it's not super straightforward.

There's no dedicated method for it (yet), no. Currently it requires you to get the group_roles field from the representative GroupContent entity. Please open a feature request for this if you haven't already. Issue queue is a bit overwhelming right now :)

@@ -69,13 +69,17 @@ class GroupMembership implements CacheableDependencyInterface {
-  public function getRoles() {
+  public function getRoles($include_implied = TRUE) {
   }

While this doesn't change any existing functionality, it does count as a minor API break. I'm considering to let this one slide for a mention in the release notes, though.

Looking into the rest of the patch still.

I'm still looking into this but there's two parts in the code that have me worried so far:

  // You can always be considered an outsider. (Joining the group cannot give
  // you LESS access.) The grant is not group specific, so we use an ID of '0'.
  $grants[$uid]['gnode_out'] = [0];

You can get less access by joining a group. It's perfectly possible for me to allow only outsiders to see "call to action to join" type of content and disallow members to see that under any circumstance. You can give any outsider role the "view node type X" permission and not give it to members.

/**
 * Implements hook_node_access_records_alter().
 */
function gnode_node_access_records_alter(&$grants, NodeInterface $node) {
  // If we have group access records, then groups alone should govern who has
  // access. Remove all other grants.
  if (isset($grants['gnode_bypass'])) {
    foreach ($grants as $i => $grant) {
      if (strpos($grant['realm'], 'gnode') !== 0) {
        unset($grants[$i]);
      }
    }
  }
}

This is not how the node access layer is supposed to work. In D7 we used to let Group trump all other modules but then people told us to play nice, so we did. We need to respect other access modules' settings.

I'm sorry, I really appreciate the effort but this patch needs work.

There is one major improvement in this patch that holds merit: If you are anonymous to one group, you are anonymous to all groups. So you're right to add all of the node's anonymous group permissions together into a gnode_anon realm and assign it a realm ID of 0. I'm sure this alone will lead to a huge performance increase.

But we can't similarly throw all of the outsiders in one big pool. Because you may be an outsider to group 1 and not 2, you must specify the group ID that the access record came from for outsiders. If not, you could run into the following scenario:

Set-up

• Outsiders to group 1 of type X can see "CTA" content, members can't
• Outsiders to group 2 of type Y can't see "CTA" content, members can't either
• CTA content "Join me!" is added to group 1 and 2

Tests

• Alice is an outsider to both groups and can see the CTA both before and after your patch because she is an outsider to group 1.
• Bob is an outsider to group 1 and member of group 2 and can see the CTA both before and after your patch because he is an outsider to group 1.
• Charlie is a member of group 1 and an outsider to group 2 and can't see the CTA before your patch, but can see it after your patch. Because your patch combines the permissions for outsiders of group 1 and 2 into the same 0-id-realm. Effectively granting Charlie access he shouldn't be getting.

This is why the original code did not keep track of grants/access records per audience because it doesn't work that way. As I said earlier, though: You uncovered an excellent optimization for anonymous users!

How about we keep the existing code and only add a 'gnode_anonymous' realm that has the power of short-circuiting gnode_node_grants() just like the bypass grant already does? We'd still need to figure out a way to improve performance for logged in users, though.

Also can someone post some numbers or experiences with the performance? For what audiences does it work well and for what audiences is it really slow. And is it slow on the database server (big query) or on the PHP side (lots of processing)?

Facing issue on node update the node_access table is not getting updated.

Eg: I have group A and group B, node/1 attached to group A. Next, I have added the same node to group B. But its not adding the gid in node_access.

In function gnode_node_access_records the code is like entering access permissions only once. If added the node in other group the node access is not adding.

Yup, I just ran into that myself while trying to write a new patch for this issue. But what you're reporting is not within the scope of this issue and probably warrants its own bug report if there isn't one already.

This isn't complete yet as I haven't adjusted the existing tests nor added new ones for the added functionality, but I figured I'd post this already so you guys can review. This takes the excellent optimization for anonymous users and implements it. Please review.

Edit: The new idea still needs to be reflected in gnode_node_grants() where we short-circuit that one for anonymous users.

This actually has the full functionality change. Will adjust/add test coverage next.

...

I think this is complete. It also checks whether subject entities are resaved when added to a group to counter the report made in #27. Can anyone test the performance of this for anonymous users both before and after the patch?

Patch makes sense to me, was derived from earlier user-contributed patches and I'm generally lacking reviewers.