From 8de919ae86baeea8515d5f659612bf8ef54b6056 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Fri, 27 May 2016 18:38:11 +0300 Subject: [PATCH 01/85] Check access to do CRUD operations on group content. --- src/Og.php | 24 ++++++++++++++ src/OgAccess.php | 81 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+) diff --git a/src/Og.php b/src/Og.php index 7bc09ce64..4b29b9c7d 100644 --- a/src/Og.php +++ b/src/Og.php @@ -255,6 +255,30 @@ public static function getUserMemberships(AccountInterface $user, array $states return static::$cache[$identifier]; } + /** + * Returns the group membership for a given user and group. + * + * @param \Drupal\Core\Session\AccountInterface $user + * The user to get the membership for. + * @param \Drupal\Core\Entity\EntityInterface $group + * The group to get the membership for. + * @param array $states + * (optional) Array with the state to return. Defaults to active. + * @param string $field_name + * (optional) The field name associated with the group. + * + * @return \Drupal\og\Entity\OgMembership|NULL + * The OgMembership entity, or NULL if the user is not a member of the + * group. + */ + public static function getUserMembership(AccountInterface $user, EntityInterface $group, array $states = [OgMembershipInterface::STATE_ACTIVE], $field_name = NULL) { + foreach (static::getUserMemberships($user, $states, $field_name) as $membership) { + if ($membership->getGroupEntityType() === $group->getEntityTypeId() && $membership->getEntityId() === $group->id()) { + return $membership; + } + } + } + /** * Returns all group IDs associated with the given group content entity. * diff --git a/src/OgAccess.php b/src/OgAccess.php index cd3cd4b3b..f1ac2d3ab 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -211,6 +211,10 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt $forbidden = AccessResult::forbidden()->addCacheTags($cache_tags); foreach ($groups as $entity_groups) { foreach ($entity_groups as $group) { + $crud_access = $this->userAccessGroupContentEntityCrud($operation, $group, $entity, $user); + if ($crud_access->isAllowed()) { + return $crud_access->addCacheTags($cache_tags); + } $user_access = $this->userAccess($group, $operation, $user); if ($user_access->isAllowed()) { return $user_access->addCacheTags($cache_tags); @@ -230,6 +234,83 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt return $result; } + /** + * Checks access for CRUD operations on group content entities. + * + * This checks access for the default CRUD operations added by the permission + * manager. + * + * @param string $operation + * The entity operation. + * @param \Drupal\Core\Entity\EntityInterface $group_entity + * The group entity, to retrieve the permissions from. + * @param \Drupal\Core\Entity\EntityInterface $group_content_entity + * The group content entity for which the CRUD operation access is + * requested. + * @param \Drupal\Core\Session\AccountInterface $user + * The user for which to check access. + * + * @return \Drupal\Core\Access\AccessResult + * The access result object. + * + * @see \Drupal\og\PermissionManager::generateCrudPermissionList() + */ + public static function userAccessGroupContentEntityCrud($operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL) { + if (!in_array($operation, ['create', 'update', 'delete'])) { + return AccessResult::neutral(); + } + + // Default to the current user. + if (!isset($user)) { + $user = \Drupal::currentUser()->getAccount(); + } + + // From this point on, every result also depends on the user so check + // whether it is the current. See https://www.drupal.org/node/2628870 + $cacheable_metadata = new CacheableMetadata; + $cacheable_metadata->addCacheableDependency($group_content_entity); + if ($user->id() == \Drupal::currentUser()->id()) { + $cacheable_metadata->addCacheContexts(['user']); + } + + $is_owner = $group_content_entity instanceof EntityOwnerInterface && $group_content_entity->getOwnerId() == $user->id(); + $group_content_entity_type_id = $group_content_entity->getEntityTypeId(); + $group_content_bundle_id = $group_content_entity->bundle(); + + // Generate the permissions to check. + switch ($operation) { + case 'create': + $permissions = ["create $group_content_bundle_id $group_content_entity_type_id"]; + break; + + case 'update': + $permissions = ["update any $group_content_bundle_id $group_content_entity_type_id"]; + if ($is_owner) { + $permissions[] = "update own $group_content_bundle_id $group_content_entity_type_id"; + } + break; + + case 'delete': + $permissions = ["delete any $group_content_bundle_id $group_content_entity_type_id"]; + if ($is_owner) { + $permissions[] = "delete own $group_content_bundle_id $group_content_entity_type_id"; + } + break; + } + + // @todo Also deal with the use case that CRUD operations are granted to + // non-members. + if ($membership = Og::getUserMembership($user, $group_entity)) { + foreach ($permissions as $permission) { + if ($membership->hasPermission($permission)) { + return AccessResult::allowed()->addCacheableDependency($cacheable_metadata); + } + } + } + + return AccessResult::neutral()->addCacheableDependency($cacheable_metadata); + } + /** * Set the permissions in the static cache. * From 73d803bc35d9ad07fb22827d54f2cf83ca97ebb0 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 8 Jun 2016 23:41:32 +0200 Subject: [PATCH 02/85] Do not use CRUD in the method names, there are more operations than only CRUD operations. --- src/OgAccess.php | 8 ++++---- src/PermissionManager.php | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index f1ac2d3ab..76fbbe091 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -211,7 +211,7 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt $forbidden = AccessResult::forbidden()->addCacheTags($cache_tags); foreach ($groups as $entity_groups) { foreach ($entity_groups as $group) { - $crud_access = $this->userAccessGroupContentEntityCrud($operation, $group, $entity, $user); + $crud_access = $this->userAccessGroupContentEntityOperations($operation, $group, $entity, $user); if ($crud_access->isAllowed()) { return $crud_access->addCacheTags($cache_tags); } @@ -235,7 +235,7 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt } /** - * Checks access for CRUD operations on group content entities. + * Checks access for entity operations on group content entities. * * This checks access for the default CRUD operations added by the permission * manager. @@ -253,9 +253,9 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt * @return \Drupal\Core\Access\AccessResult * The access result object. * - * @see \Drupal\og\PermissionManager::generateCrudPermissionList() + * @see \Drupal\og\PermissionManager::getEntityOperationPermissions() */ - public static function userAccessGroupContentEntityCrud($operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL) { + public static function userAccessGroupContentEntityOperations($operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL) { if (!in_array($operation, ['create', 'update', 'delete'])) { return AccessResult::neutral(); } diff --git a/src/PermissionManager.php b/src/PermissionManager.php index e4329a320..0cc3dcc40 100644 --- a/src/PermissionManager.php +++ b/src/PermissionManager.php @@ -67,7 +67,7 @@ public function getPermissionList($entity_type_id, $bundle_id) { foreach ($this->groupManager->getGroupContentBundleIdsByGroupBundle($entity_type_id, $bundle_id) as $group_content_entity_type_id => $group_content_bundle_ids) { foreach ($group_content_bundle_ids as $group_content_bundle_id) { - $permissions += $this->generateCrudPermissionList($group_content_entity_type_id, $group_content_bundle_id); + $permissions += $this->getEntityOperationPermissions($group_content_entity_type_id, $group_content_bundle_id); } } @@ -75,7 +75,7 @@ public function getPermissionList($entity_type_id, $bundle_id) { } /** - * Helper function to generate default crud permissions for a given bundle. + * Returns permissions related to entity operations for a given bundle. * * @param $group_content_entity_type_id * The entity type ID for which to generate the permission list. @@ -86,7 +86,7 @@ public function getPermissionList($entity_type_id, $bundle_id) { * An array of permission names and descriptions. * */ - public function generateCrudPermissionList($group_content_entity_type_id, $group_content_bundle_id) { + public function getEntityOperationPermissions($group_content_entity_type_id, $group_content_bundle_id) { $permissions = []; // Check if the bundle is a group content type. From 551fa5d046e749a480198bed8f4618798cba09c5 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 8 Jun 2016 23:42:32 +0200 Subject: [PATCH 03/85] Reuse the generation of the entity operation permissions. --- src/OgAccess.php | 35 ++++++----------------- src/PermissionManager.php | 58 ++++++++++++++++++++++++--------------- 2 files changed, 44 insertions(+), 49 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 76fbbe091..071864ffd 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -256,7 +256,13 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt * @see \Drupal\og\PermissionManager::getEntityOperationPermissions() */ public static function userAccessGroupContentEntityOperations($operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL) { - if (!in_array($operation, ['create', 'update', 'delete'])) { + // Retrieve the permissions and check if our operation is supported. + $group_content_entity_type_id = $group_content_entity->getEntityTypeId(); + $group_content_bundle_id = $group_content_entity->bundle(); + $is_owner = $group_content_entity instanceof EntityOwnerInterface && $group_content_entity->getOwnerId() == $user->id(); + $permissions = \Drupal::service('og.permission_manager')->getEntityOperationPermissions($group_content_entity_type_id, $group_content_bundle_id, $is_owner); + + if (!array_key_exists($operation, $permissions)) { return AccessResult::neutral(); } @@ -273,35 +279,10 @@ public static function userAccessGroupContentEntityOperations($operation, Entity $cacheable_metadata->addCacheContexts(['user']); } - $is_owner = $group_content_entity instanceof EntityOwnerInterface && $group_content_entity->getOwnerId() == $user->id(); - $group_content_entity_type_id = $group_content_entity->getEntityTypeId(); - $group_content_bundle_id = $group_content_entity->bundle(); - - // Generate the permissions to check. - switch ($operation) { - case 'create': - $permissions = ["create $group_content_bundle_id $group_content_entity_type_id"]; - break; - - case 'update': - $permissions = ["update any $group_content_bundle_id $group_content_entity_type_id"]; - if ($is_owner) { - $permissions[] = "update own $group_content_bundle_id $group_content_entity_type_id"; - } - break; - - case 'delete': - $permissions = ["delete any $group_content_bundle_id $group_content_entity_type_id"]; - if ($is_owner) { - $permissions[] = "delete own $group_content_bundle_id $group_content_entity_type_id"; - } - break; - } - // @todo Also deal with the use case that CRUD operations are granted to // non-members. if ($membership = Og::getUserMembership($user, $group_entity)) { - foreach ($permissions as $permission) { + foreach (array_keys($permissions[$operation]) as $permission) { if ($membership->hasPermission($permission)) { return AccessResult::allowed()->addCacheableDependency($cacheable_metadata); } diff --git a/src/PermissionManager.php b/src/PermissionManager.php index 0cc3dcc40..6b9987872 100644 --- a/src/PermissionManager.php +++ b/src/PermissionManager.php @@ -81,14 +81,14 @@ public function getPermissionList($entity_type_id, $bundle_id) { * The entity type ID for which to generate the permission list. * @param $group_content_bundle_id * The bundle ID for which to generate the permission list. + * @param bool $is_owner + * Boolean indication whether or not the permissions are being retrieved for + * a user that is the owner of the entity in question. * * @return array - * An array of permission names and descriptions. - * + * An array of permission names and descriptions, keyed by operation. */ - public function getEntityOperationPermissions($group_content_entity_type_id, $group_content_bundle_id) { - $permissions = []; - + public function getEntityOperationPermissions($group_content_entity_type_id, $group_content_bundle_id, $is_owner = TRUE) { // Check if the bundle is a group content type. if (!Og::isGroupContent($group_content_entity_type_id, $group_content_bundle_id)) { return []; @@ -105,30 +105,44 @@ public function getEntityOperationPermissions($group_content_entity_type_id, $gr // @todo This needs to support all entity operations for the given entity // type, not just the standard CRUD operations. // @see https://github.com/amitaibu/og/issues/222 - $permissions += [ - "create $group_content_bundle_id $group_content_entity_type_id" => [ - 'title' => t('Create %bundle @entity', $args), + $operation_permissions = [ + 'create' => [ + "create $group_content_bundle_id $group_content_entity_type_id" => [ + 'title' => t('Create %bundle @entity', $args), + ], ], - "update own $group_content_bundle_id $group_content_entity_type_id" => [ - 'title' => t('Edit own %bundle @entity', $args), - ], - "update any $group_content_bundle_id $group_content_entity_type_id" => [ - 'title' => t('Edit any %bundle @entity', $args), - ], - "delete own $group_content_bundle_id $group_content_entity_type_id" => [ - 'title' => t('Delete own %bundle @entity', $args), + 'update' => [ + "update any $group_content_bundle_id $group_content_entity_type_id" => [ + 'title' => t('Edit any %bundle @entity', $args), + ], ], - "delete any $group_content_bundle_id $group_content_entity_type_id" => [ - 'title' => t('Delete any %bundle @entity', $args), + 'delete' => [ + "delete any $group_content_bundle_id $group_content_entity_type_id" => [ + 'title' => t('Delete any %bundle @entity', $args), + ], ], ]; - // Add default permissions. - foreach ($permissions as $key => $value) { - $permissions[$key]['default role'] = [OgRoleInterface::ADMINISTRATOR]; + // Add the permissions for the owner of the entity if needed. + if ($is_owner) { + $operation_permissions['update']["update own $group_content_bundle_id $group_content_entity_type_id"] = [ + 'title' => t('Edit own %bundle @entity', $args), + ]; + $operation_permissions['delete']["delete own $group_content_bundle_id $group_content_entity_type_id"] = [ + 'title' => t('Delete own %bundle @entity', $args), + ]; } - return $permissions; + // Enable each permission for the administrator role by default. + foreach ($operation_permissions as $operation => $permissions) { + foreach ($permissions as $key => $permission) { + $operation_permissions[$operation][$key]['default role'] = [OgRoleInterface::ADMINISTRATOR]; + } + } + + // @todo Allow to alter the permissions. + + return $operation_permissions; } } From 764cf74519094ac5ddd1bedd8c71c720a60b7cfb Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Thu, 9 Jun 2016 17:55:35 +0200 Subject: [PATCH 04/85] Inject the permission manager. --- src/OgAccess.php | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 071864ffd..d8231b9dc 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -61,6 +61,16 @@ class OgAccess implements OgAccessInterface { */ protected $moduleHandler; + /** + * The OG permission manager. + * + * @var \Drupal\og\PermissionManager + * + * @todo This should be PermissionManagerInterface. + * @see https://github.com/amitaibu/og/issues/228 + */ + protected $permissionManager; + /** * Constructs an OgManager service. * @@ -70,11 +80,16 @@ class OgAccess implements OgAccessInterface { * The service that contains the current active user. * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler * The module handler. + * @param \Drupal\og\PermissionManager $permission_manager + * The permission manager. + * @todo This should be PermissionManagerInterface. + * @see https://github.com/amitaibu/og/issues/228 */ - public function __construct(ConfigFactoryInterface $config_factory, AccountProxyInterface $account_proxy, ModuleHandlerInterface $module_handler) { + public function __construct(ConfigFactoryInterface $config_factory, AccountProxyInterface $account_proxy, ModuleHandlerInterface $module_handler, PermissionManager $permission_manager) { $this->configFactory = $config_factory; $this->accountProxy = $account_proxy; $this->moduleHandler = $module_handler; + $this->permissionManager = $permission_manager; } /** @@ -255,12 +270,12 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt * * @see \Drupal\og\PermissionManager::getEntityOperationPermissions() */ - public static function userAccessGroupContentEntityOperations($operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL) { + public function userAccessGroupContentEntityOperations($operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL) { // Retrieve the permissions and check if our operation is supported. $group_content_entity_type_id = $group_content_entity->getEntityTypeId(); $group_content_bundle_id = $group_content_entity->bundle(); $is_owner = $group_content_entity instanceof EntityOwnerInterface && $group_content_entity->getOwnerId() == $user->id(); - $permissions = \Drupal::service('og.permission_manager')->getEntityOperationPermissions($group_content_entity_type_id, $group_content_bundle_id, $is_owner); + $permissions = $this->permissionManager->getEntityOperationPermissions($group_content_entity_type_id, $group_content_bundle_id, $is_owner); if (!array_key_exists($operation, $permissions)) { return AccessResult::neutral(); From 78fbae2ee0096adfb195a05bd39959002caa2c1b Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Thu, 9 Jun 2016 17:55:57 +0200 Subject: [PATCH 05/85] Use the injected account proxy to get information about the current user. --- src/OgAccess.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index d8231b9dc..ddd50c737 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -283,14 +283,14 @@ public function userAccessGroupContentEntityOperations($operation, EntityInterfa // Default to the current user. if (!isset($user)) { - $user = \Drupal::currentUser()->getAccount(); + $user = $this->accountProxy->getAccount(); } // From this point on, every result also depends on the user so check // whether it is the current. See https://www.drupal.org/node/2628870 $cacheable_metadata = new CacheableMetadata; $cacheable_metadata->addCacheableDependency($group_content_entity); - if ($user->id() == \Drupal::currentUser()->id()) { + if ($user->id() == $this->accountProxy->id()) { $cacheable_metadata->addCacheContexts(['user']); } From 78ee4725d9f6568a0d8953edcbefaebabe7bc3e5 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Fri, 10 Jun 2016 19:17:06 +0200 Subject: [PATCH 06/85] Inject the permission manager now that OgAccess is a service. --- og.services.yml | 2 +- tests/src/Unit/OgAccessTestBase.php | 11 ++++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/og.services.yml b/og.services.yml index 92db7870a..4f5a31be5 100644 --- a/og.services.yml +++ b/og.services.yml @@ -1,7 +1,7 @@ services: og.access: class: Drupal\og\OgAccess - arguments: ['@config.factory', '@current_user', '@module_handler'] + arguments: ['@config.factory', '@current_user', '@module_handler', '@og.permission_manager'] og.event_subscriber: class: Drupal\og\EventSubscriber\OgEventSubscriber arguments: ['@og.permission_manager'] diff --git a/tests/src/Unit/OgAccessTestBase.php b/tests/src/Unit/OgAccessTestBase.php index dfaeda126..916a03c2c 100644 --- a/tests/src/Unit/OgAccessTestBase.php +++ b/tests/src/Unit/OgAccessTestBase.php @@ -19,6 +19,7 @@ use Drupal\og\GroupManager; use Drupal\og\OgAccess; use Drupal\og\OgMembershipInterface; +use Drupal\og\PermissionManager; use Drupal\user\EntityOwnerInterface; use Prophecy\Argument; @@ -48,6 +49,13 @@ class OgAccessTestBase extends UnitTestCase { */ protected $groupManager; + /** + * The mocked permission manager. + * + * @var \Drupal\og\PermissionManager|\Prophecy\Prophecy\ObjectProphecy + */ + protected $permissionManager; + /** * The OgAccess class, this is the system under test. * @@ -108,9 +116,10 @@ public function setUp() { $account_proxy = $this->prophesize(AccountProxyInterface::class); $module_handler = $this->prophesize(ModuleHandlerInterface::class); + $this->permissionManager = $this->prophesize(PermissionManager::class); // Instantiate the system under test. - $this->ogAccess = new OgAccess($config_factory->reveal(), $account_proxy->reveal(), $module_handler->reveal()); + $this->ogAccess = new OgAccess($config_factory->reveal(), $account_proxy->reveal(), $module_handler->reveal(), $this->permissionManager->reveal()); // Set the Og::cache property values, to skip calculations. $values = []; From a4b2de2dcbbb8310800e2c4188c9ebc2fd381181 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Fri, 10 Jun 2016 19:20:55 +0200 Subject: [PATCH 07/85] Update OgAccessEntityTest now that the entity operations are also checked for access. --- tests/src/Unit/OgAccessEntityTestBase.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/src/Unit/OgAccessEntityTestBase.php b/tests/src/Unit/OgAccessEntityTestBase.php index 479598e33..904453a71 100644 --- a/tests/src/Unit/OgAccessEntityTestBase.php +++ b/tests/src/Unit/OgAccessEntityTestBase.php @@ -51,6 +51,13 @@ public function setup() { $this->entity->getEntityType()->willReturn($entity_type->reveal()); $this->entity->getEntityTypeId()->willReturn($entity_type_id); + // It is expected that a list of entity operation permissions is retrieved + // from the permission manager so that the passed in permission can be + // checked against this list. Our permissions are not in the list, so it is + // of no importance what we return here, an empty array is sufficient. + $this->permissionManager->getEntityOperationPermissions($this->entity->reveal()->getEntityTypeId(), $this->entity->reveal()->bundle(), FALSE) + ->willReturn([]); + $this->groupManager->isGroup($entity_type_id, $bundle)->willReturn(FALSE); $entity_field_manager = $this->prophesize(EntityFieldManagerInterface::class); From 15611c78f0aa7ca52e0cb38b4ea67ea303cccbd0 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Mon, 13 Jun 2016 13:58:10 +0200 Subject: [PATCH 08/85] Update getPermissionList() now that the permissions are keyed by entity operation. --- src/PermissionManager.php | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/PermissionManager.php b/src/PermissionManager.php index 6b9987872..068159179 100644 --- a/src/PermissionManager.php +++ b/src/PermissionManager.php @@ -63,15 +63,18 @@ public function __construct(GroupManager $group_manager, EntityTypeManagerInterf * @todo Provide an alter hook. */ public function getPermissionList($entity_type_id, $bundle_id) { - $permissions = []; + $permission_list = []; foreach ($this->groupManager->getGroupContentBundleIdsByGroupBundle($entity_type_id, $bundle_id) as $group_content_entity_type_id => $group_content_bundle_ids) { foreach ($group_content_bundle_ids as $group_content_bundle_id) { - $permissions += $this->getEntityOperationPermissions($group_content_entity_type_id, $group_content_bundle_id); + $operation_permissions = $this->getEntityOperationPermissions($group_content_entity_type_id, $group_content_bundle_id); + foreach ($operation_permissions as $operation => $permissions) { + $permission_list = array_merge($permission_list, $permissions); + } } } - return $permissions; + return $permission_list; } /** From 3096020e47acf1314f8d10a713534988a262173d Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Mon, 13 Jun 2016 14:11:38 +0200 Subject: [PATCH 09/85] Don't speak about CRUD, entity operations go beyond the basic CRUD operations. --- src/OgAccess.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index b541d0b85..90a1513fc 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -234,9 +234,9 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt $forbidden = AccessResult::forbidden()->addCacheTags($cache_tags); foreach ($groups as $entity_groups) { foreach ($entity_groups as $group) { - $crud_access = $this->userAccessGroupContentEntityOperations($operation, $group, $entity, $user); - if ($crud_access->isAllowed()) { - return $crud_access->addCacheTags($cache_tags); + $operation_access = $this->userAccessGroupContentEntityOperations($operation, $group, $entity, $user); + if ($operation_access->isAllowed()) { + return $operation_access->addCacheTags($cache_tags); } $user_access = $this->userAccess($group, $operation, $user); if ($user_access->isAllowed()) { @@ -268,7 +268,7 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt * @param \Drupal\Core\Entity\EntityInterface $group_entity * The group entity, to retrieve the permissions from. * @param \Drupal\Core\Entity\EntityInterface $group_content_entity - * The group content entity for which the CRUD operation access is + * The group content entity for which access to the entity operation is * requested. * @param \Drupal\Core\Session\AccountInterface $user * The user for which to check access. @@ -302,7 +302,7 @@ public function userAccessGroupContentEntityOperations($operation, EntityInterfa $cacheable_metadata->addCacheContexts(['user']); } - // @todo Also deal with the use case that CRUD operations are granted to + // @todo Also deal with the use case that entity operations are granted to // non-members. if ($membership = Og::getUserMembership($user, $group_entity)) { foreach (array_keys($permissions[$operation]) as $permission) { From 970b78c218a843c4dd54beb5d5575818b4dde502 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Mon, 13 Jun 2016 14:12:08 +0200 Subject: [PATCH 10/85] Update documentation. --- src/OgAccess.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 90a1513fc..e901dd922 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -260,8 +260,11 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt /** * Checks access for entity operations on group content entities. * - * This checks access for the default CRUD operations added by the permission - * manager. + * 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(). * * @param string $operation * The entity operation. From 9a7ef40c8c31e861d8a0e3f3b4d11032b4d047d5 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Mon, 13 Jun 2016 17:30:48 +0200 Subject: [PATCH 11/85] Put the additional metadata in the permission array. --- src/PermissionManager.php | 52 +++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 27 deletions(-) diff --git a/src/PermissionManager.php b/src/PermissionManager.php index 068159179..f1c2cb55c 100644 --- a/src/PermissionManager.php +++ b/src/PermissionManager.php @@ -63,18 +63,15 @@ public function __construct(GroupManager $group_manager, EntityTypeManagerInterf * @todo Provide an alter hook. */ public function getPermissionList($entity_type_id, $bundle_id) { - $permission_list = []; + $permissions = []; foreach ($this->groupManager->getGroupContentBundleIdsByGroupBundle($entity_type_id, $bundle_id) as $group_content_entity_type_id => $group_content_bundle_ids) { foreach ($group_content_bundle_ids as $group_content_bundle_id) { - $operation_permissions = $this->getEntityOperationPermissions($group_content_entity_type_id, $group_content_bundle_id); - foreach ($operation_permissions as $operation => $permissions) { - $permission_list = array_merge($permission_list, $permissions); - } + $permissions += $this->getEntityOperationPermissions($group_content_entity_type_id, $group_content_bundle_id); } } - return $permission_list; + return $permissions; } /** @@ -108,44 +105,45 @@ public function getEntityOperationPermissions($group_content_entity_type_id, $gr // @todo This needs to support all entity operations for the given entity // type, not just the standard CRUD operations. // @see https://github.com/amitaibu/og/issues/222 - $operation_permissions = [ - 'create' => [ - "create $group_content_bundle_id $group_content_entity_type_id" => [ - 'title' => t('Create %bundle @entity', $args), - ], + $permissions = [ + "create $group_content_bundle_id $group_content_entity_type_id" => [ + 'title' => t('Create %bundle @entity', $args), + 'operation' => 'create', ], - 'update' => [ - "update any $group_content_bundle_id $group_content_entity_type_id" => [ - 'title' => t('Edit any %bundle @entity', $args), - ], + "update any $group_content_bundle_id $group_content_entity_type_id" => [ + 'title' => t('Edit any %bundle @entity', $args), + 'operation' => 'update', ], - 'delete' => [ - "delete any $group_content_bundle_id $group_content_entity_type_id" => [ - 'title' => t('Delete any %bundle @entity', $args), - ], + "delete any $group_content_bundle_id $group_content_entity_type_id" => [ + 'title' => t('Delete any %bundle @entity', $args), + 'operation' => 'delete', ], ]; // Add the permissions for the owner of the entity if needed. if ($is_owner) { - $operation_permissions['update']["update own $group_content_bundle_id $group_content_entity_type_id"] = [ + $permissions["update own $group_content_bundle_id $group_content_entity_type_id"] = [ 'title' => t('Edit own %bundle @entity', $args), + 'operation' => 'update', + 'ownership' => 'own', ]; - $operation_permissions['delete']["delete own $group_content_bundle_id $group_content_entity_type_id"] = [ + $permissions["delete own $group_content_bundle_id $group_content_entity_type_id"] = [ 'title' => t('Delete own %bundle @entity', $args), + 'operation' => 'delete', + 'ownership' => 'own', ]; } - // Enable each permission for the administrator role by default. - foreach ($operation_permissions as $operation => $permissions) { - foreach ($permissions as $key => $permission) { - $operation_permissions[$operation][$key]['default role'] = [OgRoleInterface::ADMINISTRATOR]; - } + foreach ($permissions as &$permission) { + $permission['entity type'] = $group_content_entity_type_id; + $permission['bundle'] = $group_content_bundle_id; + // Enable each permission for the administrator role by default. + $permission['default role'] = [OgRoleInterface::ADMINISTRATOR]; } // @todo Allow to alter the permissions. - return $operation_permissions; + return $permissions; } } From aa14a438ed0e151a55bab2c097660c65e835bcc3 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Mon, 13 Jun 2016 17:31:43 +0200 Subject: [PATCH 12/85] If we handle all of this through the PermissionEvent we don't need to be able to alter it any more. It can already be altered through the event. --- src/PermissionManager.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/PermissionManager.php b/src/PermissionManager.php index f1c2cb55c..5576b10a2 100644 --- a/src/PermissionManager.php +++ b/src/PermissionManager.php @@ -59,8 +59,6 @@ public function __construct(GroupManager $group_manager, EntityTypeManagerInterf * * @return array * The list of permissions. - * - * @todo Provide an alter hook. */ public function getPermissionList($entity_type_id, $bundle_id) { $permissions = []; @@ -141,8 +139,6 @@ public function getEntityOperationPermissions($group_content_entity_type_id, $gr $permission['default role'] = [OgRoleInterface::ADMINISTRATOR]; } - // @todo Allow to alter the permissions. - return $permissions; } From c79fd2ecd1c082847124c456a0ea81db182636b0 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Mon, 13 Jun 2016 18:28:08 +0200 Subject: [PATCH 13/85] Check the presence of the group content entity type and bundle for operations. --- src/Event/PermissionEvent.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/Event/PermissionEvent.php b/src/Event/PermissionEvent.php index 85c575638..3dd0b54a8 100644 --- a/src/Event/PermissionEvent.php +++ b/src/Event/PermissionEvent.php @@ -75,6 +75,13 @@ public function setPermission($name, array $permission) { if (empty($permission['title'])) { throw new \InvalidArgumentException('The permission title is required.'); } + if (!empty($permission['operation']) && (empty($permission['entity type']) || empty($permission['bundle']))) { + throw new \InvalidArgumentException('When an operation is provided, the entity type and bundle to which this operation applies should also be provided.'); + } + // Default the ownership to 'any' for operations. + if (!empty($permission['operation']) && empty($permission['ownership'])) { + $permission['ownership'] = 'any'; + } $this->permissions[$name] = $permission; } From b886f85c155dc7c59ce1b55d19241d5e802cab2d Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Mon, 13 Jun 2016 18:28:20 +0200 Subject: [PATCH 14/85] WIP documentation. --- src/Event/PermissionEventInterface.php | 7 +++++++ src/PermissionManager.php | 2 ++ 2 files changed, 9 insertions(+) diff --git a/src/Event/PermissionEventInterface.php b/src/Event/PermissionEventInterface.php index e808186d9..db0d8a251 100644 --- a/src/Event/PermissionEventInterface.php +++ b/src/Event/PermissionEventInterface.php @@ -40,6 +40,13 @@ interface PermissionEventInterface extends \ArrayAccess, \IteratorAggregate { * permission that requires elevated privileges. Use this for permissions * that are mainly intended for the group administrator or similar roles. * Defaults to FALSE. + * - 'operation': Optional operation that is applicable to this permission + * in case this is about granting access to an entity operation. Example: + * if the permission was 'create any article content', the operation would + * be 'create'. + * - 'entity type': @todo + * - 'bundle': @todo + * - 'ownership': @todo * * @throws \InvalidArgumentException * Thrown when the permission with the given name does not exist. diff --git a/src/PermissionManager.php b/src/PermissionManager.php index 5576b10a2..c85c146dc 100644 --- a/src/PermissionManager.php +++ b/src/PermissionManager.php @@ -133,6 +133,8 @@ public function getEntityOperationPermissions($group_content_entity_type_id, $gr } foreach ($permissions as &$permission) { + // Set the entity type and bundle IDs of the group content to which this + // operation applies. $permission['entity type'] = $group_content_entity_type_id; $permission['bundle'] = $group_content_bundle_id; // Enable each permission for the administrator role by default. From 291aa23f6167f39c53334b7aa2c544d973d9c952 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 14 Jun 2016 11:30:34 +0200 Subject: [PATCH 15/85] Deliberately introduce a failure as a reminder. --- src/Event/PermissionEvent.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Event/PermissionEvent.php b/src/Event/PermissionEvent.php index 3dd0b54a8..8facc787b 100644 --- a/src/Event/PermissionEvent.php +++ b/src/Event/PermissionEvent.php @@ -128,6 +128,7 @@ public function getBundleId() { * {@inheritdoc} */ public function filterByDefaultRole($role_name) { + throw new \Exception('Move this to PermissionManager'); return array_filter($this->permissions, function ($permission) use ($role_name) { return !empty($permission['default roles']) && in_array($role_name, $permission['default roles']); }); From 47361247ca8131ce5188ff303f1a1a24707524ca Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 14 Jun 2016 11:30:50 +0200 Subject: [PATCH 16/85] Inject the permission manager. We'll need it. --- src/GroupManager.php | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/GroupManager.php b/src/GroupManager.php index c707a6c7e..7113cfee0 100644 --- a/src/GroupManager.php +++ b/src/GroupManager.php @@ -77,6 +77,15 @@ class GroupManager { */ protected $state; + /** + * The OG permission manager. + * + * @var \Drupal\og\PermissionManager + * + * @todo This should be PermissionManagerInterface. + */ + protected $permissionManager; + /** * A map of entity types and bundles. * @@ -122,13 +131,16 @@ class GroupManager { * The event dispatcher. * @param \Drupal\Core\State\StateInterface $state * The state service. + * @param \Drupal\og\PermissionManager $permission_manager + * The OG permission manager. */ - public function __construct(ConfigFactoryInterface $config_factory, EntityTypeManagerInterface $entity_type_manager, EntityTypeBundleInfoInterface $entity_type_bundle_info, EventDispatcherInterface $event_dispatcher, StateInterface $state) { + public function __construct(ConfigFactoryInterface $config_factory, EntityTypeManagerInterface $entity_type_manager, EntityTypeBundleInfoInterface $entity_type_bundle_info, EventDispatcherInterface $event_dispatcher, StateInterface $state, PermissionManager $permission_manager) { $this->configFactory = $config_factory; $this->ogRoleStorage = $entity_type_manager->getStorage('og_role'); $this->entityTypeBundleInfo = $entity_type_bundle_info; $this->eventDispatcher = $event_dispatcher; $this->state = $state; + $this->permissionManager = $permission_manager; } /** From e35e3be53c0521721df800c63d5f0bb8892da9b8 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 14 Jun 2016 11:30:58 +0200 Subject: [PATCH 17/85] Update documentation. --- src/GroupManager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/GroupManager.php b/src/GroupManager.php index 7113cfee0..03d41c036 100644 --- a/src/GroupManager.php +++ b/src/GroupManager.php @@ -233,7 +233,7 @@ public function getGroupContentBundleIdsByGroupBundle($group_entity_type_id, $gr } /** - * Sets an entity type instance as being an OG group. + * Declares a bundle of an entity type as being an OG group. * * @param string $entity_type_id * The entity type ID of the bundle to declare as being a group. From 46f9cb5b767dd22195e6d2ec94973aeb5e00a5b3 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 22 Jun 2016 07:37:21 +0200 Subject: [PATCH 18/85] WIP --- og.services.yml | 2 +- src/OgAccess.php | 32 +++++++++++++++++++++----------- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/og.services.yml b/og.services.yml index 9014e5b59..e3050961d 100644 --- a/og.services.yml +++ b/og.services.yml @@ -1,7 +1,7 @@ services: og.access: class: Drupal\og\OgAccess - arguments: ['@config.factory', '@current_user', '@module_handler', '@og.permission_manager'] + arguments: ['@config.factory', '@current_user', '@module_handler', '@og.group_manager', '@og.permission_manager'] og.event_subscriber: class: Drupal\og\EventSubscriber\OgEventSubscriber arguments: ['@og.permission_manager', '@entity_type.manager', '@entity_type.bundle.info'] diff --git a/src/OgAccess.php b/src/OgAccess.php index e901dd922..a535cb09a 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -62,12 +62,18 @@ class OgAccess implements OgAccessInterface { protected $moduleHandler; /** - * The OG permission manager. + * The group manager. + * + * @var \Drupal\og\GroupManager * - * @var \Drupal\og\PermissionManager + * @todo This should be GroupManagerInterface. + */ + protected $groupManager; + + /** + * The OG permission manager. * - * @todo This should be PermissionManagerInterface. - * @see https://github.com/amitaibu/og/issues/228 + * @var \Drupal\og\PermissionManagerInterface */ protected $permissionManager; @@ -80,15 +86,16 @@ class OgAccess implements OgAccessInterface { * The service that contains the current active user. * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler * The module handler. - * @param \Drupal\og\PermissionManager $permission_manager + * @param \Drupal\og\GroupManager + * The group manager. + * @param \Drupal\og\PermissionManagerInterface $permission_manager * The permission manager. - * @todo This should be PermissionManagerInterface. - * @see https://github.com/amitaibu/og/issues/228 */ - public function __construct(ConfigFactoryInterface $config_factory, AccountProxyInterface $account_proxy, ModuleHandlerInterface $module_handler, PermissionManager $permission_manager) { + public function __construct(ConfigFactoryInterface $config_factory, AccountProxyInterface $account_proxy, ModuleHandlerInterface $module_handler, GroupManager $group_manager, PermissionManagerInterface $permission_manager) { $this->configFactory = $config_factory; $this->accountProxy = $account_proxy; $this->moduleHandler = $module_handler; + $this->groupManager = $group_manager; $this->permissionManager = $permission_manager; } @@ -283,10 +290,13 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt */ public function userAccessGroupContentEntityOperations($operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL) { // Retrieve the permissions and check if our operation is supported. - $group_content_entity_type_id = $group_content_entity->getEntityTypeId(); - $group_content_bundle_id = $group_content_entity->bundle(); + $group_entity_type_id = $group_entity->getEntityTypeId(); + $group_bundle_id = $group_entity->bundle(); + $group_content_bundle_ids = [$group_content_entity->getEntityTypeId() => [$group_content_entity->bundle()]]; + $is_owner = $group_content_entity instanceof EntityOwnerInterface && $group_content_entity->getOwnerId() == $user->id(); - $permissions = $this->permissionManager->getEntityOperationPermissions($group_content_entity_type_id, $group_content_bundle_id, $is_owner); + + $permissions = $this->permissionManager->getDefaultEntityOperationPermissions($group_entity_type_id, $group_bundle_id, $group_content_bundle_ids); if (!array_key_exists($operation, $permissions)) { return AccessResult::neutral(); From c97e381255150d3946ce63276e21644a27a366a6 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 22 Jun 2016 12:05:03 +0200 Subject: [PATCH 19/85] Fix typo. --- og.services.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/og.services.yml b/og.services.yml index e3050961d..c94fa0d45 100644 --- a/og.services.yml +++ b/og.services.yml @@ -1,7 +1,7 @@ services: og.access: class: Drupal\og\OgAccess - arguments: ['@config.factory', '@current_user', '@module_handler', '@og.group_manager', '@og.permission_manager'] + arguments: ['@config.factory', '@current_user', '@module_handler', '@og.group.manager', '@og.permission_manager'] og.event_subscriber: class: Drupal\og\EventSubscriber\OgEventSubscriber arguments: ['@og.permission_manager', '@entity_type.manager', '@entity_type.bundle.info'] From 7bbca54577fb0f8d90b60410020cb92805dc20c7 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 22 Jun 2016 12:05:29 +0200 Subject: [PATCH 20/85] Fix matching of OgRole ID, it failed because 'non-member' contains a dash. --- src/Entity/OgRole.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Entity/OgRole.php b/src/Entity/OgRole.php index b03c50947..23390e881 100644 --- a/src/Entity/OgRole.php +++ b/src/Entity/OgRole.php @@ -205,8 +205,8 @@ public function save() { // The ID of a new OgRole has to consist of the entity type ID, bundle ID // and role name, separated by dashes. if ($this->isNew() && !empty($this->id())) { - list($entity_type_id, $bundle_id, $name) = explode('-', $this->id()); - if ($entity_type_id !== $this->getGroupType() || $bundle_id !== $this->getGroupBundle() || $name !== $this->getName()) { + $pattern = preg_quote("{$this->getGroupType()}-{$this->getGroupBundle()}-{$this->getName()}"); + if (!preg_match("/$pattern/", $this->id())) { throw new ConfigValueException('The ID should consist of the group entity type ID, group bundle ID and role name, separated by dashes.'); } } From 2362ffe6f2d4616d4747b0ea204b952215dd61e4 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 22 Jun 2016 12:05:47 +0200 Subject: [PATCH 21/85] Musing about future improvements. --- src/OgAccess.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/OgAccess.php b/src/OgAccess.php index a535cb09a..3cca60453 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -309,6 +309,7 @@ public function userAccessGroupContentEntityOperations($operation, EntityInterfa // From this point on, every result also depends on the user so check // whether it is the current. See https://www.drupal.org/node/2628870 + // @todo Should we make a cache context for OgRole entities? $cacheable_metadata = new CacheableMetadata; $cacheable_metadata->addCacheableDependency($group_content_entity); if ($user->id() == $this->accountProxy->id()) { From fba96edb32626550d28a4e4f97e2e3ffd81b38ca Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 22 Jun 2016 12:11:34 +0200 Subject: [PATCH 22/85] Inject the GroupManager. --- src/OgAccess.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 3cca60453..14155118c 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -88,10 +88,10 @@ class OgAccess implements OgAccessInterface { * The module handler. * @param \Drupal\og\GroupManager * The group manager. - * @param \Drupal\og\PermissionManagerInterface $permission_manager + * @param \Drupal\og\PermissionManager $permission_manager * The permission manager. */ - public function __construct(ConfigFactoryInterface $config_factory, AccountProxyInterface $account_proxy, ModuleHandlerInterface $module_handler, GroupManager $group_manager, PermissionManagerInterface $permission_manager) { + public function __construct(ConfigFactoryInterface $config_factory, AccountProxyInterface $account_proxy, ModuleHandlerInterface $module_handler, GroupManager $group_manager, PermissionManager $permission_manager) { $this->configFactory = $config_factory; $this->accountProxy = $account_proxy; $this->moduleHandler = $module_handler; From 6c515934bf34902c205f182143e82d53d5e9536c Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 22 Jun 2016 12:12:12 +0200 Subject: [PATCH 23/85] We have PermissionManagerInterface now. --- src/OgAccess.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 14155118c..3cca60453 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -88,10 +88,10 @@ class OgAccess implements OgAccessInterface { * The module handler. * @param \Drupal\og\GroupManager * The group manager. - * @param \Drupal\og\PermissionManager $permission_manager + * @param \Drupal\og\PermissionManagerInterface $permission_manager * The permission manager. */ - public function __construct(ConfigFactoryInterface $config_factory, AccountProxyInterface $account_proxy, ModuleHandlerInterface $module_handler, GroupManager $group_manager, PermissionManager $permission_manager) { + public function __construct(ConfigFactoryInterface $config_factory, AccountProxyInterface $account_proxy, ModuleHandlerInterface $module_handler, GroupManager $group_manager, PermissionManagerInterface $permission_manager) { $this->configFactory = $config_factory; $this->accountProxy = $account_proxy; $this->moduleHandler = $module_handler; From 7c238c46d388ec716cd1823499357ea0e6bf4927 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 22 Jun 2016 12:43:39 +0200 Subject: [PATCH 24/85] Temporarily remove early abort. --- src/OgAccess.php | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 3cca60453..b533eca79 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -207,9 +207,13 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt $result = AccessResult::neutral(); // Entity isn't saved yet. - if ($entity->isNew()) { - return $result->addCacheableDependency($entity); - } + // @todo This messes with checking access to create group content, since in + // this case the content is not saved yet. This will now always return + // neutral, but it should check if the user has permission to create a new + // entity.. +// if ($entity->isNew()) { +// return $result->addCacheableDependency($entity); +// } $entity_type = $entity->getEntityType(); $entity_type_id = $entity_type->id(); From ff655f9ee9ce65950b15b7d82bc8797ecf1f99dc Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 22 Jun 2016 12:44:29 +0200 Subject: [PATCH 25/85] Pass on the group content bundle IDs. --- src/PermissionManager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PermissionManager.php b/src/PermissionManager.php index 5300e55a3..3623ca1b3 100644 --- a/src/PermissionManager.php +++ b/src/PermissionManager.php @@ -32,7 +32,7 @@ public function __construct(EventDispatcherInterface $event_dispatcher) { * {@inheritdoc} */ public function getDefaultPermissions($group_entity_type_id, $group_bundle_id, array $group_content_bundle_ids, $role_name = NULL) { - $event = new PermissionEvent($group_entity_type_id, $group_bundle_id, []); + $event = new PermissionEvent($group_entity_type_id, $group_bundle_id, $group_content_bundle_ids); $this->eventDispatcher->dispatch(PermissionEventInterface::EVENT_NAME, $event); return $event->getPermissions(); } From 7c6a46c3a4bc73eda4424ba82478e1e6d7fdf2eb Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 22 Jun 2016 13:20:28 +0200 Subject: [PATCH 26/85] Check if the user has the entity operation permission for the group membership. --- src/OgAccess.php | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index b533eca79..c372b2bc6 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -298,21 +298,22 @@ public function userAccessGroupContentEntityOperations($operation, EntityInterfa $group_bundle_id = $group_entity->bundle(); $group_content_bundle_ids = [$group_content_entity->getEntityTypeId() => [$group_content_entity->bundle()]]; - $is_owner = $group_content_entity instanceof EntityOwnerInterface && $group_content_entity->getOwnerId() == $user->id(); - - $permissions = $this->permissionManager->getDefaultEntityOperationPermissions($group_entity_type_id, $group_bundle_id, $group_content_bundle_ids); - - if (!array_key_exists($operation, $permissions)) { - return AccessResult::neutral(); - } - // Default to the current user. if (!isset($user)) { $user = $this->accountProxy->getAccount(); } - // From this point on, every result also depends on the user so check - // whether it is the current. See https://www.drupal.org/node/2628870 + + $is_owner = $group_content_entity instanceof EntityOwnerInterface && $group_content_entity->getOwnerId() == $user->id(); + + $permissions = $this->permissionManager->getDefaultEntityOperationPermissions($group_entity_type_id, $group_bundle_id, $group_content_bundle_ids); + + // Filter the permissions by operation and ownership. + $ownerships = $is_owner ? ['any', 'own'] : ['any']; + $permissions = array_filter($permissions, function (GroupContentOperationPermission $permission) use ($operation, $ownerships) { + return $permission->getOperation() === $operation && in_array($permission->getOwnership(), $ownerships); + }); + // @todo Should we make a cache context for OgRole entities? $cacheable_metadata = new CacheableMetadata; $cacheable_metadata->addCacheableDependency($group_content_entity); @@ -322,11 +323,10 @@ public function userAccessGroupContentEntityOperations($operation, EntityInterfa // @todo Also deal with the use case that entity operations are granted to // non-members. - if ($membership = Og::getUserMembership($user, $group_entity)) { - foreach (array_keys($permissions[$operation]) as $permission) { - if ($membership->hasPermission($permission)) { - return AccessResult::allowed()->addCacheableDependency($cacheable_metadata); - } + $membership = Og::getMembership($user, $group_entity); + foreach ($permissions as $permission) { + if ($membership->hasPermission($permission->getName())) { + return AccessResult::allowed()->addCacheableDependency($cacheable_metadata); } } From 47b2f0c3881274a4c6c2d0523e991fc847a4e235 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 28 Jun 2016 21:48:02 +0200 Subject: [PATCH 27/85] Update documentation. --- src/OgAccess.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index c372b2bc6..6eb6d7183 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -245,10 +245,15 @@ 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->userAccessGroupContentEntityOperations($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); if ($user_access->isAllowed()) { return $user_access->addCacheTags($cache_tags); @@ -303,7 +308,7 @@ public function userAccessGroupContentEntityOperations($operation, EntityInterfa $user = $this->accountProxy->getAccount(); } - + // Check if the user owns the entity which is being operated on. $is_owner = $group_content_entity instanceof EntityOwnerInterface && $group_content_entity->getOwnerId() == $user->id(); $permissions = $this->permissionManager->getDefaultEntityOperationPermissions($group_entity_type_id, $group_bundle_id, $group_content_bundle_ids); From 6d666deb9ca2a6601884fd796edbac3fbdfd81c8 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 28 Jun 2016 21:48:41 +0200 Subject: [PATCH 28/85] Add support for retrieving permissions from non-members. --- src/Og.php | 22 +++++++++++++++++++--- src/OgAccess.php | 2 -- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/Og.php b/src/Og.php index 68680506c..fe574e005 100644 --- a/src/Og.php +++ b/src/Og.php @@ -15,6 +15,7 @@ use Drupal\Core\Session\AccountInterface; use Drupal\field\Entity\FieldConfig; use Drupal\field\Entity\FieldStorageConfig; +use Drupal\og\Entity\OgMembership; use Drupal\og\Plugin\EntityReferenceSelection\OgSelection; /** @@ -267,9 +268,10 @@ public static function getMemberships(AccountInterface $user, array $states = [O * @param string $field_name * (optional) The field name associated with the group. * - * @return \Drupal\og\Entity\OgMembership|NULL - * The OgMembership entity, or NULL if the user is not a member of the - * group. + * @return \Drupal\og\Entity\OgMembership + * The OgMembership entity. If the user is not a member of the group, an + * OgMembership entity will be returned in which the user has the + * 'non-member' role. */ public static function getMembership(AccountInterface $user, EntityInterface $group, array $states = [OgMembershipInterface::STATE_ACTIVE], $field_name = NULL) { foreach (static::getMemberships($user, $states, $field_name) as $membership) { @@ -277,6 +279,20 @@ public static function getMembership(AccountInterface $user, EntityInterface $gr return $membership; } } + + // The user is not a member, return an OgMembership entity in which the user + // has the 'non-member' role. + $role_id = implode('-', [ + $group->getEntityTypeId(), + $group->bundle(), + OgRoleInterface::ANONYMOUS, + ]); + return OgMembership::create(['type' => OgMembershipInterface::TYPE_DEFAULT]) + ->setUser($user) + ->setEntityId($group->id()) + ->setGroupEntityType($group->getEntityTypeId()) + ->setState(OgMembershipInterface::STATE_PENDING) + ->setRoles([$role_id]); } /** diff --git a/src/OgAccess.php b/src/OgAccess.php index 6eb6d7183..b716799de 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -326,8 +326,6 @@ public function userAccessGroupContentEntityOperations($operation, EntityInterfa $cacheable_metadata->addCacheContexts(['user']); } - // @todo Also deal with the use case that entity operations are granted to - // non-members. $membership = Og::getMembership($user, $group_entity); foreach ($permissions as $permission) { if ($membership->hasPermission($permission->getName())) { From 4f1b190d54dd1b704834666986365afde9d0f4f8 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 28 Jun 2016 21:49:06 +0200 Subject: [PATCH 29/85] Small restructuring to make the code more readable. --- src/OgAccess.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index b716799de..4a4f5e490 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -298,11 +298,6 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt * @see \Drupal\og\PermissionManager::getEntityOperationPermissions() */ public function userAccessGroupContentEntityOperations($operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL) { - // Retrieve the permissions and check if our operation is supported. - $group_entity_type_id = $group_entity->getEntityTypeId(); - $group_bundle_id = $group_entity->bundle(); - $group_content_bundle_ids = [$group_content_entity->getEntityTypeId() => [$group_content_entity->bundle()]]; - // Default to the current user. if (!isset($user)) { $user = $this->accountProxy->getAccount(); @@ -311,6 +306,11 @@ public function userAccessGroupContentEntityOperations($operation, EntityInterfa // Check if the user owns the entity which is being operated on. $is_owner = $group_content_entity instanceof EntityOwnerInterface && $group_content_entity->getOwnerId() == $user->id(); + // Retrieve the permissions and check if our operation is supported. + $group_entity_type_id = $group_entity->getEntityTypeId(); + $group_bundle_id = $group_entity->bundle(); + $group_content_bundle_ids = [$group_content_entity->getEntityTypeId() => [$group_content_entity->bundle()]]; + $permissions = $this->permissionManager->getDefaultEntityOperationPermissions($group_entity_type_id, $group_bundle_id, $group_content_bundle_ids); // Filter the permissions by operation and ownership. From 517224bd7d5f126a3d43cbf0a1832ac9220072e2 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 29 Jun 2016 17:51:03 +0200 Subject: [PATCH 30/85] More test greenage. --- tests/src/Unit/OgAccessEntityTestBase.php | 4 ++-- tests/src/Unit/OgAccessTestBase.php | 17 +++++++++++++++-- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/tests/src/Unit/OgAccessEntityTestBase.php b/tests/src/Unit/OgAccessEntityTestBase.php index 8a2173128..d0edea02b 100644 --- a/tests/src/Unit/OgAccessEntityTestBase.php +++ b/tests/src/Unit/OgAccessEntityTestBase.php @@ -15,7 +15,6 @@ use Drupal\Core\Entity\FieldableEntityInterface; use Drupal\Core\Field\FieldDefinitionInterface; use Drupal\Core\Field\FieldStorageDefinitionInterface; -use Drupal\og\OgAccessInterface; use Drupal\og\OgGroupAudienceHelper; use Prophecy\Argument; @@ -52,12 +51,13 @@ public function setup() { $this->groupContentEntity->isNew()->willReturn(FALSE); $this->groupContentEntity->getEntityType()->willReturn($entity_type->reveal()); $this->groupContentEntity->getEntityTypeId()->willReturn($entity_type_id); + $this->addCache($this->groupContentEntity); // It is expected that a list of entity operation permissions is retrieved // from the permission manager so that the passed in permission can be // checked against this list. Our permissions are not in the list, so it is // of no importance what we return here, an empty array is sufficient. - $this->permissionManager->getEntityOperationPermissions($this->entity->reveal()->getEntityTypeId(), $this->entity->reveal()->bundle(), FALSE) + $this->permissionManager->getDefaultEntityOperationPermissions($this->entityTypeId, $this->bundle, [$entity_type_id => [$bundle]]) ->willReturn([]); // The group manager is expected to declare that this is not a group. diff --git a/tests/src/Unit/OgAccessTestBase.php b/tests/src/Unit/OgAccessTestBase.php index d26a4edeb..9c12d5972 100644 --- a/tests/src/Unit/OgAccessTestBase.php +++ b/tests/src/Unit/OgAccessTestBase.php @@ -7,6 +7,7 @@ use Drupal\Core\Config\ConfigFactoryInterface; use Drupal\Core\DependencyInjection\ContainerBuilder; use Drupal\Core\Entity\EntityInterface; +use Drupal\Core\Entity\EntityTypeInterface; use Drupal\Core\Extension\ModuleHandlerInterface; use Drupal\Core\Session\AccountInterface; use Drupal\Core\Session\AccountProxyInterface; @@ -37,6 +38,13 @@ class OgAccessTestBase extends UnitTestCase { */ protected $user; + /** + * The ID of the test group. + * + * @var string + */ + protected $groupId; + /** * The entity type ID of the test group. * @@ -83,6 +91,7 @@ class OgAccessTestBase extends UnitTestCase { * {@inheritdoc} */ public function setUp() { + $this->groupId = $this->randomMachineName(); $this->entityTypeId = $this->randomMachineName(); $this->bundle = $this->randomMachineName(); @@ -137,7 +146,7 @@ public function setUp() { $this->permissionManager = $this->prophesize(PermissionManager::class); // Instantiate the system under test. - $this->ogAccess = new OgAccess($config_factory->reveal(), $account_proxy->reveal(), $module_handler->reveal(), $this->permissionManager->reveal()); + $this->ogAccess = new OgAccess($config_factory->reveal(), $account_proxy->reveal(), $module_handler->reveal(), $this->groupManager->reveal(), $this->permissionManager->reveal()); // Set the Og::cache property values, to skip calculations. $values = []; @@ -201,15 +210,19 @@ public function setUp() { * The test group. */ protected function groupEntity($is_owner = FALSE) { + $entity_type = $this->prophesize(EntityTypeInterface::class); + $entity_type->id()->willReturn($this->entityTypeId); + $group_entity = $this->prophesize(EntityInterface::class); if ($is_owner) { $group_entity->willImplement(EntityOwnerInterface::class); // Our test user is hardcoded to have UID 2. $group_entity->getOwnerId()->willReturn(2); } + $group_entity->getEntityType()->willReturn($entity_type); $group_entity->getEntityTypeId()->willReturn($this->entityTypeId); $group_entity->bundle()->willReturn($this->bundle); - $group_entity->id()->willReturn($this->randomMachineName()); + $group_entity->id()->willReturn($this->groupId); return $this->addCache($group_entity); } From f1cdff3875e0652346220c0940714212b9552101 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 29 Jun 2016 17:51:57 +0200 Subject: [PATCH 31/85] Access the storage handler directly. This will be easier to refactor once we turn Og into a service, and it saves us from having to mock the deprecated EntityManager. --- src/Og.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Og.php b/src/Og.php index fe574e005..faa1e4add 100644 --- a/src/Og.php +++ b/src/Og.php @@ -287,7 +287,8 @@ public static function getMembership(AccountInterface $user, EntityInterface $gr $group->bundle(), OgRoleInterface::ANONYMOUS, ]); - return OgMembership::create(['type' => OgMembershipInterface::TYPE_DEFAULT]) + $storage = \Drupal::entityTypeManager()->getStorage('og_membership'); + return $storage->create(['type' => OgMembershipInterface::TYPE_DEFAULT]) ->setUser($user) ->setEntityId($group->id()) ->setGroupEntityType($group->getEntityTypeId()) From bf8c41c750fa3cf3524ce9e6c19bbcb94ab1a815 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 29 Jun 2016 17:54:52 +0200 Subject: [PATCH 32/85] Move the code to always return Neutral. It's unclear to me why it is necessary that unsaved groups return neutral for every possible operation, but as this use case has a dedicated test it appears to be important. --- src/OgAccess.php | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 4a4f5e490..b3d2f844c 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -206,20 +206,16 @@ public function userAccess(EntityInterface $group, $operation, AccountInterface public function userAccessEntity($operation, EntityInterface $entity, AccountInterface $user = NULL) { $result = AccessResult::neutral(); - // Entity isn't saved yet. - // @todo This messes with checking access to create group content, since in - // this case the content is not saved yet. This will now always return - // neutral, but it should check if the user has permission to create a new - // entity.. -// if ($entity->isNew()) { -// return $result->addCacheableDependency($entity); -// } - $entity_type = $entity->getEntityType(); $entity_type_id = $entity_type->id(); $bundle = $entity->bundle(); if (Og::isGroup($entity_type_id, $bundle)) { + // Entity isn't saved yet. + if ($entity->isNew()) { + return $result->addCacheableDependency($entity); + } + $user_access = $this->userAccess($entity, $operation, $user); if ($user_access->isAllowed()) { return $user_access; From d600774ccb9bdee5ae2e89179b8979620f3dc6f0 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 29 Jun 2016 17:55:48 +0200 Subject: [PATCH 33/85] Clarify reminder. --- src/OgAccess.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index b3d2f844c..9d0d2f961 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -315,7 +315,8 @@ public function userAccessGroupContentEntityOperations($operation, EntityInterfa return $permission->getOperation() === $operation && in_array($permission->getOwnership(), $ownerships); }); - // @todo Should we make a cache context for OgRole entities? + // @todo This doesn't really vary by user but by the user's role inside of + // the group. Shall we make a cache context for OgRole entities? $cacheable_metadata = new CacheableMetadata; $cacheable_metadata->addCacheableDependency($group_content_entity); if ($user->id() == $this->accountProxy->id()) { From 60a2aa905451e7987c8c1b7e1f2705f289c23a8a Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 29 Jun 2016 17:56:13 +0200 Subject: [PATCH 34/85] Small optimization pointed out by the unit test coverage. --- src/OgAccess.php | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 9d0d2f961..f17830a2d 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -323,10 +323,12 @@ public function userAccessGroupContentEntityOperations($operation, EntityInterfa $cacheable_metadata->addCacheContexts(['user']); } - $membership = Og::getMembership($user, $group_entity); - foreach ($permissions as $permission) { - if ($membership->hasPermission($permission->getName())) { - return AccessResult::allowed()->addCacheableDependency($cacheable_metadata); + if ($permissions) { + $membership = Og::getMembership($user, $group_entity); + foreach ($permissions as $permission) { + if ($membership->hasPermission($permission->getName())) { + return AccessResult::allowed()->addCacheableDependency($cacheable_metadata); + } } } From 4b0650b7909c8ca421a9b479cf2b86f10df8fb44 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 29 Jun 2016 18:08:56 +0200 Subject: [PATCH 35/85] Actually use the GroupManager, it has been injected for a reason. --- src/OgAccess.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index f17830a2d..79afa30d1 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -110,7 +110,7 @@ public function userAccess(EntityInterface $group, $operation, AccountInterface $config = $this->configFactory->get('og.settings'); $cacheable_metadata = (new CacheableMetadata) ->addCacheableDependency($config); - if (!Og::isGroup($group_type_id, $bundle)) { + if (!$this->groupManager->isGroup($group_type_id, $bundle)) { // Not a group. return AccessResult::neutral()->addCacheableDependency($cacheable_metadata); } @@ -210,7 +210,7 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt $entity_type_id = $entity_type->id(); $bundle = $entity->bundle(); - if (Og::isGroup($entity_type_id, $bundle)) { + if ($this->groupManager->isGroup($entity_type_id, $bundle)) { // Entity isn't saved yet. if ($entity->isNew()) { return $result->addCacheableDependency($entity); From 2d210420d822ceb4bb90a00bece180df3ebc3798 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Thu, 30 Jun 2016 12:26:14 +0200 Subject: [PATCH 36/85] Correct documentation, this was never used as group content. --- tests/src/Kernel/Access/OgEntityAccessTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/src/Kernel/Access/OgEntityAccessTest.php b/tests/src/Kernel/Access/OgEntityAccessTest.php index 52417a9f1..b40b0e911 100644 --- a/tests/src/Kernel/Access/OgEntityAccessTest.php +++ b/tests/src/Kernel/Access/OgEntityAccessTest.php @@ -126,7 +126,7 @@ protected function setUp() { $this->adminUser->save(); - // Define the group content as group. + // Declare the test entity as being a group. Og::groupManager()->addGroup('entity_test', $this->groupBundle); // Create a group and associate with user 1. From d55ba51c371257cb852278fcaef5d59e5249afee Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Thu, 30 Jun 2016 12:26:33 +0200 Subject: [PATCH 37/85] Remove unused use statement. --- tests/src/Kernel/Access/OgEntityAccessTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/src/Kernel/Access/OgEntityAccessTest.php b/tests/src/Kernel/Access/OgEntityAccessTest.php index b40b0e911..bca1a599f 100644 --- a/tests/src/Kernel/Access/OgEntityAccessTest.php +++ b/tests/src/Kernel/Access/OgEntityAccessTest.php @@ -8,9 +8,9 @@ use Drupal\og\Entity\OgMembership; use Drupal\og\Entity\OgRole; use Drupal\og\Og; -use Drupal\og\OgAccess; use Drupal\og\OgMembershipInterface; use Drupal\user\Entity\User; + /** * Test permission inside a group. * From 635227d223c45b0f921d951353c375ae2892c864 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Thu, 30 Jun 2016 14:29:30 +0200 Subject: [PATCH 38/85] Start working on the test. --- .../OgGroupContentOperationAccessTest.php | 255 ++++++++++++++++++ 1 file changed, 255 insertions(+) create mode 100644 tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php diff --git a/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php b/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php new file mode 100644 index 000000000..bd52e6e1e --- /dev/null +++ b/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php @@ -0,0 +1,255 @@ +installConfig(['og']); + $this->installEntitySchema('entity_test'); + $this->installEntitySchema('node'); + $this->installEntitySchema('og_membership'); + $this->installEntitySchema('user'); + $this->installSchema('system', 'sequences'); + + $this->groupBundle = Unicode::strtolower($this->randomMachineName()); + + // Create a test user with UID 1. This user has universal access. + $this->users['uid1'] = User::create(['name' => $this->randomString()]); + $this->users['uid1']->save(); + + // Create a user that will serve as the group owner. There are no special + // permissions granted to the group owner, so this user is not tested. + $group_owner = User::create(['name' => $this->randomString()]); + $group_owner->save(); + + // Declare that the test entity is a group type. + Og::groupManager()->addGroup('entity_test', $this->groupBundle); + + // Create the test group. + $this->group = EntityTest::create([ + 'type' => $this->groupBundle, + 'name' => $this->randomString(), + 'user_id' => $group_owner->id(), + ]); + $this->group->save(); + + // @todo Create test group content types for the 'newsletter' and 'article'. + + // Create 3 test roles with associated permissions. We will simulate a + // project that has two group content types: + // - 'newsletter_subscription': Any registered user can create entities of + // this type, even if they are not a member of the group. + // - 'article': These can only be created by group members. Normal members + // can edit and delete their own articles, while admins can edit and + // delete any article. + $permission_matrix = [ + OgRoleInterface::ANONYMOUS => [ + 'create newsletter_subscription entity_test', + 'update own newsletter_subscription entity_test', + 'delete own newsletter_subscription entity_test', + ], + OgRoleInterface::AUTHENTICATED => [ + 'create newsletter_subscription entity_test', + 'update own newsletter_subscription entity_test', + 'delete own newsletter_subscription entity_test', + 'create article content', + 'edit own article content', + 'delete own article content', + ], + // The administrator is not explicitly granted permission to edit or + // delete their own group content. Having the 'any' permission should be + // sufficient to also be able to edit their own content. + OgRoleInterface::ADMINISTRATOR => [ + 'create newsletter_subscription entity_test', + 'update any newsletter_subscription entity_test', + 'delete any newsletter_subscription entity_test', + 'create article content', + 'edit any article content', + 'delete any article content', + ], + ]; + + foreach ($permission_matrix as $role_name => $permissions) { + $this->roles[$role_name] = OgRole::create(); + $this->roles[$role_name] + ->setName($role_name) + ->setLabel($this->randomString()) + ->setGroupType($this->group->getEntityTypeId()) + ->setGroupBundle($this->groupBundle) + ->setIsAdmin($role_name === OgRoleInterface::ADMINISTRATOR); + foreach ($permissions as $permission) { + $this->roles[$role_name]->grantPermission($permission); + } + $this->roles[$role_name]->save(); + + // Create a test user with this role. + $this->users[$role_name] = User::create(['name' => $this->randomString()]); + $this->users[$role_name]->save(); + + // Subscribe the user to the group. + // Skip creation of the membership for the non-member user. It is actually + // fine to save this membership, but in the most common use case this + // membership will not exist in the database. + if ($role_name !== OgRoleInterface::ANONYMOUS) { + /** @var OgMembership $membership */ + $membership = OgMembership::create(['type' => OgMembershipInterface::TYPE_DEFAULT]); + $membership + ->setUser($this->users[$role_name]->id()) + ->setEntityId($this->group->id()) + ->setGroupEntityType($this->group->getEntityTypeId()) + ->addRole($this->roles[$role_name]->id()) + ->setState(OgMembershipInterface::STATE_ACTIVE) + ->save(); + } + } + } + + /** + * Test access to group content entity operations. + * + * @dataProvider accessProvider + */ + public function testAccess($group_content_entity_type_id, $group_content_bundle_id, $expected_access) { + $og_access = $this->container->get('og.access'); + + } + + /** + * Data provider for ::testAccess(). + * + * @return array + * And array of test data sets. Each set consisting of: + * - A string representing the group content entity type ID upon which the + * operation is performed. Can be either 'node' or 'entity_test'. + * - A string representing the group content bundle ID upon which the + * operation is performed. Can be either 'newsletter_subscription' or + * 'article'. + * - An array mapping the different users and the possible operations, and + * whether or not the user is expected to be granted access to this + * operation, depending on whether they own the content or not. + */ + public function accessProvider() { + return [ + [ + 'entity_test', + 'newsletter_subscription', + [ + // The super user and the administrator have the right to create, + // update and delete any newsletter subscription. + 'uid1' => [ + 'create' => TRUE, + 'update' => ['own' => TRUE, 'any' => TRUE], + 'delete' => ['own' => TRUE, 'any' => TRUE], + ], + OgRoleInterface::ADMINISTRATOR => [ + 'create' => TRUE, + 'update' => ['own' => TRUE, 'any' => TRUE], + 'delete' => ['own' => TRUE, 'any' => TRUE], + ], + // Non-members and members have the right to subscribe to the + // newsletter, and to manage or delete their own newsletter + // subscriptions. + OgRoleInterface::ANONYMOUS => [ + 'create' => TRUE, + 'update' => ['own' => TRUE, 'any' => FALSE], + 'delete' => ['own' => TRUE, 'any' => FALSE], + ], + OgRoleInterface::AUTHENTICATED => [ + 'create' => TRUE, + 'update' => ['own' => TRUE, 'any' => FALSE], + 'delete' => ['own' => TRUE, 'any' => FALSE], + ], + ], + ], + [ + 'node', + 'article', + [ + // The super user and the administrator have the right to create, + // update and delete any article. + 'uid1' => [ + 'create' => TRUE, + 'update' => ['own' => TRUE, 'any' => TRUE], + 'delete' => ['own' => TRUE, 'any' => TRUE], + ], + OgRoleInterface::ADMINISTRATOR => [ + 'create' => TRUE, + 'update' => ['own' => TRUE, 'any' => TRUE], + 'delete' => ['own' => TRUE, 'any' => TRUE], + ], + // Non-members do not have the right to create or manage any article. + OgRoleInterface::ANONYMOUS => [ + 'create' => FALSE, + 'update' => ['own' => FALSE, 'any' => FALSE], + 'delete' => ['own' => FALSE, 'any' => FALSE], + ], + // Members have the right to create new articles, and to manage their + // own articles. + OgRoleInterface::AUTHENTICATED => [ + 'create' => TRUE, + 'update' => ['own' => TRUE, 'any' => FALSE], + 'delete' => ['own' => TRUE, 'any' => FALSE], + ], + ], + ], + ]; + } + +} From b7fce65f169a1235d9c5f5f8fc05a4c5a2fbc545 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Thu, 30 Jun 2016 18:31:47 +0200 Subject: [PATCH 39/85] First round of failure fixing. --- src/Entity/OgRole.php | 3 ++- .../OgGroupContentOperationAccessTest.php | 20 +++++++++---------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/Entity/OgRole.php b/src/Entity/OgRole.php index 23390e881..aad197463 100644 --- a/src/Entity/OgRole.php +++ b/src/Entity/OgRole.php @@ -254,7 +254,8 @@ public function set($property_name, $value) { 'group_type', 'group_bundle', ]); - if ($is_locked_property && !$this->isNew()) { + $value_has_changed = !empty($original_value = $this->get($property_name)) && $original_value != $value; + if ($is_locked_property && !$this->isNew() && $value_has_changed) { throw new OgRoleException("The $property_name cannot be changed."); } return parent::set($property_name, $value); diff --git a/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php b/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php index bd52e6e1e..f75b43d41 100644 --- a/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php +++ b/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php @@ -8,7 +8,6 @@ use Drupal\og\Entity\OgMembership; use Drupal\og\Entity\OgRole; use Drupal\og\Og; -use Drupal\og\OgAccess; use Drupal\og\OgMembershipInterface; use Drupal\og\OgRoleInterface; use Drupal\user\Entity\User; @@ -23,7 +22,14 @@ class OgGroupContentOperationAccessTest extends KernelTestBase { /** * {@inheritdoc} */ - public static $modules = ['system', 'user', 'field', 'og', 'entity_test']; + public static $modules = [ + 'entity_test', + 'field', + 'node', + 'og', + 'system', + 'user', + ]; /** * An array of test users. @@ -127,13 +133,8 @@ protected function setUp() { ]; foreach ($permission_matrix as $role_name => $permissions) { - $this->roles[$role_name] = OgRole::create(); - $this->roles[$role_name] - ->setName($role_name) - ->setLabel($this->randomString()) - ->setGroupType($this->group->getEntityTypeId()) - ->setGroupBundle($this->groupBundle) - ->setIsAdmin($role_name === OgRoleInterface::ADMINISTRATOR); + $role_id = "{$this->group->getEntityTypeId()}-{$this->group->bundle()}-$role_name"; + $this->roles[$role_name] = OgRole::load($role_id); foreach ($permissions as $permission) { $this->roles[$role_name]->grantPermission($permission); } @@ -168,7 +169,6 @@ protected function setUp() { */ public function testAccess($group_content_entity_type_id, $group_content_bundle_id, $expected_access) { $og_access = $this->container->get('og.access'); - } /** From 512549671909a277980bacf25a41f1ee70039b31 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 5 Jul 2016 14:32:47 +0200 Subject: [PATCH 40/85] Change ownership from a string to a boolean. --- src/Event/PermissionEvent.php | 14 ++++---- src/Event/PermissionEventInterface.php | 24 +++++++------- src/EventSubscriber/OgEventSubscriber.php | 16 ++++----- src/GroupContentOperationPermission.php | 33 ++++++++++--------- tests/src/Kernel/PermissionEventTest.php | 2 +- .../GroupContentOperationPermissionTest.php | 14 ++++---- tests/src/Unit/PermissionEventTest.php | 26 +++++++-------- 7 files changed, 66 insertions(+), 63 deletions(-) diff --git a/src/Event/PermissionEvent.php b/src/Event/PermissionEvent.php index 9128b0725..43a33693e 100644 --- a/src/Event/PermissionEvent.php +++ b/src/Event/PermissionEvent.php @@ -79,14 +79,14 @@ public function getPermission($name) { /** * {@inheritdoc} */ - public function getGroupContentOperationPermission($entity_type_id, $bundle_id, $operation, $ownership = 'any') { + public function getGroupContentOperationPermission($entity_type_id, $bundle_id, $operation, $owner = FALSE) { foreach ($this->getPermissions() as $permission) { if ( $permission instanceof GroupContentOperationPermission && $permission->getEntityType() === $entity_type_id && $permission->getBundle() === $bundle_id && $permission->getOperation() === $operation - && $permission->getOwnership() === $ownership + && $permission->getOwner() === $owner ) { return $permission; } @@ -130,7 +130,7 @@ public function setPermission(PermissionInterface $permission) { // Check if this permission was already registered under another name, and // remove it so the new one replaces it. try { - $this->deleteGroupContentOperationPermission($permission->getEntityType(), $permission->getBundle(), $permission->getOperation(), $permission->getOwnership()); + $this->deleteGroupContentOperationPermission($permission->getEntityType(), $permission->getBundle(), $permission->getOperation(), $permission->getOwner()); } catch (\InvalidArgumentException $e) { // The permission wasn't set. There is nothing to delete. @@ -161,8 +161,8 @@ public function deletePermission($name) { /** * {@inheritdoc} */ - public function deleteGroupContentOperationPermission($entity_type_id, $bundle_id, $operation, $ownership = 'any') { - $permission = $this->getGroupContentOperationPermission($entity_type_id, $bundle_id, $operation, $ownership); + public function deleteGroupContentOperationPermission($entity_type_id, $bundle_id, $operation, $owner = 'any') { + $permission = $this->getGroupContentOperationPermission($entity_type_id, $bundle_id, $operation, $owner); $this->deletePermission($permission->getName()); } @@ -176,9 +176,9 @@ public function hasPermission($name) { /** * {@inheritdoc} */ - public function hasGroupContentOperationPermission($entity_type_id, $bundle_id, $operation, $ownership = 'any') { + public function hasGroupContentOperationPermission($entity_type_id, $bundle_id, $operation, $owner = FALSE) { try { - $this->getGroupContentOperationPermission($entity_type_id, $bundle_id, $operation, $ownership); + $this->getGroupContentOperationPermission($entity_type_id, $bundle_id, $operation, $owner); } catch (\InvalidArgumentException $e) { return FALSE; diff --git a/src/Event/PermissionEventInterface.php b/src/Event/PermissionEventInterface.php index 664882eb5..e9d1a0fc3 100644 --- a/src/Event/PermissionEventInterface.php +++ b/src/Event/PermissionEventInterface.php @@ -40,9 +40,9 @@ public function getPermission($name); * The group content bundle ID to which this permission applies. * @param string $operation * The entity operation to which this permission applies. - * @param string $ownership - * If this applies to all entities, or only to the ones owned by the user. - * Can be either 'any' or 'own'. Defaults to 'any'. + * @param bool $owner + * Set to FALSE if this permission applies to all entities, or to TRUE if it + * only applies to the ones owned by the user. Defaults to FALSE. * * @return \Drupal\og\GroupContentOperationPermission * The permission. @@ -50,7 +50,7 @@ public function getPermission($name); * @throws \InvalidArgumentException * Thrown if the permission with the given properties does not exist. */ - public function getGroupContentOperationPermission($entity_type_id, $bundle_id, $operation, $ownership = 'any'); + public function getGroupContentOperationPermission($entity_type_id, $bundle_id, $operation, $owner = FALSE); /** * Returns all the permissions. @@ -96,11 +96,11 @@ public function deletePermission($name); * The group content bundle ID to which this permission applies. * @param string $operation * The entity operation to which this permission applies. - * @param string $ownership - * If this applies to all entities, or only to the ones owned by the user. - * Can be either 'any' or 'own'. Defaults to 'any'. + * @param bool $owner + * Set to FALSE if this permission applies to all entities, or to TRUE if it + * only applies to the ones owned by the user. Defaults to FALSE. */ - public function deleteGroupContentOperationPermission($entity_type_id, $bundle_id, $operation, $ownership = 'any'); + public function deleteGroupContentOperationPermission($entity_type_id, $bundle_id, $operation, $owner = FALSE); /** * Returns whether or not the given permission exists. @@ -122,14 +122,14 @@ public function hasPermission($name); * The group content bundle ID to which this permission applies. * @param string $operation * The entity operation to which this permission applies. - * @param string $ownership - * If this applies to all entities, or only to the ones owned by the user. - * Can be either 'any' or 'own'. Defaults to 'any'. + * @param bool $owner + * Set to FALSE if this permission applies to all entities, or to TRUE if it + * only applies to the ones owned by the user. Defaults to FALSE. * * @return bool * Whether or not the permission exists. */ - public function hasGroupContentOperationPermission($entity_type_id, $bundle_id, $operation, $ownership = 'any'); + public function hasGroupContentOperationPermission($entity_type_id, $bundle_id, $operation, $owner = FALSE); /** * Returns the entity type ID of the group to which the permissions apply. diff --git a/src/EventSubscriber/OgEventSubscriber.php b/src/EventSubscriber/OgEventSubscriber.php index 0bfe06ba5..64f607e06 100644 --- a/src/EventSubscriber/OgEventSubscriber.php +++ b/src/EventSubscriber/OgEventSubscriber.php @@ -126,25 +126,25 @@ public function provideDefaultNodePermissions(PermissionEventInterface $event) { 'name' => "edit own $bundle_id content", 'title' => $this->t('%type_name: Edit own content', $args), 'operation' => 'update', - 'ownership' => 'own', + 'owner' => TRUE, ], [ 'name' => "edit any $bundle_id content", 'title' => $this->t('%type_name: Edit any content', $args), 'operation' => 'update', - 'ownership' => 'any', + 'owner' => FALSE, ], [ 'name' => "delete own $bundle_id content", 'title' => $this->t('%type_name: Delete own content', $args), 'operation' => 'delete', - 'ownership' => 'own', + 'owner' => TRUE, ], [ 'name' => "delete any $bundle_id content", 'title' => $this->t('%type_name: Delete any content', $args), 'operation' => 'delete', - 'ownership' => 'any', + 'owner' => FALSE, ], ]; foreach ($permission_values as $values) { @@ -253,25 +253,25 @@ protected function generateEntityOperationPermissionList($group_content_entity_t 'name' => "update own $group_content_bundle_id $group_content_entity_type_id", 'title' => $this->t('Edit own %bundle @entity', $args), 'operation' => 'update', - 'ownership' => 'own', + 'owner' => TRUE, ], [ 'name' => "update any $group_content_bundle_id $group_content_entity_type_id", 'title' => $this->t('Edit any %bundle @entity', $args), 'operation' => 'update', - 'ownership' => 'any', + 'owner' => FALSE, ], [ 'name' => "delete own $group_content_bundle_id $group_content_entity_type_id", 'title' => $this->t('Delete own %bundle @entity', $args), 'operation' => 'delete', - 'ownership' => 'own', + 'owner' => TRUE, ], [ 'name' => "delete any $group_content_bundle_id $group_content_entity_type_id", 'title' => $this->t('Delete any %bundle @entity', $args), 'operation' => 'delete', - 'ownership' => 'any', + 'owner' => FALSE, ], ]; diff --git a/src/GroupContentOperationPermission.php b/src/GroupContentOperationPermission.php index 948381792..a4395fb76 100644 --- a/src/GroupContentOperationPermission.php +++ b/src/GroupContentOperationPermission.php @@ -39,10 +39,11 @@ class GroupContentOperationPermission extends Permission { * Use this to make the distinction between 'edit any article content' and * 'edit own article content'. * - * @var string - * Either 'any' or 'own'. + * @var bool + * FALSE if this permission applies to all entities, TRUE if it only applies + * to the entities owned by the user. */ - protected $ownership = 'any'; + protected $owner = 'any'; /** * Returns the group content entity type ID to which this permission applies. @@ -114,25 +115,27 @@ public function setOperation($operation) { } /** - * Returns the ownership scope of this permission. + * Returns the owner scope of this permission. * - * @return string - * The ownership scope, either 'any' or 'own'. + * @return bool + * FALSE if this permission applies to all entities, TRUE if it only applies + * to the entities owned by the user. */ - public function getOwnership() { - return $this->get('ownership'); + public function getOwner() { + return $this->get('owner'); } /** - * Sets the ownership scope of this permission. + * Sets the owner scope of this permission. * - * @param string $ownership - * The ownership scope, either 'any' or 'own'. + * @param bool + * FALSE if this permission applies to all entities, TRUE if it only applies + * to the entities owned by the user. * * @return $this */ - public function setOwnership($ownership) { - $this->set('ownership' , $ownership); + public function setOwner($owner) { + $this->set('owner' , $owner); return $this; } @@ -142,8 +145,8 @@ public function setOwnership($ownership) { protected function validate($property, $value) { parent::validate($property, $value); - if ($property === 'ownership' && !in_array($value, ['any', 'own'])) { - throw new \InvalidArgumentException('The ownership should be either "any" or "own".'); + if ($property === 'owner' && !is_bool($value)) { + throw new \InvalidArgumentException('The owner should be a boolean value.'); } } diff --git a/tests/src/Kernel/PermissionEventTest.php b/tests/src/Kernel/PermissionEventTest.php index 50ef3f390..4bfb61965 100644 --- a/tests/src/Kernel/PermissionEventTest.php +++ b/tests/src/Kernel/PermissionEventTest.php @@ -253,7 +253,7 @@ protected function getPermissionProperties(PermissionInterface $permission) { 'entity type', 'bundle', 'operation', - 'ownership', + 'owner', ]); } diff --git a/tests/src/Unit/GroupContentOperationPermissionTest.php b/tests/src/Unit/GroupContentOperationPermissionTest.php index da808f485..d72639bf0 100644 --- a/tests/src/Unit/GroupContentOperationPermissionTest.php +++ b/tests/src/Unit/GroupContentOperationPermissionTest.php @@ -97,27 +97,27 @@ public function testSetOperation(array $values) { * @param array $values * Associative array of test values, keyed by property name. * - * @covers ::getOwnership + * @covers ::getOwner * * @dataProvider groupContentOperationPermissionProvider */ public function testGetOwnership(array $values) { $permission = new GroupContentOperationPermission($values); - $this->assertEquals($values['ownership'], $permission->getOwnership()); + $this->assertEquals($values['owner'], $permission->getOwner()); } /** * @param array $values * Associative array of test values, keyed by property name. * - * @covers ::setOwnership + * @covers ::setOwner * * @dataProvider groupContentOperationPermissionProvider */ public function testSetOwnership(array $values) { $permission = new GroupContentOperationPermission(); - $permission->setOwnership($values['ownership']); - $this->assertEquals($values['ownership'], $permission->get('ownership')); + $permission->setOwner($values['owner']); + $this->assertEquals($values['owner'], $permission->get('owner')); } /** @@ -323,7 +323,7 @@ public function testSetInvalidRestrictAccessValue() { */ public function testSetInvalidOwnershipValue() { $permission = new GroupContentOperationPermission(); - $permission->set('ownership', 'invalid value'); + $permission->set('owner', 'invalid value'); } /** @@ -343,7 +343,7 @@ public function groupContentOperationPermissionProvider() { 'entity type' => 'node', 'bundle' => 'article', 'operation' => 'update', - 'ownership' => 'own', + 'owner' => TRUE, ], ], ]; diff --git a/tests/src/Unit/PermissionEventTest.php b/tests/src/Unit/PermissionEventTest.php index 0cbe41866..6c69091e4 100644 --- a/tests/src/Unit/PermissionEventTest.php +++ b/tests/src/Unit/PermissionEventTest.php @@ -72,7 +72,7 @@ public function testGetGroupContentOperationPermission(array $permissions, $enti // An exception should be thrown if the permission doesn't exist yet. foreach ($permissions as $permission) { try { - $event->getGroupContentOperationPermission($permission->getEntityType(), $permission->getBundle(), $permission->getOperation(), $permission->getOwnership()); + $event->getGroupContentOperationPermission($permission->getEntityType(), $permission->getBundle(), $permission->getOperation(), $permission->getOwner()); $this->fail('Calling ::getGroupContentOperationPermission() on a non-existing permission throws an exception.'); } catch (\InvalidArgumentException $e) { @@ -84,7 +84,7 @@ public function testGetGroupContentOperationPermission(array $permissions, $enti $event->setPermissions($permissions); foreach ($permissions as $permission) { - $this->assertEquals($permission, $event->getGroupContentOperationPermission($permission->getEntityType(), $permission->getBundle(), $permission->getOperation(), $permission->getOwnership())); + $this->assertEquals($permission, $event->getGroupContentOperationPermission($permission->getEntityType(), $permission->getBundle(), $permission->getOperation(), $permission->getOwner())); } } @@ -214,7 +214,7 @@ public function testDeleteGroupContentOperationPermission(array $permissions, $e $permission_entity_type_id = $permission->getEntityType(); $permission_bundle_id = $permission->getBundle(); $permission_operation = $permission->getOperation(); - $permission_ownership = $permission->getOwnership(); + $permission_ownership = $permission->getOwner(); // Before we delete the permission, it should still be there. $this->assertTrue($event->hasPermission($name)); @@ -272,7 +272,7 @@ public function testHasGroupContentOperationPermission(array $permissions, $enti $permission_entity_type_id = $permission->getEntityType(); $permission_bundle_id = $permission->getBundle(); $permission_operation = $permission->getOperation(); - $permission_ownership = $permission->getOwnership(); + $permission_ownership = $permission->getOwner(); $this->assertFalse($event->hasGroupContentOperationPermission($permission_entity_type_id, $permission_bundle_id, $permission_operation, $permission_ownership)); $event->setPermission($permission); @@ -553,14 +553,14 @@ public function testIteratorAggregate(array $permissions, $entity_type_id, $bund */ public function testInvalidGroupContentOperationPermissionCreation() { // An exception should be thrown when a group content operation permission - // is created with an invalid ownership type. + // is created with an invalid owner type. new GroupContentOperationPermission([ 'name' => 'invalid permission', 'title' => $this->t('This is an invalid permission.'), 'entity type' => 'node', 'bundle' => 'article', 'operation' => 'create', - 'ownership' => 'an invalid ownership', + 'owner' => 'an invalid owner', ]); } @@ -577,7 +577,7 @@ public function testOverwritingGroupContentOperationPermission() { $entity_type_id = $this->randomMachineName(); $bundle_id = $this->randomMachineName(); $operation = $this->randomMachineName(); - $ownership = 'own'; + $ownership = TRUE; $original_permission = new GroupContentOperationPermission([ 'name' => 'the original permission', @@ -586,7 +586,7 @@ public function testOverwritingGroupContentOperationPermission() { 'entity type' => $entity_type_id, 'bundle' => $bundle_id, 'operation' => $operation, - 'ownership' => $ownership, + 'owner' => $ownership, 'default roles' => [OgRoleInterface::ADMINISTRATOR], 'restrict access' => TRUE, ]); @@ -598,7 +598,7 @@ public function testOverwritingGroupContentOperationPermission() { 'entity type' => $entity_type_id, 'bundle' => $bundle_id, 'operation' => $operation, - 'ownership' => $ownership, + 'owner' => $ownership, 'default roles' => [OgRoleInterface::AUTHENTICATED], 'restrict access' => FALSE, ]); @@ -872,7 +872,7 @@ public function groupContentOperationPermissionsProvider() { ]), ], ], - // A permission with an ownership. + // A permission with an owner. [ [ new GroupContentOperationPermission([ @@ -882,7 +882,7 @@ public function groupContentOperationPermissionsProvider() { 'entity type' => 'yak', 'bundle' => 'bos grunniens', 'operation' => 'shave', - 'ownership' => 'own', + 'owner' => TRUE, 'default roles' => [ OgRoleInterface::AUTHENTICATED, OgRoleInterface::ADMINISTRATOR, @@ -899,7 +899,7 @@ public function groupContentOperationPermissionsProvider() { 'entity type' => 'wool', 'bundle' => 'yak fibre', 'operation' => 'spin', - 'ownership' => 'any', + 'owner' => TRUE, ]), new GroupContentOperationPermission([ 'name' => 'weave own yak fibre', @@ -907,7 +907,7 @@ public function groupContentOperationPermissionsProvider() { 'entity type' => 'wool', 'bundle' => 'yak fibre', 'operation' => 'weave', - 'ownership' => 'own', + 'owner' => TRUE, ]), new GroupContentOperationPermission([ 'name' => 'dye any yak fibre', From fd8847392e4d12cd0f1dede8441e9b7b1f79fcae Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 5 Jul 2016 14:49:59 +0200 Subject: [PATCH 41/85] Update documentation. --- src/EventSubscriber/OgEventSubscriber.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/EventSubscriber/OgEventSubscriber.php b/src/EventSubscriber/OgEventSubscriber.php index 64f607e06..da0f3394a 100644 --- a/src/EventSubscriber/OgEventSubscriber.php +++ b/src/EventSubscriber/OgEventSubscriber.php @@ -63,6 +63,10 @@ public function __construct(PermissionManagerInterface $permission_manager, Enti public static function getSubscribedEvents() { return [ PermissionEventInterface::EVENT_NAME => [ + // Provide a higher priority for the generic event subscriber so that it + // can run first and set default values for all supported entity types, + // which can then be overridden by other subscribers that set module + // specific permissions. ['provideDefaultOgPermissions', 10], ['provideDefaultNodePermissions'] ], From 2323d359b72a08b3888106d9a8fde422efe07983 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 5 Jul 2016 14:50:38 +0200 Subject: [PATCH 42/85] Rename [g|s]etRoles() to [g|s]etApplicableRoles(). --- src/GroupPermission.php | 4 ++-- tests/src/Unit/GroupPermissionTest.php | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/GroupPermission.php b/src/GroupPermission.php index c424ea275..2607c4594 100644 --- a/src/GroupPermission.php +++ b/src/GroupPermission.php @@ -29,7 +29,7 @@ class GroupPermission extends Permission { * An array of roles to which this permission applies. If empty, the * permission applies to all roles. */ - public function getRoles() { + public function getApplicableRoles() { return $this->get('roles'); } @@ -44,7 +44,7 @@ public function getRoles() { * * @return $this */ - public function setRoles(array $roles) { + public function setApplicableRoles(array $roles) { $this->set('roles' , $roles); return $this; } diff --git a/tests/src/Unit/GroupPermissionTest.php b/tests/src/Unit/GroupPermissionTest.php index dcb7983c0..0730fb6ec 100644 --- a/tests/src/Unit/GroupPermissionTest.php +++ b/tests/src/Unit/GroupPermissionTest.php @@ -16,26 +16,26 @@ class GroupPermissionTest extends UnitTestCase { * @param array $values * Associative array of test values, keyed by property name. * - * @covers ::getRoles + * @covers ::getApplicableRoles * * @dataProvider groupPermissionProvider */ public function testGetRoles(array $values) { $permission = new GroupPermission($values); - $this->assertEquals($values['roles'], $permission->getRoles()); + $this->assertEquals($values['roles'], $permission->getApplicableRoles()); } /** * @param array $values * Associative array of test values, keyed by property name. * - * @covers ::setRoles + * @covers ::setApplicableRoles * * @dataProvider groupPermissionProvider */ public function testSetRoles(array $values) { $permission = new GroupPermission(); - $permission->setRoles($values['roles']); + $permission->setApplicableRoles($values['roles']); $this->assertEquals($values['roles'], $permission->get('roles')); } From 9f8815ca4d6e244c24d43d0c0cb77c2db90b6426 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 5 Jul 2016 17:51:10 +0200 Subject: [PATCH 43/85] Fix fatal errors discovered while testing. --- src/Og.php | 2 +- src/OgAccess.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Og.php b/src/Og.php index faa1e4add..4e52dcc62 100644 --- a/src/Og.php +++ b/src/Og.php @@ -289,7 +289,7 @@ public static function getMembership(AccountInterface $user, EntityInterface $gr ]); $storage = \Drupal::entityTypeManager()->getStorage('og_membership'); return $storage->create(['type' => OgMembershipInterface::TYPE_DEFAULT]) - ->setUser($user) + ->setUser($user->id()) ->setEntityId($group->id()) ->setGroupEntityType($group->getEntityTypeId()) ->setState(OgMembershipInterface::STATE_PENDING) diff --git a/src/OgAccess.php b/src/OgAccess.php index 79afa30d1..800e1755a 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -312,7 +312,7 @@ public function userAccessGroupContentEntityOperations($operation, EntityInterfa // Filter the permissions by operation and ownership. $ownerships = $is_owner ? ['any', 'own'] : ['any']; $permissions = array_filter($permissions, function (GroupContentOperationPermission $permission) use ($operation, $ownerships) { - return $permission->getOperation() === $operation && in_array($permission->getOwnership(), $ownerships); + return $permission->getOperation() === $operation && in_array($permission->getOwner(), $ownerships); }); // @todo This doesn't really vary by user but by the user's role inside of From 25a6c3bc24ffa6701bc47a2116323a91145624bc Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 5 Jul 2016 17:51:25 +0200 Subject: [PATCH 44/85] Update documentation. --- src/OgMembershipInterface.php | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/OgMembershipInterface.php b/src/OgMembershipInterface.php index 1c9b396a6..072fd0d48 100644 --- a/src/OgMembershipInterface.php +++ b/src/OgMembershipInterface.php @@ -150,8 +150,10 @@ public function getGroupEntityType(); /** * Sets the membership state. * - * @param bool $state - * TRUE or FALSE. + * @param int $state + * One of OgMembershipInterface::STATE_ACTIVE, + * OgMembershipInterface::STATE_PENDING, or + * OgMembershipInterface::STATE_BLOCKED. * * @return OgMembershipInterface */ @@ -160,7 +162,10 @@ public function setState($state); /** * Gets the membership state. * - * @return bool + * @return int + * One of OgMembershipInterface::STATE_ACTIVE, + * OgMembershipInterface::STATE_PENDING, or + * OgMembershipInterface::STATE_BLOCKED. */ public function getState(); From 9a9ee7dc67d5a2d265343f902b024402ac91b813 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 5 Jul 2016 17:52:06 +0200 Subject: [PATCH 45/85] Working on test coverage. --- .../OgGroupContentOperationAccessTest.php | 154 +++++++++++++++--- 1 file changed, 128 insertions(+), 26 deletions(-) diff --git a/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php b/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php index f75b43d41..fcbc69db8 100644 --- a/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php +++ b/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php @@ -2,12 +2,15 @@ namespace Drupal\Tests\og\Kernel\Access; +use Drupal\comment\Entity\CommentType; use Drupal\Component\Utility\Unicode; use Drupal\entity_test\Entity\EntityTest; use Drupal\KernelTests\KernelTestBase; +use Drupal\node\Entity\NodeType; use Drupal\og\Entity\OgMembership; use Drupal\og\Entity\OgRole; use Drupal\og\Og; +use Drupal\og\OgGroupAudienceHelper; use Drupal\og\OgMembershipInterface; use Drupal\og\OgRoleInterface; use Drupal\user\Entity\User; @@ -23,6 +26,7 @@ class OgGroupContentOperationAccessTest extends KernelTestBase { * {@inheritdoc} */ public static $modules = [ + 'comment', 'entity_test', 'field', 'node', @@ -61,6 +65,20 @@ class OgGroupContentOperationAccessTest extends KernelTestBase { */ protected $roles; + /** + * An array of test group content, keyed by bundle ID and user ID. + * + * @var \Drupal\Core\Entity\ContentEntityInterface[][] + */ + protected $groupContent; + + /** + * The entity type manager. + * + * @var \Drupal\Core\Entity\EntityTypeManagerInterface + */ + protected $entityTypeManager; + /** * {@inheritdoc} */ @@ -69,11 +87,14 @@ protected function setUp() { $this->installConfig(['og']); $this->installEntitySchema('entity_test'); + $this->installEntitySchema('comment'); $this->installEntitySchema('node'); $this->installEntitySchema('og_membership'); $this->installEntitySchema('user'); $this->installSchema('system', 'sequences'); + $this->entityTypeManager = $this->container->get('entity_type.manager'); + $this->groupBundle = Unicode::strtolower($this->randomMachineName()); // Create a test user with UID 1. This user has universal access. @@ -81,9 +102,10 @@ protected function setUp() { $this->users['uid1']->save(); // Create a user that will serve as the group owner. There are no special - // permissions granted to the group owner, so this user is not tested. - $group_owner = User::create(['name' => $this->randomString()]); - $group_owner->save(); + // permissions granted to the group owner, so this user can be used for + // creating entities that are not owned by the user under test. + $this->users['group_owner'] = User::create(['name' => $this->randomString()]); + $this->users['group_owner']->save(); // Declare that the test entity is a group type. Og::groupManager()->addGroup('entity_test', $this->groupBundle); @@ -92,12 +114,10 @@ protected function setUp() { $this->group = EntityTest::create([ 'type' => $this->groupBundle, 'name' => $this->randomString(), - 'user_id' => $group_owner->id(), + 'user_id' => $this->users['group_owner']->id(), ]); $this->group->save(); - // @todo Create test group content types for the 'newsletter' and 'article'. - // Create 3 test roles with associated permissions. We will simulate a // project that has two group content types: // - 'newsletter_subscription': Any registered user can create entities of @@ -107,14 +127,14 @@ protected function setUp() { // delete any article. $permission_matrix = [ OgRoleInterface::ANONYMOUS => [ - 'create newsletter_subscription entity_test', - 'update own newsletter_subscription entity_test', - 'delete own newsletter_subscription entity_test', + 'create newsletter_subscription comment', + 'update own newsletter_subscription comment', + 'delete own newsletter_subscription comment', ], OgRoleInterface::AUTHENTICATED => [ - 'create newsletter_subscription entity_test', - 'update own newsletter_subscription entity_test', - 'delete own newsletter_subscription entity_test', + 'create newsletter_subscription comment', + 'update own newsletter_subscription comment', + 'delete own newsletter_subscription comment', 'create article content', 'edit own article content', 'delete own article content', @@ -123,9 +143,9 @@ protected function setUp() { // delete their own group content. Having the 'any' permission should be // sufficient to also be able to edit their own content. OgRoleInterface::ADMINISTRATOR => [ - 'create newsletter_subscription entity_test', - 'update any newsletter_subscription entity_test', - 'delete any newsletter_subscription entity_test', + 'create newsletter_subscription comment', + 'update any newsletter_subscription comment', + 'delete any newsletter_subscription comment', 'create article content', 'edit any article content', 'delete any article content', @@ -160,6 +180,74 @@ protected function setUp() { ->save(); } } + + // Create a 'newsletter_subscription' group content type. We are using the + // Comment entity for this to verify that this functionality works for all + // entity types. We cannot use the 'entity_test' entity for this since it + // has no support for bundles. Let's imagine that we have a use case where + // the user can leave a comment with the text 'subscribe' in order to + // subscribe to the newsletter. + CommentType::create([ + 'id' => 'newsletter_subscription', + 'label' => 'Newsletter subscription', + 'target_entity_type_id' => 'entity_test', + ])->save(); + $settings = [ + 'field_storage_config' => [ + 'settings' => [ + 'target_type' => 'entity_test', + ], + ], + ]; + Og::createField(OgGroupAudienceHelper::DEFAULT_FIELD, 'comment', 'newsletter_subscription', $settings); + + // Create an 'article' group content type. + NodeType::create([ + 'type' => 'article', + 'name' => 'Article', + ])->save(); + Og::createField(OgGroupAudienceHelper::DEFAULT_FIELD, 'node', 'article', $settings); + + // Create a group content entity owned by each test user, for both the + // 'newsletter_subscription' and 'article' bundles. + $user_ids = [ + 'uid1', + 'group_owner', + OgRoleInterface::ANONYMOUS, + OgRoleInterface::AUTHENTICATED, + OgRoleInterface::ADMINISTRATOR, + ]; + foreach (['newsletter_subscription', 'article'] as $bundle_id) { + foreach ($user_ids as $user_id) { + $entity_type = $bundle_id === 'article' ? 'node' : 'comment'; + + switch ($entity_type) { + case 'node': + $values = [ + 'title' => $this->randomString(), + 'type' => $bundle_id, + OgGroupAudienceHelper::DEFAULT_FIELD => [['target_id' => $this->group->id()]], + ]; + break; + + case 'comment': + $values = [ + 'subject' => 'subscribe', + 'comment_type' => $bundle_id, + 'entity_id' => $this->group->id(), + 'entity_type' => 'entity_test', + OgGroupAudienceHelper::DEFAULT_FIELD => [['target_id' => $this->group->id()]], + ]; + break; + } + + $entity = $this->entityTypeManager->getStorage($entity_type)->create($values); + $entity->setOwner($this->users[$user_id]); + $entity->save(); + + $this->groupContent[$bundle_id][$user_id] = $entity; + } + } } /** @@ -167,8 +255,22 @@ protected function setUp() { * * @dataProvider accessProvider */ - public function testAccess($group_content_entity_type_id, $group_content_bundle_id, $expected_access) { + public function testAccess($group_content_entity_type_id, $group_content_bundle_id, $expected_access_matrix) { + /** @var \Drupal\og\OgAccessInterface $og_access */ $og_access = $this->container->get('og.access'); + + foreach ($expected_access_matrix as $user_id => $operations) { + foreach ($operations as $operation => $ownerships) { + foreach ($ownerships as $ownership => $expected_access) { + // Depending on whether we're testing access to a user's own entity, + // use either the entity owned by the user, or the one used by the + // 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"); + } + } + } } /** @@ -177,7 +279,7 @@ public function testAccess($group_content_entity_type_id, $group_content_bundle_ * @return array * And array of test data sets. Each set consisting of: * - A string representing the group content entity type ID upon which the - * operation is performed. Can be either 'node' or 'entity_test'. + * operation is performed. Can be either 'node' or 'comment'. * - A string representing the group content bundle ID upon which the * operation is performed. Can be either 'newsletter_subscription' or * 'article'. @@ -188,18 +290,18 @@ public function testAccess($group_content_entity_type_id, $group_content_bundle_ public function accessProvider() { return [ [ - 'entity_test', + 'comment', 'newsletter_subscription', [ // The super user and the administrator have the right to create, // update and delete any newsletter subscription. 'uid1' => [ - 'create' => TRUE, + 'create' => ['any' => TRUE], 'update' => ['own' => TRUE, 'any' => TRUE], 'delete' => ['own' => TRUE, 'any' => TRUE], ], OgRoleInterface::ADMINISTRATOR => [ - 'create' => TRUE, + 'create' => ['any' => TRUE], 'update' => ['own' => TRUE, 'any' => TRUE], 'delete' => ['own' => TRUE, 'any' => TRUE], ], @@ -207,12 +309,12 @@ public function accessProvider() { // newsletter, and to manage or delete their own newsletter // subscriptions. OgRoleInterface::ANONYMOUS => [ - 'create' => TRUE, + 'create' => ['any' => TRUE], 'update' => ['own' => TRUE, 'any' => FALSE], 'delete' => ['own' => TRUE, 'any' => FALSE], ], OgRoleInterface::AUTHENTICATED => [ - 'create' => TRUE, + 'create' => ['any' => TRUE], 'update' => ['own' => TRUE, 'any' => FALSE], 'delete' => ['own' => TRUE, 'any' => FALSE], ], @@ -225,25 +327,25 @@ public function accessProvider() { // The super user and the administrator have the right to create, // update and delete any article. 'uid1' => [ - 'create' => TRUE, + 'create' => ['any' => TRUE], 'update' => ['own' => TRUE, 'any' => TRUE], 'delete' => ['own' => TRUE, 'any' => TRUE], ], OgRoleInterface::ADMINISTRATOR => [ - 'create' => TRUE, + 'create' => ['any' => TRUE], 'update' => ['own' => TRUE, 'any' => TRUE], 'delete' => ['own' => TRUE, 'any' => TRUE], ], // Non-members do not have the right to create or manage any article. OgRoleInterface::ANONYMOUS => [ - 'create' => FALSE, + 'create' => ['any' => FALSE], 'update' => ['own' => FALSE, 'any' => FALSE], 'delete' => ['own' => FALSE, 'any' => FALSE], ], // Members have the right to create new articles, and to manage their // own articles. OgRoleInterface::AUTHENTICATED => [ - 'create' => TRUE, + 'create' => ['any' => TRUE], 'update' => ['own' => TRUE, 'any' => FALSE], 'delete' => ['own' => TRUE, 'any' => FALSE], ], From b32285eb8a034e160113c3ed17f0ff807fa2badc Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 6 Jul 2016 15:36:13 +0300 Subject: [PATCH 46/85] Fix bug discovered by test. --- src/GroupContentOperationPermission.php | 2 +- src/OgAccess.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/GroupContentOperationPermission.php b/src/GroupContentOperationPermission.php index a4395fb76..ccb0c9c49 100644 --- a/src/GroupContentOperationPermission.php +++ b/src/GroupContentOperationPermission.php @@ -43,7 +43,7 @@ class GroupContentOperationPermission extends Permission { * FALSE if this permission applies to all entities, TRUE if it only applies * to the entities owned by the user. */ - protected $owner = 'any'; + protected $owner = FALSE; /** * Returns the group content entity type ID to which this permission applies. diff --git a/src/OgAccess.php b/src/OgAccess.php index 800e1755a..7f42dedc6 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -310,7 +310,7 @@ public function userAccessGroupContentEntityOperations($operation, EntityInterfa $permissions = $this->permissionManager->getDefaultEntityOperationPermissions($group_entity_type_id, $group_bundle_id, $group_content_bundle_ids); // Filter the permissions by operation and ownership. - $ownerships = $is_owner ? ['any', 'own'] : ['any']; + $ownerships = $is_owner ? [FALSE, TRUE] : [FALSE]; $permissions = array_filter($permissions, function (GroupContentOperationPermission $permission) use ($operation, $ownerships) { return $permission->getOperation() === $operation && in_array($permission->getOwner(), $ownerships); }); From 101f8ce7af4b7b524de2083fb085f171d625d229 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Thu, 14 Jul 2016 13:42:48 +0300 Subject: [PATCH 47/85] Only return a membership entity in which the user is a non-member if requested. --- src/Og.php | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/Og.php b/src/Og.php index 4e52dcc62..8c7af59c5 100644 --- a/src/Og.php +++ b/src/Og.php @@ -268,10 +268,12 @@ public static function getMemberships(AccountInterface $user, array $states = [O * @param string $field_name * (optional) The field name associated with the group. * - * @return \Drupal\og\Entity\OgMembership + * @return \Drupal\og\Entity\OgMembership|NULL * The OgMembership entity. If the user is not a member of the group, an * OgMembership entity will be returned in which the user has the * 'non-member' role. + * NULL will be returned if no membership is available that matches the + * passed in $states. */ public static function getMembership(AccountInterface $user, EntityInterface $group, array $states = [OgMembershipInterface::STATE_ACTIVE], $field_name = NULL) { foreach (static::getMemberships($user, $states, $field_name) as $membership) { @@ -282,18 +284,23 @@ public static function getMembership(AccountInterface $user, EntityInterface $gr // The user is not a member, return an OgMembership entity in which the user // has the 'non-member' role. - $role_id = implode('-', [ - $group->getEntityTypeId(), - $group->bundle(), - OgRoleInterface::ANONYMOUS, - ]); - $storage = \Drupal::entityTypeManager()->getStorage('og_membership'); - return $storage->create(['type' => OgMembershipInterface::TYPE_DEFAULT]) - ->setUser($user->id()) - ->setEntityId($group->id()) - ->setGroupEntityType($group->getEntityTypeId()) - ->setState(OgMembershipInterface::STATE_PENDING) - ->setRoles([$role_id]); + if (in_array(OgMembershipInterface::STATE_PENDING, $states)) { + $role_id = implode('-', [ + $group->getEntityTypeId(), + $group->bundle(), + OgRoleInterface::ANONYMOUS, + ]); + $storage = \Drupal::entityTypeManager()->getStorage('og_membership'); + return $storage->create(['type' => OgMembershipInterface::TYPE_DEFAULT]) + ->setUser($user->id()) + ->setEntityId($group->id()) + ->setGroupEntityType($group->getEntityTypeId()) + ->setState(OgMembershipInterface::STATE_PENDING) + ->setRoles([$role_id]); + } + + // No membership matches the request. + return NULL; } /** From 7a21262cab7c667c61afe3da2bfa974b2d2c9078 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Thu, 14 Jul 2016 14:00:00 +0300 Subject: [PATCH 48/85] Request both the memberships entities of members and non-members to determine if they have access. --- src/OgAccess.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 7f42dedc6..8d36180a4 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -324,7 +324,10 @@ public function userAccessGroupContentEntityOperations($operation, EntityInterfa } if ($permissions) { - $membership = Og::getMembership($user, $group_entity); + // Non-members might also have certain permissions within a group. Request + // both memberships of members (represented by STATE_ACTIVE) and + // non-members (STATE_PENDING). + $membership = Og::getMembership($user, $group_entity, [OgMembershipInterface::STATE_ACTIVE, OgMembershipInterface::STATE_PENDING]); foreach ($permissions as $permission) { if ($membership->hasPermission($permission->getName())) { return AccessResult::allowed()->addCacheableDependency($cacheable_metadata); From f8e577936fecca1643dae4d97b492f8653de2d15 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Fri, 15 Jul 2016 17:10:09 +0300 Subject: [PATCH 49/85] Add a functional test that checks access to an entity operation through the UI. --- tests/src/Functional/AccessTest.php | 153 ++++++++++++++++++++++++++++ 1 file changed, 153 insertions(+) create mode 100644 tests/src/Functional/AccessTest.php diff --git a/tests/src/Functional/AccessTest.php b/tests/src/Functional/AccessTest.php new file mode 100644 index 000000000..d16c395c5 --- /dev/null +++ b/tests/src/Functional/AccessTest.php @@ -0,0 +1,153 @@ + ['access content'], + // A site administrator with the right to administer all groups. + 'group-admin' => ['administer group', 'access content'], + // A regular member of the group. + 'member' => ['access content'], + // A user that is not a member of the group. + 'non-member' => ['access content'], + ]; + foreach ($users as $user => $permissions) { + $this->users[$user] = $this->drupalCreateUser($permissions, $user); + } + + // Create a "group" bundle on the Custom Block entity type and turn it into + // a group. Note we're not using the Entity Test entity for this since it + // does not have real support for multiple bundles. + BlockContentType::create(['type' => 'group']); + Og::groupManager()->addGroup('block_content', 'group'); + + // Create a group. + $this->group = BlockContent::create([ + 'title' => $this->randomString(), + 'type' => 'group', + 'uid' => $this->users['owner']->id(), + ]); + $this->group->save(); + + // Create a group content type. + $this->createContentType(['type' => 'group_content']); + $settings = [ + 'field_storage_config' => [ + 'settings' => [ + 'target_type' => 'block_content', + ], + ], + ]; + Og::createField(OgGroupAudienceHelper::DEFAULT_FIELD, 'node', 'group_content', $settings); + + // Grant members permission to edit their own content. + /** @var \Drupal\og\Entity\OgRole $role */ + $role = $this->container->get('entity_type.manager') + ->getStorage('og_role') + ->load('block_content-group-member'); + $role->grantPermission('edit own group_content content'); + $role->save(); + + // Subscribe the member to the group. + /** @var \Drupal\og\Entity\OgMembership $membership */ + $membership = OgMembership::create(['type' => OgMembershipInterface::TYPE_DEFAULT]); + $membership + ->setUser($this->users['member']->id()) + ->setEntityId($this->group->id()) + ->setGroupEntityType($this->group->getEntityTypeId()) + ->addRole($role->id()) + ->save(); + + // Create two group content items, one owned by the group owner, and one by + // the member. + foreach (['owner', 'member'] as $user) { + $this->groupContent[$user] = Node::create([ + 'title' => $this->randomString(), + 'type' => 'group_content', + 'uid' => $this->users[$user]->id(), + OgGroupAudienceHelper::DEFAULT_FIELD => [['target_id' => $this->group->id()]], + ]); + $this->groupContent[$user]->save(); + } + } + + /** + * Tests access to entity operations through the user interface. + * + * Note that this does not use a dataProvider because of the slow performance + * of setting up functional tests. + */ + public function testEntityOperationAccess() { + $test_matrix = [ + // The administrator should have the right to edit both group content + // items. + 'group-admin' => ['owner' => TRUE, 'member' => TRUE], + // The member should only have the right to edit their own group content. + 'member' => ['owner' => FALSE, 'member' => TRUE], + // The non-member cannot edit any group content. + 'non-member' => ['owner' => FALSE, 'member' => FALSE], + ]; + foreach ($test_matrix as $user => $expected_results) { + $this->drupalLogin($this->users[$user]); + foreach ($expected_results as $group_content => $expected_result) { + $this->drupalGet($this->groupContent[$group_content]->toUrl('edit-form')); + $expected_response = $expected_result ? 200 : 403; + $this->assertSession()->statusCodeEquals($expected_response); + } + } + } + +} From e11a71755797829ad275274b859f4c78b31ce002 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 2 Aug 2016 00:54:07 +0300 Subject: [PATCH 50/85] Declare visibility on method. --- tests/src/Functional/AccessTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/src/Functional/AccessTest.php b/tests/src/Functional/AccessTest.php index d16c395c5..34cbaa615 100644 --- a/tests/src/Functional/AccessTest.php +++ b/tests/src/Functional/AccessTest.php @@ -50,7 +50,7 @@ class AccessTest extends BrowserTestBase { /** * {@inheritdoc} */ - function setUp() { + public function setUp() { parent::setUp(); // Create a number of test users. From 0d89fba16677dddd55ff4b493d4b883637ae591a Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 2 Aug 2016 14:34:44 +0300 Subject: [PATCH 51/85] Argument order of Og::getMembership() has changed. --- src/OgAccess.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 8c09a2404..6476a57b3 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -341,7 +341,7 @@ public function userAccessGroupContentEntityOperations($operation, EntityInterfa // Non-members might also have certain permissions within a group. Request // both memberships of members (represented by STATE_ACTIVE) and // non-members (STATE_PENDING). - $membership = Og::getMembership($user, $group_entity, [OgMembershipInterface::STATE_ACTIVE, OgMembershipInterface::STATE_PENDING]); + $membership = Og::getMembership($group_entity, $user, [OgMembershipInterface::STATE_ACTIVE, OgMembershipInterface::STATE_PENDING]); foreach ($permissions as $permission) { if ($membership->hasPermission($permission->getName())) { return AccessResult::allowed()->addCacheableDependency($cacheable_metadata); From 3394ad5f1ed3fa53b5a81d5adcb839c4ee852c55 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 2 Aug 2016 14:36:05 +0300 Subject: [PATCH 52/85] Use the new DX friendly way to set the user and group when creating a membership. --- src/Entity/OgRole.php | 10 ++++++++++ src/Og.php | 19 +++++++++++++------ tests/src/Unit/DefaultRoleEventTest.php | 2 +- 3 files changed, 24 insertions(+), 7 deletions(-) diff --git a/src/Entity/OgRole.php b/src/Entity/OgRole.php index 512f9dbe9..8fb6104c7 100644 --- a/src/Entity/OgRole.php +++ b/src/Entity/OgRole.php @@ -36,6 +36,16 @@ */ class OgRole extends Role implements OgRoleInterface { + /** + * Constructs an OgRole object. + * + * @param array $values + * An array of values to set, keyed by property name. + */ + public function __construct(array $values) { + parent::__construct($values, 'og_role'); + } + /** * Sets the ID of the role. * diff --git a/src/Og.php b/src/Og.php index c49e742c0..7fe2874d4 100644 --- a/src/Og.php +++ b/src/Og.php @@ -9,6 +9,7 @@ use Drupal\field\Entity\FieldConfig; use Drupal\field\Entity\FieldStorageConfig; use Drupal\field\FieldStorageConfigInterface; +use Drupal\og\Entity\OgMembership; use Drupal\og\Entity\OgRole; use Drupal\og\Plugin\EntityReferenceSelection\OgSelection; @@ -270,13 +271,19 @@ public static function getMembership(EntityInterface $group, AccountInterface $u $group->bundle(), OgRoleInterface::ANONYMOUS, ]); - $storage = \Drupal::entityTypeManager()->getStorage('og_membership'); - return $storage->create(['type' => OgMembershipInterface::TYPE_DEFAULT]) - ->setUser($user->id()) - ->setEntityId($group->id()) - ->setGroupEntityType($group->getEntityTypeId()) + + $role = new OgRole([ + 'id' => $role_id, + 'role_type' => OgRoleInterface::ROLE_TYPE_REQUIRED, + 'label' => 'Non-member', + 'name' => OgRoleInterface::ANONYMOUS, + ]); + + return OgMembership::create() + ->setUser($user) + ->setGroup($group) ->setState(OgMembershipInterface::STATE_PENDING) - ->setRoles([$role_id]); + ->setRoles([$role]); } // No membership matches the request. diff --git a/tests/src/Unit/DefaultRoleEventTest.php b/tests/src/Unit/DefaultRoleEventTest.php index 29143f429..690ec33c0 100644 --- a/tests/src/Unit/DefaultRoleEventTest.php +++ b/tests/src/Unit/DefaultRoleEventTest.php @@ -538,7 +538,7 @@ protected function assertRoleEquals(OgRole $expected, OgRole $actual) { */ protected function expectOgRoleCreation(array &$roles) { foreach ($roles as &$properties) { - $role = new OgRole($properties, 'og_role'); + $role = new OgRole($properties); $properties = $role; } } From b03c5b77370584cc96f00c891bcac401fb0b34c9 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 2 Aug 2016 15:31:04 +0300 Subject: [PATCH 53/85] Fix PHP CodeSniffer warnings. --- src/Og.php | 2 ++ src/OgAccess.php | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Og.php b/src/Og.php index 7fe2874d4..a5563812f 100644 --- a/src/Og.php +++ b/src/Og.php @@ -249,12 +249,14 @@ public static function getMemberships(AccountInterface $user, array $states = [O * @param array $states * (optional) Array with the state to return. Defaults to active. * + * @codingStandardsIgnoreStart * @return \Drupal\og\Entity\OgMembership|NULL * The OgMembership entity. If the user is not a member of the group, an * OgMembership entity will be returned in which the user has the * 'non-member' role. * NULL will be returned if no membership is available that matches the * passed in $states. + * @codingStandardsIgnoreEnd */ public static function getMembership(EntityInterface $group, AccountInterface $user, array $states = [OgMembershipInterface::STATE_ACTIVE]) { foreach (static::getMemberships($user, $states) as $membership) { diff --git a/src/OgAccess.php b/src/OgAccess.php index 6476a57b3..4cbaf23a1 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -87,7 +87,7 @@ class OgAccess implements OgAccessInterface { * The service that contains the current active user. * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler * The module handler. - * @param \Drupal\og\GroupManager + * @param \Drupal\og\GroupManager $group_manager * The group manager. * @param \Drupal\og\PermissionManagerInterface $permission_manager * The permission manager. @@ -331,7 +331,7 @@ public function userAccessGroupContentEntityOperations($operation, EntityInterfa // @todo This doesn't really vary by user but by the user's role inside of // the group. Shall we make a cache context for OgRole entities? - $cacheable_metadata = new CacheableMetadata; + $cacheable_metadata = new CacheableMetadata(); $cacheable_metadata->addCacheableDependency($group_content_entity); if ($user->id() == $this->accountProxy->id()) { $cacheable_metadata->addCacheContexts(['user']); From fe9170da7c532226653e0b366fdfae142604f126 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 2 Aug 2016 15:36:31 +0300 Subject: [PATCH 54/85] Actually is null. --- src/Og.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Og.php b/src/Og.php index a5563812f..1e48d2409 100644 --- a/src/Og.php +++ b/src/Og.php @@ -249,14 +249,12 @@ public static function getMemberships(AccountInterface $user, array $states = [O * @param array $states * (optional) Array with the state to return. Defaults to active. * - * @codingStandardsIgnoreStart - * @return \Drupal\og\Entity\OgMembership|NULL + * @return \Drupal\og\Entity\OgMembership|null * The OgMembership entity. If the user is not a member of the group, an * OgMembership entity will be returned in which the user has the * 'non-member' role. * NULL will be returned if no membership is available that matches the * passed in $states. - * @codingStandardsIgnoreEnd */ public static function getMembership(EntityInterface $group, AccountInterface $user, array $states = [OgMembershipInterface::STATE_ACTIVE]) { foreach (static::getMemberships($user, $states) as $membership) { From 463ef9ff8051ca238e872427c155ba2a8503cd74 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 2 Aug 2016 15:48:08 +0300 Subject: [PATCH 55/85] Use the fullly qualified namespace for return values. --- src/Og.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Og.php b/src/Og.php index 1e48d2409..b4cfa9abf 100644 --- a/src/Og.php +++ b/src/Og.php @@ -11,7 +11,6 @@ use Drupal\field\FieldStorageConfigInterface; use Drupal\og\Entity\OgMembership; use Drupal\og\Entity\OgRole; -use Drupal\og\Plugin\EntityReferenceSelection\OgSelection; /** * A static helper class for OG. @@ -703,7 +702,7 @@ protected static function getFieldBaseDefinition($plugin_id) { * @param array $options * Overriding the default options of the selection handler. * - * @return OgSelection + * @return \Drupal\og\Plugin\EntityReferenceSelection\OgSelection * Returns the OG selection handler. * * @throws \Exception From 7a301837d638c30cb419f49f55617ed0ee43aaee Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 3 Aug 2016 12:15:44 +0300 Subject: [PATCH 56/85] Use shorthand array syntax. --- src/Entity/OgMembership.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Entity/OgMembership.php b/src/Entity/OgMembership.php index ed55b7422..eff20bcc2 100644 --- a/src/Entity/OgMembership.php +++ b/src/Entity/OgMembership.php @@ -196,7 +196,7 @@ public function getRoles() { /** * {@inheritdoc} */ - public function setRoles(array $roles = array()) { + public function setRoles(array $roles = []) { $role_ids = array_map(function (OgRole $role) { return $role->id(); }, $roles); From 70781653616dc984182fd0fbdd7b950b4a8f93a6 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Thu, 4 Aug 2016 08:18:17 +0300 Subject: [PATCH 57/85] Rename method. It only deals with a single operation at a time. --- src/OgAccess.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 4cbaf23a1..c13337a79 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -257,7 +257,7 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt foreach ($entity_groups as $group) { // Check if the operation matches a group content entity operation // such as 'create article content'. - $operation_access = $this->userAccessGroupContentEntityOperations($operation, $group, $entity, $user); + $operation_access = $this->userAccessGroupContentEntityOperation($operation, $group, $entity, $user); if ($operation_access->isAllowed()) { return $operation_access->addCacheTags($cache_tags); } @@ -307,7 +307,7 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt * * @see \Drupal\og\PermissionManager::getEntityOperationPermissions() */ - public function userAccessGroupContentEntityOperations($operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL) { + public function userAccessGroupContentEntityOperation($operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL) { // Default to the current user. if (!isset($user)) { $user = $this->accountProxy->getAccount(); From 878164fb6141a886d38a5deb00ea3d40d6fd34f3 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Thu, 4 Aug 2016 08:19:39 +0300 Subject: [PATCH 58/85] Improve documentation. --- src/OgAccess.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index c13337a79..74f9ad541 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -300,7 +300,8 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt * The group content entity for which access to the entity operation is * requested. * @param \Drupal\Core\Session\AccountInterface $user - * The user for which to check access. + * Optional user for which to check access. If omitted, the currently logged + * in user will be used. * * @return \Drupal\Core\Access\AccessResult * The access result object. From a42b96a38a46b0df61162429d44d5872f5d7c8f9 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Thu, 4 Aug 2016 08:21:54 +0300 Subject: [PATCH 59/85] Use the ternary operator instead of an if statement. --- src/OgAccess.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 74f9ad541..13d51d47a 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -310,9 +310,7 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt */ public function userAccessGroupContentEntityOperation($operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL) { // Default to the current user. - if (!isset($user)) { - $user = $this->accountProxy->getAccount(); - } + $user = $user ?: $this->accountProxy->getAccount(); // Check if the user owns the entity which is being operated on. $is_owner = $group_content_entity instanceof EntityOwnerInterface && $group_content_entity->getOwnerId() == $user->id(); From c63d48bfb2fc895bcb5e4bf77d156f3391654a9d Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Thu, 4 Aug 2016 08:28:11 +0300 Subject: [PATCH 60/85] Improve documentation. --- src/OgAccess.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 13d51d47a..12d24b46f 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -315,7 +315,7 @@ public function userAccessGroupContentEntityOperation($operation, EntityInterfac // Check if the user owns the entity which is being operated on. $is_owner = $group_content_entity instanceof EntityOwnerInterface && $group_content_entity->getOwnerId() == $user->id(); - // Retrieve the permissions and check if our operation is supported. + // Retrieve the group content entity operation permissions. $group_entity_type_id = $group_entity->getEntityTypeId(); $group_bundle_id = $group_entity->bundle(); $group_content_bundle_ids = [$group_content_entity->getEntityTypeId() => [$group_content_entity->bundle()]]; @@ -323,6 +323,11 @@ public function userAccessGroupContentEntityOperation($operation, EntityInterfac $permissions = $this->permissionManager->getDefaultEntityOperationPermissions($group_entity_type_id, $group_bundle_id, $group_content_bundle_ids); // Filter the permissions by operation and ownership. + // If the user does not own the group content, only the non-owner permission + // is relevant (for example 'edit any article node'). However when the user + // _is_ the owner, then both permissions are relevant: an owner will have + // access if they either have the 'edit any article node' or the 'edit own + // article node' permission. $ownerships = $is_owner ? [FALSE, TRUE] : [FALSE]; $permissions = array_filter($permissions, function (GroupContentOperationPermission $permission) use ($operation, $ownerships) { return $permission->getOperation() === $operation && in_array($permission->getOwner(), $ownerships); From 74dac2940bd30aca00777f1a58d3f82e1808eba7 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Thu, 4 Aug 2016 14:42:06 +0300 Subject: [PATCH 61/85] Rename $user to $membership_type since this better conveys what the variable holds. --- tests/src/Functional/AccessTest.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/src/Functional/AccessTest.php b/tests/src/Functional/AccessTest.php index 34cbaa615..fead66167 100644 --- a/tests/src/Functional/AccessTest.php +++ b/tests/src/Functional/AccessTest.php @@ -54,7 +54,7 @@ public function setUp() { parent::setUp(); // Create a number of test users. - $users = [ + $membership_types = [ // The group owner. 'owner' => ['access content'], // A site administrator with the right to administer all groups. @@ -64,8 +64,8 @@ public function setUp() { // A user that is not a member of the group. 'non-member' => ['access content'], ]; - foreach ($users as $user => $permissions) { - $this->users[$user] = $this->drupalCreateUser($permissions, $user); + foreach ($membership_types as $membership_type => $permissions) { + $this->users[$membership_type] = $this->drupalCreateUser($permissions, $membership_type); } // Create a "group" bundle on the Custom Block entity type and turn it into @@ -113,14 +113,14 @@ public function setUp() { // Create two group content items, one owned by the group owner, and one by // the member. - foreach (['owner', 'member'] as $user) { - $this->groupContent[$user] = Node::create([ + foreach (['owner', 'member'] as $membership_type) { + $this->groupContent[$membership_type] = Node::create([ 'title' => $this->randomString(), 'type' => 'group_content', - 'uid' => $this->users[$user]->id(), + 'uid' => $this->users[$membership_type]->id(), OgGroupAudienceHelper::DEFAULT_FIELD => [['target_id' => $this->group->id()]], ]); - $this->groupContent[$user]->save(); + $this->groupContent[$membership_type]->save(); } } From 21283865c545169e5db09f3ca543875ece18f717 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Thu, 4 Aug 2016 14:42:43 +0300 Subject: [PATCH 62/85] Rename 'newsletter_subscription' to 'newsletter'. Shorter is gooder. --- .../OgGroupContentOperationAccessTest.php | 47 +++++++++---------- 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php b/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php index fcbc69db8..6b307d135 100644 --- a/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php +++ b/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php @@ -120,21 +120,21 @@ protected function setUp() { // Create 3 test roles with associated permissions. We will simulate a // project that has two group content types: - // - 'newsletter_subscription': Any registered user can create entities of - // this type, even if they are not a member of the group. + // - 'newsletter': Any registered user can create entities of this type, + // even if they are not a member of the group. // - 'article': These can only be created by group members. Normal members // can edit and delete their own articles, while admins can edit and // delete any article. $permission_matrix = [ OgRoleInterface::ANONYMOUS => [ - 'create newsletter_subscription comment', - 'update own newsletter_subscription comment', - 'delete own newsletter_subscription comment', + 'create newsletter comment', + 'update own newsletter comment', + 'delete own newsletter comment', ], OgRoleInterface::AUTHENTICATED => [ - 'create newsletter_subscription comment', - 'update own newsletter_subscription comment', - 'delete own newsletter_subscription comment', + 'create newsletter comment', + 'update own newsletter comment', + 'delete own newsletter comment', 'create article content', 'edit own article content', 'delete own article content', @@ -143,9 +143,9 @@ protected function setUp() { // delete their own group content. Having the 'any' permission should be // sufficient to also be able to edit their own content. OgRoleInterface::ADMINISTRATOR => [ - 'create newsletter_subscription comment', - 'update any newsletter_subscription comment', - 'delete any newsletter_subscription comment', + 'create newsletter comment', + 'update any newsletter comment', + 'delete any newsletter comment', 'create article content', 'edit any article content', 'delete any article content', @@ -181,14 +181,14 @@ protected function setUp() { } } - // Create a 'newsletter_subscription' group content type. We are using the - // Comment entity for this to verify that this functionality works for all - // entity types. We cannot use the 'entity_test' entity for this since it - // has no support for bundles. Let's imagine that we have a use case where - // the user can leave a comment with the text 'subscribe' in order to - // subscribe to the newsletter. + // Create a 'newsletter' group content type. We are using the Comment entity + // for this to verify that this functionality works for all entity types. We + // cannot use the 'entity_test' entity for this since it has no support for + // bundles. Let's imagine that we have a use case where the user can leave a + // comment with the text 'subscribe' in order to subscribe to the + // newsletter. CommentType::create([ - 'id' => 'newsletter_subscription', + 'id' => 'newsletter', 'label' => 'Newsletter subscription', 'target_entity_type_id' => 'entity_test', ])->save(); @@ -199,7 +199,7 @@ protected function setUp() { ], ], ]; - Og::createField(OgGroupAudienceHelper::DEFAULT_FIELD, 'comment', 'newsletter_subscription', $settings); + Og::createField(OgGroupAudienceHelper::DEFAULT_FIELD, 'comment', 'newsletter', $settings); // Create an 'article' group content type. NodeType::create([ @@ -209,7 +209,7 @@ protected function setUp() { Og::createField(OgGroupAudienceHelper::DEFAULT_FIELD, 'node', 'article', $settings); // Create a group content entity owned by each test user, for both the - // 'newsletter_subscription' and 'article' bundles. + // 'newsletter' and 'article' bundles. $user_ids = [ 'uid1', 'group_owner', @@ -217,7 +217,7 @@ protected function setUp() { OgRoleInterface::AUTHENTICATED, OgRoleInterface::ADMINISTRATOR, ]; - foreach (['newsletter_subscription', 'article'] as $bundle_id) { + foreach (['newsletter', 'article'] as $bundle_id) { foreach ($user_ids as $user_id) { $entity_type = $bundle_id === 'article' ? 'node' : 'comment'; @@ -281,8 +281,7 @@ public function testAccess($group_content_entity_type_id, $group_content_bundle_ * - A string representing the group content entity type ID upon which the * operation is performed. Can be either 'node' or 'comment'. * - A string representing the group content bundle ID upon which the - * operation is performed. Can be either 'newsletter_subscription' or - * 'article'. + * operation is performed. Can be either 'newsletter' or 'article'. * - An array mapping the different users and the possible operations, and * whether or not the user is expected to be granted access to this * operation, depending on whether they own the content or not. @@ -291,7 +290,7 @@ public function accessProvider() { return [ [ 'comment', - 'newsletter_subscription', + 'newsletter', [ // The super user and the administrator have the right to create, // update and delete any newsletter subscription. From 1fbba71c2db27f36feaa96d7b06c5fd19d5a4699 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Thu, 4 Aug 2016 14:43:23 +0300 Subject: [PATCH 63/85] Provide an issue number with a @todo so it becomes actionable. --- src/OgAccess.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 12d24b46f..a46535aff 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -334,7 +334,8 @@ public function userAccessGroupContentEntityOperation($operation, EntityInterfac }); // @todo This doesn't really vary by user but by the user's role inside of - // the group. Shall we make a cache context for OgRole entities? + // the group. We should create a cache context for OgRole entities. + // @see https://github.com/amitaibu/og/issues/219 $cacheable_metadata = new CacheableMetadata(); $cacheable_metadata->addCacheableDependency($group_content_entity); if ($user->id() == $this->accountProxy->id()) { From 8dd3c9b7c881e541b2fb7671ad35707254470aaa Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Thu, 4 Aug 2016 15:10:15 +0300 Subject: [PATCH 64/85] Adopt the awesome new DX improvements for creating memberships. --- tests/src/Functional/AccessTest.php | 9 ++++----- .../Kernel/Access/OgGroupContentOperationAccessTest.php | 9 ++++----- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/tests/src/Functional/AccessTest.php b/tests/src/Functional/AccessTest.php index fead66167..fc4692917 100644 --- a/tests/src/Functional/AccessTest.php +++ b/tests/src/Functional/AccessTest.php @@ -103,12 +103,11 @@ public function setUp() { // Subscribe the member to the group. /** @var \Drupal\og\Entity\OgMembership $membership */ - $membership = OgMembership::create(['type' => OgMembershipInterface::TYPE_DEFAULT]); + $membership = OgMembership::create(); $membership - ->setUser($this->users['member']->id()) - ->setEntityId($this->group->id()) - ->setGroupEntityType($this->group->getEntityTypeId()) - ->addRole($role->id()) + ->setUser($this->users['member']) + ->setGroup($this->group) + ->addRole($role) ->save(); // Create two group content items, one owned by the group owner, and one by diff --git a/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php b/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php index 6b307d135..bd2b59295 100644 --- a/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php +++ b/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php @@ -170,12 +170,11 @@ protected function setUp() { // membership will not exist in the database. if ($role_name !== OgRoleInterface::ANONYMOUS) { /** @var OgMembership $membership */ - $membership = OgMembership::create(['type' => OgMembershipInterface::TYPE_DEFAULT]); + $membership = OgMembership::create(); $membership - ->setUser($this->users[$role_name]->id()) - ->setEntityId($this->group->id()) - ->setGroupEntityType($this->group->getEntityTypeId()) - ->addRole($this->roles[$role_name]->id()) + ->setUser($this->users[$role_name]) + ->setGroup($this->group) + ->addRole($this->roles[$role_name]) ->setState(OgMembershipInterface::STATE_ACTIVE) ->save(); } From 7796f5aa19075444e210b9e3a820f14a5cacdd5c Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Thu, 4 Aug 2016 15:17:43 +0300 Subject: [PATCH 65/85] Remove unused use statement. --- tests/src/Functional/AccessTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/src/Functional/AccessTest.php b/tests/src/Functional/AccessTest.php index fc4692917..178541298 100644 --- a/tests/src/Functional/AccessTest.php +++ b/tests/src/Functional/AccessTest.php @@ -9,7 +9,6 @@ use Drupal\og\Entity\OgMembership; use Drupal\og\Og; use Drupal\og\OgGroupAudienceHelper; -use Drupal\og\OgMembershipInterface; use Drupal\simpletest\ContentTypeCreationTrait; /** From 6f649860bde092f675bdef1e970f6fa3806e4f5c Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Fri, 5 Aug 2016 10:42:19 +0300 Subject: [PATCH 66/85] Change the membership state constants to strings. This makes it clearer what each state represents in exported config. --- src/OgMembershipInterface.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/OgMembershipInterface.php b/src/OgMembershipInterface.php index 273d6fc79..d194fd278 100644 --- a/src/OgMembershipInterface.php +++ b/src/OgMembershipInterface.php @@ -18,7 +18,7 @@ interface OgMembershipInterface extends ContentEntityInterface { * When a user has this membership state they are considered to be of * "member" role. */ - const STATE_ACTIVE = 1; + const STATE_ACTIVE = 'active'; /** * Define pending group content states. @@ -28,7 +28,7 @@ interface OgMembershipInterface extends ContentEntityInterface { * When a user has this membership state they are considered to be of * "non-member" role. */ - const STATE_PENDING = 2; + const STATE_PENDING = 'pending'; /** * Define blocked group content states. The user is rejected from the group. @@ -37,7 +37,7 @@ interface OgMembershipInterface extends ContentEntityInterface { * group related action. This state, however, does not prevent user to * access a group or group content node. */ - const STATE_BLOCKED = 3; + const STATE_BLOCKED = 'blocked'; /** * The default group membership type that is the bundle of group membership. From 2f2caa7ea06f6ccb0729a32014cf092948961c2f Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Fri, 5 Aug 2016 10:43:19 +0300 Subject: [PATCH 67/85] Small documentation update. --- src/OgMembershipInterface.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/OgMembershipInterface.php b/src/OgMembershipInterface.php index d194fd278..c4f515263 100644 --- a/src/OgMembershipInterface.php +++ b/src/OgMembershipInterface.php @@ -34,8 +34,8 @@ interface OgMembershipInterface extends ContentEntityInterface { * Define blocked group content states. The user is rejected from the group. * * When a user has this membership state they are denied access to any - * group related action. This state, however, does not prevent user to - * access a group or group content node. + * group related action. This state, however, does not prevent the user from + * accessing a group or group content. */ const STATE_BLOCKED = 'blocked'; From a841f2c40a4e8c96a0a4a6df5b5604490b9dee62 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Fri, 5 Aug 2016 11:50:18 +0300 Subject: [PATCH 68/85] Test that blocked users cannot create, update or delete any group content. --- tests/src/Functional/AccessTest.php | 39 ++++++++++++------- .../OgGroupContentOperationAccessTest.php | 27 +++++++++++++ 2 files changed, 51 insertions(+), 15 deletions(-) diff --git a/tests/src/Functional/AccessTest.php b/tests/src/Functional/AccessTest.php index 178541298..636552f45 100644 --- a/tests/src/Functional/AccessTest.php +++ b/tests/src/Functional/AccessTest.php @@ -2,6 +2,7 @@ namespace Drupal\Tests\og\Functional; +use Drupal\og\OgMembershipInterface; use Drupal\Tests\BrowserTestBase; use Drupal\block_content\Entity\BlockContent; use Drupal\block_content\Entity\BlockContentType; @@ -62,6 +63,8 @@ public function setUp() { 'member' => ['access content'], // A user that is not a member of the group. 'non-member' => ['access content'], + // A blocked user. + 'blocked' => ['access content'], ]; foreach ($membership_types as $membership_type => $permissions) { $this->users[$membership_type] = $this->drupalCreateUser($permissions, $membership_type); @@ -100,18 +103,22 @@ public function setUp() { $role->grantPermission('edit own group_content content'); $role->save(); - // Subscribe the member to the group. - /** @var \Drupal\og\Entity\OgMembership $membership */ - $membership = OgMembership::create(); - $membership - ->setUser($this->users['member']) - ->setGroup($this->group) - ->addRole($role) - ->save(); - - // Create two group content items, one owned by the group owner, and one by - // the member. - foreach (['owner', 'member'] as $membership_type) { + // Subscribe the normal member and the blocked member to the group. + foreach (['member', 'blocked'] as $membership_type) { + $state = $membership_type === 'member' ? OgMembershipInterface::STATE_ACTIVE : OgMembershipInterface::STATE_BLOCKED; + /** @var \Drupal\og\Entity\OgMembership $membership */ + $membership = OgMembership::create(); + $membership + ->setUser($this->users[$membership_type]) + ->setGroup($this->group) + ->addRole($role) + ->setState($state) + ->save(); + } + + // Create three group content items, one owned by the group owner, one by + // the member, and one by the blocked user. + foreach (['owner', 'member', 'blocked'] as $membership_type) { $this->groupContent[$membership_type] = Node::create([ 'title' => $this->randomString(), 'type' => 'group_content', @@ -132,11 +139,13 @@ public function testEntityOperationAccess() { $test_matrix = [ // The administrator should have the right to edit both group content // items. - 'group-admin' => ['owner' => TRUE, 'member' => TRUE], + 'group-admin' => ['owner' => TRUE, 'member' => TRUE, 'blocked' => TRUE], // The member should only have the right to edit their own group content. - 'member' => ['owner' => FALSE, 'member' => TRUE], + 'member' => ['owner' => FALSE, 'member' => TRUE, 'blocked' => FALSE], // The non-member cannot edit any group content. - 'non-member' => ['owner' => FALSE, 'member' => FALSE], + 'non-member' => ['owner' => FALSE, 'member' => FALSE, 'blocked' => FALSE], + // The blocked member cannot edit any group content, not even their own. + 'blocked' => ['owner' => FALSE, 'member' => FALSE, 'blocked' => FALSE], ]; foreach ($test_matrix as $user => $expected_results) { $this->drupalLogin($this->users[$user]); diff --git a/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php b/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php index bd2b59295..87faa6336 100644 --- a/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php +++ b/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php @@ -180,6 +180,18 @@ protected function setUp() { } } + // Create a 'blocked' user. This user is identical to the normal + // 'authenticated' member, except that she has the 'blocked' state. + $this->users['blocked'] = User::create(['name' => $this->randomString()]); + $this->users['blocked']->save(); + $membership = OgMembership::create(); + $membership + ->setUser($this->users['blocked']) + ->setGroup($this->group) + ->addRole($this->roles[OgRoleInterface::AUTHENTICATED]) + ->setState(OgMembershipInterface::STATE_BLOCKED) + ->save(); + // Create a 'newsletter' group content type. We are using the Comment entity // for this to verify that this functionality works for all entity types. We // cannot use the 'entity_test' entity for this since it has no support for @@ -215,6 +227,7 @@ protected function setUp() { OgRoleInterface::ANONYMOUS, OgRoleInterface::AUTHENTICATED, OgRoleInterface::ADMINISTRATOR, + 'blocked' ]; foreach (['newsletter', 'article'] as $bundle_id) { foreach ($user_ids as $user_id) { @@ -316,6 +329,13 @@ public function accessProvider() { 'update' => ['own' => TRUE, 'any' => FALSE], 'delete' => ['own' => TRUE, 'any' => FALSE], ], + // Blocked users cannot do anything, not even update or delete their + // own content. + 'blocked' => [ + 'create' => ['any' => FALSE], + 'update' => ['own' => FALSE, 'any' => FALSE], + 'delete' => ['own' => FALSE, 'any' => FALSE], + ], ], ], [ @@ -347,6 +367,13 @@ public function accessProvider() { 'update' => ['own' => TRUE, 'any' => FALSE], 'delete' => ['own' => TRUE, 'any' => FALSE], ], + // Blocked users cannot do anything, not even update or delete their + // own content. + 'blocked' => [ + 'create' => ['any' => FALSE], + 'update' => ['own' => FALSE, 'any' => FALSE], + 'delete' => ['own' => FALSE, 'any' => FALSE], + ], ], ], ]; From b2e37e81d6ea5b101ad35157372cdfdddd367d75 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Fri, 5 Aug 2016 11:56:43 +0300 Subject: [PATCH 69/85] Fix PHP CodeSniffer warning. --- tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php b/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php index 87faa6336..fc566441d 100644 --- a/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php +++ b/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php @@ -227,7 +227,7 @@ protected function setUp() { OgRoleInterface::ANONYMOUS, OgRoleInterface::AUTHENTICATED, OgRoleInterface::ADMINISTRATOR, - 'blocked' + 'blocked', ]; foreach (['newsletter', 'article'] as $bundle_id) { foreach ($user_ids as $user_id) { From 37d221609408f61f113baae24cafe66d10cde1e3 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Fri, 5 Aug 2016 12:03:37 +0300 Subject: [PATCH 70/85] Update the membership state field definition, it has changed from integer to string values. --- src/Entity/OgMembership.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Entity/OgMembership.php b/src/Entity/OgMembership.php index f231b5aaf..d02a7c3a6 100644 --- a/src/Entity/OgMembership.php +++ b/src/Entity/OgMembership.php @@ -255,10 +255,10 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) { ->setLabel(t('Group entity id.')) ->setDescription(t("The entity ID of the group.")); - $fields['state'] = BaseFieldDefinition::create('integer') + $fields['state'] = BaseFieldDefinition::create('string') ->setLabel(t('State')) - ->setDescription(t("The state of the group content.")) - ->setDefaultValue(TRUE); + ->setDescription(t('The user membership state: active, pending, or blocked.')) + ->setDefaultValue(OgMembershipInterface::STATE_ACTIVE); $fields['roles'] = BaseFieldDefinition::create('entity_reference') ->setLabel(t('Roles')) From 2a67361599760e1123e59751e241afcb592a0df2 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Fri, 5 Aug 2016 12:44:18 +0300 Subject: [PATCH 71/85] Move the generation of the 'non-member membership' to the calling side. --- src/Og.php | 30 ++---------------------------- src/OgAccess.php | 45 +++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 43 insertions(+), 32 deletions(-) diff --git a/src/Og.php b/src/Og.php index e6e4d5630..986f85ea5 100644 --- a/src/Og.php +++ b/src/Og.php @@ -249,11 +249,8 @@ public static function getMemberships(AccountInterface $user, array $states = [O * (optional) Array with the state to return. Defaults to active. * * @return \Drupal\og\Entity\OgMembership|null - * The OgMembership entity. If the user is not a member of the group, an - * OgMembership entity will be returned in which the user has the - * 'non-member' role. - * NULL will be returned if no membership is available that matches the - * passed in $states. + * The OgMembership entity. NULL will be returned if no membership is + * available that matches the passed in $states. */ public static function getMembership(EntityInterface $group, AccountInterface $user, array $states = [OgMembershipInterface::STATE_ACTIVE]) { foreach (static::getMemberships($user, $states) as $membership) { @@ -262,29 +259,6 @@ public static function getMembership(EntityInterface $group, AccountInterface $u } } - // The user is not a member, return an OgMembership entity in which the user - // has the 'non-member' role. - if (in_array(OgMembershipInterface::STATE_PENDING, $states)) { - $role_id = implode('-', [ - $group->getEntityTypeId(), - $group->bundle(), - OgRoleInterface::ANONYMOUS, - ]); - - $role = new OgRole([ - 'id' => $role_id, - 'role_type' => OgRoleInterface::ROLE_TYPE_REQUIRED, - 'label' => 'Non-member', - 'name' => OgRoleInterface::ANONYMOUS, - ]); - - return OgMembership::create() - ->setUser($user) - ->setGroup($group) - ->setState(OgMembershipInterface::STATE_PENDING) - ->setRoles([$role]); - } - // No membership matches the request. return NULL; } diff --git a/src/OgAccess.php b/src/OgAccess.php index a46535aff..f1e11d4ca 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -10,6 +10,8 @@ use Drupal\Core\Extension\ModuleHandlerInterface; use Drupal\Core\Session\AccountInterface; use Drupal\Core\Session\AccountProxyInterface; +use Drupal\og\Entity\OgMembership; +use Drupal\og\Entity\OgRole; use Drupal\user\EntityOwnerInterface; /** @@ -343,10 +345,16 @@ public function userAccessGroupContentEntityOperation($operation, EntityInterfac } if ($permissions) { - // Non-members might also have certain permissions within a group. Request - // both memberships of members (represented by STATE_ACTIVE) and - // non-members (STATE_PENDING). - $membership = Og::getMembership($group_entity, $user, [OgMembershipInterface::STATE_ACTIVE, OgMembershipInterface::STATE_PENDING]); + // Request the user's membership. If the user is not a member, construct a + // 'non-member membership' in which the user has the 'anonymous' role. We + // can then use this to retrieve the permissions that the user has within + // the group, regardless if they are a member or not. + $states = [ + OgMembershipInterface::STATE_ACTIVE, + OgMembershipInterface::STATE_PENDING, + OgMembershipInterface::STATE_BLOCKED, + ]; + $membership = Og::getMembership($group_entity, $user, $states) ?: $this->getNonMemberMembership($group_entity, $user); foreach ($permissions as $permission) { if ($membership->hasPermission($permission->getName())) { return AccessResult::allowed()->addCacheableDependency($cacheable_metadata); @@ -415,4 +423,33 @@ public function reset() { $this->permissionsCache = []; } + /** + * Returns a group membership in which the user is not a member. + * + * This is intended to be used to retrieve the permissions a non-member has in + * the given group. Note that this membership is created on the fly and is not + * saved. + * + * @param \Drupal\Core\Entity\EntityInterface $group + * The group to get the membership for. + * @param \Drupal\Core\Session\AccountInterface $user + * The user to get the membership for. + * + * @return \Drupal\og\Entity\OgMembership + * The OgMembership entity. + */ + protected static function getNonMemberMembership(EntityInterface $group, AccountInterface $user) { + $role_id = implode('-', [ + $group->getEntityTypeId(), + $group->bundle(), + OgRoleInterface::ANONYMOUS, + ]); + + return OgMembership::create() + ->setUser($user) + ->setGroup($group) + ->setState(OgMembershipInterface::STATE_PENDING) + ->setRoles([new OgRole(['id' => $role_id])]); + } + } From f6c5af01fb70c605521e8a3172a74d204a592e9a Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Fri, 5 Aug 2016 12:44:37 +0300 Subject: [PATCH 72/85] Blocked users should not have any permissions. --- src/Entity/OgMembership.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Entity/OgMembership.php b/src/Entity/OgMembership.php index d02a7c3a6..1505b1602 100644 --- a/src/Entity/OgMembership.php +++ b/src/Entity/OgMembership.php @@ -215,6 +215,11 @@ public function getRolesIds() { * {@inheritdoc} */ public function hasPermission($permission) { + // Blocked users do not have any permissions. + if ($this->getState() === OgMembershipInterface::STATE_BLOCKED) { + return FALSE; + } + return array_filter($this->getRoles(), function (OgRole $role) use ($permission) { return $role->hasPermission($permission); }); From 774e5c9403035eacc16b88aff352e79b1ce79e18 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Fri, 5 Aug 2016 15:33:49 +0300 Subject: [PATCH 73/85] We don't care if a group is new when doing access checks. --- src/OgAccess.php | 5 ----- tests/src/Unit/OgAccessEntityTest.php | 13 ------------- 2 files changed, 18 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index f1e11d4ca..bbe39f4fb 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -227,11 +227,6 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt $bundle = $entity->bundle(); if ($this->groupManager->isGroup($entity_type_id, $bundle)) { - // Entity isn't saved yet. - if ($entity->isNew()) { - return $result->addCacheableDependency($entity); - } - $user_access = $this->userAccess($entity, $operation, $user); if ($user_access->isAllowed()) { return $user_access; diff --git a/tests/src/Unit/OgAccessEntityTest.php b/tests/src/Unit/OgAccessEntityTest.php index 12bdd27c3..4f2d31595 100644 --- a/tests/src/Unit/OgAccessEntityTest.php +++ b/tests/src/Unit/OgAccessEntityTest.php @@ -30,19 +30,6 @@ public function testAccessByOperation($operation) { $this->assertTrue($condition); } - /** - * Tests access to an un-saved entity. - * - * @coversDefaultmethod ::userAccessEntity - * @dataProvider permissionsProvider - */ - public function testEntityNew($operation) { - $group_entity = $this->groupEntity(); - $group_entity->isNew()->willReturn(TRUE); - $user_access = $this->ogAccess->userAccessEntity($operation, $group_entity->reveal(), $this->user->reveal()); - $this->assertTrue($user_access->isNeutral()); - } - /** * Tests getting a user's group entities. * From 370d543b2f94a847cce4e3c2a81fc4976bd99b3e Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Fri, 5 Aug 2016 17:32:37 +0300 Subject: [PATCH 74/85] Convert the functional access test into a kernel test. --- tests/src/Functional/AccessTest.php | 160 ------------ tests/src/Kernel/Access/OgAccessHookTest.php | 255 +++++++++++++++++++ 2 files changed, 255 insertions(+), 160 deletions(-) delete mode 100644 tests/src/Functional/AccessTest.php create mode 100644 tests/src/Kernel/Access/OgAccessHookTest.php diff --git a/tests/src/Functional/AccessTest.php b/tests/src/Functional/AccessTest.php deleted file mode 100644 index 636552f45..000000000 --- a/tests/src/Functional/AccessTest.php +++ /dev/null @@ -1,160 +0,0 @@ - ['access content'], - // A site administrator with the right to administer all groups. - 'group-admin' => ['administer group', 'access content'], - // A regular member of the group. - 'member' => ['access content'], - // A user that is not a member of the group. - 'non-member' => ['access content'], - // A blocked user. - 'blocked' => ['access content'], - ]; - foreach ($membership_types as $membership_type => $permissions) { - $this->users[$membership_type] = $this->drupalCreateUser($permissions, $membership_type); - } - - // Create a "group" bundle on the Custom Block entity type and turn it into - // a group. Note we're not using the Entity Test entity for this since it - // does not have real support for multiple bundles. - BlockContentType::create(['type' => 'group']); - Og::groupManager()->addGroup('block_content', 'group'); - - // Create a group. - $this->group = BlockContent::create([ - 'title' => $this->randomString(), - 'type' => 'group', - 'uid' => $this->users['owner']->id(), - ]); - $this->group->save(); - - // Create a group content type. - $this->createContentType(['type' => 'group_content']); - $settings = [ - 'field_storage_config' => [ - 'settings' => [ - 'target_type' => 'block_content', - ], - ], - ]; - Og::createField(OgGroupAudienceHelper::DEFAULT_FIELD, 'node', 'group_content', $settings); - - // Grant members permission to edit their own content. - /** @var \Drupal\og\Entity\OgRole $role */ - $role = $this->container->get('entity_type.manager') - ->getStorage('og_role') - ->load('block_content-group-member'); - $role->grantPermission('edit own group_content content'); - $role->save(); - - // Subscribe the normal member and the blocked member to the group. - foreach (['member', 'blocked'] as $membership_type) { - $state = $membership_type === 'member' ? OgMembershipInterface::STATE_ACTIVE : OgMembershipInterface::STATE_BLOCKED; - /** @var \Drupal\og\Entity\OgMembership $membership */ - $membership = OgMembership::create(); - $membership - ->setUser($this->users[$membership_type]) - ->setGroup($this->group) - ->addRole($role) - ->setState($state) - ->save(); - } - - // Create three group content items, one owned by the group owner, one by - // the member, and one by the blocked user. - foreach (['owner', 'member', 'blocked'] as $membership_type) { - $this->groupContent[$membership_type] = Node::create([ - 'title' => $this->randomString(), - 'type' => 'group_content', - 'uid' => $this->users[$membership_type]->id(), - OgGroupAudienceHelper::DEFAULT_FIELD => [['target_id' => $this->group->id()]], - ]); - $this->groupContent[$membership_type]->save(); - } - } - - /** - * Tests access to entity operations through the user interface. - * - * Note that this does not use a dataProvider because of the slow performance - * of setting up functional tests. - */ - public function testEntityOperationAccess() { - $test_matrix = [ - // The administrator should have the right to edit both group content - // items. - 'group-admin' => ['owner' => TRUE, 'member' => TRUE, 'blocked' => TRUE], - // The member should only have the right to edit their own group content. - 'member' => ['owner' => FALSE, 'member' => TRUE, 'blocked' => FALSE], - // The non-member cannot edit any group content. - 'non-member' => ['owner' => FALSE, 'member' => FALSE, 'blocked' => FALSE], - // The blocked member cannot edit any group content, not even their own. - 'blocked' => ['owner' => FALSE, 'member' => FALSE, 'blocked' => FALSE], - ]; - foreach ($test_matrix as $user => $expected_results) { - $this->drupalLogin($this->users[$user]); - foreach ($expected_results as $group_content => $expected_result) { - $this->drupalGet($this->groupContent[$group_content]->toUrl('edit-form')); - $expected_response = $expected_result ? 200 : 403; - $this->assertSession()->statusCodeEquals($expected_response); - } - } - } - -} diff --git a/tests/src/Kernel/Access/OgAccessHookTest.php b/tests/src/Kernel/Access/OgAccessHookTest.php new file mode 100644 index 000000000..3a2760efc --- /dev/null +++ b/tests/src/Kernel/Access/OgAccessHookTest.php @@ -0,0 +1,255 @@ +installConfig(['og']); + $this->installEntitySchema('block_content'); + $this->installEntitySchema('node'); + $this->installEntitySchema('og_membership'); + $this->installEntitySchema('user'); + $this->installSchema('system', 'sequences'); + + // Create two roles: one for normal users, and one for administrators. + foreach (['authenticated', 'administrator'] as $role_id) { + $role = Role::create([ + 'id' => $role_id, + 'label' => $role_id, + ]); + $role->grantPermission('access content'); + + // Grant the 'administer group' permission to the administrator role. + if ($role_id === 'administrator') { + $role->grantPermission('administer group'); + } + $role->save(); + $this->roles[$role_id] = $role; + } + + // Create a test user for each membership type. + $membership_types = [ + // The group owner. + 'owner', + // A site administrator with the right to administer all groups. + 'group-admin', + // A regular member of the group. + 'member', + // A user that is not a member of the group. + 'non-member', + // A blocked user. + 'blocked', + ]; + foreach ($membership_types as $membership_type) { + $user = User::create([ + 'name' => $membership_type, + ]); + // Grant the 'administrator' role to the group administrator. + if ($membership_type === 'group-admin') { + $user->addRole('administrator'); + } + $user->save(); + $this->users[$membership_type] = $user; + } + + // Create a "group" bundle on the Custom Block entity type and turn it into + // a group. Note we're not using the Entity Test entity for this since it + // does not have real support for multiple bundles. + BlockContentType::create(['type' => 'group']); + Og::groupManager()->addGroup('block_content', 'group'); + + // Create a group. + $this->group = BlockContent::create([ + 'title' => $this->randomString(), + 'type' => 'group', + 'uid' => $this->users['owner']->id(), + ]); + $this->group->save(); + + // Create a group content type. + $type = NodeType::create([ + 'type' => 'group_content', + 'name' => $this->randomString() + ]); + $type->save(); + $settings = [ + 'field_storage_config' => [ + 'settings' => [ + 'target_type' => 'block_content', + ], + ], + ]; + Og::createField(OgGroupAudienceHelper::DEFAULT_FIELD, 'node', 'group_content', $settings); + + // Grant members permission to edit their own content. + /** @var \Drupal\og\Entity\OgRole $role */ + $role = $this->container->get('entity_type.manager') + ->getStorage('og_role') + ->load('block_content-group-member'); + $role->grantPermission('edit own group_content content'); + $role->save(); + + // Subscribe the normal member and the blocked member to the group. + foreach (['member', 'blocked'] as $membership_type) { + $state = $membership_type === 'member' ? OgMembershipInterface::STATE_ACTIVE : OgMembershipInterface::STATE_BLOCKED; + /** @var \Drupal\og\Entity\OgMembership $membership */ + $membership = OgMembership::create(); + $membership + ->setUser($this->users[$membership_type]) + ->setGroup($this->group) + ->addRole($role) + ->setState($state) + ->save(); + } + + // Create three group content items, one owned by the group owner, one by + // the member, and one by the blocked user. + foreach (['owner', 'member', 'blocked'] as $membership_type) { + $this->groupContent[$membership_type] = Node::create([ + 'title' => $this->randomString(), + 'type' => 'group_content', + 'uid' => $this->users[$membership_type]->id(), + OgGroupAudienceHelper::DEFAULT_FIELD => [['target_id' => $this->group->id()]], + ]); + $this->groupContent[$membership_type]->save(); + } + } + + /** + * Tests access to entity operations through the access hook. + * + * @param string $user + * The name of the user to test. + * @param array $expected_results + * An associative array indicating whether the user should have the right to + * edit content owned by the user represented by the array key. + * + * @dataProvider entityOperationAccessProvider + */ + public function testEntityOperationAccess($user, array $expected_results) { + foreach ($expected_results as $group_content => $expected_result) { + /** @var \Drupal\Core\Access\AccessResult $result */ + $result = og_entity_access($this->groupContent[$group_content], 'update', $this->users[$user]); + $this->assertEquals($expected_result, $result->isAllowed()); + } + } + + /** + * Data provider for ::testEntityOperationAccess(). + * + * @return array + * And array of test data sets. Each set consisting of: + * - The name of the user to test. + * - An associative array indicating whether the user should have the right + * to edit content owned by the user represented by the array key. + */ + public function entityOperationAccessProvider() { + return [ + [ + // The administrator should have the right to edit group content items + // owned by any user. + 'group-admin', + [ + 'owner' => TRUE, + 'member' => TRUE, + 'blocked' => TRUE, + ], + ], + [ + // Members should only have the right to edit their own group content. + 'member', + [ + 'owner' => FALSE, + 'member' => TRUE, + 'blocked' => FALSE, + ], + ], + [ + // The non-member cannot edit any group content. + 'non-member', + [ + 'owner' => FALSE, + 'member' => FALSE, + 'blocked' => FALSE, + ], + ], + [ + // The blocked member cannot edit any group content, not even their own. + 'blocked', + [ + 'owner' => FALSE, + 'member' => FALSE, + 'blocked' => FALSE, + ], + ], + ]; + } + +} From 0694f4524f0e40fc77ce82b2cbcfabf544bd06bd Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Fri, 5 Aug 2016 17:35:59 +0300 Subject: [PATCH 75/85] Fix PHP CodeSniffer warning. --- tests/src/Kernel/Access/OgAccessHookTest.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/src/Kernel/Access/OgAccessHookTest.php b/tests/src/Kernel/Access/OgAccessHookTest.php index 3a2760efc..19c7291dc 100644 --- a/tests/src/Kernel/Access/OgAccessHookTest.php +++ b/tests/src/Kernel/Access/OgAccessHookTest.php @@ -2,16 +2,15 @@ namespace Drupal\Tests\og\Kernel; -use Drupal\Core\Access\AccessResult; use Drupal\KernelTests\KernelTestBase; -use Drupal\node\Entity\NodeType; -use Drupal\og\OgMembershipInterface; use Drupal\block_content\Entity\BlockContent; use Drupal\block_content\Entity\BlockContentType; use Drupal\node\Entity\Node; +use Drupal\node\Entity\NodeType; use Drupal\og\Entity\OgMembership; use Drupal\og\Og; use Drupal\og\OgGroupAudienceHelper; +use Drupal\og\OgMembershipInterface; use Drupal\simpletest\ContentTypeCreationTrait; use Drupal\user\Entity\Role; use Drupal\user\Entity\User; From 9cf7f4dbfd48cc6e1bce5c1a47bd503ea35f40e2 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Fri, 5 Aug 2016 17:50:13 +0300 Subject: [PATCH 76/85] Remove unused trait. --- tests/src/Kernel/Access/OgAccessHookTest.php | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/src/Kernel/Access/OgAccessHookTest.php b/tests/src/Kernel/Access/OgAccessHookTest.php index 19c7291dc..08962514f 100644 --- a/tests/src/Kernel/Access/OgAccessHookTest.php +++ b/tests/src/Kernel/Access/OgAccessHookTest.php @@ -11,7 +11,6 @@ use Drupal\og\Og; use Drupal\og\OgGroupAudienceHelper; use Drupal\og\OgMembershipInterface; -use Drupal\simpletest\ContentTypeCreationTrait; use Drupal\user\Entity\Role; use Drupal\user\Entity\User; @@ -22,8 +21,6 @@ */ class OgAccessHookTest extends KernelTestBase { - use ContentTypeCreationTrait; - /** * {@inheritdoc} */ From 2063574701fc56fcf2d450fca2066d08ef27213c Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Fri, 5 Aug 2016 17:50:38 +0300 Subject: [PATCH 77/85] Add a test that proves that it is possible to grant permissions to non-members. --- tests/src/Kernel/Access/WikiTest.php | 227 +++++++++++++++++++++++++++ 1 file changed, 227 insertions(+) create mode 100644 tests/src/Kernel/Access/WikiTest.php diff --git a/tests/src/Kernel/Access/WikiTest.php b/tests/src/Kernel/Access/WikiTest.php new file mode 100644 index 000000000..2877aee2d --- /dev/null +++ b/tests/src/Kernel/Access/WikiTest.php @@ -0,0 +1,227 @@ +installConfig(['og']); + $this->installEntitySchema('block_content'); + $this->installEntitySchema('node'); + $this->installEntitySchema('og_membership'); + $this->installEntitySchema('user'); + $this->installSchema('system', 'sequences'); + + // Create a user role for a standard authenticated user. + $role = Role::create([ + 'id' => 'authenticated', + 'label' => 'authenticated', + ]); + $role->grantPermission('access content'); + $role->save(); + + // Create a test user for each membership type. + $membership_types = [ + // The group owner. + 'owner', + // A regular member of the group. + 'member', + // A user that is not a member of the group. + 'non-member', + // A blocked user. + 'blocked', + ]; + foreach ($membership_types as $membership_type) { + $user = User::create([ + 'name' => $membership_type, + ]); + $user->save(); + $this->users[$membership_type] = $user; + } + + // Create a "group" bundle on the Custom Block entity type and turn it into + // a group. Note we're not using the Entity Test entity for this since it + // does not have real support for multiple bundles. + BlockContentType::create(['type' => 'group']); + Og::groupManager()->addGroup('block_content', 'group'); + + // Create a group. + $this->group = BlockContent::create([ + 'title' => $this->randomString(), + 'type' => 'group', + 'uid' => $this->users['owner']->id(), + ]); + $this->group->save(); + + // Create a group content type. + $type = NodeType::create([ + 'type' => 'group_content', + 'name' => $this->randomString() + ]); + $type->save(); + $settings = [ + 'field_storage_config' => [ + 'settings' => [ + 'target_type' => 'block_content', + ], + ], + ]; + Og::createField(OgGroupAudienceHelper::DEFAULT_FIELD, 'node', 'group_content', $settings); + + // Grant both members and non-members permission to edit any group content. + foreach ([OgRoleInterface::AUTHENTICATED, OgRoleInterface::ANONYMOUS] as $role_name) { + $role_id = "block_content-group-$role_name"; + /** @var \Drupal\og\Entity\OgRole $role */ + $role = $this->container->get('entity_type.manager') + ->getStorage('og_role') + ->load($role_id); + $role->grantPermission('edit any group_content content'); + $role->save(); + } + + // Subscribe the normal member and the blocked member to the group. + foreach (['member', 'blocked'] as $membership_type) { + $state = $membership_type === 'member' ? OgMembershipInterface::STATE_ACTIVE : OgMembershipInterface::STATE_BLOCKED; + /** @var \Drupal\og\Entity\OgMembership $membership */ + $membership = OgMembership::create(); + $membership + ->setUser($this->users[$membership_type]) + ->setGroup($this->group) + ->addRole($role) + ->setState($state) + ->save(); + } + + // Create three group content items, one owned by the group owner, one by + // the member, and one by the blocked user. + foreach (['owner', 'member', 'blocked'] as $membership_type) { + $this->groupContent[$membership_type] = Node::create([ + 'title' => $this->randomString(), + 'type' => 'group_content', + 'uid' => $this->users[$membership_type]->id(), + OgGroupAudienceHelper::DEFAULT_FIELD => [['target_id' => $this->group->id()]], + ]); + $this->groupContent[$membership_type]->save(); + } + } + + /** + * Tests access to entity operations through the access hook. + * + * @param string $user + * The name of the user to test. + * @param array $expected_results + * An associative array indicating whether the user should have the right to + * edit content owned by the user represented by the array key. + * + * @dataProvider entityOperationAccessProvider + */ + public function testEntityOperationAccess($user, array $expected_results) { + foreach ($expected_results as $group_content => $expected_result) { + /** @var \Drupal\Core\Access\AccessResult $result */ + $result = og_entity_access($this->groupContent[$group_content], 'update', $this->users[$user]); + $this->assertEquals($expected_result, $result->isAllowed()); + } + } + + /** + * Data provider for ::testEntityOperationAccess(). + * + * @return array + * And array of test data sets. Each set consisting of: + * - The name of the user to test. + * - An associative array indicating whether the user should have the right + * to edit content owned by the user represented by the array key. + */ + public function entityOperationAccessProvider() { + return [ + [ + // Members should have the right to edit any group content. + 'member', + [ + 'owner' => TRUE, + 'member' => TRUE, + 'blocked' => TRUE, + ], + ], + [ + // Non-members should have the right to edit any group content. + 'non-member', + [ + 'owner' => TRUE, + 'member' => TRUE, + 'blocked' => TRUE, + ], + ], + [ + // Blocked members cannot edit any group content, not even their own. + 'blocked', + [ + 'owner' => FALSE, + 'member' => FALSE, + 'blocked' => FALSE, + ], + ], + ]; + } + +} From 82f016fa28d6d63fefa6d0135309156618c3f4f7 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Fri, 5 Aug 2016 18:25:11 +0300 Subject: [PATCH 78/85] Provide a convenient method OgRole::loadByGroupAndName(). --- src/Entity/OgRole.php | 9 +++++++++ src/Og.php | 18 ----------------- src/OgAccess.php | 2 +- src/OgRoleInterface.php | 15 ++++++++++++++ .../src/Kernel/Access/OgEntityAccessTest.php | 2 +- .../OgGroupContentOperationAccessTest.php | 3 +-- tests/src/Kernel/Entity/OgRoleTest.php | 20 +++++++++++++------ 7 files changed, 41 insertions(+), 28 deletions(-) diff --git a/src/Entity/OgRole.php b/src/Entity/OgRole.php index 8fb6104c7..58aa307e9 100644 --- a/src/Entity/OgRole.php +++ b/src/Entity/OgRole.php @@ -3,6 +3,7 @@ namespace Drupal\og\Entity; use Drupal\Core\Config\ConfigValueException; +use Drupal\Core\Entity\EntityInterface; use Drupal\og\Exception\OgRoleException; use Drupal\og\OgRoleInterface; use Drupal\user\Entity\Role; @@ -208,6 +209,14 @@ public function setName($name) { return $this; } + /** + * {@inheritdoc} + */ + public static function loadByGroupAndName(EntityInterface $group, $name) { + $role_id = "{$group->getEntityTypeId()}-{$group->bundle()}-$name"; + return self::load($role_id); + } + /** * {@inheritdoc} */ diff --git a/src/Og.php b/src/Og.php index 986f85ea5..a6ba620dd 100644 --- a/src/Og.php +++ b/src/Og.php @@ -10,7 +10,6 @@ use Drupal\field\Entity\FieldStorageConfig; use Drupal\field\FieldStorageConfigInterface; use Drupal\og\Entity\OgMembership; -use Drupal\og\Entity\OgRole; /** * A static helper class for OG. @@ -606,23 +605,6 @@ public static function groupManager() { return \Drupal::service('og.group.manager'); } - /** - * Get a role by the group's bundle and role name. - * - * @param string $entity_type_id - * The group entity type ID. - * @param string $bundle - * The group bundle name. - * @param string $role_name - * The role name. - * - * @return \Drupal\og\OgRoleInterface|null - * The OG role object, or NULL if a matching role was not found. - */ - public static function getRole($entity_type_id, $bundle, $role_name) { - return OgRole::load($entity_type_id . '-' . $bundle . '-' . $role_name); - } - /** * Return the og permission handler instance. * diff --git a/src/OgAccess.php b/src/OgAccess.php index bbe39f4fb..fa2f48fe6 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -179,7 +179,7 @@ public function userAccess(EntityInterface $group, $operation, AccountInterface else { // User is a non-member. /** @var \Drupal\og\Entity\OgRole $role */ - $role = Og::getRole($group_type_id, $bundle, OgRoleInterface::ANONYMOUS); + $role = OgRole::loadByGroupAndName($group, OgRoleInterface::ANONYMOUS); $permissions = $role->getPermissions(); } diff --git a/src/OgRoleInterface.php b/src/OgRoleInterface.php index a53f6b088..146eadf12 100644 --- a/src/OgRoleInterface.php +++ b/src/OgRoleInterface.php @@ -2,6 +2,8 @@ namespace Drupal\og; +use Drupal\Core\Entity\EntityInterface; + /** * Provides an interface defining an OG user role entity. * @@ -65,4 +67,17 @@ public function getName(); */ public function setName($name); + /** + * Returns the role represented by the given group and role name. + * + * @param \Drupal\Core\Entity\EntityInterface $group + * The group for which to return the role. + * @param string $name + * The role name for which to return the role. + * + * @return \Drupal\og\OgRoleInterface + * The role. + */ + public static function loadByGroupAndName(EntityInterface $group, $name); + } diff --git a/tests/src/Kernel/Access/OgEntityAccessTest.php b/tests/src/Kernel/Access/OgEntityAccessTest.php index 6e97d444c..054101d1c 100644 --- a/tests/src/Kernel/Access/OgEntityAccessTest.php +++ b/tests/src/Kernel/Access/OgEntityAccessTest.php @@ -280,7 +280,7 @@ public function testAccess() { // Allow the permission to a non-member user. /** @var OgRole $role */ - $role = Og::getRole('entity_test', $this->groupBundle, OgRoleInterface::ANONYMOUS); + $role = OgRole::loadByGroupAndName($this->group1, OgRoleInterface::ANONYMOUS); $role ->grantPermission('some_perm') ->save(); diff --git a/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php b/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php index fc566441d..55e9764bb 100644 --- a/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php +++ b/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php @@ -153,8 +153,7 @@ protected function setUp() { ]; foreach ($permission_matrix as $role_name => $permissions) { - $role_id = "{$this->group->getEntityTypeId()}-{$this->group->bundle()}-$role_name"; - $this->roles[$role_name] = OgRole::load($role_id); + $this->roles[$role_name] = OgRole::loadByGroupAndName($this->group, $role_name); foreach ($permissions as $permission) { $this->roles[$role_name]->grantPermission($permission); } diff --git a/tests/src/Kernel/Entity/OgRoleTest.php b/tests/src/Kernel/Entity/OgRoleTest.php index cb4dba019..533b756b2 100644 --- a/tests/src/Kernel/Entity/OgRoleTest.php +++ b/tests/src/Kernel/Entity/OgRoleTest.php @@ -7,7 +7,6 @@ use Drupal\KernelTests\KernelTestBase; use Drupal\og\Entity\OgRole; use Drupal\og\Exception\OgRoleException; -use Drupal\og\Og; /** * Test OG role creation. @@ -21,6 +20,13 @@ class OgRoleTest extends KernelTestBase { */ public static $modules = ['field', 'og']; + /** + * The entity storage handler for OgRole entities. + * + * @var \Drupal\Core\Entity\EntityStorageInterface + */ + protected $roleStorage; + /** * {@inheritdoc} */ @@ -29,6 +35,8 @@ protected function setUp() { // Installing needed schema. $this->installConfig(['og']); + + $this->roleStorage = $this->container->get('entity_type.manager')->getStorage('og_role'); } /** @@ -55,10 +63,10 @@ public function testRoleCreate() { ->setGroupBundle('group') ->save(); - $this->assertNotEmpty(OgRole::load('node-group-content_editor'), 'The role was created with the expected ID.'); - - $expected = Og::getRole('node', 'group', 'content_editor')->id(); - $this->assertEquals($expected, $og_role->id()); + /** @var \Drupal\og\Entity\OgRole $saved_role */ + $saved_role = $this->roleStorage->loadUnchanged('node-group-content_editor'); + $this->assertNotEmpty($saved_role, 'The role was created with the expected ID.'); + $this->assertEquals($og_role->id(), $saved_role->id()); // Checking creation of the role. $this->assertEquals($og_role->getPermissions(), ['administer group']); @@ -130,7 +138,7 @@ public function testRoleCreate() { ]); $og_role->save(); - $this->assertNotEmpty(OgRole::load('entity_test-group-configurator')); + $this->assertNotEmpty($this->roleStorage->loadUnchanged('entity_test-group-configurator')); // Check that we can retrieve the role name correctly. This was not // explicitly saved but it should be possible to derive this from the ID. From 768cbbe93cf562f79be2d569483e9a0a006ab40c Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Fri, 5 Aug 2016 18:33:22 +0300 Subject: [PATCH 79/85] Fix PHP CodeSniffer warnings. --- tests/src/Kernel/Access/OgAccessHookTest.php | 2 +- tests/src/Kernel/Access/WikiTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/src/Kernel/Access/OgAccessHookTest.php b/tests/src/Kernel/Access/OgAccessHookTest.php index 08962514f..6f99df31b 100644 --- a/tests/src/Kernel/Access/OgAccessHookTest.php +++ b/tests/src/Kernel/Access/OgAccessHookTest.php @@ -132,7 +132,7 @@ public function setUp() { // Create a group content type. $type = NodeType::create([ 'type' => 'group_content', - 'name' => $this->randomString() + 'name' => $this->randomString(), ]); $type->save(); $settings = [ diff --git a/tests/src/Kernel/Access/WikiTest.php b/tests/src/Kernel/Access/WikiTest.php index 2877aee2d..d9aefe621 100644 --- a/tests/src/Kernel/Access/WikiTest.php +++ b/tests/src/Kernel/Access/WikiTest.php @@ -115,7 +115,7 @@ public function setUp() { // Create a group content type. $type = NodeType::create([ 'type' => 'group_content', - 'name' => $this->randomString() + 'name' => $this->randomString(), ]); $type->save(); $settings = [ From b7df48b811e344a0d8ae0d5125e1f59e96543cf3 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Sat, 6 Aug 2016 10:54:13 +0300 Subject: [PATCH 80/85] Leverage OgAccess::userAccess() instead of duplicating half its logic and ignoring the other half. --- src/OgAccess.php | 58 +++++++++--------------------------------------- 1 file changed, 10 insertions(+), 48 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index fa2f48fe6..d502c9f42 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -330,7 +330,16 @@ public function userAccessGroupContentEntityOperation($operation, EntityInterfac return $permission->getOperation() === $operation && in_array($permission->getOwner(), $ownerships); }); - // @todo This doesn't really vary by user but by the user's role inside of + if ($permissions) { + foreach ($permissions as $permission) { + $user_access = $this->userAccess($group_entity, $permission->getName(), $user); + if ($user_access->isAllowed()) { + return $user_access; + } + } + } + + // @todo This doesn't really vary by user but by the user's roles inside of // the group. We should create a cache context for OgRole entities. // @see https://github.com/amitaibu/og/issues/219 $cacheable_metadata = new CacheableMetadata(); @@ -339,24 +348,6 @@ public function userAccessGroupContentEntityOperation($operation, EntityInterfac $cacheable_metadata->addCacheContexts(['user']); } - if ($permissions) { - // Request the user's membership. If the user is not a member, construct a - // 'non-member membership' in which the user has the 'anonymous' role. We - // can then use this to retrieve the permissions that the user has within - // the group, regardless if they are a member or not. - $states = [ - OgMembershipInterface::STATE_ACTIVE, - OgMembershipInterface::STATE_PENDING, - OgMembershipInterface::STATE_BLOCKED, - ]; - $membership = Og::getMembership($group_entity, $user, $states) ?: $this->getNonMemberMembership($group_entity, $user); - foreach ($permissions as $permission) { - if ($membership->hasPermission($permission->getName())) { - return AccessResult::allowed()->addCacheableDependency($cacheable_metadata); - } - } - } - return AccessResult::neutral()->addCacheableDependency($cacheable_metadata); } @@ -418,33 +409,4 @@ public function reset() { $this->permissionsCache = []; } - /** - * Returns a group membership in which the user is not a member. - * - * This is intended to be used to retrieve the permissions a non-member has in - * the given group. Note that this membership is created on the fly and is not - * saved. - * - * @param \Drupal\Core\Entity\EntityInterface $group - * The group to get the membership for. - * @param \Drupal\Core\Session\AccountInterface $user - * The user to get the membership for. - * - * @return \Drupal\og\Entity\OgMembership - * The OgMembership entity. - */ - protected static function getNonMemberMembership(EntityInterface $group, AccountInterface $user) { - $role_id = implode('-', [ - $group->getEntityTypeId(), - $group->bundle(), - OgRoleInterface::ANONYMOUS, - ]); - - return OgMembership::create() - ->setUser($user) - ->setGroup($group) - ->setState(OgMembershipInterface::STATE_PENDING) - ->setRoles([new OgRole(['id' => $role_id])]); - } - } From a2ffe602975cb04568675041b4cf273fe4037db5 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Sat, 6 Aug 2016 10:54:23 +0300 Subject: [PATCH 81/85] Add documentation. --- src/OgAccess.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/OgAccess.php b/src/OgAccess.php index d502c9f42..121b3c83e 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -124,6 +124,9 @@ public function userAccess(EntityInterface $group, $operation, AccountInterface // From this point on, every result also depends on the user so check // whether it is the current. See https://www.drupal.org/node/2628870 + // @todo This doesn't really vary by user but by the user's roles inside of + // the group. We should create a cache context for OgRole entities. + // @see https://github.com/amitaibu/og/issues/219 if ($user->id() == $this->accountProxy->id()) { $cacheable_metadata->addCacheContexts(['user']); } From cad927cba2600a8e1418d4785b5e7c974e4ffc04 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Sat, 6 Aug 2016 10:55:24 +0300 Subject: [PATCH 82/85] Remove unused variable. --- .../src/Kernel/Access/OgGroupContentOperationAccessTest.php | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php b/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php index 55e9764bb..222d6394f 100644 --- a/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php +++ b/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php @@ -266,7 +266,7 @@ protected function setUp() { * * @dataProvider accessProvider */ - public function testAccess($group_content_entity_type_id, $group_content_bundle_id, $expected_access_matrix) { + public function testAccess($group_content_bundle_id, $expected_access_matrix) { /** @var \Drupal\og\OgAccessInterface $og_access */ $og_access = $this->container->get('og.access'); @@ -289,8 +289,6 @@ public function testAccess($group_content_entity_type_id, $group_content_bundle_ * * @return array * And array of test data sets. Each set consisting of: - * - A string representing the group content entity type ID upon which the - * operation is performed. Can be either 'node' or 'comment'. * - A string representing the group content bundle ID upon which the * operation is performed. Can be either 'newsletter' or 'article'. * - An array mapping the different users and the possible operations, and @@ -300,7 +298,6 @@ public function testAccess($group_content_entity_type_id, $group_content_bundle_ public function accessProvider() { return [ [ - 'comment', 'newsletter', [ // The super user and the administrator have the right to create, @@ -338,7 +335,6 @@ public function accessProvider() { ], ], [ - 'node', 'article', [ // The super user and the administrator have the right to create, From ea97a93f85e59f04139e62eac71e55f55b4aca95 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Sat, 6 Aug 2016 11:23:50 +0300 Subject: [PATCH 83/85] Blocked users should not be granted any permissions. --- src/OgAccess.php | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 121b3c83e..7b837a312 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -167,16 +167,24 @@ public function userAccess(EntityInterface $group, $operation, AccountInterface $permissions = []; $user_is_group_admin = FALSE; - if ($membership = Og::getMembership($group, $user)) { - foreach ($membership->getRoles() as $role) { - // Check for the is_admin flag. - /** @var \Drupal\og\Entity\OgRole $role */ - if ($role->isAdmin()) { - $user_is_group_admin = TRUE; - break; + $states = [ + OgMembershipInterface::STATE_ACTIVE, + OgMembershipInterface::STATE_PENDING, + OgMembershipInterface::STATE_BLOCKED, + ]; + if ($membership = Og::getMembership($group, $user, $states)) { + // Blocked users don't have any permissions. + if ($membership->getState() !== OgMembershipInterface::STATE_BLOCKED) { + foreach ($membership->getRoles() as $role) { + // Check for the is_admin flag. + /** @var \Drupal\og\Entity\OgRole $role */ + if ($role->isAdmin()) { + $user_is_group_admin = true; + break; + } + + $permissions = array_merge($permissions, $role->getPermissions()); } - - $permissions = array_merge($permissions, $role->getPermissions()); } } else { From d1e6ebca137320ea0c6807f96fd753464e36b0c3 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Sat, 6 Aug 2016 11:26:24 +0300 Subject: [PATCH 84/85] Revert "Provide a convenient method OgRole::loadByGroupAndName()." This partially reverts commit 82f016fa28d6d63fefa6d0135309156618c3f4f7. --- src/Og.php | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/Og.php b/src/Og.php index a6ba620dd..986f85ea5 100644 --- a/src/Og.php +++ b/src/Og.php @@ -10,6 +10,7 @@ use Drupal\field\Entity\FieldStorageConfig; use Drupal\field\FieldStorageConfigInterface; use Drupal\og\Entity\OgMembership; +use Drupal\og\Entity\OgRole; /** * A static helper class for OG. @@ -605,6 +606,23 @@ public static function groupManager() { return \Drupal::service('og.group.manager'); } + /** + * Get a role by the group's bundle and role name. + * + * @param string $entity_type_id + * The group entity type ID. + * @param string $bundle + * The group bundle name. + * @param string $role_name + * The role name. + * + * @return \Drupal\og\OgRoleInterface|null + * The OG role object, or NULL if a matching role was not found. + */ + public static function getRole($entity_type_id, $bundle, $role_name) { + return OgRole::load($entity_type_id . '-' . $bundle . '-' . $role_name); + } + /** * Return the og permission handler instance. * From bbceea5d0d2c3127af72f517bbe1ce9c53c494ee Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Sat, 6 Aug 2016 11:38:35 +0300 Subject: [PATCH 85/85] Fix PHP_CodeSniffer warnings. --- src/OgAccess.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 7b837a312..dcc506bcb 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -10,7 +10,6 @@ use Drupal\Core\Extension\ModuleHandlerInterface; use Drupal\Core\Session\AccountInterface; use Drupal\Core\Session\AccountProxyInterface; -use Drupal\og\Entity\OgMembership; use Drupal\og\Entity\OgRole; use Drupal\user\EntityOwnerInterface; @@ -179,7 +178,7 @@ public function userAccess(EntityInterface $group, $operation, AccountInterface // Check for the is_admin flag. /** @var \Drupal\og\Entity\OgRole $role */ if ($role->isAdmin()) { - $user_is_group_admin = true; + $user_is_group_admin = TRUE; break; }