From 84bbc5730a26e6e7f128b2290d306c2a38c0d26b Mon Sep 17 00:00:00 2001 From: xjm Date: Sun, 3 Sep 2017 06:10:11 -0500 Subject: Revert "Issue #2824851 by Wim Leers, arshadcn, amateescu, tedbow, timmillwood, Berdir, tstoeckler: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior" This reverts commit 3b3daa353f4c5ebd47d241aa1ac7e9af755f56f7. 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 9b0cee2..3edd9b1 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 98539a5..fc9248a 100644 --- a/core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonTestBase.php +++ b/core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonTestBase.php @@ -26,6 +26,25 @@ abstract class CommentHalJsonTestBase extends CommentResourceTestBase { */ 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 eb01944..b64de67 100644 --- a/core/modules/hal/tests/src/Functional/EntityResource/HalEntityNormalizationTrait.php +++ b/core/modules/hal/tests/src/Functional/EntityResource/HalEntityNormalizationTrait.php @@ -73,6 +73,24 @@ trait HalEntityNormalizationTrait { /** * {@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 ae4c964..7bbe218 100644 --- a/core/modules/hal/tests/src/Functional/EntityResource/Node/NodeHalJsonAnonTest.php +++ b/core/modules/hal/tests/src/Functional/EntityResource/Node/NodeHalJsonAnonTest.php @@ -33,6 +33,19 @@ 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 430adec..a98e718 100644 --- a/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php +++ b/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php @@ -4,7 +4,6 @@ namespace Drupal\path\Plugin\Field\FieldType; use Drupal\Core\Access\AccessResult; use Drupal\Core\Field\FieldItemList; -use Drupal\Core\Field\FieldItemListInterface; use Drupal\Core\Session\AccountInterface; /** @@ -73,12 +72,4 @@ class PathFieldItemList extends FieldItemList { } } - /** - * {@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 1087468..5d9849d 100644 --- a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php @@ -11,6 +11,8 @@ use Drupal\Core\Entity\FieldableEntityInterface; use Drupal\Core\Config\ConfigFactoryInterface; use Drupal\Core\Entity\EntityInterface; use Drupal\Core\Entity\EntityStorageException; +use Drupal\Core\Field\FieldItemListInterface; +use Drupal\Core\TypedData\PrimitiveInterface; use Drupal\rest\Plugin\ResourceBase; use Drupal\rest\ResourceResponse; use Psr\Log\LoggerInterface; @@ -200,6 +202,41 @@ class EntityResource extends ResourceBase implements DependentPluginInterface { } /** + * 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 @@ -225,26 +262,35 @@ class EntityResource extends ResourceBase implements DependentPluginInterface { throw new AccessDeniedHttpException($entity_access->getReason() ?: $this->generateFallbackAccessDeniedMessage($entity, 'update')); } - // Overwrite the received fields. + // Overwrite the received properties. + $entity_keys = $entity->getEntityType()->getKeys(); foreach ($entity->_restSubmittedFields as $field_name) { $field = $entity->get($field_name); - $original_field = $original_entity->get($field_name); - - // If the user has access to view the field, we need to check update - // access regardless of the field value to avoid information disclosure. - // (Otherwise the user may try PATCHing with value after value, until they - // send the current value for the field, and then they won't get a 403 - // response anymore, which indicates that the value they sent in the PATCH - // request body matches the current value.) - if (!$original_field->access('view')) { - if (!$original_field->access('edit')) { + + // 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; + } } - // 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. - elseif (!$original_field->equals($field) && !$original_field->access('edit')) { + + if (!$original_entity->get($field_name)->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/modules/rest_test/rest_test.module b/core/modules/rest/tests/modules/rest_test/rest_test.module index 469b4c7..7df2863 100644 --- a/core/modules/rest/tests/modules/rest_test/rest_test.module +++ b/core/modules/rest/tests/modules/rest_test/rest_test.module @@ -15,7 +15,6 @@ use Drupal\Core\Access\AccessResult; * * @see \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::setUp() * @see \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::testPost() - * @see \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::testPatch() */ function rest_test_entity_field_access($operation, FieldDefinitionInterface $field_definition, AccountInterface $account, FieldItemListInterface $items = NULL) { if ($field_definition->getName() === 'field_rest_test') { diff --git a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php index 72589bd..f39b7d8 100644 --- a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php @@ -3,18 +3,13 @@ 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; @@ -930,7 +925,6 @@ abstract class EntityResourceTestBase extends ResourceTestBase { $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: @@ -1042,33 +1036,22 @@ abstract class EntityResourceTestBase extends ResourceTestBase { $this->assertResourceErrorResponse(403, "Access denied on updating field 'field_rest_test'.", $response); - $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. + // 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); for ($i = 0; $i < count(static::$patchProtectedFieldNames); $i++) { - $patch_protected_field_name = static::$patchProtectedFieldNames[$i]; - $request_options[RequestOptions::BODY] = $this->serializer->serialize($modified_entity, static::$format); + $max_normalization = $this->removeFieldsFromNormalization($max_normalization, array_slice(static::$patchProtectedFieldNames, 0, $i)); + $request_options[RequestOptions::BODY] = $this->serializer->serialize($max_normalization, static::$format); $response = $this->request('PATCH', $url, $request_options); - $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]); + $this->assertResourceErrorResponse(403, "Access denied on updating field '" . static::$patchProtectedFieldNames[$i] . "'.", $response); } - // 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); + // 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); $response = $this->request('PATCH', $url, $request_options); $this->assertResourceResponse(200, FALSE, $response); @@ -1306,57 +1289,6 @@ abstract class EntityResourceTestBase extends ResourceTestBase { } /** - * Clones the given entity and modifies all PATCH-protected fields. - * - * @param \Drupal\Core\Entity\EntityInterface $entity - * The entity being tested and to modify. - * - * @return array - * Contains two items: - * 1. The modified entity object. - * 2. The original field values, keyed by field name. - * - * @internal - */ - 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: - $original_field = clone $field; - while ($field->equals($original_field)) { - $field->generateSampleItems(); - } - break; - } - } - - return [$modified_entity, $original_values]; - } - - /** * Makes the given entity normalization invalid. * * @param array $normalization @@ -1374,6 +1306,23 @@ abstract class EntityResourceTestBase extends ResourceTestBase { } /** + * 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. -- cgit v0.10.2