Commit 90baffa4 authored by catch's avatar catch
Browse files

Issue #3452426 by mxr576, kristiaanvandeneynde: Insufficient cacheability... 

Issue #3452426 by mxr576, kristiaanvandeneynde: Insufficient cacheability information bubbled up by UserAccessControlHandler

(cherry picked from commit 9fc1bc9a)
parent a47b4177

core/modules/jsonapi/tests/src/Functional/UserTest.php

+1 −1
Original line number Diff line number Diff line
@@ -175,7 +175,7 @@ protected function getExpectedDocument() { 
   */

  protected function getExpectedCacheContexts(?array $sparse_fieldset = NULL) {

    $cache_contexts = parent::getExpectedCacheContexts($sparse_fieldset);

    if ($sparse_fieldset === NULL || in_array('mail', $sparse_fieldset)) {

    if ($sparse_fieldset === NULL || !empty(array_intersect(['mail', 'display_name'], $sparse_fieldset))) {

      $cache_contexts = Cache::mergeContexts($cache_contexts, ['user']);

    }

    return $cache_contexts;

core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php

+12 −0
Original line number Diff line number Diff line
@@ -436,6 +436,9 @@ public function testGet() { 
        // @see \Drupal\Core\EventSubscriber\AnonymousUserResponseSubscriber::onRespond()

        ->addCacheTags(['config:user.role.anonymous']);

      $expected_cacheability->addCacheableDependency($this->getExpectedUnauthorizedEntityAccessCacheability(FALSE));

      // Mitigate https://www.drupal.org/project/drupal/issues/3451483 until

      // it gets resolved.

      $response = $response->withoutHeader('X-Drupal-Dynamic-Cache');

      $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response, $expected_cacheability->getCacheTags(), $expected_cacheability->getCacheContexts(), 'MISS', FALSE);

    }

    else {
@@ -448,6 +451,9 @@ public function testGet() { 
    // response.

    if (static::$auth) {

      $response = $this->request('GET', $url, $request_options);

      // Mitigate https://www.drupal.org/project/drupal/issues/3451483 until

      // it gets resolved.

      $response = $response->withoutHeader('X-Drupal-Dynamic-Cache');

      $this->assertResponseWhenMissingAuthentication('GET', $response);

    }
@@ -485,6 +491,9 @@ public function testGet() { 
    // DX: 403 because unauthorized.

    $url->setOption('query', ['_format' => static::$format]);

    $response = $this->request('GET', $url, $request_options);

    // Mitigate https://www.drupal.org/project/drupal/issues/3451483 until

    // it gets resolved.

    $response = $response->withoutHeader('X-Drupal-Dynamic-Cache');

    $this->assertResourceErrorResponse(403, FALSE, $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', FALSE);



    // Then, what we'll use for the remainder of the test: multiple formats.
@@ -505,6 +514,9 @@ public function testGet() { 
    // DX: 403 because unauthorized.

    $url->setOption('query', ['_format' => static::$format]);

    $response = $this->request('GET', $url, $request_options);

    // Mitigate https://www.drupal.org/project/drupal/issues/3451483 until

    // it gets resolved.

    $response = $response->withoutHeader('X-Drupal-Dynamic-Cache');

    $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', FALSE);

    $this->assertArrayNotHasKey('Link', $response->getHeaders());

core/modules/rest/tests/src/Functional/ResourceTestBase.php

+2 −1
Original line number Diff line number Diff line
@@ -406,7 +406,8 @@ protected function assertResourceResponse($expected_status_code, $expected_body, 
    // Expected cache contexts: X-Drupal-Cache-Contexts header.

    $this->assertSame($expected_cache_contexts !== FALSE, $response->hasHeader('X-Drupal-Cache-Contexts'));

    if (is_array($expected_cache_contexts)) {

      $this->assertEqualsCanonicalizing($expected_cache_contexts, explode(' ', $response->getHeader('X-Drupal-Cache-Contexts')[0]));

      $optimized_expected_cache_contexts = \Drupal::service('cache_contexts_manager')->optimizeTokens($expected_cache_contexts);

      $this->assertEqualsCanonicalizing($optimized_expected_cache_contexts, explode(' ', $response->getHeader('X-Drupal-Cache-Contexts')[0]));

    }



    // Expected Page Cache header value: X-Drupal-Cache header.

core/modules/user/src/UserAccessControlHandler.php

+16 −10
Original line number Diff line number Diff line
@@ -3,7 +3,6 @@ 
namespace Drupal\user;



use Drupal\Core\Access\AccessResult;

use Drupal\Core\Access\AccessResultNeutral;

use Drupal\Core\Access\AccessResultReasonInterface;

use Drupal\Core\Entity\EntityInterface;

use Drupal\Core\Entity\EntityAccessControlHandler;
@@ -51,17 +50,24 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter 
    switch ($operation) {

      case 'view':

        // Only allow view access if the account is active.

        if ($account->hasPermission('access user profiles') && $entity->isActive()) {

          return AccessResult::allowed()->cachePerPermissions()->addCacheableDependency($entity);

        $result = AccessResult::allowedIfHasPermission($account, 'access user profiles');



        if ($result->isAllowed()) {

          $result = $result->andIf(

            AccessResult::allowedIf($entity->isActive())->addCacheableDependency($entity)

          );



          if ($result instanceof AccessResultReasonInterface) {

            $result->setReason("The 'access user profiles' permission is required and the user must be active.");

          }

        // Users can view own profiles at all times.

        elseif ($account->id() == $entity->id()) {

          return AccessResult::allowed()->cachePerUser();



          if ($result->isAllowed()) {

            return $result;

          }

        else {

          return AccessResultNeutral::neutral("The 'access user profiles' permission is required and the user must be active.")->cachePerPermissions()->addCacheableDependency($entity);

        }

        break;



        // Users can view own profiles at all times.

        return $result->orIf(AccessResult::allowedIf($account->id() == $entity->id())->addCacheContexts(['user']));



      case 'update':

        // Users can always edit their own account.

core/modules/user/tests/src/Functional/Rest/UserResourceTestBase.php

+8 −3
Original line number Diff line number Diff line
@@ -309,7 +309,7 @@ public function testPatchSecurityOtherUser() { 
  protected function getExpectedUnauthorizedAccessMessage($method) {

    switch ($method) {

      case 'GET':

        return "The 'access user profiles' permission is required and the user must be active.";

        return "The 'access user profiles' permission is required.";



      case 'PATCH':

        return "Users can only update their own account, unless they have the 'administer users' permission.";
@@ -327,8 +327,13 @@ protected function getExpectedUnauthorizedAccessMessage($method) { 
   */

  protected function getExpectedUnauthorizedEntityAccessCacheability($is_authenticated) {

    // @see \Drupal\user\UserAccessControlHandler::checkAccess()

    return parent::getExpectedUnauthorizedEntityAccessCacheability($is_authenticated)

      ->addCacheTags(['user:3']);

    $result = parent::getExpectedUnauthorizedEntityAccessCacheability($is_authenticated);



    if (!\Drupal::currentUser()->hasPermission('access user profiles')) {

      $result->addCacheContexts(['user']);

    }



    return $result;

  }



  /**