Commit 3a8edbfd authored by catch's avatar catch
Browse files

Issue #2886800 by jonathanshaw, claudiu.cristea, quietone, hctom, geek-merlin,...

Issue #2886800 by jonathanshaw, claudiu.cristea, quietone, hctom, geek-merlin, larowlan, sam152, joachim, smustgrave: EntityAccessControlHandler::createAccess() returns false positive cache hits because it ignores context

(cherry picked from commit 36dda075)
parent 8768d638
Loading
Loading
Loading
Loading
Loading
+29 −3
Original line number Diff line number Diff line
@@ -238,8 +238,8 @@ public function createAccess($entity_bundle = NULL, ?AccountInterface $account =
      'langcode' => LanguageInterface::LANGCODE_DEFAULT,
    ];

    $cid = $entity_bundle ? 'create:' . $entity_bundle : 'create';
    if (($access = $this->getCache($cid, 'create', $context['langcode'], $account)) !== NULL) {
    $cid = $this->buildCreateAccessCid($context, $entity_bundle);
    if ($cid && ($access = $this->getCache($cid, 'create', $context['langcode'], $account)) !== NULL) {
      // Cache hit, no work necessary.
      return $return_as_object ? $access : $access->isAllowed();
    }
@@ -265,7 +265,7 @@ public function createAccess($entity_bundle = NULL, ?AccountInterface $account =
    if (!$return->isForbidden()) {
      $return = $return->orIf($this->checkCreateAccess($account, $context, $entity_bundle));
    }
    $result = $this->setCache($return, $cid, 'create', $context['langcode'], $account);
    $result = $cid ? $this->setCache($return, $cid, 'create', $context['langcode'], $account) : $return;
    return $return_as_object ? $result : $result->isAllowed();
  }

@@ -403,4 +403,30 @@ protected function checkFieldAccess($operation, FieldDefinitionInterface $field_
    return AccessResult::allowed();
  }

  /**
   * Builds the create access result cache ID.
   *
   * 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
   * needed to ensure cacheability, and the default implementation here
   * returns null.
   *
   * @param array $context
   *   The create access context.
   * @param string|null $entity_bundle
   *   The entity bundle, if the entity type has bundles.
   *
   * @return string|null
   *   The create access result cache ID, or null if uncacheable.
   */
  protected function buildCreateAccessCid(array $context, ?string $entity_bundle): ?string {
    $extendedContext = array_filter($context, function ($key) {
      return !(in_array($key, ['entity_type_id', 'langcode']));
    }, ARRAY_FILTER_USE_KEY);
    if (empty($extendedContext)) {
      return $entity_bundle ? 'create:' . $entity_bundle : 'create';
    }
    return NULL;
  }

}
+10 −0
Original line number Diff line number Diff line
@@ -102,4 +102,14 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
    ], 'OR');
  }

  /**
   * {@inheritdoc}
   */
  protected function buildCreateAccessCid(array $context, ?string $entity_bundle): string {
    $cid = parent::buildCreateAccessCid([], $entity_bundle);
    $cid .= isset($context['context_var1']) ? ":{$context['context_var1']}" : '';
    $cid .= isset($context['context_var2']) ? ":{$context['context_var2']}" : '';
    return $cid;
  }

}
+301 −0
Original line number Diff line number Diff line
<?php

declare(strict_types=1);

namespace Drupal\Tests\Core\Entity\Access;

use Drupal\Core\Access\AccessResult;
use Drupal\Core\Cache\Context\CacheContextsManager;
use Drupal\Core\DependencyInjection\ContainerBuilder;
use Drupal\Core\Entity\EntityAccessControlHandler;
use Drupal\Core\Entity\EntityTypeInterface;
use Drupal\Core\Extension\ModuleHandlerInterface;
use Drupal\Core\Language\LanguageManager;
use Drupal\Core\Session\AccountInterface;
use Drupal\entity_test\EntityTestAccessControlHandler;
use Drupal\Tests\UnitTestCase;

/**
 * Tests entity access control handler custom internal cache ID.
 *
 * @coversDefaultClass \Drupal\Core\Entity\EntityAccessControlHandler
 *
 * @group Entity
 */
class EntityCreateAccessCustomCidTest extends UnitTestCase {

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

  /**
   * A mock account.
   *
   * @var \Drupal\Core\Session\AccountInterface
   */
  protected AccountInterface $account;

  /**
   * A language code.
   *
   * @var string
   */
  protected string $langcode;

  /**
   * A mock module handler.
   *
   * @var \Drupal\Core\Extension\ModuleHandlerInterface
   */
  protected ModuleHandlerInterface $moduleHandler;

  /**
   * {@inheritdoc}
   */
  protected function setUp(): void {
    parent::setUp();
    $this->entityType = $this->getMockBuilder(EntityTypeInterface::class)
      ->disableOriginalConstructor()
      ->getMock();
    $this->entityType->expects($this->any())
      ->method('id')
      ->willReturn($this->randomMachineName());

    $this->account = $this->getMockBuilder(AccountInterface::class)
      ->disableOriginalConstructor()
      ->getMock();
    $this->account->expects($this->any())
      ->method('id')
      ->willReturn(rand());

    $language_ids = array_keys(LanguageManager::getStandardLanguageList());
    $this->langcode = $language_ids[array_rand($language_ids)];

    $this->moduleHandler = $this->createMock(ModuleHandlerInterface::class);
    $this->moduleHandler->expects($this->any())
      ->method('invokeAll')
      ->willReturn([]);
  }

  /**
   * Setup the access cache on the entity handler for testing.
   *
   * @param \Drupal\Core\Entity\EntityAccessControlHandler $handler
   *   The access control handler.
   * @param bool $in_cache
   *   Whether to prefill the handler's access cache.
   * @param string $cid
   *   The cache ID.
   *
   * @return \ReflectionProperty
   *   A reflection of the handler's accessCache property.
   *
   * @throws \ReflectionException
   */
  protected function setUpAccessCache(EntityAccessControlHandler $handler, bool $in_cache, string $cid): \ReflectionProperty {
    $access_cache = new \ReflectionProperty($handler, 'accessCache');
    $access_cache->setAccessible(TRUE);

    $cache = [];
    if ($in_cache) {
      // Prefill the handler's internal static cache.
      $cache = [
        $this->account->id() => [
          $cid => [
            $this->langcode => [
              'create' => AccessResult::allowed(),
            ],
          ],
        ],
      ];
    }
    $access_cache->setValue($handler, $cache);
    return $access_cache;
  }

  /**
   * Tests the entity access control handler caching with context.
   *
   * @param array $context
   *   The context array for the test createAccess() check.
   * @param bool $in_cache
   *   Whether there is already a cached createAccess() check for the cache ID.
   * @param bool $cacheable
   *   If the test createAccess() check should be cacheable.
   *
   * @covers ::buildCreateAccessCid
   * @dataProvider providerTestDefaultCid
   */
  public function testDefaultCid(array $context, bool $in_cache, bool $cacheable): void {
    $bundle = $this->randomMachineName();
    $cid = "create:{$bundle}";
    $context['langcode'] = $this->langcode;

    $handler = new EntityAccessControlHandler($this->entityType);
    $handler->setModuleHandler($this->moduleHandler);

    $access_cache = $this->setUpAccessCache($handler, $in_cache, $cid);
    $cache = $access_cache->getValue($handler);

    // The cached value is AccessResult::allowed() but default result is
    // neutral() so createAccess returns TRUE for a cache hit, FALSE otherwise.
    $should_get_from_cache = $in_cache && $cacheable;
    $this->assertSame($should_get_from_cache, $handler->createAccess($bundle, $this->account, $context));

    $should_add_to_cache = $cacheable && !$in_cache;
    $cache_is_changed = $cache !== $access_cache->getValue($handler);
    $this->assertSame($should_add_to_cache, $cache_is_changed);
  }

  /**
   * Provides test cases for ::testDefaultCid().
   *
   * @return array[]
   *   A list of test cases.
   */
  public static function providerTestDefaultCid(): array {
    return [
      'no context, cached' => [
        'context' => [],
        'in_cache' => TRUE,
        'cacheable' => TRUE,
      ],
      'no context, uncached' => [
        'context' => [],
        'in_cache' => FALSE,
        'cacheable' => TRUE,
      ],
      'one context var, cached' => [
        'context' => [
          'context_var1' => 'val1',
        ],
        'in_cache' => TRUE,
        'cacheable' => FALSE,
      ],
      'one context var, uncached' => [
        'context' => [
          'context_var1' => 'val1',
        ],
        'in_cache' => FALSE,
        'cacheable' => FALSE,
      ],
      'two context vars, cached' => [
        'context' => [
          'context_var1' => 'val1',
          'context_var2' => 'val2',
        ],
        'in_cache' => TRUE,
        'cacheable' => FALSE,
      ],
      'two context vars, uncached' => [
        'context' => [
          'context_var1' => 'val1',
          'context_var2' => 'val2',
        ],
        'in_cache' => FALSE,
        'cacheable' => FALSE,
      ],

    ];
  }

  /**
   * Tests the entity access control handler with a custom static cache ID.
   *
   * @param string $bundle
   *   The machine name of the entity bundle.
   * @param array $context
   *   The context array.
   * @param string $cid
   *   The static cache ID.
   * @param bool $in_cache
   *   Whether there is already a cached createAccess() check for the cache ID.
   *
   * @covers ::buildCreateAccessCid
   * @dataProvider providerTestCustomCid
   */
  public function testCustomCid(string $bundle, array $context, string $cid, bool $in_cache): void {
    $context['langcode'] = $this->langcode;

    // Drupal\Core\Cache is used when merging access results in
    // checkCreateAccess(), and it calls the cache context manager service.
    $cache_context_manager = $this->createMock(CacheContextsManager::class);
    $cache_context_manager->expects($this->any())
      ->method('assertValidTokens')
      ->willReturn(TRUE);
    $container = new ContainerBuilder();
    \Drupal::setContainer($container);
    $container->set('cache_contexts_manager', $cache_context_manager);

    $handler = new EntityTestAccessControlHandler($this->entityType);
    $handler->setModuleHandler($this->moduleHandler);

    $this->setUpAccessCache($handler, $in_cache, $cid);

    // The prefilled cache is set to AccessResult::allowed(), but the default
    // for EntityTestAccessControlHandler() is neutral(); so createAccess() will
    // return TRUE for a cache hit and FALSE otherwise.
    $this->assertSame($in_cache, $handler->createAccess($bundle, $this->account, $context));
  }

  /**
   * Provides test cases for ::testCustomCid().
   *
   * @return array[]
   *   A list of test cases.
   */
  public static function providerTestCustomCid(): array {
    return [
      'no context var, in cache' => [
        'bundle' => 'bundle_1',
        'context' => [],
        'cid' => 'create:bundle_1',
        'in_cache' => TRUE,
      ],
      'no context var, not in cache' => [
        'bundle' => 'bundle_2',
        'context' => [],
        'cid' => 'create:bundle_2',
        'in_cache' => FALSE,
      ],
      'one context var, in cache' => [
        'bundle' => 'bundle_3',
        'context' => [
          'context_var1' => 'val1',
        ],
        'cid' => 'create:bundle_3:val1',
        'in_cache' => TRUE,
      ],
      'one context var, not in cache' => [
        'bundle' => 'bundle_4',
        'context' => [
          'context_var1' => 'val1',
        ],
        'cid' => 'create:bundle_4:val1',
        'in_cache' => FALSE,
      ],
      'two context vars, in cache' => [
        'bundle' => 'bundle_5',
        'context' => [
          'context_var1' => 'val1',
          'context_var2' => 'val2',
        ],
        'cid' => 'create:bundle_5:val1:val2',
        'in_cache' => TRUE,
      ],
      'two context vars, not in cache' => [
        'bundle' => 'bundle_6',
        'context' => [
          'context_var1' => 'val1',
          'context_var2' => 'val2',
        ],
        'cid' => 'create:bundle_6:val1:val2',
        'in_cache' => FALSE,
      ],
    ];
  }

}