From 7e8d2129c43a43824325872c3268a19d8c4acb74 Mon Sep 17 00:00:00 2001 From: Alex Pott Date: Sat, 18 Apr 2015 18:26:45 +0200 Subject: [PATCH] Issue #2099137 by Wim Leers, amateescu, rteijeiro, larowlan, effulgentsia: Entity/field access and node grants not taken into account with core cache contexts --- core/lib/Drupal/Core/Access/AccessResult.php | 19 +++-- .../Core/Entity/Entity/EntityFormDisplay.php | 8 --- .../Core/Entity/Entity/EntityViewDisplay.php | 7 +- .../Entity/EntityAccessControlHandler.php | 6 +- .../Drupal/Core/Entity/EntityDisplayBase.php | 9 +++ .../Core/Entity/EntityTypeInterface.php | 2 +- .../Drupal/Core/Entity/EntityViewBuilder.php | 5 -- core/lib/Drupal/Core/Field/FormatterBase.php | 12 ++-- .../EntityReferenceFormatterBase.php | 72 ++++++++++++++++--- .../src/Tests/BlockContentCacheTagsTest.php | 13 +++- .../src/Tests/CommentCacheTagsTest.php | 12 ++++ .../EntityReferenceFormatterTest.php | 21 +++++- .../FieldFormatter/FileFormatterBase.php | 13 +++- .../FieldFormatter/RSSEnclosureFormatter.php | 1 + .../src/Tests/FilterFormatAccessTest.php | 2 +- .../node/src/Tests/Views/FrontPageTest.php | 1 - core/modules/system/entity.api.php | 1 + .../Tests/Entity/EntityCacheTagsTestBase.php | 67 ++++++++--------- .../Tests/Entity/EntityViewBuilderTest.php | 6 +- .../Entity/EntityWithUriCacheTagsTestBase.php | 8 ++- .../Field/FieldFormatter/AuthorFormatter.php | 31 +++++--- .../views/src/Plugin/views/field/Field.php | 27 ++++--- .../Tests/Core/Access/AccessResultTest.php | 31 ++++++++ 23 files changed, 266 insertions(+), 108 deletions(-) diff --git a/core/lib/Drupal/Core/Access/AccessResult.php b/core/lib/Drupal/Core/Access/AccessResult.php index 741f2ed8689..24d5051033d 100644 --- a/core/lib/Drupal/Core/Access/AccessResult.php +++ b/core/lib/Drupal/Core/Access/AccessResult.php @@ -339,13 +339,24 @@ abstract class AccessResult implements AccessResultInterface, CacheableDependenc * {@inheritdoc} */ public function orIf(AccessResultInterface $other) { + $merge_other = FALSE; // $other's cacheability metadata is merged if $merge_other gets set to TRUE - // and this happens in two cases: + // and this happens in three cases: // 1. $other's access result is the one that determines the combined access // result. // 2. This access result is not cacheable and $other's access result is the // same. i.e. attempt to return a cacheable access result. - $merge_other = FALSE; + // 3. Neither access result is 'forbidden' and both are cacheable: inherit + // the other's cacheability metadata because it may turn into a + // 'forbidden' for another value of the cache contexts in the + // cacheability metadata. In other words: this is necessary to respect + // the contagious nature of the 'forbidden' access result. + // e.g. we have two access results A and B. Neither is forbidden. A is + // globally cacheable (no cache contexts). B is cacheable per role. If we + // don't have merging case 3, then A->orIf(B) will be globally cacheable, + // which means that even if a user of a different role logs in, the + // cached access result will be used, even though for that other role, B + // is forbidden! if ($this->isForbidden() || $other->isForbidden()) { $result = static::forbidden(); if (!$this->isForbidden() || ($this->getCacheMaxAge() === 0 && $other->isForbidden())) { @@ -354,13 +365,13 @@ abstract class AccessResult implements AccessResultInterface, CacheableDependenc } elseif ($this->isAllowed() || $other->isAllowed()) { $result = static::allowed(); - if (!$this->isAllowed() || ($this->getCacheMaxAge() === 0 && $other->isAllowed())) { + if (!$this->isAllowed() || ($this->getCacheMaxAge() === 0 && $other->isAllowed()) || ($this->getCacheMaxAge() !== 0 && $other instanceof CacheableDependencyInterface && $other->getCacheMaxAge() !== 0)) { $merge_other = TRUE; } } else { $result = static::neutral(); - if (!$this->isNeutral() || ($this->getCacheMaxAge() === 0 && $other->isNeutral())) { + if (!$this->isNeutral() || ($this->getCacheMaxAge() === 0 && $other->isNeutral()) || ($this->getCacheMaxAge() !== 0 && $other instanceof CacheableDependencyInterface && $other->getCacheMaxAge() !== 0)) { $merge_other = TRUE; } } diff --git a/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php index 882286ad904..fea9d9c36cb 100644 --- a/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php @@ -33,13 +33,6 @@ class EntityFormDisplay extends EntityDisplayBase implements EntityFormDisplayIn */ protected $displayContext = 'form'; - /** - * The renderer. - * - * @var \Drupal\Core\Render\RendererInterface - */ - protected $renderer; - /** * Returns the entity_form_display object used to build an entity form. * @@ -122,7 +115,6 @@ class EntityFormDisplay extends EntityDisplayBase implements EntityFormDisplayIn */ public function __construct(array $values, $entity_type) { $this->pluginManager = \Drupal::service('plugin.manager.field.widget'); - $this->renderer = \Drupal::service('renderer'); parent::__construct($values, $entity_type); } diff --git a/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php b/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php index efffdbc03e6..6712fb57146 100644 --- a/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php +++ b/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php @@ -238,8 +238,11 @@ class EntityViewDisplay extends EntityDisplayBase implements EntityViewDisplayIn // Then let the formatter build the output for each entity. foreach ($entities as $id => $entity) { $items = $grouped_items[$id]; - $build_list[$id][$name] = $formatter->view($items); - $build_list[$id][$name]['#access'] = $items->access('view'); + /** @var \Drupal\Core\Access\AccessResultInterface $field_access */ + $field_access = $items->access('view', NULL, TRUE); + $build_list[$id][$name] = $field_access->isAllowed() ? $formatter->view($items) : []; + // Apply the field access cacheability metadata to the render array. + $this->renderer->addDependency($build_list[$id][$name], $field_access); } } } diff --git a/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php index 8a25c2aee9d..c34cffc726e 100644 --- a/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php +++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php @@ -218,9 +218,9 @@ class EntityAccessControlHandler extends EntityHandlerBase implements EntityAcce // Invoke hook_entity_create_access() and hook_ENTITY_TYPE_create_access(). // Hook results take precedence over overridden implementations of - // EntityAccessControlHandler::checkAccess(). Entities that have checks that - // need to be done before the hook is invoked should do so by overriding - // this method. + // EntityAccessControlHandler::checkCreateAccess(). Entities that have + // checks that need to be done before the hook is invoked should do so by + // overriding this method. // We grant access to the entity if both of these conditions are met: // - No modules say to deny access. diff --git a/core/lib/Drupal/Core/Entity/EntityDisplayBase.php b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php index 39e953fb877..4636c2a28c7 100644 --- a/core/lib/Drupal/Core/Entity/EntityDisplayBase.php +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php @@ -113,6 +113,13 @@ abstract class EntityDisplayBase extends ConfigEntityBase implements EntityDispl */ protected $pluginManager; + /** + * The renderer. + * + * @var \Drupal\Core\Render\RendererInterface + */ + protected $renderer; + /** * {@inheritdoc} */ @@ -125,6 +132,8 @@ abstract class EntityDisplayBase extends ConfigEntityBase implements EntityDispl throw new \InvalidArgumentException('EntityDisplay entities can only handle fieldable entity types.'); } + $this->renderer = \Drupal::service('renderer'); + // A plugin manager and a context type needs to be set by extending classes. if (!isset($this->pluginManager)) { throw new \RuntimeException('Missing plugin manager.'); diff --git a/core/lib/Drupal/Core/Entity/EntityTypeInterface.php b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php index 6b68b01b08f..33552cdfb2f 100644 --- a/core/lib/Drupal/Core/Entity/EntityTypeInterface.php +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php @@ -660,7 +660,7 @@ interface EntityTypeInterface { * * Enables code listing entities of this type to ensure that rendered listings * are varied as necessary, typically to ensure users of role A see other - * entities listed as users of role B. + * entities listed than users of role B. * * @return string[] */ diff --git a/core/lib/Drupal/Core/Entity/EntityViewBuilder.php b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php index 13a7b506254..6d2caf7de0e 100644 --- a/core/lib/Drupal/Core/Entity/EntityViewBuilder.php +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php @@ -174,11 +174,6 @@ class EntityViewBuilder extends EntityHandlerBase implements EntityHandlerInterf ), ); - // @todo Remove when https://www.drupal.org/node/2099137 lands. - $build['#cache']['contexts'] = Cache::mergeContexts($build['#cache']['contexts'], [ - 'user.roles', - ]); - // Cache the rendered output if permitted by the view mode and global entity // type configuration. if ($this->isViewModeCacheable($view_mode) && !$entity->isNew() && $entity->isDefaultRevision() && $this->entityType->isRenderCacheable()) { diff --git a/core/lib/Drupal/Core/Field/FormatterBase.php b/core/lib/Drupal/Core/Field/FormatterBase.php index f7df6013479..a6c8cc8caea 100644 --- a/core/lib/Drupal/Core/Field/FormatterBase.php +++ b/core/lib/Drupal/Core/Field/FormatterBase.php @@ -8,6 +8,7 @@ namespace Drupal\Core\Field; use Drupal\Core\Form\FormStateInterface; +use Drupal\Core\Render\Element; /** * Base class for 'Field formatter' plugin implementations. @@ -76,10 +77,11 @@ abstract class FormatterBase extends PluginSettingsBase implements FormatterInte * {@inheritdoc} */ public function view(FieldItemListInterface $items) { - $addition = array(); - $elements = $this->viewElements($items); - if ($elements) { + + // If there are actual renderable children, use #theme => field, otherwise, + // let access cacheability metadata pass through for correct bubbling. + if (Element::children($elements)) { $entity = $items->getEntity(); $entity_type = $entity->getEntityTypeId(); $field_name = $this->fieldDefinition->getName(); @@ -99,10 +101,10 @@ abstract class FormatterBase extends PluginSettingsBase implements FormatterInte '#formatter' => $this->getPluginId(), ); - $addition = array_merge($info, $elements); + $elements = array_merge($info, $elements); } - return $addition; + return $elements; } /** diff --git a/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFormatterBase.php b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFormatterBase.php index 25c802c8fe6..351d51c083a 100644 --- a/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFormatterBase.php +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFormatterBase.php @@ -7,9 +7,12 @@ namespace Drupal\Core\Field\Plugin\Field\FieldFormatter; +use Drupal\Core\Entity\EntityInterface; use Drupal\Core\Field\EntityReferenceFieldItemListInterface; +use Drupal\Core\Field\FieldItemListInterface; use Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem; use Drupal\Core\Field\FormatterBase; +use Drupal\Core\Render\BubbleableMetadata; use Drupal\Core\TypedData\TranslatableInterface; /** @@ -36,6 +39,8 @@ abstract class EntityReferenceFormatterBase extends FormatterBase { * * @return \Drupal\Core\Entity\EntityInterface[] * The array of referenced entities to display, keyed by delta. + * + * @see ::prepareView() */ protected function getEntitiesToView(EntityReferenceFieldItemListInterface $items) { $entities = array(); @@ -51,8 +56,10 @@ abstract class EntityReferenceFormatterBase extends FormatterBase { $entity = $entity->getTranslation($parent_entity_langcode); } - // Check entity access if needed. - if (!$this->needsAccessCheck($item) || $entity->access('view')) { + $access = $this->checkAccess($entity); + // Add the access result's cacheability, ::view() needs it. + $item->_accessCacheability = BubbleableMetadata::createFromObject($access); + if ($access->isAllowed()) { // Add the referring item, in case the formatter needs it. $entity->_referringItem = $items[$delta]; $entities[$delta] = $entity; @@ -63,6 +70,49 @@ abstract class EntityReferenceFormatterBase extends FormatterBase { return $entities; } + /** + * {@inheritdoc} + * + * @see ::prepareView() + * @see ::getEntitiestoView() + */ + public function view(FieldItemListInterface $items) { + $elements = parent::view($items); + + $field_level_access_cacheability = new BubbleableMetadata(); + + // Try to map the cacheability of the access result that was set at + // _accessCacheability in getEntitiesToView() to the corresponding render + // subtree. If no such subtree is found, then merge it with the field-level + // access cacheability. + foreach ($items as $delta => $item) { + // Ignore items for which access cacheability could not be determined in + // prepareView(). + if (!empty($item->_accessCacheability)) { + if (isset($elements[$delta])) { + BubbleableMetadata::createFromRenderArray($elements[$delta]) + ->merge($item->_accessCacheability) + ->applyTo($elements[$delta]); + } + else { + $field_level_access_cacheability = $field_level_access_cacheability->merge($item->_accessCacheability); + } + } + } + + // Apply the cacheability metadata for the inaccessible entities and the + // entities for which the corresponding render subtree could not be found. + // This causes the field to be rendered (and cached) according to the cache + // contexts by which the access results vary, to ensure only users with + // access to this field can view it. It also tags this field with the cache + // tags on which the access results depend, to ensure users that cannot view + // this field at the moment will gain access once any of those cache tags + // are invalidated. + $field_level_access_cacheability->applyTo($elements); + + return $elements; + } + /** * {@inheritdoc} * @@ -121,16 +171,20 @@ abstract class EntityReferenceFormatterBase extends FormatterBase { } /** - * Returns whether entity access should be checked. + * Checks access to the given entity. * - * @param \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem $item - * The item to check. + * By default, entity access is checked. However, a subclass can choose to + * exclude certain items from entity access checking by immediately granting + * access. * - * @return bool - * TRUE if entity access should be checked. + * @param \Drupal\Core\Entity\EntityInterface $entity + * The entity to check. + * + * @return \Drupal\Core\Access\AccessResult + * A cacheable access result. */ - protected function needsAccessCheck(EntityReferenceItem $item) { - return TRUE; + protected function checkAccess(EntityInterface $entity) { + return $entity->access('view', NULL, TRUE); } } diff --git a/core/modules/block_content/src/Tests/BlockContentCacheTagsTest.php b/core/modules/block_content/src/Tests/BlockContentCacheTagsTest.php index 8fb80d5da01..582d8328736 100644 --- a/core/modules/block_content/src/Tests/BlockContentCacheTagsTest.php +++ b/core/modules/block_content/src/Tests/BlockContentCacheTagsTest.php @@ -52,6 +52,15 @@ class BlockContentCacheTagsTest extends EntityCacheTagsTestBase { return $block_content; } + /** + * {@inheritdoc} + * + * @see \Drupal\block_content\BlockContentAccessControlHandler::checkAccess() + */ + protected function getAccessCacheContextsForEntity(EntityInterface $entity) { + return []; + } + /** * {@inheritdoc} * @@ -86,12 +95,12 @@ class BlockContentCacheTagsTest extends EntityCacheTagsTestBase { // Expected contexts and tags for the BlockContent entity. // @see \Drupal\Core\Entity\EntityViewBuilder::getBuildDefaults(). - $expected_entity_cache_contexts = ['theme', 'user.roles']; + $expected_entity_cache_contexts = ['theme']; $expected_entity_cache_tags = Cache::mergeTags(['block_content_view'], $this->entity->getCacheTags(), $this->getAdditionalCacheTagsForEntity($this->entity)); // Verify that what was render cached matches the above expectations. $cid = $this->createCacheId($expected_block_cache_keys, $expected_block_cache_contexts); $redirected_cid = $this->createCacheId($expected_block_cache_keys, Cache::mergeContexts($expected_block_cache_contexts, $expected_entity_cache_contexts)); - $this->verifyRenderCache($cid, Cache::mergeTags($expected_block_cache_tags, $expected_entity_cache_tags), $redirected_cid); + $this->verifyRenderCache($cid, Cache::mergeTags($expected_block_cache_tags, $expected_entity_cache_tags), ($cid !== $redirected_cid) ? $redirected_cid : NULL); } } diff --git a/core/modules/comment/src/Tests/CommentCacheTagsTest.php b/core/modules/comment/src/Tests/CommentCacheTagsTest.php index fa95260c92c..04e0848b9ba 100644 --- a/core/modules/comment/src/Tests/CommentCacheTagsTest.php +++ b/core/modules/comment/src/Tests/CommentCacheTagsTest.php @@ -81,6 +81,18 @@ class CommentCacheTagsTest extends EntityWithUriCacheTagsTestBase { return $comment; } + + /** + * {@inheritdoc} + */ + protected function getAdditionalCacheContextsForEntity(EntityInterface $entity) { + return [ + // Field access for the user picture rendered as part of the node that + // this comment is created on. + 'user.permissions', + ]; + } + /** * {@inheritdoc} * diff --git a/core/modules/field/src/Tests/EntityReference/EntityReferenceFormatterTest.php b/core/modules/field/src/Tests/EntityReference/EntityReferenceFormatterTest.php index 9979c0d544b..7bd22f7727f 100644 --- a/core/modules/field/src/Tests/EntityReference/EntityReferenceFormatterTest.php +++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceFormatterTest.php @@ -10,6 +10,7 @@ namespace Drupal\field\Tests\EntityReference; use Drupal\Core\Cache\Cache; use Drupal\Core\Field\FieldStorageDefinitionInterface; use Drupal\entity_reference\Tests\EntityReferenceTestTrait; +use Drupal\Core\Render\BubbleableMetadata; use Drupal\field\Entity\FieldStorageConfig; use Drupal\filter\Entity\FilterFormat; use Drupal\system\Tests\Entity\EntityUnitTestBase; @@ -218,26 +219,44 @@ class EntityReferenceFormatterTest extends EntityUnitTestBase { // The 'link' settings is TRUE by default. $build = $this->buildRenderArray([$this->referencedEntity, $this->unsavedReferencedEntity], $formatter); + $expected_field_cacheability = [ + 'contexts' => [], + 'tags' => [], + 'max-age' => Cache::PERMANENT, + ]; + $this->assertEqual($build['#cache'], $expected_field_cacheability, 'The field render array contains the entity access cacheability metadata'); $expected_item_1 = array( '#type' => 'link', '#title' => $this->referencedEntity->label(), '#url' => $this->referencedEntity->urlInfo(), '#options' => $this->referencedEntity->urlInfo()->getOptions(), '#cache' => array( + 'contexts' => [ + 'user.permissions', + ], 'tags' => $this->referencedEntity->getCacheTags(), ), + '#attached' => [], + '#post_render_cache' => [], ); $this->assertEqual(drupal_render($build[0]), drupal_render($expected_item_1), sprintf('The markup returned by the %s formatter is correct for an item with a saved entity.', $formatter)); + $this->assertEqual(BubbleableMetadata::createFromRenderArray($build[0]), BubbleableMetadata::createFromRenderArray($expected_item_1)); // The second referenced entity is "autocreated", therefore not saved and // lacking any URL info. $expected_item_2 = array( '#markup' => $this->unsavedReferencedEntity->label(), '#cache' => array( + 'contexts' => [ + 'user.permissions', + ], 'tags' => $this->unsavedReferencedEntity->getCacheTags(), + 'max-age' => Cache::PERMANENT, ), + '#attached' => [], + '#post_render_cache' => [], ); - $this->assertEqual($build[1], $expected_item_2, sprintf('The markup returned by the %s formatter is correct for an item with a unsaved entity.', $formatter)); + $this->assertEqual($build[1], $expected_item_2, sprintf('The render array returned by the %s formatter is correct for an item with a unsaved entity.', $formatter)); // Test with the 'link' setting set to FALSE. $build = $this->buildRenderArray([$this->referencedEntity, $this->unsavedReferencedEntity], $formatter, array('link' => FALSE)); diff --git a/core/modules/file/src/Plugin/Field/FieldFormatter/FileFormatterBase.php b/core/modules/file/src/Plugin/Field/FieldFormatter/FileFormatterBase.php index cdd5b7ba9cf..9cbe807d7c4 100644 --- a/core/modules/file/src/Plugin/Field/FieldFormatter/FileFormatterBase.php +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileFormatterBase.php @@ -7,6 +7,8 @@ namespace Drupal\file\Plugin\Field\FieldFormatter; +use Drupal\Core\Access\AccessResult; +use Drupal\Core\Entity\EntityInterface; use Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceFormatterBase; use Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem; @@ -25,11 +27,16 @@ abstract class FileFormatterBase extends EntityReferenceFormatterBase { /** * {@inheritdoc} */ - protected function needsAccessCheck(EntityReferenceItem $item) { + protected function checkAccess(EntityInterface $entity) { // Only check access if the current file access control handler explicitly // opts in by implementing FileAccessFormatterControlHandlerInterface. - $access_handler_class = $item->entity->getEntityType()->getHandlerClass('access'); - return is_subclass_of($access_handler_class, '\Drupal\file\FileAccessFormatterControlHandlerInterface'); + $access_handler_class = $entity->getEntityType()->getHandlerClass('access'); + if (is_subclass_of($access_handler_class, '\Drupal\file\FileAccessFormatterControlHandlerInterface')) { + return $entity->access('view', NULL, TRUE); + } + else { + return AccessResult::allowed(); + } } } diff --git a/core/modules/file/src/Plugin/Field/FieldFormatter/RSSEnclosureFormatter.php b/core/modules/file/src/Plugin/Field/FieldFormatter/RSSEnclosureFormatter.php index c3247022cc8..2706e2fc5c8 100644 --- a/core/modules/file/src/Plugin/Field/FieldFormatter/RSSEnclosureFormatter.php +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/RSSEnclosureFormatter.php @@ -39,6 +39,7 @@ class RSSEnclosureFormatter extends FileFormatterBase { ), ); } + return []; } } diff --git a/core/modules/filter/src/Tests/FilterFormatAccessTest.php b/core/modules/filter/src/Tests/FilterFormatAccessTest.php index 8dfe449db40..32d5d041226 100644 --- a/core/modules/filter/src/Tests/FilterFormatAccessTest.php +++ b/core/modules/filter/src/Tests/FilterFormatAccessTest.php @@ -126,7 +126,7 @@ class FilterFormatAccessTest extends WebTestBase { $this->assertTrue($this->allowedFormat->access('use', $this->webUser), 'A regular user has access to use a text format they were granted access to.'); $this->assertEqual(AccessResult::allowed()->addCacheContexts(['user.permissions']), $this->allowedFormat->access('use', $this->webUser, TRUE), 'A regular user has access to use a text format they were granted access to.'); $this->assertFalse($this->disallowedFormat->access('use', $this->webUser), 'A regular user does not have access to use a text format they were not granted access to.'); - $this->assertEqual(AccessResult::neutral(), $this->disallowedFormat->access('use', $this->webUser, TRUE)); //, 'A regular user does not have access to use a text format they were not granted access to.'); + $this->assertEqual(AccessResult::neutral()->cachePerPermissions(), $this->disallowedFormat->access('use', $this->webUser, TRUE), 'A regular user does not have access to use a text format they were not granted access to.'); $this->assertTrue($fallback_format->access('use', $this->webUser), 'A regular user has access to use the fallback format.'); $this->assertEqual(AccessResult::allowed(), $fallback_format->access('use', $this->webUser, TRUE), 'A regular user has access to use the fallback format.'); diff --git a/core/modules/node/src/Tests/Views/FrontPageTest.php b/core/modules/node/src/Tests/Views/FrontPageTest.php index 0e2efbf609c..83e54b00fea 100644 --- a/core/modules/node/src/Tests/Views/FrontPageTest.php +++ b/core/modules/node/src/Tests/Views/FrontPageTest.php @@ -290,7 +290,6 @@ class FrontPageTest extends ViewTestBase { } $cache_contexts = Cache::mergeContexts($cache_contexts, [ 'timezone', - 'user.roles' ]); // First page. diff --git a/core/modules/system/entity.api.php b/core/modules/system/entity.api.php index d188375a87b..9d9b33eb511 100644 --- a/core/modules/system/entity.api.php +++ b/core/modules/system/entity.api.php @@ -1844,6 +1844,7 @@ function hook_entity_field_access($operation, \Drupal\Core\Field\FieldDefinition if ($field_definition->getName() == 'field_of_interest' && $operation == 'edit') { return AccessResult::allowedIfHasPermission($account, 'update field of interest'); } + return AccessResult::neutral(); } /** diff --git a/core/modules/system/src/Tests/Entity/EntityCacheTagsTestBase.php b/core/modules/system/src/Tests/Entity/EntityCacheTagsTestBase.php index 91f98b3e90f..0b365cbab06 100644 --- a/core/modules/system/src/Tests/Entity/EntityCacheTagsTestBase.php +++ b/core/modules/system/src/Tests/Entity/EntityCacheTagsTestBase.php @@ -129,6 +129,21 @@ abstract class EntityCacheTagsTestBase extends PageCacheTagsTestBase { */ abstract protected function createEntity(); + /** + * Returns the access cache contexts for the tested entity. + * + * @param \Drupal\Core\Entity\EntityInterface $entity + * The entity to be tested, as created by createEntity(). + * + * @return string[] + * An array of the additional cache contexts. + * + * @see \Drupal\Core\Entity\EntityAccessControlHandlerInterface + */ + protected function getAccessCacheContextsForEntity(EntityInterface $entity) { + return ['user.permissions']; + } + /** * Returns the additional (non-standard) cache contexts for the tested entity. * @@ -317,7 +332,7 @@ abstract class EntityCacheTagsTestBase extends PageCacheTagsTestBase { // The default cache contexts for rendered entities. $default_cache_contexts = ['languages:' . LanguageInterface::TYPE_INTERFACE, 'theme']; - $entity_cache_contexts = Cache::mergeContexts($default_cache_contexts, ['user.roles']); + $entity_cache_contexts = $default_cache_contexts; // Cache tags present on every rendered page. $page_cache_tags = Cache::mergeTags( @@ -326,6 +341,7 @@ abstract class EntityCacheTagsTestBase extends PageCacheTagsTestBase { // which adds the block config entity type's list cache tags. \Drupal::moduleHandler()->moduleExists('block') ? ['config:block_list']: [] ); + $page_cache_tags_referencing_entity = in_array('user.permissions', $this->getAccessCacheContextsForEntity($this->referencingEntity)) ? ['config:user.role.anonymous'] : []; $view_cache_tag = array(); if ($this->entity->getEntityType()->hasHandlerClass('view_builder')) { @@ -366,11 +382,15 @@ abstract class EntityCacheTagsTestBase extends PageCacheTagsTestBase { $this->pass("Test referencing entity.", 'Debug'); $this->verifyPageCache($referencing_entity_url, 'MISS'); // Verify a cache hit, but also the presence of the correct cache tags. - $this->verifyPageCache($referencing_entity_url, 'HIT', Cache::mergeTags($referencing_entity_cache_tags, $page_cache_tags)); + $this->verifyPageCache($referencing_entity_url, 'HIT', Cache::mergeTags($referencing_entity_cache_tags, $page_cache_tags, $page_cache_tags_referencing_entity)); // Also verify the existence of an entity render cache entry. $cache_keys = ['entity_view', 'entity_test', $this->referencingEntity->id(), 'full']; $cid = $this->createCacheId($cache_keys, $entity_cache_contexts); - $redirected_cid = $this->createRedirectedCacheId($cache_keys, $entity_cache_contexts); + $access_cache_contexts = $this->getAccessCacheContextsForEntity($this->entity); + $redirected_cid = NULL; + if (count($access_cache_contexts)) { + $redirected_cid = $this->createCacheId($cache_keys, Cache::mergeContexts($entity_cache_contexts, $this->getAdditionalCacheContextsForEntity($this->referencingEntity), $access_cache_contexts)); + } $this->verifyRenderCache($cid, $referencing_entity_cache_tags, $redirected_cid); $this->pass("Test non-referencing entity.", 'Debug'); @@ -387,7 +407,7 @@ abstract class EntityCacheTagsTestBase extends PageCacheTagsTestBase { // Prime the page cache for the listing of referencing entities. $this->verifyPageCache($listing_url, 'MISS'); // Verify a cache hit, but also the presence of the correct cache tags. - $this->verifyPageCache($listing_url, 'HIT', Cache::mergeTags($referencing_entity_cache_tags, $page_cache_tags)); + $this->verifyPageCache($listing_url, 'HIT', Cache::mergeTags($referencing_entity_cache_tags, $page_cache_tags, $page_cache_tags_referencing_entity)); $this->pass("Test empty listing.", 'Debug'); @@ -638,32 +658,6 @@ abstract class EntityCacheTagsTestBase extends PageCacheTagsTestBase { return implode(':', $cid_parts); } - /** - * Creates the redirected cache ID, if any. - * - * If a subclass overrides ::getAdditionalCacheContextsForEntity(), it can - * specify the additional cache contexts by which the given entity must be - * varied, because those are the cache contexts that are bubbled from the - * field formatters. - * - * @param string[] $keys - * A list of cache keys used for the regular (pre-bubbling) CID. - * @param string[] $contexts - * A set of cache contexts used for the regular (pre-bubbling) CID. - * - * @return string|null - * The redirected (post-bubbling) CID, if any. - */ - protected function createRedirectedCacheId(array $keys, array $contexts) { - $additional_cache_contexts = $this->getAdditionalCacheContextsForEntity($this->referencingEntity); - if (count($additional_cache_contexts)) { - return $this->createCacheId($keys, Cache::mergeContexts($contexts, $additional_cache_contexts)); - } - else { - return NULL; - } - } - /** * Verify that a given render cache entry exists, with the correct cache tags. * @@ -681,12 +675,21 @@ abstract class EntityCacheTagsTestBase extends PageCacheTagsTestBase { sort($cache_entry->tags); sort($tags); $this->assertIdentical($cache_entry->tags, $tags); + $is_redirecting_cache_item = isset($cache_entry->data['#cache_redirect']); if ($redirected_cid === NULL) { - $this->assertTrue(!isset($cache_entry->data['#cache_redirect']), 'Render cache entry is not a redirect.'); + $this->assertFalse($is_redirecting_cache_item, 'Render cache entry is not a redirect.'); + // If this is a redirecting cache item unlike we expected, log it. + if ($is_redirecting_cache_item) { + debug($cache_entry->data); + } } else { // Verify that $cid contains a cache redirect. - $this->assertTrue(isset($cache_entry->data['#cache_redirect']), 'Render cache entry is a redirect.'); + $this->assertTrue($is_redirecting_cache_item, 'Render cache entry is a redirect.'); + // If this is not a redirecting cache item unlike we expected, log it. + if (!$is_redirecting_cache_item) { + debug($cache_entry->data); + } // Verify that the cache redirect points to the expected CID. $redirect_cache_metadata = $cache_entry->data['#cache']; $actual_redirection_cid = $this->createCacheId( diff --git a/core/modules/system/src/Tests/Entity/EntityViewBuilderTest.php b/core/modules/system/src/Tests/Entity/EntityViewBuilderTest.php index dd275cbb094..333dcda6803 100644 --- a/core/modules/system/src/Tests/Entity/EntityViewBuilderTest.php +++ b/core/modules/system/src/Tests/Entity/EntityViewBuilderTest.php @@ -63,7 +63,7 @@ class EntityViewBuilderTest extends EntityUnitTestBase { // Get a fully built entity view render array. $entity_test->save(); $build = $this->container->get('entity.manager')->getViewBuilder('entity_test')->view($entity_test, 'full'); - $cid_parts = array_merge($build['#cache']['keys'], $cache_contexts_manager->convertTokensToKeys(Cache::mergeContexts($build['#cache']['contexts'], ['languages:' . LanguageInterface::TYPE_INTERFACE, 'theme']))); + $cid_parts = array_merge($build['#cache']['keys'], $cache_contexts_manager->convertTokensToKeys(['languages:' . LanguageInterface::TYPE_INTERFACE, 'theme'])); $cid = implode(':', $cid_parts); $bin = $build['#cache']['bin']; @@ -113,7 +113,7 @@ class EntityViewBuilderTest extends EntityUnitTestBase { // Get a fully built entity view render array for the referenced entity. $build = $this->container->get('entity.manager')->getViewBuilder('entity_test')->view($entity_test_reference, 'full'); - $cid_parts = array_merge($build['#cache']['keys'], $cache_contexts_manager->convertTokensToKeys(Cache::mergeContexts($build['#cache']['contexts'], ['languages:' . LanguageInterface::TYPE_INTERFACE, 'theme']))); + $cid_parts = array_merge($build['#cache']['keys'], $cache_contexts_manager->convertTokensToKeys(['languages:' . LanguageInterface::TYPE_INTERFACE, 'theme'])); $cid_reference = implode(':', $cid_parts); $bin_reference = $build['#cache']['bin']; @@ -132,7 +132,7 @@ class EntityViewBuilderTest extends EntityUnitTestBase { // Get a fully built entity view render array. $build = $this->container->get('entity.manager')->getViewBuilder('entity_test')->view($entity_test, 'full'); - $cid_parts = array_merge($build['#cache']['keys'], $cache_contexts_manager->convertTokensToKeys(Cache::mergeContexts($build['#cache']['contexts'], ['languages:' . LanguageInterface::TYPE_INTERFACE, 'theme']))); + $cid_parts = array_merge($build['#cache']['keys'], $cache_contexts_manager->convertTokensToKeys(['languages:' . LanguageInterface::TYPE_INTERFACE, 'theme'])); $cid = implode(':', $cid_parts); $bin = $build['#cache']['bin']; diff --git a/core/modules/system/src/Tests/Entity/EntityWithUriCacheTagsTestBase.php b/core/modules/system/src/Tests/Entity/EntityWithUriCacheTagsTestBase.php index 299f8111277..0b6f4301e38 100644 --- a/core/modules/system/src/Tests/Entity/EntityWithUriCacheTagsTestBase.php +++ b/core/modules/system/src/Tests/Entity/EntityWithUriCacheTagsTestBase.php @@ -32,7 +32,7 @@ abstract class EntityWithUriCacheTagsTestBase extends EntityCacheTagsTestBase { $view_mode = $this->selectViewMode($entity_type); // The default cache contexts for rendered entities. - $entity_cache_contexts = ['languages:' . LanguageInterface::TYPE_INTERFACE, 'theme', 'user.roles']; + $entity_cache_contexts = ['languages:' . LanguageInterface::TYPE_INTERFACE, 'theme']; // Generate the standardized entity cache tags. $cache_tag = $this->entity->getCacheTags(); @@ -51,7 +51,11 @@ abstract class EntityWithUriCacheTagsTestBase extends EntityCacheTagsTestBase { if (\Drupal::entityManager()->getDefinition($entity_type)->isRenderCacheable()) { $cache_keys = ['entity_view', $entity_type, $this->entity->id(), $view_mode]; $cid = $this->createCacheId($cache_keys, $entity_cache_contexts); - $redirected_cid = $this->createRedirectedCacheId($cache_keys, $entity_cache_contexts); + $redirected_cid = NULL; + $additional_cache_contexts = $this->getAdditionalCacheContextsForEntity($this->entity); + if (count($additional_cache_contexts)) { + $redirected_cid = $this->createCacheId($cache_keys, Cache::mergeContexts($entity_cache_contexts, $additional_cache_contexts)); + } $expected_cache_tags = Cache::mergeTags($cache_tag, $view_cache_tag, $this->getAdditionalCacheTagsForEntity($this->entity), array($render_cache_tag)); $this->verifyRenderCache($cid, $expected_cache_tags, $redirected_cid); } diff --git a/core/modules/user/src/Plugin/Field/FieldFormatter/AuthorFormatter.php b/core/modules/user/src/Plugin/Field/FieldFormatter/AuthorFormatter.php index 328f5135fea..c880066f94a 100644 --- a/core/modules/user/src/Plugin/Field/FieldFormatter/AuthorFormatter.php +++ b/core/modules/user/src/Plugin/Field/FieldFormatter/AuthorFormatter.php @@ -7,6 +7,8 @@ namespace Drupal\user\Plugin\Field\FieldFormatter; +use Drupal\Core\Access\AccessResult; +use Drupal\Core\Entity\EntityInterface; use Drupal\Core\Field\FieldDefinitionInterface; use Drupal\Core\Field\FieldItemListInterface; use Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceFormatterBase; @@ -31,18 +33,16 @@ class AuthorFormatter extends EntityReferenceFormatterBase { public function viewElements(FieldItemListInterface $items) { $elements = array(); - foreach ($items as $delta => $item) { + foreach ($this->getEntitiesToView($items) as $delta => $entity) { /** @var $referenced_user \Drupal\user\UserInterface */ - if ($referenced_user = $item->entity) { - $elements[$delta] = array( - '#theme' => 'username', - '#account' => $referenced_user, - '#link_options' => array('attributes' => array('rel' => 'author')), - '#cache' => array( - 'tags' => $referenced_user->getCacheTags(), - ), - ); - } + $elements[$delta] = array( + '#theme' => 'username', + '#account' => $entity, + '#link_options' => array('attributes' => array('rel' => 'author')), + '#cache' => array( + 'tags' => $entity->getCacheTags(), + ), + ); } return $elements; @@ -55,4 +55,13 @@ class AuthorFormatter extends EntityReferenceFormatterBase { return $field_definition->getFieldStorageDefinition()->getSetting('target_type') == 'user'; } + /** + * {@inheritdoc} + */ + protected function checkAccess(EntityInterface $entity) { + // Always allow an entity author's username to be read, even if the current + // user does not have permission to view the entity author's profile. + return AccessResult::allowed(); + } + } diff --git a/core/modules/views/src/Plugin/views/field/Field.php b/core/modules/views/src/Plugin/views/field/Field.php index 66dbdeb0ba9..439d83533c8 100644 --- a/core/modules/views/src/Plugin/views/field/Field.php +++ b/core/modules/views/src/Plugin/views/field/Field.php @@ -18,6 +18,7 @@ use Drupal\Core\Form\FormHelper; use Drupal\Core\Form\FormStateInterface; use Drupal\Core\Language\LanguageInterface; use Drupal\Core\Language\LanguageManagerInterface; +use Drupal\Core\Render\BubbleableMetadata; use Drupal\Core\Render\Element; use Drupal\Core\Render\RendererInterface; use Drupal\Core\Session\AccountInterface; @@ -731,26 +732,22 @@ class Field extends FieldPluginBase implements CacheablePluginInterface, MultiIt ); $render_array = $entity->get($this->definition['field_name'])->view($display); - $items = array(); if ($this->options['field_api_classes']) { return array(array('rendered' => $this->renderer->render($render_array))); } - foreach (Element::children($render_array) as $count) { - $items[$count]['rendered'] = $render_array[$count]; - // FieldItemListInterface::view() adds an #access property to the render - // array that determines whether or not the current user is allowed to - // view the field in the context of the current entity. We need to respect - // this parameter when we pull out the children of the field array for - // rendering. - if (isset($render_array['#access'])) { - $items[$count]['rendered']['#access'] = $render_array['#access']; - } - // Only add the raw field items (for use in tokens) if the current user - // has access to view the field content. - if ((!isset($items[$count]['rendered']['#access']) || $items[$count]['rendered']['#access']) && !empty($render_array['#items'][$count])) { - $items[$count]['raw'] = $render_array['#items'][$count]; + $items = array(); + foreach (Element::children($render_array) as $delta) { + $items[$delta]['rendered'] = $render_array[$delta]; + // Merge the cacheability metadata of the top-level render array into + // each child because they will most likely be rendered individually. + if (isset($render_array['#cache'])) { + BubbleableMetadata::createFromRenderArray($render_array) + ->merge(BubbleableMetadata::createFromRenderArray($items[$delta]['rendered'])) + ->applyTo($items[$delta]['rendered']); } + // Add the raw field items (for use in tokens). + $items[$delta]['raw'] = $render_array['#items'][$delta]; } return $items; } diff --git a/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php index 43ae63a49ff..123888aaeed 100644 --- a/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php @@ -796,6 +796,37 @@ class AccessResultTest extends UnitTestCase { } } + /** + * @covers ::orIf + * + * Tests the special case of ORing non-forbidden access results that are both + * cacheable but have different cacheability metadata. + * This is only the case for non-forbidden access results; we still abort the + * ORing process as soon as a forbidden access result is encountered. This is + * tested in ::testOrIf(). + */ + public function testOrIfCacheabilityMerging() { + $merge_both_directions = function(AccessResult $a, AccessResult $b) { + // A globally cacheable access result. + $a->setCacheMaxAge(3600); + // Another access result that is cacheable per permissions. + $b->setCacheMaxAge(86400)->cachePerPermissions(); + + $r1 = $a->orIf($b); + $this->assertTrue($r1->getCacheMaxAge() === 3600); + $this->assertSame(['user.permissions'], $r1->getCacheContexts()); + $r2 = $b->orIf($a); + $this->assertTrue($r2->getCacheMaxAge() === 3600); + $this->assertSame(['user.permissions'], $r2->getCacheContexts()); + }; + + // Merge either direction, get the same result. + $merge_both_directions(AccessResult::allowed(), AccessResult::allowed()); + $merge_both_directions(AccessResult::allowed(), AccessResult::neutral()); + $merge_both_directions(AccessResult::neutral(), AccessResult::neutral()); + $merge_both_directions(AccessResult::neutral(), AccessResult::allowed()); + } + /** * Tests allowedIfHasPermissions(). *