Verified Commit aa747ca7 authored by Lee Rowlands's avatar Lee Rowlands
Browse files

Issue #3278759 by mxr576, kristiaanvandeneynde, acbramley, danflanagan8,...

Issue #3278759 by mxr576, kristiaanvandeneynde, acbramley, danflanagan8, larowlan, bbrala: Access cacheability is not correct when "view own unpublished content" is in use

(cherry picked from commit 7102b46b)
parent 6e347fa8
Loading
Loading
Loading
Loading
Loading
+25 −6
Original line number Diff line number Diff line
@@ -346,17 +346,13 @@ public function testGetIndividual(): void {
      ['4xx-response', 'http_response', 'node:1'],
      ['url.query_args', 'url.site', 'user.permissions'],
      'UNCACHEABLE (request policy)',
      'MISS'
      TRUE
    );

    // 200 after granting permission.
    $this->grantPermissionsToTestedRole(['view own unpublished content']);
    $response = $this->request('GET', $url, $request_options);
    // The response varies by 'user', causing the 'user.permissions' cache
    // context to be optimized away.
    $expected_cache_contexts = Cache::mergeContexts($this->getExpectedCacheContexts(), ['user']);
    $expected_cache_contexts = array_diff($expected_cache_contexts, ['user.permissions']);
    $this->assertResourceResponse(200, FALSE, $response, $this->getExpectedCacheTags(), $expected_cache_contexts, 'UNCACHEABLE (request policy)', 'UNCACHEABLE (poor cacheability)');
    $this->assertResourceResponse(200, FALSE, $response, $this->getExpectedCacheTags(), $this->getExpectedCacheContexts(), 'UNCACHEABLE (request policy)', TRUE);
  }

  /**
@@ -413,6 +409,29 @@ protected function assertNormalizedFieldsAreCached(array $field_names): void {
    });
  }

  /**
   * {@inheritdoc}
   */
  protected function getExpectedCacheContexts(?array $sparse_fieldset = NULL) {
    // \Drupal\Tests\jsonapi\Functional\ResourceTestBase::testRevisions()
    // loads different revisions via query parameters, we do our best
    // here to react to those directly, or indirectly.
    $cache_contexts = parent::getExpectedCacheContexts($sparse_fieldset);

    // This is bubbled up by
    // \Drupal\node\NodeAccessControlHandler::checkAccess() directly.
    if ($this->entity->isPublished()) {
      return $cache_contexts;
    }
    if (!\Drupal::currentUser()->isAuthenticated()) {
      return Cache::mergeContexts($cache_contexts, ['user.roles:authenticated']);
    }
    if (\Drupal::currentUser()->hasPermission('view own unpublished content')) {
      return Cache::mergeContexts($cache_contexts, ['user']);
    }
    return $cache_contexts;
  }

  /**
   * {@inheritdoc}
   */
+62 −20
Original line number Diff line number Diff line
@@ -3,6 +3,8 @@
namespace Drupal\node;

use Drupal\Core\Access\AccessResult;
use Drupal\Core\Access\AccessResultInterface;
use Drupal\Core\Cache\CacheableMetadata;
use Drupal\Core\Cache\RefinableCacheableDependencyInterface;
use Drupal\Core\Entity\EntityHandlerInterface;
use Drupal\Core\Entity\EntityTypeInterface;
@@ -126,15 +128,15 @@ public function createAccess($entity_bundle = NULL, ?AccountInterface $account =
   * {@inheritdoc}
   */
  protected function checkAccess(EntityInterface $node, $operation, AccountInterface $account) {
    /** @var \Drupal\node\NodeInterface $node */

    // Fetch information from the node object if possible.
    $status = $node->isPublished();
    $uid = $node->getOwnerId();
    assert($node instanceof NodeInterface);
    $cacheability = new CacheableMetadata();

    // Check if authors can view their own unpublished nodes.
    if ($operation === 'view' && !$status && $account->hasPermission('view own unpublished content') && $account->isAuthenticated() && $account->id() == $uid) {
      return AccessResult::allowed()->cachePerPermissions()->cachePerUser()->addCacheableDependency($node);
    /** @var \Drupal\node\NodeInterface $node */
    if ($operation === 'view') {
      $result = $this->checkViewAccess($node, $account, $cacheability);
      if ($result !== NULL) {
        return $result;
      }
    }

    [$revision_permission_operation, $entity_operation] = static::REVISION_OPERATION_MAP[$operation] ?? [
@@ -144,25 +146,28 @@ protected function checkAccess(EntityInterface $node, $operation, AccountInterfa

    // Revision operations.
    if ($revision_permission_operation) {
      $cacheability->addCacheContexts(['user.permissions']);
      $bundle = $node->bundle();

      // If user doesn't have any of these then quit.
      if (!$account->hasPermission("$revision_permission_operation all revisions") && !$account->hasPermission("$revision_permission_operation $bundle revisions") && !$account->hasPermission('administer nodes')) {
        return AccessResult::neutral()->cachePerPermissions();
        return AccessResult::neutral()->addCacheableDependency($cacheability);
      }

      // If the user has the view all revisions permission and this is the view
      // all revisions operation then we can allow access.
      if ($operation === 'view all revisions') {
        return AccessResult::allowed()->cachePerPermissions();
        return AccessResult::allowed()->addCacheableDependency($cacheability);
      }

      // If this is the default revision, return access denied for revert or
      // delete operations.
      $cacheability->addCacheableDependency($node);
      if ($node->isDefaultRevision() && ($operation === 'revert revision' || $operation === 'delete revision')) {
        return AccessResult::forbidden()->addCacheableDependency($node);
        return AccessResult::forbidden()->addCacheableDependency($cacheability);
      }
      elseif ($account->hasPermission('administer nodes')) {
        return AccessResult::allowed()->cachePerPermissions();
        return AccessResult::allowed()->addCacheableDependency($cacheability);
      }

      // First check the access to the default revision and finally, if the
@@ -173,22 +178,59 @@ protected function checkAccess(EntityInterface $node, $operation, AccountInterfa
      if (!$node->isDefaultRevision()) {
        $access = $access->andIf($this->access($node, $entity_operation, $account, TRUE));
      }
      return $access->cachePerPermissions()->addCacheableDependency($node);
      return $access->addCacheableDependency($cacheability);
    }

    // Evaluate node grants.
    $access_result = $this->grantStorage->access($node, $operation, $account);
    if ($operation === 'view' && $access_result instanceof RefinableCacheableDependencyInterface) {
      // Node variations can affect the access to the node. For instance, the
      // access result cache varies on the node's published status. Only the
      // 'view' node grant can currently be cached. The 'update' and 'delete'
      // grants are already marked as uncacheable in the node grant storage.
      // @see \Drupal\node\NodeGrantDatabaseStorage::access()
      $access_result->addCacheableDependency($node);
    if ($access_result instanceof RefinableCacheableDependencyInterface) {
      $access_result->addCacheableDependency($cacheability);
    }
    return $access_result;
  }

  /**
   * Performs view access checks.
   *
   * @param \Drupal\node\NodeInterface $node
   *   The node for which to check access.
   * @param \Drupal\Core\Session\AccountInterface $account
   *   The user for which to check access.
   * @param \Drupal\Core\Cache\CacheableMetadata $cacheability
   *   Allows cacheability information bubble up from this method.
   *
   * @return \Drupal\Core\Access\AccessResultInterface|null
   *   The calculated access result or null when no opinion.
   */
  protected function checkViewAccess(NodeInterface $node, AccountInterface $account, CacheableMetadata $cacheability): ?AccessResultInterface {
    // If the node status changes, so does the outcome of the check below, so
    // we need to add the node as a cacheable dependency.
    $cacheability->addCacheableDependency($node);

    if ($node->isPublished()) {
      return NULL;
    }
    $cacheability->addCacheContexts(['user.permissions']);

    if (!$account->hasPermission('view own unpublished content')) {
      return NULL;
    }

    $cacheability->addCacheContexts(['user.roles:authenticated']);
    // The "view own unpublished content" permission must not be granted
    // to anonymous users for security reasons.
    if (!$account->isAuthenticated()) {
      return NULL;
    }

    $cacheability->addCacheContexts(['user']);
    if ($account->id() != $node->getOwnerId()) {
      return NULL;
    }

    return AccessResult::allowed()->addCacheableDependency($cacheability);
  }

  /**
   * {@inheritdoc}
   */
+9 −5
Original line number Diff line number Diff line
@@ -66,15 +66,19 @@ public function access(NodeInterface $node, $operation, AccountInterface $accoun

    // If no module implements the hook or the node does not have an id there is
    // no point in querying the database for access grants.
    if (!$this->moduleHandler->hasImplementations('node_grants') || !$node->id()) {
    if (!$this->moduleHandler->hasImplementations('node_grants') || $node->isNew()) {
      // Return the equivalent of the default grant, defined by
      // self::writeDefault().
      if ($operation === 'view') {
        return AccessResult::allowedIf($node->isPublished());
        $result = AccessResult::allowedIf($node->isPublished());
        if (!$node->isNew()) {
          $result->addCacheableDependency($node);
        }
      else {
        return AccessResult::neutral();

        return $result;
      }

      return AccessResult::neutral();
    }

    // Check the database for potential access grants.
+12 −0
Original line number Diff line number Diff line
@@ -4,3 +4,15 @@ node_access_test_auto_bubbling:
    _controller: '\Drupal\node_access_test_auto_bubbling\Controller\NodeAccessTestAutoBubblingController::latest'
  requirements:
    _access: 'TRUE'
node_access_test_auto_bubbling.node_access:
  path: '/node_access_test_auto_bubbling_node_access/{node}'
  defaults:
    _controller: '\Drupal\node_access_test_auto_bubbling\Controller\NodeAccessTestAutoBubblingController::nodeAccessCacheability'
  requirements:
    # Access checking intentionally happens in the controller instead of here.
    _access: 'TRUE'
    node: \d+
  options:
    parameters:
      node:
        type: entity:node
+25 −0
Original line number Diff line number Diff line
@@ -2,6 +2,7 @@

namespace Drupal\node_access_test_auto_bubbling\Controller;

use Drupal\Core\Cache\CacheableMetadata;
use Drupal\Core\Controller\ControllerBase;
use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
use Drupal\node\NodeInterface;
@@ -27,4 +28,28 @@ public function latest() {
    return ['#markup' => $this->t('The three latest nodes are: @nids.', ['@nids' => implode(', ', $nids)])];
  }

  /**
   * Exposes the node label when a user has access to view a node.
   *
   * @param \Drupal\node\NodeInterface $node
   *   A node entity.
   *
   * @return array
   *   A render array.
   */
  public function nodeAccessCacheability(NodeInterface $node) {
    $build = [];
    $cacheability = new CacheableMetadata();
    $access = $node->access('view', return_as_object: TRUE);
    $cacheability->addCacheableDependency($access);
    if ($access->isAllowed()) {
      $build[] = [
        '#markup' => $node->label(),
      ];
      $cacheability->addCacheableDependency($node);
    }
    $cacheability->applyTo($build);
    return $build;
  }

}
Loading