.../Drupal/Core/Cache/CacheableDependencyTrait.php | 67 ++++++++++++++++++++++ .../Cache/RefinableCacheableDependencyTrait.php | 42 +------------- .../DefaultExceptionHtmlSubscriber.php | 7 +++ .../EventSubscriber/ExceptionJsonSubscriber.php | 14 ++++- .../CacheableAccessDeniedHttpException.php | 24 ++++++++ .../Exception/CacheableBadRequestHttpException.php | 24 ++++++++ .../Exception/CacheableConflictHttpException.php | 24 ++++++++ .../Http/Exception/CacheableGoneHttpException.php | 24 ++++++++ .../CacheableLengthRequiredHttpException.php | 24 ++++++++ .../CacheableMethodNotAllowedHttpException.php | 24 ++++++++ .../CacheableNotAcceptableHttpException.php | 24 ++++++++ .../Exception/CacheableNotFoundHttpException.php | 24 ++++++++ .../CacheablePreconditionFailedHttpException.php | 24 ++++++++ .../CacheablePreconditionRequiredHttpException.php | 24 ++++++++ .../CacheableServiceUnavailableHttpException.php | 24 ++++++++ .../CacheableTooManyRequestsHttpException.php | 24 ++++++++ .../CacheableUnauthorizedHttpException.php | 24 ++++++++ .../CacheableUnprocessableEntityHttpException.php | 24 ++++++++ .../CacheableUnsupportedMediaTypeHttpException.php | 24 ++++++++ .../src/Authentication/Provider/BasicAuth.php | 31 +++++++++- .../tests/src/Functional/BasicAuthTest.php | 42 ++++++++++++++ .../tests/modules/rest_test/rest_test.services.yml | 5 ++ .../RequestPolicy/DenyTestAuthRequests.php | 26 +++++++++ .../EntityResource/EntityResourceTestBase.php | 15 +++-- 24 files changed, 560 insertions(+), 49 deletions(-) diff --git a/core/lib/Drupal/Core/Cache/CacheableDependencyTrait.php b/core/lib/Drupal/Core/Cache/CacheableDependencyTrait.php new file mode 100644 index 0000000..19cb0f2 --- /dev/null +++ b/core/lib/Drupal/Core/Cache/CacheableDependencyTrait.php @@ -0,0 +1,67 @@ +cacheContexts = $cacheability->getCacheContexts(); + $this->cacheTags = $cacheability->getCacheTags(); + $this->cacheMaxAge = $cacheability->getCacheMaxAge(); + return $this; + } + + /** + * {@inheritdoc} + */ + public function getCacheTags() { + return $this->cacheTags; + } + + /** + * {@inheritdoc} + */ + public function getCacheContexts() { + return $this->cacheContexts; + } + + /** + * {@inheritdoc} + */ + public function getCacheMaxAge() { + return $this->cacheMaxAge; + } + +} diff --git a/core/lib/Drupal/Core/Cache/RefinableCacheableDependencyTrait.php b/core/lib/Drupal/Core/Cache/RefinableCacheableDependencyTrait.php index 21b61b2..fcbc11f 100644 --- a/core/lib/Drupal/Core/Cache/RefinableCacheableDependencyTrait.php +++ b/core/lib/Drupal/Core/Cache/RefinableCacheableDependencyTrait.php @@ -7,47 +7,7 @@ */ trait RefinableCacheableDependencyTrait { - /** - * Cache contexts. - * - * @var string[] - */ - protected $cacheContexts = []; - - /** - * Cache tags. - * - * @var string[] - */ - protected $cacheTags = []; - - /** - * Cache max-age. - * - * @var int - */ - protected $cacheMaxAge = Cache::PERMANENT; - - /** - * {@inheritdoc} - */ - public function getCacheTags() { - return $this->cacheTags; - } - - /** - * {@inheritdoc} - */ - public function getCacheContexts() { - return $this->cacheContexts; - } - - /** - * {@inheritdoc} - */ - public function getCacheMaxAge() { - return $this->cacheMaxAge; - } + use CacheableDependencyTrait; /** * {@inheritdoc} diff --git a/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php index f168a02..1e5df6d 100644 --- a/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php @@ -2,6 +2,8 @@ namespace Drupal\Core\EventSubscriber; +use Drupal\Core\Cache\CacheableResponseInterface; +use Drupal\Core\Cache\Exception\CacheableHttpExceptionInterface; use Drupal\Core\Routing\RedirectDestinationInterface; use Drupal\Core\Utility\Error; use Psr\Log\LoggerInterface; @@ -170,6 +172,11 @@ protected function makeSubrequest(GetResponseForExceptionEvent $event, $url, $st $response->setStatusCode($status_code); } + // Persist the exception's cacheability metadata, if any. + if ($response instanceof CacheableResponseInterface && $exception instanceof CacheableHttpExceptionInterface) { + $response->addCacheableDependency($exception->getCacheableMetadata()); + } + // Persist any special HTTP headers that were set on the exception. if ($exception instanceof HttpExceptionInterface) { $response->headers->add($exception->getHeaders()); diff --git a/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php index 6ccb743..620ad1b 100644 --- a/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php @@ -2,6 +2,9 @@ namespace Drupal\Core\EventSubscriber; +use Drupal\Core\Cache\CacheableDependencyInterface; +use Drupal\Core\Cache\CacheableJsonResponse; +use Drupal\Core\Http\Exception\CacheableHttpExceptionInterface; use Symfony\Component\HttpFoundation\JsonResponse; use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent; @@ -35,7 +38,16 @@ protected static function getPriority() { public function on4xx(GetResponseForExceptionEvent $event) { /** @var \Symfony\Component\HttpKernel\Exception\HttpExceptionInterface $exception */ $exception = $event->getException(); - $response = new JsonResponse(['message' => $event->getException()->getMessage()], $exception->getStatusCode(), $exception->getHeaders()); + + // If the exception is cacheable, generate a cacheable response. + if ($exception instanceof CacheableDependencyInterface) { + $response = new CacheableJsonResponse(['message' => $event->getException()->getMessage()], $exception->getStatusCode(), $exception->getHeaders()); + $response->addCacheableDependency($exception); + } + else { + $response = new JsonResponse(['message' => $event->getException()->getMessage()], $exception->getStatusCode(), $exception->getHeaders()); + } + $event->setResponse($response); } diff --git a/core/lib/Drupal/Core/Http/Exception/CacheableAccessDeniedHttpException.php b/core/lib/Drupal/Core/Http/Exception/CacheableAccessDeniedHttpException.php new file mode 100644 index 0000000..e0fd5cb --- /dev/null +++ b/core/lib/Drupal/Core/Http/Exception/CacheableAccessDeniedHttpException.php @@ -0,0 +1,24 @@ +setCacheability($cacheability); + parent::__construct($message, $previous, $code); + } + +} diff --git a/core/lib/Drupal/Core/Http/Exception/CacheableBadRequestHttpException.php b/core/lib/Drupal/Core/Http/Exception/CacheableBadRequestHttpException.php new file mode 100644 index 0000000..97d432a --- /dev/null +++ b/core/lib/Drupal/Core/Http/Exception/CacheableBadRequestHttpException.php @@ -0,0 +1,24 @@ +setCacheability($cacheability); + parent::__construct($message, $previous, $code); + } + +} diff --git a/core/lib/Drupal/Core/Http/Exception/CacheableConflictHttpException.php b/core/lib/Drupal/Core/Http/Exception/CacheableConflictHttpException.php new file mode 100644 index 0000000..ca804fb --- /dev/null +++ b/core/lib/Drupal/Core/Http/Exception/CacheableConflictHttpException.php @@ -0,0 +1,24 @@ +setCacheability($cacheability); + parent::__construct($message, $previous, $code); + } + +} diff --git a/core/lib/Drupal/Core/Http/Exception/CacheableGoneHttpException.php b/core/lib/Drupal/Core/Http/Exception/CacheableGoneHttpException.php new file mode 100644 index 0000000..4568c91 --- /dev/null +++ b/core/lib/Drupal/Core/Http/Exception/CacheableGoneHttpException.php @@ -0,0 +1,24 @@ +setCacheability($cacheability); + parent::__construct($message, $previous, $code); + } + +} diff --git a/core/lib/Drupal/Core/Http/Exception/CacheableLengthRequiredHttpException.php b/core/lib/Drupal/Core/Http/Exception/CacheableLengthRequiredHttpException.php new file mode 100644 index 0000000..a75f80a --- /dev/null +++ b/core/lib/Drupal/Core/Http/Exception/CacheableLengthRequiredHttpException.php @@ -0,0 +1,24 @@ +setCacheability($cacheability); + parent::__construct($message, $previous, $code); + } + +} diff --git a/core/lib/Drupal/Core/Http/Exception/CacheableMethodNotAllowedHttpException.php b/core/lib/Drupal/Core/Http/Exception/CacheableMethodNotAllowedHttpException.php new file mode 100644 index 0000000..1ccc388 --- /dev/null +++ b/core/lib/Drupal/Core/Http/Exception/CacheableMethodNotAllowedHttpException.php @@ -0,0 +1,24 @@ +setCacheability($cacheability); + parent::__construct($message, $previous, $code); + } + +} diff --git a/core/lib/Drupal/Core/Http/Exception/CacheableNotAcceptableHttpException.php b/core/lib/Drupal/Core/Http/Exception/CacheableNotAcceptableHttpException.php new file mode 100644 index 0000000..94bf1c2 --- /dev/null +++ b/core/lib/Drupal/Core/Http/Exception/CacheableNotAcceptableHttpException.php @@ -0,0 +1,24 @@ +setCacheability($cacheability); + parent::__construct($message, $previous, $code); + } + +} diff --git a/core/lib/Drupal/Core/Http/Exception/CacheableNotFoundHttpException.php b/core/lib/Drupal/Core/Http/Exception/CacheableNotFoundHttpException.php new file mode 100644 index 0000000..9e5e136 --- /dev/null +++ b/core/lib/Drupal/Core/Http/Exception/CacheableNotFoundHttpException.php @@ -0,0 +1,24 @@ +setCacheability($cacheability); + parent::__construct($message, $previous, $code); + } + +} diff --git a/core/lib/Drupal/Core/Http/Exception/CacheablePreconditionFailedHttpException.php b/core/lib/Drupal/Core/Http/Exception/CacheablePreconditionFailedHttpException.php new file mode 100644 index 0000000..7921d3e --- /dev/null +++ b/core/lib/Drupal/Core/Http/Exception/CacheablePreconditionFailedHttpException.php @@ -0,0 +1,24 @@ +setCacheability($cacheability); + parent::__construct($message, $previous, $code); + } + +} diff --git a/core/lib/Drupal/Core/Http/Exception/CacheablePreconditionRequiredHttpException.php b/core/lib/Drupal/Core/Http/Exception/CacheablePreconditionRequiredHttpException.php new file mode 100644 index 0000000..d66b255 --- /dev/null +++ b/core/lib/Drupal/Core/Http/Exception/CacheablePreconditionRequiredHttpException.php @@ -0,0 +1,24 @@ +setCacheability($cacheability); + parent::__construct($message, $previous, $code); + } + +} diff --git a/core/lib/Drupal/Core/Http/Exception/CacheableServiceUnavailableHttpException.php b/core/lib/Drupal/Core/Http/Exception/CacheableServiceUnavailableHttpException.php new file mode 100644 index 0000000..313b9ae --- /dev/null +++ b/core/lib/Drupal/Core/Http/Exception/CacheableServiceUnavailableHttpException.php @@ -0,0 +1,24 @@ +setCacheability($cacheability); + parent::__construct($retryAfter, $message, $previous, $code); + } + +} diff --git a/core/lib/Drupal/Core/Http/Exception/CacheableTooManyRequestsHttpException.php b/core/lib/Drupal/Core/Http/Exception/CacheableTooManyRequestsHttpException.php new file mode 100644 index 0000000..e709c0b --- /dev/null +++ b/core/lib/Drupal/Core/Http/Exception/CacheableTooManyRequestsHttpException.php @@ -0,0 +1,24 @@ +setCacheability($cacheability); + parent::__construct($retryAfter, $message, $previous, $code); + } + +} diff --git a/core/lib/Drupal/Core/Http/Exception/CacheableUnauthorizedHttpException.php b/core/lib/Drupal/Core/Http/Exception/CacheableUnauthorizedHttpException.php new file mode 100644 index 0000000..35dbd72 --- /dev/null +++ b/core/lib/Drupal/Core/Http/Exception/CacheableUnauthorizedHttpException.php @@ -0,0 +1,24 @@ +setCacheability($cacheability); + parent::__construct($challenge, $message, $previous, $code); + } + +} diff --git a/core/lib/Drupal/Core/Http/Exception/CacheableUnprocessableEntityHttpException.php b/core/lib/Drupal/Core/Http/Exception/CacheableUnprocessableEntityHttpException.php new file mode 100644 index 0000000..655c67a --- /dev/null +++ b/core/lib/Drupal/Core/Http/Exception/CacheableUnprocessableEntityHttpException.php @@ -0,0 +1,24 @@ +setCacheability($cacheability); + parent::__construct($message, $previous, $code); + } + +} diff --git a/core/lib/Drupal/Core/Http/Exception/CacheableUnsupportedMediaTypeHttpException.php b/core/lib/Drupal/Core/Http/Exception/CacheableUnsupportedMediaTypeHttpException.php new file mode 100644 index 0000000..c6f6023 --- /dev/null +++ b/core/lib/Drupal/Core/Http/Exception/CacheableUnsupportedMediaTypeHttpException.php @@ -0,0 +1,24 @@ +setCacheability($cacheability); + parent::__construct($message, $previous, $code); + } + +} diff --git a/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php b/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php index c72e3f0..01034f4 100644 --- a/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php +++ b/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php @@ -5,12 +5,13 @@ use Drupal\Component\Utility\SafeMarkup; use Drupal\Core\Authentication\AuthenticationProviderInterface; use Drupal\Core\Authentication\AuthenticationProviderChallengeInterface; +use Drupal\Core\Cache\CacheableMetadata; use Drupal\Core\Config\ConfigFactoryInterface; use Drupal\Core\Entity\EntityManagerInterface; use Drupal\Core\Flood\FloodInterface; +use Drupal\Core\Http\Exception\CacheableUnauthorizedHttpException; use Drupal\user\UserAuthInterface; use Symfony\Component\HttpFoundation\Request; -use Symfony\Component\HttpKernel\Exception\UnauthorizedHttpException; /** * HTTP Basic authentication provider. @@ -126,11 +127,35 @@ public function authenticate(Request $request) { * {@inheritdoc} */ public function challengeException(Request $request, \Exception $previous) { - $site_name = $this->configFactory->get('system.site')->get('name'); + $site_config = $this->configFactory->get('system.site'); + $site_name = $site_config->get('name'); $challenge = SafeMarkup::format('Basic realm="@realm"', [ '@realm' => !empty($site_name) ? $site_name : 'Access restricted', ]); - return new UnauthorizedHttpException((string) $challenge, 'No authentication credentials provided.', $previous); + + // A 403 is converted to a 401 here, but it doesn't matter what the + // cacheability was of the 403 exception: what matters here is that + // authentication credentials are missing, i.e. that this request was made + // as the anonymous user. + // Therefore, all we must do, is make this response: + // 1. vary by whether the current user has the 'anonymous' role or not. This + // works fine because: + // - Thanks to \Drupal\basic_auth\PageCache\DisallowBasicAuthRequests, + // Page Cache never caches a response whose request has Basic Auth + // credentials. + // - Dynamic Page Cache will cache a different result for when the + // request is unauthenticated (this 401) versus authenticated (some + // other response) + // 2. have the 'config:user.role.anonymous' cache tag, because the only + // reason this 401 would no longer be a 401 is if permissions for the + // 'anonymous' role change, causing that cache tag to be invalidated. + // @see \Drupal\Core\EventSubscriber\AuthenticationSubscriber::onExceptionSendChallenge() + // @see \Drupal\Core\EventSubscriber\ClientErrorResponseSubscriber() + // @see \Drupal\Core\EventSubscriber\FinishResponseSubscriber::onAllResponds() + $cacheability = CacheableMetadata::createFromObject($site_config) + ->addCacheTags(['config:user.role.anonymous']) + ->addCacheContexts(['user.roles:anonymous']); + return new CacheableUnauthorizedHttpException($cacheability, (string) $challenge, 'No authentication credentials provided.', $previous); } } diff --git a/core/modules/basic_auth/tests/src/Functional/BasicAuthTest.php b/core/modules/basic_auth/tests/src/Functional/BasicAuthTest.php index b197a76..f844cb9 100644 --- a/core/modules/basic_auth/tests/src/Functional/BasicAuthTest.php +++ b/core/modules/basic_auth/tests/src/Functional/BasicAuthTest.php @@ -7,6 +7,7 @@ use Drupal\Tests\basic_auth\Traits\BasicAuthTestTrait; use Drupal\language\Entity\ConfigurableLanguage; use Drupal\Tests\BrowserTestBase; +use Drupal\user\Entity\Role; /** * Tests for BasicAuth authentication provider. @@ -181,6 +182,47 @@ public function testUnauthorizedErrorMessage() { } /** + * Tests the cacheability of Basic Auth's 401 response. + * + * @see \Drupal\basic_auth\Authentication\Provider\BasicAuth::challengeException() + */ + public function testCacheabilityOf401Response() { + $session = $this->getSession(); + $url = Url::fromRoute('router_test.11'); + + $assert_response_cacheability = function ($expected_page_cache_header_value, $expected_dynamic_page_cache_header_value) use ($session, $url) { + $this->drupalGet($url); + $this->assertSession()->statusCodeEquals(401); + $this->assertSame($expected_page_cache_header_value, $session->getResponseHeader('X-Drupal-Cache')); + $this->assertSame($expected_dynamic_page_cache_header_value, $session->getResponseHeader('X-Drupal-Dynamic-Cache')); + }; + + // 1. First request: cold caches, both Page Cache and Dynamic Page Cache are + // now primed. + $assert_response_cacheability('MISS', 'MISS'); + // 2. Second request: Page Cache HIT, we don't even hit Dynamic Page Cache. + // This is going to keep happening. + $assert_response_cacheability('HIT', 'MISS'); + // 3. Third request: after clearing Page Cache, we now see that Dynamic Page + // Cache is a HIT too. + $this->container->get('cache.page')->deleteAll(); + $assert_response_cacheability('MISS', 'HIT'); + // 4. Fourth request: warm caches. + $assert_response_cacheability('HIT', 'HIT'); + + // If the permissions of the 'anonymous' role change, it may no longer be + // necessary to be authenticated to access this route. Therefore the cached + // 401 responses should be invalidated. + $this->grantPermissions(Role::load(Role::ANONYMOUS_ID), [$this->randomMachineName()]); + $assert_response_cacheability('MISS', 'MISS'); + $assert_response_cacheability('HIT', 'MISS'); + // Idem for when the 'system.site' config changes. + $this->config('system.site')->save(); + $assert_response_cacheability('MISS', 'MISS'); + $assert_response_cacheability('HIT', 'MISS'); + } + + /** * Tests if the controller is called before authentication. * * @see https://www.drupal.org/node/2817727 diff --git a/core/modules/rest/tests/modules/rest_test/rest_test.services.yml b/core/modules/rest/tests/modules/rest_test/rest_test.services.yml index ccdbeae..d316cf6 100644 --- a/core/modules/rest/tests/modules/rest_test/rest_test.services.yml +++ b/core/modules/rest/tests/modules/rest_test/rest_test.services.yml @@ -7,3 +7,8 @@ services: class: Drupal\rest_test\Authentication\Provider\TestAuthGlobal tags: - { name: authentication_provider, provider_id: 'rest_test_auth_global', global: TRUE } + rest_test.page_cache_request_policy.deny_test_auth_requests: + class: Drupal\rest_test\PageCache\RequestPolicy\DenyTestAuthRequests + public: false + tags: + - { name: page_cache_request_policy } diff --git a/core/modules/rest/tests/modules/rest_test/src/PageCache/RequestPolicy/DenyTestAuthRequests.php b/core/modules/rest/tests/modules/rest_test/src/PageCache/RequestPolicy/DenyTestAuthRequests.php new file mode 100644 index 0000000..8b547a9 --- /dev/null +++ b/core/modules/rest/tests/modules/rest_test/src/PageCache/RequestPolicy/DenyTestAuthRequests.php @@ -0,0 +1,26 @@ +headers->has('REST-test-auth') || $request->headers->has('REST-test-auth-global')) { + return self::DENY; + } + } + +} diff --git a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php index 41d9eec..ca7424d 100644 --- a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php @@ -430,14 +430,20 @@ public function testGet() { ':pattern' => '%[route]=rest.%', ]) ->fetchAllAssoc('cid'); - $this->assertCount(2, $cache_items); + $this->assertTrue(count($cache_items) >= 2); $found_cache_redirect = FALSE; - $found_cached_response = FALSE; + $found_cached_200_response = FALSE; + $other_cached_responses_are_4xx = TRUE; foreach ($cache_items as $cid => $cache_item) { $cached_data = unserialize($cache_item->data); if (!isset($cached_data['#cache_redirect'])) { - $found_cached_response = TRUE; $cached_response = $cached_data['#response']; + if ($cached_response->getStatusCode() === 200) { + $found_cached_200_response = TRUE; + } + elseif (!$cached_response->isClientError()) { + $other_cached_responses_are_4xx = FALSE; + } $this->assertNotInstanceOf(ResourceResponseInterface::class, $cached_response); $this->assertInstanceOf(CacheableResponseInterface::class, $cached_response); } @@ -446,7 +452,8 @@ public function testGet() { } } $this->assertTrue($found_cache_redirect); - $this->assertTrue($found_cached_response); + $this->assertTrue($found_cached_200_response); + $this->assertTrue($other_cached_responses_are_4xx); } } else {