It's too easy to write entity queries with access checks that must not have them

We have similar issues with multilingual-ness it is too easy to forget about it.

And here's the example that prompted this issue... #2785155: _node_access_rebuild_batch_operation uses queries that check access

Is using one service for access check, and a different one for no access check any different to adding accessCheck(FALSE), other than you need to specifically pick with or without access check, ie there is no default.

@timmillwood
In D9 that sounds like a good idea, but for D8 we need some way of BC layer.

This bites me again and again. And others. It seems like leaving a live electric wire around for children to play with, just asking for trouble.

How about we deprecate the default value and trigger a deprecation warning if accessCheck is not specified on the query?

How about we change it from

->getQuery();

to

->getQuery($access_check = NULL) 

i.e. make it an upfront decision

And if $access_check is NULL, we default it to TRUE and trigger a warning that says creating an entity query without specifying a value for access check is deprecated in D9 and will break in D10?

Our slack conversation was:

@catch:

The deprecation would be less disruptive since existing accessCheck(FALSE) code would work fine, and it makes you think about it when you write the query.
The two services would make you decide up front, and let us document more which one is which, but seems like it would be easy to pick the wrong one/not realise the other one is there.
So overall requiring an accessCheck call on queries seems preferable, but only by a bit.

Referring to #15 I said:

As the query can technically be accessed in other ways than by getQuery() it would probably make sense to do something like the second option as well if we were doing the 3rd. i.e. require accessCheck() and have getQuery() add an accessCheck() for us if an optional boolean is provided. I'm not sure about whether encouraging getQuery(TRUE/FALSE) is a good idea though; it's convenient, but it's semantic is also opaque.

@alexpott said:

I think I favour the deprecation and making you think upfront rather than two services. I agree with the getQuery(‘OR’, TRUE) or getQuery(‘OR’, FALSE) looking odd… if only we had the new enums from PHP 8.1 :slightly_smiling_face:

@dww said:

I also think requiring the accessCheck() call is better than separate services. Easy to miss that the other service exists. But forcing authors to add the access check and have to pick a value seems like a good way forward. We should always consider access, so being required to do so is progress...

Also, if you got it wrong, easier to change your mind and s/FALSE/TRUE/ in that one spot than to change the whole service used for the query.

Overall it seems like there's a consensus for requiring the access check to be specified in some form on a single query service rather than splitting into multiple query services.

I'm hoping the test bot will identify all the places where we need to add an access check.

The fix I'm proposing is that in core/lib/Drupal/Core/Entity/Query/QueryBase.php we default the accessCheck property to null
protected $accessCheck = NULL;

and then in core/lib/Drupal/Core/Entity/Query/Sql/Query.php we make sure it's not null i.e. it's been specified as a boolean.

if (is_null($this->accessCheck)) {
      @trigger_error('Omitting to explicitly specify whether access should be checked on an entity query is deprecated in drupal:9.2.0. An error will be thrown from drupal:10.0.0. See %cr-link%', E_USER_DEPRECATED);
}

Temporarily on the MR I've got it triggering an exception no a deprecation warning, as the exception gives a stack trace on Drupal CI that is useful for running down test failures.

Residual test fails are all of one kind to do with page cache tags. I may have missed one last accessCheck() needed somewhere, but I've no idea where.

Found the culprit for the page cache fails, also fixed MakeUniqueEntityFieldTest.

We have a change record now: https://www.drupal.org/node/3201242

Thanks for the review @longwave! And especially for fixing the caching test fail.

Are we going to throw a full deprecation here, but fall back to TRUE for now?

I have now restored the deprecation warning in place of temporary debugging code as we are now green

I think technically this could be $this->any() as it doesn't matter how many times it's called, it always returns the same thing.

This is overridden with FALSE two lines below.

Fixed

Is using TRUE in the examples setting a better precedent?

I do not believe so. I think that the reason we have this whole pickle in the first place is that defaulting to TRUE is not what developers expect or normally need in an API query class; FALSE is more commonly what we want when we're doing backend code.

But it certainly doesn't matter to me and feel free to change to FALSE if you're convinced that's better.

Should this be FALSE?

I will add these cases to the follow-up #3200985: [meta] Fix undesirable access checking on entity query usages. This is a significant change to DX, and touches 100+ files, so for reasons of issue management and sanity preservation I think it's vital that in this issue we don't change the behavior of any current query.

====

I believe this is complete and now ready for RTBC. I will create a follow-up issue for 10.x to change the deprecation warning to an exception.

I created a follow-up on 10.x to go from a deprecation to an exception: #3201287: Throw an exception if accessCheck is not called on an entity query

Reviewing this closely, I suspect that the following queries involve config entities and don't need the accessCheck added in the MR:
core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
core/modules/layout_builder/src/Form/LayoutBuilderEntityViewDisplayForm.php
core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php removeSectionField
core/modules/media_library/src/Plugin/CKEditorPlugin/DrupalMediaLibrary.php
core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
core/modules/serialization/src/Normalizer/FieldableEntityNormalizerTrait.php extractBundleData

Fixed #27.

I've listed in #3200985: [meta] Fix undesirable access checking on entity query usages all the places where we are setting accessCheck(TRUE) but it should probably be FALSE. Some items of concern there.

NW for a deprecation test. After that I think this is ready for RTBC.

Thanks, looks good - RTBC if bot agrees.

Per #31 this is now postponed on #3200985: [meta] Fix undesirable access checking on entity query usages.

I'm preparing a verison 2 of the MR. First step is just a test version that throws an exception, to see if we have any outstanding queries in core where accessCheck is not specified, which we have missed from #3200985: [meta] Fix undesirable access checking on entity query usages.

I don't understand the previous fails, trying a Gitlab rebase.

TaxonomyStorage::getParents() is missing an accessCheck(), and there are maybe some more after that too.

Thanks @longwave, really appreciated. Looks like I missed TermStorage and VocabularyStorage when creating the meta. I've temporarily fixed them here, let's see where that gets us.

All blockers are committed

Let's get agreement on RTBC for this in principle regardless of the test fails, because we might be adding new entity query uses to the core codebase so fast that by the time this gets reviewed the tests will be failing again. I'm working on the test fails in child issues of #3200985: [meta] Fix undesirable access checking on entity query usages.

A single case of a missing accessCheck slipped through in EntityQueryAggregateTest, I suggest fixing it here otherwise new cases will get added to core faster than we can find and fix them.

A single case of a missing accessCheck slipped through in EntityQueryAggregateTest, I suggest fixing it here otherwise new cases will get added to core faster than we can find and fix them.

I was wrong about this. We just needed to rebase.

Polished CR as wrong interface been mentioned https://www.drupal.org/node/3201242/revisions/view/12260497/12271174

Hope this went green to RTBC!

+1 to @andypost's changes to code and CR.

Fixed last broken test

+1 to that last fix, thanks @andypost.

Changed deprecation message to "Relying on entity queries to check access by default is deprecated in drupal:9.2.0 and an error will be thrown from drupal:10.0.0. Call \Drupal\Core\Entity\Query\QueryInterface::accessCheck() with TRUE or FALSE to specify whether access should be checked. See https://www.drupal.org/node/3201242"

As agreed in Slack the new message is good, this is ready to go I think assuming bot agrees.

We have it already: #3201287: Throw an exception if accessCheck is not called on an entity query