Merge pull request #692 from Gizra/neutral-access-result

Permission based access checks should return neutral if a permission is not granted

src/Event/AccessEventBase.php

@@ -57,15 +57,25 @@ 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;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function mergeAccessResult(AccessResultInterface $access_result): AccessResultInterface {
+    $this->access = $this->access->orIf($access_result);
+    return $this->access;
   }
 
   /**

src/Event/AccessEventInterface.php

@@ -19,17 +19,31 @@ 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.
    *
    * 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(): AccessResultInterface;
+
+  /**
+   * Merges the given access result with the existing access result.
+   *
+   * @return \Drupal\Core\Access\AccessResultInterface
+   *   The updated access result.
    */
-  public function denyAccess(): void;
+  public function mergeAccessResult(AccessResultInterface $access_result): AccessResultInterface;
 
   /**
    * Returns the group that provides the context for the access check.

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));
       }
     }
   }

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;
   }
 

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.
    *

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.

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);
   }
 

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);
   }