From 754747c1a3512c280c6464d8059d34ca141a78d0 Mon Sep 17 00:00:00 2001 From: Kristiaan Van den Eynde Date: Tue, 21 Feb 2023 15:18:18 +0100 Subject: [PATCH] Issue #3343405 by kristiaanvandeneynde: Some query access incorrectly checks group type rather than group relationship type --- src/PermissionScopeInterface.php | 5 ++ src/QueryAccess/EntityQueryAlter.php | 39 +++-------- .../GroupRelationshipQueryAlter.php | 36 +--------- src/QueryAccess/PluginBasedQueryAlterBase.php | 68 +++++++++++++++++++ src/QueryAccess/QueryAlterBase.php | 2 +- .../QueryAlter/EntityQueryAlterTestBase.php | 17 +++-- .../GroupRelationshipQueryAlterTest.php | 10 +-- 7 files changed, 105 insertions(+), 72 deletions(-) create mode 100644 src/QueryAccess/PluginBasedQueryAlterBase.php diff --git a/src/PermissionScopeInterface.php b/src/PermissionScopeInterface.php index 8ad6425..b8e9b2e 100644 --- a/src/PermissionScopeInterface.php +++ b/src/PermissionScopeInterface.php @@ -22,4 +22,9 @@ interface PermissionScopeInterface { */ const INDIVIDUAL_ID = 'individual'; + /** + * Collection of scope IDs that are synchronized to a global role. + */ + const SYNCHRONIZED_IDS = [self::OUTSIDER_ID, self::INSIDER_ID]; + } diff --git a/src/QueryAccess/EntityQueryAlter.php b/src/QueryAccess/EntityQueryAlter.php index 3ffe65a..9acc4cc 100644 --- a/src/QueryAccess/EntityQueryAlter.php +++ b/src/QueryAccess/EntityQueryAlter.php @@ -2,10 +2,8 @@ namespace Drupal\group\QueryAccess; -use Drupal\Core\Database\Query\ConditionInterface; use Drupal\Core\Entity\EntityPublishedInterface; use Drupal\Core\Entity\Sql\SqlContentEntityStorage; -use Drupal\group\PermissionScopeInterface; use Symfony\Component\DependencyInjection\ContainerInterface; /** @@ -15,7 +13,7 @@ use Symfony\Component\DependencyInjection\ContainerInterface; * * @internal */ -class EntityQueryAlter extends QueryAlterBase { +class EntityQueryAlter extends PluginBasedQueryAlterBase { /** * The group relation type manager. @@ -141,28 +139,28 @@ class EntityQueryAlter extends QueryAlterBase { foreach ($calculated_permissions->getItems() as $item) { if ($admin_permission !== FALSE && $item->hasPermission($admin_permission)) { - $allowed_any_ids[$item->getScope()][] = $item->getIdentifier(); + $allowed_any_ids[$item->getScope()][$plugin_id][] = $item->getIdentifier(); } elseif(!$check_published) { if ($any_permission !== FALSE && $item->hasPermission($any_permission)) { - $allowed_any_ids[$item->getScope()][] = $item->getIdentifier(); + $allowed_any_ids[$item->getScope()][$plugin_id][] = $item->getIdentifier(); } elseif($own_permission !== FALSE && $item->hasPermission($own_permission)) { - $allowed_own_ids[$item->getScope()][] = $item->getIdentifier(); + $allowed_own_ids[$item->getScope()][$plugin_id][] = $item->getIdentifier(); } } else { if ($any_permission !== FALSE && $item->hasPermission($any_permission)) { - $allowed_any_by_status_ids[1][$item->getScope()][] = $item->getIdentifier(); + $allowed_any_by_status_ids[1][$item->getScope()][$plugin_id][] = $item->getIdentifier(); } elseif($own_permission !== FALSE && $item->hasPermission($own_permission)) { - $allowed_own_by_status_ids[1][$item->getScope()][] = $item->getIdentifier(); + $allowed_own_by_status_ids[1][$item->getScope()][$plugin_id][] = $item->getIdentifier(); } if ($any_unpublished_permission !== FALSE && $item->hasPermission($any_unpublished_permission)) { - $allowed_any_by_status_ids[0][$item->getScope()][] = $item->getIdentifier(); + $allowed_any_by_status_ids[0][$item->getScope()][$plugin_id][] = $item->getIdentifier(); } elseif($own_unpublished_permission !== FALSE && $item->hasPermission($own_unpublished_permission)) { - $allowed_own_by_status_ids[0][$item->getScope()][] = $item->getIdentifier(); + $allowed_own_by_status_ids[0][$item->getScope()][$plugin_id][] = $item->getIdentifier(); } } } @@ -234,25 +232,8 @@ class EntityQueryAlter extends QueryAlterBase { /** * {@inheritdoc} */ - protected function addSynchronizedConditions(array $allowed_ids, ConditionInterface $scope_conditions, $scope) { - $membership_alias = $this->ensureMembershipJoin(); - - $sub_condition = $this->query->andConditionGroup(); - $sub_condition->condition("$this->joinAliasPlugins.group_type", array_unique($allowed_ids), 'IN'); - if ($scope === PermissionScopeInterface::OUTSIDER_ID) { - $sub_condition->isNull("$membership_alias.entity_id"); - } - else { - $sub_condition->isNotNull("$membership_alias.entity_id"); - } - $scope_conditions->condition($sub_condition); - } - - /** - * {@inheritdoc} - */ - protected function addIndividualConditions(array $allowed_ids, ConditionInterface $scope_conditions) { - $scope_conditions->condition("$this->joinAliasPlugins.gid", array_unique($allowed_ids) , 'IN'); + protected function getPluginDataTable() { + return $this->joinAliasPlugins; } /** diff --git a/src/QueryAccess/GroupRelationshipQueryAlter.php b/src/QueryAccess/GroupRelationshipQueryAlter.php index 6625691..4c53a59 100644 --- a/src/QueryAccess/GroupRelationshipQueryAlter.php +++ b/src/QueryAccess/GroupRelationshipQueryAlter.php @@ -2,8 +2,6 @@ namespace Drupal\group\QueryAccess; -use Drupal\Core\Database\Query\ConditionInterface; -use Drupal\group\PermissionScopeInterface; use Symfony\Component\DependencyInjection\ContainerInterface; /** @@ -13,7 +11,7 @@ use Symfony\Component\DependencyInjection\ContainerInterface; * * @internal */ -class GroupRelationshipQueryAlter extends QueryAlterBase { +class GroupRelationshipQueryAlter extends PluginBasedQueryAlterBase { /** * The group relation type manager. @@ -112,36 +110,8 @@ class GroupRelationshipQueryAlter extends QueryAlterBase { /** * {@inheritdoc} */ - protected function addSynchronizedConditions(array $allowed_ids, ConditionInterface $scope_conditions, $scope) { - $data_table = $this->ensureDataTable(); - $membership_alias = $this->ensureMembershipJoin(); - - foreach ($allowed_ids as $plugin_id => $identifiers) { - $sub_condition = $this->query->andConditionGroup(); - $sub_condition->condition("$data_table.group_type", array_unique($identifiers), 'IN'); - $sub_condition->condition("$data_table.plugin_id", $plugin_id); - if ($scope === PermissionScopeInterface::OUTSIDER_ID) { - $sub_condition->isNull("$membership_alias.entity_id"); - } - else { - $sub_condition->isNotNull("$membership_alias.entity_id"); - } - $scope_conditions->condition($sub_condition); - } - } - - /** - * {@inheritdoc} - */ - protected function addIndividualConditions(array $allowed_ids, ConditionInterface $scope_conditions) { - $data_table = $this->ensureDataTable(); - - foreach ($allowed_ids as $plugin_id => $identifiers) { - $sub_condition = $this->query->andConditionGroup(); - $sub_condition->condition("$data_table.gid", array_unique($identifiers) , 'IN'); - $sub_condition->condition("$data_table.plugin_id", $plugin_id); - $scope_conditions->condition($sub_condition); - } + protected function getPluginDataTable() { + return $this->ensureDataTable(); } /** diff --git a/src/QueryAccess/PluginBasedQueryAlterBase.php b/src/QueryAccess/PluginBasedQueryAlterBase.php new file mode 100644 index 0000000..13834dc --- /dev/null +++ b/src/QueryAccess/PluginBasedQueryAlterBase.php @@ -0,0 +1,68 @@ +getPluginDataTable(); + $membership_alias = $this->ensureMembershipJoin(); + + // A list of plugin IDs and group types can be optimized into a list of + // group relationship type IDs. This to avoid having to add an IN query per + // plugin ID as seen in ::addIndividualConditions(). + $storage = $this->entityTypeManager->getStorage('group_relationship_type'); + assert($storage instanceof GroupRelationshipTypeStorageInterface); + + $group_relationship_type_ids = []; + foreach ($allowed_ids as $plugin_id => $group_type_ids) { + foreach (array_unique($group_type_ids) as $group_type_id) { + $group_relationship_type_ids[] = $storage->getRelationshipTypeId($group_type_id, $plugin_id); + } + } + + $scope_conditions->condition($sub_condition = $this->query->andConditionGroup()); + $sub_condition->condition("$data_table.type", $group_relationship_type_ids, 'IN'); + if ($scope === PermissionScopeInterface::OUTSIDER_ID) { + $sub_condition->isNull("$membership_alias.entity_id"); + } + else { + $sub_condition->isNotNull("$membership_alias.entity_id"); + } + } + + /** + * {@inheritdoc} + */ + protected function addIndividualConditions(array $allowed_ids, ConditionInterface $scope_conditions) { + $data_table = $this->getPluginDataTable(); + + foreach ($allowed_ids as $plugin_id => $identifiers) { + $sub_condition = $this->query->andConditionGroup(); + $sub_condition->condition("$data_table.gid", array_unique($identifiers), 'IN'); + $sub_condition->condition("$data_table.plugin_id", $plugin_id); + $scope_conditions->condition($sub_condition); + } + } + +} diff --git a/src/QueryAccess/QueryAlterBase.php b/src/QueryAccess/QueryAlterBase.php index db6087b..8fe15e5 100644 --- a/src/QueryAccess/QueryAlterBase.php +++ b/src/QueryAccess/QueryAlterBase.php @@ -172,7 +172,7 @@ abstract class QueryAlterBase implements ContainerInjectionInterface { $scope_conditions = $this->ensureOrConjunction($parent_condition); // Add the group types where synchronized access is granted. - foreach ([PermissionScopeInterface::OUTSIDER_ID, PermissionScopeInterface::INSIDER_ID] as $scope) { + foreach (PermissionScopeInterface::SYNCHRONIZED_IDS as $scope) { if (!empty($allowed_ids[$scope])) { $this->addSynchronizedConditions($allowed_ids[$scope], $scope_conditions, $scope); } diff --git a/tests/src/Kernel/QueryAlter/EntityQueryAlterTestBase.php b/tests/src/Kernel/QueryAlter/EntityQueryAlterTestBase.php index 7fa5da5..8112841 100644 --- a/tests/src/Kernel/QueryAlter/EntityQueryAlterTestBase.php +++ b/tests/src/Kernel/QueryAlter/EntityQueryAlterTestBase.php @@ -104,13 +104,17 @@ abstract class EntityQueryAlterTestBase extends QueryAlterTestBase { * {@inheritdoc} */ protected function addSynchronizedConditions(array $allowed_ids, ConditionInterface $conditions, $outsider) { - $conditions->condition($type_conditions = $conditions->andConditionGroup()); - $type_conditions->condition('gcfd.group_type', $allowed_ids, 'IN'); + $storage = $this->entityTypeManager->getStorage('group_relationship_type'); + assert($storage instanceof GroupRelationshipTypeStorageInterface); + $group_relationship_type_id = $storage->getRelationshipTypeId(reset($allowed_ids), $this->pluginId); + + $conditions->condition($sub_condition = $conditions->andConditionGroup()); + $sub_condition->condition('gcfd.type', [$group_relationship_type_id], 'IN'); if ($outsider) { - $type_conditions->isNull('gcfd_2.entity_id'); + $sub_condition->isNull('gcfd_2.entity_id'); } else { - $type_conditions->isNotNull('gcfd_2.entity_id'); + $sub_condition->isNotNull('gcfd_2.entity_id'); } } @@ -118,7 +122,10 @@ abstract class EntityQueryAlterTestBase extends QueryAlterTestBase { * {@inheritdoc} */ protected function addIndividualConditions(array $allowed_ids, ConditionInterface $conditions) { - $conditions->condition('gcfd.gid', $allowed_ids, 'IN'); + $sub_condition = $conditions->andConditionGroup(); + $sub_condition->condition('gcfd.gid', $allowed_ids, 'IN'); + $sub_condition->condition('gcfd.plugin_id', $this->pluginId); + $conditions->condition($sub_condition); } } diff --git a/tests/src/Kernel/QueryAlter/GroupRelationshipQueryAlterTest.php b/tests/src/Kernel/QueryAlter/GroupRelationshipQueryAlterTest.php index a22b21d..237ff88 100644 --- a/tests/src/Kernel/QueryAlter/GroupRelationshipQueryAlterTest.php +++ b/tests/src/Kernel/QueryAlter/GroupRelationshipQueryAlterTest.php @@ -145,16 +145,18 @@ class GroupRelationshipQueryAlterTest extends QueryAlterTestBase { * {@inheritdoc} */ protected function addSynchronizedConditions(array $allowed_ids, ConditionInterface $conditions, $outsider) { - $sub_condition = $conditions->andConditionGroup(); - $sub_condition->condition('group_relationship_field_data.group_type', $allowed_ids, 'IN'); - $sub_condition->condition('group_relationship_field_data.plugin_id', $this->pluginId); + $storage = $this->entityTypeManager->getStorage('group_relationship_type'); + assert($storage instanceof GroupRelationshipTypeStorageInterface); + $group_relationship_type_id = $storage->getRelationshipTypeId(reset($allowed_ids), $this->pluginId); + + $conditions->condition($sub_condition = $conditions->andConditionGroup()); + $sub_condition->condition('group_relationship_field_data.type', [$group_relationship_type_id], 'IN'); if ($outsider) { $sub_condition->isNull('gcfd.entity_id'); } else { $sub_condition->isNotNull('gcfd.entity_id'); } - $conditions->condition($sub_condition); } /** -- 2.17.1