Cannot create comments from REST - field access is too strict [#2430669]

Problem/Motivation

It is not possible to create a comment via a REST POST because the field access is too strict.

Well, this is the way to add a comment using hal+json

{
    "_links": {
        "type": {
            "href": "http://d8/rest/type/comment/comment_type"
        },
        "http://d8/rest/relation/comment/comment/entity_id": [
            {
                "href": "http://d8/node/1"
            }
        ]
    },
    "entity_type": [{
            "value": "node"
        }],
    "entity_id":
      [{"target_id":"1"
}],
    "field_name": [{
        "value":"field_test_comment"
    }],
    "uid": [{
        "target_id":"1"
    }],
    "comment_body":[{
      "value":"HEYYYYYYYYYYY"
    }]
}

Here another example #2300827-19: How to POST a comment and other entities using entity references

So, if you avoid the validation, the comment is created as expected. It is not correct because we need to avoid to add the uid for example and of course we need the validation.

But, if you try to add the comment using the validation you will see this error:

{"error":"Access denied on creating field entity_id"}

or the same for entity_type...

If you remove the field from the json then you will see:

{"error":"Unprocessable Entity: validation failed.\nentity_id: This value should not be null.\n"}

And the problem is here (CommentAccessControlHandler::checkFieldAccess)

      // No user can change read-only fields.
      $read_only_fields = array(
        'hostname',
        'uuid',
        'cid',
        'thread',
        'comment_type',
        'pid',
        'entity_id',
        'entity_type',
        'field_name',
      );
      if (in_array($field_definition->getName(), $read_only_fields, TRUE)) {
        return AccessResult::forbidden();
      }

Proposed resolution

Some of the read-only fields are in fact editable when the comment is new. Update access controller to reflect this.

Remaining tasks

Reviews

Original report by @marthinal

Comment	File	Size	Author
#13	comment-rest-access-2430669.pass_.patch	8.41 KB	larowlan
#13	comment-rest-access-2430669.fail_.patch	6.97 KB	larowlan
#4	2430669-4.patch	1.83 KB	marthinal
#2	2430669-2.patch	1021 bytes	marthinal

Comments

Comment #1

marthinal commented 21 February 2015 at 07:54

Issue summary:

View changes

Comment #2

marthinal commented 21 February 2015 at 18:45

Status:

Needs work

» Needs review

Status	File	Size
new	2430669-2.patch	1021 bytes

Working a little bit more on this I have detected that we set to 0 the uid by default. No uid is added when creating a comment programatically. This patch fixes the problem but maybe we should add this method(getCurrentUserId()) to the ContentEntityBase class...

Comment #3

larowlan

🇦🇺🏝.au GMT+10

commented 21 February 2015 at 20:30

So that fixes entity type too, or are you fixing something else with that patch?

Comment #4

marthinal commented 23 February 2015 at 09:41

Status	File	Size
new	2430669-4.patch	1.83 KB

@larowlan I was adding the uid by default when creating a comment programatically. The current patch fixes the entity type problem.

By default only cid and uuid are read only fields...

    $fields['cid'] = BaseFieldDefinition::create('integer')
      ->setLabel(t('Comment ID'))
      ->setDescription(t('The comment ID.'))
      <strong>->setReadOnly(TRUE)</strong>
      ->setSetting('unsigned', TRUE);

    $fields['uuid'] = BaseFieldDefinition::create('uuid')
      ->setLabel(t('UUID'))
      ->setDescription(t('The comment UUID.'))
     <strong> ->setReadOnly(TRUE);</strong>

Comment #5

larowlan

🇦🇺🏝.au GMT+10

commented 23 February 2015 at 09:53

Yeah but this is going to impact #2227503: Apply formatters and widgets to Comment base fields - we have to make sure those fields don't show up in the edit form after we change the access controls.

Also, expect CommentFieldAccessTest will fail with current patch.

Comment #6

23 February 2015 at 09:58

Status:

Needs review

» Needs work

The last submitted patch, 4: 2430669-4.patch, failed testing.

Comment #7

clemens.tolboom

Dutch

Groningen, 🇳🇱/🇪🇺

commented 12 April 2015 at 11:23

Issue summary:

View changes

In trying patch from #2 or #4 I cannot post a comment.

It fails on \Drupal\comment\Plugin\Validation\Constraint\CommentNameConstraintValidator maybe due to anonymous user?

I thought this issue was connected with #1964034: Pass entity_type into Serializer via context but are not sure anymore. To post a comment we now need lot of tricky data: entity_type, entity_id, field_name of which the first 2 are duplicates as their values are derivable from _links.

I've created #2469581: Add tests for comments CRUD for REST as we are chasing head for POST comments :-/

Comment #8

larowlan

🇦🇺🏝.au GMT+10

commented 12 August 2015 at 04:50

Issue tags:

+Needs reroll

tagging

Comment #9

larowlan

🇦🇺🏝.au GMT+10

commented 12 August 2015 at 06:09

Assigned:

Unassigned

» larowlan

Looking

Comment #10

larowlan

🇦🇺🏝.au GMT+10

commented 12 August 2015 at 06:10

Component:

rest.module

» comment.module

Comment #11

larowlan

🇦🇺🏝.au GMT+10

commented 12 August 2015 at 09:35

Closed #2469581: Add tests for comments CRUD for REST in favour of this. Bringing in the tests to here.

Comment #12

alexpott

he/they

English

🇪🇺🌍

commented 12 August 2015 at 09:36

Creating @clemens.tolboom as requested by @larowlan

Comment #13

larowlan

🇦🇺🏝.au GMT+10

commented 12 August 2015 at 09:53

Status:	Needs work	» Needs review
Issue tags:	-Needs tests, -Needs reroll

Status	File	Size
new	comment-rest-access-2430669.fail_.patch	6.97 KB
new	comment-rest-access-2430669.pass_.patch	8.41 KB

Brings in and finishes tests from #2469581: Add tests for comments CRUD for REST and changes implementation to make sure that the fields are only editable on create, when the entity is new.

Comment #14

12 August 2015 at 10:19

The last submitted patch, 13: comment-rest-access-2430669.fail_.patch, failed testing.

Comment #15

larowlan

🇦🇺🏝.au GMT+10

commented 12 August 2015 at 21:22

Issue summary:

View changes

Issue summary changes

Comment #16

larowlan

🇦🇺🏝.au GMT+10

commented 12 August 2015 at 21:22

Title:

Cannot create comments from REST {"error":"Access denied on creating field entity_id"}

» Cannot create comments from REST - field access is too strict

Comment #17

jibran

he/him

Urdu

Toronto, Canada

commented 13 August 2015 at 22:53

+++ b/core/modules/comment/src/CommentAccessControlHandler.php
@@ -80,15 +80,26 @@ protected function checkFieldAccess($operation, FieldDefinitionInterface $field_
+        'uuid',

How is this create only field?

Comment #18

larowlan

🇦🇺🏝.au GMT+10

commented 13 August 2015 at 22:49

How is this create only field?

You can't edit uuid after an entity is created right?

Comment #19

larowlan

🇦🇺🏝.au GMT+10

commented 13 August 2015 at 23:09

@jibran and I discussed this on IRC - whether uuid should be read only, or allowed on creation. Based on our existing tests we concluded that it is allowed on creation.

logs below

jibran
larowlan: ping
larowlan: My point was user can set uid field ever so why we are allowing it for comment creation.
8:55
larowlan
jibran: uuid or uid?
jibran: you are saying it should not be settable at all?
8:55
jibran
larowlan: uuid*
larowlan: afaik it is not settable at all
8:57
larowlan
jibran: it is via the serializer
jibran: e.g. content staging scenario where config depends on content with given uuid, you need to be able to nominate the uuid so reference remain intact
jibran: all of our rest tests enforce that
8:58
jibran
larowlan: does default content set uuid in yml files?
8:59
larowlan
jibran: e.g. all entity types in CreateTest explicitly test that you can send a uuid
jibran: yes default content does, thats how ER fields are dereferenced
8:59
jibran
larowlan: config importer use uuid to update configs.
larowlan: if we import new config with uuid using importer it throws error that config doesn't exist.
9:01
larowlan
jibran: this is content, not config
9:02
jibran
larowlan: "all entity types in CreateTest explicitly test that you can send a uuid" which test?
9:02
larowlan
jibran: if you can ship a view that relies on an entity for its header with a given uuid, then you have to be able to create content with that uuid
9:02
jibran
larowlan: I want to have a look
larowlan: yeah I know this case
9:02
larowlan
jibran: in rest CreateTest uses the serializer to turn an unsaved entity (created with EntityType::create) into hal+json
jibran: then it posts that
jibran: then it asserts all the fields sent were found in the entity after loading it back from the db
jibran: uuid is one of those fields
jibran: some fields are explicitly unset before serializing (e.g. changed)

Comment #20

jibran

he/him

Urdu

Toronto, Canada

commented 13 August 2015 at 23:20

Status:

Needs review

» Reviewed & tested by the community

Patch has a dedicated test for access check which is good and it also has a comment creation test using rest api which fixes the original bug so it's awesome. Code changes look good so RTBC.

Comment #21

alexpott

he/they

English

🇪🇺🌍

commented 14 August 2015 at 12:09

Status:

Reviewed & tested by the community

» Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed db390cd and pushed to 8.0.x. Thanks!

Comment #22

14 August 2015 at 12:24

alexpott committed db390cd on 8.0.x

Issue #2430669 by larowlan, marthinal, clemens.tolboom, jibran: Cannot...

Comment #23

28 August 2015 at 12:24

Status:

Fixed

» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Cannot create comments from REST - field access is too strict

Problem/Motivation

Proposed resolution

Remaining tasks

Original report by @marthinal

Comments

Comment #1

Comment #2

Comment #3

Comment #4

Comment #5

Comment #6

Comment #7

Comment #8

Comment #9

Comment #10

Comment #11

Comment #12

Comment #13

Comment #14

Comment #15

Comment #16

Comment #17

Comment #18

Comment #19

Comment #20

Comment #21

Comment #22

Comment #23

Referenced by

News items

Our community

Documentation

Drupal code base

Governance of community