From 29cb3de54e57f6a70b9bd4c02d28e46e167d1ce3 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 12 Aug 2020 11:53:06 +0300 Subject: [PATCH 1/5] Return the updated access result after changing it. --- src/Event/AccessEventBase.php | 6 ++++-- src/Event/AccessEventInterface.php | 10 ++++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/Event/AccessEventBase.php b/src/Event/AccessEventBase.php index 740864ce8..307ae6e93 100644 --- a/src/Event/AccessEventBase.php +++ b/src/Event/AccessEventBase.php @@ -57,15 +57,17 @@ public function __construct(ContentEntityInterface $group, AccountInterface $use /** * {@inheritdoc} */ - public function grantAccess(): void { + public function grantAccess(): AccessResultInterface { $this->access = $this->access->orIf(AccessResult::allowed()); + return $this->access; } /** * {@inheritdoc} */ - public function denyAccess(): void { + public function denyAccess(): AccessResultInterface { $this->access = $this->access->orIf(AccessResult::forbidden()); + return $this->access; } /** diff --git a/src/Event/AccessEventInterface.php b/src/Event/AccessEventInterface.php index 01f23bf08..33afa84f8 100644 --- a/src/Event/AccessEventInterface.php +++ b/src/Event/AccessEventInterface.php @@ -19,8 +19,11 @@ interface AccessEventInterface extends RefinableCacheableDependencyInterface { * * Calling this method will cause access to be granted for the action that is * being checked, unless another event listener denies access. + * + * @return \Drupal\Core\Access\AccessResultInterface + * The updated access result. */ - public function grantAccess(): void; + public function grantAccess(): AccessResultInterface; /** * Declare that access is being denied. @@ -28,8 +31,11 @@ public function grantAccess(): void; * Calling this method will cause access to be denied for the action that is * being checked. This takes precedence over any other event listeners that * might grant access. + * + * @return \Drupal\Core\Access\AccessResultInterface + * The updated access result. */ - public function denyAccess(): void; + public function denyAccess(): AccessResultInterface; /** * Returns the group that provides the context for the access check. From fed3cc251940343d51bcd81d601a7fb2a2b5b323 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 12 Aug 2020 12:11:23 +0300 Subject: [PATCH 2/5] Provide a method to merge the existing access result with another access result. --- src/Event/AccessEventBase.php | 8 ++++++++ src/Event/AccessEventInterface.php | 8 ++++++++ src/EventSubscriber/OgEventSubscriber.php | 11 +---------- 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/Event/AccessEventBase.php b/src/Event/AccessEventBase.php index 307ae6e93..8723774f1 100644 --- a/src/Event/AccessEventBase.php +++ b/src/Event/AccessEventBase.php @@ -70,6 +70,14 @@ public function denyAccess(): AccessResultInterface { return $this->access; } + /** + * {@inheritdoc} + */ + public function mergeAccessResult(AccessResultInterface $access_result): AccessResultInterface { + $this->access = $this->access->orIf($access_result); + return $this->access; + } + /** * {@inheritdoc} */ diff --git a/src/Event/AccessEventInterface.php b/src/Event/AccessEventInterface.php index 33afa84f8..87e6f922b 100644 --- a/src/Event/AccessEventInterface.php +++ b/src/Event/AccessEventInterface.php @@ -37,6 +37,14 @@ public function grantAccess(): AccessResultInterface; */ public function denyAccess(): AccessResultInterface; + /** + * Merges the given access result with the existing access result. + * + * @return \Drupal\Core\Access\AccessResultInterface + * The updated access result. + */ + public function mergeAccessResult(AccessResultInterface $access_result): AccessResultInterface; + /** * Returns the group that provides the context for the access check. * diff --git a/src/EventSubscriber/OgEventSubscriber.php b/src/EventSubscriber/OgEventSubscriber.php index 8d363fab8..5b42b2596 100644 --- a/src/EventSubscriber/OgEventSubscriber.php +++ b/src/EventSubscriber/OgEventSubscriber.php @@ -416,16 +416,7 @@ public function checkGroupContentEntityOperationAccess(GroupContentEntityOperati if ($permissions) { foreach ($permissions as $permission) { - // This currently does not handle access in the same way as Drupal core - // does - if any of the permissions grant access we will grant access. - // Once OgAccess::userAccess() is refactored to return a neutral result - // in case no access is determined we can just apply the result - // directly. - $access_result = $this->ogAccess->userAccess($group_entity, $permission->getName(), $user); - if ($access_result->isAllowed()) { - $event->grantAccess(); - break; - } + $event->mergeAccessResult($this->ogAccess->userAccess($group_entity, $permission->getName(), $user)); } } } From 28578145f3ba802bdd39a75f6a06141b82af53c9 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 12 Aug 2020 12:12:06 +0300 Subject: [PATCH 3/5] Align permission based access checks with how it is handled in core. In Drupal core the access result is neutral if a user doesn't have the permission. --- src/OgAccess.php | 66 ++++++------------- .../Kernel/Access/GroupLevelAccessTest.php | 6 +- tests/src/Unit/OgAccessEntityTest.php | 2 +- tests/src/Unit/OgAccessTest.php | 2 +- 4 files changed, 24 insertions(+), 52 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 9471adaba..379a71545 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -217,7 +217,7 @@ public function userAccess(EntityInterface $group, string $permission, AccountIn return AccessResult::allowed()->addCacheableDependency($cacheable_metadata); } - return AccessResult::forbidden()->addCacheableDependency($cacheable_metadata); + return AccessResult::neutral()->addCacheableDependency($cacheable_metadata); } /** @@ -231,46 +231,31 @@ public function userAccessEntity(string $permission, EntityInterface $entity, Ac $bundle = $entity->bundle(); if ($this->groupTypeManager->isGroup($entity_type_id, $bundle)) { - $user_access = $this->userAccess($entity, $permission, $user); - if ($user_access->isAllowed()) { - return $user_access; - } - else { - // An entity can be a group and group content in the same time. The - // group didn't allow access, but the user still might have access to - // the permission in group content context. So instead of returning a - // deny here, we set the result, that might change if an access is - // found. - $result = AccessResult::forbidden()->inheritCacheability($user_access); + // An entity can be a group and group content in the same time. If the + // group returns a neutral result the user still might have access to + // the permission in group content context. So if we get a neutral result + // we will continue with the group content access check below. + $result = $this->userAccess($entity, $permission, $user); + if (!$result->isNeutral()) { + return $result; } } if ($this->groupTypeManager->isGroupContent($entity_type_id, $bundle)) { - $cache_tags = $entity_type->getListCacheTags(); + $result->addCacheTags($entity_type->getListCacheTags()); // The entity might be a user or a non-user entity. $groups = $entity instanceof UserInterface ? $this->membershipManager->getUserGroups($entity->id()) : $this->membershipManager->getGroups($entity); if ($groups) { - $forbidden = AccessResult::forbidden()->addCacheTags($cache_tags); foreach ($groups as $entity_groups) { foreach ($entity_groups as $group) { - $user_access = $this->userAccess($group, $permission, $user); - if ($user_access->isAllowed()) { - return $user_access->addCacheTags($cache_tags); - } - - $forbidden->inheritCacheability($user_access); + $result = $result->orIf($this->userAccess($group, $permission, $user)); } } - return $forbidden; } - - $result->addCacheTags($cache_tags); } - // Either the user didn't have permission, or the entity might be orphaned - // group content. return $result; } @@ -290,45 +275,32 @@ public function userAccessEntityOperation(string $operation, EntityInterface $en if (array_key_exists($operation, self::OPERATION_GROUP_PERMISSION_MAPPING)) { $permission = self::OPERATION_GROUP_PERMISSION_MAPPING[$operation]; - $user_access = $this->userAccess($entity, $permission, $user); - if ($user_access->isAllowed()) { - return $user_access; - } - else { - // An entity can be a group and group content in the same time. The - // group permission check didn't allow access, but the user still - // might have access to perform the operation in group content - // context. So instead of returning a deny here, we set the result, - // that might change if an access is found. - $result = AccessResult::forbidden()->inheritCacheability($user_access); + // An entity can be a group and group content in the same time. If the + // group returns a neutral result the user still might have access to + // the permission in group content context. So if we get a neutral result + // we will continue with the group content access check below. + $result = $this->userAccess($entity, $permission, $user); + if (!$result->isNeutral()) { + return $result; } } } if ($this->groupTypeManager->isGroupContent($entity_type_id, $bundle)) { - $cache_tags = $entity_type->getListCacheTags(); + $result->addCacheTags($entity_type->getListCacheTags()); // The entity might be a user or a non-user entity. $groups = $entity instanceof UserInterface ? $this->membershipManager->getUserGroups($entity->id()) : $this->membershipManager->getGroups($entity); if ($groups) { - $forbidden = AccessResult::forbidden()->addCacheTags($cache_tags); foreach ($groups as $entity_groups) { foreach ($entity_groups as $group) { - $operation_access = $this->userAccessGroupContentEntityOperation($operation, $group, $entity, $user); - if ($operation_access->isAllowed()) { - return $operation_access->addCacheTags($cache_tags); - } + $result = $result->orIf($this->userAccessGroupContentEntityOperation($operation, $group, $entity, $user)); } } - return $forbidden; } - - $result->addCacheTags($cache_tags); } - // Either the user didn't have permission, or the entity might be orphaned - // group content. return $result; } diff --git a/tests/src/Kernel/Access/GroupLevelAccessTest.php b/tests/src/Kernel/Access/GroupLevelAccessTest.php index 47fc0520b..47c0cffef 100644 --- a/tests/src/Kernel/Access/GroupLevelAccessTest.php +++ b/tests/src/Kernel/Access/GroupLevelAccessTest.php @@ -146,15 +146,15 @@ public function testUserAccessArbitraryPermissions() { $this->assertTrue($this->ogAccess->userAccess($this->group, 'some_perm', $users['has_permission_in_both_groups'])->isAllowed()); // This user should not have access to 'some_perm_2' as that was only // assigned to group 2. - $this->assertTrue($this->ogAccess->userAccess($this->group, 'some_perm_2', $users['has_permission_in_both_groups'])->isForbidden()); + $this->assertTrue($this->ogAccess->userAccess($this->group, 'some_perm_2', $users['has_permission_in_both_groups'])->isNeutral()); // Check the permission of group 1 again. $this->assertTrue($this->ogAccess->userAccess($this->group, 'some_perm', $users['has_permission_in_both_groups'])->isAllowed()); // A member user without the correct role. - $this->assertTrue($this->ogAccess->userAccess($this->group, 'some_perm', $users['has_no_permission'])->isForbidden()); + $this->assertTrue($this->ogAccess->userAccess($this->group, 'some_perm', $users['has_no_permission'])->isNeutral()); // A non-member user. - $this->assertTrue($this->ogAccess->userAccess($this->group, 'some_perm', $this->nonMemberUser)->isForbidden()); + $this->assertTrue($this->ogAccess->userAccess($this->group, 'some_perm', $this->nonMemberUser)->isNeutral()); // Grant the arbitrary permission to non-members and check that our // non-member now has the permission. diff --git a/tests/src/Unit/OgAccessEntityTest.php b/tests/src/Unit/OgAccessEntityTest.php index dd41f1d49..c1ea6e19b 100644 --- a/tests/src/Unit/OgAccessEntityTest.php +++ b/tests/src/Unit/OgAccessEntityTest.php @@ -25,7 +25,7 @@ public function testAccessByPermission($permission) { // We populate the allowed permissions cache in // OgAccessEntityTestBase::setup(). - $condition = $permission == 'update group' ? $user_access->isAllowed() : $user_access->isForbidden(); + $condition = $permission == 'update group' ? $user_access->isAllowed() : $user_access->isNeutral(); $this->assertTrue($condition); } diff --git a/tests/src/Unit/OgAccessTest.php b/tests/src/Unit/OgAccessTest.php index 4b7841ce1..467e057f7 100644 --- a/tests/src/Unit/OgAccessTest.php +++ b/tests/src/Unit/OgAccessTest.php @@ -35,7 +35,7 @@ public function testAccessByOperation($operation) { // We populate the allowed permissions cache in // OgAccessTestBase::setup(). - $condition = $operation == 'update group' ? $user_access->isAllowed() : $user_access->isForbidden(); + $condition = $operation == 'update group' ? $user_access->isAllowed() : $user_access->isNeutral(); $this->assertTrue($condition); } From a98424d1befde7399debc2474f8dd7c996a100f4 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 12 Aug 2020 12:22:06 +0300 Subject: [PATCH 4/5] Adhere to coding standards. --- src/OgAccess.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 379a71545..c8e3a826c 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -277,8 +277,8 @@ public function userAccessEntityOperation(string $operation, EntityInterface $en // An entity can be a group and group content in the same time. If the // group returns a neutral result the user still might have access to - // the permission in group content context. So if we get a neutral result - // we will continue with the group content access check below. + // the permission in group content context. So if we get a neutral + // result we will continue with the group content access check below. $result = $this->userAccess($entity, $permission, $user); if (!$result->isNeutral()) { return $result; From cc0a723e4eb910336e142fc11b168df078aef441 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Sat, 15 Aug 2020 21:09:45 +0300 Subject: [PATCH 5/5] Update documentation. --- src/OgAccessInterface.php | 50 +++++++++++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 10 deletions(-) diff --git a/src/OgAccessInterface.php b/src/OgAccessInterface.php index 59fb7d557..eecd2db18 100644 --- a/src/OgAccessInterface.php +++ b/src/OgAccessInterface.php @@ -25,8 +25,12 @@ interface OgAccessInterface { * - The user has a role in the group that specifically grants the permission. * - The user is not a member of the group, and the permission has been * granted to non-members. + * - The access result can be altered (granted or denied) by implementing + * hook_og_user_access(). * - * The access result can be altered by implementing hook_og_user_access(). + * For access to be granted, at least one of the above checks should grant + * access, and none of the checks should deny access. A neutral result is + * returned only if all checks are neutral. * * All access checks in OG should go through this function. This way we * guarantee consistent behavior, and ensure that the superuser and group @@ -38,7 +42,7 @@ interface OgAccessInterface { * The name of the OG permission being checked. This includes both group * level permissions such as 'subscribe without approval' and group content * entity operation permissions such as 'edit own article content'. - * @param \Drupal\Core\Session\AccountInterface $user + * @param \Drupal\Core\Session\AccountInterface|null $user * (optional) The user to check. Defaults to the current user. * @param bool $skip_alter * (optional) If TRUE then user access will not be sent to other modules @@ -61,6 +65,13 @@ public function userAccess(EntityInterface $group, string $permission, AccountIn * will return a positive result if the user has the requested permission in * either the entity itself or its parent group(s). * + * For access to be granted, at least one of the groups with which the entity + * is associated should allow access, and none of the associated groups should + * deny access. Access is denied if one or more groups deny access, regardless + * of how many groups grant access. A neutral result is returned if the passed + * in entity is not associated with any groups, or if all the associated + * groups return a neutral result. + * * In case you know the specific group you want to check access for then it is * recommended to use the faster ::userAccess(). * @@ -70,7 +81,7 @@ public function userAccess(EntityInterface $group, string $permission, AccountIn * entity operation permissions such as 'edit own article content'. * @param \Drupal\Core\Entity\EntityInterface $entity * The entity object. This can be either a group or group content entity. - * @param \Drupal\Core\Session\AccountInterface $user + * @param \Drupal\Core\Session\AccountInterface|null $user * (optional) The user object. If empty the current user will be used. * * @return \Drupal\Core\Access\AccessResultInterface @@ -87,10 +98,19 @@ public function userAccessEntity(string $permission, EntityInterface $entity, Ac * can be performed. It works both with groups and group content entities. It * will iterate over all groups that are associated with the entity and check * if the user is allowed to perform the entity operation on the group content - * entity according to their role within each group. If a passed in entity is - * both a group and group content, it will return a positive result if the - * user has permission to perform the operation in either the entity itself or - * one of its parent group(s). + * entity according to their role within each group. + * + * For access to be granted, at least one of the groups with which the entity + * is associated should allow access, and none of the associated groups should + * deny access. Access is denied if one or more groups deny access, regardless + * of how many groups grant access. A neutral result is returned if the passed + * in entity is not associated with any groups, or if all the associated + * groups return a neutral result. + * + * If a passed in entity is both a group and group content, it will first + * check if the user has permission to perform the operation on the entity + * itself. Only if this returns a neutral result the access check will be + * performed on its parent group(s). * * In case you know the specific group you want to check access for then it is * recommended to use the faster ::userAccessGroupContentEntityOperation(). @@ -99,7 +119,7 @@ public function userAccessEntity(string $permission, EntityInterface $entity, Ac * The entity operation, such as "create", "update" or "delete". * @param \Drupal\Core\Entity\EntityInterface $entity * The entity object. This can be either a group or group content entity. - * @param \Drupal\Core\Session\AccountInterface $user + * @param \Drupal\Core\Session\AccountInterface|null $user * (optional) The user object. If empty the current user will be used. * * @return \Drupal\Core\Access\AccessResultInterface @@ -114,7 +134,17 @@ public function userAccessEntityOperation(string $operation, EntityInterface $en * * This checks if the user has permission to perform the requested operation * on the given group content entity according to the user's membership status - * in the given group. + * in the given group. It also passes through ::userAccess() to check for any' + * of the special cases, such as being the root user, having global permission + * to administer all groups, etc. + * + * The access result can be altered by implementing an event listener for + * GroupContentEntityOperationAccessEventInterface::EVENT_NAME. + * + * For access to be granted, at least one of the above checks should grant + * access, and none of the event listeners should deny access. A neutral + * result is returned only if all checks are neutral or if the passed in + * entity is not group content. * * @param string $operation * The entity operation, such as "create", "update" or "delete". @@ -123,7 +153,7 @@ public function userAccessEntityOperation(string $operation, EntityInterface $en * @param \Drupal\Core\Entity\EntityInterface $group_content_entity * The group content entity for which access to the entity operation is * requested. - * @param \Drupal\Core\Session\AccountInterface $user + * @param \Drupal\Core\Session\AccountInterface|null $user * Optional user for which to check access. If omitted, the currently logged * in user will be used. *