.../Comment/CommentHalJsonAnonTest.php | 2 +- .../Comment/CommentHalJsonTestBase.php | 19 ------ .../EntityResource/HalEntityNormalizationTrait.php | 18 ----- .../EntityResource/Node/NodeHalJsonAnonTest.php | 13 ---- .../Plugin/Field/FieldType/PathFieldItemList.php | 9 +++ .../src/Plugin/rest/resource/EntityResource.php | 67 ++----------------- .../EntityResource/EntityResourceTestBase.php | 76 +++++++++++++--------- 7 files changed, 63 insertions(+), 141 deletions(-) diff --git a/core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonAnonTest.php b/core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonAnonTest.php index 3edd9b1..9b0cee2 100644 --- a/core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonAnonTest.php +++ b/core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonAnonTest.php @@ -25,11 +25,11 @@ class CommentHalJsonAnonTest extends CommentHalJsonTestBase { * @see ::setUpAuthorization */ protected static $patchProtectedFieldNames = [ + 'entity_id', 'changed', 'thread', 'entity_type', 'field_name', - 'entity_id', ]; } diff --git a/core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonTestBase.php b/core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonTestBase.php index fc9248a..98539a5 100644 --- a/core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonTestBase.php +++ b/core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonTestBase.php @@ -26,25 +26,6 @@ */ protected static $mimeType = 'application/hal+json'; - /** - * {@inheritdoc} - * - * The HAL+JSON format causes different PATCH-protected fields. For some - * reason, the 'pid' and 'homepage' fields are NOT PATCH-protected, even - * though they are for non-HAL+JSON serializations. - * - * @todo fix in https://www.drupal.org/node/2824271 - */ - protected static $patchProtectedFieldNames = [ - 'status', - 'created', - 'changed', - 'thread', - 'entity_type', - 'field_name', - 'entity_id', - 'uid', - ]; /** * {@inheritdoc} diff --git a/core/modules/hal/tests/src/Functional/EntityResource/HalEntityNormalizationTrait.php b/core/modules/hal/tests/src/Functional/EntityResource/HalEntityNormalizationTrait.php index b64de67..eb01944 100644 --- a/core/modules/hal/tests/src/Functional/EntityResource/HalEntityNormalizationTrait.php +++ b/core/modules/hal/tests/src/Functional/EntityResource/HalEntityNormalizationTrait.php @@ -73,24 +73,6 @@ protected function applyHalFieldNormalization(array $normalization) { /** * {@inheritdoc} */ - protected function removeFieldsFromNormalization(array $normalization, $field_names) { - $normalization = parent::removeFieldsFromNormalization($normalization, $field_names); - foreach ($field_names as $field_name) { - $relation_url = Url::fromUri('base:rest/relation/' . static::$entityTypeId . '/' . $this->entity->bundle() . '/' . $field_name) - ->setAbsolute(TRUE) - ->toString(); - $normalization['_links'] = array_diff_key($normalization['_links'], [$relation_url => TRUE]); - if (isset($normalization['_embedded'])) { - $normalization['_embedded'] = array_diff_key($normalization['_embedded'], [$relation_url => TRUE]); - } - } - - return array_diff_key($normalization, array_flip($field_names)); - } - - /** - * {@inheritdoc} - */ protected function assertNormalizationEdgeCases($method, Url $url, array $request_options) { // \Drupal\hal\Normalizer\EntityNormalizer::denormalize(): entity // types with bundles MUST send their bundle field to be denormalizable. diff --git a/core/modules/hal/tests/src/Functional/EntityResource/Node/NodeHalJsonAnonTest.php b/core/modules/hal/tests/src/Functional/EntityResource/Node/NodeHalJsonAnonTest.php index 7bbe218..ae4c964 100644 --- a/core/modules/hal/tests/src/Functional/EntityResource/Node/NodeHalJsonAnonTest.php +++ b/core/modules/hal/tests/src/Functional/EntityResource/Node/NodeHalJsonAnonTest.php @@ -33,19 +33,6 @@ class NodeHalJsonAnonTest extends NodeResourceTestBase { /** * {@inheritdoc} */ - protected static $patchProtectedFieldNames = [ - 'revision_timestamp', - 'created', - 'changed', - 'promote', - 'sticky', - 'path', - 'revision_uid', - ]; - - /** - * {@inheritdoc} - */ protected function getExpectedNormalizedEntity() { $default_normalization = parent::getExpectedNormalizedEntity(); diff --git a/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php b/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php index a98e718..430adec 100644 --- a/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php +++ b/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php @@ -4,6 +4,7 @@ use Drupal\Core\Access\AccessResult; use Drupal\Core\Field\FieldItemList; +use Drupal\Core\Field\FieldItemListInterface; use Drupal\Core\Session\AccountInterface; /** @@ -72,4 +73,12 @@ protected function ensureLoaded() { } } + /** + * {@inheritdoc} + */ + public function equals(FieldItemListInterface $list_to_compare) { + $this->ensureLoaded(); + return parent::equals($list_to_compare); + } + } diff --git a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php index 5d9849d..b4352b7 100644 --- a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php @@ -202,41 +202,6 @@ public function post(EntityInterface $entity = NULL) { } /** - * Gets the values from the field item list casted to the correct type. - * - * Values are casted to the correct type so we can determine whether or not - * something has changed. REST formats such as JSON support typed data but - * Drupal's database API will return values as strings. Currently, only - * primitive data types know how to cast their values to the correct type. - * - * @param \Drupal\Core\Field\FieldItemListInterface $field_item_list - * The field item list to retrieve its data from. - * - * @return mixed[][] - * The values from the field item list casted to the correct type. The array - * of values returned is a multidimensional array keyed by delta and the - * property name. - */ - protected function getCastedValueFromFieldItemList(FieldItemListInterface $field_item_list) { - $value = $field_item_list->getValue(); - - foreach ($value as $delta => $field_item_value) { - /** @var \Drupal\Core\Field\FieldItemInterface $field_item */ - $field_item = $field_item_list->get($delta); - $properties = $field_item->getProperties(TRUE); - // Foreach field value we check whether we know the underlying property. - // If we exists we try to cast the value. - foreach ($field_item_value as $property_name => $property_value) { - if (isset($properties[$property_name]) && ($property = $field_item->get($property_name)) && $property instanceof PrimitiveInterface) { - $value[$delta][$property_name] = $property->getCastedValue(); - } - } - } - - return $value; - } - - /** * Responds to entity PATCH requests. * * @param \Drupal\Core\Entity\EntityInterface $original_entity @@ -262,35 +227,15 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity throw new AccessDeniedHttpException($entity_access->getReason() ?: $this->generateFallbackAccessDeniedMessage($entity, 'update')); } - // Overwrite the received properties. - $entity_keys = $entity->getEntityType()->getKeys(); + // Overwrite the received fields. foreach ($entity->_restSubmittedFields as $field_name) { $field = $entity->get($field_name); + $original_field = $original_entity->get($field_name); - // Entity key fields need special treatment: together they uniquely - // identify the entity. Therefore it does not make sense to modify any of - // them. However, rather than throwing an error, we just ignore them as - // long as their specified values match their current values. - if (in_array($field_name, $entity_keys, TRUE)) { - // @todo Work around the wrong assumption that entity keys need special - // treatment, when only read-only fields need it. - // This will be fixed in https://www.drupal.org/node/2824851. - if ($entity->getEntityTypeId() == 'comment' && $field_name == 'status' && !$original_entity->get($field_name)->access('edit')) { - throw new AccessDeniedHttpException("Access denied on updating field '$field_name'."); - } - - // Unchanged values for entity keys don't need access checking. - if ($this->getCastedValueFromFieldItemList($original_entity->get($field_name)) === $this->getCastedValueFromFieldItemList($entity->get($field_name))) { - continue; - } - // It is not possible to set the language to NULL as it is automatically - // re-initialized. As it must not be empty, skip it if it is. - elseif (isset($entity_keys['langcode']) && $field_name === $entity_keys['langcode'] && $field->isEmpty()) { - continue; - } - } - - if (!$original_entity->get($field_name)->access('edit')) { + // Check access for all received fields, but only if they are being + // changed. The bundle of an entity, for example, must be provided for + // denormalization to succeed, but it may not be changed. + if (!$original_field->equals($field) && !$original_field->access('edit')) { throw new AccessDeniedHttpException("Access denied on updating field '$field_name'."); } $original_entity->set($field_name, $field->getValue()); diff --git a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php index 8638adf..c8240d2 100644 --- a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php @@ -3,13 +3,17 @@ namespace Drupal\Tests\rest\Functional\EntityResource; use Drupal\Component\Utility\NestedArray; +use Drupal\Component\Utility\Random; use Drupal\Core\Cache\Cache; use Drupal\Core\Cache\CacheableResponseInterface; use Drupal\Core\Config\Entity\ConfigEntityInterface; use Drupal\Core\Entity\FieldableEntityInterface; +use Drupal\Core\Field\Plugin\Field\FieldType\BooleanItem; +use Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem; use Drupal\Core\Url; use Drupal\field\Entity\FieldConfig; use Drupal\field\Entity\FieldStorageConfig; +use Drupal\path\Plugin\Field\FieldType\PathItem; use Drupal\rest\ResourceResponseInterface; use Drupal\Tests\rest\Functional\ResourceTestBase; use GuzzleHttp\RequestOptions; @@ -1020,22 +1024,53 @@ public function testPatch() { $this->assertResourceErrorResponse(403, "Access denied on updating field 'field_rest_test'.", $response); - // DX: 403 when sending PATCH request with read-only fields. - // First send all fields (the "maximum normalization"). Assert the expected - // error message for the first PATCH-protected field. Remove that field from - // the normalization, send another request, assert the next PATCH-protected - // field error message. And so on. - $max_normalization = $this->getNormalizedPatchEntity() + $this->serializer->normalize($this->entity, static::$format); + // DX: 403 when sending PATCH request with updated read-only fields. + // Clone the entity, modifies all PATCH-protected fields. Then performs a + $modified_entity = clone $this->entity; + $original_values = []; + foreach (static::$patchProtectedFieldNames as $field_name) { + $field = $modified_entity->get($field_name); + $original_values[$field_name] = $field->getValue(); + switch ($field->getItemDefinition()->getClass()) { + case EntityReferenceItem::class: + // EntityReferenceItem::generateSampleValue() picks one of the last 50 + // entities of the supported type & bundle. We don't care if the value + // is valid, we only care that it's different. + $field->setValue(['target_id' => 99999]); + break; + case BooleanItem::class: + // BooleanItem::generateSampleValue() picks either 0 or 1. So a 50% + // chance of not picking a different value. + $field->value = ((int) $field->value) === 1 ? '0' : '1'; + break; + case PathItem::class: + // PathItem::generateSampleValue() doesn't set a PID, which causes + // PathItem::postSave() to fail. Keep the PID (and other properties), + // just modify the alias. + $value = $field->getValue(); + $value['alias'] = str_replace(' ', '-', strtolower((new Random())->sentences(3))); + $field->setValue($value); + break; + default: + $field->generateSampleItems(); + break; + } + } + // Send PATCH request by serializing the cloned entity, assert the error + // response, change the cloned entity field that caused the error response + // back to its original value, repeat. for ($i = 0; $i < count(static::$patchProtectedFieldNames); $i++) { - $max_normalization = $this->removeFieldsFromNormalization($max_normalization, array_slice(static::$patchProtectedFieldNames, 0, $i)); - $request_options[RequestOptions::BODY] = $this->serializer->serialize($max_normalization, static::$format); + $patch_protected_field_name = static::$patchProtectedFieldNames[$i]; + $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 '" . static::$patchProtectedFieldNames[$i] . "'.", $response); + $this->assertResourceErrorResponse(403, "Access denied on updating field '" . $patch_protected_field_name . "'.", $response); + $modified_entity->get($patch_protected_field_name)->setValue($original_values[$patch_protected_field_name]); } - // 200 for well-formed request that sends the maximum number of fields. - $max_normalization = $this->removeFieldsFromNormalization($max_normalization, static::$patchProtectedFieldNames); - $request_options[RequestOptions::BODY] = $this->serializer->serialize($max_normalization, static::$format); + // 200 for well-formed PATCH request that sends all fields (even including + // read-only ones, but with unchanged values). + $valid_request_body = $this->getNormalizedPatchEntity() + $this->serializer->normalize($this->entity, static::$format); + $request_options[RequestOptions::BODY] = $this->serializer->serialize($valid_request_body, static::$format); $response = $this->request('PATCH', $url, $request_options); $this->assertResourceResponse(200, FALSE, $response); @@ -1275,23 +1310,6 @@ protected function makeNormalizationInvalid(array $normalization) { } /** - * Removes fields from a normalization. - * - * @param array $normalization - * An entity normalization. - * @param string[] $field_names - * The field names to remove from the entity normalization. - * - * @return array - * The updated entity normalization. - * - * @see ::testPatch - */ - protected function removeFieldsFromNormalization(array $normalization, $field_names) { - return array_diff_key($normalization, array_flip($field_names)); - } - - /** * Asserts a 406 response… or in some cases a 403 response, because weirdness. * * Asserting a 406 response should be easy, but it's not, due to bugs.