OgAccess::userAccess() doesn't differentiate between entity operations and group level permissions

[pfrenssen]

There is a problem with the architecture of OgAcces::userAccess() in that it doesn't make a distinction between two concepts in core that are depending on eachother but are fundamentally different:

Entity operations

Entity operations are basic actions that can be taken on entity level. They are mostly used for CRUD level operations like create, view, update and delete, but there is also a widespread operation view label and entity types are free to check for custom operations. For example the core Workspaces module has an additional operation publish.

Permissions

Permissions are module defined privileges that can be assigned to roles, and granted to users. They allow access to functionalities provided by the module. They might be related to entities (e.g. delete own article content) but they can also be giving access to some functionality which is completely unrelated to entities (e.g. access site in maintenance mode).

How are they related?

When a piece of code wants to perform an entity operation on behalf of a user they will need to check if the user has the rights to do this. For example when access is checked for an entity edit form controller we will execute code similar to the following:

if ($entity->access('update', $user)) {
  return AccessResult::allowed();
}

The call to $entity->access() will invoke the correct AccessControlHandler through the entity type manager, and inside the access control handler (or in access hooks that are fired by the handler) there will be code that checks the user's permissions that are relevant for the update entity operation. For example:

  $type = $node->bundle();
  switch ($operation) {
    case 'update':
      $access = AccessResult::allowedIfHasPermission($account, 'edit any ' . $type . ' content');
      break;
  }

This demonstrates how entity operations and permissions are related. Note that they are still distinct concepts: the operation update is related to edit any article content but at no point will we check if the user has an update permission, because this permission doesn't exist. Also we will not check for an edit any article content entity operation.

How about OgAccess?

According to the current documentation and method signature OgAccess::userAccess() is designed to determine access to entity operations. But it also accepts permissions instead of operations and handles both in the same way.

Example call to check access on an entity operation:

  public function userAccessEntity($operation, EntityInterface $entity, AccountInterface $user = NULL): AccessResultInterface {
    // ...
    $user_access = $this->userAccess($entity, $operation, $user);

Example call to check access on a user permission (from GroupSubscribeForm):

  public function isStateActive() {
    // ...
    $skip_approval = $this->ogAccess->userAccess($group, 'subscribe without approval', $user)->isAllowed();

How did the mixup happen?

The code is based on the D7 function og_user_access() which does not suffer from this problem. og_user_access() is only for checking user permissions, and this is clearly documented. There is nothing in the code that deals with entity operations in any way. When this function was ported to D8, it was done as a proof of concept in scope of checking entity access (inside og_entity_access()) and the mixup was present in the initial commit of this class (ref commit d24cd80).

What is interesting is that in this initial work, the only problem is in the argument naming and documentation: the method takes an argument $operation (instead of $permission) and it is documented as The entity operation being checked for.. The code itself is still 100% fine at this point: it is dealing only with permissions.

But as the port progressed, we started to call this method to check entity operations (after all, it says that this is its purpose!) and gradually the code was morphed into some kind of Frankenmethod that delivers "good enough" results to pass tests for both entity operations and permissions!

How to solve it

I propose to revert to the original intention as it was designed in D7: we should use this method exclusively for permissions. Let's fix the documentation and variable naming, and strip out the workarounds to make this work with entity operations. All code calling this to check for entity operations should be moved to entity hooks, and call in here with a relevant permission such as "edit group".