diff --git a/core/lib/Drupal/Core/Access/AccessResult.php b/core/lib/Drupal/Core/Access/AccessResult.php index f67b9f5..d87872a 100644 --- a/core/lib/Drupal/Core/Access/AccessResult.php +++ b/core/lib/Drupal/Core/Access/AccessResult.php @@ -31,17 +31,22 @@ /** * Creates an AccessResultInterface object with isNeutral() === TRUE. * - * @return \Drupal\Core\Access\AccessResult + * @param string|null $reason + * (optional) The reason why access is forbidden. Intended for developers, + * hence not translatable. + * + * @return \Drupal\Core\Access\AccessResultNeutral * isNeutral() will be TRUE. */ - public static function neutral() { - return new AccessResultNeutral(); + public static function neutral($reason = NULL) { + assert('is_string($reason) || is_null($reason)'); + return new AccessResultNeutral($reason); } /** * Creates an AccessResultInterface object with isAllowed() === TRUE. * - * @return \Drupal\Core\Access\AccessResult + * @return \Drupal\Core\Access\AccessResultAllowed * isAllowed() will be TRUE. */ public static function allowed() { @@ -55,7 +60,7 @@ public static function allowed() { * (optional) The reason why access is forbidden. Intended for developers, * hence not translatable. * - * @return \Drupal\Core\Access\AccessResult + * @return \Drupal\Core\Access\AccessResultForbidden * isForbidden() will be TRUE. */ public static function forbidden($reason = NULL) { @@ -106,7 +111,12 @@ public static function forbiddenIf($condition) { * isNeutral() will be TRUE. */ public static function allowedIfHasPermission(AccountInterface $account, $permission) { - return static::allowedIf($account->hasPermission($permission))->addCacheContexts(['user.permissions']); + $access_result = static::allowedIf($account->hasPermission($permission))->addCacheContexts(['user.permissions']); + + if ($access_result instanceof AccessResultReasonInterface) { + $access_result->setReason("The '$permission' permission is required."); + } + return $access_result; } /** @@ -147,7 +157,21 @@ public static function allowedIfHasPermissions(AccountInterface $account, array } } - return static::allowedIf($access)->addCacheContexts(empty($permissions) ? [] : ['user.permissions']); + $access_result = static::allowedIf($access)->addCacheContexts(empty($permissions) ? [] : ['user.permissions']); + + if ($access_result instanceof AccessResultReasonInterface) { + if (count($permissions) === 1) { + $access_result->setReason("The '$permission' permission is required."); + } + elseif (count($permissions) > 1) { + $quote = function ($s) { + return "'$s'"; + }; + $access_result->setReason(sprintf("The following permissions are required: %s.", implode(" $conjunction ", array_map($quote, $permissions)))); + } + } + + return $access_result; } /** @@ -308,6 +332,13 @@ public function orIf(AccessResultInterface $other) { if (!$this->isForbidden() || ($this->getCacheMaxAge() === 0 && $other->isForbidden())) { $merge_other = TRUE; } + + if ($this->isForbidden() && $this instanceof AccessResultReasonInterface) { + $result->setReason($this->getReason()); + } + elseif ($other->isForbidden() && $other instanceof AccessResultReasonInterface) { + $result->setReason($other->getReason()); + } } elseif ($this->isAllowed() || $other->isAllowed()) { $result = static::allowed(); @@ -319,6 +350,14 @@ public function orIf(AccessResultInterface $other) { $result = static::neutral(); if (!$this->isNeutral() || ($this->getCacheMaxAge() === 0 && $other->isNeutral()) || ($this->getCacheMaxAge() !== 0 && $other instanceof CacheableDependencyInterface && $other->getCacheMaxAge() !== 0)) { $merge_other = TRUE; + if ($other instanceof AccessResultReasonInterface) { + $result->setReason($other->getReason()); + } + } + else { + if ($this instanceof AccessResultReasonInterface) { + $result->setReason($this->getReason()); + } } } $result->inheritCacheability($this); @@ -358,6 +397,14 @@ public function andIf(AccessResultInterface $other) { $result = static::neutral(); if (!$this->isNeutral()) { $merge_other = TRUE; + if ($other instanceof AccessResultReasonInterface) { + $result->setReason($other->getReason()); + } + } + else { + if ($this instanceof AccessResultReasonInterface) { + $result->setReason($this->getReason()); + } } } $result->inheritCacheability($this); diff --git a/core/lib/Drupal/Core/Access/AccessResultNeutral.php b/core/lib/Drupal/Core/Access/AccessResultNeutral.php index 7a180f8..2fb75eb 100644 --- a/core/lib/Drupal/Core/Access/AccessResultNeutral.php +++ b/core/lib/Drupal/Core/Access/AccessResultNeutral.php @@ -5,7 +5,24 @@ /** * Value object indicating a neutral access result, with cacheability metadata. */ -class AccessResultNeutral extends AccessResult { +class AccessResultNeutral extends AccessResult implements AccessResultReasonInterface { + + /** + * The reason why access is neutral. For use in messages. + * + * @var string|null + */ + protected $reason; + + /** + * Constructs a new AccessResultNeutral instance. + * + * @param null|string $reason + * (optional) a message to provide details about this access result + */ + public function __construct($reason = NULL) { + $this->reason = $reason; + } /** * {@inheritdoc} @@ -14,4 +31,19 @@ public function isNeutral() { return TRUE; } + /** + * {@inheritdoc} + */ + public function getReason() { + return $this->reason; + } + + /** + * {@inheritdoc} + */ + public function setReason($reason) { + $this->reason = $reason; + return $this; + } + } diff --git a/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php index 62f5486..be3f55e 100644 --- a/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php +++ b/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php @@ -96,7 +96,7 @@ public function onKernelRequestFilterProvider(GetResponseEvent $event) { if (isset($this->filter) && $event->getRequestType() === HttpKernelInterface::MASTER_REQUEST) { $request = $event->getRequest(); if ($this->authenticationProvider->applies($request) && !$this->filter->appliesToRoutedRequest($request, TRUE)) { - throw new AccessDeniedHttpException(); + throw new AccessDeniedHttpException('The used authentication method is not allowed on this route.'); } } } diff --git a/core/modules/basic_auth/src/Tests/Authentication/BasicAuthTest.php b/core/modules/basic_auth/src/Tests/Authentication/BasicAuthTest.php index 2aca988..04bb0fd 100644 --- a/core/modules/basic_auth/src/Tests/Authentication/BasicAuthTest.php +++ b/core/modules/basic_auth/src/Tests/Authentication/BasicAuthTest.php @@ -173,6 +173,12 @@ function testUnauthorizedErrorMessage() { $this->basicAuthGet($url, $account->getUsername(), $this->randomMachineName()); $this->assertResponse('403', 'The user is blocked when wrong credentials are passed.'); $this->assertText('Access denied', "A user friendly access denied message is displayed"); + + // Case when correct credentials but hasn't access to the route. + $url = Url::fromRoute('router_test.15'); + $this->basicAuthGet($url, $account->getUsername(), $account->pass_raw); + $this->assertResponse('403', 'The used authentication method is not allowed on this route.'); + $this->assertText('Access denied', "A user friendly access denied message is displayed"); } /** diff --git a/core/modules/comment/src/CommentAccessControlHandler.php b/core/modules/comment/src/CommentAccessControlHandler.php index fc0d4b2..639575a 100644 --- a/core/modules/comment/src/CommentAccessControlHandler.php +++ b/core/modules/comment/src/CommentAccessControlHandler.php @@ -36,8 +36,13 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter switch ($operation) { case 'view': - return AccessResult::allowedIf($account->hasPermission('access comments') && $entity->isPublished())->cachePerPermissions()->addCacheableDependency($entity) + $access_result = AccessResult::allowedIf($account->hasPermission('access comments') && $entity->isPublished())->cachePerPermissions()->addCacheableDependency($entity) ->andIf($entity->getCommentedEntity()->access($operation, $account, TRUE)); + if (!$access_result->isAllowed()) { + $access_result->setReason("The 'access comments' permission is required and the comment must be published."); + } + + return $access_result; case 'update': return AccessResult::allowedIf($account->id() && $account->id() == $entity->getOwnerId() && $entity->isPublished() && $account->hasPermission('edit own comments'))->cachePerPermissions()->cachePerUser()->addCacheableDependency($entity); diff --git a/core/modules/editor/src/Tests/QuickEditIntegrationLoadingTest.php b/core/modules/editor/src/Tests/QuickEditIntegrationLoadingTest.php index 9a41f6b..9b9d13c 100644 --- a/core/modules/editor/src/Tests/QuickEditIntegrationLoadingTest.php +++ b/core/modules/editor/src/Tests/QuickEditIntegrationLoadingTest.php @@ -84,10 +84,17 @@ public function testUsersWithoutPermission() { // Ensure the text is transformed. $this->assertRaw('

Do you also love Drupal?

Druplicon
'); - // Retrieving the untransformed text should result in an empty 403 response. + // Retrieving the untransformed text should result in an 403 response and + // return a different error message depending of the missing permission. $response = $this->drupalPost('editor/' . 'node/1/body/en/full', '', array(), array('query' => array(MainContentViewSubscriber::WRAPPER_FORMAT => 'drupal_ajax'))); $this->assertResponse(403); - $this->assertIdentical('{}', $response); + if (!$user->hasPermission('access in-place editing')) { + $message = "A fatal error occurred: The 'access in-place editing' permission is required."; + $this->assertIdentical(Json::encode(['message' => $message]), $response); + } + else { + $this->assertIdentical('{}', $response); + } } } diff --git a/core/modules/filter/src/Tests/FilterFormatAccessTest.php b/core/modules/filter/src/Tests/FilterFormatAccessTest.php index 496cfb8..5f20404 100644 --- a/core/modules/filter/src/Tests/FilterFormatAccessTest.php +++ b/core/modules/filter/src/Tests/FilterFormatAccessTest.php @@ -122,10 +122,11 @@ function testFormatPermissions() { // Make sure that a regular user only has access to the text formats for // which they were granted access. $fallback_format = FilterFormat::load(filter_fallback_format()); + $disallowed_format_name = $this->disallowedFormat->getPermissionName(); $this->assertTrue($this->allowedFormat->access('use', $this->webUser), 'A regular user has access to use a text format they were granted access to.'); $this->assertEqual(AccessResult::allowed()->addCacheContexts(['user.permissions']), $this->allowedFormat->access('use', $this->webUser, TRUE), 'A regular user has access to use a text format they were granted access to.'); $this->assertFalse($this->disallowedFormat->access('use', $this->webUser), 'A regular user does not have access to use a text format they were not granted access to.'); - $this->assertEqual(AccessResult::neutral()->cachePerPermissions(), $this->disallowedFormat->access('use', $this->webUser, TRUE), 'A regular user does not have access to use a text format they were not granted access to.'); + $this->assertEqual(AccessResult::neutral("The '$disallowed_format_name' permission is required.")->cachePerPermissions(), $this->disallowedFormat->access('use', $this->webUser, TRUE), 'A regular user does not have access to use a text format they were not granted access to.'); $this->assertTrue($fallback_format->access('use', $this->webUser), 'A regular user has access to use the fallback format.'); $this->assertEqual(AccessResult::allowed(), $fallback_format->access('use', $this->webUser, TRUE), 'A regular user has access to use the fallback format.'); diff --git a/core/modules/node/src/NodeAccessControlHandler.php b/core/modules/node/src/NodeAccessControlHandler.php index 988de9e..89b8169 100644 --- a/core/modules/node/src/NodeAccessControlHandler.php +++ b/core/modules/node/src/NodeAccessControlHandler.php @@ -62,10 +62,11 @@ public function access(EntityInterface $entity, $operation, AccountInterface $ac return $return_as_object ? $result : $result->isAllowed(); } if (!$account->hasPermission('access content')) { - $result = AccessResult::forbidden()->cachePerPermissions(); + $result = AccessResult::forbidden("The 'access content' permission is required.")->cachePerPermissions(); return $return_as_object ? $result : $result->isAllowed(); } $result = parent::access($entity, $operation, $account, TRUE)->cachePerPermissions(); + return $return_as_object ? $result : $result->isAllowed(); } diff --git a/core/modules/quickedit/src/Tests/QuickEditLoadingTest.php b/core/modules/quickedit/src/Tests/QuickEditLoadingTest.php index 6cda648..ab66227 100644 --- a/core/modules/quickedit/src/Tests/QuickEditLoadingTest.php +++ b/core/modules/quickedit/src/Tests/QuickEditLoadingTest.php @@ -112,7 +112,7 @@ public function testUserWithoutPermission() { // Retrieving the metadata should result in an empty 403 response. $post = array('fields[0]' => 'node/1/body/en/full'); $response = $this->drupalPostWithFormat(Url::fromRoute('quickedit.metadata'), 'json', $post); - $this->assertIdentical('{"message":""}', $response); + $this->assertIdentical(Json::encode(['message' => "The 'access in-place editing' permission is required."]), $response); $this->assertResponse(403); // Quick Edit's JavaScript would SearchRankingTestnever hit these endpoints if the metadata @@ -120,11 +120,12 @@ public function testUserWithoutPermission() { // able to use any of the other endpoints either. $post = array('editors[0]' => 'form') + $this->getAjaxPageStatePostData(); $response = $this->drupalPost('quickedit/attachments', '', $post, ['query' => [MainContentViewSubscriber::WRAPPER_FORMAT => 'drupal_ajax']]); - $this->assertIdentical('{}', $response); + $message = Json::encode(['message' => "A fatal error occurred: The 'access in-place editing' permission is required."]); + $this->assertIdentical($message, $response); $this->assertResponse(403); $post = array('nocssjs' => 'true') + $this->getAjaxPageStatePostData(); $response = $this->drupalPost('quickedit/form/' . 'node/1/body/en/full', '', $post, ['query' => [MainContentViewSubscriber::WRAPPER_FORMAT => 'drupal_ajax']]); - $this->assertIdentical('{}', $response); + $this->assertIdentical($message, $response); $this->assertResponse(403); $edit = array(); $edit['form_id'] = 'quickedit_field_form'; @@ -135,11 +136,11 @@ public function testUserWithoutPermission() { $edit['body[0][format]'] = 'filtered_html'; $edit['op'] = t('Save'); $response = $this->drupalPost('quickedit/form/' . 'node/1/body/en/full', '', $edit, ['query' => [MainContentViewSubscriber::WRAPPER_FORMAT => 'drupal_ajax']]); - $this->assertIdentical('{}', $response); + $this->assertIdentical($message, $response); $this->assertResponse(403); $post = array('nocssjs' => 'true'); $response = $this->drupalPostWithFormat('quickedit/entity/' . 'node/1', 'json', $post); - $this->assertIdentical('{"message":""}', $response); + $this->assertIdentical(Json::encode(['message' => "The 'access in-place editing' permission is required."]), $response); $this->assertResponse(403); } diff --git a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php index a1631bd..3e35f83 100644 --- a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php @@ -120,7 +120,7 @@ public static function create(ContainerInterface $container, array $configuratio public function get(EntityInterface $entity) { $entity_access = $entity->access('view', NULL, TRUE); if (!$entity_access->isAllowed()) { - throw new AccessDeniedHttpException(); + throw new AccessDeniedHttpException($entity_access->getReason() ?: $this->generateFallbackAccessDeniedMessage($entity, 'view')); } $response = new ResourceResponse($entity, 200); @@ -160,8 +160,9 @@ public function post(EntityInterface $entity = NULL) { throw new BadRequestHttpException('No entity content received.'); } - if (!$entity->access('create')) { - throw new AccessDeniedHttpException(); + $entity_access = $entity->access('create', NULL, TRUE); + if (!$entity_access->isAllowed()) { + throw new AccessDeniedHttpException($entity_access->getReason() ?: $this->generateFallbackAccessDeniedMessage($entity, 'create')); } $definition = $this->getPluginDefinition(); // Verify that the deserialized entity is of the type that we expect to @@ -215,8 +216,9 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity if ($entity->getEntityTypeId() != $definition['entity_type']) { throw new BadRequestHttpException('Invalid entity type'); } - if (!$original_entity->access('update')) { - throw new AccessDeniedHttpException(); + $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 properties. @@ -279,8 +281,9 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity * @throws \Symfony\Component\HttpKernel\Exception\HttpException */ public function delete(EntityInterface $entity) { - if (!$entity->access('delete')) { - throw new AccessDeniedHttpException(); + $entity_access = $entity->access('delete', NULL, TRUE); + if (!$entity_access->isAllowed()) { + throw new AccessDeniedHttpException($entity_access->getReason() ?: $this->generateFallbackAccessDeniedMessage($entity, 'delete')); } try { $entity->delete(); @@ -295,6 +298,26 @@ public function delete(EntityInterface $entity) { } /** + * Generates a fallback access denied message, when no specific reason is set. + * + * @param \Drupal\Core\Entity\EntityInterface $entity + * The entity object. + * @param string $operation + * The disallowed entity operation. + * + * @return string + * The proper message to display in the AccessDeniedHttpException. + */ + protected function generateFallbackAccessDeniedMessage(EntityInterface $entity, $operation) { + $message = "You are not authorized to {$operation} this {$entity->getEntityTypeId()} entity"; + + if ($entity->bundle() !== $entity->getEntityTypeId()) { + $message .= " of bundle {$entity->bundle()}"; + } + return "{$message}."; + } + + /** * {@inheritdoc} */ public function permissions() { 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 new file mode 100644 index 0000000..ca23eb2 --- /dev/null +++ b/core/modules/rest/tests/modules/rest_test/rest_test.services.yml @@ -0,0 +1,5 @@ +services: + rest_test.authentication.test_auth: + class: Drupal\rest_test\Authentication\Provider\TestAuth + tags: + - { name: authentication_provider, provider_id: 'rest_test_auth' } diff --git a/core/modules/rest/tests/modules/rest_test/src/Authentication/Provider/TestAuth.php b/core/modules/rest/tests/modules/rest_test/src/Authentication/Provider/TestAuth.php new file mode 100644 index 0000000..36302ac --- /dev/null +++ b/core/modules/rest/tests/modules/rest_test/src/Authentication/Provider/TestAuth.php @@ -0,0 +1,27 @@ +headers->has('REST-test-auth'); + } + + /** + * {@inheritdoc} + */ + public function authenticate(Request $request) { + return NULL; + } + +} diff --git a/core/modules/rest/tests/src/Functional/CookieResourceTestTrait.php b/core/modules/rest/tests/src/Functional/CookieResourceTestTrait.php index 41fde93..2780629 100644 --- a/core/modules/rest/tests/src/Functional/CookieResourceTestTrait.php +++ b/core/modules/rest/tests/src/Functional/CookieResourceTestTrait.php @@ -95,7 +95,10 @@ protected function getAuthenticationRequestOptions($method) { * {@inheritdoc} */ protected function assertResponseWhenMissingAuthentication(ResponseInterface $response) { - $this->assertResourceErrorResponse(403, '', $response); + // Requests needing cookie authentication but missing it results in a 403 + // response. The cookie authentication mechanism sets no response message. + // @todo https://www.drupal.org/node/2847623 + $this->assertResourceErrorResponse(403, FALSE, $response); } /** diff --git a/core/modules/rest/tests/src/Functional/EntityResource/Block/BlockResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/Block/BlockResourceTestBase.php index 89bb9cf..d87e423 100644 --- a/core/modules/rest/tests/src/Functional/EntityResource/Block/BlockResourceTestBase.php +++ b/core/modules/rest/tests/src/Functional/EntityResource/Block/BlockResourceTestBase.php @@ -127,4 +127,20 @@ protected function getExpectedCacheTags() { }); } + /** + * {@inheritdoc} + */ + protected function getExpectedUnauthorizedAccessMessage($method) { + if ($this->config('rest.settings')->get('bc_entity_resource_permissions')) { + return parent::getExpectedUnauthorizedAccessMessage($method); + } + + switch ($method) { + case 'GET': + return "You are not authorized to view this block entity."; + default: + return parent::getExpectedUnauthorizedAccessMessage($method); + } + } + } diff --git a/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php index ecae9f8..7e38b33 100644 --- a/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php +++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php @@ -310,4 +310,22 @@ public function testPostDxWithoutCriticalBaseFields() { //$this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nfield_name: This value should not be null.\n", $response); } + /** + * {@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 comments' permission is required and the comment must be published."; + case 'POST'; + return "The 'post comments' permission is required."; + default: + return parent::getExpectedUnauthorizedAccessMessage($method); + } + } + } diff --git a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php index 603fb35..25bedfa 100644 --- a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php @@ -241,6 +241,43 @@ protected function getNormalizedPatchEntity() { } /** + * {@inheritdoc} + */ + protected function getExpectedUnauthorizedAccessMessage($method) { + + if ($this->config('rest.settings')->get('bc_entity_resource_permissions')) { + return $this->getExpectedBCUnauthorizedAccessMessage($method); + } + + $permission = $this->entity->getEntityType()->getAdminPermission(); + if ($permission !== FALSE) { + return "The '{$permission}' permission is required."; + } + + $http_method_to_entity_operation = [ + 'GET' => 'view', + 'POST' => 'create', + 'PATCH' => 'update', + 'DELETE' => 'delete', + ]; + $operation = $http_method_to_entity_operation[$method]; + $message = sprintf('You are not authorized to %s this %s entity', $operation, $this->entity->getEntityTypeId()); + + if ($this->entity->bundle() !== $this->entity->getEntityTypeId()) { + $message .= ' of bundle ' . $this->entity->bundle(); + } + + return "$message."; + } + + /** + * {@inheritdoc} + */ + protected function getExpectedBcUnauthorizedAccessMessage($method) { + return "The 'restful " . strtolower($method) . " entity:" . $this->entity->getEntityTypeId() . "' permission is required."; + } + + /** * The expected cache tags for the GET/HEAD response of the test entity. * * @see ::testGet @@ -301,7 +338,7 @@ public function testGet() { // response because ?_format query string is present. $response = $this->request('GET', $url, $request_options); if ($has_canonical_url) { - $this->assertResourceErrorResponse(403, '', $response); + $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response); } else { $this->assertResourceErrorResponse(404, 'No route found for "GET ' . str_replace($this->baseUrl, '', $this->getUrl()->setAbsolute()->toString()) . '"', $response); @@ -335,16 +372,23 @@ public function testGet() { $this->assertResponseWhenMissingAuthentication($response); } + $request_options[RequestOptions::HEADERS]['REST-test-auth'] = '1'; + + // DX: 403 when attempting to use unallowed authentication provider. + $response = $this->request('GET', $url, $request_options); + $this->assertResourceErrorResponse(403, 'The used authentication method is not allowed on this route.', $response); + + unset($request_options[RequestOptions::HEADERS]['REST-test-auth']); $request_options = NestedArray::mergeDeep($request_options, $this->getAuthenticationRequestOptions('GET')); // DX: 403 when unauthorized. $response = $this->request('GET', $url, $request_options); - // @todo Update the message in https://www.drupal.org/node/2808233. - $this->assertResourceErrorResponse(403, '', $response); + $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response); $this->assertArrayNotHasKey('Link', $response->getHeaders()); + $this->setUpAuthorization('GET'); @@ -420,8 +464,7 @@ public function testGet() { // DX: 403 when unauthorized. $response = $this->request('GET', $url, $request_options); - // @todo Update the message in https://www.drupal.org/node/2808233. - $this->assertResourceErrorResponse(403, '', $response); + $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response); $this->grantPermissionsToTestedRole(['restful get entity:' . static::$entityTypeId]); @@ -570,8 +613,7 @@ public function testPost() { // DX: 403 when unauthorized. $response = $this->request('POST', $url, $request_options); - // @todo Update the message in https://www.drupal.org/node/2808233. - $this->assertResourceErrorResponse(403, '', $response); + $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('POST'), $response); $this->setUpAuthorization('POST'); @@ -638,8 +680,7 @@ public function testPost() { // DX: 403 when unauthorized. $response = $this->request('POST', $url, $request_options); - // @todo Update the message in https://www.drupal.org/node/2808233. - $this->assertResourceErrorResponse(403, '', $response); + $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('POST'), $response); $this->grantPermissionsToTestedRole(['restful post entity:' . static::$entityTypeId]); @@ -760,8 +801,7 @@ public function testPatch() { // DX: 403 when unauthorized. $response = $this->request('PATCH', $url, $request_options); - // @todo Update the message in https://www.drupal.org/node/2808233. - $this->assertResourceErrorResponse(403, '', $response); + $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('PATCH'), $response); $this->setUpAuthorization('PATCH'); @@ -843,8 +883,7 @@ public function testPatch() { // DX: 403 when unauthorized. $response = $this->request('PATCH', $url, $request_options); - // @todo Update the message in https://www.drupal.org/node/2808233. - $this->assertResourceErrorResponse(403, '', $response); + $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('PATCH'), $response); $this->grantPermissionsToTestedRole(['restful patch entity:' . static::$entityTypeId]); @@ -915,8 +954,7 @@ public function testDelete() { // DX: 403 when unauthorized. $response = $this->request('DELETE', $url, $request_options); - // @todo Update the message in https://www.drupal.org/node/2808233. - $this->assertResourceErrorResponse(403, '', $response); + $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('DELETE'), $response); $this->setUpAuthorization('DELETE'); @@ -945,8 +983,7 @@ public function testDelete() { // DX: 403 when unauthorized. $response = $this->request('DELETE', $url, $request_options); - // @todo Update the message in https://www.drupal.org/node/2808233. - $this->assertResourceErrorResponse(403, '', $response); + $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('DELETE'), $response); $this->grantPermissionsToTestedRole(['restful delete entity:' . static::$entityTypeId]); diff --git a/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestResourceTestBase.php index da86d00..bd45410 100644 --- a/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestResourceTestBase.php +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestResourceTestBase.php @@ -124,4 +124,22 @@ protected function getNormalizedPostEntity() { ]; } + /** + * {@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 test entity' permission is required."; + case 'POST': + return "The following permissions are required: 'administer entity_test content' OR 'administer entity_test_with_bundle content' OR 'create entity_test entity_test_with_bundle entities'."; + default: + return parent::getExpectedUnauthorizedAccessMessage($method); + } + } + } diff --git a/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php index d7651c4..04db15d 100644 --- a/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php +++ b/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php @@ -193,4 +193,17 @@ protected function getNormalizedPostEntity() { ]; } + /** + * {@inheritdoc} + */ + protected function getExpectedUnauthorizedAccessMessage($method) { + if ($this->config('rest.settings')->get('bc_entity_resource_permissions')) { + return parent::getExpectedUnauthorizedAccessMessage($method); + } + + if ($method === 'GET' || $method == 'PATCH' || $method == 'DELETE') { + return "The 'access content' permission is required."; + } + return parent::getExpectedUnauthorizedAccessMessage($method); + } } diff --git a/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php index b6dce4f..b6bde1d 100644 --- a/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php +++ b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php @@ -137,4 +137,26 @@ protected function getNormalizedPostEntity() { ]; } + /** + * {@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 content' permission is required."; + case 'POST': + return "The 'administer taxonomy' permission is required."; + case 'PATCH': + return "The following permissions are required: 'edit terms in camelids' OR 'administer taxonomy'."; + case 'DELETE': + return "The following permissions are required: 'delete terms in camelids' OR 'administer taxonomy'."; + default: + return parent::getExpectedUnauthorizedAccessMessage($method); + } + } + } diff --git a/core/modules/rest/tests/src/Functional/EntityResource/User/UserResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/User/UserResourceTestBase.php index 38452c2..fe9b750 100644 --- a/core/modules/rest/tests/src/Functional/EntityResource/User/UserResourceTestBase.php +++ b/core/modules/rest/tests/src/Functional/EntityResource/User/UserResourceTestBase.php @@ -217,4 +217,24 @@ public function testPatchDxForSecuritySensitiveBaseFields() { $this->assertSame(200, $response->getStatusCode()); } + /** + * {@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 user profiles' permission is required and the user must be active."; + case 'PATCH': + return "You are not authorized to update this user entity."; + case 'DELETE': + return 'You are not authorized to delete this user entity.'; + default: + return parent::getExpectedUnauthorizedAccessMessage($method); + } + } + } diff --git a/core/modules/rest/tests/src/Functional/ResourceTestBase.php b/core/modules/rest/tests/src/Functional/ResourceTestBase.php index 218e96f..2674d70 100644 --- a/core/modules/rest/tests/src/Functional/ResourceTestBase.php +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php @@ -204,6 +204,29 @@ protected function provisionResource($resource_type, $formats = [], $authenticat abstract protected function assertAuthenticationEdgeCases($method, Url $url, array $request_options); /** + * Return the expected error message. + * + * @param string $method + * The HTTP method (GET, POST, PATCH, DELETE). + * + * @return string + * The error string. + */ + abstract protected function getExpectedUnauthorizedAccessMessage($method); + + /** + * Return the default expected error message if the + * bc_entity_resource_permissions is true. + * + * @param string $method + * The HTTP method (GET, POST, PATCH, DELETE). + * + * @return string + * The error string. + */ + abstract protected function getExpectedBcUnauthorizedAccessMessage($method); + + /** * Initializes authentication. * * E.g. for cookie authentication, we first need to get a cookie. @@ -321,7 +344,7 @@ protected function assertResourceResponse($expected_status_code, $expected_body, * The error response to assert. */ protected function assertResourceErrorResponse($expected_status_code, $expected_message, ResponseInterface $response) { - $expected_body = $this->serializer->encode(['message' => $expected_message], static::$format); + $expected_body = ($expected_message !== FALSE) ? $this->serializer->encode(['message' => $expected_message], static::$format) : FALSE; $this->assertResourceResponse($expected_status_code, $expected_body, $response); } diff --git a/core/modules/user/src/UserAccessControlHandler.php b/core/modules/user/src/UserAccessControlHandler.php index 9c86758..dd7142c 100644 --- a/core/modules/user/src/UserAccessControlHandler.php +++ b/core/modules/user/src/UserAccessControlHandler.php @@ -3,6 +3,7 @@ namespace Drupal\user; use Drupal\Core\Access\AccessResult; +use Drupal\Core\Access\AccessResultNeutral; use Drupal\Core\Entity\EntityInterface; use Drupal\Core\Entity\EntityAccessControlHandler; use Drupal\Core\Field\FieldDefinitionInterface; @@ -56,6 +57,9 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter elseif ($account->id() == $entity->id()) { return AccessResult::allowed()->cachePerUser(); } + else { + return AccessResultNeutral::neutral("The 'access user profiles' permission is required and the user must be active."); + } break; case 'update': diff --git a/core/modules/user/tests/src/Unit/PermissionAccessCheckTest.php b/core/modules/user/tests/src/Unit/PermissionAccessCheckTest.php index bcfd750..84290cc 100644 --- a/core/modules/user/tests/src/Unit/PermissionAccessCheckTest.php +++ b/core/modules/user/tests/src/Unit/PermissionAccessCheckTest.php @@ -55,10 +55,10 @@ public function providerTestAccess() { return [ [[], FALSE], [['_permission' => 'allowed'], TRUE, ['user.permissions']], - [['_permission' => 'denied'], FALSE, ['user.permissions']], + [['_permission' => 'denied'], FALSE, ['user.permissions'], "The 'denied' permission is required."], [['_permission' => 'allowed+denied'], TRUE, ['user.permissions']], [['_permission' => 'allowed+denied+other'], TRUE, ['user.permissions']], - [['_permission' => 'allowed,denied'], FALSE, ['user.permissions']], + [['_permission' => 'allowed,denied'], FALSE, ['user.permissions'], "The following permissions are required: 'allowed' AND 'denied'."], ]; } @@ -68,8 +68,11 @@ public function providerTestAccess() { * @dataProvider providerTestAccess * @covers ::access */ - public function testAccess($requirements, $access, array $contexts = []) { + public function testAccess($requirements, $access, array $contexts = [], $message = '') { $access_result = AccessResult::allowedIf($access)->addCacheContexts($contexts); + if (!empty($message)) { + $access_result->setReason($message); + } $user = $this->getMock('Drupal\Core\Session\AccountInterface'); $user->expects($this->any()) ->method('hasPermission') diff --git a/core/tests/Drupal/Tests/Core/Access/AccessResultNeutralTest.php b/core/tests/Drupal/Tests/Core/Access/AccessResultNeutralTest.php new file mode 100644 index 0000000..5b96b19 --- /dev/null +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultNeutralTest.php @@ -0,0 +1,44 @@ +assertNull($a->getReason()); + + $reason = $this->getRandomGenerator()->string(); + $b = new AccessResultNeutral($reason); + $this->assertEquals($reason, $b->getReason()); + } + + /** + * Test setReason() + * + * @covers ::setReason + */ + public function testSetReason() { + $a = new AccessResultNeutral(); + + $reason = $this->getRandomGenerator()->string(); + $return = $a->setReason($reason); + + $this->assertSame($reason, $a->getReason()); + $this->assertSame($a, $return); + } + +} diff --git a/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php index ca262c7..c1b6c65 100644 --- a/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php @@ -174,9 +174,9 @@ public function testAccessConditionallyForbidden() { * @covers ::andIf */ public function testAndIf() { - $neutral = AccessResult::neutral(); + $neutral = AccessResult::neutral('neutral message'); $allowed = AccessResult::allowed(); - $forbidden = AccessResult::forbidden(); + $forbidden = AccessResult::forbidden('forbidden message'); $unused_access_result_due_to_lazy_evaluation = $this->getMock('\Drupal\Core\Access\AccessResultInterface'); $unused_access_result_due_to_lazy_evaluation->expects($this->never()) ->method($this->anything()); @@ -193,6 +193,7 @@ public function testAndIf() { $this->assertFalse($access->isAllowed()); $this->assertFalse($access->isForbidden()); $this->assertTrue($access->isNeutral()); + $this->assertEquals('neutral message', $access->getReason()); $this->assertDefaultCacheability($access); // ALLOWED && FORBIDDEN === FORBIDDEN. @@ -200,6 +201,7 @@ public function testAndIf() { $this->assertFalse($access->isAllowed()); $this->assertTrue($access->isForbidden()); $this->assertFalse($access->isNeutral()); + $this->assertEquals('forbidden message', $access->getReason()); $this->assertDefaultCacheability($access); // NEUTRAL && ALLOW == NEUTRAL @@ -207,6 +209,7 @@ public function testAndIf() { $this->assertFalse($access->isAllowed()); $this->assertFalse($access->isForbidden()); $this->assertTrue($access->isNeutral()); + $this->assertEquals('neutral message', $access->getReason()); $this->assertDefaultCacheability($access); // NEUTRAL && NEUTRAL === NEUTRAL. @@ -214,6 +217,7 @@ public function testAndIf() { $this->assertFalse($access->isAllowed()); $this->assertFalse($access->isForbidden()); $this->assertTrue($access->isNeutral()); + $this->assertEquals('neutral message', $access->getReason()); $this->assertDefaultCacheability($access); // NEUTRAL && FORBIDDEN === FORBIDDEN. @@ -221,6 +225,7 @@ public function testAndIf() { $this->assertFalse($access->isAllowed()); $this->assertTrue($access->isForbidden()); $this->assertFalse($access->isNeutral()); + $this->assertEquals('forbidden message', $access->getReason()); $this->assertDefaultCacheability($access); // FORBIDDEN && ALLOWED = FORBIDDEN @@ -228,6 +233,7 @@ public function testAndIf() { $this->assertFalse($access->isAllowed()); $this->assertTrue($access->isForbidden()); $this->assertFalse($access->isNeutral()); + $this->assertEquals('forbidden message', $access->getReason()); $this->assertDefaultCacheability($access); // FORBIDDEN && NEUTRAL = FORBIDDEN @@ -235,6 +241,7 @@ public function testAndIf() { $this->assertFalse($access->isAllowed()); $this->assertTrue($access->isForbidden()); $this->assertFalse($access->isNeutral()); + $this->assertEquals('forbidden message', $access->getReason()); $this->assertDefaultCacheability($access); // FORBIDDEN && FORBIDDEN = FORBIDDEN @@ -242,6 +249,7 @@ public function testAndIf() { $this->assertFalse($access->isAllowed()); $this->assertTrue($access->isForbidden()); $this->assertFalse($access->isNeutral()); + $this->assertEquals('forbidden message', $access->getReason()); $this->assertDefaultCacheability($access); // FORBIDDEN && * === FORBIDDEN: lazy evaluation verification. @@ -249,6 +257,7 @@ public function testAndIf() { $this->assertFalse($access->isAllowed()); $this->assertTrue($access->isForbidden()); $this->assertFalse($access->isNeutral()); + $this->assertEquals('forbidden message', $access->getReason()); $this->assertDefaultCacheability($access); } @@ -256,9 +265,9 @@ public function testAndIf() { * @covers ::orIf */ public function testOrIf() { - $neutral = AccessResult::neutral(); + $neutral = AccessResult::neutral('neutral message'); $allowed = AccessResult::allowed(); - $forbidden = AccessResult::forbidden(); + $forbidden = AccessResult::forbidden('forbidden message'); $unused_access_result_due_to_lazy_evaluation = $this->getMock('\Drupal\Core\Access\AccessResultInterface'); $unused_access_result_due_to_lazy_evaluation->expects($this->never()) ->method($this->anything()); @@ -282,6 +291,7 @@ public function testOrIf() { $this->assertFalse($access->isAllowed()); $this->assertTrue($access->isForbidden()); $this->assertFalse($access->isNeutral()); + $this->assertEquals('forbidden message', $access->getReason()); $this->assertDefaultCacheability($access); // NEUTRAL || NEUTRAL === NEUTRAL. @@ -289,6 +299,7 @@ public function testOrIf() { $this->assertFalse($access->isAllowed()); $this->assertFalse($access->isForbidden()); $this->assertTrue($access->isNeutral()); + $this->assertEquals('neutral message', $access->getReason()); $this->assertDefaultCacheability($access); // NEUTRAL || ALLOWED === ALLOWED. @@ -303,6 +314,7 @@ public function testOrIf() { $this->assertFalse($access->isAllowed()); $this->assertTrue($access->isForbidden()); $this->assertFalse($access->isNeutral()); + $this->assertEquals('forbidden message', $access->getReason()); $this->assertDefaultCacheability($access); // FORBIDDEN || ALLOWED === FORBIDDEN. @@ -310,6 +322,7 @@ public function testOrIf() { $this->assertFalse($access->isAllowed()); $this->assertTrue($access->isForbidden()); $this->assertFalse($access->isNeutral()); + $this->assertEquals('forbidden message', $access->getReason()); $this->assertDefaultCacheability($access); // FORBIDDEN || NEUTRAL === FORBIDDEN. @@ -317,6 +330,7 @@ public function testOrIf() { $this->assertFalse($access->isAllowed()); $this->assertTrue($access->isForbidden()); $this->assertFalse($access->isNeutral()); + $this->assertEquals('forbidden message', $access->getReason()); $this->assertDefaultCacheability($access); // FORBIDDEN || FORBIDDEN === FORBIDDEN. @@ -324,6 +338,7 @@ public function testOrIf() { $this->assertFalse($access->isAllowed()); $this->assertTrue($access->isForbidden()); $this->assertFalse($access->isNeutral()); + $this->assertEquals('forbidden message', $access->getReason()); $this->assertDefaultCacheability($access); // FORBIDDEN || * === FORBIDDEN. @@ -331,6 +346,7 @@ public function testOrIf() { $this->assertFalse($access->isAllowed()); $this->assertTrue($access->isForbidden()); $this->assertFalse($access->isNeutral()); + $this->assertEquals('forbidden message', $access->getReason()); $this->assertDefaultCacheability($access); } @@ -901,18 +917,31 @@ public function testAllowedIfHasPermissions($permissions, $conjunction, AccessRe * @return array */ public function providerTestAllowedIfHasPermissions() { - return [ - [[], 'AND', AccessResult::allowedIf(FALSE)], - [[], 'OR', AccessResult::allowedIf(FALSE)], - [['allowed'], 'OR', AccessResult::allowedIf(TRUE)], - [['allowed'], 'AND', AccessResult::allowedIf(TRUE)], - [['denied'], 'OR', AccessResult::allowedIf(FALSE)], - [['denied'], 'AND', AccessResult::allowedIf(FALSE)], - [['allowed', 'denied'], 'OR', AccessResult::allowedIf(TRUE)], - [['denied', 'allowed'], 'OR', AccessResult::allowedIf(TRUE)], - [['allowed', 'denied', 'other'], 'OR', AccessResult::allowedIf(TRUE)], - [['allowed', 'denied'], 'AND', AccessResult::allowedIf(FALSE)], - ]; + $access_result = AccessResult::allowedIf(FALSE); + $data[] = [[], 'AND', $access_result]; + $data[] = [[], 'OR', $access_result]; + + $access_result = AccessResult::allowedIf(TRUE); + $data[] = [['allowed'], 'OR', $access_result]; + $data[] = [['allowed'], 'AND', $access_result]; + + $access_result = AccessResult::allowedIf(FALSE); + $access_result->setReason("The 'denied' permission is required."); + $data[] = [['denied'], 'OR', $access_result]; + $data[] = [['denied'], 'AND', $access_result]; + + $access_result = AccessResult::allowedIf(TRUE); + $data[] = [['allowed', 'denied'], 'OR', $access_result]; + $data[] = [['denied', 'allowed'], 'OR', $access_result]; + + $access_result = AccessResult::allowedIf(TRUE); + $data[] = [['allowed', 'denied', 'other'], 'OR', $access_result]; + + $access_result = AccessResult::allowedIf(FALSE); + $access_result->setReason("The following permissions are required: 'allowed' AND 'denied'."); + $data[] = [['allowed', 'denied'], 'AND', $access_result]; + + return $data; } }