Skip to content

Clean up and clarify the OgAccess unit tests #230

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
amitaibu merged 5 commits into from
Jun 29, 2016
Merged

Clean up and clarify the OgAccess unit tests #230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dc33a3a
Clean up and document the OgAccess unit tests.
pfrenssen Jun 10, 2016
2a9bde1
Merge remote-tracking branch 'origin/8.x-1.x' into cleanup-og-access-…
pfrenssen Jun 10, 2016
18c49b2
Merge remote-tracking branch 'origin/8.x-1.x' into cleanup-og-access-…
pfrenssen Jun 14, 2016
77c3c94
Merge remote-tracking branch 'origin/8.x-1.x' into cleanup-og-access-…
pfrenssen Jun 27, 2016
7ac7f60
Rename method, this actually returns test permissions, not operations.
pfrenssen Jun 27, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions tests/src/Unit/OgAccessEntityTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,13 @@ class OgAccessEntityTest extends OgAccessEntityTestBase {

/**
* @coversDefaultmethod ::userAccessEntity
* @dataProvider operationProvider
* @dataProvider permissionsProvider
*/
public function testAccessByOperation($operation) {
$group_entity = $this->groupEntity();
$group_entity->isNew()->willReturn(FALSE);
$user_access = $this->ogAccess->userAccessEntity($operation, $this->entity->reveal(), $this->user->reveal());

$user_access = $this->ogAccess->userAccessEntity($operation, $this->groupContentEntity->reveal(), $this->user->reveal());

// We populate the allowed permissions cache in
// OgAccessEntityTestBase::setup().
Expand All @@ -33,7 +34,7 @@ public function testAccessByOperation($operation) {

/**
* @coversDefaultmethod ::userAccessEntity
* @dataProvider operationProvider
* @dataProvider permissionsProvider
*/
public function testEntityNew($operation) {
$group_entity = $this->groupEntity();
Expand All @@ -44,11 +45,11 @@ public function testEntityNew($operation) {

/**
* @coversDefaultmethod ::userAccessEntity
* @dataProvider operationProvider
* @dataProvider permissionsProvider
*/
public function testGetEntityGroups($operation) {
$this->user->hasPermission(OgAccess::ADMINISTER_GROUP_PERMISSION)->willReturn(TRUE);
$user_entity_access = $this->ogAccess->userAccessEntity($operation, $this->entity->reveal(), $this->user->reveal());
$user_entity_access = $this->ogAccess->userAccessEntity($operation, $this->groupContentEntity->reveal(), $this->user->reveal());
$this->assertTrue($user_entity_access->isAllowed());
}

Expand Down
42 changes: 26 additions & 16 deletions tests/src/Unit/OgAccessEntityTestBase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,20 @@

class OgAccessEntityTestBase extends OgAccessTestBase {

protected $entity;

/**
* A test group content entity.
*
* @var \Drupal\Core\Entity\ContentEntityInterface|\Prophecy\Prophecy\ObjectProphecy
*/
protected $groupContentEntity;

/**
* {@inheritdoc}
*/
public function setup() {
parent::setUp();

$field_definition = $this->prophesize(FieldDefinitionInterface::class);
$field_definition->getType()->willReturn(OgGroupAudienceHelper::NON_USER_TO_GROUP_REFERENCE_FIELD_TYPE);
$field_definition->getFieldStorageDefinition()
->willReturn($this->prophesize(FieldStorageDefinitionInterface::class)->reveal());
$field_definition->getSetting("handler_settings")->willReturn([]);
$field_definition->getName()->willReturn($this->randomMachineName());

// Mock a group content entity.
$entity_type_id = $this->randomMachineName();
$bundle = $this->randomMachineName();

Expand All @@ -44,15 +46,24 @@ public function setup() {
$entity_type->isSubclassOf(FieldableEntityInterface::class)->willReturn(TRUE);
$entity_type->id()->willReturn($entity_type_id);

$this->entity = $this->prophesize(ContentEntityInterface::class);
$this->entity->id()->willReturn($entity_id);
$this->entity->bundle()->willReturn($bundle);
$this->entity->isNew()->willReturn(FALSE);
$this->entity->getEntityType()->willReturn($entity_type->reveal());
$this->entity->getEntityTypeId()->willReturn($entity_type_id);
$this->groupContentEntity = $this->prophesize(ContentEntityInterface::class);
$this->groupContentEntity->id()->willReturn($entity_id);
$this->groupContentEntity->bundle()->willReturn($bundle);
$this->groupContentEntity->isNew()->willReturn(FALSE);
$this->groupContentEntity->getEntityType()->willReturn($entity_type->reveal());
$this->groupContentEntity->getEntityTypeId()->willReturn($entity_type_id);

// The group manager is expected to declare that this is not a group.
$this->groupManager->isGroup($entity_type_id, $bundle)->willReturn(FALSE);

// Mock retrieval of field definitions.
$field_definition = $this->prophesize(FieldDefinitionInterface::class);
$field_definition->getType()->willReturn(OgGroupAudienceHelper::NON_USER_TO_GROUP_REFERENCE_FIELD_TYPE);
$field_definition->getFieldStorageDefinition()
->willReturn($this->prophesize(FieldStorageDefinitionInterface::class)->reveal());
$field_definition->getSetting('handler_settings')->willReturn([]);
$field_definition->getName()->willReturn($this->randomMachineName());

$entity_field_manager = $this->prophesize(EntityFieldManagerInterface::class);
$entity_field_manager->getFieldDefinitions($entity_type_id, $bundle)->willReturn([$field_definition->reveal()]);

Expand All @@ -64,7 +75,6 @@ public function setup() {
$entity_type_manager->getDefinition($entity_type_id)->willReturn($entity_type->reveal());
$entity_type_manager->getStorage($group_type_id)->willReturn($storage->reveal());


$container = \Drupal::getContainer();
$container->set('entity_type.manager', $entity_type_manager->reveal());
$container->set('entity_field.manager', $entity_field_manager->reveal());
Expand Down
28 changes: 20 additions & 8 deletions tests/src/Unit/OgAccessHookTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
<?php

/**
* @file
* Contains \Drupal\Tests\og\Unit\OgAccessHookTest.
*/

namespace Drupal\Tests\og\Unit;

use Drupal\Core\Entity\EntityInterface;
use Drupal\og\OgAccess;

Expand All @@ -14,6 +10,9 @@
*/
class OgAccessHookTest extends OgAccessEntityTestBase {

/**
* {@inheritdoc}
*/
public function setUp() {
parent::setUp();
// Since this is a unit test, we don't enable the module. However, we test
Expand All @@ -22,25 +21,38 @@ public function setUp() {
}

/**
* @dataProvider operationProvider
* Tests that an entity which is not a group or group content is ignored.
*
* @dataProvider permissionsProvider
*/
public function testNotContentEntity($operation) {
$entity = $this->prophesize(EntityInterface::class);
$access = og_entity_access($entity->reveal(), $operation, $this->user->reveal());

// An entity which is not a group or group content should always return
// neutral, since we have no opinion over it.
$this->assertTrue($access->isNeutral());
}

/**
* @dataProvider operationProvider
* Tests that an administrator with 'administer group' permission has access.
*
* @dataProvider permissionsProvider
*/
public function testGetEntityGroups($operation) {
$this->user->hasPermission(OgAccess::ADMINISTER_GROUP_PERMISSION)->willReturn(TRUE);
$user_entity_access = og_entity_access($this->entity->reveal(), $operation, $this->user->reveal());
$user_entity_access = og_entity_access($this->groupContentEntity->reveal(), $operation, $this->user->reveal());

// @todo This is strange, 'view' is not part of the operations supplied by
// ::permissionsProvider(). And why would a group administrator be allowed
// access to all operations, except 'view'? Shouldn't this also return
// 'allowed'?

@pfrenssen pfrenssen Jun 10, 2016

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is puzzling to me. It seems to be the intention to hand off the 'view' permission to core, but this is counterintuitive. What would be the use case of not granting 'view' access to a group admin, while all the other permissions and operations are granted?

@amitaibu amitaibu Jun 15, 2016

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to go in and check - I don't really remember what's going on here ;)

@amitaibu amitaibu Jun 15, 2016 •
edited
Loading

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is related to the incorrect crud operation To permission name bug/ feature we have.

The gist of things is - OG's hook_entity_access() cares only about non-view operations. If it's a view we are neutral about it. The reason is that for example node access should be determined by the node grants, and not on the fly.

So this

$user_entity_access = og_entity_access($this->entity->reveal(), $operation, $this->user->reveal());

should probably change to something like

$permission_name = OgPermissionHandler::getPermissionFromEntityOperation($entity, 'view');
$user_entity_access = og_entity_access($this->entity->reveal(), $permissions_name, $this->user->reveal());

It can probably be a follow up, as it's out of the scope of this PR. So maybe open an issue instead of the @todo?

@amitaibu amitaibu Jun 15, 2016

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry I mean $permission_name = OgPermissionHandler::getPermissionFromEntityOperation($entity, 'view'); should happen inside \Drupal\og\OgAccess::userAccessEntity, so the test can remain as is.

if ($operation == 'view') {
$this->assertTrue($user_entity_access->isNeutral());
}
else {
$this->assertTrue($user_entity_access->isAllowed());
}
}

}
10 changes: 5 additions & 5 deletions tests/src/Unit/OgAccessTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class OgAccessTest extends OgAccessTestBase {

/**
* @coversDefaultmethod ::userAccess
* @dataProvider operationProvider
* @dataProvider permissionsProvider
*/
public function testUserAccessNotAGroup($operation) {
$this->groupManager->isGroup($this->entityTypeId, $this->bundle)->willReturn(FALSE);
Expand All @@ -27,7 +27,7 @@ public function testUserAccessNotAGroup($operation) {

/**
* @coversDefaultmethod ::userAccess
* @dataProvider operationProvider
* @dataProvider permissionsProvider
*/
public function testAccessByOperation($operation) {
$user_access = $this->ogAccess->userAccess($this->group, $operation, $this->user->reveal());
Expand All @@ -41,7 +41,7 @@ public function testAccessByOperation($operation) {

/**
* @coversDefaultmethod ::userAccess
* @dataProvider operationProvider
* @dataProvider permissionsProvider
*/
public function testUserAccessUser1($operation) {
$this->user->id()->willReturn(1);
Expand All @@ -51,7 +51,7 @@ public function testUserAccessUser1($operation) {

/**
* @coversDefaultmethod ::userAccess
* @dataProvider operationProvider
* @dataProvider permissionsProvider
*/
public function testUserAccessAdminPermission($operation) {
$this->user->hasPermission(OgAccess::ADMINISTER_GROUP_PERMISSION)->willReturn(TRUE);
Expand All @@ -61,7 +61,7 @@ public function testUserAccessAdminPermission($operation) {

/**
* @coversDefaultmethod ::userAccess
* @dataProvider operationProvider
* @dataProvider permissionsProvider
*/
public function testUserAccessOwner($operation) {
$this->config->get('group_manager_full_access')->willReturn(TRUE);
Expand Down
53 changes: 43 additions & 10 deletions tests/src/Unit/OgAccessTestBase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
<?php

/**
* @file
* Contains \Drupal\Tests\og\Unit\OgAccessTestBase.
*/

namespace Drupal\Tests\og\Unit;

use Drupal\Core\Cache\Context\CacheContextsManager;
Expand All @@ -22,29 +17,50 @@
use Drupal\user\EntityOwnerInterface;
use Prophecy\Argument;

/**
* Base class for tests of the OgAccess class.
*/
class OgAccessTestBase extends UnitTestCase {

/**
* The mocked config handler.
*
* @var \Drupal\Core\Config\Config|\Prophecy\Prophecy\ObjectProphecy
*/
protected $config;

/**
* @var \Drupal\user\UserInterface
* A mocked test user.
*
* @var \Drupal\user\UserInterface|\Prophecy\Prophecy\ObjectProphecy
*/
protected $user;

/**
* The entity type ID of the test group.
*
* @var string
*/
protected $entityTypeId;

/**
* The bundle ID of the test group.
*
* @var string
*/
protected $bundle;

/**
* The mocked test group.
*
* @var \Drupal\Core\Entity\EntityInterface|\Prophecy\Prophecy\ObjectProphecy
*/
protected $group;

/**
* @var \Drupal\og\GroupManager
* The mocked group manager.
*
* @var \Drupal\og\GroupManager|\Prophecy\Prophecy\ObjectProphecy
*/
protected $groupManager;

Expand Down Expand Up @@ -87,6 +103,7 @@ public function setUp() {
$this->config->getCacheTags()->willReturn([]);
$this->config->getCacheMaxAge()->willReturn(0);

// Create a mocked test user.
$this->user = $this->prophesize(AccountInterface::class);
$this->user->isAuthenticated()->willReturn(TRUE);
$this->user->id()->willReturn(2);
Expand All @@ -106,6 +123,7 @@ public function setUp() {

$entity_id = 20;

// Mock all dependencies for the system under test.
$account_proxy = $this->prophesize(AccountProxyInterface::class);
$module_handler = $this->prophesize(ModuleHandlerInterface::class);

Expand Down Expand Up @@ -148,7 +166,9 @@ public function setUp() {

$reflection_property->setValue($values);

// Set the allowed permissions cache.
// Set the allowed permissions cache. This simulates that the access results
// have been retrieved from the database in an earlier pass. This saves us
// from having to mock all the database interaction.
$r = new \ReflectionClass('Drupal\og\OgAccess');
$reflection_property = $r->getProperty('permissionsCache');
$reflection_property->setAccessible(TRUE);
Expand All @@ -162,19 +182,26 @@ public function setUp() {
}

/**
* Returns a mocked test group.
*
* @param bool $is_owner
* Whether or not this test group should be owned by the test user which is
* used in the test.
*
* @return \Drupal\Core\Entity\EntityInterface
* @return \Drupal\Core\Entity\EntityInterface|\Prophecy\Prophecy\ObjectProphecy
* The test group.
*/
protected function groupEntity($is_owner = FALSE) {
$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->getEntityTypeId()->willReturn($this->entityTypeId);
$group_entity->bundle()->willReturn($this->bundle);
$group_entity->id()->willReturn($this->randomMachineName());

return $this->addCache($group_entity);
}

Expand All @@ -188,7 +215,13 @@ protected function addCache($prophecy) {
return $prophecy;
}

public function operationProvider() {
/**
* Provides permissions to use in access tests.
*
* @return array
* An array of test permissions.
*/
public function permissionsProvider() {
return [
// In the unit tests we don't really care about the permission name - it
// can be an arbitrary string; except for OgAccessTest::testUserAccessAdminPermission
Expand Down