Split permissions

[pfrenssen]

This is a followup of #238.

We have two types of permissions:

1. Permissions that apply to the group as a whole, such as 'subscribe to group' and 'administer group'.
2. Permissions that apply to operations on group content, such as 'edit own article content'.

Currently there is no distinction between the two, both are passed by PermissionEvent subscribers as arrays of data, for example:

  public function provideDefaultOgPermissions(PermissionEventInterface $event) {
    $event->setPermissions([
      // 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',
      ],

      // 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,
      ],
    ]);
  }

This is not ideal, both permission types are distinctive data types with different properties, and we need to be able to distinguish between them so they can be applied properly in the right context.

@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 store the data in two separate classes: GroupPermission and GroupContentPermission.

We could do something like this:

  public function provideDefaultOgPermissions(PermissionEventInterface $event) {
    $event->setPermissions([
      // Example permission for an entity operation on group content.
      new GroupContentPermission([
        '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 GroupPermission([
        '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);
  }

[amitaibu]

and instead store the data in two separate classes: GroupPermission and GroupContentPermission.

I'm not sure I understand the distinction. Remember that we try to model our roles and permissions based on core. So we have the site-wide roles/permissions and we have the group level. (e.g. in core we don't have this distinction of SiteWidePermission vs EntityRelatedPermission)

Of I understand correctly new GroupContentPermission([ is trying to deal with entity related operations. This seems to be the job of another contrib module we can have a soft dependency on, and if it doesn't exist hardcode the permissions just for node.

[pfrenssen]

I need some work from #217 now, specifically the declaration of the operations in PermissionManager::getEntityOperationPermissions().

[pfrenssen]

The distinction is between these properties:

• Group level permissions can have a 'roles' property that limits the permission to certain roles. For example only non-members should get the 'subscribe' permission.
• Group content level permissions need the entity type ID and the bundle ID of the group content type to which they apply, as well as the operation (e.g. 'create' or 'update') and the 'ownership scope': e.g. 'edit own article node' vs 'edit any article node'.

We can also distinguish this by checking if the array keys are present, but that is not as nice as having object which can have validation etc.

[amitaibu]

Ok I think I understand what you try to accomplish, but what about cases such as edit group - which class what it be?

[pfrenssen]

Of I understand correctly new GroupContentPermission([ is trying to deal with entity related operations. This seems to be the job of another contrib module we can have a soft dependency on, and if it doesn't exist hardcode the permissions just for node.

Yes, exactly that's where I'm trying to go with this. My immediate use case in our project is to have this for nodes, blocks, OG Menu entities and RDF entities (a custom entity type we have). Maybe GroupContentPermission is not the correct name for it, maybe it should be like EntityOperationPermission.

Do you think it would still be a good idea to limit this only to nodes in D8? We are already using blocks in addition to nodes. It's so useful to have group-specific blocks :) I can also imagine use cases for any other content entity type, like comments and taxonomy terms. Even Shortcut entities could be per-group.

Since everything is an entity now I thought to provide at least the basic CRUD permissions to all entities. It seems weird to me that you are able to select any entity as group content, but then cannot control their permissions in the UI.

The main problem with permissions is that these are not formalized, they are in human readable language. For example 'create own article content' doesn't even mention that this is about nodes!

But this doesn't mean we cannot supply our own set of pro-forma permissions that cover the basic CRUD operations. Even if they don't use the exact wording they will still be usable for as long as they can be configured in the UI in an understandable way. If a certain module doesn't have support for OG permissions yet, then we can offer a UI that allows to configure a generic set of permissions like create own foobar entities of type llama even if in the module it might be worded create own llama foobars. The entity system knows about operations, and we can check if the user owns the entity, so this seems to be entirely possible to me.

This is already partly implemented by the way, the generation of these generic entity operation permissions are handled in PermissionManager::generateCrudPermissionList().

I think there is a bright future for a module such as Entity Operations that can formalize this. If they could implement our event listener (or we implement theirs) then we can properly unlock all operations in the future. And for the moment we can make do with these generic permissions.

[pfrenssen]

Sorry, cross-posted :)

I haven't thought about edit group. This seems to be a special case. This is also an entity operation, but not about group content. I think we should put it with the other group level permissions, and we need to hard code its access in hook_entity_access()

[pfrenssen]

@amitaibu is it OK if I continue with this?

[amitaibu]

is it OK if I continue with this?

Sure :)

[pfrenssen]

Following on https://github.com/amitaibu/og/pull/238#discussion_r67726552 PermissionManager::getDefaultPermissions() should return only default permissions needed when a new group is created, so we should preventively filter the results to only return GroupPermission objects.

We should also add a counterpart to return the default permissions for when a new group content type is created.

[pfrenssen]

I'm going to make PermissionManager::generateEntityOperationPermissionList() protected. This is a helper method for PermissionManager::getEntityOperationPermissions() and I don't see the use for this outside of the parent method.

[pfrenssen]

I'm unhappy with the way I structured the code between the PermissionManager and OgEventSubscriber.

PermissionManager::getDefaultGroupPermissions() now invokes OgEventSubscriber::provideDefaultOgPermissions() which in turn calls PermissionManager::getEntityOperationPermissions() again.

I'm going to flip this around. PermissionManager should be the entry point to retrieve information about the available permissions. I think this should contain four methods:

• getPermissions(): should show the actual permissions that are in use for a group. This will be needed to generate permission tables in the UI. For this we need to inject Config into the class. This seems fine to me, it is to be expected that the permission manager has access to config to retrieve the current permissions from the database.
• getDefaultPermissions(): should get all permissions (group level + entity operations) from the event listeners. This will be used internally to invoke the event listeners. We have no direct use for this outside of this class, but this seems like a nice method to make public anyway, since it is able to provide the full information about our OG permissions to whatever module needs to know about this.
• getDefaultGroupPermissions() should invoke the event listeners, but only return group level permissions. This will be used after a new group is created.
• getDefaultEntityOperationPermissions() should invoke the event listeners, but only return group content entity operation permissions. This will be used after a new group content type is created.

Then OgEventSubscriber gets the task of generating the list of default permissions that OG supports, so the current 2 methods dealing with that will move in there. This seems to make a lot more sense.

[pfrenssen]

This is nearing completion. I have refactored the code as outlined in my previous comment. I have also provided an additional event listener that sets the proper operation permissions for Node group content as a proof of concept. The Kernel\PermissionEventTest proves that it now works as expected: a permission is generated that uses the same name as the core Node module (create test_group_content content instead of the generic create test_group_content node).

This is now matching the D7 functionality. We have the proper permission for Node, and the rest are using generic permissions for now.

I have introduced a few new methods to make it easier to deal with group content operation permissions. These are not identified by name, but by their entity type, bundle, operation and ownership (any/all). These methods now make it easy to check if another module declares e.g. a permission to create nodes of type article, and modify it.

This still needs coverage for the new methods in PermissionEvent, and unit tests for the 2 new classes GroupPermission and GroupContentOperationPermission.

[pfrenssen]

OK thanks! I'll fix it.

[pfrenssen]

I've removed the permissions on node operations so now it matches the way it works in D7. I've also merged in the latest changes from the main branch, and fixed a stray typo.

This is now ready for review if the tests come back green.

[pfrenssen]

Failing on 8.1.x because of https://www.drupal.org/node/2752315. This is already RTBC and has been committed before on 8.2.x. I'm expecting this to land shortly. If it hasn't by tomorrow I will create an issue with a temporary workaround like we had for 8.2.x.

[pfrenssen]

This still needs work as per comment https://github.com/amitaibu/og/pull/240#issuecomment-227524860:

This still needs coverage for the new methods in PermissionEvent, and unit tests for the 2 new classes GroupPermission and GroupContentOperationPermission.

[pfrenssen]

Added test coverage for the new methods in PermissionEvent. I've also done a code coverage analysis and added tests for all lines that were missing, and I fixed some missing functionality that was uncovered by the tests.

[pfrenssen]

This is now done, but is failing due to issue #2757139: Fix visibility of AssertLegacyTrait::buildXPathQuery().

[pfrenssen]

The bug in core is fixed! This is ready for review finally :)

[amitaibu]

Wonderful PR! I've added a few comments.

[pfrenssen]

Addressed the remarks, ready for another look!

[amitaibu]

Merged, Thank you!