.../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 | 104 +++++++++++++++------ 7 files changed, 91 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..3a18303 100644 --- a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php @@ -3,13 +3,18 @@ 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\EntityInterface; 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; @@ -908,6 +913,7 @@ public function testPatch() { $parseable_valid_request_body_2 = $this->serializer->encode($this->getNormalizedPatchEntity(), static::$format); $parseable_invalid_request_body = $this->serializer->encode($this->makeNormalizationInvalid($this->getNormalizedPatchEntity()), static::$format); $parseable_invalid_request_body_2 = $this->serializer->encode($this->getNormalizedPatchEntity() + ['field_rest_test' => [['value' => $this->randomString()]]], static::$format); + $parseable_invalid_request_body_3 = $this->serializer->encode($this->getNormalizedPatchEntity() + ['field_rest_test' => [['value' => 'All the faith he had had had had no effect on the outcome of his life.', 'format' => NULL]]], static::$format); // The URL and Guzzle request options that will be used in this test. The // request options will be modified/expanded throughout this test: @@ -1020,22 +1026,33 @@ 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); + $request_options[RequestOptions::BODY] = $parseable_invalid_request_body_3; + + + // DX: 403 when entity contains field without 'edit' nor 'view' access, even + // when the value for that field matches the current value. This is allowed + // in principle, but leads to information disclosure. + $response = $this->request('PATCH', $url, $request_options); + $this->assertResourceErrorResponse(403, "Access denied on updating field 'field_rest_test'.", $response); + + + // DX: 403 when sending PATCH request with updated read-only fields. + 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++) { - $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); @@ -1258,37 +1275,66 @@ protected function getEntityResourcePostUrl() { } /** - * Makes the given entity normalization invalid. + * Clones the given entity and modifies all PATCH-protected fields. * - * @param array $normalization - * An entity normalization. + * @param \Drupal\Core\Entity\EntityInterface $entity + * The entity being tested and to modify. * * @return array - * The updated entity normalization, now invalid. + * Contains two items: + * 1. The modified entity object. + * 2. The original field values, keyed by field name. */ - protected function makeNormalizationInvalid(array $normalization) { - // Add a second label to this entity to make it invalid. - $label_field = $this->entity->getEntityType()->hasKey('label') ? $this->entity->getEntityType()->getKey('label') : static::$labelFieldName; - $normalization[$label_field][1]['value'] = 'Second Title'; + protected static function getModifiedEntityForPatchTesting(EntityInterface $entity) { + $modified_entity = clone $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; + } + } - return $normalization; + return [$modified_entity, $original_values]; } /** - * Removes fields from a normalization. + * Makes the given entity normalization invalid. * * @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 + * The updated entity normalization, now invalid. */ - protected function removeFieldsFromNormalization(array $normalization, $field_names) { - return array_diff_key($normalization, array_flip($field_names)); + protected function makeNormalizationInvalid(array $normalization) { + // Add a second label to this entity to make it invalid. + $label_field = $this->entity->getEntityType()->hasKey('label') ? $this->entity->getEntityType()->getKey('label') : static::$labelFieldName; + $normalization[$label_field][1]['value'] = 'Second Title'; + + return $normalization; } /**