summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorNathaniel Catchpole2017-08-22 16:54:10 +0900
committerNathaniel Catchpole2017-08-22 16:54:10 +0900
commit3b3daa353f4c5ebd47d241aa1ac7e9af755f56f7 (patch)
treea98a80ca4af9121ed30ff6c318d34a9b24874f3f
parent4884fb154f26a469cc524a7f85dbb6ee845840c7 (diff)
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
-rw-r--r--core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonAnonTest.php2
-rw-r--r--core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonTestBase.php19
-rw-r--r--core/modules/hal/tests/src/Functional/EntityResource/HalEntityNormalizationTrait.php18
-rw-r--r--core/modules/hal/tests/src/Functional/EntityResource/Node/NodeHalJsonAnonTest.php13
-rw-r--r--core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php9
-rw-r--r--core/modules/rest/src/Plugin/rest/resource/EntityResource.php76
-rw-r--r--core/modules/rest/tests/modules/rest_test/rest_test.module1
-rw-r--r--core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php109
8 files changed, 106 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 @@ 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 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 @@ 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 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 @@ 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;
/**
@@ -72,4 +73,12 @@ 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 5d9849d..1087468 100644
--- a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -11,8 +11,6 @@ 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;
@@ -202,41 +200,6 @@ 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
@@ -262,35 +225,26 @@ class EntityResource extends ResourceBase implements DependentPluginInterface {
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')) {
+ $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')) {
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.
+ elseif (!$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/modules/rest_test/rest_test.module b/core/modules/rest/tests/modules/rest_test/rest_test.module
index 7df2863..469b4c7 100644
--- a/core/modules/rest/tests/modules/rest_test/rest_test.module
+++ b/core/modules/rest/tests/modules/rest_test/rest_test.module
@@ -15,6 +15,7 @@ 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 f39b7d8..72589bd 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;
@@ -925,6 +930,7 @@ 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:
@@ -1036,22 +1042,33 @@ abstract class EntityResourceTestBase extends ResourceTestBase {
$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);
@@ -1289,37 +1306,71 @@ abstract class EntityResourceTestBase extends ResourceTestBase {
}
/**
- * 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;
}
/**