Introduce entity permission providers

Issue summary needs a lot more detail

This issue should also credit mglaman since he wrote the Entity API code with me.

Here's an initial patch. One permission provider, one access handler, matching unit tests.

Questions:
1) EntityType has getRouteProviders(), do we want a matching getPermissionProviders()? Don't see much of a benefit.
2) Do we want to call the permission providers in Drupal\user\PermissionHandler? If yes, we'll need to expand its doc block. If no, we'll need to find a new home for this code. Perhaps a permission_callback + class in system.permissions.yml? Not that great.
3) How do we feel about GenericEntityAccessControlHandler? I assume we don't want to modify the parent class to keep this functionality opt-in. Bikesheds on the name?

Note for the next reroll:

+ * @code
+ *  handlers = {
+ *    "access" = "Drupal\Core\Entity\EntityAccessControlHandler",
+ *    "permission_provider" = "Drupal\Core\Entity\EntityPermissionProvider",
+ *  }
+ * @endcode
+ *
+ * @see \Drupal\entity\EntityAccessControlHandler

Both of these EntityAccessControlHandler references should be \Drupal\Core\Entity\GenericEntityAccessControlHandler.

+++ b/core/lib/Drupal/Core/Entity/EntityPermissionProvider.php
@@ -0,0 +1,244 @@
+    if ($has_owner) {
+      $permissions["view any {$entity_type_id}"] = [
+        'title' => $this->t('View any @type', [
+          '@type' => $singular_label,
+        ]),
+      ];
+      $permissions["view own {$entity_type_id}"] = [
+        'title' => $this->t('View own @type', [
+          '@type' => $plural_label,
+        ]),
+      ];
+    }
+    else {
+      $permissions["view {$entity_type_id}"] = [
+        'title' => $this->t('View @type', [
+          '@type' => $plural_label,
+        ]),
+      ];
+    }

We briefly discussed this at media sprint in Berlin and mostly agreed that it would make sense to also support per-bundle view permissions.

We briefly discussed this at media sprint in Berlin and mostly agreed that it would make sense to also support per-bundle view permissions.

We can't do that until we implement query access, otherwise there would be no way to enforce it.
That makes it followup material.

Here is a patch for #7 + the test bot explosions.

1. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProvider.php
   @@ -0,0 +1,244 @@
   + * @code
   + *  handlers = {
   + *    "access" = "Drupal\Core\Entity\GenericEntityAccessControlHandler",
   + *    "permission_provider" = "Drupal\Core\Entity\EntityPermissionProvider",
   + *  }
   

   +1 for adding documentation how to use it

2. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProvider.php
   @@ -0,0 +1,244 @@
   +    if ($entity_type->getPermissionGranularity() == 'entity_type') {
   

   Nitpick: I always use tripple equal for strings these days.

3. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProvider.php
   @@ -0,0 +1,244 @@
   +    else {
   +      $permissions += $this->buildBundlePermissions($entity_type);
   +    }
   +
   

   Note: I would use an elseif here as well, and otherwise throw an exception or so. Do you think this makes sense?

4. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProvider.php
   @@ -0,0 +1,244 @@
   +      ];
   +      // TranslatableMarkup objects don't sort properly.
   +      $permissions[$name]['title'] = (string) $permission['title'];
   +    }
   

   I don't see something like that in \Drupal\node\NodePermissions. Is this really needed?

5. +++ b/core/lib/Drupal/Core/Entity/GenericEntityAccessControlHandler.php
   @@ -0,0 +1,112 @@
   +    if ($result->isNeutral()) {
   +      if ($entity instanceof EntityOwnerInterface) {
   +        $result = $this->checkEntityOwnerPermissions($entity, $operation, $account);
   +      }
   +      else {
   +        $result = $this->checkEntityPermissions($entity, $operation, $account);
   +      }
   +    }
   +
   

   One thing I was wondering, ... should we have two classes instead of this if/else?

6. +++ b/core/modules/user/src/PermissionHandler.php
   @@ -187,6 +199,30 @@ protected function buildPermissionsYaml() {
   +   * Builds all permissions provided by entity permission providers.
   +   *
   +   * @return array[]
   +   *   The permissions.
   +   *   Each permission is an array with the following keys:
   +   *   - title: The title of the permission.
   +   *   - description: The description of the permission, defaults to NULL.
   +   *   - provider: The provider of the permission.
   +   */
   +  protected function buildEntityTypePermissions() {
   +    $permissions = [];
   +    /** @var \Drupal\Core\Entity\EntityTypeInterface[] $entity_types */
   +    foreach ($this->entityTypeManager->getDefinitions() as $entity_type) {
   +      if ($entity_type->hasHandlerClass('permission_provider')) {
   +        $permission_provider_class = $entity_type->getHandlerClass('permission_provider');
   +        $permission_provider = $this->entityTypeManager->createHandlerInstance($permission_provider_class, $entity_type);
   +        $permissions += $permission_provider->buildPermissions($entity_type);
   +      }
   +    }
   +
   +    return $permissions;
   +  }
   +
   +  /**
   

   Mh, I would have expected system module to implement this hook and integrate it?

1) EntityType has getRouteProviders(), do we want a matching getPermissionProviders()? Don't see much of a benefit.

me neither to be honest

2) Do we want to call the permission providers in Drupal\user\PermissionHandler? If yes, we'll need to expand its doc block. If no, we'll need to find a new home for this code. Perhaps a permission_callback + class in system.permissions.yml? Not that great.

In an ideal world, \Drupal\Component\Discovery\YamlDiscovery would somehow find a core.permissions.yml file as well :( When we put it into a yml file, moving it into a different one, is no BC break at all.

3) How do we feel about GenericEntityAccessControlHandler? I assume we don't want to modify the parent class to keep this functionality opt-in. Bikesheds on the name?

That's a tricky one. I also commented/wondered about the class for itself.

Re: #12
2. Done
3. Makes sense to me, done.
4. I'm not sure, left it in there.
5. While I was having a look at this I noticed that the only method from GenericEntityAccessControlHandler that is not an exact copy of EntityAccessControlHandler's methods is GenericEntityAccessControlHandler::checkCreateAccess so perhaps there's no need to define the rest here?

Ran out of time as I've got to feed the kids, but anyway here's a tiny bit of progress =)

+++ b/core/lib/Drupal/Core/Entity/EntityPermissionProvider.php
@@ -0,0 +1,248 @@
+        $permissions["update any {$bundle_name} {$entity_type_id}"] = [
+          'title' => $this->t('@bundle: Update any @type', [
...
+        $permissions["delete any {$bundle_name} {$entity_type_id}"] = [
+          'title' => $this->t('@bundle: Delete any @type', [
...
+        $permissions["update {$bundle_name} {$entity_type_id}"] = [
+          'title' => $this->t('@bundle: Update @type', [
...
+        $permissions["delete {$bundle_name} {$entity_type_id}"] = [
+          'title' => $this->t('@bundle: Delete @type', [

Why they are separate names? I think better to use "any" in both cases (means they should be added unconditionally)

On other hand when owner should just add only "personal/own" update/delete

A few random responses.
No opinion yet on #17. I think that the permission names are separate because implementing code (D7, D8 contrib) always did it that way, so it was carried over.

I don't see something like that in \Drupal\node\NodePermissions. Is this really needed?

Yes, otherwise the labels don't sort as expected.

One thing I was wondering, ... should we have two classes instead of this if/else?

Feels simpler to have a single class, 1 access control handler relying on 1 permission provider. If 1 permission provider becomes 2, keeping them in sync becomes a task. But I'll accept being outvoted if others feel strongly about this.

Patch in #15 still applies. Fixing test failures with some cleanup on test file.

We need to reroll the patch from Entity API, beta1 has major improvements (split into cacheable and non-cacheable providers)

Here is a new patch. I applied https://github.com/fago/entity/commit/9e60a18f49290032cbfbae315259a68cb2... on top of #2809177: Introduce entity permission providers

Fixed the last two failing tests...

Thank you @chr.fritsch for the patches!

1. We should clearly document what the patch is doing now and why in the issue summary (cacheable vs. non cacheable permissions)
2. I'm quite sure we should have a subsystem maintainer review for this addition.
3. Is this issue actually as task or a feature request?

I really like this patch a lot. I think it will smooth out permission handling quite a bit -- it's a nice win, and a nice looking patch. I didn't get all the way through it, but here is what I found:

1. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProvider.php
   @@ -0,0 +1,54 @@
   + * Provides generic entity permissions which are still cacheable.
   

   Should be rephrased a bit: "Provides generic, cacheable entity permissions."

2. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProvider.php
   @@ -0,0 +1,54 @@
   + * This includes:
   

   "These include"

3. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProvider.php
   @@ -0,0 +1,54 @@
   + * results in caching per user. If you need this use case, please use
   + * \Drupal\entity\UncacheableEntityPermissionProvider instead.
   

   UncacheableEntityPermissionProvider is in the \Drupal\Core\Entity namespace.

4. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProviderBase.php
   @@ -0,0 +1,231 @@
   +/**
   + * @internal
   + */
   +class EntityPermissionProviderBase implements EntityPermissionProviderInterface, EntityHandlerInterface {
   

   Class is missing a description. Also, is there any reason for this class to not be abstract?

5. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProviderBase.php
   @@ -0,0 +1,231 @@
   +    $permissions["access {$entity_type_id} overview"] = [
   +      'title' => $this->t('Access the @type overview page', ['@type' => $plural_label]),
   +    ];
   

   This assumes there is an overview page. Maybe this permission should only be generated if there is a 'collection' route for the entity type?

6. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProviderBase.php
   @@ -0,0 +1,231 @@
   +    $has_owner = $entity_type->entityClassImplements(EntityOwnerInterface::class);
   

   This check is repeated a couple of times. Maybe it should be a helper method?

7. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProviderInterface.php
   @@ -0,0 +1,25 @@
   +   * @return array[]
   +   *   The permissions.
   +   *   Each permission is an array with the following keys:
   +   *   - title: The title of the permission.
   +   *   - description: The description of the permission.
   +   *   - provider: The provider of the permission.
   

   Will #2924785: Provide a mechanism to deprecate permissions affect this? If so, how?

8. +++ b/core/lib/Drupal/Core/Entity/GenericEntityAccessControlHandler.php
   @@ -0,0 +1,147 @@
   +class GenericEntityAccessControlHandler extends EntityAccessControlHandler {
   

   This name is misleading, because the constructor makes it quite clear that the entity type must have a permission provider. Maybe we should rename this to something like PermissionsBasedEntityAccessControlHandler or similar?

9. +++ b/core/lib/Drupal/Core/Entity/GenericEntityAccessControlHandler.php
   @@ -0,0 +1,147 @@
   +      $permissions = [
   +        "$operation {$entity->getEntityTypeId()}",
   +        "$operation {$entity->bundle()} {$entity->getEntityTypeId()}",
   +      ];
   

   Considering that $operation can be anything (filter formats, for example, define a 'use' operation), this seems potentially brittle. Not sure what, if anything, we should do about that, though...just thought I'd bring it up.

10. +++ b/core/lib/Drupal/Core/Entity/GenericEntityAccessControlHandler.php
    @@ -0,0 +1,147 @@
    +        if (($account->id() == $entity->getOwnerId())) {
    

    Nit: There's an extra set of parentheses here.

11. +++ b/core/lib/Drupal/Core/Entity/GenericEntityAccessControlHandler.php
    @@ -0,0 +1,147 @@
    +      if (($account->id() == $entity->getOwnerId())) {
    

    Same here.

12. +++ b/core/lib/Drupal/Core/Entity/GenericEntityAccessControlHandler.php
    @@ -0,0 +1,147 @@
    +        $result = AccessResult::allowedIfHasPermissions($account, [
    +          "$operation own {$entity->getEntityTypeId()}",
    +          "$operation any {$entity->getEntityTypeId()}",
    +          "$operation own {$entity->bundle()} {$entity->getEntityTypeId()}",
    +          "$operation any {$entity->bundle()} {$entity->getEntityTypeId()}",
    +        ], 'OR');
    +      }
    +      else {
    +        $result = AccessResult::allowedIfHasPermissions($account, [
    +          "$operation any {$entity->getEntityTypeId()}",
    +          "$operation any {$entity->bundle()} {$entity->getEntityTypeId()}",
    +        ], 'OR');
    

    I feel like we could shorten this a bit by using $result->orIf().

I have fixed below points mentioned in the comment #29
Fixed : 1, 2, 3 , 8 and 11 numbered points.

Here is the interdiff and patch.

Thanks for the review!

Regarding:

+    $has_owner = $entity_type->entityClassImplements(EntityOwnerInterface::class);
This check is repeated a couple of times. Maybe it should be a helper method?

I generally dislike adding helpers that replace a single line check, it just adds a mental jump. I don't mind being overruled though.

I'll start working on the failed test, and coding standard messages

+++ b/core/lib/Drupal/Core/Entity/GenericEntityAccessControlHandler.php
@@ -11,7 +11,7 @@
-class GenericEntityAccessControlHandler extends EntityAccessControlHandler {
+class GenericEntityAccessControlHandler extends PermissionsBasedEntityAccessControlHandler {

Found the failing code: The new class should be called PermissionBasedAccessControlHandler it does not extend this class.
Note: as non-native-engllish-speaker I feel the the correct Name should be PermissionBasedAccessControlHandler not PermissionsBasedAccessControlHandler. I will name the class like this. Ofcourse a native speaker will convince me to keep the suggested name (including the 's')

Refactored handler's an Test's names to PermissionBasedEntityAccessControlHandler
enforced coding standards, as annotated during last test.

Continue here:

+++ b/core/lib/Drupal/Core/Entity/EntityPermissionProviderBase.php
@@ -0,0 +1,231 @@
+ $permissions["access {$entity_type_id} overview"] = [
+ 'title' => $this->t('Access the @type overview page', ['@type' => $plural_label]),
+ ];
This assumes there is an overview page. Maybe this permission should only be generated if there is a 'collection' route for the entity type?

#29 5. - removing the assumption, that the entity_type has an collection route is finished, but I have to leave the Sprint, I'll provide the patch tomorrow.

Here comes the promised patch, I had to adjust a few things:

In EntityPermissionProviderBase::buildPermission I checked the existance of the collection-linkTemplate.

    if ( $entity_type->hasLinkTemplate('collection')) {
      $permissions["access {$entity_type_id} overview"] = [
        'title' => $this->t('Access the @type overview page', ['@type' => $plural_label]),
      ];
    }

This pushed me straight to the tests, as the prophesized entity_types did not provide the method 'hasLinkTemplate', so they failed.
When comparing both test-classes 'EntityPermissionProviderTest' and 'UncacheableEntityPermissionProviderTest' I found them nearly identitcal, because of this I made 'UncacheableEntityPermissionProviderTest' a derived class of 'EntityPermissionProviderTest'. So I could eliminate the identical methods and variables in 'UncacheableEntityPermissionProviderTest'.
Further more I could introduce the method 'EntityPermissionProviderTest::prophesizeEntityType' that prophesizes an entity_type that is defined by function's parameter, making the entityTypeProvider more clearly.

The already existing test-entity_types claim to have an overview-page, the new orange-entity_type doesn't.

Unfortunately (viewed from the test side) the expectations of both entityTypeProviders are not identical - so they are both needed.

Thoughts:

The provider baseclass 'EntityPermissionProviderBase' is marked as @internal, (to keep other developers from using it yet?) shouldn't the derived classes @internal as well?

I feel the tests need a base-class - but I didn't want to change it in my first larger patch without consultations. If it's likely that more EntityPermissionProviders will be introduced I think it's necessary, if the two existing will remain the only providers it's a nice-to-have.

Failed to apply ? :_( - I'll check this

Sorry - Second try - interdiff of #40 is still valid

I think we could do with a change record for this.

We need to roll-in the fix from this Entity API issue: #2951270: Core's generated collection routes do not support the provided "access overview" permission.
Should be simpler here, we just modify the core's DefaultHtmlRouteProvider::getCollectionRoute with something like:

    if ($entity_type->hasHandlerClass('permission_provider')) {
      $admin_permission = $entity_type->getAdminPermission();
      $overview_permission = "access {$entity_type->id()} overview";
      $route->setRequirement('_permission', "$admin_permission+$overview_permission");
    }

Re #46: I opened #2953566: Allow entities to specify a "collection permission", FYI

+++ b/core/lib/Drupal/Core/Entity/EntityPermissionProvider.php
@@ -0,0 +1,54 @@
+ * - administer $entity_type
...
+ * Intended for content entity types, since config entity types usually rely
+ * on a single "administer" permission.

+++ b/core/lib/Drupal/Core/Entity/EntityPermissionProviderBase.php
@@ -0,0 +1,233 @@
+    $permissions["administer {$entity_type_id}"] = [

I think it would make sense to use the admin permission here instead of hardcoding the permission machine name that is declared by the entity type and also update the documentation accordingly.

Just adding this because the _somewhat_ recent work around entity access policies in #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter() and broken out into https://www.drupal.org/project/entity_access_policies
seems related.

Postponed #273595: Move permission "view any unpublished content" from Content Moderation to Node on this.

I've updated that patch and rolled-in the fix from this Entity API issue: #2951270: Core's generated collection routes do not support the provided "access overview" permission, as suggested in comment #46.

I took a look at the suggestion in #48, but I wasn't exactly sure how to "use the admin permission". I can get the admin permission machine name with entity_type->getAdminPermission(), but if that permission is already defined elsewhere, it doesn't need to be built... I think I must be missing something here. My brief efforts to make the proposed change resulted in a bunch of broken tests. So I figured I should get some clarification first before proceeding.

Re #51, thanks for the update!

You were right on track, using $entity_type->getAdminPermission() was exactly what I had in mind. The idea would be that with this patch modules would no longer have to define the admin permission explicitly themselves, as we would do it for them. It might result in a few broken tests, but we should try to fix the tests in that case. Thanks for your effort!

I am planning to come back to this patch soon.

I've made a number of improvements upstream, in preparation for a reroll of the core patch.

Bug fixes:
#2977224: EntityAccessControlHandler::checkEntityOwnerPermissions() doesn't cache update/delete results per user
#2977379: UncacheableEntityAccessControlHandler::checkEntityOwnerPermissions() incorrectly checks access for unpublished entities
New feature (resolving #9 from this issue):
#2977231: EntityPermissionProvider should provide per-bundle view permissions

I've also cleaned up both the tests and the docblocks.
Currently exploring an access handler logic deduplication in #2977381: Reduce duplication between EntityAccessControlHandler and UncacheableEntityAccessControlHandler.

Of course, if anyone is interested in rerolling the patch even partially (with a portion of the fixes from upstream), please go ahead.

I've rerolled the patch with the fixes from:

#2977224: EntityAccessControlHandler::checkEntityOwnerPermissions() doesn't cache update/delete results per user
#2977379: UncacheableEntityAccessControlHandler::checkEntityOwnerPermissions() incorrectly checks access for unpublished entities
#2977231: EntityPermissionProvider should provide per-bundle view permissions

I'm gathering issues that reduce the amount of boilerplate required for custom entity types at #2316171: [meta] Improve DX of entity defining (if you want a UI).

Thanks! - I think this is ready for a round of reviews.

Found and fixed another bug upstream: #2978943: UncacheableEntityPermissionProvider generates incorrect view permissions for entities without an owner.

While rerolling, let's also include #2977381: Reduce duplication between EntityAccessControlHandler and UncacheableEntityAccessControlHandler.

1. +++ b/core/lib/Drupal/Core/Entity/PermissionBasedEntityAccessControlHandler.php
   @@ -0,0 +1,142 @@
   +      if ($account->id() == $entity->getOwnerId()) {
   +        $result = AccessResult::allowedIfHasPermissions($account, [
   +          "$operation own {$entity->getEntityTypeId()}",
   +          "$operation any {$entity->getEntityTypeId()}",
   +          "$operation own {$entity->bundle()} {$entity->getEntityTypeId()}",
   +          "$operation any {$entity->bundle()} {$entity->getEntityTypeId()}",
   +        ], 'OR');
   +      }
   

   The $operation own should always be cached per user right?

2. +++ b/core/lib/Drupal/Core/Entity/PermissionBasedEntityAccessControlHandler.php
   @@ -0,0 +1,142 @@
   +  protected function checkCreateAccess(AccountInterface $account, array $context, $entity_bundle = NULL) {
   ...
   +      $permissions = [
   +        'administer ' . $this->entityTypeId,
   +        'create ' . $this->entityTypeId,
   +      ];
   +      if ($entity_bundle) {
   +        $permissions[] = 'create ' . $entity_bundle . ' ' . $this->entityTypeId;
   +      }
   +
   +      $result = AccessResult::allowedIfHasPermissions($account, $permissions, 'OR')->cachePerUser();
   

   And I also think the $operation any should not have to be cached per user?

3. +++ b/core/lib/Drupal/Core/Entity/UncacheableEntityAccessControlHandler.php
   @@ -0,0 +1,141 @@
   + * Note: this access control handler will cause pages to be cached per user.
   ...
   +class UncacheableEntityAccessControlHandler extends EntityAccessControlHandler {
   

   I must be missing something, but how does providing the permission view own ($bundle) $entity_type impact the caching of the update/delete operations?

4. +++ b/core/lib/Drupal/Core/Entity/UncacheableEntityAccessControlHandler.php
   @@ -0,0 +1,141 @@
   +        // There's no permission for viewing other user's unpublished entity.
   +        return AccessResult::neutral()->cachePerUser();
   

   This seems like a valid use case? Could we add this?

#59.1++ and #59.2++, absolutely!

#59.3: good question. I looked at the commit history of the entity module; this was introduced in https://github.com/fago/entity/pull/51. The fundamental reason seems to have been Be honest that the default access controller is not cacheable => @bojanz and myself agreed on renaming the existing one and provide a cacheable one by default. Provide a cacheable permission provider with just "view own unpublished $entity_type" but none of 'view any and view own $bundle ...'. @dawehner in #27.1 also said the reasoning behind this needs to be clearly documented in the issue summary.
I think the reasoning is that anything showing unpublished entities is uncacheable anyway: listing unpublished entities is not something you ever do for anonymous visitors. Hence the presence of view own unpublished {$entity_type_id} in the cacheable permissions provider. But using view own {$entity_type_id} applies also to published content, and listing published entities is of course something you would do for anonymous visitors; using view own {$entity_type_id} would "infect" that listing with poor cacheability, hence it's named UncacheableEntityPermissionsProvider. Even though it's technically not uncacheable, but per-user cacheable aka poorly cacheable, aka pretty much uncacheable in most real-world situations (unless you throw massive cache storage at the problem).

Initial quick review, deep dive coming:

1. +++ b/core/lib/Drupal/Core/Entity/UncacheableEntityAccessControlHandler.php
   @@ -0,0 +1,141 @@
   +      throw new \Exception("This entity access control handler requires the entity permissions provider: {EntityPermissionProvider::class}");
   

   s/{Entity/{UncacheableEntity/

2. +++ b/core/lib/Drupal/Core/Entity/UncacheableEntityAccessControlHandler.php
   @@ -0,0 +1,141 @@
   +    if ($result->isNeutral()) {
   +      if ($entity instanceof EntityOwnerInterface) {
   +        $result = $this->checkEntityOwnerPermissions($entity, $operation, $account);
   +      }
   +      else {
   +        $result = $this->checkEntityPermissions($entity, $operation, $account);
   +      }
   +    }
   

   Rather than overwriting, use ->orIf().

3. +++ b/core/lib/Drupal/Core/Entity/UncacheableEntityAccessControlHandler.php
   @@ -0,0 +1,141 @@
   +        // There's no permission for viewing other user's unpublished entity.
   +        return AccessResult::neutral()->cachePerUser();
   

   Let's remove the comment and instead pass it as the $reason argument — this will result in better DX for Drupal's HTTP APIs :)

This is really impressive work!

I only had time to review the non-test code. Please understand this is mostly bike shed/nitpicky review... so, feel free to ignore and counter whatever you'd like and tell me how wrong I am 😅.

1. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProvider.php
   @@ -0,0 +1,91 @@
   + * - update (own|any) ($bundle) $entity_type
   + * - delete (own|any) ($bundle) $entity_type
   ...
   + * \Drupal\Core\Entity\UncacheableEntityPermissionProvider instead.
   

   ubernit: just expand these into their own lines.

2. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProvider.php
   @@ -0,0 +1,91 @@
   + * This class does not support "view own ($bundle) $entity_type", because this
   + * results in caching per user. If you need this use case, please use
   

   I wonder if there's a way that this requirement can be met without defining an entirely separate class.

   Maybe we could find a way to put a protected static $hasViewOwnEntityPermission = FALSE on the base class.

3. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProvider.php
   @@ -0,0 +1,91 @@
   + * on a single "administer" permission.
   + * Example annotation:
   

   nit: add an extra newline

4. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProvider.php
   @@ -0,0 +1,91 @@
   + *    "access" = "Drupal\Core\Entity\PermissionBasedEntityAccessControlHandler",
   

   ❤️ this class name.

   Maybe we need an Entity\Access namespace though?

5. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProvider.php
   @@ -0,0 +1,91 @@
   + *    "permission_provider" = "Drupal\Core\Entity\EntityPermissionProvider",
   

   nit: s/permission/permissions/g

6. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProvider.php
   @@ -0,0 +1,91 @@
   +class EntityPermissionProvider extends EntityPermissionProviderBase {
   

   same.

7. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProvider.php
   @@ -0,0 +1,91 @@
   +   * Builds permissions for the entity_type granularity.
   

   "Builds permissions based solely on entity type."

8. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProvider.php
   @@ -0,0 +1,91 @@
   +        '@type' => $plural_label,
   

   strtolower($plural_label)?

9. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProvider.php
   @@ -0,0 +1,91 @@
   +   * Builds permissions for the bundle granularity.
   

   "Builds entity permissions based on their bundle."

10. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProviderBase.php
    @@ -0,0 +1,233 @@
    +      'title' => $this->t('Administer @type', ['@type' => $plural_label]),
    

    Again, I think this should be lower-cased.

11. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProviderBase.php
    @@ -0,0 +1,233 @@
    +        'title' => $this->t('Access the @type overview page', ['@type' => $plural_label]),
    

    In this case, shouldn't it be the singular label?

12. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProviderBase.php
    @@ -0,0 +1,233 @@
    +    if ($has_owner && $entity_type->entityClassImplements(EntityPublishedInterface::class)) {
    +      $permissions["view own unpublished {$entity_type_id}"] = [
    +        'title' => $this->t('View own unpublished @type', [
    +          '@type' => $plural_label,
    +        ]),
    +      ];
    +    }
    

    Doesn't this also cause "uncacheability"? Does it need to be moved?

13. +++ b/core/lib/Drupal/Core/Entity/EntityPermissions.php
    @@ -0,0 +1,62 @@
    +/**
    + * Generates entity permissions via their permission providers.
    ...
    +class EntityPermissions implements ContainerInjectionInterface {
    

    I don't see where this class is ever actually used. What am I missing? Is it intended to be a core service?

    If it's not a leftover, should it implement EntityPermissionsProviderInterface?

14. +++ b/core/lib/Drupal/Core/Entity/EntityPermissions.php
    @@ -0,0 +1,62 @@
    +  /**
    +   * Builds a list of permissions for the participating entity types.
    +   *
    +   * @return array
    +   *   The permissions.
    +   */
    +  public function buildPermissions() {
    

    If so, this would just be {@inheritdoc}

15. +++ b/core/lib/Drupal/Core/Entity/PermissionBasedEntityAccessControlHandler.php
    @@ -0,0 +1,142 @@
    +    // Ensure that access is evaluated again when the entity changes.
    +    return $result->addCacheableDependency($entity);
    

    This cache info should be added in the checkEntityOwnerPermissions and checkEntityPermissions methods. Why? Because checkEntityPermissions does not actually vary per entity.

16. +++ b/core/lib/Drupal/Core/Entity/PermissionBasedEntityAccessControlHandler.php
    @@ -0,0 +1,142 @@
    +          return AccessResult::allowedIfHasPermissions($account, $permissions)
    +            ->cachePerUser();
    +        }
    +        return AccessResult::neutral()->cachePerUser();
    +      }
    +      else {
    +        return AccessResult::allowedIfHasPermissions($account, [
    

    This is where addCacheableDependency($entity) should be. It should be on all of these because it depends on the entity's published status.

17. +++ b/core/lib/Drupal/Core/Entity/PermissionBasedEntityAccessControlHandler.php
    @@ -0,0 +1,142 @@
    +        $result = AccessResult::allowedIfHasPermissions($account, [
    ...
    +        $result = AccessResult::allowedIfHasPermissions($account, [
    

    These, however, do not need to be cached per entity.

18. +++ b/core/modules/user/src/PermissionHandler.php
    @@ -186,6 +198,30 @@ protected function buildPermissionsYaml() {
    +  protected function buildEntityTypePermissions() {
    

    Shouldn't this be named buildPermissions? It will include bundle permissions AFAICT.

This has the potential to massively improve the DX and SX, by bringing more consistency to both developers and site builders. Tagging and bumping to Major.

#61.8+10: Germans won't be happy :)
#61.12+15+16+17: agreed, and you addressed this in https://github.com/fago/entity/pull/53
#61.13: see entity.permissions.yml
#61.7+9+18: see \Drupal\Core\Entity\EntityTypeInterface::getPermissionGranularity().

I've been working on this in the past few days, and here's what I propose for this issue:

- Let's handle only permission providers in this patch and introduce the matching access handler in a followup, because I think the permission provider topic is complex enough to require proper discussion on its own.

- The current implementation from the contrib Entity API module lays an excellent foundation for this work, but IMO it's missing a key concept: a generic way of getting the permission name for a specific operation of a given entity type. For example: "what's the name of the view any published permission for nodes? how about media items? (access content and view media)".

While the existing implementation takes the approach of standardizing permission names, I think it's very unlikely that every core/contrib/custom module will (or can) deprecate their existing permissions and use the ones provided by the new entity handler. In fact, I'm pretty sure that renaming the access content permission to view node would be so disruptive that it can't be accepted even for Drupal 9.

Therefore, my proposal is to add a bunch of methods to EntityPermissionProviderInterface that will allow each entity type to provide a mapping between their existing permission names to a known set of "operations": view any/own published, update any/own, delete any/own, etc.

If the number of proposed methods is too high for a single interface, we could move the "own" methods to a separate one. Same for publishing related methods.

Another goal that I'd like to accomplish here is to deprecate the admin_permission and permission_granularity entity type annotation properties, since they can be expressed now by providing a custom permission handler.

Here's a patch for this proposal, really looking forward to any feedback :) I also generated an interdiff to #54, but it's not really useful for anything.

We also need to think about providing similar methods for revision-related permissions, but let's settle on the basics first and then decide if we want to tackle those here or in a followup.

> - Let's handle only permission providers in this patch and introduce the matching access handler in a followup

+1
Smaller scope and smaller patches is good!

> IMO it's missing a key concept: a generic way of getting the permission name for a specific operation of a given entity type. For example: "what's the name of the view any published permission for nodes? how about media items? (access content and view media)".

That seems useful.

I haven't the time right now for an in-depth review, so here are just a few observations:

1. +++ b/core/lib/Drupal/Core/Entity/DefaultEntityPermissionProvider.php
   @@ -0,0 +1,123 @@
   +          'description' => $this->t('Maintain all the @type available.'),
   

   Missing a value for @type.

2. +++ b/core/lib/Drupal/Core/Entity/DefaultEntityPermissionProvider.php
   @@ -0,0 +1,123 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getViewAnyPublishedPermission($bundle = NULL) {
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getViewOwnPublishedPermission($bundle = NULL) {
   +  }
   

   Vague pondering: what about combining these pairs of methods into one, with a $scope parameter that takes 'any' / 'own' / NULL? Too complex?

3. +++ b/core/lib/Drupal/Core/Entity/EditorialEntityPermissionProvider.php
   @@ -0,0 +1,540 @@
   + * - administer $entity_type
   

   Nitpick: I think in documentation, variables are usually given in CAPS. So: 'access ENTITY_TYPE'.

Vague pondering: what about combining these pairs of methods into one, with a $scope parameter that takes 'any' / 'own' / NULL? Too complex?

That's exactly how I started writing these methods, but they seemed too complex indeed and decided to go for more explicit ones instead :)

Fixed the problems pointed out in #65 and the test fails.

Sigh.. fixed those fails and opened #3010805: Fix misleading variable names in EntityTypeManagerInterface.

Maybe we should go one step further and enforce the usage of the default permission handler for all entity types..

Re #64:

- Let's handle only permission providers in this patch and introduce the matching access handler in a followup, because I think the permission provider topic is complex enough to require proper discussion on its own.

+1

Therefore, my proposal is to add a bunch of methods to EntityPermissionProviderInterface that will allow each entity type to provide a mapping between their existing permission names to a known set of "operations": view any/own published, update any/own, delete any/own, etc.

+1, but careful. I proposed this when discussing the query access handler and Bojan said it was a no-go (because it wasn't in the permission handler, which we're working on here so it might be a go after we change it here). See the discussion here: https://github.com/fago/entity/pull/52/files#diff-e0716a56db3064f97a0a45...

If the number of proposed methods is too high for a single interface, we could move the "own" methods to a separate one. Same for publishing related methods.

+1

Another goal that I'd like to accomplish here is to deprecate the admin_permission and permission_granularity entity type annotation properties, since they can be expressed now by providing a custom permission handler.

Definitely +1, entity type definitions are becoming more and more bloated.

Re #65 / #66:
I'd use explicit methods instead of a catch-all because if we forget about something now, we might need to break BC to factor in new logic. With explicit methods, we can simply add new methods.

+++ b/core/lib/Drupal/Core/Entity/DefaultEntityPermissionProvider.php
@@ -0,0 +1,142 @@
+  /**
+   * Adds the provider and converts the titles to strings to allow sorting.
+   *
+   * @param array $permissions
+   *   The array of permissions
+   *
+   * @return array
+   *   An array of processed permissions.
+   */
+  protected function processPermissions(array $permissions) {
+    foreach ($permissions as $name => $permission) {
+      // Permissions are grouped by provider on admin/people/permissions.
+      $permissions[$name]['provider'] = $this->entityType->getProvider();
+      // TranslatableMarkup objects don't sort properly.
+      $permissions[$name]['title'] = (string) $permission['title'];
+    }
+    return $permissions;
+  }

You're losing valuable translatable metadata by converting the permissions to strings early on. I'd use a sort callable that converts the permissions to strings inside the callable, leaving the originals untouched or introduce a sortPermissions method on the handler (not a fan, but would work).

On an unrelated note, there are many entity types that don't have a notion of "own" entities (almost all config entities). Should the default permission provider really do calculations regarding "own" entities? Seems like we could shave of a few nano/milliseconds here by having two providers to extend from?

Then in EntityType we could default to a given provider based on whether the entity type implements EntityOwnerInterface.

> That's exactly how I started writing these methods, but they seemed too complex indeed and decided to go for more explicit ones instead :)

Thinking about this more: whether we want them explicit or systematic depends on what their intended purpose is.

I remembered a module I made for D7, which shows entity permissions in an easy-to-read grid: https://www.drupal.org/project/permission_grid

For a use case like this, it would be useful to have just one method: getPermission($verb, $scope, $bundle), so the caller can iterate and get everything in a systematic way.

Another thought: the whole of the entity access API works with verb parameters -- as in $entity->access($operation, $account).

So it would be consistent to use that here too.

Looks like #57 might have been forgotten along the way.

@jonathanshaw, re #78: Nope, the patch from #64 has been written mostly from scratch, and it doesn't resemble anymore the code from the Entity API contrib module.

I discussed this patch with @bojanz a couple of times in the last month and we agreed to provide a middle ground between explicit and systematic methods, and fold the "any|own" concept into a $scope argument. This also resolves the problem pointed out in the second paragraph from #75: entity types that don't implement EntityOwnerInterface will simply not return anything for the "own" permission names.

Another thing we agreed on was to take out the "published" part from the viewPublishedPermission() method, so it makes more sense for entity types that don't implement EntityPublishedInterface.

@joachim:

Another thought: the whole of the entity access API works with verb parameters -- as in $entity->access($operation, $account).

The difference with with the entity access layer is that here we have 3 concepts ($operation, $scope, $bundle) instead of one, and bundling all three of them in a single method would provide an inferior DX (compared to the access layer) :)

Fixed the last test fail.

I went ahead and made some of the changes I talk about in the review below (the ones with checkmarks), feel free to take or leave any of them :)

1. +++ b/core/lib/Drupal/Core/Entity/DefaultEntityPermissionProvider.php
   @@ -0,0 +1,124 @@
   +  public function buildPermissions() {
   

   Hmm, why not call this getPermissions and build it during construction of the provider?

2. +++ b/core/lib/Drupal/Core/Entity/DefaultEntityPermissionProvider.php
   @@ -0,0 +1,124 @@
   +  public function getViewPermission($scope = 'any', $bundle = NULL) {
   

   I like the $scope idea!

3. +++ b/core/lib/Drupal/Core/Entity/DefaultEntityPermissionProvider.php
   @@ -0,0 +1,124 @@
   +      // TranslatableMarkup objects don't sort properly.
   +      $permissions[$name]['title'] = (string) $permission['title'];
   

   ✅ We could create another issue to add a static compare(TranslatableMarkup $a, TranslatableMarkup $b) to the TranslatableMarkup class.

4. +++ b/core/lib/Drupal/Core/Entity/EditorialEntityPermissionProvider.php
   @@ -0,0 +1,490 @@
   + * These include:
   + * - administer ENTITY_TYPE
   + * - access ENTITY_TYPE overview
   + * - view (BUNDLE) ENTITY_TYPE
   + * - view own (BUNDLE) ENTITY_TYPE
   + * - view own unpublished ENTITY_TYPE
   + * - create (BUNDLE) ENTITY_TYPE
   + * - edit (own|any) (BUNDLE) ENTITY_TYPE
   + * - delete (own|any) (BUNDLE) ENTITY_TYPE
   

   ✅ This is an incomplete listing, it's missing the 'published' permissions and I think that this should just be removed, the $permissionMap property is self-documenting enough.

5. +++ b/core/lib/Drupal/Core/Entity/EditorialEntityPermissionProvider.php
   @@ -0,0 +1,490 @@
   + * Note that the 'view own (BUNDLE) ENTITY_TYPE' permission will enable caching
   + * per user which might harm performance of most sites, so it is disabled by
   + * default.
   

   ✅ Nit: "Note that the 'view own published' permissions will require caching per user. This may harm performance on most sites and is therefore disabled by default."

   It's not just bundle specific. Right?

6. +++ b/core/lib/Drupal/Core/Entity/EditorialEntityPermissionProvider.php
   @@ -0,0 +1,490 @@
   +    $this->buildPermissionMap();
   

   ✅ This would be easier to alter/extend by method decoration if it returned the map array, rather than setting the class state within the method. Like so:

   $this->permissionMap = $this->buildPermissionMap()

7. +++ b/core/lib/Drupal/Core/Entity/EditorialEntityPermissionProvider.php
   @@ -0,0 +1,490 @@
   +      $this->buildPerEntityTypePermissionMap();
   ...
   +      $this->buildPerBundlePermissionMap();
   

   ✅ Same as above.

8. +++ b/core/lib/Drupal/Core/Entity/EditorialEntityPermissionProvider.php
   @@ -0,0 +1,490 @@
   +      $this->perBundlePermissionMap['view any published'] = "view __BUNDLE__ {$entity_type_id}";
   

   ✅ __BUNDLE__ is begging for a class constant.

9. +++ b/core/lib/Drupal/Core/Entity/EditorialEntityPermissionProvider.php
   @@ -0,0 +1,490 @@
   +  public function getViewPermission($scope = 'any', $bundle = NULL) {
   

   ✅ If $bundle were required, it might make this a little simpler to use since EntityInterface->bundle() will return the entity type ID if the entity does not have bundles.
   That way callers writing generic code won't need to do any gymnastics to figure out whether to pass the value or not. Additionally, I think it will allow getting rid of $perBundlePermissionMap.

d.o had a little too much I think.

1. +++ b/core/lib/Drupal/Core/Entity/DefaultEntityPermissionProvider.php
   @@ -0,0 +1,124 @@
   +  protected function processPermissions(array $permissions) {
   +    foreach ($permissions as $name => $permission) {
   +      // Permissions are grouped by provider on admin/people/permissions.
   +      $permissions[$name]['provider'] = $this->entityType->getProvider();
   +      // TranslatableMarkup objects don't sort properly.
   +      $permissions[$name]['title'] = $permission['title'];
   +    }
   +    return $permissions;
   +  }
   +
   

   May I ask whether the better fix wouldn't be to fix admin/people/permissions?

2. +++ b/core/lib/Drupal/Core/Entity/EditorialEntityPermissionProvider.php
   @@ -0,0 +1,479 @@
   +          $permissions[$permission_name] = $this->getPermissionInfoForBundle($key, $bundle_info);
   ...
   +        $permissions[$permission_name] = $this->getPermissionInfo($key);
   ...
   +  protected function getPermissionInfo($permission_key) {
   ...
   +    $info = [];
   ...
   +      default:
   +        break;
   +    }
   

   Is there a case where 'default' is an expected behaviour? Returning an empty array seems not needed. How about throwing an exception in that case?

3. +++ b/core/lib/Drupal/Core/Entity/EntityPermissionProviderInterface.php
   @@ -0,0 +1,107 @@
   +  public function buildPermissions();
   

   I do agree with @gabesullice ... given that everything is prefixed with get already using get for all permissions too seems sensible.

4. +++ b/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php
   @@ -318,21 +318,28 @@ protected function getDeleteFormRoute(EntityTypeInterface $entity_type) {
   +        $permission = trim(implode('+', [$admin_permission, $collection_permission]), '+');
   

   Is it just me or would $permission = implode('+', array_filter([$admin_permission, $collection_permission])) be more readable?

Had a quick stroll through this issue and the latest patch, here's some of my thoughts.

Regarding permission names: there seems to be a standardisation trend in Core now for generated permissions towards a schema of ENTITY_TYPE BUNDLE: PERMISSION. Most recent example of this is #2914486: Add granular permissions to the Layout Builder, which was inspired by the format used for generated Field UI permissions. I vote we try and stick with this new de facto standard for permission naming as it encourages consistency and so makes everyone's lives easier.

Regarding #6:

1) EntityType has getRouteProviders(), do we want a matching getPermissionProviders()? Don't see much of a benefit.

I'm tempted to say that it could be useful to provide this method, it keeps things consistent, and probably makes it easier to get a list of the providers. Is there a reason we wouldn't want to provide this method?

DefaultEntityPermissionProvider class: this has a bunch of stub methods, will these be filled out later? Otherwise it might make sense to just remove these so that the EntityPermissionProviderInterface forces any extending classes to define these, therefor increasing reliability.

I notice that the Entity Type Label Singular and Plural values used to build the permission titles. I wonder if it would be useful to introduce new optional keys to the Entity Type Annotation in the event the Entity Type wants to provide different labels for the Permission Titles. There is already president for doing this, e.g. the key label_collection which overrides the title of the Collection route. Of course if we did decide to implement this, the new keys should be optional and if not specified simply fall-back to the Singular and Plural Labels.

I am at Seattle Sprint and working on this issue.
Update: No longer working on this. We thought this was a novice issue and it is not.

I see mentions of "view own unpublished ENTITY_TYPE," but not "view unpublished (BUNDLE) ENTITY_TYPE" (or "view own unpublished (BUNDLE) ENTITY_TYPE") -- are these perms out of scope for this issue?

(I ask b/c there was a question over on #2875867: Add per-bundle unpublished content permissions about possibly switching gears to this issue, instead of that one -- or maybe it's just that #2875867 ought to be postponed until after this one (#2809177) is done?)

Re-rolled for D8.9.x

For Comment entity there is an additional factor: You can only create comments if the comment field of parent node/entity is still open. For comments there is now only an 'edit own' permissions, but not 'edit any', should that really be introduced?

Reroll for 9.1.x

NW for #85.

Thanks for all the work on this so far.

1. The stub methods in DefaultEntityPermissionProvider should either explicitly return NULL or just return the value of $this->getAdminPermission().
   

2. +  /**
   +   * Gets the name of the collection permission.
   +   *
   +   * This permission is mostly used for providing access to the 'overview' page
   +   * of an entity type. For other kind of listings, for example one that is
   +   * provided by a HTTP REST API, the 'view any published' permission should be
   +   * used instead.
   +   *
   +   * @return string|null
   +   */
   +  public function getCollectionPermission();
   

   This is on EntityPermissionProviderInterface so 'view any published' might not make sense here, as "published" is only applicable to EditorialEntityPermissionProvider.
   
   

3. Moreover I'm not totally comfortable with EntityPermissionProviderInterface::getViewUnpublishedPermission even existing in that interface, the concept of published vs unpublished is only applicable to Content Entities which implement the Publishable interface, and is completely irrelevant for Config Entities. Perhaps we need a EditorialEntityPermissionProviderInterface.
   
   
4. I notice that in some places null (lowercase) is used, I believe our coding standards say NULL (uppercase) should be used.
   

5. diff --git a/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
   index dcff3fc3b2..410d31379b 100644
   --- a/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
   +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
   @@ -228,4 +228,18 @@ public function count() {
        return mb_strlen($this->render());
      }
    
   +  /**
   +   * Provides a comparison function so instances of this class can be sorted.
   +   *
   +   * @param \Drupal\Core\StringTranslation\TranslatableMarkup|string $a
   +   *   The first instance.
   +   * @param \Drupal\Core\StringTranslation\TranslatableMarkup|string $b
   +   *   The second instance.
   +   *
   +   * @return int
   +   */
   +  public static function compare($a, $b) {
   +    return strcmp($a instanceof self ? $a->render() : $a, $b instanceof self ? $b->render() : $b);
   +  }
   +
   

   While this might be useful, I think introducing that in this issue is far too out of scope, as the scope of this issue should be within the Entity system, if we need this we should open a separate issue, correctly scoped, and postpone on it.
   
   

6. I'd also like to stand by what I said in #88 about the naming of permissions:
   

   Regarding permission names: there seems to be a standardisation trend in Core now for generated permissions towards a schema of ENTITY_TYPE BUNDLE: PERMISSION. Most recent example of this is #2914486: Add granular permissions to the Layout Builder, which was inspired by the format used for generated Field UI permissions. I vote we try and stick with this new de facto standard for permission naming as it encourages consistency and so makes everyone's lives easier.

   I see we are using the pattern of BUNDLE: PERMISSION, but at least for the machine names (so not the ones that people see in the UI, although why not for those too) I do believe we should prefix that with ENTITY_TYPE, even if that might require some BC work for core or contrib while implementing this new API, I think it's better to be consistent with this new precedent rather than continue old inconsistencies.
   
   

7. Lastly, it's worth being aware of #3043321: Use generic access API for node and media revision UI and #2350939: Implement a generic revision UI, if those land first we should try to account for those new permissions here and vis-versa.

Thanks,
-Aaron

#3043321: Use generic access API for node and media revision UI has been fixed.

MR #2912 Is a reroll of patch from #96 for 10.1.x branch review comments from #98 need to be addressed.

@bojanz in #53 you wrote:

New feature (resolving #9 from this issue):
#2977231: EntityPermissionProvider should provide per-bundle view permissions

but the issue summary still says:

Note that view permissions are never per-bundle cause we have no way to enforce it, we'd need query access for that (ala node access).

I wasn't confident enough to remove that from the issue summary, so could you perhaps have a look to be sure it should be removed?

Looks like the MR already contains the relevant code:

  /**
   * Whether per-bundle permissions should be provided for the 'view' operation.
   *
   * Note that self::$bundleGranularity must also be TRUE in order for this
   * setting to be taken into consideration.
   *
   * @var bool
   */
  protected $viewBundleGranularity = FALSE;

(https://git.drupalcode.org/project/drupal/-/merge_requests/2912/diffs#10...)

I just created #3353503: Add "view $bundle media" permission where exactly that is needed, but I can't tell if it's still an issue. I think it isn't?
Thanks!

Next steps: Solve #85 and #98

We're currently writing a new permission module: https://www.drupal.org/project/entity_access_by_reference_field

For that reason, we had a look into the different hook_entity_access implementations in core:

• node
• media
• taxonomy term
• user

All of them, but node, use "update", only node uses "edit".

Also, the documentation at https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21... says

The operation that is to be performed on $entity. Usually one of:
"view"
"update"
"delete"

but the code in the MR uses "edit" instead of "update". That's quite confusing and we think there should be a consensus on one correct term!

Here's the current implementation from the MR:

    /**
   * @var array
   */
  protected $permissionMap = [
    'admin' => NULL,
    'collection' => NULL,
    'view any published' => NULL,
    'view own published' => NULL,
    'view own unpublished' => NULL,
    'edit any' => NULL,
    'edit own' => NULL,
    'delete any' => NULL,
    'delete own' => NULL,
    'create' => NULL,
  ];

So, we'd strongly suggest to change "edit" to "update". Also, the method names use "update", like getUpdatePermission()

Should I update the MR accordingly?

My bad, the operation is always "update", but the permission is "edit" (which is still quite confusing).

@Core maintainers: Any chance or way to push this forward? Seems this lost traction again, but other important issues are postponed on this (like #273595: Move permission "view any unpublished content" from Content Moderation to Node) and that is still something weird in core, because in the current state users are permitted to view a node they can edit and can't save the node they edit, because URL alias checks view access! See #3167732: Non-admins cannot save unpublished nodes with path alias

So I think for the permission system this is really important to unblock the other issues. How to proceed?

I'd be willing to give this another go, but with what I've learned in the past 6 years since I commented here I'd have to revisit if what was being discussed back then is still relevant now. I have since dropped my dependency on contrib Entity API and created new mechanisms in Group to deal with access.

Some of these are permission providers and access control handlers, but as services that can easily be decorated. Ultimately, I'd like to see if we can do something similar in core but that would be a huge endeavor. Either way, most of the discussion happened years ago and perhaps the first step should be to check how much of the discussion is still applicable today.

Made changes against head and fixed phpstan issues and tests.

Please read my latest comment.

One thing I for instance think is a huge foot-gun is putting the specific permission methods on the interface. Right now it's getUpdatePermission, getDeletePermission, etc. but we cannot know all of the possible entity operations in a site. In Group I fixed this by having a method that takes an operation and returns a permission.

Either way, I don't think we should introduce yet another handler that no-one else can alter properly due to how handlers work in core right now.

Adding workaround in Issue Summary:

Until this gets committed in Drupal core, Node view permissions module enables permissions "View own content" and "View any content" for each content type on permissions page as it was on Drupal 6.

Based on comments in #3090468: Access Control with View Own Content / View Own Entity in Core, thanks @pameeela and @anybody!

Read through the issue. The MR as now is quite small but I wonder, would it make sense to split EntityAccessControlHandler and providers into separate issues. Would that make this issue movable again?

Either way, I don't think we should introduce yet another handler that no-one else can alter properly due to how handlers work in core right now.

@kristiaanvandeneynde can you elaborate a bit? I am not sure I follow.