Contrib.social

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

Open on Drupal.org →
Created on 16 June 2017, about 7 years ago
Updated 29 February 2024, 4 months ago

Problem/Motivation

Currently the EntityAccessControlHandler::createAccess() method does not take the optional $context array into account when statically caching the access result. This leads to unpredicted behaviors when you want to perform create access checks with different contexts.

For example, think of an entity that might only be created if some context parameter has a special value (e.g. the ID of a parent entity). When performing several access checks with different context values, the first call gets cached and its result is always used regardless of what the context array contains.

Current code is:

$cid = $entity_bundle ? 'create:' . $entity_bundle : 'create';
if (($access = $this->getCache($cid, 'create', $context['langcode'], $account)) !== NULL) {
  // Cache hit, no work necessary.
  return $return_as_object ? $access : $access->isAllowed();
}

As you can see, only $context['langcode'] is respected when statically caching the create access result.

The only solution right now, is to completely override the createAccess() method with custom logic for the cache ID in an access handler (if you have the chance to do so).

Proposed resolution

  • Move cache id generation into a helper method, so it is easier for inheriting classes to customise.
  • By default, skip static caching when there is context other than langcode.

Remaining tasks

  • Review patch

User interface changes

n/a

API changes

n/a

Data model changes

n/a

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
Entity  →

Last updated about 1 hour ago

Created by

🇩🇪Germany hctom

Live updates comments and jobs are added and updated live.
  • Security

    It is used for security vulnerabilities which do not need a security advisory. For example, security issues in projects which do not have security advisory coverage, or forward-porting a change already disclosed in a security advisory. See Drupal’s security advisory policy for details. Be careful publicly disclosing security vulnerabilities! Use the “Report a security vulnerability” link in the project page’s sidebar. See how to report a security issue for details.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment 4 months ago →
    🇷🇴Romania claudiu.cristea Arad 🇷🇴

    This is at least Major if not Critical

  • Comment 4 months ago →
    🇬🇧United Kingdom jonathanshaw Stroud, UK

    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.

  • Status changed to Needs review 4 months ago
  • Comment 4 months ago →
    🇬🇧United Kingdom jonathanshaw Stroud, UK

    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.

  • Status changed to Needs work 4 months ago
  • Comment 4 months ago →
    needs-review-queue-bot

    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.)

contrib.social Blog FAQ Discussions
Production build 0.69.0 2024