diff --git a/.travis.yml b/.travis.yml index 9f07ca274..2704b7cae 100644 --- a/.travis.yml +++ b/.travis.yml @@ -28,8 +28,6 @@ matrix: env: TEST_SUITE=PHP_CodeSniffer - php: 7.4 env: TEST_SUITE=PHP_CodeSniffer - allow_failures: - - php: 7.4 mysql: database: og @@ -71,6 +69,9 @@ before_script: # Install Composer dependencies for core. Skip this for the coding standards test. - test ${TEST_SUITE} == "PHP_CodeSniffer" || composer install --working-dir=$DRUPAL_DIR + # Update PHPUnit. + - test ${TEST_SUITE} == "PHP_CodeSniffer" || travis_retry composer update phpunit/phpunit symfony/phpunit-bridge phpspec/prophecy symfony/yaml --with-dependencies --working-dir=$DRUPAL_DIR + # Start a web server on port 8888 in the background. - test ${TEST_SUITE} == "PHP_CodeSniffer" || nohup php -S localhost:8888 --docroot $DRUPAL_DIR > /dev/null 2>&1 & diff --git a/og.module b/og.module index c74cbf7c3..34f226931 100755 --- a/og.module +++ b/og.module @@ -154,7 +154,7 @@ function og_entity_access(EntityInterface $entity, $operation, AccountInterface } /** @var \Drupal\Core\Access\AccessResult $access */ - $access = \Drupal::service('og.access')->userAccessEntity($operation, $entity, $account); + $access = \Drupal::service('og.access')->userAccessEntityOperation($operation, $entity, $account); $audience_fields = \Drupal::service('og.group_audience_helper')->getAllGroupAudienceFields($entity_type_id, $bundle_id); if (count($audience_fields) === 1) { diff --git a/og.services.yml b/og.services.yml index 17787794c..be1821d89 100644 --- a/og.services.yml +++ b/og.services.yml @@ -21,12 +21,7 @@ services: - { name: 'cache.context'} og.access: class: Drupal\og\OgAccess - arguments: ['@config.factory', '@current_user', '@module_handler', '@og.group_type_manager', '@og.permission_manager', '@og.membership_manager', '@og.group_audience_helper'] - og.add_field: - class: Drupal\og\Command\OgAddFieldCommand - arguments: ['@entity_type.bundle.info', '@entity_type.repository', '@plugin.manager.og.fields'] - tags: - - { name: drupal.command } + arguments: ['@config.factory', '@current_user', '@module_handler', '@og.group_type_manager', '@og.permission_manager', '@og.membership_manager'] og.context: class: Drupal\og\ContextProvider\OgContext arguments: ['@plugin.manager.og.group_resolver', '@config.factory'] diff --git a/src/EventSubscriber/OgEventSubscriber.php b/src/EventSubscriber/OgEventSubscriber.php index 08b51bac3..516f3570a 100644 --- a/src/EventSubscriber/OgEventSubscriber.php +++ b/src/EventSubscriber/OgEventSubscriber.php @@ -88,7 +88,7 @@ public function provideDefaultOgPermissions(PermissionEventInterface $event) { new GroupPermission([ 'name' => OgAccess::UPDATE_GROUP_PERMISSION, 'title' => $this->t('Edit group'), - 'description' => $this->t('Edit the group. Note: This permission controls only node entity type groups.'), + 'description' => $this->t('Edit the group entity.'), 'default roles' => [OgRoleInterface::ADMINISTRATOR], ]), new GroupPermission([ diff --git a/src/Og.php b/src/Og.php index 9536c126b..2ec908961 100644 --- a/src/Og.php +++ b/src/Og.php @@ -260,8 +260,6 @@ public static function isGroup($entity_type_id, $bundle_id) { /** * Check if the given entity type and bundle is a group content. * - * This works by checking if the bundle has one or more group audience fields. - * * @param string $entity_type_id * The entity type. * @param string $bundle_id @@ -271,7 +269,7 @@ public static function isGroup($entity_type_id, $bundle_id) { * True or false if the given entity is group content. */ public static function isGroupContent($entity_type_id, $bundle_id) { - return \Drupal::service('og.group_audience_helper')->hasGroupAudienceField($entity_type_id, $bundle_id); + return \Drupal::service('og.group_type_manager')->isGroupContent($entity_type_id, $bundle_id); } /** diff --git a/src/OgAccess.php b/src/OgAccess.php index 48a4d5d83..8f6cb68c8 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -1,5 +1,7 @@ self::UPDATE_GROUP_PERMISSION, + ]; + /** * The config factory. * @@ -75,14 +84,7 @@ class OgAccess implements OgAccessInterface { protected $membershipManager; /** - * The OG group audience helper. - * - * @var \Drupal\og\OgGroupAudienceHelperInterface - */ - protected $groupAudienceHelper; - - /** - * Constructs an OgManager service. + * Constructs the OgAccess service. * * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory * The config factory. @@ -96,23 +98,20 @@ class OgAccess implements OgAccessInterface { * The permission manager. * @param \Drupal\og\MembershipManagerInterface $membership_manager * The group membership manager. - * @param \Drupal\og\OgGroupAudienceHelperInterface $group_audience_helper - * The OG group audience helper. */ - public function __construct(ConfigFactoryInterface $config_factory, AccountProxyInterface $account_proxy, ModuleHandlerInterface $module_handler, GroupTypeManagerInterface $group_manager, PermissionManagerInterface $permission_manager, MembershipManagerInterface $membership_manager, OgGroupAudienceHelperInterface $group_audience_helper) { + public function __construct(ConfigFactoryInterface $config_factory, AccountProxyInterface $account_proxy, ModuleHandlerInterface $module_handler, GroupTypeManagerInterface $group_manager, PermissionManagerInterface $permission_manager, MembershipManagerInterface $membership_manager) { $this->configFactory = $config_factory; $this->accountProxy = $account_proxy; $this->moduleHandler = $module_handler; $this->groupTypeManager = $group_manager; $this->permissionManager = $permission_manager; $this->membershipManager = $membership_manager; - $this->groupAudienceHelper = $group_audience_helper; } /** * {@inheritdoc} */ - public function userAccess(EntityInterface $group, $operation, AccountInterface $user = NULL, $skip_alter = FALSE): AccessResultInterface { + public function userAccess(EntityInterface $group, string $permission, AccountInterface $user = NULL, bool $skip_alter = FALSE): AccessResultInterface { $group_type_id = $group->getEntityTypeId(); $bundle = $group->bundle(); // As Og::isGroup depends on this config, we retrieve it here and set it as @@ -151,14 +150,6 @@ public function userAccess(EntityInterface $group, $operation, AccountInterface return $user_access->addCacheableDependency($cacheable_metadata); } - // Update group special permission. At this point, the operation should have - // already been handled by Og. If the operation is simply 'edit' - // (or 'update' for content entities), it is referring to the current group, - // so we have to map it to the special permission. - if (in_array($operation, ['update', 'edit'])) { - $operation = OgAccess::UPDATE_GROUP_PERMISSION; - } - if ($config->get('group_manager_full_access') && $user->isAuthenticated() && $group instanceof EntityOwnerInterface) { $cacheable_metadata->addCacheableDependency($group); if ($group->getOwnerId() == $user->id()) { @@ -188,10 +179,10 @@ public function userAccess(EntityInterface $group, $operation, AccountInterface $permissions = array_unique($permissions); - if (!$skip_alter && !in_array($operation, $permissions)) { + if (!$skip_alter && !in_array($permission, $permissions)) { // Let modules alter the permissions. $context = [ - 'operation' => $operation, + 'permission' => $permission, 'group' => $group, 'user' => $user, ]; @@ -202,7 +193,7 @@ public function userAccess(EntityInterface $group, $operation, AccountInterface // permissions. // @todo It should be possible for modules to alter the permissions even if // the user is a group admin, UID 1 or has 'administer group' permission. - if ($user_is_group_admin || in_array($operation, $permissions)) { + if ($user_is_group_admin || in_array($permission, $permissions)) { // User is a group admin, and we do not ignore this special permission // that grants access to all the group permissions. return AccessResult::allowed()->addCacheableDependency($cacheable_metadata); @@ -214,7 +205,7 @@ public function userAccess(EntityInterface $group, $operation, AccountInterface /** * {@inheritdoc} */ - public function userAccessEntity($operation, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface { + public function userAccessEntity(string $permission, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface { $result = AccessResult::neutral(); $entity_type = $entity->getEntityType(); @@ -222,22 +213,21 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt $bundle = $entity->bundle(); if ($this->groupTypeManager->isGroup($entity_type_id, $bundle)) { - $user_access = $this->userAccess($entity, $operation, $user); + $user_access = $this->userAccess($entity, $permission, $user); if ($user_access->isAllowed()) { return $user_access; } else { // An entity can be a group and group content in the same time. The // group didn't allow access, but the user still might have access to - // the permission in group content context. So instead of retuning a + // the permission in group content context. So instead of returning a // deny here, we set the result, that might change if an access is // found. $result = AccessResult::forbidden()->inheritCacheability($user_access); } } - $is_group_content = $this->groupAudienceHelper->hasGroupAudienceField($entity_type_id, $bundle); - if ($is_group_content) { + if ($this->groupTypeManager->isGroupContent($entity_type_id, $bundle)) { $cache_tags = $entity_type->getListCacheTags(); // The entity might be a user or a non-user entity. @@ -247,17 +237,7 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt $forbidden = AccessResult::forbidden()->addCacheTags($cache_tags); foreach ($groups as $entity_groups) { foreach ($entity_groups as $group) { - // Check if the operation matches a group content entity operation - // such as 'create article content'. - $operation_access = $this->userAccessGroupContentEntityOperation($operation, $group, $entity, $user); - - if ($operation_access->isAllowed()) { - return $operation_access->addCacheTags($cache_tags); - } - - // Check if the operation matches a group level operation such as - // 'subscribe without approval'. - $user_access = $this->userAccess($group, $operation, $user); + $user_access = $this->userAccess($group, $permission, $user); if ($user_access->isAllowed()) { return $user_access->addCacheTags($cache_tags); } @@ -271,15 +251,73 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt $result->addCacheTags($cache_tags); } - // Either the user didn't have permission, or the entity might be an - // orphaned group content. + // Either the user didn't have permission, or the entity might be orphaned + // group content. + return $result; + } + + /** + * {@inheritdoc} + */ + public function userAccessEntityOperation(string $operation, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface { + $result = AccessResult::neutral(); + + $entity_type = $entity->getEntityType(); + $entity_type_id = $entity_type->id(); + $bundle = $entity->bundle(); + + if ($this->groupTypeManager->isGroup($entity_type_id, $bundle)) { + // We are performing an entity operation on a group entity. Map the + // operation to the corresponding group level permission. + if (array_key_exists($operation, self::OPERATION_GROUP_PERMISSION_MAPPING)) { + $permission = self::OPERATION_GROUP_PERMISSION_MAPPING[$operation]; + + $user_access = $this->userAccess($entity, $permission, $user); + if ($user_access->isAllowed()) { + return $user_access; + } + else { + // An entity can be a group and group content in the same time. The + // group permission check didn't allow access, but the user still + // might have access to perform the operation in group content + // context. So instead of returning a deny here, we set the result, + // that might change if an access is found. + $result = AccessResult::forbidden()->inheritCacheability($user_access); + } + } + } + + if ($this->groupTypeManager->isGroupContent($entity_type_id, $bundle)) { + $cache_tags = $entity_type->getListCacheTags(); + + // The entity might be a user or a non-user entity. + $groups = $entity instanceof UserInterface ? $this->membershipManager->getUserGroups($entity->id()) : $this->membershipManager->getGroups($entity); + + if ($groups) { + $forbidden = AccessResult::forbidden()->addCacheTags($cache_tags); + foreach ($groups as $entity_groups) { + foreach ($entity_groups as $group) { + $operation_access = $this->userAccessGroupContentEntityOperation($operation, $group, $entity, $user); + if ($operation_access->isAllowed()) { + return $operation_access->addCacheTags($cache_tags); + } + } + } + return $forbidden; + } + + $result->addCacheTags($cache_tags); + } + + // Either the user didn't have permission, or the entity might be orphaned + // group content. return $result; } /** * {@inheritdoc} */ - public function userAccessGroupContentEntityOperation($operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL): AccessResultInterface { + public function userAccessGroupContentEntityOperation(string $operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL): AccessResultInterface { // Default to the current user. $user = $user ?: $this->accountProxy->getAccount(); @@ -329,7 +367,7 @@ public function userAccessGroupContentEntityOperation($operation, EntityInterfac * {@inheritdoc} */ public function reset(): void { - trigger_error('OgAccessInterface::reset() is deprecated in og:8.1.0-alpha6 and is removed from og:8.1.0-beta1. The static cache has been removed and this no longer server any purpose. See https://github.com/Gizra/og/issues/654', E_USER_DEPRECATED); + trigger_error('OgAccessInterface::reset() is deprecated in og:8.1.0-alpha6 and is removed from og:8.1.0-beta1. The static cache has been removed and this method no longer serves any purpose. Any calls to this method can safely be removed. See https://github.com/Gizra/og/issues/654', E_USER_DEPRECATED); } } diff --git a/src/OgAccessInterface.php b/src/OgAccessInterface.php index 1ed65dde0..59fb7d557 100644 --- a/src/OgAccessInterface.php +++ b/src/OgAccessInterface.php @@ -1,5 +1,7 @@ groupOwner = $this->drupalCreateUser(); - $this->groupEditor = $this->drupalCreateUser(); - $this->normalUser = $this->drupalCreateUser(); - - $this->setUpContentEntity(); - $this->setUpEntityTestEntity(); - } - - /** - * Setup dummy content group entity and appropriate og permissions. - */ - public function setUpContentEntity() { - // Create a node bundle called 'content_group' and make it an Og group. - $this->createContentType(['type' => 'content_group']); - Og::groupTypeManager()->addGroup('node', 'content_group'); - - // Create a role with only the 'update group' permission. - $content_editor_role = OgRole::create(); - $content_editor_role - ->setName('content_editor') - ->setLabel('Content group editor') - ->setGroupType('node') - ->setGroupBundle('content_group') - ->grantPermission(OgAccess::UPDATE_GROUP_PERMISSION) - ->save(); - - // Create a group content owned by the group owner. - $values = [ - 'title' => $this->randomString(), - 'type' => 'content_group', - 'uid' => $this->groupOwner->id(), - 'status' => 1, - ]; - $this->contentGroup = Node::create($values); - $this->contentGroup->save(); - - // Subscribe the editor user to the groups. - $this->createOgMembership($this->contentGroup, $this->groupEditor, ['content_editor']); - } - - /** - * Setup dummy entity_test group entity and appropriate OG permissions. - */ - public function setUpEntityTestEntity() { - // Create an entity_test bundle called 'entity_group' and make it - // an Og group. - entity_test_create_bundle('entity_group'); - Og::groupTypeManager()->addGroup('entity_test', 'entity_group'); - - // Create a role with only the 'update group' permission. - $entity_editor_role = OgRole::create(); - $entity_editor_role - ->setName('entity_editor') - ->setLabel('Entity group editor') - ->setGroupType('entity_test') - ->setGroupBundle('entity_group') - ->grantPermission(OgAccess::UPDATE_GROUP_PERMISSION) - ->save(); - - // Create an entity_test entity owned by the group owner. - $values = [ - 'title' => $this->randomString(), - 'type' => 'entity_group', - 'uid' => $this->groupOwner->id(), - ]; - $this->entityGroup = EntityTest::create($values); - $this->entityGroup->save(); - - $this->createOgMembership($this->entityGroup, $this->groupEditor, ['entity_editor']); - } - - /** - * Tests 'update group' special permission. - * - * @dataProvider ogUpdateAccessProvider - */ - public function testUpdateAccess($entity) { - // The editor should have permissions due to the 'update group' special - // permission. - $this->drupalLogin($this->groupEditor); - $this->drupalGet($this->{$entity}->toUrl('edit-form')); - $this->assertSession()->statusCodeEquals(200); - - // A normal user should not be able to edit the group. - $this->drupalLogin($this->normalUser); - $this->drupalGet($this->{$entity}->toUrl('edit-form')); - $this->assertSession()->statusCodeEquals(403); - } - - /** - * Data provider for ::testUpdateAccess() - * - * @return array - * The names of the variables that will be tested. - */ - public function ogUpdateAccessProvider() { - return [ - // Mapping of operation 'update'. - ['contentGroup'], - // Mapping of operation 'edit'. - ['entityGroup'], - ]; - } - -} diff --git a/tests/src/Kernel/Access/OgEntityAccessTest.php b/tests/src/Kernel/Access/OgEntityAccessTest.php index 2e7604cbd..e8d292e23 100644 --- a/tests/src/Kernel/Access/OgEntityAccessTest.php +++ b/tests/src/Kernel/Access/OgEntityAccessTest.php @@ -1,5 +1,7 @@ assertTrue($og_access->userAccess($this->group1, 'some_perm', $this->user3)->isAllowed()); - // A member with permission to update the group. The operation edit is - // passed to the userAccess method. - $this->assertTrue($og_access->userAccess($this->group1, 'edit', $this->user4)->isAllowed()); - // Group admin user should have access regardless. $this->assertTrue($og_access->userAccess($this->group1, 'some_perm', $this->adminUser)->isAllowed()); $this->assertTrue($og_access->userAccess($this->group1, $this->randomMachineName(), $this->adminUser)->isAllowed()); diff --git a/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php b/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php index 79a5b4e5d..4c794cc3d 100644 --- a/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php +++ b/tests/src/Kernel/Access/OgGroupContentOperationAccessTest.php @@ -267,7 +267,7 @@ public function testAccess($group_content_bundle_id, $expected_access_matrix) { // group owner. $entity = $ownership === 'own' ? $this->groupContent[$group_content_bundle_id][$user_id] : $this->groupContent[$group_content_bundle_id]['group_owner']; $user = $this->users[$user_id]; - $this->assertEquals($expected_access, $og_access->userAccessEntity($operation, $entity, $user)->isAllowed(), "Operation: $operation, ownership: $ownership, user: $user_id, bundle: $group_content_bundle_id"); + $this->assertEquals($expected_access, $og_access->userAccessEntityOperation($operation, $entity, $user)->isAllowed(), "Operation: $operation, ownership: $ownership, user: $user_id, bundle: $group_content_bundle_id"); } } } diff --git a/tests/src/Unit/OgAccessEntityTest.php b/tests/src/Unit/OgAccessEntityTest.php index 10df432f0..2ac1034e1 100644 --- a/tests/src/Unit/OgAccessEntityTest.php +++ b/tests/src/Unit/OgAccessEntityTest.php @@ -11,33 +11,33 @@ class OgAccessEntityTest extends OgAccessEntityTestBase { /** - * Tests access to an entity by different operations. + * Tests access to an entity by different permissions. * * @coversDefaultmethod ::userAccessEntity * @dataProvider permissionsProvider */ - public function testAccessByOperation($operation) { + public function testAccessByPermission($permission) { $this->membershipManager->getGroups($this->groupContentEntity->reveal())->willReturn([$this->entityTypeId => [$this->group]]); - $user_access = $this->ogAccess->userAccessEntity($operation, $this->groupContentEntity->reveal(), $this->user->reveal()); + $user_access = $this->ogAccess->userAccessEntity($permission, $this->groupContentEntity->reveal(), $this->user->reveal()); // We populate the allowed permissions cache in // OgAccessEntityTestBase::setup(). - $condition = $operation == 'update group' ? $user_access->isAllowed() : $user_access->isForbidden(); + $condition = $permission == 'update group' ? $user_access->isAllowed() : $user_access->isForbidden(); $this->assertTrue($condition); } /** - * Tests access to an entity by different operations, by an admin member. + * Tests access to an entity by different permissions, by an admin member. * * @coversDefaultmethod ::userAccessEntity * @dataProvider permissionsProvider */ - public function testAccessByOperationAdmin($operation) { + public function testAccessByPermissionAdmin($permission) { $this->membershipManager->getGroups($this->groupContentEntity->reveal())->willReturn([$this->entityTypeId => [$this->group]]); $this->user->hasPermission('administer organic groups')->willReturn(TRUE); - $user_entity_access = $this->ogAccess->userAccessEntity($operation, $this->groupContentEntity->reveal(), $this->user->reveal()); + $user_entity_access = $this->ogAccess->userAccessEntity($permission, $this->groupContentEntity->reveal(), $this->user->reveal()); $this->assertTrue($user_entity_access->isAllowed()); } diff --git a/tests/src/Unit/OgAccessEntityTestBase.php b/tests/src/Unit/OgAccessEntityTestBase.php index 4baf2dd78..8087db9e3 100644 --- a/tests/src/Unit/OgAccessEntityTestBase.php +++ b/tests/src/Unit/OgAccessEntityTestBase.php @@ -50,9 +50,9 @@ protected function setUp(): void { $this->groupContentEntity->getEntityTypeId()->willReturn($entity_type_id); $this->addCache($this->groupContentEntity); - // If the group audience helper is asked if the group content entity has any - // group audience fields, it is expected that this will return TRUE. - $this->groupAudienceHelper->hasGroupAudienceField($entity_type_id, $bundle) + // If the group type manager is asked if the group content entity is group + // content, it is expected that this will return TRUE. + $this->groupTypeManager->isGroupContent($entity_type_id, $bundle) ->willReturn(TRUE); // It is expected that a list of entity operation permissions is retrieved diff --git a/tests/src/Unit/OgAccessTestBase.php b/tests/src/Unit/OgAccessTestBase.php index 6e1a25728..79946f9ac 100644 --- a/tests/src/Unit/OgAccessTestBase.php +++ b/tests/src/Unit/OgAccessTestBase.php @@ -13,7 +13,6 @@ use Drupal\Core\Session\AccountInterface; use Drupal\Core\Session\AccountProxyInterface; use Drupal\og\MembershipManagerInterface; -use Drupal\og\OgGroupAudienceHelperInterface; use Drupal\og\OgMembershipInterface; use Drupal\Tests\UnitTestCase; use Drupal\og\GroupTypeManagerInterface; @@ -98,13 +97,6 @@ class OgAccessTestBase extends UnitTestCase { */ protected $membershipManager; - /** - * The OG group audience helper. - * - * @var \Drupal\og\OgGroupAudienceHelperInterface - */ - protected $groupAudienceHelper; - /** * The entity type manager service. * @@ -178,8 +170,6 @@ protected function setUp(): void { $this->membershipManager->getGroupCount(Argument::any())->willReturn(1); $this->membership->getRoles()->willReturn([$this->ogRole->reveal()]); - $this->groupAudienceHelper = $this->prophesize(OgGroupAudienceHelperInterface::class); - // @todo: Move to test. $this->ogRole->isAdmin()->willReturn(FALSE); $this->ogRole->getPermissions()->willReturn(['update group']); @@ -196,8 +186,7 @@ protected function setUp(): void { $module_handler->reveal(), $this->groupTypeManager->reveal(), $this->permissionManager->reveal(), - $this->membershipManager->reveal(), - $this->groupAudienceHelper->reveal() + $this->membershipManager->reveal() ); $container = new ContainerBuilder(); @@ -207,7 +196,6 @@ protected function setUp(): void { $container->set('module_handler', $this->prophesize(ModuleHandlerInterface::class)->reveal()); $container->set('og.group_type_manager', $this->groupTypeManager->reveal()); $container->set('og.membership_manager', $this->membershipManager->reveal()); - $container->set('og.group_audience_helper', $this->groupAudienceHelper->reveal()); // This is for caching purposes only. $container->set('current_user', $this->user->reveal());