Provide a generic entity access handler and permissions

Our main remaining task is figuring out the interaction between "administer $entity_type", "access $entity_type_overview", "bypass $entity_type access'. The current code only generates the "bypass" one.

The way I understand this, the permissions work like this:
- administer: you can do anything
- access overview: you can view the admin table, but you might not have access to any operations
- bypass: you can do anything

If my understand is correct, then we can generate and use the "administer" and "access overview" ones, leaving out bypass.

Other points:
I generally prefer seeing $entity_type to $definition as a variable name, less generic. We also need to use the plural label where it makes sense (probably everywhere?)

Nitpicks:

class EntityApiPermissions

We can remove Api from class names, we don't do that in general.

+  protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
+    /** @var \Drupal\profile\Entity\ProfileInterface $entity */
+    $account = $this->prepareUser($account);

Wrong interface here.

+  /**
+   * Get the entity type ID represented in this permission handler.
+   *
+   * @return string
+   *   The entity type ID.
+   */
+  abstract protected function getEntityTypeId();
+
+  /**
+   * Returns an array of profile type permissions.
+   *
+   * @return array
+   *    Returns an array of permissions.
+   */
+  public function getPermissions() {

Gets the entity type ID. Gets the permissions.

+    $definition = $this->entityTypeManager->getDefinition($entity_type_id);
+    $has_owner = (new \ReflectionClass($definition->getClass()))->implementsInterface(EntityOwnerInterface::class);

you can write this as:

$entity_type = $this->entityTypeManager->getDefinition($entity_type_id);
$has_owner = $entity_type->isSubclassOf(EntityOwnerInterface::class);

Extra newline at the end of this test method:

+    // User with bypass can edit both entities.
+    $this->assertTrue($entity->access('edit', $user6));
+    $this->assertTrue($other_entity->access('edit', $user6));
+
+  }

Sharing a historical note: I did a PoC previously for dawehner: https://gist.github.com/bojanz/1b69c9809477312a5efb that used a generic EntityPermissions class defined by Entity, then required Entity Types to use our extended access control handler.
This allowed people to participate in the generic permission behavior just by defining the extended handler.

No longer convinced this is needed, though Matt's solution does require overriding the permissions class for every entity type (to specify the entity type id).

Retitling for clarity.

patch did not apply for me

Looks better.

+class EntityAccessHandler extends EntityAccessControlHandler {

Sounds like we should figure out a better name for this handler.
Perhaps ExtendedEntityAccessControlHandler like in my gist. Or GenericEntityAccessControlHandler?

+              'title' => $this->t('@bundle: @name @who @type', [

I'm afraid this approach makes t() completely useless. We might need to maintain a map of expected strings, ran through t().

+                '@type' => $definition->getLabel(),

We're still not using the plural labels (and $definition still isn't $entity_type).

+  /**
+   * Constructs a new EntityApiPermissions object.
+   *

Incorrect class name.

+    $has_owner = $definition->isSubclassOf(EntityOwnerInterface::class);
+    $has_owner = (new \ReflectionClass($definition->getClass()))->implementsInterface(EntityOwnerInterface::class);

These two lines are not in sync, let's fix the second one.

Let's try this. RTBC from my side if green.

1. +++ b/src/EntityAccessControlHandler.php
   @@ -0,0 +1,103 @@
   +      if ($entity instanceof EntityOwnerInterface) {
   +        $result = $this->checkEntityOwnerPermissions($entity, $operation, $account);
   +      }
   

   same as below, I think it would be nice to have a flag to decide if you want to have own/any permissions or not. You might not need that despite having an owner of your entity type.

2. +++ b/src/EntityAccessControlHandler.php
   @@ -0,0 +1,103 @@
   +  protected function checkEntityPermissions(EntityInterface $entity, $operation, AccountInterface $account) {
   +    $permission = "$operation {$entity->bundle()} {$entity->getEntityTypeId()}";
   +    return AccessResult::allowedIfHasPermission($account, $permission);
   +  }
   +
   

   could be done in a follow-up, but content_translation defines permission_granularity entity type property to define whether permissions are per-bundle or not.

3. +++ b/src/EntityAccessControlHandler.php
   @@ -0,0 +1,103 @@
   +  protected function checkEntityOwnerPermissions(EntityInterface $entity, $operation, AccountInterface $account) {
   

   we should add the user cache context here, as this means that it can be different for users, even if they have the same permissions. (another reason to allow not having those per-user permissions.)

4. +++ b/src/EntityAccessControlHandler.php
   @@ -0,0 +1,103 @@
   +      $result = AccessResult::allowedIfHasPermission($account, $own_permission);
   +      if ($result->isNeutral()) {
   +        // Check if user has "any" permission, still.
   +        // Users may provide only "any" permission not just "own".
   +        $result = AccessResult::allowedIfHasPermission($account, $any_permission);
   +      }
   

   AccessResult::allowedIfPermissions($account, [$own_permission, $any_permission, 'OR'] basically does the same. And I see you use that below, so why not here?

   I also don't really get the comment :)

5. +++ b/src/EntityPermissions.php
   @@ -0,0 +1,292 @@
   + * Usage:
   + * Extend the class and implement the getEntityTypeId() method.
   + * Then add the class to your module's permissions.yml file:
   + * @code
   + * permission_callbacks:
   + *   - \Drupal\mymodule\MyEntityPermissions::getPermissions
   + * @endcode
   

   Hm, yeah, that is a bit annoying :)

   the callback syntax also supports services. If you define it as a service, you can pass in a constructor argument to set the entity type?

   Or loop over entity types and look for them using your access controller? Would be tricky to sublcass then, though, in case you want to customize those permissions.

   not sure what's the best option.

6. +++ b/src/EntityPermissions.php
   @@ -0,0 +1,292 @@
   +    $permissions["administer $entity_type_id"] = [
   +      'title' => $this->t('Administer @type', ['@type' => $entity_type->getPluralLabel()]),
   +      'restrict access' => TRUE,
   +    ];
   

   I hate administer nodes :)

   At this point, it doesn't do much more than control access to status/promote/owner/sticky. I don't see why it needs to be restrict access, that's IMHO a leftover from before bypass and access oveview were split off.

   Why not define the bypass permission in the same way as node does and leave administer xy out for now?

7. +++ b/src/EntityPermissions.php
   @@ -0,0 +1,292 @@
   +    $permissions["access $entity_type_id overview"] = [
   +      'title' => $this->t('Access @type overview', ['@type' => $entity_type->getPluralLabel()]),
   +      'restrict access' => TRUE,
   +    ];
   

   access content overview isn't a restricted permission, why is this?

8. +++ b/src/EntityPermissions.php
   @@ -0,0 +1,292 @@
   +    if ($entity_type->getPermissionGranularity() == 'entity_type') {
   +      $permissions += $this->getEntityTypePermissions();
   +    }
   

   oh, you actually do check this here. doesn't seeem to be considered when actually checking the permissions though?

Thought I'd weigh in and explain the bypass permission and why we need it:

• administer: You can do anything to the entities you can access
• access overview: You can view the admin table, but you might not have access to any operations
• bypass: You can bypass access systems such as grants

The latter has an important distinction and was added to the node module for a reason. It allows people to edit any node's content, without being able to change its published status or author. Vice versa does the administer permission allow someone to edit a node's metadata, as long as they actually have access to that node.

Given the work I intend to do in #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter() (if I ever get to it), I'd be careful with removing the bypass permission. We may end up needing it in the grant system.

Right, but bypass is IMHO a better name for "allow everything". Which doesn't actually seem to be implemented yet? As mentioned, node doesn't check administer nodes anywhere for entity-level permissions, just on field level and there it is too generic (you can allow users to see own unpublished content but you can not give them permission to publish and unpublish content without the restricted administer nodes permission).

With the service, I mean that you would add this to services.yml:

mymodule.entity_permissions:
class: \Drupal\entity\EntityPermissions
arguments: ['my_entity_type', '@foo', '@bar']

And then specific it as mymodule.entity_permissions:getPermissions (single colon = service, double colon = class name)

If that's actually easier, I'm not sure. Just mentioning it as a possibility.

About generating permissions, my idea would basically be to support 3 flags on the entity type annotation:

permissions_generate (if not set, default value depends on implementing this access control handler or not. )
permissions_granularity (defined by core, already implemented in the patch for generating permissions but not for checking them)
permissions_owner (if set and entity implements the interface, generate and check owner permissions. not sure about default value)

Then, entity.module provides a permission callback that generates permissions for all entity types that explicitly or implicitly have permissions_generate set to TRUE. that class also has a generatePermissionForEntityType(EntityTypeInterface $entity_type). If you want to customize the permissions, you implement your own class/permission callback, extend from the default, call that method and customize the returned permissions. Or do it completely yourself.

Crosspost, my comment was not a response to #27.

> administer: You can do anything to the entities you can access

If you mean by that to be able to change fields like owner, status, ... Yes, that is correct. But this patch doesn't do anything with field level permissions, it doesn't use that permission.. so why generate it? IMHO, I would leave that for another issue. You have to specify it manually but then you can also provide a useful description about what it actually allows to do, as I imagine this could vary a lot between entity types (you don't want to allow users to change the owner of a profile entity, if that would even be possible, but for another entity type, that migh be a perfectly valid and non-restricted use case)

Edit: I think we actually agree for the most part. You're suggesting also to keep bypass, not sure if you're for or against generating an administer permission here :)

I was clarifying bojanz's understanding of the permissions in #9.

I don't think a content entity access handler needs to worry about any administer permission, only a bypass permission. However, there is currently a discrepancy in core regarding the permission names:

• Content entities (nodes) check for the bypass permission
• Config entities (node types) check for the entity type's admin_permission key, which is administer content types

So nodes use bypass for the same purpose node types use administer. Yet on the node level, there is a clear distinction between bypass and administer. Looks like a case of confusing naming patterns :)

Yeah, node is the only entity type with a bypass permission in core, the others just have administer. But node is also the only one that same level flexibility as we're proposing here, so I think we should use that as example and not others :)

I think it's a mistake to use Node module as an absolute example here.
You've said it yourself, Node has permission patterns that are not followed by the rest of core. Or contrib. In neither D7 nor D8.

The only permission that the entity api in D8 forces you to have is the administer permission. Since the early D7 days (outside of Node) it has always meant "you can do anything". Meanwhile, "bypass" is a node specific permission that 10 Drupal developers couldn't define together the same way. I think that terminology is worse and goes against what we're used to.

Furthermore, I am not convinced non-node entity types have a need for the distinction between "I can do anything" and "I can do anything except publish the entity". Especially since we don't even take the "status" key in account when generating permissions, since we can't know the labels (published/unpublished, enabled/disabled, active/inactive). That's something that should be done in the classes that extend these. These classes should provide sane defaults for content and config entity types alike.

So I suggest reverting back to not defining the bypass permission, using the administer one as the "god mode" one.

@berdir
The service approach doesn't seem to be an improvement over just extending the class. It's a level of indirection that doesn't hide the awkwardness of the current approach completely.

Do you find this preferable: https://gist.github.com/bojanz/1b69c9809477312a5efb
It makes the access control responsible for generating the permissions. Is that a pattern we want to introduce?
We'd need to figure out the right name for the interface (does ExtendedEntityAccessControlHandlerInterface make sense)?
I am also not 100% certain that the permissions are generated early enough.

@bojanz: Just to be clear: You're advocating for the administer permission to be the 'god mode' switch for being able to do anything to an entity, including getting full access? Because if we go down that road, creating a generic entity grants system could turn out to be quite tricky.

I kind of like the distinction between an all-access pass and a do-what-you-want-within-your-sandbox pass.

Edit: Even though the current naming pattern and lack of documentation is confusing the hell out of everyone :D

@bojanz: Just to be clear: You're advocating for the administer permission to be the 'god mode' switch for being able to do anything to an entity, including getting full access?

Correct. It's already that for config entities. And it was already that in D7 everywhere, including Commerce 1.x which had query level access filtering. Entity types will be free to provide a less-god permission such as "bypass query access", which would then be clear about what it affects.

My point is that if we want to standardize something, we must do it by looking at what's common, not base it on an outlier.

All right, I can live with that :)

Guess we'll have to create an exception for 'bypass node access' in the grant system until D9. I can imagine people having written code that checks for that permission. Others could use a more descriptive "bypass ENTITY_TYPE query access" permission as you suggested.

+++ b/src/EntityAccessControlHandler.php
@@ -28,7 +33,7 @@ class EntityAccessControlHandler extends CoreEntityAccessControlHandler {
       }
     }
-    $result->cachePerPermissions()->addCacheableDependency($entity);
+    $result->cachePerUser()->cachePerPermissions()->addCacheableDependency($entity);

This is the main reason for offering to not have owner permissions.

Per-user caching is very problematic and should be avoided if possible. Some things aren't cached anymore at all then and if they are, you're creating possibly hundreds of variations.

Only add cachePerUser() if there are owner permissions, not always.

I can live with only having administer, fine. I think bypass is actually the easy part and administer is weird, at least for nodes. The entity API doesn't require you to have an administer permission anywhere. it just has a feature to have a simple one-permission-for-everything, how you name it is up to you.

I plan to open issues to replace that with more specific permissions and there are multiple bugs around administer nodes usage, for example #2779389: Node bulk delete confirmation from requires 'administer nodes' permission where it is simply incorrectly used. IMHO, administer xy is useful for a simple scenario where you have that to do everything and maybe 1-2 more, like view xy. But again, I can live with doing this ;)

Moving the generation of the permissions to the access control handler sounds nice to me, +1 :)

#2799209: Incorrect permission check in views node access filter is another one of those bugs, which I reported privately initially but was made public now.

Here we go again.

1) After speaking with dawehner, decided to introduce a permission_provider entity handler, mirroring the route provider. Cleanest option so far (VS putting it on the access control handler, or forcing people to extend EntityPermissions).
2) Fixed berdir's cachePerUser() remarks.
3) Removed the bypass permission as agreed on.
4) Fixed the bundle permissions for entity types that don't use config entity type bundles.

Implemented the patch in Commerce, caught bugs in the permission names.
Also switched from plural labels to singular labels for the "any" permissions ("Create any product" instead of "Create any products").

We need to expand our test coverage to cover both entity type & bundle granularity, with and without owners. Currently not all combinations are there.

Permissions weren't sorted properly on the page because TranslatableMarkup objects don't sort, they need to be cast to string. Crazy.

Noticed that for bundle granularity we need a generic view permission, for various views. Perhaps it makes sense not to generate per-bundle view permissions for now?

I talked with @dawehner and he told he was about to commit this and asked me to start an issue for porting the feature into core. A start on that is here: #2809177: Introduce entity permission providers

BTW documentation on the workings of this would really be nice.

Perhaps it makes sense not to generate per-bundle view permissions for now?

I think this really mostly makes sense for nodes displayed on the page, for which we have the node access system already, with all of its glory and shame.

Rerolling.

Here we go.

1) Rewrote the tests. Both the access control handler and the permission provider now have unit tests with (what should be) full coverage. Looking at the EntityPermissionProviderTest also provides a good overview of what permissions are generated.

2) Removed the bundle specific view permissions, as agreed on.

3) Unified the (accidental) "create own" / "create any" permissions into a single "create" permission.

4) Fixed a bug in the create permissions for bundle-less entity types.

5) Stopped generating the "access $entity_type overview" permission for config entity types. A config entity type now gets the following permissions: administer, view, create, update, delete. Is that too much? Do we want to limit it to the administer permission only?

Re 5: What's the use of being able to see node types if you can't manipulate them? :) I'd say for config entities, 'administer' would suffice.

How about adding "view unpublished" permission, as worked on at #273595: Move permission "view any unpublished content" from Content Moderation to Node? Could it be added through here?

5) Stopped generating the "access $entity_type overview" permission for config entity types. A config entity type now gets the following permissions: administer, view, create, update, delete. Is that too much? Do we want to limit it to the administer permission only?

For me the default usecase for config entities are indeed really just "administer" and call it a day. I guess we should let people choose by using one handler or another.

Isn't that what we already do? If a single permission is all you need then core already provides that with the existing admin permission and the default access control handler. And generating a single permission doesn't seem worth it to me, that's more overhead than just defining it?

How about adding "view unpublished" permission, as worked on at #273595: Move permission "view any unpublished content" from content moderation to core? Could it be added through here?

Problem is, there is no guarantee that a status field will use "unpublished" / "published" as the value labels. No API provides those labels, and it's an assumption Core didn't make previously. That might be changing with the work going on for 8.3.x, but I definitely need more feedback here before introducing it.

Okay, we decided to stop caring about config entity types completely. Removed the config entity from the test and the condition from the permission provider. Did my best with the docs, feel free to provide suggetsions.

Thank you a lot @mglaman and @bojanz!

Let's see us in the core issue queue.

Note that the commit landed in a phantom 8.x-2.x first, which we decided to remove in favor of the existing 8.x-1.x branch.