Address reviews in #2809177

[gabesullice]

This addresses some of the caching reviews given in #2809177, specifically:

• that view own unpublished must be cachePerUser() and therefore isn't very cacheable.
• there were some edge cases where it is possible to have an access result that should not be cached per entity
• that the $operation any permissions do not need to be cached per user (caching by the user.permissions cache context is enough).

It uses a new entity annotation key to factor away the two derivative classes of EntityAccessControlHandlerBase which removed quite a bit of code, which was discussed today on a call between @bojanz, @wimleers and myself as something that could be attempted.

It renames EntityAccessControlHandlerBase to PermissionBasedEntityAccessControlHandler because that was favored on the aforementioned call.

What remains:

Possibly merging EntityPermissionProvider and UncacheablePermissionProvider to better mirror the access control handler.
Providing useful $reason arguments where appropriate to improve DX.
Nits and things.

[wimleers]

Commit 1 (Move 'view own unpublished $entity_type' permissions to UncacheableEntityPermissionProvider.)

Makes sense because this is the only permission that also needs the user cache context. This seems to have been a simple oversight in earlier patch iterations. view (own|any) ($bundle) $entity_type already lives on the uncacheable permission provider, so this one should too.

@todo: this should then also have updated the docs and the entity access control handler. Basically, the "cacheable" permission provider/access control handler should ONLY have the "mutate/write own/any" permissions, because those permissions don't impact cacheability (as much).

Commit 2 (Do not cache access per entity where it's not required.)

Had to carefully triple-check this, thought I spotted a bug twice, but AFAICT it is correct.

Introduces an extraneous semi-colon :)

Commit 3, part 1 (Combine EntityAccessControlHandler and UnprocessableEntityAccessControlHandler into their base class)

If this was done in a separate commit, would've made the changes clearer.

Commit 3, part 2 ( Also, do not applycachePerUser()for the$operation anycases. When$operation anyis not allowed, it then checks the$operation own permissions.)

YES!!!

I'd written a comment about exactly that:

In HEAD, EntityAccessControlHandlerBase::checkEntityOwnerPermissions() was incorrectly varying always by the user cache context. It shouldn't do that for … any … permissions. Those should be checked first, and only if the result to those is "neutral", should we check the … own … permissions.

That's now obsolete feedback! 🎉

---

Having looked at this, I once again got confused by the permissions in the UncacheableEntity* vs Entity* classes. In the Entity* class, I still see plenty of permissions that need to vary by user:

 * - update (own|any) ($bundle) $entity_type
 * - delete (own|any) ($bundle) $entity_type

Why are these okay? #51 does not clearly say. The docs only say:

 * As this class supports "view own ($bundle) $entity_type" it is just cacheable
 * per user, which might harm performance of sites. Given that please use 
 * \Drupal\entity\EntityPermissionProvider unless you need the feature, or your
 * entity type is not really user facing (commerce orders for example).

The answer is — I'm pretty sure — what I wrote earlier:

Basically, the "cacheable" permission provider/access control handler should ONLY have the "mutate/write own/any" permissions, because those permissions don't impact cacheability (as much).
So, expanding those comments.

Also addressing other comment-related unclarities, some I found myself, some were raised by @gabesullice in https://www.drupal.org/project/drupal/issues/2809177#comment-12782117.

[wimleers]

Because github is terrible for multiple people to collaborate, I had to create my own fork that duplicates @gabesullice's. I've pushed my suggested additional changes there. See https://github.com/wimleers/entity/commits/permission-provider.

I'll explain the commits when I'm done.

Gabe will be able to pull them into his pull request when he's reviewed them.

[wimleers]

1. wimleers@381d2bb — just improving/fixing comments.
2. wimleers@1498b6c — making the most affected unit test executable again. 6 failures.
3. wimleers@b0d0f5a — fix bug in Gabe's code. 6 -> 3 failures.
4. wimleers@34ee02e — labeling test cases to aid debugging of failing tests
5. wimleers@eb693cc — fix bug in HEAD of this module, which Gabe's refactor unveiled. More about that below.

---

In Gabe's refactor, there was a subtle behavior change that was thankfully caught by the unit test coverage.

For an entity that implements EntityOwnerInterface, HEAD's EntityAccessControlHandlerBase ::checkAccess() calls ::checkEntityOwnerPermissions(). In HEAD (before this patch), EntityAccessControlHandler::checkEntityOwnerPermissions() overrides its parent implementation to ensure that view ($bundle) ($entity_type_id) gets called IF the entity is either published or it does not implement EntityPublishedInterface.

In this patch, that subtle (subtle at least in terms of code) edge case is removed.

I wanted to say that this is a bug that Gabe introduced. But after looking at the permission provider test coverage, I'm not so sure.

Quoting:

    // Content entity type with bundles and owner.
…
      'view any black_entity' => 'View any black entities',
      'view own black_entity' => 'View own black entities',
      'view any third black_entity' => 'Third: View any black entities',
      'view own third black_entity' => 'Third: View own black entities',
…
    // Content entity type with bundles and owner and entity published.
…
      'view any pink_entity' => 'View any pink entities',
      'view own pink_entity' => 'View own pink entities',
      'view own unpublished pink_entity' => 'View own unpublished pink entities',
      'view any third pink_entity' => 'Third: View any pink entities',
      'view own third pink_entity' => 'Third: View own pink entities',
      'view own unpublished third pink_entity' => 'Third: View own unpublished pink entities',

Those are all view * permissions. Note the absence of view ($bundle) ($entity_type_id)!

IOW: the test coverage was wrong. It should have granted the view any ($bundle) ($entity_type_id) permission for that test to pass! IOW this bug was similar to https://www.drupal.org/project/drupal/issues/2977379.

[wimleers]

Before I take this too far, I'd like feedback from @bojanz and @gabesullice. Stopping to work on this for now.

[bojanz]

Note that Entity API needs to maintain BC because it's used by contrib (Commerce, Media).
Removing UncacheablePermissionProvider or renaming EntityPermission provider would be BC breaks.

I have pinged @dawehner about the "view own published" move, but as far as I remember, him Berdir and myself agreed on the current version when reviewing the code, because most queries deal with published entities only, making the impact of that permission on cacheability low. On the other hand, it's definitely useful to have it, and core tends to agree (Node, for example).

[wimleers]

Note that Entity API needs to maintain BC because it's used by contrib (Commerce, Media).
Removing UncacheablePermissionProvider or renaming EntityPermission provider would be BC breaks.

Ok so then let's definitely not merge those. Renaming the entity access control handler is fine though, right?

I have pinged @dawehner about the "view own published" move, but as far as I remember, him Berdir and myself agreed on the current version when reviewing the code, because most queries deal with published entities only, making the impact of that permission on cacheability low. On the other hand, it's definitely useful to have it, and core tends to agree (Node, for example).

I see. It'd have been very helpful if the rationale behind this was documented somewhere. Because "uncacheable" is tied to "per user" cacheability for sure, and then seeing some things exempted and others not is super super confusing. I once already spent about 2 hours trying to reconstruct the reasoning, see the long blob of text at https://www.drupal.org/project/drupal/issues/2809177#comment-12749733.
And now in this PR, both Gabe and I are then apparently wasting time on it. Let's get clarity on this so that we can end the confusion here.

[dawehner]

If its wrong to have the cacheability like that, than we do have a security issue in core for that, don't we?

[gabesullice]

Okay, I pulled @wimleers first three commits.

I need to dig into his last two, which update tests.

Then, I'll refactor in a separate commit to preserve BC for the contrib module.

[gabesullice]

I stepped through the coverage myself to verify @wimleers conclusion that:

the test coverage was wrong. It should have granted the view any ($bundle) ($entity_type_id) permission for that test to pass

and I agree. When the entity type implements EntityOwnerInterface the permissions should be named view any {entity_type} and view own {entity type}. When it does not implement the owner interface, the permission name should not have the any|own distinction.

[wimleers]

I think @gabesullice's latest commits addressed @bojanz' concerns WRT breaking BC in the contrib entity module.

At the same time, they help ensure that the difference between what would end up in core versus what is in contrib is minimal.

Hopefully @bojanz sees no more problems?

[bojanz]

I am very confused by the current state of this work.

1. "view own unpublished ($bundle) $entity_type" has still been moved to the uncacheable version, even though we clarified that there's no consensus to doing that. And even if there was, moving it represents a BC break. None of the Commerce entity types use the uncacheable version, but people do rely on the permission to view their own unpublished products (just like they do for nodes).

2. The access control handlers were merged, but the permission providers weren't. Those two should always be in sync, logic-wise.

3. PermissionBasedAccessControlHandler::checkAccess has this logic:

    if ($result->isNeutral()) {
      if ($entity instanceof EntityOwnerInterface && $this->requiresViewOwnAccessCheck) {
        $result = $this->checkEntityOwnerPermissions($entity, $operation, $account);
      }
      else {

This means that for the regular ("cacheable") version we no longer check update/delete own/any.

I am also nervous about the fact that we're modifying the tests at the same time as doing the refactoring, which means we have no guard against unintentional breaks.

We need an issue that applies cacheability fixes (perUser, etc). An issue that expands tests. And then an issue that merges handlers. Doing it all at once is going to take a long time to verify.

[bojanz]

Opened https://www.drupal.org/project/entity/issues/3004849