From 020ae7c7b49d417249f6791682d2556a39bc0c50 Mon Sep 17 00:00:00 2001 From: Kristiaan Van den Eynde Date: Tue, 21 Feb 2023 14:26:19 +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 | 28 +++++++++---- src/QueryAccess/QueryAlterBase.php | 2 +- .../QueryAlter/EntityQueryAlterTestBase.php | 11 ++++- .../Kernel/QueryAlter/QueryAlterTestBase.php | 40 +++++++++++++------ 5 files changed, 64 insertions(+), 22 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..b71e03f 100644 --- a/src/QueryAccess/EntityQueryAlter.php +++ b/src/QueryAccess/EntityQueryAlter.php @@ -5,6 +5,7 @@ namespace Drupal\group\QueryAccess; use Drupal\Core\Database\Query\ConditionInterface; use Drupal\Core\Entity\EntityPublishedInterface; use Drupal\Core\Entity\Sql\SqlContentEntityStorage; +use Drupal\group\Entity\Storage\GroupRelationshipTypeStorageInterface; use Drupal\group\PermissionScopeInterface; use Symfony\Component\DependencyInjection\ContainerInterface; @@ -128,6 +129,10 @@ class EntityQueryAlter extends QueryAlterBase { $owner_key = $this->entityType->getKey('owner'); $published_key = $this->entityType->getKey('published'); + // We might need to construct group relationship type IDs further down. + $group_relationship_type_storage = $this->entityTypeManager->getStorage('group_relationship_type'); + assert($group_relationship_type_storage instanceof GroupRelationshipTypeStorageInterface); + $allowed_any_ids = $allowed_own_ids = $allowed_any_by_status_ids = $allowed_own_by_status_ids = []; foreach ($plugin_ids_in_use as $plugin_id) { $handler = $this->pluginManager->getPermissionProvider($plugin_id); @@ -140,29 +145,36 @@ class EntityQueryAlter extends QueryAlterBase { } foreach ($calculated_permissions->getItems() as $item) { + // For individual scope access, we need to get the group ID, but for + // synchronized scope access, we need to use the group relationship type + // ID to ensure the unique group type vs plugin combo. + $identifier = in_array($item->getScope(), PermissionScopeInterface::SYNCHRONIZED_IDS, TRUE) + ? $group_relationship_type_storage->getRelationshipTypeId($item->getIdentifier(), $plugin_id) + : $item->getIdentifier(); + if ($admin_permission !== FALSE && $item->hasPermission($admin_permission)) { - $allowed_any_ids[$item->getScope()][] = $item->getIdentifier(); + $allowed_any_ids[$item->getScope()][] = $identifier; } elseif(!$check_published) { if ($any_permission !== FALSE && $item->hasPermission($any_permission)) { - $allowed_any_ids[$item->getScope()][] = $item->getIdentifier(); + $allowed_any_ids[$item->getScope()][] = $identifier; } elseif($own_permission !== FALSE && $item->hasPermission($own_permission)) { - $allowed_own_ids[$item->getScope()][] = $item->getIdentifier(); + $allowed_own_ids[$item->getScope()][] = $identifier; } } 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()][] = $identifier; } 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()][] = $identifier; } 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()][] = $identifier; } 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()][] = $identifier; } } } @@ -238,7 +250,7 @@ class EntityQueryAlter extends QueryAlterBase { $membership_alias = $this->ensureMembershipJoin(); $sub_condition = $this->query->andConditionGroup(); - $sub_condition->condition("$this->joinAliasPlugins.group_type", array_unique($allowed_ids), 'IN'); + $sub_condition->condition("$this->joinAliasPlugins.type", array_unique($allowed_ids), 'IN'); if ($scope === PermissionScopeInterface::OUTSIDER_ID) { $sub_condition->isNull("$membership_alias.entity_id"); } 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..7526761 100644 --- a/tests/src/Kernel/QueryAlter/EntityQueryAlterTestBase.php +++ b/tests/src/Kernel/QueryAlter/EntityQueryAlterTestBase.php @@ -38,6 +38,15 @@ abstract class EntityQueryAlterTestBase extends QueryAlterTestBase { return "administer $this->pluginId"; } + /** + * {@inheritdoc} + */ + protected function getSynchronizedIdentifier($group_type_id) { + $storage = $this->entityTypeManager->getStorage('group_relationship_type'); + assert($storage instanceof GroupRelationshipTypeStorageInterface); + return $storage->getRelationshipTypeId($group_type_id, $this->pluginId); + } + /** * {@inheritdoc} */ @@ -105,7 +114,7 @@ abstract class EntityQueryAlterTestBase extends QueryAlterTestBase { */ protected function addSynchronizedConditions(array $allowed_ids, ConditionInterface $conditions, $outsider) { $conditions->condition($type_conditions = $conditions->andConditionGroup()); - $type_conditions->condition('gcfd.group_type', $allowed_ids, 'IN'); + $type_conditions->condition('gcfd.type', $allowed_ids, 'IN'); if ($outsider) { $type_conditions->isNull('gcfd_2.entity_id'); } diff --git a/tests/src/Kernel/QueryAlter/QueryAlterTestBase.php b/tests/src/Kernel/QueryAlter/QueryAlterTestBase.php index 401bd4b..6449f9d 100644 --- a/tests/src/Kernel/QueryAlter/QueryAlterTestBase.php +++ b/tests/src/Kernel/QueryAlter/QueryAlterTestBase.php @@ -244,6 +244,9 @@ abstract class QueryAlterTestBase extends GroupKernelTestBase { $this->addNoAccessConditions($control); } else { + $synchronized_ids = [$this->getSynchronizedIdentifier($group_type->id())]; + $individual_ids = [$group->id()]; + if ($definition->getDataTable() && $joins_data_table) { $this->joinTargetEntityDataTable($control); } @@ -256,15 +259,15 @@ abstract class QueryAlterTestBase extends GroupKernelTestBase { if ($outsider_simple_check) { $scope_conditions = $this->ensureOrConjunction($scope_conditions); - $this->addSynchronizedConditions([$group_type->id()], $scope_conditions, TRUE); + $this->addSynchronizedConditions($synchronized_ids, $scope_conditions, TRUE); } if ($insider_simple_check) { $scope_conditions = $this->ensureOrConjunction($scope_conditions); - $this->addSynchronizedConditions([$group_type->id()], $scope_conditions, FALSE); + $this->addSynchronizedConditions($synchronized_ids, $scope_conditions, FALSE); } if ($individual_simple_check) { $scope_conditions = $this->ensureOrConjunction($scope_conditions); - $this->addIndividualConditions([$group->id()], $scope_conditions); + $this->addIndividualConditions($individual_ids, $scope_conditions); } if ($this->isOwnable && !$operation_supports_status) { @@ -276,13 +279,13 @@ abstract class QueryAlterTestBase extends GroupKernelTestBase { } if ($outsider_owner_check) { - $this->addSynchronizedConditions([$group_type->id()], $owner_conditions, TRUE); + $this->addSynchronizedConditions($synchronized_ids, $owner_conditions, TRUE); } if ($insider_owner_check) { - $this->addSynchronizedConditions([$group_type->id()], $owner_conditions, FALSE); + $this->addSynchronizedConditions($synchronized_ids, $owner_conditions, FALSE); } if ($individual_owner_check) { - $this->addIndividualConditions([$group->id()], $owner_conditions); + $this->addIndividualConditions($individual_ids, $owner_conditions); } } elseif ($operation_supports_status) { @@ -310,13 +313,13 @@ abstract class QueryAlterTestBase extends GroupKernelTestBase { $status_group->condition($status_conditions = $control->orConditionGroup()); if (${'outsider_' . $variable_key . '_simple_check'}) { - $this->addSynchronizedConditions([$group_type->id()], $status_conditions, TRUE); + $this->addSynchronizedConditions($synchronized_ids, $status_conditions, TRUE); } if (${'insider_' . $variable_key . '_simple_check'}) { - $this->addSynchronizedConditions([$group_type->id()], $status_conditions, FALSE); + $this->addSynchronizedConditions($synchronized_ids, $status_conditions, FALSE); } if (${'individual_' . $variable_key . '_simple_check'}) { - $this->addIndividualConditions([$group->id()], $status_conditions); + $this->addIndividualConditions($individual_ids, $status_conditions); } if (${'individual_' . $variable_key . '_owner_check'} @@ -329,13 +332,13 @@ abstract class QueryAlterTestBase extends GroupKernelTestBase { $owner_group->condition($owner_conditions = $control->orConditionGroup()); if (${'outsider_' . $variable_key . '_owner_check'}) { - $this->addSynchronizedConditions([$group_type->id()], $owner_conditions, TRUE); + $this->addSynchronizedConditions($synchronized_ids, $owner_conditions, TRUE); } if (${'insider_' . $variable_key . '_owner_check'}) { - $this->addSynchronizedConditions([$group_type->id()], $owner_conditions, FALSE); + $this->addSynchronizedConditions($synchronized_ids, $owner_conditions, FALSE); } if (${'individual_' . $variable_key . '_owner_check'}) { - $this->addIndividualConditions([$group->id()], $owner_conditions); + $this->addIndividualConditions($individual_ids, $owner_conditions); } } } @@ -883,6 +886,19 @@ abstract class QueryAlterTestBase extends GroupKernelTestBase { */ abstract protected function getAdminPermission(); + /** + * Returns an identifier to use in the query for synchronized scopes. + * + * @param $group_type_id + * The group type ID to get the identifier for. + * + * @return string + * The query identifier for the group type ID. + */ + protected function getSynchronizedIdentifier($group_type_id) { + return $group_type_id; + } + /** * Builds and returns a query that will be altered. * -- 2.17.1