From c57c0aeb36df2c9a3116ef96036d2600e91aaddc 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 | 38 ++++++++++++------- .../GroupRelationshipQueryAlter.php | 18 +++++---- src/QueryAccess/QueryAlterBase.php | 2 +- .../QueryAlter/EntityQueryAlterTestBase.php | 17 ++++++--- .../GroupRelationshipQueryAlterTest.php | 14 ++++--- 6 files changed, 62 insertions(+), 32 deletions(-) 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..59a52cb 100644 --- a/src/QueryAccess/EntityQueryAlter.php +++ b/src/QueryAccess/EntityQueryAlter.php @@ -141,28 +141,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(); } } } @@ -237,22 +237,34 @@ class EntityQueryAlter extends QueryAlterBase { 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'); + // Only check for membership once rather than in the loop below. + $scope_conditions->condition($membership_condition = $this->query->andConditionGroup()); if ($scope === PermissionScopeInterface::OUTSIDER_ID) { - $sub_condition->isNull("$membership_alias.entity_id"); + $membership_condition->isNull("$membership_alias.entity_id"); } else { - $sub_condition->isNotNull("$membership_alias.entity_id"); + $membership_condition->isNotNull("$membership_alias.entity_id"); + } + $membership_condition->condition($plugin_conditions = $this->query->orConditionGroup()); + + foreach ($allowed_ids as $plugin_id => $identifiers) { + $sub_condition = $this->query->andConditionGroup(); + $sub_condition->condition("$this->joinAliasPlugins.group_type", array_unique($identifiers), 'IN'); + $sub_condition->condition("$this->joinAliasPlugins.plugin_id", $plugin_id); + $plugin_conditions->condition($sub_condition); } - $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'); + foreach ($allowed_ids as $plugin_id => $identifiers) { + $sub_condition = $this->query->andConditionGroup(); + $sub_condition->condition("$this->joinAliasPlugins.gid", array_unique($identifiers), 'IN'); + $sub_condition->condition("$this->joinAliasPlugins.plugin_id", $plugin_id); + $scope_conditions->condition($sub_condition); + } } /** diff --git a/src/QueryAccess/GroupRelationshipQueryAlter.php b/src/QueryAccess/GroupRelationshipQueryAlter.php index 6625691..c39a6b8 100644 --- a/src/QueryAccess/GroupRelationshipQueryAlter.php +++ b/src/QueryAccess/GroupRelationshipQueryAlter.php @@ -116,17 +116,21 @@ class GroupRelationshipQueryAlter extends QueryAlterBase { $data_table = $this->ensureDataTable(); $membership_alias = $this->ensureMembershipJoin(); + // Only check for membership once rather than in the loop below. + $scope_conditions->condition($membership_condition = $this->query->andConditionGroup()); + if ($scope === PermissionScopeInterface::OUTSIDER_ID) { + $membership_condition->isNull("$membership_alias.entity_id"); + } + else { + $membership_condition->isNotNull("$membership_alias.entity_id"); + } + $membership_condition->condition($plugin_conditions = $this->query->orConditionGroup()); + 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); + $plugin_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..0c3a190 100644 --- a/tests/src/Kernel/QueryAlter/EntityQueryAlterTestBase.php +++ b/tests/src/Kernel/QueryAlter/EntityQueryAlterTestBase.php @@ -104,21 +104,28 @@ 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'); + $conditions->condition($membership_condition = $this->query->andConditionGroup()); if ($outsider) { - $type_conditions->isNull('gcfd_2.entity_id'); + $membership_condition->isNull('gcfd_2.entity_id'); } else { - $type_conditions->isNotNull('gcfd_2.entity_id'); + $membership_condition->isNotNull('gcfd_2.entity_id'); } + $membership_condition->condition($plugin_conditions = $this->query->orConditionGroup()); + + $plugin_conditions->condition($type_conditions = $membership_condition->andConditionGroup()); + $type_conditions->condition('gcfd.group_type', $allowed_ids, 'IN'); + $type_conditions->condition('gcfd.plugin_id', $this->pluginId); } /** * {@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..aff92fa 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); + $conditions->condition($membership_condition = $this->query->andConditionGroup()); if ($outsider) { - $sub_condition->isNull('gcfd.entity_id'); + $membership_condition->isNull('gcfd.entity_id'); } else { - $sub_condition->isNotNull('gcfd.entity_id'); + $membership_condition->isNotNull('gcfd.entity_id'); } - $conditions->condition($sub_condition); + $membership_condition->condition($plugin_conditions = $this->query->orConditionGroup()); + + $plugin_conditions->condition($type_conditions = $membership_condition->andConditionGroup()); + $type_conditions->condition('group_relationship_field_data.group_type', $allowed_ids, 'IN'); + $type_conditions->condition('group_relationship_field_data.plugin_id', $this->pluginId); } /** -- 2.17.1