.../src/Authentication/Provider/BasicAuth.php | 31 +++++++++++++++++----- .../tests/src/Functional/AnonResourceTestTrait.php | 2 +- .../src/Functional/BasicAuthResourceTestTrait.php | 6 +++-- .../src/Functional/CookieResourceTestTrait.php | 12 +++++++-- .../EntityResource/EntityResourceTestBase.php | 8 +++--- .../rest/tests/src/Functional/ResourceTestBase.php | 7 ++++- 6 files changed, 49 insertions(+), 17 deletions(-) diff --git a/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php b/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php index 7545452..8b59125 100644 --- a/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php +++ b/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php @@ -5,6 +5,7 @@ use Drupal\Component\Utility\SafeMarkup; use Drupal\Core\Authentication\AuthenticationProviderInterface; use Drupal\Core\Authentication\AuthenticationProviderChallengeInterface; +use Drupal\Core\Cache\CacheableMetadata; use Drupal\Core\Cache\Exception\CacheableHttpExceptionInterface; use Drupal\Core\Cache\Exception\CacheableUnauthorizedHttpException; use Drupal\Core\Config\ConfigFactoryInterface; @@ -133,13 +134,29 @@ public function challengeException(Request $request, \Exception $previous) { '@realm' => !empty($site_name) ? $site_name : 'Access restricted', ]); - $exception = new UnauthorizedHttpException((string) $challenge, 'No authentication credentials provided.', $previous); - - // If the previous exception is cacheable, the converted one must be too. - if ($previous instanceof CacheableHttpExceptionInterface) { - $exception = new CacheableUnauthorizedHttpException((string) $challenge, $exception->getMessage(), $exception->getPrevious()); - $exception->addCacheableDependency($previous->getCacheableMetadata()); - } + // 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: + // - Page Cache never caches a response whose request has Basic Auth + // credentials thanks to \Drupal\basic_auth\PageCache\DisallowBasicAuthRequests. + // - 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() + $exception = new CacheableUnauthorizedHttpException((string) $challenge, 'No authentication credentials provided.', $previous); + $exception->addCacheableDependency((new CacheableMetadata()) + ->setCacheTags(['config:user.role.anonymous']) + ->setCacheContexts(['user.roles:anonymous']) + ); return $exception; } diff --git a/core/modules/rest/tests/src/Functional/AnonResourceTestTrait.php b/core/modules/rest/tests/src/Functional/AnonResourceTestTrait.php index b05ddf2..6527433 100644 --- a/core/modules/rest/tests/src/Functional/AnonResourceTestTrait.php +++ b/core/modules/rest/tests/src/Functional/AnonResourceTestTrait.php @@ -24,7 +24,7 @@ /** * {@inheritdoc} */ - protected function assertResponseWhenMissingAuthentication(ResponseInterface $response) { + protected function assertResponseWhenMissingAuthentication($method, ResponseInterface $response) { throw new \LogicException('When testing for anonymous users, authentication cannot be missing.'); } diff --git a/core/modules/rest/tests/src/Functional/BasicAuthResourceTestTrait.php b/core/modules/rest/tests/src/Functional/BasicAuthResourceTestTrait.php index 6f8c621..9fbdfe0 100644 --- a/core/modules/rest/tests/src/Functional/BasicAuthResourceTestTrait.php +++ b/core/modules/rest/tests/src/Functional/BasicAuthResourceTestTrait.php @@ -31,8 +31,10 @@ protected function getAuthenticationRequestOptions($method) { /** * {@inheritdoc} */ - protected function assertResponseWhenMissingAuthentication(ResponseInterface $response) { - $this->assertResourceErrorResponse(401, 'No authentication credentials provided.', $response); + protected function assertResponseWhenMissingAuthentication($method, ResponseInterface $response) { + $expected_page_cache_header_value = $method === 'GET' ? 'MISS' : FALSE; + // @see \Drupal\basic_auth\Authentication\Provider\BasicAuth::challengeException() + $this->assertResourceErrorResponse(401, 'No authentication credentials provided.', $response, ['4xx-response', 'config:user.role.anonymous', 'http_response'], ['user.roles:anonymous'], $expected_page_cache_header_value, $expected_page_cache_header_value); } /** diff --git a/core/modules/rest/tests/src/Functional/CookieResourceTestTrait.php b/core/modules/rest/tests/src/Functional/CookieResourceTestTrait.php index 8975c3f..7cd4ff3 100644 --- a/core/modules/rest/tests/src/Functional/CookieResourceTestTrait.php +++ b/core/modules/rest/tests/src/Functional/CookieResourceTestTrait.php @@ -91,11 +91,19 @@ protected function getAuthenticationRequestOptions($method) { /** * {@inheritdoc} */ - protected function assertResponseWhenMissingAuthentication(ResponseInterface $response) { + protected function assertResponseWhenMissingAuthentication($method, ResponseInterface $response) { // Requests needing cookie authentication but missing it results in a 403 // response. The cookie authentication mechanism sets no response message. + // Hence, effectively, this is just the 403 response that one gets as the + // anonymous user trying to access a certain REST resource. + // @see \Drupal\user\Authentication\Provider\Cookie // @todo https://www.drupal.org/node/2847623 - $this->assertResourceErrorResponse(403, FALSE, $response); + if ($method === 'GET') { + $this->assertResourceErrorResponse(403, FALSE, $response, ['4xx-response', 'config:user.role.anonymous', 'http_response'], ['user.permissions'], 'MISS', 'MISS'); + } + else { + $this->assertResourceErrorResponse(403, FALSE, $response); + } } /** diff --git a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php index 81baf9e..c25c787 100644 --- a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php @@ -353,7 +353,7 @@ public function testGet() { // response. if (static::$auth) { $response = $this->request('GET', $url, $request_options); - $this->assertResponseWhenMissingAuthentication($response); + $this->assertResponseWhenMissingAuthentication('GET', $response); } $request_options[RequestOptions::HEADERS]['REST-test-auth'] = '1'; @@ -669,7 +669,7 @@ public function testPost() { // DX: forgetting authentication: authentication provider-specific error // response. $response = $this->request('POST', $url, $request_options); - $this->assertResponseWhenMissingAuthentication($response); + $this->assertResponseWhenMissingAuthentication('POST', $response); } @@ -869,7 +869,7 @@ public function testPatch() { // DX: forgetting authentication: authentication provider-specific error // response. $response = $this->request('PATCH', $url, $request_options); - $this->assertResponseWhenMissingAuthentication($response); + $this->assertResponseWhenMissingAuthentication('PATCH', $response); } @@ -1024,7 +1024,7 @@ public function testDelete() { // DX: forgetting authentication: authentication provider-specific error // response. $response = $this->request('DELETE', $url, $request_options); - $this->assertResponseWhenMissingAuthentication($response); + $this->assertResponseWhenMissingAuthentication('DELETE', $response); } diff --git a/core/modules/rest/tests/src/Functional/ResourceTestBase.php b/core/modules/rest/tests/src/Functional/ResourceTestBase.php index d00bf1c..6e7d784 100644 --- a/core/modules/rest/tests/src/Functional/ResourceTestBase.php +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php @@ -204,8 +204,13 @@ protected function refreshTestStateAfterRestConfigChange() { /** * Verifies the error response in case of missing authentication. + * + * @param string $method + * HTTP method. + * @param \Psr\Http\Message\ResponseInterface $response + * The response to assert. */ - abstract protected function assertResponseWhenMissingAuthentication(ResponseInterface $response); + abstract protected function assertResponseWhenMissingAuthentication($method, ResponseInterface $response); /** * Asserts normalization-specific edge cases.