From db390cded07b9b420e018f615c4b5e8d0ccdc5d6 Mon Sep 17 00:00:00 2001 From: Alex Pott Date: Fri, 14 Aug 2015 13:08:26 +0100 Subject: [PATCH] Issue #2430669 by larowlan, marthinal, clemens.tolboom, jibran: Cannot create comments from REST - field access is too strict --- .../src/CommentAccessControlHandler.php | 17 ++++-- .../src/Tests/CommentFieldAccessTest.php | 36 +++++++++++-- core/modules/rest/src/Tests/CreateTest.php | 53 ++++++++++++++++++- core/modules/rest/src/Tests/RESTTestBase.php | 27 ++++++++++ 4 files changed, 126 insertions(+), 7 deletions(-) diff --git a/core/modules/comment/src/CommentAccessControlHandler.php b/core/modules/comment/src/CommentAccessControlHandler.php index 405b1d2eaeb..984fda15b67 100644 --- a/core/modules/comment/src/CommentAccessControlHandler.php +++ b/core/modules/comment/src/CommentAccessControlHandler.php @@ -80,15 +80,26 @@ class CommentAccessControlHandler extends EntityAccessControlHandler { // No user can change read-only fields. $read_only_fields = array( 'hostname', - 'uuid', + 'changed', 'cid', 'thread', + ); + // These fields can be edited during comment creation. + $create_only_fields = [ 'comment_type', - 'pid', + 'uuid', 'entity_id', 'entity_type', 'field_name', - ); + 'pid', + ]; + if ($items && ($entity = $items->getEntity()) && $entity->isNew() && in_array($field_definition->getName(), $create_only_fields, TRUE)) { + // We are creating a new comment, user can edit create only fields. + return AccessResult::allowedIfHasPermission($account, 'post comments')->addCacheableDependency($entity); + } + // We are editing an existing comment - create only fields are now read + // only. + $read_only_fields = array_merge($read_only_fields, $create_only_fields); if (in_array($field_definition->getName(), $read_only_fields, TRUE)) { return AccessResult::forbidden(); } diff --git a/core/modules/comment/src/Tests/CommentFieldAccessTest.php b/core/modules/comment/src/Tests/CommentFieldAccessTest.php index bd08dfea59a..ba1c0d380c0 100644 --- a/core/modules/comment/src/Tests/CommentFieldAccessTest.php +++ b/core/modules/comment/src/Tests/CommentFieldAccessTest.php @@ -53,15 +53,23 @@ class CommentFieldAccessTest extends EntityUnitTestBase { protected $readOnlyFields = array( 'changed', 'hostname', - 'uuid', 'cid', 'thread', - 'comment_type', + ); + + /** + * These fields can be edited on create only. + * + * @var array + */ + protected $createOnlyFields = [ + 'uuid', 'pid', + 'comment_type', 'entity_id', 'entity_type', 'field_name', - ); + ]; /** * These fields can only be edited by the admin or anonymous users if allowed. @@ -252,6 +260,28 @@ class CommentFieldAccessTest extends EntityUnitTestBase { } } + // Check create-only fields. + foreach ($this->createOnlyFields as $field) { + // Check view operation. + foreach ($permutations as $set) { + $may_view = $set['comment']->{$field}->access('view', $set['user']); + $may_update = $set['comment']->{$field}->access('edit', $set['user']); + $this->assertEqual($may_view, $field != 'hostname' && ($set['user']->hasPermission('administer comments') || + ($set['comment']->isPublished() && $set['user']->hasPermission('access comments'))), SafeMarkup::format('User @user !state view field !field on comment @comment', [ + '@user' => $set['user']->getUsername(), + '!state' => $may_view ? 'can' : 'cannot', + '@comment' => $set['comment']->getSubject(), + '!field' => $field, + ])); + $this->assertEqual($may_update, $set['user']->hasPermission('post comments') && $set['comment']->isNew(), SafeMarkup::format('User @user !state update field !field on comment @comment', [ + '@user' => $set['user']->getUsername(), + '!state' => $may_update ? 'can' : 'cannot', + '@comment' => $set['comment']->getSubject(), + '!field' => $field, + ])); + } + } + // Check contact fields. foreach ($this->contactFields as $field) { // Check view operation. diff --git a/core/modules/rest/src/Tests/CreateTest.php b/core/modules/rest/src/Tests/CreateTest.php index 31fbd88e35f..77f70bebf5a 100644 --- a/core/modules/rest/src/Tests/CreateTest.php +++ b/core/modules/rest/src/Tests/CreateTest.php @@ -7,11 +7,13 @@ namespace Drupal\rest\Tests; +use Drupal\comment\Tests\CommentTestTrait; use Drupal\Component\Serialization\Json; use Drupal\Core\Entity\EntityInterface; use Drupal\entity_test\Entity\EntityTest; use Drupal\node\Entity\Node; use Drupal\user\Entity\User; +use Drupal\comment\Entity\Comment; /** * Tests the creation of resources. @@ -20,12 +22,13 @@ use Drupal\user\Entity\User; */ class CreateTest extends RESTTestBase { + use CommentTestTrait; /** * Modules to install. * * @var array */ - public static $modules = array('hal', 'rest', 'entity_test'); + public static $modules = array('hal', 'rest', 'entity_test', 'comment'); /** * The 'serializer' service. @@ -36,6 +39,7 @@ class CreateTest extends RESTTestBase { protected function setUp() { parent::setUp(); + $this->addDefaultCommentField('node', 'resttest'); // Get the 'serializer' service. $this->serializer = $this->container->get('serializer'); } @@ -224,6 +228,52 @@ class CreateTest extends RESTTestBase { } + /** + * Test comment creation. + */ + protected function testCreateComment() { + $node = Node::create([ + 'type' => 'resttest', + 'title' => 'some node', + ]); + $node->save(); + $entity_type = 'comment'; + // Enable the REST service for 'comment' entity type. + $this->enableService('entity:' . $entity_type, 'POST'); + // Create two accounts that have the required permissions to create + // resources, The second one has administrative permissions. + $accounts = $this->createAccountPerEntity($entity_type); + $account = end($accounts); + + $this->drupalLogin($account); + $entity_values = $this->entityValues($entity_type); + $entity_values['entity_id'] = $node->id(); + + $entity = Comment::create($entity_values); + + // Changed field can never be added. + unset($entity->changed); + + $serialized = $this->serializer->serialize($entity, $this->defaultFormat, ['account' => $account]); + + // Create the entity over the REST API. + $this->assertCreateEntityOverRestApi($entity_type, $serialized); + + // Get the new entity ID from the location header and try to read it from + // the database. + $this->assertReadEntityIdFromHeaderAndDb($entity_type, $entity, $entity_values); + + // Try to send invalid data that cannot be correctly deserialized. + $this->assertCreateEntityInvalidData($entity_type); + + // Try to send no data at all, which does not make sense on POST requests. + $this->assertCreateEntityNoData($entity_type); + + // Try to send invalid data to trigger the entity validation constraints. + // Send a UUID that is too long. + $this->assertCreateEntityInvalidSerialized($entity, $entity_type); + } + /** * Tests several valid and invalid create requests for 'user' entity type. */ @@ -293,6 +343,7 @@ class CreateTest extends RESTTestBase { // Add administrative permissions for nodes and users. $permissions[] = 'administer nodes'; $permissions[] = 'administer users'; + $permissions[] = 'administer comments'; // Create an administrative user. $accounts[] = $this->drupalCreateUser($permissions); diff --git a/core/modules/rest/src/Tests/RESTTestBase.php b/core/modules/rest/src/Tests/RESTTestBase.php index 46003af33ea..8fac798752f 100644 --- a/core/modules/rest/src/Tests/RESTTestBase.php +++ b/core/modules/rest/src/Tests/RESTTestBase.php @@ -213,6 +213,17 @@ abstract class RESTTestBase extends WebTestBase { ); case 'user': return array('name' => $this->randomMachineName()); + + case 'comment': + return [ + 'subject' => $this->randomMachineName(), + 'entity_type' => 'node', + 'comment_type' => 'comment', + 'comment_body' => $this->randomString(), + 'entity_id' => 'invalid', + 'field_name' => 'comment', + ]; + default: return array(); } @@ -313,6 +324,22 @@ abstract class RESTTestBase extends WebTestBase { return array('delete any resttest content'); } + case 'comment': + switch ($operation) { + case 'view': + return ['access comments']; + + case 'create': + return ['post comments', 'skip comment approval']; + + case 'update': + return ['edit own comments']; + + case 'delete': + return ['administer comments']; + } + break; + case 'user': switch ($operation) { case 'view':