Issue #2824851 by Wim Leers, arshadcn, amateescu, effulgentsia, tedbow,...

Issue #2824851 by Wim Leers, arshadcn, amateescu, effulgentsia, tedbow, timmillwood, cburschka, tstoeckler, Berdir, xjm, catch: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior

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',
   ];
 
 }

core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonTestBase.php

@@ -26,25 +26,6 @@ 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}

core/modules/hal/tests/src/Functional/EntityResource/HalEntityNormalizationTrait.php

@@ -70,24 +70,6 @@ protected function applyHalFieldNormalization(array $normalization) {
     return $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}
    */

core/modules/hal/tests/src/Functional/EntityResource/Node/NodeHalJsonAnonTest.php

@@ -30,19 +30,6 @@ class NodeHalJsonAnonTest extends NodeResourceTestBase {
    */
   protected static $mimeType = 'application/hal+json';
 
-  /**
-   * {@inheritdoc}
-   */
-  protected static $patchProtectedFieldNames = [
-    'revision_timestamp',
-    'created',
-    'changed',
-    'promote',
-    'sticky',
-    'path',
-    'revision_uid',
-  ];
-
   /**
    * {@inheritdoc}
    */

core/modules/rest/src/Plugin/rest/resource/EntityResource.php

@@ -11,6 +11,7 @@
 use Drupal\Core\Config\ConfigFactoryInterface;
 use Drupal\Core\Entity\EntityInterface;
 use Drupal\Core\Entity\EntityStorageException;
+use Drupal\Core\Field\FieldItemListInterface;
 use Drupal\Core\Http\Exception\CacheableAccessDeniedHttpException;
 use Drupal\rest\Plugin\ResourceBase;
 use Drupal\rest\ResourceResponse;
@@ -226,38 +227,18 @@ 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);
-
-      // 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 ($original_entity->get($field_name)->equals($field)) {
-          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;
-        }
+      // 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.
+      // @todo Remove in https://www.drupal.org/project/drupal/issues/2933408.
+      if ($entity->getEntityType()->hasKey('langcode') && $field_name === $entity->getEntityType()->getKey('langcode') && $field->isEmpty()) {
+        continue;
       }
-
-      if (!$original_entity->get($field_name)->access('edit')) {
-        throw new AccessDeniedHttpException("Access denied on updating field '$field_name'.");
+      if ($this->checkPatchFieldAccess($original_entity->get($field_name), $field)) {
+        $original_entity->set($field_name, $field->getValue());
       }
-      $original_entity->set($field_name, $field->getValue());
     }
 
     // Validate the received data before saving.
@@ -274,6 +255,49 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
     }
   }
 
+  /**
+   * Checks whether the given field should be PATCHed.
+   *
+   * @param \Drupal\Core\Field\FieldItemListInterface $original_field
+   *   The original (stored) value for the field.
+   * @param \Drupal\Core\Field\FieldItemListInterface $received_field
+   *   The received value for the field.
+   *
+   * @return bool
+   *   Whether the field should be PATCHed or not.
+   *
+   * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
+   *   Thrown when the user sending the request is not allowed to update the
+   *   field. Only thrown when the user could not abuse this information to
+   *   determine the stored value.
+   *
+   * @internal
+   */
+  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')) {
+      return TRUE;
+    }
+
+    // The user might not have access to edit the field, but still needs to
+    // submit the current field value as part of the PATCH request. For
+    // example, the entity keys required by denormalizers. Therefore, if the
+    // received value equals the stored value, return FALSE without throwing an
+    // exception. But only for fields that the user has access to view, because
+    // the user has no legitimate way of knowing the current value of fields
+    // that they are not allowed to view, and we must not make the presence or
+    // absence of a 403 response a way to find that out.
+    if ($original_field->equals($received_field) && $original_field->access('view')) {
+      return FALSE;
+    }
+
+    // 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'.");
+  }
+
   /**
    * Responds to entity DELETE requests.
    *

core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php

@@ -3,15 +3,20 @@
 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\Cache\CacheableMetadata;
 use Drupal\Core\Config\Entity\ConfigEntityInterface;
 use Drupal\Core\Entity\ContentEntityNullStorage;
+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;
@@ -906,6 +911,10 @@ 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);
+    // The 'field_rest_test' field does not allow 'view' access, so does not end
+    // up in the normalization. Even when we explicitly add it the normalization
+    // that we send in the body of a PATCH request, it is considered invalid.
+    $parseable_invalid_request_body_3 = $this->serializer->encode($this->getNormalizedPatchEntity() + ['field_rest_test' => $this->entity->get('field_rest_test')->getValue()], 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:
@@ -997,22 +1006,31 @@ public function testPatch() {
     $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 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);
 
@@ -1235,37 +1253,71 @@ 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.
+   *
+   * @internal
    */
-  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:
+          $original_field = clone $field;
+          while ($field->equals($original_field)) {
+            $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;
   }
 
   /**

core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php

@@ -235,20 +235,6 @@ public function testPatchPath() {
     $response = $this->request('GET', $url, $this->getAuthenticationRequestOptions('GET'));
     $normalization = $this->serializer->decode((string) $response->getBody(), static::$format);
 
-    // @todo In https://www.drupal.org/node/2824851, we will be able to stop
-    //       unsetting these fields from the normalization, because
-    //       EntityResource::patch() will ignore any fields that are sent that
-    //       match the current value (and obviously we're sending the current
-    //       value).
-    $normalization = $this->removeFieldsFromNormalization($normalization, [
-      'revision_timestamp',
-      'revision_uid',
-      'created',
-      'changed',
-      'promote',
-      'sticky',
-    ]);
-
     // Change node's path alias.
     $normalization['path'][0]['alias'] .= 's-rule-the-world';
 
@@ -258,8 +244,11 @@ public function testPatchPath() {
     $request_options = array_merge_recursive($request_options, $this->getAuthenticationRequestOptions('PATCH'));
     $request_options[RequestOptions::BODY] = $this->serializer->encode($normalization, static::$format);
 
-    // PATCH request: 403 when creating URL aliases unauthorized.
+    // PATCH request: 403 when creating URL aliases unauthorized. Before
+    // asserting the 403 response, assert that the stored path alias remains
+    // 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);
 
     // Grant permission to create URL aliases.

core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php

@@ -208,15 +208,6 @@ public function testPatchPath() {
     $response = $this->request('GET', $url, $this->getAuthenticationRequestOptions('GET'));
     $normalization = $this->serializer->decode((string) $response->getBody(), static::$format);
 
-    // @todo In https://www.drupal.org/node/2824851, we will be able to stop
-    //       unsetting these fields from the normalization, because
-    //       EntityResource::patch() will ignore any fields that are sent that
-    //       match the current value (and obviously we're sending the current
-    //       value).
-    $normalization = $this->removeFieldsFromNormalization($normalization, [
-      'changed',
-    ]);
-
     // Change term's path alias.
     $normalization['path'][0]['alias'] .= 's-rule-the-world';