Commit 585e6338 authored by larowlan's avatar larowlan

Issue #2938035 by Wim Leers, alexpott: When PATCHing a field is disallowed, no...

Issue #2938035 by Wim Leers, alexpott: When PATCHing a field is disallowed, no reason is given for *why* this happens
parent 729b6308
......@@ -321,13 +321,13 @@ public function fieldAccess($operation, FieldDefinitionInterface $field_definiti
if ($field_definition->getName() === $this->entityType->getKey('id')) {
// String IDs can be set when creating the entity.
if (!($entity->isNew() && $field_definition->getType() === 'string')) {
return $return_as_object ? AccessResult::forbidden('The entity ID cannot be changed')->addCacheableDependency($entity) : FALSE;
return $return_as_object ? AccessResult::forbidden('The entity ID cannot be changed.')->addCacheableDependency($entity) : FALSE;
}
}
elseif ($field_definition->getName() === $this->entityType->getKey('uuid')) {
// UUIDs can be set when creating an entity.
if (!$entity->isNew()) {
return $return_as_object ? AccessResult::forbidden('The entity UUID cannot be changed')->addCacheableDependency($entity) : FALSE;
return $return_as_object ? AccessResult::forbidden('The entity UUID cannot be changed.')->addCacheableDependency($entity) : FALSE;
}
}
}
......
......@@ -29,7 +29,7 @@ abstract class BlockContentResourceTestBase extends EntityResourceTestBase {
* {@inheritdoc}
*/
protected static $patchProtectedFieldNames = [
'changed',
'changed' => NULL,
];
/**
......
......@@ -25,11 +25,11 @@ class CommentHalJsonAnonTest extends CommentHalJsonTestBase {
* @see ::setUpAuthorization
*/
protected static $patchProtectedFieldNames = [
'changed',
'thread',
'entity_type',
'field_name',
'entity_id',
'changed' => NULL,
'thread' => NULL,
'entity_type' => NULL,
'field_name' => NULL,
'entity_id' => NULL,
];
}
......@@ -36,14 +36,14 @@ abstract class CommentHalJsonTestBase extends CommentResourceTestBase {
* @todo fix in https://www.drupal.org/node/2824271
*/
protected static $patchProtectedFieldNames = [
'status',
'created',
'changed',
'thread',
'entity_type',
'field_name',
'entity_id',
'uid',
'status' => "The 'administer comments' permission is required.",
'created' => "The 'administer comments' permission is required.",
'changed' => NULL,
'thread' => NULL,
'entity_type' => NULL,
'field_name' => NULL,
'entity_id' => NULL,
'uid' => "The 'administer comments' permission is required.",
];
/**
......
......@@ -34,12 +34,12 @@ class CommentJsonAnonTest extends CommentResourceTestBase {
* @see ::setUpAuthorization
*/
protected static $patchProtectedFieldNames = [
'pid',
'entity_id',
'changed',
'thread',
'entity_type',
'field_name',
'pid' => NULL,
'entity_id' => NULL,
'changed' => NULL,
'thread' => NULL,
'entity_type' => NULL,
'field_name' => NULL,
];
}
......@@ -30,17 +30,17 @@ abstract class CommentResourceTestBase extends EntityResourceTestBase {
* {@inheritdoc}
*/
protected static $patchProtectedFieldNames = [
'status',
'pid',
'entity_id',
'uid',
'name',
'homepage',
'created',
'changed',
'thread',
'entity_type',
'field_name',
'status' => "The 'administer comments' permission is required.",
'pid' => NULL,
'entity_id' => NULL,
'uid' => "The 'administer comments' permission is required.",
'name' => "The 'administer comments' permission is required.",
'homepage' => "The 'administer comments' permission is required.",
'created' => "The 'administer comments' permission is required.",
'changed' => NULL,
'thread' => NULL,
'entity_type' => NULL,
'field_name' => NULL,
];
/**
......
......@@ -30,11 +30,11 @@ abstract class FileResourceTestBase extends EntityResourceTestBase {
* {@inheritdoc}
*/
protected static $patchProtectedFieldNames = [
'uri',
'filemime',
'filesize',
'status',
'changed',
'uri' => NULL,
'filemime' => NULL,
'filesize' => NULL,
'status' => NULL,
'changed' => NULL,
];
/**
......
......@@ -38,7 +38,7 @@ abstract class MediaResourceTestBase extends EntityResourceTestBase {
* {@inheritdoc}
*/
protected static $patchProtectedFieldNames = [
'changed',
'changed' => NULL,
];
/**
......
......@@ -27,7 +27,7 @@ abstract class MenuLinkContentResourceTestBase extends EntityResourceTestBase {
* {@inheritdoc}
*/
protected static $patchProtectedFieldNames = [
'changed',
'changed' => NULL,
];
/**
......
......@@ -34,13 +34,13 @@ class NodeHalJsonAnonTest extends NodeResourceTestBase {
* {@inheritdoc}
*/
protected static $patchProtectedFieldNames = [
'revision_timestamp',
'created',
'changed',
'promote',
'sticky',
'path',
'revision_uid',
'revision_timestamp' => NULL,
'created' => "The 'administer nodes' permission is required.",
'changed' => NULL,
'promote' => "The 'administer nodes' permission is required.",
'sticky' => "The 'administer nodes' permission is required.",
'path' => "The following permissions are required: 'create url aliases' OR 'administer url aliases'.",
'revision_uid' => NULL,
];
/**
......
......@@ -27,13 +27,13 @@ abstract class NodeResourceTestBase extends EntityResourceTestBase {
* {@inheritdoc}
*/
protected static $patchProtectedFieldNames = [
'revision_timestamp',
'revision_uid',
'created',
'changed',
'promote',
'sticky',
'path',
'revision_timestamp' => NULL,
'revision_uid' => NULL,
'created' => "The 'administer nodes' permission is required.",
'changed' => NULL,
'promote' => "The 'administer nodes' permission is required.",
'sticky' => "The 'administer nodes' permission is required.",
'path' => "The following permissions are required: 'create url aliases' OR 'administer url aliases'.",
];
/**
......@@ -249,7 +249,7 @@ public function testPatchPath() {
// unchanged.
$response = $this->request('PATCH', $url, $request_options);
$this->assertSame('/llama', $this->entityStorage->loadUnchanged($this->entity->id())->get('path')->alias);
$this->assertResourceErrorResponse(403, "Access denied on updating field 'path'.", $response);
$this->assertResourceErrorResponse(403, "Access denied on updating field 'path'. " . static::$patchProtectedFieldNames['path'], $response);
// Grant permission to create URL aliases.
$this->grantPermissionsToTestedRole(['create url aliases']);
......
......@@ -4,6 +4,7 @@
use Drupal\Component\Plugin\DependentPluginInterface;
use Drupal\Component\Plugin\PluginManagerInterface;
use Drupal\Core\Access\AccessResultReasonInterface;
use Drupal\Core\Cache\CacheableResponseInterface;
use Drupal\Core\Config\Entity\ConfigEntityType;
use Drupal\Core\Entity\EntityTypeManagerInterface;
......@@ -276,7 +277,8 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
protected function checkPatchFieldAccess(FieldItemListInterface $original_field, FieldItemListInterface $received_field) {
// If the user is allowed to edit the field, it is always safe to set the
// received value. We may be setting an unchanged value, but that is ok.
if ($original_field->access('edit')) {
$field_edit_access = $original_field->access('edit', NULL, TRUE);
if ($field_edit_access->isAllowed()) {
return TRUE;
}
......@@ -295,7 +297,14 @@ protected function checkPatchFieldAccess(FieldItemListInterface $original_field,
// It's helpful and safe to let the user know when they are not allowed to
// update a field.
$field_name = $received_field->getName();
throw new AccessDeniedHttpException("Access denied on updating field '$field_name'.");
$error_message = "Access denied on updating field '$field_name'.";
if ($field_edit_access instanceof AccessResultReasonInterface) {
$reason = $field_edit_access->getReason();
if ($reason) {
$error_message .= ' ' . $reason;
}
}
throw new AccessDeniedHttpException($error_message);
}
/**
......
......@@ -2,6 +2,7 @@
namespace Drupal\Tests\rest\Functional\EntityResource;
use Drupal\Component\Assertion\Inspector;
use Drupal\Component\Utility\NestedArray;
use Drupal\Component\Utility\Random;
use Drupal\Core\Cache\Cache;
......@@ -82,6 +83,8 @@ abstract class EntityResourceTestBase extends ResourceTestBase {
/**
* The fields that are protected against modification during PATCH requests.
*
* Keys are field names, values are expected access denied reasons.
*
* @var string[]
*/
protected static $patchProtectedFieldNames;
......@@ -1118,13 +1121,13 @@ public function testPatch() {
// DX: 403 when entity trying to update an entity's ID field.
$request_options[RequestOptions::BODY] = $this->serializer->encode($this->makeNormalizationInvalid($this->getNormalizedPatchEntity(), 'id'), static::$format);;
$response = $this->request('PATCH', $url, $request_options);
$this->assertResourceErrorResponse(403, "Access denied on updating field '{$this->entity->getEntityType()->getKey('id')}'.", $response);
$this->assertResourceErrorResponse(403, "Access denied on updating field '{$this->entity->getEntityType()->getKey('id')}'. The entity ID cannot be changed.", $response);
if ($this->entity->getEntityType()->hasKey('uuid')) {
// DX: 403 when entity trying to update an entity's UUID field.
$request_options[RequestOptions::BODY] = $this->serializer->encode($this->makeNormalizationInvalid($this->getNormalizedPatchEntity(), 'uuid'), static::$format);;
$response = $this->request('PATCH', $url, $request_options);
$this->assertResourceErrorResponse(403, "Access denied on updating field '{$this->entity->getEntityType()->getKey('uuid')}'.", $response);
$this->assertResourceErrorResponse(403, "Access denied on updating field '{$this->entity->getEntityType()->getKey('uuid')}'. The entity UUID cannot be changed.", $response);
}
$request_options[RequestOptions::BODY] = $parseable_invalid_request_body_3;
......@@ -1136,15 +1139,15 @@ public function testPatch() {
$this->assertResourceErrorResponse(403, "Access denied on updating field 'field_rest_test'.", $response);
// DX: 403 when sending PATCH request with updated read-only fields.
$this->assertPatchProtectedFieldNamesStructure();
list($modified_entity, $original_values) = static::getModifiedEntityForPatchTesting($this->entity);
// Send PATCH request by serializing the modified entity, assert the error
// response, change the modified entity field that caused the error response
// back to its original value, repeat.
for ($i = 0; $i < count(static::$patchProtectedFieldNames); $i++) {
$patch_protected_field_name = static::$patchProtectedFieldNames[$i];
foreach (static::$patchProtectedFieldNames as $patch_protected_field_name => $reason) {
$request_options[RequestOptions::BODY] = $this->serializer->serialize($modified_entity, static::$format);
$response = $this->request('PATCH', $url, $request_options);
$this->assertResourceErrorResponse(403, "Access denied on updating field '" . $patch_protected_field_name . "'.", $response);
$this->assertResourceErrorResponse(403, "Access denied on updating field '" . $patch_protected_field_name . "'." . ($reason !== NULL ? ' ' . $reason : ''), $response);
$modified_entity->get($patch_protected_field_name)->setValue($original_values[$patch_protected_field_name]);
}
......@@ -1348,6 +1351,18 @@ protected function assertNormalizationEdgeCases($method, Url $url, array $reques
}
}
/**
* Asserts structure of $patchProtectedFieldNames.
*/
protected function assertPatchProtectedFieldNamesStructure() {
$is_null_or_string = function ($value) {
return is_null($value) || is_string($value);
};
$keys_are_field_names = Inspector::assertAllStrings(array_keys(static::$patchProtectedFieldNames));
$values_are_expected_access_denied_reasons = Inspector::assertAll($is_null_or_string, static::$patchProtectedFieldNames);
$this->assertTrue($keys_are_field_names && $values_are_expected_access_denied_reasons, 'In Drupal 8.6, the structure of $patchProtectectedFieldNames changed. It used to be an array with field names as values. Now those values are the keys, and their values should be either NULL or a string: a string containing the reason for why the field cannot be PATCHed, or NULL otherwise.');
}
/**
* Gets an entity resource's GET/PATCH/DELETE URL.
*
......@@ -1389,7 +1404,7 @@ protected function getEntityResourcePostUrl() {
protected static function getModifiedEntityForPatchTesting(EntityInterface $entity) {
$modified_entity = clone $entity;
$original_values = [];
foreach (static::$patchProtectedFieldNames as $field_name) {
foreach (array_keys(static::$patchProtectedFieldNames) as $field_name) {
$field = $modified_entity->get($field_name);
$original_values[$field_name] = $field->getValue();
switch ($field->getItemDefinition()->getClass()) {
......
......@@ -27,7 +27,7 @@ abstract class TermResourceTestBase extends EntityResourceTestBase {
* {@inheritdoc}
*/
protected static $patchProtectedFieldNames = [
'changed',
'changed' => NULL,
];
/**
......
......@@ -26,7 +26,7 @@ abstract class UserResourceTestBase extends EntityResourceTestBase {
* {@inheritdoc}
*/
protected static $patchProtectedFieldNames = [
'changed',
'changed' => NULL,
];
/**
......@@ -294,7 +294,7 @@ public function testPatchSecurityOtherUser() {
$response = $this->request('PATCH', $url, $request_options);
// Ensure the email address has not changed.
$this->assertEquals('admin@example.com', $this->entityStorage->loadUnchanged(1)->getEmail());
$this->assertResourceErrorResponse(403, "Access denied on updating field 'uid'.", $response);
$this->assertResourceErrorResponse(403, "Access denied on updating field 'uid'. The entity ID cannot be changed.", $response);
}
/**
......
......@@ -27,7 +27,9 @@ abstract class WorkspaceResourceTestBase extends EntityResourceTestBase {
/**
* {@inheritdoc}
*/
protected static $patchProtectedFieldNames = ['changed'];
protected static $patchProtectedFieldNames = [
'changed' => NULL,
];
/**
* {@inheritdoc}
......
......@@ -334,13 +334,13 @@ public function testFieldAccess($entity_class, array $entity_create_values, $exp
$this->assertFalse($entity->get('uuid')->access('edit'));
$access_result = $entity->get('uuid')->access('edit', NULL, TRUE);
$this->assertTrue($access_result->isForbidden());
$this->assertEquals('The entity UUID cannot be changed', $access_result->getReason());
$this->assertEquals('The entity UUID cannot be changed.', $access_result->getReason());
// Ensure the ID is still not allowed to be edited.
$this->assertFalse($entity->get('id')->access('edit'));
$access_result = $entity->get('id')->access('edit', NULL, TRUE);
$this->assertTrue($access_result->isForbidden());
$this->assertEquals('The entity ID cannot be changed', $access_result->getReason());
$this->assertEquals('The entity ID cannot be changed.', $access_result->getReason());
}
public function providerTestFieldAccess() {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment