Fix permissions for non-administrators

[pfrenssen]

The logic in og_entity_access() is faulty. It is calling OgAccess::userAccessEntity() with the operation 'administer group', but this is invalid, the only allowed entity operations are 'view', 'create', 'update' and 'delete'.

It also seems that some of the other code in og_entity_access() are earlier attempts for working around this bug.

The solution seems pretty simple, if the user has the 'administer group' permission, then we simply return AccessResult::neutral(), which will grant access according to the user's normal privileges.

Let's see what this breaks.

[pfrenssen]

This test is failing:

  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());
    if ($operation == 'view') {
      $this->assertTrue($user_entity_access->isNeutral());
    }
    else {
      $this->assertTrue($user_entity_access->isAllowed());
    }
  }

Shouldn't this be simply $this->assertTrue($user_entity_access->isNeutral()) for both 'view' and other operations?

Neutral means to let the normal entity access handler take care of it. If this is OK for viewing the group content, then it should also be OK for the other operations right?

To me this seems strange, if we allow for editing and deleting, then we should definitely allow for viewing.

[pfrenssen]

Ok I think that this is the actual intention.

• If the passed in entity is not a group, or group content, then we don't care: neutral.
• If we have 'administer group', we grant allowed. This will trump any other modules that would return neutral, but other modules can still return forbidden if they really don't want group admins to access this content.
• Do not special case the view operation, just let this pass through OgAccess::userAccessEntity() so it can work its magic. I don't see why view would be any different and always return neutral.

[pfrenssen]

What we also need is a comprehensive set of KernelTests that check all access related use cases. Unit tests don't cut it in this case. I suppose we had a bunch of access tests in D7, it wouldn't be bad to start porting these.

[pfrenssen]

I have restored the functionality that returns AccessResult::neutral() for the view operation. Looking at D7 this seems to be the intention.

Here is the implementation of og_list_permissions() in Drupal 7: it only provides per-group content type permissions for 'create', 'update', and 'delete', not for 'view'.

function og_list_permissions($type) {
  $info = node_type_get_type($type);
  $type = check_plain($info->type);
  $perms = array();

  // Check type is of group content.
  if (og_is_group_content_type('node', $type)) {
    // Build standard list of node permissions for this type.
    $perms += array(
      "create $type content" => array(
        'title' => t('Create %type_name content', array('%type_name' => $info->name)),

      ),
      "update own $type content" => array(
        'title' => t('Edit own %type_name content', array('%type_name' => $info->name)),
      ),
      "update any $type content" => array(
        'title' => t('Edit any %type_name content', array('%type_name' => $info->name)),
      ),
      "delete own $type content" => array(
        'title' => t('Delete own %type_name content', array('%type_name' => $info->name)),
      ),
      "delete any $type content" => array(
        'title' => t('Delete any %type_name content', array('%type_name' => $info->name)),
      ),
    );

    if (!module_exists('entityreference_prepopulate')) {
      // We allow the create permission only on members, as otherwise we would
      // have to iterate over every single group to decide if the user has
      // permissions for it.
      $perms["create $type content"]['roles'] = array(OG_AUTHENTICATED_ROLE);
    }

    // Add default permissions.
    foreach ($perms as $key => $value) {
      $perms[$key]['default role'] = array(OG_AUTHENTICATED_ROLE);
    }
  }
  return $perms;
}

The test is failing because of a random failure in OgComplexWidgetTest. I have made an issue for it: #215.

[damiankloip]

I think when this was originally added we were actually checking a permission not an operation. So this changed at some point and the function didn't follow?

[pfrenssen]

That's very well possible, that would explain the strange implementation.

[damiankloip]

Well, only saying it because I remember adding it originally but I am pretty sure it was before there was a userAccessEntity. Maybe. I think... :P

[amitaibu]

It is calling OgAccess::userAccessEntity() with the operation 'administer group', but this is invalid, the only allowed entity operations are 'view', 'create', 'update' and 'delete'.

Not exactly :) The idea of userAccessEntity (which we might want to rename to userAccessByEntity) is to allow OG to do the heavy lifting in checking if a certain user has access to do an OG related operation on an entity.

That entity can be:

• Not related to OG at all
• A group entity
• A group entity that is also a group content
• A group content
• A group content attached to multiple groups

So basically this function should figure out for us what's the relation of the entity to OG, and then to iterate over all the possible cases, and return early if an access was granted.

Also, fully agree we need Kernel test to assert the functionality:)

[pfrenssen]

I still don't understand it, how is 'administer group' an OG related operation on an entity? Operations are 'view', 'edit', 'delete' and 'create'. 'Administer group' is a permission.

[amitaibu]

Let's take an imaginary OG permission called send this content to group member by email.
And let's say we have some content (e.g. node/10). We want to know if user Alice is allowed to create that operation on that entity.

But we don't know yet what is the relation of the entity to OG. Is it a group? A group content? A group content with multiple groups?

userAccessByEntity does the heavy lifting - its finds what the entity is, and then under the hood runs userAccess iterating over the groups the entity is related to.

If not clear, we can have a quick skype :)

[pfrenssen]

OK that's clear, but then we shouldn't call it an operation, but a permission, that seems to be the whole cause of my confusion :)

[pfrenssen]

The only call to userAccessEntity() is now in og_entity_access(), which only deals with operations, not with permissions. If userAccessEntity() is expected to check for permissions, then it should not be called there, because it won't pass any permissions such as 'edit own article nodes', instead it will pass 'update'.

Maybe we should make a clear separation between checking access based on permissions, and access based on operations.

[amitaibu]

The only call to userAccessEntity() is now in og_entity_access(), which only deals with operations, not with permissions.

Indeed, this is wrong in OG8. It should follow the same logic we have in OG7's hook_node_access()

[pfrenssen]

@amitaibu do you want the separation of permissions and operations in scope of this ticket? Or test coverage?

I didn't do test coverage yet since none of this is currently covered by a proper integration test, and I'm afraid that any proper test coverage will be greatly enlarging the scope of this.

I'd like to keep the scope of this manageable. If you want I can look into providing a single dedicated test that proves the bug and its fix, and then later on we can provide more complete coverage.

This is blocking a feature request in our project at the moment. We have a role similar to a 'group moderator' which has elevated permissions inside their groups, but they do not have 'administer group' permissions.

[amitaibu]

Sure, I'm ok with keeping this PR with a small scope (and we can have tests as follow up), but I think this part is still wrong. For example, if the bundle == 'article' and operation == 'update', it should be converted to something like:

$access = OgAccess::userAccessEntity('update own article content', $entity, $account) && $entity>getUserId() == $account->uid || OgAccess::userAccessEntity('update any article content', $entity, $account);

[pfrenssen]

Ah! I'm actually tackling that in #217. In this issue this is not touched, this is exactly the same code as it was before.

#217 is depending on #196 so it is unreviewable at the moment, but the commit that will interest you is this one: 6aca08c

[amitaibu]

👍

[amitaibu]

in that case, it's ready for merge, right?

[pfrenssen]

Yes this should be good to go. I'll make a followup issue to provide test coverage.

[amitaibu]

Thanks.