Provide a default query_access handler for core (maybe all?) entity types

I applaud the idea, but we've found this idea to be very problematic in practice.

Adding a query_access handler causes all queries to now be altered and respect permissions. When we did this for Commerce, it caused many bugs across the ecosystem, where existing entity queries had to be given ->accessCheck(FALSE). Examples of that include code that runs on cron or in queues (since the user might be anonymous) and custom load methods in entity storage classes (since storage should generally return entities unaffected by access). Plus the general user surprise, when permissions start being enforced where they previously weren't.

Considering how many contribs load and manage core entity types, all of these problems have the potential to be much worse than they were in Commerce.

FYI, going to create a light version of this in Group:

• Does the entity type have a query access handler? Subscribe to the entity.query_access.{$entity_type_id} event to add Group's logic.
• Does the entity type not have a query access handler? Add one that only fires the event listed above so nothing changes as far as the original entity type is concerned, but Group can add its access control checks.
• If core or Entity API ever decide to add one for core's entity types, then nothing needs to be changed in Group.

Thought this idea might help anyone trying to get query access going on entity types they do not own so that their module might intervene.

Come to think of it, we could just as well do this in Entity API so that at least the alter event is fired for any entity type and the actual query_access handler does absolutely nothing but fire the event. At least this way, modules can benefit of the streamlined query altering system provided by Entity API and we wouldn't break core/contrib in the process.

Setting back to Active. If you disagree with this approach, please just close it again @bojanz :)

I'd like to mention about set of existing domain entity modules - they use different hacks to make entities "domainable"

Something as simple as:

/**
 * Implements hook_entity_type_alter().
 */
function group_entity_type_alter(array &$entity_types) {
  /** @var $entity_types \Drupal\Core\Entity\EntityTypeInterface[] */
  // Sets a generic query_access handler for all entity types that have none.
  foreach ($entity_types as $entity_type_id => $entity_type) {
    if (!$entity_type->hasHandlerClass('query_access')) {
      $entity_type->setHandlerClass('query_access', 'Drupal\group\Entity\Access\GenericQueryAccessHandler');
    }
  }
}

With the generic class being:

namespace Drupal\group\Entity\Access;

use Drupal\Core\Entity\EntityHandlerInterface;
use Drupal\Core\Entity\EntityTypeInterface;
use Drupal\Core\Session\AccountInterface;
use Drupal\entity\QueryAccess\ConditionGroup;
use Drupal\entity\QueryAccess\QueryAccessEvent;
use Drupal\entity\QueryAccess\QueryAccessHandlerInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\EventDispatcher\EventDispatcherInterface;

/**
 * Generic query access handler for entity types that do not have one.
 *
 * This query_access handler only fires the alter event so that modules like
 * Group can subscribe to the event and further alter the queries for the entity
 * type in question without having to duplicate the code from Entity API.
 *
 * @see \Drupal\entity\QueryAccess\QueryAccessHandler
 */
final class GenericQueryAccessHandler implements EntityHandlerInterface, QueryAccessHandlerInterface {

  /**
   * The entity type.
   *
   * @var \Drupal\Core\Entity\EntityTypeInterface
   */
  protected $entityType;

  /**
   * The event dispatcher.
   *
   * @var \Symfony\Component\EventDispatcher\EventDispatcherInterface
   */
  protected $eventDispatcher;

  /**
   * The current user.
   *
   * @var \Drupal\Core\Session\AccountInterface
   */
  protected $currentUser;

  /**
   * Constructs a new QueryAccessHandlerBase object.
   *
   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
   *   The entity type.
   * @param \Symfony\Component\EventDispatcher\EventDispatcherInterface $event_dispatcher
   *   The event dispatcher.
   * @param \Drupal\Core\Session\AccountInterface $current_user
   *   The current user.
   */
  public function __construct(EntityTypeInterface $entity_type, EventDispatcherInterface $event_dispatcher, AccountInterface $current_user) {
    $this->entityType = $entity_type;
    $this->eventDispatcher = $event_dispatcher;
    $this->currentUser = $current_user;
  }

  /**
   * {@inheritdoc}
   */
  public static function createInstance(ContainerInterface $container, EntityTypeInterface $entity_type) {
    return new static(
      $entity_type,
      $container->get('event_dispatcher'),
      $container->get('current_user')
    );
  }

  /**
   * {@inheritdoc}
   */
  public function getConditions($operation, AccountInterface $account = NULL) {
    $account = $account ?: $this->currentUser;
    $conditions = new ConditionGroup('OR');

    // Allow other modules to modify the conditions before they are used.
    $event = new QueryAccessEvent($conditions, $operation, $account);
    $this->eventDispatcher->dispatch('entity.query_access.' . $this->entityType->id(), $event);

    return $conditions;
  }

}

Not sure we want to be testing this as it does not change anything functionally aside from entity types without a handler now having a basic "empty" one. We could write a test for a "test entity type" without a handler and then call $entity_type->hasHandler() on it to verify that we set the handler, I suppose.

Here's the patch from #7 that takes the change from #3134363: Support a catch-all QueryAccessEvent for modules that may want to alter all entity queries into account already, in case that issue gets committed first.

One thing that would be interesting is to add a test which verifies the generic query access is added to Node, by providing a test module which subscribes to node query access.

Yeah I was just thinking the same when we talked about these patches yesterday, will add that.

This is the standalone patch, does not combo with the other issue.

Forgot core has some interesting naming patterns on node entity types.

1. +++ b/src/QueryAccess/GenericQueryAccessHandler.php
   @@ -0,0 +1,82 @@
   +  public function getConditions($operation, AccountInterface $account = NULL) {
   +    $account = $account ?: $this->currentUser;
   +    $conditions = new ConditionGroup('OR');
   +
   +    // Allow other modules to modify the conditions before they are used.
   +    $event = new QueryAccessEvent($conditions, $operation, $account);
   +    $this->eventDispatcher->dispatch('entity.query_access.' . $this->entityType->id(), $event);
   +
   +    return $conditions;
   

   haven't looked at the entity query access api before, but why OR? Wouldn't that mean that if two separate modules implement the event, it would allow one *or* the other condition? That's not how access API's usually work.

   The event also doesn't have a way to set a new condition group object, so you couldn't change it to an AND condition object even if you'd want to.

   I did check that nothing happens if the conditions object is empty, so that's good.

2. +++ b/tests/modules/entity_module_test/src/EventSubscriber/QueryAccessSubscriber.php
   @@ -13,10 +13,25 @@ class QueryAccessSubscriber implements EventSubscriberInterface {
   +   *   The event.
   +   */
   ...
   +    $conditions = $event->getConditions();
   +    $conditions->addCondition('type', 'foo');
   +  }
   

   adding this to an existing test and always doing it is going to be really confuse someone writing some other tests.

   Usually entity/node access tests in core work with flags and/or dedicated modules. Lets add a \Drupal::state()->get('something') and enable it just for our specific test?

haven't looked at the entity query access api before, but why OR? Wouldn't that mean that if two separate modules implement the event, it would allow one *or* the other condition? That's not how access API's usually work.

AFAICT: It tries to stay as close to node grants as possible so that the transition into core might be smoother. With node grants, it's also the case that if you have one key, you have access, regardless of the amount of locks. The difference here being that a handler could set isAlwaysFalse() to TRUE, which would make the query return no results.

The default conjunction of ConditionGroup is actually AND, but looking through the code, it's basically always instantiated with OR.

The event also doesn't have a way to set a new condition group object, so you couldn't change it to an AND condition object even if you'd want to.

This part of the API has raised the same question for me, but I suppose if you allow it to be altered you could break other modules' access logic. You can nest groups, though. So if you need an AND conjunction, you can create your own ConditionGroup and add it to the main one.

Re #14.2
I'll have another look at the test

Here we go.

And this one makes it ready to be committed after #3134363: Support a catch-all QueryAccessEvent for modules that may want to alter all entity queries lands.

Needs a reroll.

Rerolled patch #17, please review.

The reroll in #19 is missing the subscriber. I actually already pre-emptively rerolled this issue in #17. I'll just re-attach the same patch from that comment.

Hmm I see what was going on. Both patches used the same event subscriber method name. I changed the wording a bit, code remains the same functionally.

I think this does implement it in a way that addresses the initial concerns from @bojanz, it won't do anything without someone actually implementing this. It does have a bit of a performance overhead, but hopefully not too bad compared to actually building the entity query.

1. +++ b/src/QueryAccess/GenericQueryAccessHandler.php
   @@ -0,0 +1,84 @@
   +/**
   + * Generic query access handler for entity types that do not have one.
   + *
   + * This query access handler only fires the alter event so that modules can
   + * subscribe to the query access alter event to alter any entity query or views
   + * query without having to duplicate the related code from Entity API.
   + */
   +final class GenericQueryAccessHandler implements EntityHandlerInterface, QueryAccessHandlerInterface {
   

   A bit unsure about the naming here.

   With this, we now have a QueryAccessHandlerBase with a ton of default logic, and we have a GenericQueryAccessHandler that's just doing the events.

   Feels like it should be kind of the other way round, but it's too late to change that now.

   Specifically, \Drupal\entity\QueryAccess\QueryAccessHandlerBase::getConditions() could be the shared base class with an empty buildConditions() method that a DefaultQueryAccess method could then implement with that owner logic and so on.

   As I said, too late now here, but maybe something to consider if/when we port this to core.

   What about at least naming this differently though, after it what actually does, something like EventOnlyQueryAccessHandler?

   Thoughts?

2. +++ b/entity.module
   @@ -67,6 +67,19 @@ function entity_entity_type_build(array &$entity_types) {
   +  // Sets a generic query_access handler for all entity types that have none.
   +  foreach ($entity_types as $entity_type_id => $entity_type) {
   +    if (!$entity_type->hasHandlerClass('query_access')) {
   +      $entity_type->setHandlerClass('query_access', 'Drupal\entity\QueryAccess\GenericQueryAccessHandler');
   +    }
   

   For the comment, I'd suggest "default query_access handler" or so.

   Slowly getting into this and wondering about two things now, considering that we learned that the query access logic is actually on the the SQL query layer.

   a) It doesn't really make sense for config entities, it will never be called for them anyway. Lets skip them?

   b) It will also only be called on content entities with an sql storage. Limit it to storages that implement \Drupal\Core\Entity\Sql\SqlEntityStorageInterface?

3. +++ b/tests/modules/entity_module_test/src/EventSubscriber/QueryAccessSubscriber.php
   @@ -15,6 +15,7 @@ class QueryAccessSubscriber implements EventSubscriberInterface {
        return [
          'entity.query_access' => 'onGenericQueryAccess',
          'entity.query_access.entity_test_enhanced' => 'onQueryAccess',
   +      'entity.query_access.node' => 'onPolyfilledQueryAccess',
        ];
      }
   

   more naming fun.

   This is just the test module, but still.

   Her we now have generic, which actually maps to the logic of the base query handler, and the other one uses polyfill, but this doesn't fit my understanding of the word polyfill, it's also not used elsewhere. Maybe something with Default, or again the event only thing from above.

4. +++ b/tests/modules/entity_module_test/src/EventSubscriber/QueryAccessSubscriber.php
   @@ -81,4 +82,21 @@ class QueryAccessSubscriber implements EventSubscriberInterface {
   +   *
   +   * This is just a convenient example for testing whether the generic query
   +   * access subscriber is added to entity types that do not specify a query
   +   * access handler; in this case: node.
   

   the word "just" should be avoided in documentation I think, also, "convenient" (for whom?) doesn't seem necessary, "This is an example" seems enough here?

Re 1. and 3. You're right, "generic" does not apply here as much as it did for the "fire the event for any entity type" issue. Let's rename it to EventOnly across the board. The base/default class change for core I completely agree with.

Re 2.a and 2.b: We should skip the config entities. The SQL one is already being checked for in ViewsQueryAlter/EntityQueryAlter, but I guess it makes sense to test it early to reduce overhead.

Re 4. copied that straight from the test of that test class.

I'll make changes to 1, 2 and 3 already and leave 4 up for you to decide.

Think I got everything

Hmm forgot to address 2. Turns out I can accommodate 2a, but not 2b. As you're trying to load the storage during entity type compilation, meaning you'll end up in an endless loop by calling the entity type manager while entity types are still being built. So the checking for SQL storage needs to remain in the 2 QueryAlter classes.

For 2b, I think you should be able to use is_subclass_of($entity_type->getStorageClass(), SqlEntityStorageInterface::class).

That could possibly conflict on an entity type that has its storage altered later on, but is possibly something we can ignore.

Ok, here we go. And I agree that we should not need to worry about people altering entity types.

was looking a bit more at the inner workings of the query alter stuff, not related to this issue directly, but posting it here for now as they were just in slack before, might be useful for the core issue eventually.

huuu... that's some pretty heavy dark magic in \Drupal\entity\QueryAccess\EntityQueryAlter::mapConditions

re-using the entity query tables query builder, creative

so it acts on database queries but the conditions you pass in are entity query conditions

was wondering if just specifying 'type' there would case some conflicts

does result in possible quite inefficient queries, or better put, making them even worse. as it doesn't reuse the existing joins, so an entity query with condition on title and then the access condition on type starts on node and then adds two separate joins to node_field_data

this isn't really specific about this patch, just slowly starting to understand the whole entity query access thing

I was wondering if applying it on the lower level is so that you could specify more complex sql stuff than entity query, but if the conditions are again basically entity query, so I guess not, maybe just the only way we have to alter those queries?

Same comments are still a bit suboptimal I think, but the class names at least are much better now, this makes more sense to me. Committed.

Huge thanks!

Re #30 cross-posting from Slack:

I too am seeing a few oddities about the new system (such as Views queries being altered twice), but it's the best we've got :slightly_smiling_face: Node grants won't do any longer and other entity types have nothing aside from this new system.
So changes I'd definitely expect to discuss are:

• Make it work on all queries without triggering Views queries twice
• Make top-level conjunction AND rather than OR
• Avoid creative code reuse but instead make core facilitate these types of creations so that any changes in core's query handling immediately takes effect in query access alters. This means easier condition compiling, reuse of existing joins, etc.

For instance adding the initially used Tables object as metadata to the query would easily fix the recreation of joins, as you could then call ensureEntityTable() on that object and it would find that it had already added them during initial query building.