From 62098da42cb34f5b72e70778ed69a95ccf192eb0 Mon Sep 17 00:00:00 2001 From: Kristiaan Van den Eynde Date: Fri, 27 Oct 2017 15:02:08 +0200 Subject: [PATCH] Issue #2888588 by kevin.dutra, kristiaanvandeneynde, seanB: Performance issues with node access control --- modules/gnode/gnode.module | 154 +++++++++++++++++---- .../src/Kernel/GroupNodeAccessRecordsTest.php | 24 +++- .../tests/src/Kernel/GroupNodeAccessTestBase.php | 8 ++ tests/modules/group_test/group_test.info.yml | 9 ++ tests/modules/group_test/group_test.module | 17 +++ .../group_test_config/group_test_config.info.yml | 3 +- .../group_test_plugin/group_test_plugin.info.yml | 2 +- .../group_test_views/group_test_views.info.yml | 2 +- tests/src/Kernel/GroupContentTest.php | 25 ++++ 9 files changed, 210 insertions(+), 34 deletions(-) create mode 100644 tests/modules/group_test/group_test.info.yml create mode 100644 tests/modules/group_test/group_test.module diff --git a/modules/gnode/gnode.module b/modules/gnode/gnode.module index 0efb6b1..2ee41ac 100644 --- a/modules/gnode/gnode.module +++ b/modules/gnode/gnode.module @@ -8,6 +8,7 @@ use Drupal\Core\Access\AccessResult; use Drupal\Core\Session\AccountInterface; use Drupal\group\Entity\GroupContentType; +use Drupal\group\Entity\GroupRoleInterface; use Drupal\node\NodeInterface; use Drupal\node\NodeTypeInterface; @@ -22,6 +23,37 @@ function gnode_node_type_insert(NodeTypeInterface $node_type) { } /** + * Implements hook_ENTITY_TYPE_insert(). + */ +function gnode_group_role_insert(GroupRoleInterface $group_role) { + // Because we optimize for anonymous users, it also means we need to rebuild + // the node grants table when an anonymous role is added. + // See: gnode_node_access_records() + if ($group_role->isAnonymous()) { + node_access_needs_rebuild(TRUE); + } +} + +/** + * Implements hook_ENTITY_TYPE_update(). + */ +function gnode_group_role_update(GroupRoleInterface $group_role) { + // Because we optimize for anonymous users, it also means we need to rebuild + // the node grants table when an anonymous role has its permissions changed. + // See: gnode_node_access_records() + if ($group_role->isAnonymous()) { + $new = array_unique($group_role->getPermissions()); + $old = array_unique($group_role->original->getPermissions()); + sort($new); + sort($old); + + if ($new != $old) { + node_access_needs_rebuild(TRUE); + } + } +} + +/** * Implements hook_node_access(). * * When trying to view, update or delete a node it suffices to have the right to @@ -114,6 +146,7 @@ function gnode_node_access(NodeInterface $node, $op, AccountInterface $account) * - 'gnode:NODE_TYPE': Grants view and update or delete any access to nodes. * - 'gnode_unpublished:NODE_TYPE': Grants view access to unpublished nodes. * - 'gnode_author:UID:NODE_TYPE': Grants update or delete access to authors. + * - 'gnode_anonymous': Given to anonymous users. * - 'gnode_bypass': Given to anyone with the 'bypass group access' permission. * * @see gnode_node_access_records() @@ -124,6 +157,12 @@ function gnode_node_grants(AccountInterface $account, $op) { return ['gnode_bypass' => [GNODE_MASTER_GRANT_ID]]; } + // Anonymous users get the anonymous grant. See the implementation in the + // gnode_node_access_records() function as to why that is. + if ($account->isAnonymous()) { + return ['gnode_anonymous' => [GNODE_MASTER_GRANT_ID]]; + } + // Gather the machine names of all node types. $entity_type_manager = \Drupal::entityTypeManager(); $node_type_ids = $entity_type_manager @@ -131,8 +170,8 @@ function gnode_node_grants(AccountInterface $account, $op) { ->getQuery() ->execute(); - // Initialize a grant array for members and one for anonymous/outsider users. - $grants_m = $grants_ao = []; + // Initialize a grant array for members and one for outsider users. + $grants_m = $grants_o = []; // If the user could not bypass group access, we need to check their access // for every single group. Because loading every group would incur a massive @@ -174,8 +213,8 @@ function gnode_node_grants(AccountInterface $account, $op) { } // All other groups have the benefit of sharing the same permission set among - // all anonymous or authenticated users per group type. We can therefore know - // the user's permissions for all groups of the same type they aren't part of. + // all authenticated users per group type. We can therefore know the user's + // permissions for all groups of the same group type they aren't part of. /** @var \Drupal\group\Entity\GroupTypeInterface[] $group_types */ $group_types = $entity_type_manager->getStorage('group_type')->loadMultiple(); foreach ($group_types as $group_type) { @@ -192,11 +231,8 @@ function gnode_node_grants(AccountInterface $account, $op) { continue; } - // Grab the anonymous or outsider role for the group type depending on the - // user's account status (anonymous or authenticated). - $group_role = $account->isAnonymous() - ? $group_type->getAnonymousRole() - : $group_type->getOutsiderRole(); + // Grab the outsider role for the group type. + $group_role = $group_type->getOutsiderRole(); foreach ($node_type_ids as $node_type_id) { $plugin_id = "group_node:$node_type_id"; @@ -210,10 +246,10 @@ function gnode_node_grants(AccountInterface $account, $op) { switch ($op) { case 'view': if ($group_role->hasPermission("view $plugin_id entity")) { - $grants_ao["gnode:$node_type_id"][] = $gids; + $grants_o["gnode:$node_type_id"][] = $gids; } if ($group_role->hasPermission("view unpublished $plugin_id entity")) { - $grants_ao["gnode_unpublished:$node_type_id"][] = $gids; + $grants_o["gnode_unpublished:$node_type_id"][] = $gids; } break; @@ -221,26 +257,26 @@ function gnode_node_grants(AccountInterface $account, $op) { case 'delete': // If you can act on any node, there's no need for the author grant. if ($group_role->hasPermission("$op any $plugin_id entity")) { - $grants_ao["gnode:$node_type_id"][] = $gids; + $grants_o["gnode:$node_type_id"][] = $gids; } elseif ($group_role->hasPermission("$op own $plugin_id entity")) { $uid = $account->id(); - $grants_ao["gnode_author:$uid:$node_type_id"][] = $gids; + $grants_o["gnode_author:$uid:$node_type_id"][] = $gids; } break; } } } - // The code above populated the anonymous/outsider grants by adding the group - // IDs per group type. We need to combine this into one big list of group IDs - // per entry in the $grants_ao array. - foreach ($grants_ao as $key => $gids_per_group_type) { - $grants_ao[$key] = array_reduce($gids_per_group_type, 'array_merge', []); + // The code above populated the outsider grants by adding the group IDs per + // group type. We need to combine this into one big list of group IDs per + // entry in the $grants_o array. + foreach ($grants_o as $key => $gids_per_group_type) { + $grants_o[$key] = array_reduce($gids_per_group_type, 'array_merge', []); } - // Recursively merge the member grants with the anonymous/outsider grants. - return array_merge_recursive($grants_m, $grants_ao); + // Recursively merge the member grants with the outsider grants. + return array_merge_recursive($grants_m, $grants_o); } /** @@ -249,11 +285,12 @@ function gnode_node_grants(AccountInterface $account, $op) { * @see gnode_node_grants() */ function gnode_node_access_records(NodeInterface $node) { - $records = []; - $type = $node->bundle(); + $records = $group_types = []; + $node_type_id = $node->bundle(); + $plugin_id = "group_node:$node_type_id"; // Only act if there are group content types for this node type. - $group_content_types = GroupContentType::loadByContentPluginId("group_node:$type"); + $group_content_types = GroupContentType::loadByContentPluginId($plugin_id); if (empty($group_content_types)) { return $records; } @@ -281,21 +318,82 @@ function gnode_node_access_records(NodeInterface $node) { ]; // Set records for every group the node belongs to. + $uid = $node->getOwnerId(); + $prefix = $node->isPublished() ? 'gnode' : 'gnode_unpublished'; foreach ($group_contents as $group_content) { /** @var \Drupal\group\Entity\GroupContentInterface $group_content */ - $gid = $group_content->getGroup()->id(); + $group = $group_content->getGroup(); + $group_type_id = $group_content->bundle(); + + // Gather all group types the node belongs to. + if (!isset($group_types[$group_type_id])) { + $group_types[$group_type_id] = $group->getGroupType(); + } + + // We use the group ID as the realm ID. + $gid = $group->id(); // Add the non-author record for viewing nodes. - $prefix = $node->isPublished() ? 'gnode' : 'gnode_unpublished'; - $records[] = ['gid' => $gid, 'realm' => "$prefix:$type"] + $base; + $records[] = ['gid' => $gid, 'realm' => "$prefix:$node_type_id"] + $base; // Add the author record for updating or deleting. - $uid = $node->getOwnerId(); - $records[] = ['gid' => $gid, 'realm' => "gnode_author:$uid:$type"] + $base; + $records[] = ['gid' => $gid, 'realm' => "gnode_author:$uid:$node_type_id"] + $base; } // Add the general access bypass record. $records[] = ['gid' => GNODE_MASTER_GRANT_ID, 'realm' => 'gnode_bypass'] + $base; + // For anonymous users we actually build the access record based on the groups + // the node belongs to. After all: If you're anonymous to one group, you're + // anonymous to all groups. Meaning that if one of the node's groups allows + // anonymous users to view the node, all anonymous users can view it. We can + // use this to our advantage by assigning a special access record that we can + // provide a grant for in gnode_node_grants(). + $anonymous_record = [ + 'gid' => GNODE_MASTER_GRANT_ID, + 'realm' => 'gnode_anonymous', + 'grant_view' => 0, + 'grant_update' => 0, + 'grant_delete' => 0, + 'priority' => 0, + ]; + + // Get references to the grants for faster and more readable loops below. + $can_view = &$anonymous_record['grant_view']; + $can_update = &$anonymous_record['grant_update']; + $can_delete = &$anonymous_record['grant_delete']; + + $view_permission = $node->isPublished() + ? "view $plugin_id entity" + : "view unpublished $plugin_id entity"; + + foreach ($group_types as $group_type) { + /** @var \Drupal\group\Entity\GroupTypeInterface $group_type */ + $group_role = $group_type->getAnonymousRole(); + + if (!$can_view && $group_role->hasPermission($view_permission)) { + $can_view = 1; + } + if (!$can_update && $group_role->hasPermission("update any $plugin_id entity")) { + $can_update = 1; + } + if (!$can_delete && $group_role->hasPermission("delete any $plugin_id entity")) { + $can_delete = 1; + } + + // If the node is owned by anonymous, we also need to check for the author + // permissions following the pattern "$op own $plugin_id entity". + if ($uid == 0) { + if (!$can_update && $group_role->hasPermission("update own $plugin_id entity")) { + $can_update = 1; + } + if (!$can_delete && $group_role->hasPermission("delete own $plugin_id entity")) { + $can_delete = 1; + } + } + } + + $records[] = $anonymous_record; + return $records; } diff --git a/modules/gnode/tests/src/Kernel/GroupNodeAccessRecordsTest.php b/modules/gnode/tests/src/Kernel/GroupNodeAccessRecordsTest.php index 10607ca..5b03d2a 100644 --- a/modules/gnode/tests/src/Kernel/GroupNodeAccessRecordsTest.php +++ b/modules/gnode/tests/src/Kernel/GroupNodeAccessRecordsTest.php @@ -48,7 +48,7 @@ class GroupNodeAccessRecordsTest extends GroupNodeAccessTestBase { $this->groupA1->addContent($node, 'group_node:a'); $records = gnode_node_access_records($node); - $this->assertCount(3, $records, '3 access records set for a published group node.'); + $this->assertCount(4, $records, '4 access records set for a published group node.'); $base = [ 'grant_view' => 1, @@ -61,6 +61,16 @@ class GroupNodeAccessRecordsTest extends GroupNodeAccessTestBase { $this->assertEquals(['gid' => $gid, 'realm' => 'gnode:a'] + $base, $records[0], 'General gnode:NODE_TYPE grant found.'); $this->assertEquals(['gid' => $gid, 'realm' => "gnode_author:$uid:a"] + $base, $records[1], 'Author gnode_author:UID:NODE_TYPE grant found.'); $this->assertEquals(['gid' => GNODE_MASTER_GRANT_ID, 'realm' => 'gnode_bypass'] + $base, $records[2], 'Admin gnode_bypass grant found.'); + + $anonymous = [ + 'gid' => GNODE_MASTER_GRANT_ID, + 'realm' => 'gnode_anonymous', + 'grant_view' => 1, + 'grant_update' => 0, + 'grant_delete' => 0, + 'priority' => 0 + ]; + $this->assertEquals($anonymous, $records[3], 'Anonymous catch-all grant found.'); } /** @@ -79,7 +89,7 @@ class GroupNodeAccessRecordsTest extends GroupNodeAccessTestBase { $this->groupA1->addContent($node, 'group_node:a'); $records = gnode_node_access_records($node); - $this->assertCount(3, $records, '3 access records set for an unpublished group node.'); + $this->assertCount(4, $records, '4 access records set for an unpublished group node.'); $base = [ 'grant_view' => 1, @@ -92,6 +102,16 @@ class GroupNodeAccessRecordsTest extends GroupNodeAccessTestBase { $this->assertEquals(['gid' => $gid, 'realm' => 'gnode_unpublished:a'] + $base, $records[0], 'General gnode_unpublished:NODE_TYPE grant found.'); $this->assertEquals(['gid' => $gid, 'realm' => "gnode_author:$uid:a"] + $base, $records[1], 'Author gnode_author:UID:NODE_TYPE grant found.'); $this->assertEquals(['gid' => GNODE_MASTER_GRANT_ID, 'realm' => 'gnode_bypass'] + $base, $records[2], 'Admin gnode_bypass grant found.'); + + $anonymous = [ + 'gid' => GNODE_MASTER_GRANT_ID, + 'realm' => 'gnode_anonymous', + 'grant_view' => 0, + 'grant_update' => 0, + 'grant_delete' => 0, + 'priority' => 0 + ]; + $this->assertEquals($anonymous, $records[3], 'Anonymous catch-all grant found.'); } } diff --git a/modules/gnode/tests/src/Kernel/GroupNodeAccessTestBase.php b/modules/gnode/tests/src/Kernel/GroupNodeAccessTestBase.php index 19a36eb..ca89484 100644 --- a/modules/gnode/tests/src/Kernel/GroupNodeAccessTestBase.php +++ b/modules/gnode/tests/src/Kernel/GroupNodeAccessTestBase.php @@ -134,10 +134,18 @@ abstract class GroupNodeAccessTestBase extends EntityKernelTestBase { 'update own group_node:b entity', 'delete own group_node:b entity', ]; + $anonymous_a = [ + 'view group_node:a entity', + ]; + $anonymous_b = [ + 'update any group_node:a entity', + ]; $this->groupTypeA->getMemberRole()->grantPermissions($member_a)->save(); $this->groupTypeB->getMemberRole()->grantPermissions($member_b)->save(); $this->groupTypeA->getOutsiderRole()->grantPermissions($outsider_a)->save(); $this->groupTypeB->getOutsiderRole()->grantPermissions($outsider_b)->save(); + $this->groupTypeA->getAnonymousRole()->grantPermissions($anonymous_a)->save(); + $this->groupTypeB->getAnonymousRole()->grantPermissions($anonymous_b)->save(); // Create some groups. $storage = $this->entityTypeManager->getStorage('group'); diff --git a/tests/modules/group_test/group_test.info.yml b/tests/modules/group_test/group_test.info.yml new file mode 100644 index 0000000..7021488 --- /dev/null +++ b/tests/modules/group_test/group_test.info.yml @@ -0,0 +1,9 @@ +name: 'Group test' +description: 'Support module for Group tests.' +package: 'Testing' +type: 'module' +version: VERSION +core: '8.x' +dependencies: + - 'group' + - 'user' diff --git a/tests/modules/group_test/group_test.module b/tests/modules/group_test/group_test.module new file mode 100644 index 0000000..b1768f5 --- /dev/null +++ b/tests/modules/group_test/group_test.module @@ -0,0 +1,17 @@ +getChangedTime() == 123456789) { + $user->setChangedTime(530496000)->save(); + } +} diff --git a/tests/modules/group_test_config/group_test_config.info.yml b/tests/modules/group_test_config/group_test_config.info.yml index 8dab173..93fe6ad 100644 --- a/tests/modules/group_test_config/group_test_config.info.yml +++ b/tests/modules/group_test_config/group_test_config.info.yml @@ -2,8 +2,7 @@ name: 'Group configuration tests' description: 'Support module for group configuration tests.' package: 'Testing' type: 'module' -version: '1.0' +version: VERSION core: '8.x' - dependencies: - 'group' diff --git a/tests/modules/group_test_plugin/group_test_plugin.info.yml b/tests/modules/group_test_plugin/group_test_plugin.info.yml index ff9ea2f..c63674c 100644 --- a/tests/modules/group_test_plugin/group_test_plugin.info.yml +++ b/tests/modules/group_test_plugin/group_test_plugin.info.yml @@ -2,7 +2,7 @@ name: 'Group test plugin' description: 'Provides group plugins to run tests with.' package: 'Testing' type: 'module' -version: '1.0' +version: VERSION core: '8.x' dependencies: - 'group' diff --git a/tests/modules/group_test_views/group_test_views.info.yml b/tests/modules/group_test_views/group_test_views.info.yml index 176f19d..478774f 100644 --- a/tests/modules/group_test_views/group_test_views.info.yml +++ b/tests/modules/group_test_views/group_test_views.info.yml @@ -2,7 +2,7 @@ name: 'Group test views' description: 'Provides default views for group views tests.' package: 'Testing' type: 'module' -version: '1.0' +version: VERSION core: '8.x' dependencies: - 'group' diff --git a/tests/src/Kernel/GroupContentTest.php b/tests/src/Kernel/GroupContentTest.php index c7863ed..d0f3b41 100644 --- a/tests/src/Kernel/GroupContentTest.php +++ b/tests/src/Kernel/GroupContentTest.php @@ -12,6 +12,11 @@ namespace Drupal\Tests\group\Kernel; class GroupContentTest extends GroupKernelTestBase { /** + * {@inheritdoc} + */ + public static $modules = ['group_test']; + + /** * Ensure entity url templates are functional. * * @covers ::urlRouteParameters @@ -56,4 +61,24 @@ class GroupContentTest extends GroupKernelTestBase { } } + /** + * Tests that after adding an entity to a group, it gets saved again. + * + * @covers ::postSave + * + * @see group_test_user_update() + */ + public function testSubjectResaved() { + $changed = 123456789; + $account = $this->createUser(['changed' => $changed]); + + $group = $this->createGroup(); + $group->addContent($account, 'group_membership'); + + // All users whose changed time was set to 123456789 get their changed time + // set to 530496000 in group_test_user_update() when the account is updated. + $account_unchanged = $this->entityTypeManager->getStorage('user')->loadUnchanged($account->id()); + $this->assertEquals(530496000, $account_unchanged->getChangedTime(), 'Account was saved as part of being added to a group.'); + } + } -- 2.8.1