Problem/Motivation

In #2785449: It's too easy to write entity queries with access checks that must not have them it became a requirement to explicitly set QueryInterface::accessCheck (at least for content entities). However, most engineers are surprised to learn that the method doesn't actually do any access checking in core. This can lead developers to assuming that their query is being modified to ensure that only the entities the current user has access too are returned, but that is not the case.

Indeed, in the associated change record, this example is provided:

// Unchanged: This gets all articles the current user can view.
$ids = \Drupal::entityQuery('node')
  ->accessCheck(TRUE)
  ->condition('type', 'article')
  ->execute();

// Unchanged: This gets all articles that exist regardless of access.
$ids = \Drupal::entityQuery('node')
  ->accessCheck(FALSE)
  ->condition('type', 'article')
  ->execute();

Both of the above queries return the exact same result, regardless of the user's level of access.

Steps to reproduce

  1. On a fresh Drupal install, create an unpublished node
  2. Locally, install/enable the Entity Query Access module
  3. While logged out, go to /entity_query_access/node (or any other entity type you should not have access too)
  4. You should get the first 10 entities, regardless of your level of access to those entities

Proposed resolution

TBD

Remaining tasks

TBD

User interface changes

N/A

Introduced terminology

TBD

API changes

TBD

Data model changes

TBD

Release notes snippet

TBD

Comments

davidwbarratt created an issue. See original summary.

ghost of drupal past’s picture

ghost of drupal past
he/him
commented
Status: Active » Postponed (maintainer needs more info)

NodeHooks1::queryNodeAccessAlter née node_query_node_access_alter exists and is tested by NodeQueryAlterTest. Also, among others #3452449: Grant query level access to own/any unpublished nodes has the opposite claim. So I think more details are needed here on how to reproduce this.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Just like node grants, it only does something if you have modules implementing those query alters. It does something out of the box for commerce orders for example.

Yes, it's a problem, there are plenty of open issues about it, it's essentially about core not implementing its own access systems "for performance reasons".

davidwbarratt’s picture

davidwbarratt commented
Issue summary: View changes
Status: Postponed (maintainer needs more info) » Active

NodeHooks1::queryNodeAccessAlter née node_query_node_access_alter exists and is tested by NodeQueryAlterTest. Also, among others #3452449: Grant query level access to own/any unpublished nodes has the opposite claim. So I think more details are needed here on how to reproduce this.

The implementation you mention performs a no-op if no modules implement hook_node_grants

if (!\Drupal::moduleHandler()->hasImplementations('node_grants')) {
  return;
}

https://git.drupalcode.org/project/drupal/-/blob/401a157fc120a0883964670...

As far as I can tell, there are no modules in core that implements that hook other than in a test for node and a test for path. Therefore, node_grants only works if you have a contrib module that utilizes it, otherwise it doesn't do anything.

Regardless, this issue isn't specific to node, the same problem happens with media where all media is returned/exposed regardless of whether you have the view media permission or not, despite that being a hypothetical query alter.

My assumption in #3452449: Grant query level access to own/any unpublished nodes, based on the conversation in #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter() is that the reporter has some sort of module that is "providing context" (read: removing the permission context) to the query. As I have argued multiple times in #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter(), modules should only be restricting access, not granting additional access. Or it's possible that a contrib module isn't being granular enough when it is restricting access (i.e. the access is being restricted further than it ought to be and there is no way to undo that).

Yes, it's a problem, there are plenty of open issues about it, it's essentially about core not implementing its own access systems "for performance reasons".

That is wild to me. There are many cases (i.e. view media) where a query could simply no-op because the user doesn't have permission to view the entity in the first place. Likewise we don't have a system for adding cacheability metadata to acccess alterations on queries so node does some weird things.

davidwbarratt’s picture

davidwbarratt commented

This problem actually creates some really weird behavior like in DefaultMenuLinkTreeManipulators::checkNodeAccess where the node access query gets modified outside of the context of node

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

To be clear I agree it's a mess, but I don't think this issue will help with changing that.

For starters, It's factually wrong. What this method/API does is control whether or not the query access alter hooks are invoked or not. Whether or not you have those is an entirely different question. And that's I think is covered with existing issues.

For context, one useful resource are selection plugins, beside some entity type specific settings, the main reason they were moved into core together with the entity_reference module is access control. See \Drupal\node\Plugin\EntityReferenceSelection\NodeSelection and \Drupal\user\Plugin\EntityReferenceSelection\UserSelection for two examples.

Similar problems with views, which has custom hardcoded access plugins that don't work with some newer permissions from content moderation and so on.

The goal would essentially be to move that one layer deeper directly into entity query access. But between that and the node grants API and BC and very little interest by the general community, it's really hard to make meaningful improvements in this space.

davidwbarratt’s picture

davidwbarratt commented
Title: QueryInterface::accessCheck does nothing » QueryInterface::accessCheck does not perform access checking in core
davidwbarratt’s picture

davidwbarratt commented
Issue summary: View changes
davidwbarratt’s picture

davidwbarratt commented

I added some clarity to the title/description. You are correct that it does something it just doesn't do what I think most people expect it to do.

I'm going to take a stab and making an interface for entity query access that is at least somewhat consistent with our existing entity access patterns (for better or worse). I think there is perhaps a way to solve this problem for contrib/custom entities in 11.x and start being more strict in core in 12.x.

I agree though, there seems to be some fundamental disagreements on how access should or shouldn't work.

quietone’s picture

quietone commented
Version: 11.1.x-dev » 11.x-dev
mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented

Leaving #3516113: EntityReferenceSelection handlers does not consider entity view/view label operations here as a reference that not everything is perfect in the entity selection handlers either, or precisely probably everything works as expected but better documentation is needed about which type access check should be applied where and how...

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.