Check group content entity operation access

[pfrenssen]

This is a followup for #192. Now that we can assign default roles to users which are populated with a set of default permissions for doing CRUD operations on group content, we should use these when checking access.

This PR will e.g. allow members to edit nodes in a group if they have the OG permission "edit any node_type node".

[pfrenssen]

This builds on #192 so it contains all that code. The actual implementation is in the last commit https://github.com/amitaibu/og/pull/217/commits/6aca08c59cb9060634eb7991b855215284a2c374.

This still needs tests.

[pfrenssen]

I'm using static methods because I'm following the established pattern, but this is now going to break the unit tests again, since I am statically calling the PermissionManager service. This is going to require some trickery again to make the tests pass :(

[pfrenssen]

I have converted OgAccess into a service in #226. This solves the problem with the unit tests.

Merging this was a bit awkward, I'm going to rebase this branch on top of #226 so that the commit progression is more logical.

[amitaibu]

This will require a re-roll

[pfrenssen]

Picking this back up, will first merge in latest changes and try to get a green test run, then I'll have a look to make a list of what still needs to be done here.

[pfrenssen]

I am thinking how we can best provide the entity operation permissions.

We have two types of permissions:

1. "Group level permissions", such as 'subscribe without approval' and 'manage users'. These are currently provided by the PermissionEvent subscribers.
2. "Entity operation level permissions" on group content, so that we can allow members to create and edit group content.

In D7 we only supplied a hard coded set of permissions for the Node module, but in D8's everything-is-an-entity world we should extend this to all entities. A good example use case is the OG Menu module, in which each menu is group content type of the custom OgMenuInstance entity. In order to allow certain group roles to create or edit OG Menus we need know that OG Menu exposes this entity type, and that it supports certain operations on it.

In core we don't have this information currently (ref. #222). We need to retrieve it somehow, through a hook or event listener.

I am now thinking of extending the current PermissionEvent to handle both the group level and entity operation level permissions. This seems better from a DX perspective, so that modules don't need to implement two subscribers to declare a set of permissions.

The problem with the entity operation permissions in Drupal is that there is no formal framework, and the permissions are based on human language. A good example is the basic 'edit own article content' permission - neither the operation ('update') or the entity type ('node') are even mentioned in the permission, it's impossile to parse the needed information out of it.

So we need to be able to associate a permission with an operation, and with an entity type and bundle. We also need to distinguish between a user having permission to operate on their own content, or on all content: ('edit own article content' vs 'edit any article content'). We also need the machine name and the title.

It seems like it would be a good idea to use a class for this, so something like the 'edit any article content' can be provided in the permission subscriber using a new method like setEntityOperationPermission():

  public function provideDefaultOgPermissions(PermissionEventInterface $event) {
    $permission = new EntityOperationPermission();
    $permission
      ->setName('edit any article content')
      ->setTitle(t('Article: edit any content'))
      ->setDescription(t('Allows to edit any node of type article'))
      ->setEntityType('node')
      ->setBundle('article')
      ->setOwnership('any')
      ->setOperation('update');
    $event->setEntityOperationPermission($permission);

    // Here is an example of a normal group-level permission being set in the
    // same event subscriber.
    $event->setPermissions([
      'administer group' => [
        'title' => t('Administer group'),
        'description' => t('Manage group members and content in the group.'),
        'default roles' => [OgRoleInterface::ADMINISTRATOR],
        'restrict access' => TRUE,
      ],
    ]);
  }

For basic CRUD operations we can supply all permissions out of the box with generic wording such as update own $bundle_id $entity_type_id. We can then allow modules to alter this if the generic wording is not to their liking.

[pfrenssen]

On second thought, I'm going to use an array structure for the moment to pass the data, to be consistent with the group level permissions. This would make it a lot less work to implement. We can later on still make an object to store the permissions.

If we do it like this we don't even need two separate methods, we can pass everything in the same method, and distinguish between the two if the 'operation' key is present.

So the example implementation above would instead look like this:

  public function provideDefaultOgPermissions(PermissionEventInterface $event) {
    $event->setPermissions([
      // Example entity operation type permission.
      'edit any article content' => [
        'title' => t('Article: edit any content'),
        'description' => t('Allows to edit any node of type article'),
        'entity type' => 'node',
        'bundle' => 'article',
        'ownership' => 'any',
        'operation' => 'update',
      ],

      // Example group-level type permission.
      'administer group' => [
        'title' => t('Administer group'),
        'description' => t('Manage group members and content in the group.'),
        'default roles' => [OgRoleInterface::ADMINISTRATOR],
        'restrict access' => TRUE,
      ],
    ]);
  }

[amitaibu]

This should be split into two methods. One is declaring the permissions that might be used. And the second is the permissions associated with a certain role.

(As you could have a role with no permissions at all)

[amitaibu]

Also my thoughts for providing default roles is that we maybe should pass a partially applied OgRole instead of the array.

And when we create the roles we just need to fill in the group type and bundle/group I'd

[pfrenssen]

I'm not entirely sure what you mean. This code is intended to be supplied by modules to declare their own OG permissions, they don't know to which roles these permissions apply.

For the passing of the partially applied OgRole, you mean to do this where the event is called? This is already implemented, it currently looks like this (from GroupManager::createPerBundleRoles()):

  protected function createPerBundleRoles($entity_type_id, $bundle_id) {
      // Populate the default permissions.
      $event = new PermissionEvent($entity_type_id, $bundle_id);
      /** @var \Drupal\og\Event\PermissionEventInterface $permissions */
      $permissions = $this->eventDispatcher->dispatch(PermissionEventInterface::EVENT_NAME, $event);
      $properties['permissions'] = array_keys($permissions->filterByDefaultRole($role_name));

      $role = $this->ogRoleStorage->create($properties + $default_properties);
      $role->save();
    }
  }

We could probably create the OgRole first and then put it in the PermissionEvent and apply the permissions to the roles inside the PermissionEvent, but to me it seems like it's not the responsibility of the PermissionEvent.

I would try to avoid a direct hard coupling between OgRole and PermissionEvent, but I agree that the applying of the permissions to the role is also not something that belongs in GroupManager either. The natural place to handle this seems PermissionManager.

[pfrenssen]

If I'm dreaming that we are doing this 100% right we shouldn't be messing with arrays at all :) We could make a proper OgPermission object that has all the relevant getters and setters, and can do validation internally. Then we can set the permissions on the role like this:

  protected function createPerBundleRoles($entity_type_id, $bundle_id) {
    foreach ($this->getDefaultRoles() as $role_name => $default_properties) {
      $properties = [
        'group_type' => $entity_type_id,
        'group_bundle' => $bundle_id,
        'id' => $role_name,
        'role_type' => OgRole::getRoleTypeByName($role_name),
        'permissions' => array_keys(PermissionManager::getDefaultPermissions($entity_type_id, $bundle_id)),
      ];

      $role = $this->ogRoleStorage->create($properties + $default_properties);
      $role->save();
    }
  }

And PermissionManager::getDefaultPermissions() would do the heavy lifting, returning an array of OgPermission objects, keyed by permission name.

In this case we only need the permission names, but the full objects will be handy when doing things like building the permission matrix in the og_ui module.

[amitaibu]

What I think we can do is something like:

protected function createPerBundleRoles($entity_type_id, $bundle_id) {

  foreach ($this->getDefaultRoles() as $og_role) {
    // Get the permissions by the role name.
    $permissions = PermissionManager::getDefaultPermissions($og_role->getLabel());

    /** @var \Drupal\og\Entity\OgRoleInterface $og_role */
    $og_role
      ->setGroupType($group_type)
      ->setId($bundle_id)
      // ...
      ->setPermissions($permissions),
      ->save();
  }

As reference you can follow the flow of OG7's og_roles_override() where it first called to get the default roles and then get the default permissions that should be assigned to the role (by the role name)

[amitaibu]

(btw, Passing the partially applied OgRole (i.e. one that doesn't have the group type/ bundle set yet) would also fix #232 )

[pfrenssen]

OK that's very clear, I like this approach, it lets PermissionManager handle all the logic, including the filtering which is now handled in PermissionEvent. That will reduce PermissionEvent to the simple storage class it is intended to be.

I'll also try to sneak in the fix for #232.

[pfrenssen]

In order to limit the scope of this I'm going to put the returning of partial OgRoles in a separate PR. That will fix #232 and #216 too.

Once that's done I'll return to this ticket.

[pfrenssen]

The returning of partially applied OgRole objects has been proposed in #237. I'm going to merge that code in here.

The next step will be to refactor the filtering of permissions by default role so that it is no longer split between GroupManager and DefaultPermissionEvent but is handled by PermissionManager where it belongs. I will also do this in a separate ticket to limit the scope and make it easier to review.

[pfrenssen]

The filtering is now in PermissionManager in PR https://github.com/amitaibu/og/pull/238.

Next up is the splitting up of permissions in group-level permissions and group-content-level permissions.

@amitaibu suggested in https://github.com/amitaibu/og/pull/217#issuecomment-225601360 to split this into two different methods:

This should be split into two methods. One is declaring the permissions that might be used. And the second is the permissions associated with a certain role.

But I've been thinking about this while working on the related tickets in the last couple of days and I think it would make more sense to keep 1 set of methods, and instead pass different 'permission types'. After all these are different data types that require different properties.

So instead of having two separate methods:

  public function provideDefaultOgPermissions(PermissionEventInterface $event) {
    $event->setGroupContentPermissions([
      // Example permission for an entity operation on group content.
      'edit any article content' => [
        'title' => t('Article: edit any content'),
        'description' => t('Allows to edit any node of type article'),
        'entity type' => 'node',
        'bundle' => 'article',
        'ownership' => 'any',
        'operation' => 'update',
      ],
    ]);

    $event->setGroupPermissions([
      // Example group-level type permission.
      'administer group' => [
        'title' => t('Administer group'),
        'description' => t('Manage group members and content in the group.'),
        'default roles' => [OgRoleInterface::ADMINISTRATOR],
        'restrict access' => TRUE,
      ],
    ]);
  }

We could do something like this:

  public function provideDefaultOgPermissions(PermissionEventInterface $event) {
    $event->setPermissions([
      // Example permission for an entity operation on group content.
      new OgGroupContentPermission([
        'name' => 'edit any article content',
        'title' => t('Article: edit any content'),
        'description' => t('Allows to edit any node of type article'),
        'entity type' => 'node',
        'bundle' => 'article',
        'ownership' => 'any',
        'operation' => 'update',
      ]),

      // Example group-level type permission.
      new OgGroupPermission([
        'name' => 'administer group',
        'title' => t('Administer group'),
        'description' => t('Manage group members and content in the group.'),
        'default roles' => [OgRoleInterface::ADMINISTRATOR],
        'restrict access' => TRUE,
      ]),
    ]);
  }

This would be great from a DX standpoint, since we can then provide convenience methods, and easily alter the existing permissions:

  public function alterExistingOgPermissions(PermissionEventInterface $event) {
    $permission = $event->getGroupContentPermission('node', 'article', 'update');
    $permission->setDescription(t('Some new help text.'));
    $event->setPermission($permission);
  }

[pfrenssen]

Started working on the splitting of the permissions in #240.

[pfrenssen]

This is now missing a full week of work (#238 and #240) and everything that has been merged into 8.x-1.x in the meanwhile. Going to attempt a merge to bring this back up to date.

[pfrenssen]

I'm going to finish this now. This will probably be the last issue I can work on unfortunately. Our development focus will move away from OG when this is in, since we will have all functionality available that our project needs.

[amitaibu]

Well, all your efforts are greatly appreciated!

[pfrenssen]

You're welcome! I really love working on OG. I'll still check in from time to time, and I want to finish the roles & permission UI as well, but I'll have to do this in my spare time. And I have a lot of projects that are competing to get a slice of my spare time :)

[pfrenssen]

I have to fix the failures in OgAccessEntityTestBase here, but I cannot understand it in the current form. It is using things like $this->entity without any documentation, and it is unclear what this entity contains: it might be a group, or group content?

This was researched and clarified in #230, I will merge in that branch so I can at least make sense of it.

[pfrenssen]

I need to add a test for this, and I would ideally like to create a unit test, but I have been looking at it for 30 minutes and it really doesn't seem practical at the moment. The number of static methods involved in the process is just too much, it's completely counterproductive. This PR only adds very little code, but I already spent hours yesterday jumping through hoops to get the existing test coverage green.

A very simple section such as this is incredibly problematic for unit testing because it contains a static method call:

if ($membership = Og::getMembership($user, $group)) {
  foreach ($membership->getRoles() as $role) {
    // ...
  }
}

Because we cannot mock static methods, we now need to mock the entire logic that is happening inside of the method. I started writing this and I got to this point before giving up:

// Mock the group entity that is accessed in Og::getMembership().
$entity_type = $this->prophesize(EntityTypeInterface::class);
$entity_type->id()->willReturn($this->entityTypeId);
$group_entity = $this->prophesize(EntityInterface::class);
$group_entity->getEntityTypeId()->willReturn($this->entityTypeId);
$group_entity->bundle()->willReturn($this->bundle);
$group_entity->id()->willReturn($this->groupId);
$this->addCache($group_entity);

// Mock the creation of an empty OgMembership entity, in case the
// permissions are requested for a user that is not a member of the group.
// For non-members the entity that is returned will have the 'non-member'
// role and the 'pending' state.
$non_membership = $this->prophesize(OgMembership::class);
$non_membership->setUser($this->user->reveal())->willReturn($non_membership->reveal());
$non_membership->setEntityId($this->groupId)->willReturn($non_membership->reveal());
$non_membership->setGroupEntityType($this->entityTypeId)->willReturn($non_membership->reveal());
$non_membership->setState(OgMembership::STATE_PENDING)->willReturn($non_membership->reveal());
$non_member_role_id = implode('-', [
  $this->entityTypeId,
  $this->bundle,
  'non-member',
]);
$non_membership->setRoles([$non_member_role_id])->willReturn($non_membership->reveal());
$entity_type_manager->getStorage('og_membership')->willReturn($storage->reveal());
$storage->create(['type' => OgMembership::TYPE_DEFAULT])->willReturn($non_membership->reveal());
$non_member_role = $this->prophesize(OgRole::class);
$non_membership->getRoles()->willReturn($non_member_role);

This is not only bad from the standpoint of how much work is involved, but we are also testing the underlying implementation, and this is very brittle, touch one line of code and you're looking at rewriting dozens of lines of test code.

Imagine instead that this was written using a service with dependency injection - the calling code would be incredibly similar:

if ($membership = $this->membershipManager()->getMembership($user, $group)) {
  foreach ($membership->getRoles() as $role) {
    // ...
  }
}

And the unit testing would be a dream, instead of mimicking dozens of lines of code, just mock the method call and it return value and you're done:

$non_member_role = $this->prophesize(OgRole::class);
$non_membership = $this->prophesize(OgMembership::class);
$non_membership->getRoles()->willReturn($non_member_role);
$this->membershipManager()->getMembership($this->user, $this->group)->willReturn($non_membership);

In conclusion, I think we should probably not attempt to add any more unit tests for classes that do not consist of 100% OO code. The effort involved and the future maintenance cost is too high. The nice thing is that converting the existing code to OO is not very difficult. We did this for example in #226 and it was relatively quick.

So for this PR I will probably just provide a KernelTest.

[amitaibu]

Yeah, no need to go nuts over the tests. It's probably the right time as you say to start moving classes to be proper services - which classes do you think we should start with?

[pfrenssen]

It seems like my last comment here has not come through. I had answered the question from @amitai and explained what was left to be done here. I probably forgot to submit it, or was possibly on the move with a bad connection.

In short, I have no preference for a certain class to start with. If it was me I'd tackle Og first, since it is the biggest one.

What still needs to be done here is to add a functional test that tries to perform an entity operation and checks if the user has access or not. This will cover the last untested part: og_entity_access().

[amitai]

I think you may have tagged the wrong user.

Sent from my iPad

On Jul 14, 2016, at 05:58, Pieter Frenssen notifications@github.com wrote:

It seems like my last comment here has not come through. I had answered the question from @amitai and explained what was left to be done here. I probably forgot to submit it, or was possibly on the move with a bad connection.

In short, I have no preference for a certain class to start with. If it was me I'd tackle Og first, since it is the biggest one.

What still needs to be done here is to add a functional test that tries to perform an entity operation and checks if the user has access or not. This will cover the last untested part: {{og_entity_access()}}.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[amitaibu]

Hi @amitai nice to meet you, I'm @amitaibu ✋

[amitai]

Hi! 🙌

-DABL (mobile)

On Jul 14, 2016, at 7:52 AM, Amitai Burstein notifications@github.com wrote:

Hi @amitai nice to meet you, I'm @amitaibu ✋

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[pfrenssen]

Oops! I messed up the usernames, sorry!

[amitai]

​no problem ;-)

[pfrenssen]

I added a functional test that checks access to group content by visiting node/ID/edit with the browser. This validates that the code is invoked correctly from the entity access handler, through og_entity_access().

[pfrenssen]

This is ready for review if the tests come back green!

[amitaibu]

Sorry, hectic days. I hope to get to it tomorrow..

[pfrenssen]

No worries!

[amitaibu]

Wonderful, thank you. I've added a few comments. Most important comment is around handeling OgMembership for non-members.

[pfrenssen]

Thanks a lot for the great suggestions :) I'll get to it after the weekend.

[amitaibu]

Needs a re-roll as-well

[pfrenssen]

Getting a false positive for the coding standards check:

| ERROR   | [ ] Expected "\Drupal\og\Entity\OgMembership|null"
|         |     but found "\Drupal\og\Entity\OgMembership|NULL"
|         |     for function return type

[amitaibu]

yeah, it should be null (lower case)

[pfrenssen]

I'm ready with this! I have addressed all the remarks (I think :) )

[amitaibu]

Thanks for keeping up the good work!

Made comments about the "dummy non-member memberships" (dnmm in short 😜). (Also it will need a re-roll)

[pfrenssen]

Addressed the remarks, rerolled, and tests are green :)

[amitaibu]

Wonderful, thanks!

[pfrenssen]

Woohoo!! Thanks a lot!!