Issue #3305707 by Marios Anagnostopoulos, kristiaanvandeneynde: Inconsitencies... (f7462ffe) · Commits · project / group

src/QueryAccess/EntityQueryAlter.php

+17 −0

Original line number	Diff line number	Diff line
		@@ -11,6 +11,8 @@ use Symfony\Component\DependencyInjection\ContainerInterface;
		/**
		* Defines a class for altering entity queries.
		*
		* @todo Revisit cacheability and see if we can optimize some more.
		*
		* @internal
		*/
		class EntityQueryAlter extends QueryAlterBase {
		@@ -87,6 +89,21 @@ class EntityQueryAlter extends QueryAlterBase {
		return;
		}

		// Check if any of the plugins actually support the operation. If not, we
		// can simply bail out here to play nice with other modules that do support
		// the provided operation.
		$operation_is_supported = FALSE;
		foreach ($plugin_ids_in_use as $plugin_id) {
		if ($this->pluginManager->getAccessControlHandler($plugin_id)->supportsOperation($operation, 'entity')) {
		$operation_is_supported = TRUE;
		break;
		}
		}

		if (!$operation_is_supported) {
		return;
		}

		// From this point onward, we know that there are grouped entities and that
		// we need to check access, so we can LEFT JOIN the necessary table.
		$id_key = $this->entityType->getKey('id');

src/QueryAccess/GroupQueryAlter.php

+52 −24

Original line number	Diff line number	Diff line
		@@ -23,6 +23,15 @@ class GroupQueryAlter extends QueryAlterBase {
		* {@inheritdoc}
		*/
		protected function doAlter($operation) {
		// Do not restrict access on operations we do not support (yet). Same reason
		// as in GroupAccessControlHandler:checkAccess().
		if (!$permission = $this->getPermission($operation)) {
		// For 'view' we also need to see if unpublished is supported.
		if ($operation !== 'view' \|\| !$this->getPermission('view', 'any', TRUE) && !$this->getPermission('view', 'own', TRUE)) {
		return;
		}
		}

		// If any new group is added, it might change access.
		$this->cacheableMetadata->addCacheTags(['group_list']);

		@@ -30,15 +39,12 @@ class GroupQueryAlter extends QueryAlterBase {
		$this->cacheableMetadata->addCacheContexts(['user.group_permissions']);
		$calculated_permissions = $this->permissionCalculator->calculateFullPermissions($this->currentUser);

		$check_published = $operation === 'view';
		$permission = $this->getPermissionName($operation);

		$allowed_ids = $allowed_any_by_status_ids = $allowed_own_by_status_ids = [];
		foreach ($calculated_permissions->getItems() as $item) {
		if ($item->isAdmin()) {
		$allowed_ids[$item->getScope()][] = $item->getIdentifier();
		}
		elseif (!$check_published) {
		elseif ($operation !== 'view') {
		if ($item->hasPermission($permission)) {
		$allowed_ids[$item->getScope()][] = $item->getIdentifier();
		}
		@@ -47,26 +53,42 @@ class GroupQueryAlter extends QueryAlterBase {
		if ($item->hasPermission($permission)) {
		$allowed_any_by_status_ids[1][$item->getScope()][] = $item->getIdentifier();
		}
		if ($item->hasPermission('view any unpublished group')) {
		if (($view_any_unpub = $this->getPermission('view', 'any', TRUE)) && $item->hasPermission($view_any_unpub)) {
		$allowed_any_by_status_ids[0][$item->getScope()][] = $item->getIdentifier();
		}
		elseif ($item->hasPermission('view own unpublished group')) {
		elseif (($view_own_unpub = $this->getPermission('view', 'own', TRUE)) && $item->hasPermission($view_own_unpub)) {
		$allowed_own_by_status_ids[0][$item->getScope()][] = $item->getIdentifier();
		}
		}
		}

		$has_regular_ids = !empty($allowed_ids);
		$has_status_ids = !empty($allowed_any_by_status_ids) \|\| !empty($allowed_own_by_status_ids);

		// If no group type or group gave access, we deny access altogether.
		if (empty($allowed_ids) && empty($allowed_any_by_status_ids) && empty($allowed_own_by_status_ids)) {
		if (!$has_regular_ids && !$has_status_ids) {
		$this->query->alwaysFalse();
		return;
		}

		if (!empty($allowed_ids)) {
		$this->addScopedConditions($allowed_ids, $this->query);
		// If we only have regular IDs or status IDs, we can simply add those
		// conditions in their dedicated section below. However, if we have both, we
		// need to add both sections to an OR group to avoid two contradicting
		// membership checks to cancel each other out, leading to no results.
		$condition_attacher = $this->query;
		if ($has_regular_ids && $has_status_ids) {
		$condition_attacher = $this->ensureOrConjunction($this->query);

		// We're going to need a data table anyhow, might as well initialize it
		// here so all group type checks are added to the same table.
		$this->ensureDataTable();
		}

		if ($has_regular_ids) {
		$this->addScopedConditions($allowed_ids, $condition_attacher);
		}

		if ($check_published) {
		if ($has_status_ids) {
		foreach ([0, 1] as $status) {
		// Nothing gave access for this status so bail out entirely.
		if (empty($allowed_any_by_status_ids[$status]) && empty($allowed_own_by_status_ids[$status])) {
		@@ -74,7 +96,7 @@ class GroupQueryAlter extends QueryAlterBase {
		}

		$data_table = $this->ensureDataTable();
		$this->query->condition($status_conditions = $this->query->andConditionGroup());
		$condition_attacher->condition($status_conditions = $this->query->andConditionGroup());
		$status_conditions->condition("$data_table.status", $status);
		$status_conditions->condition($status_sub_conditions = $this->query->orConditionGroup());

		@@ -93,31 +115,37 @@ class GroupQueryAlter extends QueryAlterBase {
		}

		/**
		* Retrieves the group permission name for the given operation.
		* Gets the permission name for the given operation and scope.
		*
		* @param string $operation
		* The access operation. Usually one of "view", "update" or "delete".
		* The operation.
		* @param string $scope
		* The operation scope ('any' or 'own'). Defaults to 'any'.
		* @param bool $unpublished
		* Whether to check for the unpublished permission. Defaults to FALSE.
		*
		* @return string
		* The group permission name.
		* The permission name.
		*/
		protected function getPermissionName($operation) {
		protected function getPermission($operation, $scope = 'any', $unpublished = FALSE) {
		// @todo We're using this to define operation support, as do we use the same
		// logic in GroupAccessControlHandler. Ideally, centralize this somewhere.
		switch ($operation) {
		// @todo Could use the below if permission were named 'update group'.
		case 'view':
		if ($unpublished) {
		return "$operation $scope unpublished group";
		}
		return 'view group';

		case 'update':
		$permission = 'edit group';
		break;
		return 'edit group';

		case 'delete':
		case 'view':
		$permission = "$operation group";
		break;
		return 'delete group';

		default:
		$permission = 'view group';
		return FALSE;
		}

		return $permission;
		}

		/**

src/QueryAccess/GroupRelationshipQueryAlter.php

+33 −4

Original line number	Diff line number	Diff line
		@@ -9,6 +9,8 @@ use Symfony\Component\DependencyInjection\ContainerInterface;
		/**
		* Defines a class for altering group queries.
		*
		* @todo Revisit cacheability and see if we can optimize some more.
		*
		* @internal
		*/
		class GroupRelationshipQueryAlter extends QueryAlterBase {
		@@ -33,6 +35,24 @@ class GroupRelationshipQueryAlter extends QueryAlterBase {
		* {@inheritdoc}
		*/
		protected function doAlter($operation) {
		// @todo Move to plugin manager method and remove copy-paste.
		$installed_ids = array_unique(array_merge(...array_values($this->pluginManager->getGroupTypePluginMap())));

		// Check if any of the plugins actually support the operation. If not, we
		// can simply bail out here to play nice with other modules that do support
		// the provided operation.
		$operation_is_supported = FALSE;
		foreach ($installed_ids as $plugin_id) {
		if ($this->pluginManager->getAccessControlHandler($plugin_id)->supportsOperation($operation, 'relationship')) {
		$operation_is_supported = TRUE;
		break;
		}
		}

		if (!$operation_is_supported) {
		return;
		}

		// If any new relationship is added, it might change access.
		$this->cacheableMetadata->addCacheTags(['group_relationship_list']);

		@@ -41,7 +61,7 @@ class GroupRelationshipQueryAlter extends QueryAlterBase {
		$calculated_permissions = $this->permissionCalculator->calculateFullPermissions($this->currentUser);

		$allowed_any_ids = $allowed_own_ids = [];
		foreach (array_keys($this->pluginManager->getDefinitions()) as $plugin_id) {
		foreach ($installed_ids as $plugin_id) {
		$handler = $this->pluginManager->getPermissionProvider($plugin_id);
		$admin_permission = $handler->getAdminPermission();
		$any_permission = $handler->getPermission($operation, 'relationship', 'any');
		@@ -66,15 +86,24 @@ class GroupRelationshipQueryAlter extends QueryAlterBase {
		return;
		}

		// If we only have any IDs or own IDs, we can simply add those conditions
		// in their dedicated section below. However, if we have both, we need to
		// add both sections to an OR group to avoid two contradicting access checks
		// to cancel each other out, leading to no results.
		$condition_attacher = $this->query;
		if (!empty($allowed_any_ids) && !empty($allowed_own_ids)) {
		$condition_attacher = $this->ensureOrConjunction($this->query);
		}

		if (!empty($allowed_any_ids)) {
		$this->addScopedConditions($allowed_any_ids, $this->query);
		$this->addScopedConditions($allowed_any_ids, $condition_attacher);
		}

		if (!empty($allowed_own_ids)) {
		$this->cacheableMetadata->addCacheContexts(['user']);
		$data_table = $this->ensureDataTable();

		$this->query->condition($owner_conditions = $this->query->andConditionGroup());
		$condition_attacher->condition($owner_conditions = $this->query->andConditionGroup());
		$owner_conditions->condition("$data_table.uid", $this->currentUser->id());
		$this->addScopedConditions($allowed_own_ids, $owner_conditions);
		}

src/QueryAccess/QueryAlterBase.php

+21 −10

Original line number	Diff line number	Diff line
		@@ -169,16 +169,7 @@ abstract class QueryAlterBase implements ContainerInjectionInterface {
		* The parent condition to add the subconditions to.
		*/
		protected function addScopedConditions(array $allowed_ids, ConditionInterface $parent_condition) {
		$parent_conditions = $parent_condition->conditions();

		// If the parent conditions are an OR group, add directly to that.
		if ($parent_conditions['#conjunction'] === 'OR') {
		$scope_conditions = $parent_condition;
		}
		// Otherwise initialize an OR group to add our conditions to.
		else {
		$parent_condition->condition($scope_conditions = $this->query->orConditionGroup());
		}
		$scope_conditions = $this->ensureOrConjunction($parent_condition);

		// Add the group types where synchronized access is granted.
		foreach ([PermissionScopeInterface::OUTSIDER_ID, PermissionScopeInterface::INSIDER_ID] as $scope) {
		@@ -193,6 +184,26 @@ abstract class QueryAlterBase implements ContainerInjectionInterface {
		}
		}

		/**
		* Makes sure a ConditionInterface has the OR conjunction.
		*
		* @param \Drupal\Core\Database\Query\ConditionInterface $parent
		* The parent ConditionInterface to potentially add the OR group to.
		*
		* @return \Drupal\Core\Database\Query\ConditionInterface
		* An OR condition group attached to the parent in case the parent did not
		* already use said conjunction or the passed in parent if it did.
		*/
		protected function ensureOrConjunction(ConditionInterface $parent) {
		$conditions_array = $parent->conditions();
		if ($conditions_array['#conjunction'] === 'OR') {
		return $parent;
		}

		$parent->condition($or_group = $this->query->orConditionGroup());
		return $or_group;
		}

		/**
		* Adds conditions for a synchronized scope.
		*

tests/src/Kernel/QueryAlter/EntityQueryAlterTestBase.php

+10 −2

Original line number	Diff line number	Diff line
		@@ -24,6 +24,9 @@ abstract class EntityQueryAlterTestBase extends QueryAlterTestBase {
		* {@inheritdoc}
		*/
		protected function getPermission($operation, $scope, $unpublished = FALSE) {
		if ($operation === 'unsupported') {
		return FALSE;
		}
		$status = $unpublished ? 'unpublished ' : '';
		return "$operation $scope $status$this->pluginId entity";
		}
		@@ -107,11 +110,16 @@ abstract class EntityQueryAlterTestBase extends QueryAlterTestBase {
		/**
		* {@inheritdoc}
		*/
		protected function addSynchronizedConditions(array $allowed_ids, ConditionInterface $conditions) {
		protected function addSynchronizedConditions(array $allowed_ids, ConditionInterface $conditions, $outsider) {
		$conditions->condition($type_conditions = $conditions->andConditionGroup());
		$type_conditions->condition('gcfd.group_type', $allowed_ids, 'IN');
		if ($outsider) {
		$type_conditions->isNull('gcfd_2.entity_id');
		}
		else {
		$type_conditions->isNotNull('gcfd_2.entity_id');
		}
		}

		/**
		* {@inheritdoc}