EntityAccessControlHandler::createAccess() and EntityAccessControlHandler::access() return false positive cache hits because it ignores context

This is a valid request. I'm facing this in #2879087: Use comment access handler instead of hardcoding permissions , where I need to cache different access results based on the commented entity or even on the parent comment, in case of replies. Right now with the comment entity, regardless of $context content, the static cache location is the same.

@hctom,

What if we keep the same structure of $this->accessCache[$account->id()][$cid][$langcode][$operation] but we allow the classes extending EntityAccessControlHandler to provide their own $cid? I'm thinking on creating a new method on EntityAccessControlHandler:

protected function buildCid(?string $entity_bundle, array $context): string {
  return $entity_bundle ? 'create:' . $entity_bundle : 'create';
}

Then your class, extending EntityAccessControlHandler, will just have to extend ::buildCid() and create a custom cid.

Here's a patch based on #9. Does anybody think that this needs tests?

This blocks #2879087: Use comment access handler instead of hardcoding permissions .

This seems perfect as is. We have no current test coverage for the internals of EntityAccessControlHandler, and this is a very minor refactoring.

This looks like a good approach to me, but without any tests, it might break in the future.

Can we add an implementation to one of the test entities that verifies this works as we expect (and intend to use in #2879087: Use comment access handler instead of hardcoding permissions )

Thanks.

This might never be solved by Drupal core's default entity access control handler(s) because you never know what kind of values are passed as context and thus you are lost normalizing them for a unique cache ID.

Is there a reason that $context can't be serialized and hashed as the default cid? While it allows individual entity types to fix the bug at their own discretion, I do feel like thi does leave an open bug by default for all other entity types.

I'm guessing performance is the main consideration

@Sam152, @larowlan

Me and @hctom, we had a discussion on this. Indeed serializing the whole context would cover all cases. However, we both agreed that this could be performance cost. Context may contain arbitrary data, including entities. We probably would need to index context by keys, then serialize the whole array and, finally, convert to a hash. I'm sure this would be too expensive.

Added tests.

We could serialise if there were no objects in context, and throw an exception (or for now trigger a deprecation error) if there were. So any entity type that provided objects in context would be warned that it needed to provide custom cache id logic.

I don't understand what's the deal with that deprecation.

@jonathanshaw,

We could serialise if there were no objects in context, and throw an exception (or for now trigger a deprecation error) if there were (...)

I fact we plan to pass objects in #2879087: Use comment access handler instead of hardcoding permissions . And I think we complicate too much a minor refactoring. A deep discussion about static caching should cover all aspects, static caching is problematic in many places. Probably, in many places we better move to a memory caching service that allows also modern cache invalidation and varying by using cache metadata. I would not overcomplicate this particular issue.

A deep discussion about static caching should cover all aspects, static caching is problematic in many places. Probably, in many places we better move the to a memory caching service that allows cache metadata.

That makes sense, especially if there's a related issue we can link to.

I think we complicate too much a minor refactoring

My concern is the same as @Sam152 in #15.This isn't simply a minor refactoring task. It's filed as a bug report. And we're currently not actually fixing the bug, just enabling others to fix it more easily.

We could do trigger_error / throw() if getCacheId is called with context values other than langcode, indicating a need to implement a custom getEntityCacheId not rely on the default implementation.

We could also punt this suggestion to a bug follow up and change the present refactoring issue to a minor task. Or create a new issue for the task and postpone this bug on the task.

@jonathanshaw my main point here is that we are overdoing and trying to add some complex logic for something that doesn't add to much value. How many custom entity types would enrich the cache ID with context vars? We hit this in the other ticket, with comments. Finally, I will not touch this issue but I'll unblock #2879087: Use comment access handler instead of hardcoding permissions and move this trivial fix in the scope of that issue. Then this can be fixed as a bug, but I really don't have time resources to work on such issue. The comment ticket is blocked here and that blocks other big comment ticket #2786587: [PP-1] Add thread depth configuration to comments, both I need to my current project at European Commission.

How's this? If there is context that need to be considered for caching, then don't cache.

WIP test coverage.

self review:

+++ b/core/tests/Drupal/Tests/Core/Entity/Access/EntityCreateAccessCustomStaticCidTest.php
@@ -76,30 +193,26 @@ public function testCustomCid(int $account_id, array $context, string $cid, bool
+        'cid' => 'create:some_bundle:val1:val2',

should be bundle_2

I'd be interested in some profiling that proves fixing this properly is slow. For the vast majority of handlers that don't pass a large number of complex objects, I'm guessing it wouldn't be a huge penalty?

Tests fixed.

I opened #3180960: EntityAccessControlHandler::createAccess() and EntityAccessControlHandler::access() should use whole context array when caching as a feature request to allow caching when there is context "aka fix this properly" (per #21 or #15/#28), focusing this issue on solving the bug that the current implementation can return false positive cache hits.

Minor coding standards and test fix.

1. +++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
   @@ -382,4 +382,28 @@ protected function checkFieldAccess($operation, FieldDefinitionInterface $field_
   +    // If there is no context other than langcode and entity type id, then the
   +    // cache id can be simply the bundle. Otherwise, a custom implementation is
   

   Acronyms should be uppercased: id > ID. See https://www.drupal.org/drupalorg/style-guide/content#abbreviations

2. +++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
   @@ -382,4 +382,28 @@ protected function checkFieldAccess($operation, FieldDefinitionInterface $field_
   +    $nonLangcodeContext = array_filter($context, function ($key) {
   

   Lower camel case local variable naming are allowed by Drupal coding standards but should not be mixed with "snake case" naming. See https://www.drupal.org/docs/develop/standards/coding-standards#s-functio....

   Also the variable name doesn't reflect exactly what the variable is capturing. There is also the entity type as standard context var. Probably, $has_non_standard_context and cast the whole result to (bool).

The main problem here is that this patch is breaking the static cache when a non-standard context var is passed, even that var has no intention to participate to the cache ID value. This is clearly a regression.

The main problem here is that this patch is breaking the static cache when a non-standard context var is passed, even that var has no intention to participate to the cache ID value. This is clearly a regression.

Could you help me understand this? My thinking was: if someone is supplying context, then they must be supplying it so that it will be used in access checking. In which case we get false positive cache hits if we ignore it.

It's true that if someone is providing context unnecessarily, then the static cache will stop working for them. But that seems to me like an acceptable outcome: it's better to only use cache when we can do so safely. If you want caching, don't provide unecessary context. Providing context means "consider this context".

I reviewed all the uses of createAccess I could find in http://grep.xnddx.ru that provided a context array.

The following seem affected by this bug, and would be helped by the proposed fix.

• commerce_inventory http://grep.xnddx.ru/node/30874884 http://grep.xnddx.ru/node/30874891
• group https://git.drupalcode.org/project/group/-/blob/8.x-1.x/src/Entity/Acces...
• and therefore ggroup http://grep.xnddx.ru/node/30926131
• and group_content_menu http://grep.xnddx.ru/node/30930068
• jsonapi_access http://grep.xnddx.ru/node/30940338
• profile https://git.drupalcode.org/project/profile/-/blob/8.x-1.x/src/ProfileAcc... even though it extends UncacheableEntityAccessControlHandler

The following use context, but are not affected by this bug. They are also not affected by the fix I propose because they override createAccess() entirely with their own implementations.

• pach is already serializing context: http://grep.xnddx.ru/node/30970514
• rng is already adding cacheable dependencies instead of using static cache http://grep.xnddx.ru/node/30989598

I found no cases of a contrib module that would suffer from the regression you suggest, no cases where a contrib module provides context but does not use if for checking access.

Am I misunderstanding something?

As this is a generic class that is used for by default for different entity types, this might lead to trouble when two or more entity types have bundles of the same name. With this implementation (without the entity type ID in the cache ID), a cache ID would be returned due to the fact that different entity_type_id context values are not handled on their own.

Technically this should be a seperate issue, but it's simple to handle here as it doesn't seem to have any BC edges. Looks like our default cache ID should be create:entity_type:bundle.

This is a try with the original idea of the ticket and I've benchmarked the solution against the current implementation from HEAD. The benchmark script is included in the patch just to understand how it was tested.

Benchmark results:

$ drush php:script benchmark.php
Testing the average of 1000000 iterations:
Standard context average: 0.00019ms
Complex context average: 0.10206ms

This gives an answer to @Sam152 question from #28.

There's a mistake when creating the hash.

However, I think the benchmark shows that serialising big objects has a cost.

Re. #37

As this is a generic class that is used for by default for different entity types, this might lead to trouble when two or more entity types have bundles of the same name.

@hctom, that's not exactly true. Yes, it's the same class but it's not the same instance. Each entity type will create a separate instance because the class constructor receives the entity type. For each entity type, a new instance is created and each instance encapsulates its own static cache. There's no potential collision.

For each entity type, a new instance is created and each instance encapsulates its own static cache. There's no potential collision.

/**
   * Stores calculated access check results.
   *
   * @var array
   */
  protected $accessCache = [];

You're right, thanks for clarifying that. I'd got confused by us calling it a "static cache" when it's not actually declared static.

@claudiu.cristea what do you think in response to #36?

Re. #42. So, we're trading here to fix a security bug with the cost of losing some static caching for some projects.

Side note: There might be more modules we're breaking the internal cache, than the list from #36, as we don't know what's outside, in the wild, in custom code.

Thinking on this aspect, I'm more and more tempted by the solution from #36. What about creating an issue in each project mentioned in #36 as soon as this is RTBCed with "Postponed" status. So that we inform the maintainers that when this goes in, they will have to fix the internal caching in their project?

What about creating an issue in each project mentioned in #36

+1

as soon as this is RTBCed with "Postponed" status. So that we inform the maintainers that when this goes in, they will have to fix the internal caching in their project?

No need to postpone it. They're already have a security issue, #33 just changes it to a caching issue :)

Expanding the scope -- this affects access() as well as createAccess().

I'm seeing this problem in a kernel test where I am testing the result of requests made to a REST service:

    $http_kernel = $this->container->get('http_kernel');

    // Try to access the unpublished node.
    $request = Request::create('/endpoint/node/' . $node->id());
    $request->headers->set('Accept', 'application/json');

    $response = $http_kernel->handle($request);
    $this->assertEqual($response->getStatusCode(), Response::HTTP_OK);

My test fails because subsequent requests for the same node get the same cached access value from EntityAccessControlHandler::$accessCache.

Instead of a class property that statically caches the access values, shouldn't we inject MemoryCache instead?

The only service that uses that currently is entity.memory_cache which is used for entity storage, so we might want to define a new service so it's a separate cache bin.

But then that should take care of cached data being invalidated automatically, if we set the proper tags.

Re #46 moving away from static caching to memory caching was discussed in #21, but suggested as a separate issue, with this issue just a minor refactoring.

The current approach has been worked on for 4 years, so I don't think it counts as a minor change. It actually sounds like the current proposed fix is more complicated than switching to using memory cache, because of the problem with other modules doing unexpected things with $context.

And anyway, including the $context that's in createAccess() isn't a complete fix for this bug: A module might implement hook_entity_create_access() and return an access value that's based on something else. It would correctly add a cache tag for that dependency into the returned AccessResult, only to have that ignored by the static caching.

This is at least Major if not Critical

We had converged on consensus on the solution in #36. But #48 makes this point:

including the $context that's in createAccess() isn't a complete fix for this bug: A module might implement hook_entity_create_access() and return an access value that's based on something else. It would correctly add a cache tag for that dependency into the returned AccessResult, only to have that ignored by the static caching.

I think this is a valid concern.

It seems therefore that in order to fix the bug here, we should use MemoryCache.

I think I'm wrong above in #55.
I don't see a way to get information back from the cacheable metadata added by hooks about what createAccess() $context they used in their logic. Therefore we cannot handle context intelligently even with MemoryCache.
So we either have to serialize the whole context, which has performance concerns, or we just don't do static caching when there is context. Which is the approach in #36 we mostly all like.

This is valid regardless of whether we use MemoryCache or not in the future. $context could contain anything, and so we shouldn't automatically be stuffing it into MemoryCache either.

MemoryCache would solve the issue identified in #48, but it's not the right solution for this, so let's rule #48 out of scope here and move it to another issue.

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)