.../EventSubscriber/AuthenticationSubscriber.php | 16 +++++ .../RouteAccessResponseSubscriber.php | 8 ++- core/lib/Drupal/Core/Routing/AccessAwareRouter.php | 9 ++- .../src/Authentication/Provider/BasicAuth.php | 5 +- .../block/src/BlockAccessControlHandler.php | 5 +- .../src/Functional/Rest/BlockResourceTestBase.php | 25 +++++--- .../Rest/BlockContentResourceTestBase.php | 4 +- .../Functional/Rest/CommentResourceTestBase.php | 8 +-- .../Functional/Rest/ConfigTestResourceTestBase.php | 16 +++++ .../tests/src/Functional/DbLogResourceTest.php | 2 +- .../src/Functional/Rest/FileResourceTestBase.php | 3 + .../ConfigurableLanguageHalJsonBasicAuthTest.php | 4 +- ...ContentLanguageSettingsHalJsonBasicAuthTest.php | 4 +- .../Rest/ConfigurableLanguageJsonBasicAuthTest.php | 4 +- .../Rest/ConfigurableLanguageXmlBasicAuthTest.php | 4 +- .../ContentLanguageSettingsJsonBasicAuthTest.php | 4 +- .../ContentLanguageSettingsXmlBasicAuthTest.php | 4 +- .../src/Functional/Rest/MediaResourceTestBase.php | 4 +- .../src/MenuLinkContentAccessControlHandler.php | 3 +- .../Rest/MenuLinkContentResourceTestBase.php | 2 +- .../src/Plugin/rest/resource/EntityResource.php | 28 ++++----- core/modules/rest/src/Routing/ResourceRoutes.php | 4 +- .../config_test_rest/config_test_rest.module | 7 ++- .../src/Functional/BasicAuthResourceTestTrait.php | 21 +++++-- ...thResourceWithInterfaceTranslationTestTrait.php | 28 --------- .../src/Functional/CookieResourceTestTrait.php | 6 +- .../EntityResource/EntityResourceTestBase.php | 69 ++++++++++++++++------ .../Functional/Rest/SearchPageResourceTestBase.php | 4 +- .../src/ShortcutSetAccessControlHandler.php | 2 +- .../Rest/ShortcutSetResourceTestBase.php | 16 +++++ core/modules/user/src/UserAccessControlHandler.php | 10 +++- .../src/Functional/Rest/UserResourceTestBase.php | 8 +-- 32 files changed, 221 insertions(+), 116 deletions(-) diff --git a/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php index be3f55e..8ab7f50 100644 --- a/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php +++ b/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php @@ -125,6 +125,21 @@ public function onExceptionSendChallenge(GetResponseForExceptionEvent $event) { } /** + * Detect disallowed authentication methods on access denied exceptions. + * + * @param \Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent $event + */ + public function onExceptionAccessDenied(GetResponseForExceptionEvent $event) { + if (isset($this->filter) && $event->getRequestType() === HttpKernelInterface::MASTER_REQUEST) { + $request = $event->getRequest(); + $exception = $event->getException(); + if ($exception instanceof AccessDeniedHttpException && $this->authenticationProvider->applies($request) && !$this->filter->appliesToRoutedRequest($request, TRUE)) { + $event->setException(new AccessDeniedHttpException('The used authentication method is not allowed on this route.', $exception)); + } + } + } + + /** * {@inheritdoc} */ public static function getSubscribedEvents() { @@ -137,6 +152,7 @@ public static function getSubscribedEvents() { // Access check must be performed after routing. $events[KernelEvents::REQUEST][] = ['onKernelRequestFilterProvider', 31]; $events[KernelEvents::EXCEPTION][] = ['onExceptionSendChallenge', 75]; + $events[KernelEvents::EXCEPTION][] = ['onExceptionAccessDenied', 75]; return $events; } diff --git a/core/lib/Drupal/Core/EventSubscriber/RouteAccessResponseSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/RouteAccessResponseSubscriber.php index b2f1312..e0305b7 100644 --- a/core/lib/Drupal/Core/EventSubscriber/RouteAccessResponseSubscriber.php +++ b/core/lib/Drupal/Core/EventSubscriber/RouteAccessResponseSubscriber.php @@ -48,9 +48,11 @@ public function onRespond(FilterResponseEvent $event) { * {@inheritdoc} */ public static function getSubscribedEvents() { - // Priority 10, so that it runs before FinishResponseSubscriber, which will - // expose the cacheability metadata in the form of headers. - $events[KernelEvents::RESPONSE][] = ['onRespond', 10]; + // Priority 110, so that it runs before FinishResponseSubscriber (0), which + // will expose the cacheability metadata in the form of headers, but also + // before DynamicPageCacheSubscriber (100), otherwise Dynamic Page Cache + // does not take the cacheability of route access into account. + $events[KernelEvents::RESPONSE][] = ['onRespond', 110]; return $events; } diff --git a/core/lib/Drupal/Core/Routing/AccessAwareRouter.php b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php index 8f9e7be..1a93b42 100644 --- a/core/lib/Drupal/Core/Routing/AccessAwareRouter.php +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php @@ -4,6 +4,8 @@ use Drupal\Core\Access\AccessManagerInterface; use Drupal\Core\Access\AccessResultReasonInterface; +use Drupal\Core\Cache\CacheableDependencyInterface; +use Drupal\Core\Http\Exception\CacheableAccessDeniedHttpException; use Drupal\Core\Session\AccountInterface; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; @@ -111,7 +113,12 @@ protected function checkAccess(Request $request) { $request->attributes->set(AccessAwareRouterInterface::ACCESS_RESULT, $access_result); } if (!$access_result->isAllowed()) { - throw new AccessDeniedHttpException($access_result instanceof AccessResultReasonInterface ? $access_result->getReason() : NULL); + if ($access_result instanceof CacheableDependencyInterface && $request->isMethodCacheable()) { + throw new CacheableAccessDeniedHttpException($access_result, $access_result instanceof AccessResultReasonInterface ? $access_result->getReason() : NULL); + } + else { + throw new AccessDeniedHttpException($access_result instanceof AccessResultReasonInterface ? $access_result->getReason() : NULL); + } } } diff --git a/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php b/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php index 48732bc..72b81a7 100644 --- a/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php +++ b/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php @@ -12,6 +12,7 @@ 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. @@ -155,7 +156,9 @@ public function challengeException(Request $request, \Exception $previous) { $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); + return $request->isMethodCacheable() + ? new CacheableUnauthorizedHttpException($cacheability, (string) $challenge, 'No authentication credentials provided.', $previous) + : new UnauthorizedHttpException((string) $challenge, 'No authentication credentials provided.', $previous); } } diff --git a/core/modules/block/src/BlockAccessControlHandler.php b/core/modules/block/src/BlockAccessControlHandler.php index 2652251..1c08f46 100644 --- a/core/modules/block/src/BlockAccessControlHandler.php +++ b/core/modules/block/src/BlockAccessControlHandler.php @@ -127,7 +127,10 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter } } else { - $access = AccessResult::forbidden(); + $reason = count($conditions) > 1 + ? "One of the block visibility conditions ('%s') denied access." + : "The block visibility condition '%s' denied access."; + $access = AccessResult::forbidden(sprintf($reason, implode("', '", array_keys($conditions)))); } $this->mergeCacheabilityFromConditions($access, $conditions); diff --git a/core/modules/block/tests/src/Functional/Rest/BlockResourceTestBase.php b/core/modules/block/tests/src/Functional/Rest/BlockResourceTestBase.php index 998690c..a6af258 100644 --- a/core/modules/block/tests/src/Functional/Rest/BlockResourceTestBase.php +++ b/core/modules/block/tests/src/Functional/Rest/BlockResourceTestBase.php @@ -3,6 +3,7 @@ namespace Drupal\Tests\block\Functional\Rest; use Drupal\block\Entity\Block; +use Drupal\Core\Cache\CacheableMetadata; use Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase; abstract class BlockResourceTestBase extends EntityResourceTestBase { @@ -135,7 +136,7 @@ protected function getExpectedUnauthorizedAccessMessage($method) { switch ($method) { case 'GET': - return "You are not authorized to view this block entity."; + return "The block visibility condition 'user_role' denied access."; default: return parent::getExpectedUnauthorizedAccessMessage($method); } @@ -143,17 +144,25 @@ protected function getExpectedUnauthorizedAccessMessage($method) { /** * {@inheritdoc} + * + * @todo Fix this in https://www.drupal.org/node/2820315. */ protected function getExpectedUnauthorizedAccessCacheability() { + return (new CacheableMetadata()) + ->setCacheTags(['4xx-response', 'http_response']) + ->setCacheContexts(['user.roles']); + } + + /** + * {@inheritdoc} + */ + protected function getExpectedUnauthorizedEntityAccessCacheability($is_authenticated) { // @see \Drupal\block\BlockAccessControlHandler::checkAccess() - return parent::getExpectedUnauthorizedAccessCacheability() - ->setCacheTags([ - '4xx-response', + return parent::getExpectedUnauthorizedEntityAccessCacheability($is_authenticated) + ->addCacheTags([ 'config:block.block.llama', - 'http_response', - static::$auth ? 'user:2' : 'user:0', - ]) - ->setCacheContexts(['user.roles']); + $is_authenticated ? 'user:2' : 'user:0', + ]); } } diff --git a/core/modules/block_content/tests/src/Functional/Rest/BlockContentResourceTestBase.php b/core/modules/block_content/tests/src/Functional/Rest/BlockContentResourceTestBase.php index c77585e..d8e57d7 100644 --- a/core/modules/block_content/tests/src/Functional/Rest/BlockContentResourceTestBase.php +++ b/core/modules/block_content/tests/src/Functional/Rest/BlockContentResourceTestBase.php @@ -175,9 +175,9 @@ protected function getExpectedUnauthorizedAccessMessage($method) { /** * {@inheritdoc} */ - protected function getExpectedUnauthorizedAccessCacheability() { + protected function getExpectedUnauthorizedEntityAccessCacheability($is_authenticated) { // @see \Drupal\block_content\BlockContentAccessControlHandler() - return parent::getExpectedUnauthorizedAccessCacheability() + return parent::getExpectedUnauthorizedEntityAccessCacheability($is_authenticated) ->addCacheTags(['block_content:1']); } diff --git a/core/modules/comment/tests/src/Functional/Rest/CommentResourceTestBase.php b/core/modules/comment/tests/src/Functional/Rest/CommentResourceTestBase.php index 7d1abdb..36970b5 100644 --- a/core/modules/comment/tests/src/Functional/Rest/CommentResourceTestBase.php +++ b/core/modules/comment/tests/src/Functional/Rest/CommentResourceTestBase.php @@ -337,8 +337,8 @@ protected function getExpectedUnauthorizedAccessMessage($method) { return "The 'post comments' permission is required."; case 'PATCH'; return "The 'edit own comments' permission is required, the user must be the comment author, and the comment must be published."; - default: - return parent::getExpectedUnauthorizedAccessMessage($method); + case 'DELETE': + return ''; } } @@ -378,9 +378,9 @@ public function testPostSkipCommentApproval() { /** * {@inheritdoc} */ - protected function getExpectedUnauthorizedAccessCacheability() { + protected function getExpectedUnauthorizedEntityAccessCacheability($is_authenticated) { // @see \Drupal\comment\CommentAccessControlHandler::checkAccess() - return parent::getExpectedUnauthorizedAccessCacheability() + return parent::getExpectedUnauthorizedEntityAccessCacheability($is_authenticated) ->addCacheTags(['comment:1']); } diff --git a/core/modules/config/tests/config_test/tests/src/Functional/Rest/ConfigTestResourceTestBase.php b/core/modules/config/tests/config_test/tests/src/Functional/Rest/ConfigTestResourceTestBase.php index 9ab65d7..dbd71b7 100644 --- a/core/modules/config/tests/config_test/tests/src/Functional/Rest/ConfigTestResourceTestBase.php +++ b/core/modules/config/tests/config_test/tests/src/Functional/Rest/ConfigTestResourceTestBase.php @@ -70,4 +70,20 @@ protected function getNormalizedPostEntity() { // @todo Update in https://www.drupal.org/node/2300677. } + /** + * {@inheritdoc} + */ + protected function getExpectedUnauthorizedAccessMessage($method) { + if ($this->config('rest.settings')->get('bc_entity_resource_permissions')) { + return parent::getExpectedUnauthorizedAccessMessage($method); + } + + switch ($method) { + case 'GET': + return "The 'view config_test' permission is required."; + default: + return parent::getExpectedUnauthorizedAccessMessage($method); + } + } + } diff --git a/core/modules/dblog/tests/src/Functional/DbLogResourceTest.php b/core/modules/dblog/tests/src/Functional/DbLogResourceTest.php index 0d46303..904cd29 100644 --- a/core/modules/dblog/tests/src/Functional/DbLogResourceTest.php +++ b/core/modules/dblog/tests/src/Functional/DbLogResourceTest.php @@ -66,7 +66,7 @@ public function testWatchdog() { $request_options = $this->getAuthenticationRequestOptions('GET'); $response = $this->request('GET', $url, $request_options); - $this->assertResourceErrorResponse(403, "The 'restful get dblog' permission is required.", $response); + $this->assertResourceErrorResponse(403, "The 'restful get dblog' permission is required.", $response, ['4xx-response', 'http_response'], ['user.permissions'], FALSE, FALSE); // Create a user account that has the required permissions to read // the watchdog resource via the REST API. diff --git a/core/modules/file/tests/src/Functional/Rest/FileResourceTestBase.php b/core/modules/file/tests/src/Functional/Rest/FileResourceTestBase.php index 04b7c87..d36f5b3 100644 --- a/core/modules/file/tests/src/Functional/Rest/FileResourceTestBase.php +++ b/core/modules/file/tests/src/Functional/Rest/FileResourceTestBase.php @@ -218,6 +218,9 @@ public function testPost() { */ protected function getExpectedUnauthorizedAccessMessage($method) { if ($this->config('rest.settings')->get('bc_entity_resource_permissions')) { + if ($method === 'DELETE') { + return 'Only the file owner can update or delete the file entity.'; + } return parent::getExpectedUnauthorizedAccessMessage($method); } diff --git a/core/modules/language/tests/src/Functional/Hal/ConfigurableLanguageHalJsonBasicAuthTest.php b/core/modules/language/tests/src/Functional/Hal/ConfigurableLanguageHalJsonBasicAuthTest.php index 3239b7e..4647acb 100644 --- a/core/modules/language/tests/src/Functional/Hal/ConfigurableLanguageHalJsonBasicAuthTest.php +++ b/core/modules/language/tests/src/Functional/Hal/ConfigurableLanguageHalJsonBasicAuthTest.php @@ -3,14 +3,14 @@ namespace Drupal\Tests\language\Functional\Hal; use Drupal\Tests\language\Functional\Rest\ConfigurableLanguageResourceTestBase; -use Drupal\Tests\rest\Functional\BasicAuthResourceWithInterfaceTranslationTestTrait; +use Drupal\Tests\rest\Functional\BasicAuthResourceTestTrait; /** * @group hal */ class ConfigurableLanguageHalJsonBasicAuthTest extends ConfigurableLanguageResourceTestBase { - use BasicAuthResourceWithInterfaceTranslationTestTrait; + use BasicAuthResourceTestTrait; /** * {@inheritdoc} diff --git a/core/modules/language/tests/src/Functional/Hal/ContentLanguageSettingsHalJsonBasicAuthTest.php b/core/modules/language/tests/src/Functional/Hal/ContentLanguageSettingsHalJsonBasicAuthTest.php index 61306ea..fefd0db 100644 --- a/core/modules/language/tests/src/Functional/Hal/ContentLanguageSettingsHalJsonBasicAuthTest.php +++ b/core/modules/language/tests/src/Functional/Hal/ContentLanguageSettingsHalJsonBasicAuthTest.php @@ -3,14 +3,14 @@ namespace Drupal\Tests\language\Functional\Hal; use Drupal\Tests\language\Functional\Rest\ContentLanguageSettingsResourceTestBase; -use Drupal\Tests\rest\Functional\BasicAuthResourceWithInterfaceTranslationTestTrait; +use Drupal\Tests\rest\Functional\BasicAuthResourceTestTrait; /** * @group hal */ class ContentLanguageSettingsHalJsonBasicAuthTest extends ContentLanguageSettingsResourceTestBase { - use BasicAuthResourceWithInterfaceTranslationTestTrait; + use BasicAuthResourceTestTrait; /** * {@inheritdoc} diff --git a/core/modules/language/tests/src/Functional/Rest/ConfigurableLanguageJsonBasicAuthTest.php b/core/modules/language/tests/src/Functional/Rest/ConfigurableLanguageJsonBasicAuthTest.php index 847e781..5352c9a 100644 --- a/core/modules/language/tests/src/Functional/Rest/ConfigurableLanguageJsonBasicAuthTest.php +++ b/core/modules/language/tests/src/Functional/Rest/ConfigurableLanguageJsonBasicAuthTest.php @@ -2,14 +2,14 @@ namespace Drupal\Tests\language\Functional\Rest; -use Drupal\Tests\rest\Functional\BasicAuthResourceWithInterfaceTranslationTestTrait; +use Drupal\Tests\rest\Functional\BasicAuthResourceTestTrait; /** * @group rest */ class ConfigurableLanguageJsonBasicAuthTest extends ConfigurableLanguageResourceTestBase { - use BasicAuthResourceWithInterfaceTranslationTestTrait; + use BasicAuthResourceTestTrait; /** * {@inheritdoc} diff --git a/core/modules/language/tests/src/Functional/Rest/ConfigurableLanguageXmlBasicAuthTest.php b/core/modules/language/tests/src/Functional/Rest/ConfigurableLanguageXmlBasicAuthTest.php index 7a79c4d..afc017f 100644 --- a/core/modules/language/tests/src/Functional/Rest/ConfigurableLanguageXmlBasicAuthTest.php +++ b/core/modules/language/tests/src/Functional/Rest/ConfigurableLanguageXmlBasicAuthTest.php @@ -2,7 +2,7 @@ namespace Drupal\Tests\language\Functional\Rest; -use Drupal\Tests\rest\Functional\BasicAuthResourceWithInterfaceTranslationTestTrait; +use Drupal\Tests\rest\Functional\BasicAuthResourceTestTrait; use Drupal\Tests\rest\Functional\EntityResource\XmlEntityNormalizationQuirksTrait; /** @@ -10,7 +10,7 @@ */ class ConfigurableLanguageXmlBasicAuthTest extends ConfigurableLanguageResourceTestBase { - use BasicAuthResourceWithInterfaceTranslationTestTrait; + use BasicAuthResourceTestTrait; use XmlEntityNormalizationQuirksTrait; /** diff --git a/core/modules/language/tests/src/Functional/Rest/ContentLanguageSettingsJsonBasicAuthTest.php b/core/modules/language/tests/src/Functional/Rest/ContentLanguageSettingsJsonBasicAuthTest.php index 14437a9..f1063a3 100644 --- a/core/modules/language/tests/src/Functional/Rest/ContentLanguageSettingsJsonBasicAuthTest.php +++ b/core/modules/language/tests/src/Functional/Rest/ContentLanguageSettingsJsonBasicAuthTest.php @@ -2,14 +2,14 @@ namespace Drupal\Tests\language\Functional\Rest; -use Drupal\Tests\rest\Functional\BasicAuthResourceWithInterfaceTranslationTestTrait; +use Drupal\Tests\rest\Functional\BasicAuthResourceTestTrait; /** * @group rest */ class ContentLanguageSettingsJsonBasicAuthTest extends ContentLanguageSettingsResourceTestBase { - use BasicAuthResourceWithInterfaceTranslationTestTrait; + use BasicAuthResourceTestTrait; /** * {@inheritdoc} diff --git a/core/modules/language/tests/src/Functional/Rest/ContentLanguageSettingsXmlBasicAuthTest.php b/core/modules/language/tests/src/Functional/Rest/ContentLanguageSettingsXmlBasicAuthTest.php index 741e75b..dee7ad2 100644 --- a/core/modules/language/tests/src/Functional/Rest/ContentLanguageSettingsXmlBasicAuthTest.php +++ b/core/modules/language/tests/src/Functional/Rest/ContentLanguageSettingsXmlBasicAuthTest.php @@ -2,7 +2,7 @@ namespace Drupal\Tests\language\Functional\Rest; -use Drupal\Tests\rest\Functional\BasicAuthResourceWithInterfaceTranslationTestTrait; +use Drupal\Tests\rest\Functional\BasicAuthResourceTestTrait; use Drupal\Tests\rest\Functional\EntityResource\XmlEntityNormalizationQuirksTrait; /** @@ -10,7 +10,7 @@ */ class ContentLanguageSettingsXmlBasicAuthTest extends ContentLanguageSettingsResourceTestBase { - use BasicAuthResourceWithInterfaceTranslationTestTrait; + use BasicAuthResourceTestTrait; use XmlEntityNormalizationQuirksTrait; /** diff --git a/core/modules/media/tests/src/Functional/Rest/MediaResourceTestBase.php b/core/modules/media/tests/src/Functional/Rest/MediaResourceTestBase.php index 85e4e5d..ee9a84e 100644 --- a/core/modules/media/tests/src/Functional/Rest/MediaResourceTestBase.php +++ b/core/modules/media/tests/src/Functional/Rest/MediaResourceTestBase.php @@ -422,9 +422,9 @@ protected function getExpectedNormalizedFileEntity() { /** * {@inheritdoc} */ - protected function getExpectedUnauthorizedAccessCacheability() { + protected function getExpectedUnauthorizedEntityAccessCacheability($is_authenticated) { // @see \Drupal\media\MediaAccessControlHandler::checkAccess() - return parent::getExpectedUnauthorizedAccessCacheability() + return parent::getExpectedUnauthorizedEntityAccessCacheability($is_authenticated) ->addCacheTags(['media:1']); } diff --git a/core/modules/menu_link_content/src/MenuLinkContentAccessControlHandler.php b/core/modules/menu_link_content/src/MenuLinkContentAccessControlHandler.php index eadf045..b663f27 100644 --- a/core/modules/menu_link_content/src/MenuLinkContentAccessControlHandler.php +++ b/core/modules/menu_link_content/src/MenuLinkContentAccessControlHandler.php @@ -72,7 +72,8 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter } case 'delete': - return AccessResult::allowedIf(!$entity->isNew() && $account->hasPermission('administer menu'))->cachePerPermissions()->addCacheableDependency($entity); + return AccessResult::allowedIfHasPermission($account, 'administer menu') + ->andIf(AccessResult::allowedIf(!$entity->isNew())->addCacheableDependency($entity)); } } diff --git a/core/modules/menu_link_content/tests/src/Functional/Rest/MenuLinkContentResourceTestBase.php b/core/modules/menu_link_content/tests/src/Functional/Rest/MenuLinkContentResourceTestBase.php index 31b89ae..ee07ec3 100644 --- a/core/modules/menu_link_content/tests/src/Functional/Rest/MenuLinkContentResourceTestBase.php +++ b/core/modules/menu_link_content/tests/src/Functional/Rest/MenuLinkContentResourceTestBase.php @@ -185,7 +185,7 @@ protected function getExpectedUnauthorizedAccessMessage($method) { switch ($method) { case 'DELETE': - return "You are not authorized to delete this menu_link_content entity."; + return "The 'administer menu' permission is required."; default: return parent::getExpectedUnauthorizedAccessMessage($method); } diff --git a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php index 9dce5df..db6b0a1 100644 --- a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php @@ -13,7 +13,6 @@ use Drupal\Core\Entity\EntityInterface; use Drupal\Core\Entity\EntityStorageException; use Drupal\Core\Field\FieldItemListInterface; -use Drupal\Core\Http\Exception\CacheableAccessDeniedHttpException; use Drupal\rest\Plugin\ResourceBase; use Drupal\rest\ResourceResponse; use Psr\Log\LoggerInterface; @@ -121,14 +120,8 @@ public static function create(ContainerInterface $container, array $configuratio * @throws \Symfony\Component\HttpKernel\Exception\HttpException */ public function get(EntityInterface $entity) { - $entity_access = $entity->access('view', NULL, TRUE); - if (!$entity_access->isAllowed()) { - throw new CacheableAccessDeniedHttpException($entity_access, $entity_access->getReason() ?: $this->generateFallbackAccessDeniedMessage($entity, 'view')); - } - $response = new ResourceResponse($entity, 200); $response->addCacheableDependency($entity); - $response->addCacheableDependency($entity_access); if ($entity instanceof FieldableEntityInterface) { foreach ($entity as $field_name => $field) { @@ -223,10 +216,6 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity if ($entity->getEntityTypeId() != $definition['entity_type']) { throw new BadRequestHttpException('Invalid entity type'); } - $entity_access = $original_entity->access('update', NULL, TRUE); - if (!$entity_access->isAllowed()) { - throw new AccessDeniedHttpException($entity_access->getReason() ?: $this->generateFallbackAccessDeniedMessage($entity, 'update')); - } // Overwrite the received fields. foreach ($entity->_restSubmittedFields as $field_name) { @@ -319,10 +308,6 @@ protected function checkPatchFieldAccess(FieldItemListInterface $original_field, * @throws \Symfony\Component\HttpKernel\Exception\HttpException */ public function delete(EntityInterface $entity) { - $entity_access = $entity->access('delete', NULL, TRUE); - if (!$entity_access->isAllowed()) { - throw new AccessDeniedHttpException($entity_access->getReason() ?: $this->generateFallbackAccessDeniedMessage($entity, 'delete')); - } try { $entity->delete(); $this->logger->notice('Deleted entity %type with ID %id.', ['%type' => $entity->getEntityTypeId(), '%id' => $entity->id()]); @@ -375,6 +360,19 @@ public function permissions() { */ protected function getBaseRoute($canonical_path, $method) { $route = parent::getBaseRoute($canonical_path, $method); + + switch ($method) { + case 'GET': + $route->setRequirement('_entity_access', $this->entityType->id() . '.view'); + break; + case 'PATCH': + $route->setRequirement('_entity_access', $this->entityType->id() . '.update'); + break; + case 'DELETE': + $route->setRequirement('_entity_access', $this->entityType->id() . '.delete'); + break; + } + $definition = $this->getPluginDefinition(); $parameters = $route->getOption('parameters') ?: []; diff --git a/core/modules/rest/src/Routing/ResourceRoutes.php b/core/modules/rest/src/Routing/ResourceRoutes.php index 27da48a..01a7fd0 100644 --- a/core/modules/rest/src/Routing/ResourceRoutes.php +++ b/core/modules/rest/src/Routing/ResourceRoutes.php @@ -97,7 +97,9 @@ protected function getRoutesForResourceConfig(RestResourceConfigInterface $rest_ // - that exist for BC // @see \Drupal\rest\RouteProcessor\RestResourceGetRouteProcessorBC if (($methods && ($method = $methods[0]) && $supported_formats = $rest_resource_config->getFormats($method)) || $route->hasOption('bc_route')) { - $route->setRequirement('_csrf_request_header_token', 'TRUE'); + if (!in_array($method, ['GET', 'HEAD'], TRUE)) { + $route->setRequirement('_csrf_request_header_token', 'TRUE'); + } // Check that authentication providers are defined. if (empty($rest_resource_config->getAuthenticationProviders($method))) { diff --git a/core/modules/rest/tests/modules/config_test_rest/config_test_rest.module b/core/modules/rest/tests/modules/config_test_rest/config_test_rest.module index fcd9979..aa9faba 100644 --- a/core/modules/rest/tests/modules/config_test_rest/config_test_rest.module +++ b/core/modules/rest/tests/modules/config_test_rest/config_test_rest.module @@ -6,6 +6,7 @@ */ use Drupal\Core\Access\AccessResult; +use Drupal\Core\Access\AccessResultReasonInterface; use Drupal\Core\Entity\EntityInterface; use Drupal\Core\Session\AccountInterface; @@ -26,5 +27,9 @@ function config_test_rest_config_test_access(EntityInterface $entity, $operation // Add permission, so that EntityResourceTestBase's scenarios can test access // being denied. By default, all access is always allowed for the config_test // config entity. - return AccessResult::forbiddenIf(!$account->hasPermission('view config_test'))->cachePerPermissions(); + $access_result = AccessResult::forbiddenIf(!$account->hasPermission('view config_test'))->cachePerPermissions(); + if (!$access_result->isAllowed() && $access_result instanceof AccessResultReasonInterface) { + $access_result->setReason("The 'view config_test' permission is required."); + } + return $access_result; } diff --git a/core/modules/rest/tests/src/Functional/BasicAuthResourceTestTrait.php b/core/modules/rest/tests/src/Functional/BasicAuthResourceTestTrait.php index e4dcd70..4a58254 100644 --- a/core/modules/rest/tests/src/Functional/BasicAuthResourceTestTrait.php +++ b/core/modules/rest/tests/src/Functional/BasicAuthResourceTestTrait.php @@ -14,8 +14,6 @@ * authenticated, a 401 response must be sent. * - Because every request must send an authorization, there is no danger of * CSRF attacks. - * - * @see \Drupal\Tests\rest\Functional\BasicAuthResourceWithInterfaceTranslationTestTrait */ trait BasicAuthResourceTestTrait { @@ -34,10 +32,23 @@ protected function getAuthenticationRequestOptions($method) { * {@inheritdoc} */ protected function assertResponseWhenMissingAuthentication($method, ResponseInterface $response) { + if ($method !== 'GET') { + return $this->assertResourceErrorResponse(401, 'No authentication credentials provided.', $response); + } + $expected_page_cache_header_value = $method === 'GET' ? 'MISS' : FALSE; - // @see \Drupal\basic_auth\Authentication\Provider\BasicAuth::challengeException() - $expected_dynamic_page_cache_header_value = $expected_page_cache_header_value; - $this->assertResourceErrorResponse(401, 'No authentication credentials provided.', $response, ['4xx-response', 'config:system.site', 'config:user.role.anonymous', 'http_response'], ['user.roles:anonymous'], $expected_page_cache_header_value, $expected_dynamic_page_cache_header_value); + $expected_cacheability = $this->getExpectedUnauthorizedAccessCacheability() + ->addCacheableDependency($this->getExpectedUnauthorizedEntityAccessCacheability(FALSE)) + // @see \Drupal\basic_auth\Authentication\Provider\BasicAuth::challengeException() + ->addCacheableDependency($this->config('system.site')) + // @see \Drupal\Core\EventSubscriber\AnonymousUserResponseSubscriber::onRespond() + ->addCacheTags(['config:user.role.anonymous']); + // Only add the 'user.roles:anonymous' cache context if its parent cache + // context is not already present. + if (!in_array('user.roles', $expected_cacheability->getCacheContexts(), TRUE)) { + $expected_cacheability->addCacheContexts(['user.roles:anonymous']); + } + $this->assertResourceErrorResponse(401, 'No authentication credentials provided.', $response, $expected_cacheability->getCacheTags(), $expected_cacheability->getCacheContexts(), $expected_page_cache_header_value, FALSE); } /** diff --git a/core/modules/rest/tests/src/Functional/BasicAuthResourceWithInterfaceTranslationTestTrait.php b/core/modules/rest/tests/src/Functional/BasicAuthResourceWithInterfaceTranslationTestTrait.php deleted file mode 100644 index 37b8381..0000000 --- a/core/modules/rest/tests/src/Functional/BasicAuthResourceWithInterfaceTranslationTestTrait.php +++ /dev/null @@ -1,28 +0,0 @@ -assertResourceErrorResponse(401, 'No authentication credentials provided.', $response, ['4xx-response', 'config:system.site', 'config:user.role.anonymous', 'http_response'], ['languages:language_interface', '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 7ce381b..2d25b94 100644 --- a/core/modules/rest/tests/src/Functional/CookieResourceTestTrait.php +++ b/core/modules/rest/tests/src/Functional/CookieResourceTestTrait.php @@ -99,7 +99,9 @@ protected function assertResponseWhenMissingAuthentication($method, ResponseInte // @see \Drupal\user\Authentication\Provider\Cookie // @todo https://www.drupal.org/node/2847623 if ($method === 'GET') { - $expected_cookie_403_cacheability = $this->getExpectedUnauthorizedAccessCacheability(); + $expected_cookie_403_cacheability = $this->getExpectedUnauthorizedAccessCacheability() + // @see \Drupal\Core\EventSubscriber\AnonymousUserResponseSubscriber::onRespond() + ->addCacheableDependency($this->getExpectedUnauthorizedEntityAccessCacheability(FALSE)); // - \Drupal\Core\EventSubscriber\AnonymousUserResponseSubscriber applies // to cacheable anonymous responses: it updates their cacheability. // - A 403 response to a GET request is cacheable. @@ -111,7 +113,7 @@ protected function assertResponseWhenMissingAuthentication($method, ResponseInte if (static::$entityTypeId === 'block') { $expected_cookie_403_cacheability->setCacheTags(str_replace('user:2', 'user:0', $expected_cookie_403_cacheability->getCacheTags())); } - $this->assertResourceErrorResponse(403, FALSE, $response, $expected_cookie_403_cacheability->getCacheTags(), $expected_cookie_403_cacheability->getCacheContexts(), 'MISS', 'MISS'); + $this->assertResourceErrorResponse(403, FALSE, $response, $expected_cookie_403_cacheability->getCacheTags(), $expected_cookie_403_cacheability->getCacheContexts(), 'MISS', FALSE); } 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 76f0139..e41d453 100644 --- a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php @@ -379,6 +379,20 @@ protected function getExpectedUnauthorizedAccessCacheability() { } /** + * The cacheability of unauthorized 'view' entity access. + * + * @param bool $is_authenticated + * Whether the current request is authenticated or not. This matters for + * some entity access control handlers, but not for most. + * + * @return \Drupal\Core\Cache\CacheableMetadata + * The expected cacheability. + */ + protected function getExpectedUnauthorizedEntityAccessCacheability($is_authenticated) { + return new CacheableMetadata(); + } + + /** * The expected cache tags for the GET/HEAD response of the test entity. * * @see ::testGet @@ -440,7 +454,17 @@ public function testGet() { // response because ?_format query string is present. $response = $this->request('GET', $url, $request_options); if ($has_canonical_url) { - $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response); + $expected_cacheability = $this->getExpectedUnauthorizedAccessCacheability() + // @see \Drupal\Core\EventSubscriber\AnonymousUserResponseSubscriber::onRespond() + ->addCacheTags(['config:user.role.anonymous']); + $entity_type = $this->entity->getEntityType(); + // When the canonical URL *is* the same as the edit form's, the canonical + // is just an alias, and hence an 'update' entity access is checked, not + // 'view'. Hence only add entity access cacheability conditionally. + if ($this->entity->hasLinkTemplate('edit-form') && $entity_type->getLinkTemplate('canonical') !== $entity_type->getLinkTemplate('edit-form')) { + $expected_cacheability->addCacheableDependency($this->getExpectedUnauthorizedEntityAccessCacheability(FALSE)); + } + $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response, $expected_cacheability->getCacheTags(), $expected_cacheability->getCacheContexts(), 'MISS', FALSE); } else { $this->assertResourceErrorResponse(404, 'No route found for "GET ' . str_replace($this->baseUrl, '', $this->getEntityResourceUrl()->setAbsolute()->toString()) . '"', $response); @@ -473,7 +497,8 @@ public function testGet() { // First: single format. Drupal will automatically pick the only format. $this->provisionEntityResource(TRUE); - $expected_403_cacheability = $this->getExpectedUnauthorizedAccessCacheability(); + $expected_403_cacheability = $this->getExpectedUnauthorizedAccessCacheability() + ->addCacheableDependency($this->getExpectedUnauthorizedEntityAccessCacheability(static::$auth !== FALSE)); // DX: 403 because unauthorized single-format route, ?_format is omittable. $url->setOption('query', []); $response = $this->request('GET', $url, $request_options); @@ -482,13 +507,13 @@ public function testGet() { $this->assertSame(['text/html; charset=UTF-8'], $response->getHeader('Content-Type')); } else { - $this->assertResourceErrorResponse(403, FALSE, $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', 'MISS'); + $this->assertResourceErrorResponse(403, FALSE, $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', FALSE); } $this->assertSame(static::$auth ? [] : ['MISS'], $response->getHeader('X-Drupal-Cache')); // DX: 403 because unauthorized. $url->setOption('query', ['_format' => static::$format]); $response = $this->request('GET', $url, $request_options); - $this->assertResourceErrorResponse(403, FALSE, $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', $has_canonical_url ? 'MISS' : 'HIT'); + $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. $this->provisionEntityResource(); @@ -508,7 +533,7 @@ public function testGet() { // DX: 403 because unauthorized. $url->setOption('query', ['_format' => static::$format]); $response = $this->request('GET', $url, $request_options); - $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', 'HIT'); + $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()); $this->setUpAuthorization('GET'); @@ -682,7 +707,15 @@ public function testGet() { // DX: 403 when unauthorized. $response = $this->request('GET', $url, $request_options); - $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response); + $expected_403_cacheability = $this->getExpectedUnauthorizedAccessCacheability(); + // Permission checking now happens first, so it's the only cache context we + // could possibly vary by. + $expected_403_cacheability->setCacheContexts(['user.permissions']); + // @see \Drupal\Core\EventSubscriber\AnonymousUserResponseSubscriber::onRespond() + if (static::$auth === FALSE) { + $expected_403_cacheability->addCacheTags(['config:user.role.anonymous']); + } + $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', FALSE); $this->grantPermissionsToTestedRole(['restful get entity:' . static::$entityTypeId]); @@ -1079,18 +1112,6 @@ public function testPatch() { $request_options[RequestOptions::HEADERS]['Content-Type'] = static::$mimeType; - // DX: 400 when no request body. - $response = $this->request('PATCH', $url, $request_options); - $this->assertResourceErrorResponse(400, 'No entity content received.', $response); - - $request_options[RequestOptions::BODY] = $unparseable_request_body; - - // DX: 400 when unparseable request body. - $response = $this->request('PATCH', $url, $request_options); - $this->assertResourceErrorResponse(400, 'Syntax error', $response); - - $request_options[RequestOptions::BODY] = $parseable_invalid_request_body; - if (static::$auth) { // DX: forgetting authentication: authentication provider-specific error // response. @@ -1106,6 +1127,18 @@ public function testPatch() { $this->setUpAuthorization('PATCH'); + // DX: 400 when no request body. + $response = $this->request('PATCH', $url, $request_options); + $this->assertResourceErrorResponse(400, 'No entity content received.', $response); + + $request_options[RequestOptions::BODY] = $unparseable_request_body; + + // DX: 400 when unparseable request body. + $response = $this->request('PATCH', $url, $request_options); + $this->assertResourceErrorResponse(400, 'Syntax error', $response); + + $request_options[RequestOptions::BODY] = $parseable_invalid_request_body; + // DX: 422 when invalid entity: multiple values sent for single-value field. $response = $this->request('PATCH', $url, $request_options); $label_field = $this->entity->getEntityType()->hasKey('label') ? $this->entity->getEntityType()->getKey('label') : static::$labelFieldName; diff --git a/core/modules/search/tests/src/Functional/Rest/SearchPageResourceTestBase.php b/core/modules/search/tests/src/Functional/Rest/SearchPageResourceTestBase.php index e711a34..1730bad 100644 --- a/core/modules/search/tests/src/Functional/Rest/SearchPageResourceTestBase.php +++ b/core/modules/search/tests/src/Functional/Rest/SearchPageResourceTestBase.php @@ -102,9 +102,9 @@ protected function getExpectedUnauthorizedAccessMessage($method) { /** * {@inheritdoc} */ - protected function getExpectedUnauthorizedAccessCacheability() { + protected function getExpectedUnauthorizedEntityAccessCacheability($is_authenticated) { // @see \Drupal\search\SearchPageAccessControlHandler::checkAccess() - return parent::getExpectedUnauthorizedAccessCacheability() + return parent::getExpectedUnauthorizedEntityAccessCacheability($is_authenticated) ->addCacheTags(['config:search.page.hinode_search']); } diff --git a/core/modules/shortcut/src/ShortcutSetAccessControlHandler.php b/core/modules/shortcut/src/ShortcutSetAccessControlHandler.php index 3a55f74..87be25d 100644 --- a/core/modules/shortcut/src/ShortcutSetAccessControlHandler.php +++ b/core/modules/shortcut/src/ShortcutSetAccessControlHandler.php @@ -20,7 +20,7 @@ class ShortcutSetAccessControlHandler extends EntityAccessControlHandler { protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) { switch ($operation) { case 'view': - return AccessResult::allowedIf($account->hasPermission('access shortcuts'))->cachePerPermissions(); + return AccessResult::allowedIfHasPermission($account, 'access shortcuts'); case 'update': if ($account->hasPermission('administer shortcuts')) { return AccessResult::allowed()->cachePerPermissions(); diff --git a/core/modules/shortcut/tests/src/Functional/Rest/ShortcutSetResourceTestBase.php b/core/modules/shortcut/tests/src/Functional/Rest/ShortcutSetResourceTestBase.php index b25f028..1510df2 100644 --- a/core/modules/shortcut/tests/src/Functional/Rest/ShortcutSetResourceTestBase.php +++ b/core/modules/shortcut/tests/src/Functional/Rest/ShortcutSetResourceTestBase.php @@ -85,4 +85,20 @@ protected function getNormalizedPostEntity() { // @todo Update in https://www.drupal.org/node/2300677. } + /** + * {@inheritdoc} + */ + protected function getExpectedUnauthorizedAccessMessage($method) { + if ($this->config('rest.settings')->get('bc_entity_resource_permissions')) { + return parent::getExpectedUnauthorizedAccessMessage($method); + } + + switch ($method) { + case 'GET': + return "The 'access shortcuts' permission is required."; + default: + return parent::getExpectedUnauthorizedAccessMessage($method); + } + } + } diff --git a/core/modules/user/src/UserAccessControlHandler.php b/core/modules/user/src/UserAccessControlHandler.php index 8ff01d1..27438c2 100644 --- a/core/modules/user/src/UserAccessControlHandler.php +++ b/core/modules/user/src/UserAccessControlHandler.php @@ -4,6 +4,7 @@ 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; use Drupal\Core\Field\FieldDefinitionInterface; @@ -64,11 +65,16 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter case 'update': // Users can always edit their own account. - return AccessResult::allowedIf($account->id() == $entity->id())->cachePerUser(); + $access_result = AccessResult::allowedIf($account->id() == $entity->id())->cachePerUser(); + if (!$access_result->isAllowed() && $access_result instanceof AccessResultReasonInterface) { + $access_result->setReason("Users can only update their own account, unless they have the 'administer users' permission."); + } + return $access_result; case 'delete': // Users with 'cancel account' permission can cancel their own account. - return AccessResult::allowedIf($account->id() == $entity->id() && $account->hasPermission('cancel account'))->cachePerPermissions()->cachePerUser(); + return AccessResult::allowedIfHasPermission($account, 'cancel account') + ->andIf(AccessResult::allowedIf($account->id() == $entity->id())->cachePerUser()); } // No opinion. diff --git a/core/modules/user/tests/src/Functional/Rest/UserResourceTestBase.php b/core/modules/user/tests/src/Functional/Rest/UserResourceTestBase.php index d8fe60e..6b544f4 100644 --- a/core/modules/user/tests/src/Functional/Rest/UserResourceTestBase.php +++ b/core/modules/user/tests/src/Functional/Rest/UserResourceTestBase.php @@ -309,9 +309,9 @@ protected function getExpectedUnauthorizedAccessMessage($method) { case 'GET': return "The 'access user profiles' permission is required and the user must be active."; case 'PATCH': - return "You are not authorized to update this user entity."; + return "Users can only update their own account, unless they have the 'administer users' permission."; case 'DELETE': - return 'You are not authorized to delete this user entity.'; + return "The 'cancel account' permission is required."; default: return parent::getExpectedUnauthorizedAccessMessage($method); } @@ -320,9 +320,9 @@ protected function getExpectedUnauthorizedAccessMessage($method) { /** * {@inheritdoc} */ - protected function getExpectedUnauthorizedAccessCacheability() { + protected function getExpectedUnauthorizedEntityAccessCacheability($is_authenticated) { // @see \Drupal\user\UserAccessControlHandler::checkAccess() - return parent::getExpectedUnauthorizedAccessCacheability() + return parent::getExpectedUnauthorizedEntityAccessCacheability($is_authenticated) ->addCacheTags(['user:3']); }