From ddb8577fee6545274bd9892086560a6d8c7b595f Mon Sep 17 00:00:00 2001 From: Maarten Segers Date: Mon, 20 Jul 2020 10:11:53 +0200 Subject: [PATCH 01/18] Fixed grammar --- src/OgAccess.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 48a4d5d83..807be5f45 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -329,7 +329,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); } } From 4b187cb54774ff4895ab43c6927be62c1da8a7f7 Mon Sep 17 00:00:00 2001 From: Maarten Segers Date: Mon, 20 Jul 2020 22:18:08 +0200 Subject: [PATCH 02/18] 667 Use PHPUnit 7 for testing PHPUnit 7 can be used for testing on PHP 7.1+ https://www.drupal.org/node/3078763 --- .travis.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.travis.yml b/.travis.yml index 9f07ca274..0e4abdbfb 100644 --- a/.travis.yml +++ b/.travis.yml @@ -71,6 +71,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 PHP + - 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 & From a7966a5f2bcdbb911f8fbf01394db27fb4613322 Mon Sep 17 00:00:00 2001 From: Maarten Segers Date: Mon, 20 Jul 2020 22:18:44 +0200 Subject: [PATCH 03/18] Update .travis.yml --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 0e4abdbfb..c5e70157f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -72,7 +72,7 @@ before_script: - test ${TEST_SUITE} == "PHP_CodeSniffer" || composer install --working-dir=$DRUPAL_DIR # Update PHP - - test ${TEST_SUITE} == "PHP_CodeSniffer" || travis_retry composer update phpunit/phpunit symfony/phpunit-bridge phpspec/prophecy symfony/yaml --with-dependencies --working-dir=$DRUPAL_DIR + - test ${TEST_SUITE} == "PHP_CodeSniffer" || travis_retry composer update phpunit/phpunit symfony/phpunit-bridge phpspec/prophecy symfony/yaml --with-dependencies # 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 & From c3ca0f80f292a5c01c59384a2623ad87354b9137 Mon Sep 17 00:00:00 2001 From: Maarten Segers Date: Mon, 20 Jul 2020 22:19:20 +0200 Subject: [PATCH 04/18] Use PHPUnit 7 for testing --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index c5e70157f..0e4abdbfb 100644 --- a/.travis.yml +++ b/.travis.yml @@ -72,7 +72,7 @@ before_script: - test ${TEST_SUITE} == "PHP_CodeSniffer" || composer install --working-dir=$DRUPAL_DIR # Update PHP - - test ${TEST_SUITE} == "PHP_CodeSniffer" || travis_retry composer update phpunit/phpunit symfony/phpunit-bridge phpspec/prophecy symfony/yaml --with-dependencies + - 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 & From b9996dd919921118df2960501c4b2c817d4ce513 Mon Sep 17 00:00:00 2001 From: Maarten Segers Date: Tue, 28 Jul 2020 12:26:35 +0200 Subject: [PATCH 05/18] Fix comment --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 0e4abdbfb..33cd7a1c3 100644 --- a/.travis.yml +++ b/.travis.yml @@ -71,7 +71,7 @@ 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 PHP + # 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. From 11ab5a3247061901f3e514ceddf782d46622c1b5 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 28 Jul 2020 19:09:39 +0300 Subject: [PATCH 06/18] Remove obsolete service definition. --- og.services.yml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/og.services.yml b/og.services.yml index 17787794c..45e31dacf 100644 --- a/og.services.yml +++ b/og.services.yml @@ -22,11 +22,6 @@ services: 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 } og.context: class: Drupal\og\ContextProvider\OgContext arguments: ['@plugin.manager.og.group_resolver', '@config.factory'] From 003ff8e8d371a670d815285adfa020a4f838f0bb Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 28 Jul 2020 21:03:05 +0300 Subject: [PATCH 07/18] Only allow to check access based on permissions. This was incorrectly documented as checking on operations, and partly implemented as such, but this was not the intention. It has been reverted to accept only permissions so this no longer conflicts with the original D7 implementation and no longer leads to inconsistencies. --- src/OgAccess.php | 22 ++++++++-------------- src/OgAccessInterface.php | 4 +++- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 807be5f45..7317f30c7 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -1,5 +1,7 @@ getEntityTypeId(); $bundle = $group->bundle(); // As Og::isGroup depends on this config, we retrieve it here and set it as @@ -151,14 +153,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 +182,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 +196,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); @@ -229,7 +223,7 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt 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); diff --git a/src/OgAccessInterface.php b/src/OgAccessInterface.php index 1ed65dde0..37f1ded10 100644 --- a/src/OgAccessInterface.php +++ b/src/OgAccessInterface.php @@ -1,5 +1,7 @@ Date: Tue, 28 Jul 2020 21:05:18 +0300 Subject: [PATCH 08/18] Update documentation. --- src/OgAccessInterface.php | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/OgAccessInterface.php b/src/OgAccessInterface.php index 37f1ded10..c15522261 100644 --- a/src/OgAccessInterface.php +++ b/src/OgAccessInterface.php @@ -14,7 +14,19 @@ interface OgAccessInterface { /** - * Determines whether a user can perform a given operation on a given group. + * Determines whether a user has a certain permission in a given group. + * + * The following conditions will result in a positive result: + * - The user is the global super user (UID 1). + * - The user has the global permission to administer all organic groups. + * - The user is the owner of the group, and OG has been configured to allow + * full access to the group owner. + * - The user has the role of administrator in the group. + * - The user has a role in the group that specifically grants the permission. + * - The user is not a member of the group, and the permission has been + * granted to non-members. + * + * The access result can be altered by implementing hook_og_user_access(). * * All access checks in OG should go through this function. This way we * guarantee consistent behavior, and ensure that the superuser and group @@ -22,8 +34,8 @@ interface OgAccessInterface { * * @param \Drupal\Core\Entity\EntityInterface $group * The group entity. - * @param string $operation - * The entity operation being checked for. + * @param string $permission + * The permission being checked. * @param \Drupal\Core\Session\AccountInterface $user * (optional) The user to check. Defaults to the current user. * @param bool $skip_alter From 6bc4f58bd355eccfb07c8e7e0a976bd5567adaa0 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 28 Jul 2020 21:05:43 +0300 Subject: [PATCH 09/18] Passing in operations to the user access check is no longer supported. This has reverted back to the original functionality of checking only permissions so this is consistent with the D7 implementation. --- tests/src/Kernel/Access/OgEntityAccessTest.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) 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()); From b5241af3ddfc42366a17a05997c7d3947efe3176 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 28 Jul 2020 21:39:55 +0300 Subject: [PATCH 10/18] Remove the test that was checking that entity operations are mapped to permissions. Checking access on entity operations is no longer supported in the standard user access check. It now only accepts permissions as was the original intention. --- tests/src/Functional/GroupUpdateTest.php | 182 ----------------------- 1 file changed, 182 deletions(-) delete mode 100644 tests/src/Functional/GroupUpdateTest.php diff --git a/tests/src/Functional/GroupUpdateTest.php b/tests/src/Functional/GroupUpdateTest.php deleted file mode 100644 index e4817efb3..000000000 --- a/tests/src/Functional/GroupUpdateTest.php +++ /dev/null @@ -1,182 +0,0 @@ -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'], - ]; - } - -} From 5f6d8a990c965f86f25c4aeefa13150f349fba7e Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 29 Jul 2020 16:03:25 +0300 Subject: [PATCH 11/18] No need to inject the GroupAudienceHelper service, the GroupTypeManager also provides this information. --- og.services.yml | 2 +- src/Og.php | 4 +--- src/OgAccess.php | 15 ++------------- tests/src/Unit/OgAccessEntityTestBase.php | 6 +++--- tests/src/Unit/OgAccessTestBase.php | 14 +------------- 5 files changed, 8 insertions(+), 33 deletions(-) diff --git a/og.services.yml b/og.services.yml index 45e31dacf..be1821d89 100644 --- a/og.services.yml +++ b/og.services.yml @@ -21,7 +21,7 @@ services: - { name: 'cache.context'} og.access: class: Drupal\og\OgAccess - arguments: ['@config.factory', '@current_user', '@module_handler', '@og.group_type_manager', '@og.permission_manager', '@og.membership_manager', '@og.group_audience_helper'] + arguments: ['@config.factory', '@current_user', '@module_handler', '@og.group_type_manager', '@og.permission_manager', '@og.membership_manager'] og.context: class: Drupal\og\ContextProvider\OgContext arguments: ['@plugin.manager.og.group_resolver', '@config.factory'] diff --git a/src/Og.php b/src/Og.php index 1b7feb218..34043962a 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 7317f30c7..7439e192f 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -76,13 +76,6 @@ class OgAccess implements OgAccessInterface { */ protected $membershipManager; - /** - * The OG group audience helper. - * - * @var \Drupal\og\OgGroupAudienceHelperInterface - */ - protected $groupAudienceHelper; - /** * Constructs the OgAccess service. * @@ -98,17 +91,14 @@ class OgAccess implements OgAccessInterface { * The permission manager. * @param \Drupal\og\MembershipManagerInterface $membership_manager * The group membership manager. - * @param \Drupal\og\OgGroupAudienceHelperInterface $group_audience_helper - * The OG group audience helper. */ - public function __construct(ConfigFactoryInterface $config_factory, AccountProxyInterface $account_proxy, ModuleHandlerInterface $module_handler, GroupTypeManagerInterface $group_manager, PermissionManagerInterface $permission_manager, MembershipManagerInterface $membership_manager, OgGroupAudienceHelperInterface $group_audience_helper) { + public function __construct(ConfigFactoryInterface $config_factory, AccountProxyInterface $account_proxy, ModuleHandlerInterface $module_handler, GroupTypeManagerInterface $group_manager, PermissionManagerInterface $permission_manager, MembershipManagerInterface $membership_manager) { $this->configFactory = $config_factory; $this->accountProxy = $account_proxy; $this->moduleHandler = $module_handler; $this->groupTypeManager = $group_manager; $this->permissionManager = $permission_manager; $this->membershipManager = $membership_manager; - $this->groupAudienceHelper = $group_audience_helper; } /** @@ -230,8 +220,7 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt } } - $is_group_content = $this->groupAudienceHelper->hasGroupAudienceField($entity_type_id, $bundle); - if ($is_group_content) { + if ($this->groupTypeManager->isGroupContent($entity_type_id, $bundle)) { $cache_tags = $entity_type->getListCacheTags(); // The entity might be a user or a non-user entity. 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()); From 6c3fe28960d39d1700a26e2ddb9245e827aefd28 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 29 Jul 2020 16:58:34 +0300 Subject: [PATCH 12/18] Use correct terminology. ::userAccessEntity() deals with permissions rather than operations. --- src/OgAccess.php | 6 +++--- tests/src/Unit/OgAccessEntityTest.php | 14 +++++++------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 7439e192f..7c5ec3ab7 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -198,7 +198,7 @@ public function userAccess(EntityInterface $group, string $permission, AccountIn /** * {@inheritdoc} */ - public function userAccessEntity($operation, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface { + public function userAccessEntity($permission, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface { $result = AccessResult::neutral(); $entity_type = $entity->getEntityType(); @@ -206,7 +206,7 @@ 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; } @@ -240,7 +240,7 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt // 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); } 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()); } From 74570b507135974e7752e1ec486a01861b0b0bd9 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 29 Jul 2020 17:00:33 +0300 Subject: [PATCH 13/18] Adopt scalar type hints. --- src/OgAccess.php | 4 ++-- src/OgAccessInterface.php | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/OgAccess.php b/src/OgAccess.php index 7c5ec3ab7..86acfe476 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -198,7 +198,7 @@ public function userAccess(EntityInterface $group, string $permission, AccountIn /** * {@inheritdoc} */ - public function userAccessEntity($permission, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface { + public function userAccessEntity(string $permission, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface { $result = AccessResult::neutral(); $entity_type = $entity->getEntityType(); @@ -262,7 +262,7 @@ public function userAccessEntity($permission, EntityInterface $entity, AccountIn /** * {@inheritdoc} */ - public function userAccessGroupContentEntityOperation($operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL): AccessResultInterface { + public function userAccessGroupContentEntityOperation(string $operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL): AccessResultInterface { // Default to the current user. $user = $user ?: $this->accountProxy->getAccount(); diff --git a/src/OgAccessInterface.php b/src/OgAccessInterface.php index c15522261..3f3b6fca9 100644 --- a/src/OgAccessInterface.php +++ b/src/OgAccessInterface.php @@ -62,7 +62,7 @@ public function userAccess(EntityInterface $group, string $permission, AccountIn * @return \Drupal\Core\Access\AccessResultInterface * An access result object. */ - public function userAccessEntity($operation, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface; + public function userAccessEntity(string $operation, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface; /** * Checks access for entity operations on group content entities. @@ -89,7 +89,7 @@ public function userAccessEntity($operation, EntityInterface $entity, AccountInt * * @see \Drupal\og\PermissionManager::getEntityOperationPermissions() */ - public function userAccessGroupContentEntityOperation($operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL): AccessResultInterface; + public function userAccessGroupContentEntityOperation(string $operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL): AccessResultInterface; /** * Resets the static cache. From e27ef9980ef6ec7526eefef4f88130229115de0c Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 29 Jul 2020 17:02:38 +0300 Subject: [PATCH 14/18] Split up ::userAccessEntity() in two methods. This method was previously handling both user permissions and entity operations but this was confusing and problematic. It is now split into two separate methods with a clear scope: one is handling user permissions and the other handles entity operations. --- og.module | 2 +- src/OgAccess.php | 51 ++++++++++++++----- src/OgAccessInterface.php | 38 +++++++++++++- .../OgGroupContentOperationAccessTest.php | 2 +- 4 files changed, 77 insertions(+), 16 deletions(-) diff --git a/og.module b/og.module index 0dfda6370..cfb2791c4 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); if ($access->isAllowed()) { return $access; diff --git a/src/OgAccess.php b/src/OgAccess.php index 86acfe476..0d23a4d41 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -230,16 +230,6 @@ public function userAccessEntity(string $permission, EntityInterface $entity, Ac $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, $permission, $user); if ($user_access->isAllowed()) { return $user_access->addCacheTags($cache_tags); @@ -254,8 +244,45 @@ public function userAccessEntity(string $permission, EntityInterface $entity, Ac $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->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; } diff --git a/src/OgAccessInterface.php b/src/OgAccessInterface.php index 3f3b6fca9..88190fef4 100644 --- a/src/OgAccessInterface.php +++ b/src/OgAccessInterface.php @@ -50,7 +50,41 @@ interface OgAccessInterface { public function userAccess(EntityInterface $group, string $permission, AccountInterface $user = NULL, bool $skip_alter = FALSE): AccessResultInterface; /** - * Check if a user has access to a permission on a certain entity context. + * Determines whether a user has a group permission in a given entity. + * + * This does an exhaustive, but slow, check to discover whether access can be + * granted and works both on groups and group content. It will iterate over + * all groups that are associated with the entity and do a permission check on + * each group. If a passed in entity is both a group and group content, it + * will return a positive result if the user has the requested permission in + * either the entity itself or its parent group(s). + * + * In case you know the specific group you want to check access for then it is + * recommended to use the faster ::userAccess(). + * + * @param string $permission + * The name of the OG permission being checked. This includes both group + * level permissions such as 'subscribe without approval' and group content + * entity operation permissions such as 'edit own article content'. + * @param \Drupal\Core\Entity\EntityInterface $entity + * The entity object. This can be either a group or group content entity. + * @param \Drupal\Core\Session\AccountInterface $user + * (optional) The user object. If empty the current user will be used. + * + * @return \Drupal\Core\Access\AccessResultInterface + * An access result object. + */ + public function userAccessEntity(string $permission, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface; + + /** + * Checks whether a user can perform an operation on a group content entity. + * + * This does an exhaustive, but slow, check to discover whether the operation + * can be performed. It will iterate over all groups that are associated with + * the group content entity and do an operation check on each group. + * + * In case you know the specific group you want to check access for then it is + * recommended to use the faster ::userAccessGroupContentEntityOperation(). * * @param string $operation * The operation to perform on the entity. @@ -62,7 +96,7 @@ public function userAccess(EntityInterface $group, string $permission, AccountIn * @return \Drupal\Core\Access\AccessResultInterface * An access result object. */ - public function userAccessEntity(string $operation, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface; + public function userAccessEntityOperation(string $operation, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface; /** * Checks access for entity operations on group content entities. 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"); } } } From 489ca3610e5f9c6faaac82b362e1c5a6d189b041 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Wed, 29 Jul 2020 17:05:03 +0300 Subject: [PATCH 15/18] Update documentation. --- src/OgAccessInterface.php | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/OgAccessInterface.php b/src/OgAccessInterface.php index 88190fef4..ba3e63c83 100644 --- a/src/OgAccessInterface.php +++ b/src/OgAccessInterface.php @@ -14,7 +14,7 @@ interface OgAccessInterface { /** - * Determines whether a user has a certain permission in a given group. + * Determines whether a user has a group permission in a given group. * * The following conditions will result in a positive result: * - The user is the global super user (UID 1). @@ -35,7 +35,9 @@ interface OgAccessInterface { * @param \Drupal\Core\Entity\EntityInterface $group * The group entity. * @param string $permission - * The permission being checked. + * The name of the OG permission being checked. This includes both group + * level permissions such as 'subscribe without approval' and group content + * entity operation permissions such as 'edit own article content'. * @param \Drupal\Core\Session\AccountInterface $user * (optional) The user to check. Defaults to the current user. * @param bool $skip_alter @@ -87,9 +89,9 @@ public function userAccessEntity(string $permission, EntityInterface $entity, Ac * recommended to use the faster ::userAccessGroupContentEntityOperation(). * * @param string $operation - * The operation to perform on the entity. + * The entity operation, such as "create", "update" or "delete". * @param \Drupal\Core\Entity\EntityInterface $entity - * The entity object. + * The group content entity. * @param \Drupal\Core\Session\AccountInterface $user * (optional) The user object. If empty the current user will be used. * @@ -99,16 +101,14 @@ public function userAccessEntity(string $permission, EntityInterface $entity, Ac public function userAccessEntityOperation(string $operation, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface; /** - * Checks access for entity operations on group content entities. + * Checks access for entity operations on group content in a specific group. * * This checks if the user has permission to perform the requested operation * on the given group content entity according to the user's membership status - * in the given group. There is no formal support for access control on entity - * operations in core, so the mapping of permissions to operations is provided - * by PermissionManager::getEntityOperationPermissions(). + * in the given group. * * @param string $operation - * The entity operation. + * The entity operation, such as "create", "update" or "delete". * @param \Drupal\Core\Entity\EntityInterface $group_entity * The group entity, to retrieve the permissions from. * @param \Drupal\Core\Entity\EntityInterface $group_content_entity @@ -120,8 +120,6 @@ public function userAccessEntityOperation(string $operation, EntityInterface $en * * @return \Drupal\Core\Access\AccessResultInterface * The access result object. - * - * @see \Drupal\og\PermissionManager::getEntityOperationPermissions() */ public function userAccessGroupContentEntityOperation(string $operation, EntityInterface $group_entity, EntityInterface $group_content_entity, AccountInterface $user = NULL): AccessResultInterface; From da7f9d9e6d67f8ee3126e86d45ba56ae6f5c4d93 Mon Sep 17 00:00:00 2001 From: Maarten Segers Date: Thu, 30 Jul 2020 20:15:06 +0200 Subject: [PATCH 16/18] 676 Add support for php 7.4 --- .travis.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 33cd7a1c3..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 From a35ca50682e69e608ce7215049c41f961fbb45c1 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Thu, 30 Jul 2020 21:39:10 +0300 Subject: [PATCH 17/18] Restore access checks on entity operations for both groups and group content. This benefits og_entity_access() which relies on this to work for any entity. --- src/EventSubscriber/OgEventSubscriber.php | 2 +- src/OgAccess.php | 28 +++++++++++++++++++++++ src/OgAccessInterface.php | 13 +++++++---- 3 files changed, 38 insertions(+), 5 deletions(-) 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/OgAccess.php b/src/OgAccess.php index 0d23a4d41..8f6cb68c8 100644 --- a/src/OgAccess.php +++ b/src/OgAccess.php @@ -34,6 +34,13 @@ class OgAccess implements OgAccessInterface { */ const UPDATE_GROUP_PERMISSION = 'update group'; + /** + * Maps entity operations performed on groups to group level permissions. + */ + const OPERATION_GROUP_PERMISSION_MAPPING = [ + 'update' => self::UPDATE_GROUP_PERMISSION, + ]; + /** * The config factory. * @@ -259,6 +266,27 @@ public function userAccessEntityOperation(string $operation, EntityInterface $en $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(); diff --git a/src/OgAccessInterface.php b/src/OgAccessInterface.php index ba3e63c83..c3a58c42c 100644 --- a/src/OgAccessInterface.php +++ b/src/OgAccessInterface.php @@ -79,11 +79,16 @@ public function userAccess(EntityInterface $group, string $permission, AccountIn public function userAccessEntity(string $permission, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface; /** - * Checks whether a user can perform an operation on a group content entity. + * Checks whether a user can perform an operation on a given entity. * * This does an exhaustive, but slow, check to discover whether the operation - * can be performed. It will iterate over all groups that are associated with - * the group content entity and do an operation check on each group. + * can be performed. It works both with groups and group content entities. It + * will iterate over all groups that are associated with the entity and check + * if the user is allowed to perform the entity operation on the group content + * entity according to their role within each group. If a passed in entity is + * both a group and group content, it will return a positive result if the + * user has permission to perform the operation in either the entity itself or + * one of its parent group(s). * * In case you know the specific group you want to check access for then it is * recommended to use the faster ::userAccessGroupContentEntityOperation(). @@ -91,7 +96,7 @@ public function userAccessEntity(string $permission, EntityInterface $entity, Ac * @param string $operation * The entity operation, such as "create", "update" or "delete". * @param \Drupal\Core\Entity\EntityInterface $entity - * The group content entity. + * The entity object. This can be either a group or group content entity. * @param \Drupal\Core\Session\AccountInterface $user * (optional) The user object. If empty the current user will be used. * From 34a02f0c227cc90344a7455a4eeee1f29a3f812a Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Sun, 2 Aug 2020 18:32:57 +0300 Subject: [PATCH 18/18] Add @see links to both methods that refer to faster alternatives in their documentation. --- src/OgAccessInterface.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/OgAccessInterface.php b/src/OgAccessInterface.php index c3a58c42c..59fb7d557 100644 --- a/src/OgAccessInterface.php +++ b/src/OgAccessInterface.php @@ -75,6 +75,8 @@ public function userAccess(EntityInterface $group, string $permission, AccountIn * * @return \Drupal\Core\Access\AccessResultInterface * An access result object. + * + * @see \Drupal\og\userAccess(); */ public function userAccessEntity(string $permission, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface; @@ -102,6 +104,8 @@ public function userAccessEntity(string $permission, EntityInterface $entity, Ac * * @return \Drupal\Core\Access\AccessResultInterface * An access result object. + * + * @see \Drupal\og\userAccessGroupContentEntityOperation(); */ public function userAccessEntityOperation(string $operation, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface;