Merge pull request #673 from Gizra/split-up-useraccessentity

Split up OgAccess::userAccessEntity()
Gizra · Aug 2, 2020 · 8029b4b · 8029b4b
2 parents ce0fb10 + 34a02f0
commit 8029b4b
Show file tree

Hide file tree

Showing 10 changed files with 144 additions and 73 deletions.
diff --git a/og.module b/og.module
@@ -154,7 +154,7 @@ function og_entity_access(EntityInterface $entity, $operation, AccountInterface
   }
 
   /** @var \Drupal\Core\Access\AccessResult $access */
-  $access = \Drupal::service('og.access')->userAccessEntity($operation, $entity, $account);
+  $access = \Drupal::service('og.access')->userAccessEntityOperation($operation, $entity, $account);
 
   if ($access->isAllowed()) {
     return $access;

diff --git a/og.services.yml b/og.services.yml
@@ -21,7 +21,7 @@ services:
       - { name: 'cache.context'}
   og.access:
     class: Drupal\og\OgAccess
-    arguments: ['@config.factory', '@current_user', '@module_handler', '@og.group_type_manager', '@og.permission_manager', '@og.membership_manager', '@og.group_audience_helper']
+    arguments: ['@config.factory', '@current_user', '@module_handler', '@og.group_type_manager', '@og.permission_manager', '@og.membership_manager']
   og.context:
     class: Drupal\og\ContextProvider\OgContext
     arguments: ['@plugin.manager.og.group_resolver', '@config.factory']

diff --git a/src/EventSubscriber/OgEventSubscriber.php b/src/EventSubscriber/OgEventSubscriber.php
@@ -88,7 +88,7 @@ public function provideDefaultOgPermissions(PermissionEventInterface $event) {
       new GroupPermission([
         'name' => OgAccess::UPDATE_GROUP_PERMISSION,
         'title' => $this->t('Edit group'),
-        'description' => $this->t('Edit the group. Note: This permission controls only node entity type groups.'),
+        'description' => $this->t('Edit the group entity.'),
         'default roles' => [OgRoleInterface::ADMINISTRATOR],
       ]),
       new GroupPermission([

diff --git a/src/Og.php b/src/Og.php
@@ -260,8 +260,6 @@ public static function isGroup($entity_type_id, $bundle_id) {
   /**
    * Check if the given entity type and bundle is a group content.
    *
-   * This works by checking if the bundle has one or more group audience fields.
-   *
    * @param string $entity_type_id
    *   The entity type.
    * @param string $bundle_id
@@ -271,7 +269,7 @@ public static function isGroup($entity_type_id, $bundle_id) {
    *   True or false if the given entity is group content.
    */
   public static function isGroupContent($entity_type_id, $bundle_id) {
-    return \Drupal::service('og.group_audience_helper')->hasGroupAudienceField($entity_type_id, $bundle_id);
+    return \Drupal::service('og.group_type_manager')->isGroupContent($entity_type_id, $bundle_id);
   }
 
   /**

diff --git a/src/OgAccess.php b/src/OgAccess.php
@@ -34,6 +34,13 @@ class OgAccess implements OgAccessInterface {
    */
   const UPDATE_GROUP_PERMISSION = 'update group';
 
+  /**
+   * Maps entity operations performed on groups to group level permissions.
+   */
+  const OPERATION_GROUP_PERMISSION_MAPPING = [
+    'update' => self::UPDATE_GROUP_PERMISSION,
+  ];
+
   /**
    * The config factory.
    *
@@ -76,13 +83,6 @@ class OgAccess implements OgAccessInterface {
    */
   protected $membershipManager;
 
-  /**
-   * The OG group audience helper.
-   *
-   * @var \Drupal\og\OgGroupAudienceHelperInterface
-   */
-  protected $groupAudienceHelper;
-
   /**
    * Constructs the OgAccess service.
    *
@@ -98,17 +98,14 @@ class OgAccess implements OgAccessInterface {
    *   The permission manager.
    * @param \Drupal\og\MembershipManagerInterface $membership_manager
    *   The group membership manager.
-   * @param \Drupal\og\OgGroupAudienceHelperInterface $group_audience_helper
-   *   The OG group audience helper.
    */
-  public function __construct(ConfigFactoryInterface $config_factory, AccountProxyInterface $account_proxy, ModuleHandlerInterface $module_handler, GroupTypeManagerInterface $group_manager, PermissionManagerInterface $permission_manager, MembershipManagerInterface $membership_manager, OgGroupAudienceHelperInterface $group_audience_helper) {
+  public function __construct(ConfigFactoryInterface $config_factory, AccountProxyInterface $account_proxy, ModuleHandlerInterface $module_handler, GroupTypeManagerInterface $group_manager, PermissionManagerInterface $permission_manager, MembershipManagerInterface $membership_manager) {
     $this->configFactory = $config_factory;
     $this->accountProxy = $account_proxy;
     $this->moduleHandler = $module_handler;
     $this->groupTypeManager = $group_manager;
     $this->permissionManager = $permission_manager;
     $this->membershipManager = $membership_manager;
-    $this->groupAudienceHelper = $group_audience_helper;
   }
 
   /**
@@ -208,15 +205,15 @@ public function userAccess(EntityInterface $group, string $permission, AccountIn
   /**
    * {@inheritdoc}
    */
-  public function userAccessEntity($operation, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface {
+  public function userAccessEntity(string $permission, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface {
     $result = AccessResult::neutral();
 
     $entity_type = $entity->getEntityType();
     $entity_type_id = $entity_type->id();
     $bundle = $entity->bundle();
 
     if ($this->groupTypeManager->isGroup($entity_type_id, $bundle)) {
-      $user_access = $this->userAccess($entity, $operation, $user);
+      $user_access = $this->userAccess($entity, $permission, $user);
       if ($user_access->isAllowed()) {
         return $user_access;
       }
@@ -230,8 +227,7 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt
       }
     }
 
-    $is_group_content = $this->groupAudienceHelper->hasGroupAudienceField($entity_type_id, $bundle);
-    if ($is_group_content) {
+    if ($this->groupTypeManager->isGroupContent($entity_type_id, $bundle)) {
       $cache_tags = $entity_type->getListCacheTags();
 
       // The entity might be a user or a non-user entity.
@@ -241,17 +237,7 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt
         $forbidden = AccessResult::forbidden()->addCacheTags($cache_tags);
         foreach ($groups as $entity_groups) {
           foreach ($entity_groups as $group) {
-            // Check if the operation matches a group content entity operation
-            // such as 'create article content'.
-            $operation_access = $this->userAccessGroupContentEntityOperation($operation, $group, $entity, $user);
-
-            if ($operation_access->isAllowed()) {
-              return $operation_access->addCacheTags($cache_tags);
-            }
-
-            // Check if the operation matches a group level operation such as
-            // 'subscribe without approval'.
-            $user_access = $this->userAccess($group, $operation, $user);
+            $user_access = $this->userAccess($group, $permission, $user);
             if ($user_access->isAllowed()) {
               return $user_access->addCacheTags($cache_tags);
             }
@@ -265,15 +251,73 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt
       $result->addCacheTags($cache_tags);
     }
 
-    // Either the user didn't have permission, or the entity might be an
-    // orphaned group content.
+    // Either the user didn't have permission, or the entity might be orphaned
+    // group content.
+    return $result;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function userAccessEntityOperation(string $operation, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface {
+    $result = AccessResult::neutral();
+
+    $entity_type = $entity->getEntityType();
+    $entity_type_id = $entity_type->id();
+    $bundle = $entity->bundle();
+
+    if ($this->groupTypeManager->isGroup($entity_type_id, $bundle)) {
+      // We are performing an entity operation on a group entity. Map the
+      // operation to the corresponding group level permission.
+      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);
+        }
+      }
+    }
+
+    if ($this->groupTypeManager->isGroupContent($entity_type_id, $bundle)) {
+      $cache_tags = $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);
+            }
+          }
+        }
+        return $forbidden;
+      }
+
+      $result->addCacheTags($cache_tags);
+    }
+
+    // Either the user didn't have permission, or the entity might be orphaned
+    // group content.
     return $result;
   }
 
   /**
    * {@inheritdoc}
    */
-  public function userAccessGroupContentEntityOperation($operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL): AccessResultInterface {
+  public function userAccessGroupContentEntityOperation(string $operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL): AccessResultInterface {
     // Default to the current user.
     $user = $user ?: $this->accountProxy->getAccount();
 

diff --git a/src/OgAccessInterface.php b/src/OgAccessInterface.php
@@ -14,7 +14,7 @@
 interface OgAccessInterface {
 
   /**
-   * Determines whether a user has a certain permission in a given group.
+   * Determines whether a user has a group permission in a given group.
    *
    * The following conditions will result in a positive result:
    * - The user is the global super user (UID 1).
@@ -35,7 +35,9 @@ interface OgAccessInterface {
    * @param \Drupal\Core\Entity\EntityInterface $group
    *   The group entity.
    * @param string $permission
-   *   The permission being checked.
+   *   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
    *   (optional) The user to check. Defaults to the current user.
    * @param bool $skip_alter
@@ -50,31 +52,72 @@ interface OgAccessInterface {
   public function userAccess(EntityInterface $group, string $permission, AccountInterface $user = NULL, bool $skip_alter = FALSE): AccessResultInterface;
 
   /**
-   * Check if a user has access to a permission on a certain entity context.
+   * Determines whether a user has a group permission in a given entity.
+   *
+   * This does an exhaustive, but slow, check to discover whether access can be
+   * granted and works both on groups and group content. It will iterate over
+   * all groups that are associated with the entity and do a permission check on
+   * each group. If a passed in entity is both a group and group content, it
+   * will return a positive result if the user has the requested permission in
+   * either the entity itself or 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 ::userAccess().
+   *
+   * @param string $permission
+   *   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\Entity\EntityInterface $entity
+   *   The entity object. This can be either a group or group content entity.
+   * @param \Drupal\Core\Session\AccountInterface $user
+   *   (optional) The user object. If empty the current user will be used.
+   *
+   * @return \Drupal\Core\Access\AccessResultInterface
+   *   An access result object.
+   *
+   * @see \Drupal\og\userAccess();
+   */
+  public function userAccessEntity(string $permission, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface;
+
+  /**
+   * Checks whether a user can perform an operation on a given entity.
+   *
+   * This does an exhaustive, but slow, check to discover whether the operation
+   * 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).
+   *
+   * In case you know the specific group you want to check access for then it is
+   * recommended to use the faster ::userAccessGroupContentEntityOperation().
    *
    * @param string $operation
-   *   The operation to perform on the entity.
+   *   The entity operation, such as "create", "update" or "delete".
    * @param \Drupal\Core\Entity\EntityInterface $entity
-   *   The entity object.
+   *   The entity object. This can be either a group or group content entity.
    * @param \Drupal\Core\Session\AccountInterface $user
    *   (optional) The user object. If empty the current user will be used.
    *
    * @return \Drupal\Core\Access\AccessResultInterface
    *   An access result object.
+   *
+   * @see \Drupal\og\userAccessGroupContentEntityOperation();
    */
-  public function userAccessEntity($operation, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface;
+  public function userAccessEntityOperation(string $operation, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface;
 
   /**
-   * Checks access for entity operations on group content entities.
+   * Checks access for entity operations on group content in a specific group.
    *
    * 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. There is no formal support for access control on entity
-   * operations in core, so the mapping of permissions to operations is provided
-   * by PermissionManager::getEntityOperationPermissions().
+   * in the given group.
    *
    * @param string $operation
-   *   The entity operation.
+   *   The entity operation, such as "create", "update" or "delete".
    * @param \Drupal\Core\Entity\EntityInterface $group_entity
    *   The group entity, to retrieve the permissions from.
    * @param \Drupal\Core\Entity\EntityInterface $group_content_entity
@@ -86,10 +129,8 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt
    *
    * @return \Drupal\Core\Access\AccessResultInterface
    *   The access result object.
-   *
-   * @see \Drupal\og\PermissionManager::getEntityOperationPermissions()
    */
-  public function userAccessGroupContentEntityOperation($operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL): AccessResultInterface;
+  public function userAccessGroupContentEntityOperation(string $operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL): AccessResultInterface;
 
   /**
    * Resets the static cache.

diff --git a/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php b/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php
@@ -267,7 +267,7 @@ public function testAccess($group_content_bundle_id, $expected_access_matrix) {
           // group owner.
           $entity = $ownership === 'own' ? $this->groupContent[$group_content_bundle_id][$user_id] : $this->groupContent[$group_content_bundle_id]['group_owner'];
           $user = $this->users[$user_id];
-          $this->assertEquals($expected_access, $og_access->userAccessEntity($operation, $entity, $user)->isAllowed(), "Operation: $operation, ownership: $ownership, user: $user_id, bundle: $group_content_bundle_id");
+          $this->assertEquals($expected_access, $og_access->userAccessEntityOperation($operation, $entity, $user)->isAllowed(), "Operation: $operation, ownership: $ownership, user: $user_id, bundle: $group_content_bundle_id");
         }
       }
     }

diff --git a/tests/src/Unit/OgAccessEntityTest.php b/tests/src/Unit/OgAccessEntityTest.php
@@ -11,33 +11,33 @@
 class OgAccessEntityTest extends OgAccessEntityTestBase {
 
   /**
-   * Tests access to an entity by different operations.
+   * Tests access to an entity by different permissions.
    *
    * @coversDefaultmethod ::userAccessEntity
    * @dataProvider permissionsProvider
    */
-  public function testAccessByOperation($operation) {
+  public function testAccessByPermission($permission) {
     $this->membershipManager->getGroups($this->groupContentEntity->reveal())->willReturn([$this->entityTypeId => [$this->group]]);
 
-    $user_access = $this->ogAccess->userAccessEntity($operation, $this->groupContentEntity->reveal(), $this->user->reveal());
+    $user_access = $this->ogAccess->userAccessEntity($permission, $this->groupContentEntity->reveal(), $this->user->reveal());
 
     // We populate the allowed permissions cache in
     // OgAccessEntityTestBase::setup().
-    $condition = $operation == 'update group' ? $user_access->isAllowed() : $user_access->isForbidden();
+    $condition = $permission == 'update group' ? $user_access->isAllowed() : $user_access->isForbidden();
     $this->assertTrue($condition);
   }
 
   /**
-   * Tests access to an entity by different operations, by an admin member.
+   * Tests access to an entity by different permissions, by an admin member.
    *
    * @coversDefaultmethod ::userAccessEntity
    * @dataProvider permissionsProvider
    */
-  public function testAccessByOperationAdmin($operation) {
+  public function testAccessByPermissionAdmin($permission) {
     $this->membershipManager->getGroups($this->groupContentEntity->reveal())->willReturn([$this->entityTypeId => [$this->group]]);
 
     $this->user->hasPermission('administer organic groups')->willReturn(TRUE);
-    $user_entity_access = $this->ogAccess->userAccessEntity($operation, $this->groupContentEntity->reveal(), $this->user->reveal());
+    $user_entity_access = $this->ogAccess->userAccessEntity($permission, $this->groupContentEntity->reveal(), $this->user->reveal());
     $this->assertTrue($user_entity_access->isAllowed());
   }
 

diff --git a/tests/src/Unit/OgAccessEntityTestBase.php b/tests/src/Unit/OgAccessEntityTestBase.php
@@ -50,9 +50,9 @@ protected function setUp(): void {
     $this->groupContentEntity->getEntityTypeId()->willReturn($entity_type_id);
     $this->addCache($this->groupContentEntity);
 
-    // If the group audience helper is asked if the group content entity has any
-    // group audience fields, it is expected that this will return TRUE.
-    $this->groupAudienceHelper->hasGroupAudienceField($entity_type_id, $bundle)
+    // If the group type manager is asked if the group content entity is group
+    // content, it is expected that this will return TRUE.
+    $this->groupTypeManager->isGroupContent($entity_type_id, $bundle)
       ->willReturn(TRUE);
 
     // It is expected that a list of entity operation permissions is retrieved