Cannot create user entities - {"error":"Access denied on creating field pass"}

That is correct, it is not possible to write to changed.

+++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
@@ -174,7 +174,7 @@ public function denormalize($data, $class, $format = NULL, array $context = arra
     if (isset($context['request_method']) && $context['request_method'] == 'patch') {
-      $entity->_restPatchFields = array_keys($data);
+      $entity->_restSubmittedFields = array_keys($data);
     }

You need to remove the request_method check then, because a create will be a post, not a patch.

This check used to be necessary when we were messing with the entity object in weird ways. We no longer do, which means we can remove the check, and decide in EntityResource when and how to use it.

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -87,8 +87,8 @@ public function post(EntityInterface $entity = NULL) {
-    foreach ($entity as $field_name => $field) {
-      if (!$field->access('create')) {
+    foreach ($entity->_restSubmittedFields as $field_name => $field) {
+      if (!$field->access('edit')) {

_restSubmittedFields is just a list of field names, which you need to call as $entity->get($field_name)->access()

status requires administer nodes permission I think, see fieldAccess() in NodeAccessControlHandler. Try to remove that or give the user that permission.

Also, this is a security issue, because it means we have no/invalid access handling for creating new content.

Yes this is bad per #16, marking as triaged.

+++ b/core/modules/rest/src/Tests/CreateTest.php
@@ -41,6 +41,16 @@ public function testCreate() {
       $entity = entity_create($entity_type, $entity_values);
+      ¶
+      // These fields can only be added if we have "administer content" permission.

Trailing spaces here.

Yes, I think it would be useful to have a second test as a user with administer nodes permission, to ensure that we can then write those.

And we need a third test, to verify that it is not allowed to write e.g. the node status as a normal user. One field as an example should be enough to verify that the glue code between rest and field access is working.

And of course a test for creating a user :)

The issue summary states

but we should cover the user entity too

How is this patch accomplish that.

+++ b/core/modules/rest/src/Tests/CreateTest.php
@@ -41,6 +41,16 @@ public function testCreate() {
+      ¶

whitespace

1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -87,9 +87,9 @@ public function post(EntityInterface $entity = NULL) {
   +    foreach ($entity->_restSubmittedFields as $key => $field_name) {
   +      if (!$entity->get($field_name)->access('edit')) {
   +        throw new AccessDeniedHttpException(String::format('Access denied on creating field @field', array('@field' => $field_name)));
          }
   

   Let's add a comment here, that explains that we only want to check edit permissions for fields that were actually submitted by the user. Maybe also mention that field access makes no difference between between create and update, so the edit operation is used here.

2. +++ b/core/modules/rest/src/Tests/CreateTest.php
   @@ -28,88 +28,126 @@ class CreateTest extends RESTTestBase {
      public function testCreate() {
        $serializer = $this->container->get('serializer');
   -    $entity_types = array('entity_test', 'node');
   

   This test is getting pretty hard to read, with all those different cases. Maybe we can find a different way to structure it, a separate method per entity type and some helper methods for he parts that are actually the same?

3. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -351,4 +351,21 @@ protected function loadEntityFromLocationHeader($location_url) {
   +  /**
   +   * Remove nod fields where a non-admin user cannot write to.
   +   *
   +   * @param $entity
   +   */
   +  protected function removeNodeFieldsForNonAdminUsers($entity) {
   

   As this is specific to nodes, maybe we should name it $node and type hint on NodeInterface.

   unset() on entity fields is a bit strange, I suggest you use $entity->status = NULL or maybe array() instead, if that works. Maybe check with @yched.

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -87,9 +87,12 @@ public function post(EntityInterface $entity = NULL) {
+    // Only want to check 'edit' permissions for fields that were actually submitted by the user.
+    // Field access makes no difference between 'create' and 'update', so the 'edit' operation is used here.

>80 chars

Will try and take a look at the user tests later in the week

Tagging in the hope someone comes to critical office hours interested in this

Awesome, you do keep finding bugs :)

This is as simple as stupid.

The user entity annotation has:

*   admin_permission = "administer user",

That is now how this permission is called :)

That said, there were also a few things wrong in the test. You checked that access is denied for non-administer users, but then still continued to call createEntityOverRestApi(), which obviously failed :)

Also, createEntityWithoutProperPermissions() did not work for users (because there *are* users), and it is not necessary, so removed that.

The test is certainly already better to understand, still thinking about how to improve it. Not sure yet. Also don't want to delay fixing those critical bugs because of imperfect tests :) That said, comments and coding standards (docblocks, comment lengths and such) certainly need some work.

+++ b/core/modules/rest/src/Tests/CreateTest.php
@@ -276,17 +329,24 @@ public function createEntityNoData($entity_type) {
    * @param $entity
    * @param $entity_type
    * @param array $context
    */
   public function createEntityInvalidSerialized($entity, $entity_type, $context = array()) {

Adding more comments is good, but what I meant is coding standards for e.g. @param, with a type and description, correctly type hint on EntityInterface and so on.

1. Yes, I suggest you use the storage handler if you don't know the entity class (e.g. Node::create() works too, but only if you know that you are working wit nodes).

Seems good to me, Just adding t() for notices.

1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -87,9 +87,13 @@ public function post(EntityInterface $entity = NULL) {
   -        throw new AccessDeniedHttpException(String::format('Access denied on creating field ', array('@field' => $field_name)));
   ...
   +        throw new AccessDeniedHttpException(String::format(t('Access denied on creating field @field'), array('@field' => $field_name)));
   

   -1 to use t() in exceptions IMHO

2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -87,9 +87,13 @@ public function post(EntityInterface $entity = NULL) {
   +    foreach ($entity->_restSubmittedFields as $key => $field_name) {
   
   @@ -97,7 +101,7 @@ public function post(EntityInterface $entity = NULL) {
   -      $this->logger->notice('Created entity %type with ID %id.', array('%type' => $entity->getEntityTypeId(), '%id' => $entity->id()));
   +      $this->logger->notice(t('Created entity %type with ID %id.'), array('%type' => $entity->getEntityTypeId(), '%id' => $entity->id()));
   

   ... We don't translate logging messages

3. +++ b/core/modules/rest/src/Tests/CreateTest.php
   @@ -24,103 +28,361 @@ class CreateTest extends RESTTestBase {
   +    //$this->assertFalse(entity_load_multiple('entity_test', NULL, TRUE), 'No entity has been created in the database.');
   

   Alright, let's get rid of this line :)

4. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -351,4 +353,22 @@ protected function loadEntityFromLocationHeader($location_url) {
   +    unset($node->status);
   +    unset($node->created);
   +    unset($node->changed);
   +    unset($node->promote);
   +    unset($node->sticky);
   +    unset($node->revision_timestamp);
   +    unset($node->uid);
   +
   +    return $node;
   

   It feels like a hack / implementation detail, that this is even possible.

@RavindraSingh Looks like we don't need any translation here.

Its not about needing, its about making things translated which shouldn't, as things are broken, so t() causes even potentially harm.

1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -87,9 +87,13 @@ public function post(EntityInterface $entity = NULL) {
   +    // submitted by the user.Field access makes no difference between 'create'
   

   missing space after user.

2. +++ b/core/modules/rest/src/Tests/CreateTest.php
   @@ -24,103 +28,358 @@ class CreateTest extends RESTTestBase {
   +      $this->createEntityOverRestApi($entity_type, $serialized);
   

   Might want to rename or change the accepted parameters. It's not clear why this isn't user later where it's using httpRequest(). Maybe something like:

   
   public function assertCreateEntityResponse($entity_type, $serialized = NULL, $response = 201) {
   
   

3. +++ b/core/modules/rest/src/Tests/CreateTest.php
   @@ -24,103 +28,358 @@ class CreateTest extends RESTTestBase {
   +      $this->readEntityIdFromHeaderAndDb($entity_type, $entity, $entity_values);
   

   Might want to rename this make it clear that this method deletes the entity.

4. +++ b/core/modules/rest/src/Tests/CreateTest.php
   @@ -24,103 +28,358 @@ class CreateTest extends RESTTestBase {
   +      // Try to send invalid data that cannot be correctly deserialized.
   +      $this->createEntityInvalidData($entity_type);
   +
   +      // Try to send no data at all, which does not make sense on POST requests.
   +      $this->createEntityNoData($entity_type);
   +
   +      // Try to send invalid data to trigger the entity validation constraints. Send a UUID that is too long.
   +      $this->createEntityInvalidSerialized($entity, $entity_type);
   +
   +      // Try to create an entity without proper permissions.
   +      $this->createEntityWithoutProperPermissions($entity_type, $serialized, ['account' => $account]);
   +
   

   Not sure if this is too pedantic, but I would expect any helper method that has assertions in it to start with assert, i.e. assertCreateEntityInvalidData(). Is there a core convention for this?

Looks good to me.

git ac https://www.drupal.org/files/issues/2405091-44.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 26548  100 26548    0     0   158k      0 --:--:-- --:--:-- --:--:--  227k
error: patch failed: core/modules/rest/src/Tests/RESTTestBase.php:67
error: core/modules/rest/src/Tests/RESTTestBase.php: patch does not apply

Overall looks good. Found some nitpicks.

1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -87,9 +87,13 @@ public function post(EntityInterface $entity = NULL) {
   +    // Only want to check 'edit' permissions for fields that were actually
   

   Can skip 'want to'.

2. +++ b/core/modules/rest/src/Tests/CreateTest.php
   @@ -24,103 +28,358 @@ class CreateTest extends RESTTestBase {
   +
   +    // @todo Remove the user reference field for now until deserialization for
   +    // entity references is implemented.
   +    unset($entity_values['user_id']);
   +
   

   Is there an issue for this?

3. +++ b/core/modules/rest/src/Tests/CreateTest.php
   @@ -24,103 +28,358 @@ class CreateTest extends RESTTestBase {
   +  /**
   +   * Try to send invalid data to trigger the entity validation constraints.
   +   * Send a UUID that is too long.
   +   *
   

   Should be one line.

4. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -351,4 +353,22 @@ protected function loadEntityFromLocationHeader($location_url) {
   +   * Remove node fields where a non-admin user cannot write to.
   

   s/where/that/

   Or I'd reword to 'Remove node fields that can only be written by an admin user'.

2. That comment is just moved in the patch. However, it looks weird to me, we supported entity reference fields for a very, very long time? maybe we can just remove that unset() ?

1. +++ b/core/modules/rest/src/Tests/CreateTest.php
   @@ -341,8 +337,7 @@ public function assertCreateEntityNoData($entity_type) {
      /**
   -   * Try to send invalid data to trigger the entity validation constraints.
   -   * Send a UUID that is too long.
   +   * Try to send invalid data to trigger the entity validation constraints. Send a UUID that is too long.
   

   Neither version is correct.

   The first sentence must not be more than 80 characters.

   Two ways to fix it:

   1. Keep the old version, but add an empty line inbetween.
   2. Rewrite into a single, shorter sentence. I'd prefer that. Something like "Send an invalid UUID to trigger the entity validation." ?

2. +++ b/core/modules/rest/src/Tests/CreateTest.php
   @@ -341,8 +337,7 @@ public function assertCreateEntityNoData($entity_type) {
       * @param EntityInterface $entity
       *   The entity we want to check that was inserted correctly.
   

   classes/interfaces in @param/@return should always use the full namespace, so \Drupal\Core\Entity\EntityInterface.

3. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -348,7 +348,7 @@ protected function loadEntityFromLocationHeader($location_url) {
       * @param NodeInterface $node
       * @return NodeInterface
   

   Also namespace missing and missing description for @param and possibly @return (not visible from context)

+++ b/core/modules/rest/src/Tests/RESTTestBase.php
@@ -69,10 +69,13 @@ protected function setUp() {
+   *
+   * @return boolean
+   *   The content returned from the call to curl_exec().

Not sure if we need to mention curl_exec() here, I'd just say "... from the request." ? Also, this should be a string, not a boolean? (which would be "bool").

I agree that the unset business is spooky. I think it would be better to set them to NULL/empty array, that should have the same effect. But that unset stuff seems to be an existing thing in rest test, so probably better to clean it up in a separate issue, this is big enough and solves the critical part.

Thanks, back to RTBC.

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

I'd started reviewing this but only found comments over 80 chars. Opened #2418559: Fix comment formatting in /core/modules/rest/src/Tests/CreateTest.php as very minor follow-up.