Problem/Motivation

Since SA-2017-002 it is no longer possible to manually set the ID for newly created entities.

Proposed resolution

Allow the ID to be set for newly created entities with string IDs.

Entities that do not use string IDs have further problems to overcome, see #31 and they will be handled in a different issue.

Remaining tasks

Review.

User interface changes

Nope.

API changes

Nope.

Data model changes

It is now possible to set the ID for newly created entities with string IDs.

Original issue summary

Problem/Motivation

I have custom entities in my project. With Drupal 8.2.6 I was able to make a POST through REST API. With Drupal 8.3.2 & 8.3.3 I have a problem with the field ID.

I need to be able to add the ID by myself. But the field ID should be optional in my REST call. That was the possible with Drupal 8.2.6.

In Drupal 8.3.2 & 8.3.3 I can do a POST only if the body doesn't provide field ID inside.

Is there any changes regarding user access and field ID since version 8.2.6?

Examples

This call is working only in Drupal 8.2.6

{
  "id": [{"value":3}],
  "name":[{"value":"Test name"}]
}

This call is working for all versions

{
  "name":[{"value":"Test name"}]
}
CommentFileSizeAuthor
#103 2885469-103.patch23.65 KBWim Leers
#103 interdiff.txt3.16 KBWim Leers
#101 interdiff-101.txt53.16 KBamateescu
#101 2885469-101.patch23.05 KBamateescu
#94 2885469-94.patch70.26 KBamateescu
#93 2885469-93.patch74.6 KBWim Leers
#93 interdiff.txt15.63 KBWim Leers
#86 2885469-86.patch71.62 KBWim Leers
#86 interdiff.txt1.36 KBWim Leers
#84 2885469-84.patch71.62 KBWim Leers
#84 interdiff.txt1.15 KBWim Leers
#83 interdiff.txt3.69 KBWim Leers
#81 2885469-81.patch71.54 KBWim Leers
#81 interdiff.txt417 bytesWim Leers
#79 2885469-79.patch71.26 KBWim Leers
#79 interdiff.txt743 bytesWim Leers
#77 2885469-77.patch71.03 KBWim Leers
#77 interdiff.txt903 bytesWim Leers
#74 2885469-73.patch70.89 KBWim Leers
#74 interdiff.txt9.35 KBWim Leers
#72 2885469-72.patch68.55 KBWim Leers
#72 interdiff.txt51.19 KBWim Leers
#70 2885469-70.patch20.37 KBWim Leers
#70 interdiff.txt6.73 KBWim Leers
#66 2885469-66.patch17.17 KBtimmillwood
#66 interdiff-2885469-66.txt1.86 KBtimmillwood
#64 2885469-64.patch17.06 KBtimmillwood
#64 interdiff-2885469-64.txt4.84 KBtimmillwood
#59 2885469-59.patch15.28 KBtimmillwood
#59 interdiff-2885469-59.txt5.13 KBtimmillwood
#57 2885469-57.patch15.58 KBtimmillwood
#57 interdiff-2885469-57.txt631 bytestimmillwood
#55 2885469-55.patch15.65 KBtimmillwood
#55 2885469-55-sa-tests.patch12.14 KBtimmillwood
#54 sa-tests.patch12.59 KBxjm
#37 2885469-37.patch5.58 KBalexpott
#37 35-37-interdiff.txt934 bytesalexpott
#35 2885469-35.patch5.59 KBalexpott
#35 34-35-interdiff.txt3.44 KBalexpott
#34 interdiff-34.txt3.68 KBamateescu
#34 2885469-34.patch4.55 KBamateescu
#29 interdiff.txt928 bytesamateescu
#29 2885469-29.patch5.94 KBamateescu
#25 interdiff.txt2.33 KBamateescu
#25 2885469-25.patch5.87 KBamateescu
#25 2885469-25-test-only.patch2.33 KBamateescu
#12 2885469-12.patch3.54 KBtstoeckler
#12 2885469-8-12--interdiff.txt1.81 KBtstoeckler
#8 2885469-8.patch1.73 KBtstoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

remram created an issue. See original summary.

remram’s picture

With version 8.3.3 I'm getting the same message:
403 Forbidden

{
    "message": "Access denied on creating field 'id'."
}
remram’s picture

Version: 8.3.2 » 8.3.3
remram’s picture

Issue summary: View changes
remram’s picture

This check in class EntityAccessControlHandler and method fieldAccess is causing the problem:

// Explicitly disallow changing the entity ID and entity UUID.
    if ($operation === 'edit') {
      if ($field_definition->getName() === $this->entityType->getKey('id')) {
        return $return_as_object ? AccessResult::forbidden('The entity ID cannot be changed') : FALSE;
      }
      elseif ($field_definition->getName() === $this->entityType->getKey('uuid')) {
        // UUIDs can be set when creating an entity.
        if ($items && ($entity = $items->getEntity()) && !$entity->isNew()) {
          return $return_as_object ? AccessResult::forbidden('The entity UUID cannot be changed')->addCacheableDependency($entity) : FALSE;
        }
      }
    }

I also found the security advisor ticket of this change:
Drupal Core - Critical - Access Bypass - SA-CORE-2017-002

Here is the commit on GitHub: SA-CORE-2017-002 by alexpott, xjm, larowlan, Wim Leers, samuel.morten

Is there any solution to my use case?

Wim Leers’s picture

Status: Active » Postponed (maintainer needs more info)

You're right, this was a security hardening.

I want to make sure I fully understand your use case though. Why do you want to specify the numeric ID? This is normally auto-generated by the database; why do you want to be able to specify it when creating entities of your custom content entity type?

Wim Leers’s picture

Issue tags: +Entity Field API
tstoeckler’s picture

Status: Postponed (maintainer needs more info) » Needs review
FileSize
1.73 KB

Explicitly setting the ID during creation is an explicit feature of the Entity API and is why we converted everything that in D7 did !empty($node->nid) to $entity->isNew(). For entities with integer IDs the use-case is a bit harder to justify, although it can be needed when doing migrations. But we also support (content) entities with string IDs and there it's quite necessary to set the ID explicitly.

Luckily the code that is responsible already allows the UUID to be set initially, so I just moved this code around to include the ID as well.

Wim Leers’s picture

Title: Access denied on creating field 'id' » Regression: Access denied on creating field 'id'
Version: 8.3.3 » 8.3.x-dev
Status: Needs review » Reviewed & tested by the community
Issue tags: +Regression

Oh, nice!

Thanks, @tstoeckler!

Wim Leers’s picture

Note that I also referenced this issue on the (private) security.drupal.org issue tracker. I think that's how @tstoeckler found this so quickly/suddenly :)

I didn't know whether this was an intentional feature, but @tstoeckler is an Entity API maintainer and he says it is, therefore I felt it was okay to RTBC #8 :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 2885469-8.patch, failed testing. View results

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
1.81 KB
3.54 KB

This should be green.

remram’s picture

Hi Guys and thank you for the response :)

Well, I have the current case: We need to import existing data from the old system to the Drupal project. The requirement, that I have, I have to import the old IDs from the old system. New records should take the next ID by using the default functionality from the database (auto increment).

remram’s picture

Thank you guys. REST is working pretty nice with this patch :) great job

Wim Leers’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/tests/src/Unit/UserAccessControlHandlerTest.php
@@ -75,8 +76,12 @@ protected function setUp() {
+      ->method('isNew')
+      ->will($this->returnValue(FALSE));

This is now only testing the "update" case. Should this not also be testing the "create" case? Otherwise we may still regress.

Wim Leers’s picture

Issue tags: +Needs tests
Wim Leers’s picture

Assigned: Unassigned » tstoeckler

Pinged @tstoeckler via IRC.

tstoeckler’s picture

Assigned: tstoeckler » Unassigned

Yes, #15 is correct, nicely spotted. UserAccessControlHandlerTest is a bit weird, though. And I'm not sure when I will get to it. So unassigning for now in the hopes that someone else may pick this up.

tstoeckler’s picture

Oh, FWIW, I haven't been on IRC for ages and I don't think that's changing soon...

Wim Leers’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
index 216e8d6..2da8e43 100644
--- a/core/modules/user/tests/src/Unit/UserAccessControlHandlerTest.php

And actually, changes in EntityAccessControlHandler should not have test coverage in UserAccessControlhandlerTest. We have \Drupal\KernelTests\Core\Entity\EntityAccessControlHandlerTest for that. Perhaps we want to add a unit test for that though.

Wim Leers’s picture

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Issue tags: +API-First Initiative
amateescu’s picture

Title: Regression: Access denied on creating field 'id' » Regression: manually setting the ID field for newly-created content entities is not possible anymore
Component: rest.module » entity system
Assigned: tstoeckler » Unassigned
Priority: Normal » Major
Status: Needs work » Needs review
Issue tags: -BC break, -Needs tests +Workflow Initiative
FileSize
2.33 KB
5.87 KB
2.33 KB

I also stumbled upon this regression while working on the Workspaces module, which will be the first entity type with a string ID provided by core. I had a similar fix in the (related) patch from #2685749-10: Add a 'machine_name' widget for string field types with a UniqueField constraint, but I'm glad I searched before opening a new issue because @tstoeckler's code fix looks more clear than what I did.

Here's some proper test coverage for this regression.

The last submitted patch, 25: 2885469-25-test-only.patch, failed testing. View results

tstoeckler’s picture

Thanks for the test, it looks great! One minor question:

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityAccessControlHandlerTest.php
@@ -293,4 +294,45 @@ public function testHooks() {
+    $entity->save();
+    $this->assertSame($uuid, $entity->uuid());

Any reason you are not checking for the ID?

Wim Leers’s picture

@amateescu++

I wrote this in #20:

And actually, changes in EntityAccessControlHandler should not have test coverage in UserAccessControlhandlerTest. We have \Drupal\KernelTests\Core\Entity\EntityAccessControlHandlerTest for that. Perhaps we want to add a unit test for that though.

@amateescu did that. Does that mean we want to revert the changes made in UserAccessControlhandlerTest?

amateescu’s picture

Any reason you are not checking for the ID?

No good reason, but you're right, we should try to set and check the ID explicitly as well.

Does that mean we want to revert the changes made in UserAccessControlhandlerTest?

Nope, those changes are needed in order to make the test pass, see the failures from #8.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

I think this is ready now :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

This does kind of open an interesting scenario. If you can set integer IDs yourself you can easily ensure that Drupal can no longer create any entities of that type (if it is a serial field) by setting the ID to 4294967295.

I think we should only allow IDs via REST to be set when they are not auto-increment.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

@alexpott, that distinction seems a bit arbitrary to me and it would break at least the use case for which this issue was opened, see a detailed explanation of it in #13.

Also, what you describe was already possible before https://www.drupal.org/forum/newsletters/security-advisories-for-drupal-... , so we should maybe open a new issue to discuss it?

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

@amateescu well the migrate system existing for #13. I don't think it is beyond the realm of possibility for a site to allow anonymous comments and have REST enabled for commenting. This change going in as is makes me uneasy because of the fragility it introduces.

I'm not sure what a solution looks like but I think just opening up for stringy IDs to start with is probably a better way to go.

amateescu’s picture

alexpott’s picture

Adding more test coverage since the whole serial vs string thing was my point.

@amateescu thanks for implementing - the tests prove it works nicely.

Status: Needs review » Needs work

The last submitted patch, 35: 2885469-35.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
934 bytes
5.58 KB

Oops

dawehner’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
@@ -316,13 +316,17 @@ public function fieldAccess($operation, FieldDefinitionInterface $field_definiti
+        if (!($entity->isNew() && $field_definition->getType() === 'string')) {

I'm wondering whether this should be rather a check for config entities given they have a different semantic than content entities.

Wim Leers’s picture

#38++

amateescu’s picture

#38/#39: not sure what you mean, can you elaborate a bit?

Wim Leers’s picture

@amateescu: rather than reading the type of the id field (string or int), check if $entity instanceof ConfigEntityInterface.

amateescu’s picture

Then I'm not sure what that comment has to do with this issue. This is about a string ID field for content entities, like the title says...

Wim Leers’s picture

Hm, you're right. I think @dawehner and I both jumped to say this because the only entity types with string IDs in core are config entities. So I think you're right, that the patch in #37 is ready as-is.

Pinged @dawehner on IRC.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I'm sorry, this makes more sense now. Thank you for the explanation!

xjm’s picture

Title: Regression: manually setting the ID field for newly-created content entities is not possible anymore » Regression: manually setting the ID field for newly-created content entEities is not possible anymore (public followup to SA-2017-002)
Assigned: Unassigned » xjm
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs followup

This seems a minimal and fairly reasonable fix. #38 throusg #44, might make sense in a separate issue?

Since this is also a target bugfixes that targets only the regressed cases, this probably makes sense. However, since this issue is a followup to behavior deliberately added in SA-CORE-2017-002, I'd like to review and test a bit further.

xjm’s picture

Issue tags: +Security
Wim Leers’s picture

18:37:00 <timmillwood> WimLeers: thanks, another issue I had was it checks the 'edit' permission on POST, which prevents setting the 'id' field. Workspaces have a string id set by the user.
18:45:35 <drupalbot> WimLeers: I'll pass that on when chrfritsch is around.
18:46:05 <WimLeers> timmillwood: hm, field 'edit' access on the 'id' field, right?
18:46:14 <WimLeers> timmillwood: we have an issue for that, hold on
18:48:45 <WimLeers> timmillwood: ok was harder to find than expected, but here you go: https://www.drupal.org/project/drupal/issues/2885469
Wim Leers’s picture

Issue tags: -Needs followup

#38 through #44 does not need a follow-up, see #42 + #43 + #44.

Wim Leers’s picture

Issue tags: +blocker
18:51:51 <timmillwood> I'll see if I can help move that forward tomorrow, because if we need entity resource tests for workspaces before they get it I core, this is a major blocker.
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

In fact, I think that it was only un-RTBC'd for needing a follow-up, so back to RTBC now.

timmillwood’s picture

+1 for RTBC.

timmillwood’s picture

larowlan’s picture

Pinged @xjm to clarify if

However, since this issue is a followup to behavior deliberately added in SA-CORE-2017-002, I'd like to review and test a bit further.

still applies

xjm’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
12.59 KB

So we were holding back the test for this fix to avoid disclosing the specific exploits. Enough time has elapsed though that we can release the tests now.

They need a reroll but I don't think we should commit this without the coverage in the tests.

timmillwood’s picture

Version: 8.4.x-dev » 8.5.x-dev
Status: Needs work » Needs review
Issue tags: +Contributed project blocker
FileSize
12.14 KB
15.65 KB

Here is a reroll off the sa-test.patch from #54, and also combined with the patch from #37. There were some conflicts between the two patches, so hopefully I resolved them successfully.

Status: Needs review » Needs work

The last submitted patch, 55: 2885469-55.patch, failed testing. View results

timmillwood’s picture

Not really sure what's going on with the EntityResourceTest breaks, but this patch fixes the EntityAccessControlHandlerTest failures.

Status: Needs review » Needs work

The last submitted patch, 57: 2885469-57.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
5.13 KB
15.28 KB

Let's try to fix some of these failing tests.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
@@ -316,13 +316,17 @@ public function fieldAccess($operation, FieldDefinitionInterface $field_definiti
       if ($field_definition->getName() === $this->entityType->getKey('id')) {
-        return $return_as_object ? AccessResult::forbidden('The entity ID cannot be changed') : FALSE;
+        // String IDs can be set when creating the entity.
+        if (!($entity->isNew() && $field_definition->getType() === 'string')) {
+          return $return_as_object ? AccessResult::forbidden('The entity ID cannot be changed')->addCacheableDependency($entity) : FALSE;
+        }
       }

IMHO, for new entities, the ID can always be set unless some custom access denies that. We only care about existing entities, where again IMHO, the ID can never be changed, that is not supported by the storage.

The initial example in OP uses an integer, not a string and there are valid reasons for supporting that.

amateescu’s picture

@Berdir, try to convince @alexpott of that, see #31 - #34.

Berdir’s picture

I see, missed that argument. I can see how that could be a problem and hard to protect against that.

Maybe there is a way to make it easier to override if you have that use case, without having to duplicate the whole method? Instead of returning a forbidden early, we could change the default from an allowed to a neutral in that case, so someone would need to explicitly allow access in checkFieldAccess() or a hook?

Or the implementation of it could be in checkFieldAccess(), then you can decide to not call that, but then the protection relies on all implementations calling the parent.

Status: Needs review » Needs work

The last submitted patch, 59: 2885469-59.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
4.84 KB
17.06 KB

Fixing some more test (and coding standards)

Status: Needs review » Needs work

The last submitted patch, 64: 2885469-64.patch, failed testing. View results

timmillwood’s picture

Status: Needs work » Needs review
FileSize
1.86 KB
17.17 KB

User entities don't like random strings for the label field, so lets use random machine names.
Message entities don't have storage, so are allowed duplicate UUIDs, so lets not test that.

Wim Leers’s picture

#54: thanks for providing that patch, @xjm!

Note that @alexpott wrote the test coverage in #54, so he should be credited. I reviewed it, as did @tstoeckler and @samuel.mortenson, so all of them should also be credited.

larowlan’s picture

Added those credits

Wim Leers’s picture

FileSize
6.73 KB
20.37 KB

Reviewed, but only the changes made to the security test coverage from #54, i.e. I reviewed how it was merged, in extreme detail.

  1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -916,6 +939,38 @@ public function testPost() {
    +    if ($this->entity->getEntityType()->getStorageClass() != 'Drupal\Core\Entity\ContentEntityNullStorage' &&  $this->entity->getEntityType()->hasKey('uuid')) {
    

    Let's use ::class.

    Also: two spaces, should be one.

    Also: strict inequality.

  2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -916,6 +939,38 @@ public function testPost() {
    +      $created_entity = $this->createAnotherEntity();
    

    We shouldn't need this. #59 made this change, where it also reverted a few other changes (the lines starting with $created_entity =… and $location = …).

    The reason you ended up needing to resolve the conflict this way is because this new test coverage was being added below the BC test coverage that #2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available added in May 2017, but the security test coverage in #54 was created in April.

    If we simply move the security test coverage to run before the BC test coverage (which is a best practice anyway), then the problem disappears :)

    The only thing we then need to do is to delete the newly created entity, so that the BC layer test coverage can still run.

    (I realize the createAnotherEntity() call was also in the patch in #54, but in a slightly different place. That itself was wrong too, it was introduced in an incorrect reroll in https://security.drupal.org/node/161938, comment 132, which I realize most of you can't see.)

  3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -916,6 +939,38 @@ public function testPost() {
    +      if (isset($normalized_entity['url'])) {
    +        $normalized_entity['url'] = [['value' => 'http://example.com/feed1']];
    +      }
    ...
    +      if (isset($normalized_entity['url'])) {
    +        $normalized_entity['url'] = [['value' => 'http://example.com/feed1']];
    +      }
    

    #64 added this, but I'd really like to avoid it.

    I understand this is due to class FeedUrlConstraint extends UniqueFieldConstraint {…}.

    We can fix this in a generic way by adding a new optional parameter to \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::getNormalizedPostEntity().

    (In doing so, I noticed that FeedResourceTestBase extends EntityTestResourceTestBase instead of EntityResourceTestBase. I had to fix that too due to the addition of this new optional parameter.)

Addressed all my remarks.

Status: Needs review » Needs work

The last submitted patch, 70: 2885469-70.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
51.19 KB
68.55 KB
+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -253,9 +253,15 @@ protected function createAnotherEntity() {
-  abstract protected function getNormalizedPostEntity();
+  abstract protected function getNormalizedPostEntity($which = NULL);

Ugh, adding new optional parameters to base classes requires all subclasses to also be updated. I didn't realize that. That's annoying…

The last submitted patch, 70: 2885469-70.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

FileSize
9.35 KB
70.89 KB

Reviewed everything:

  1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -906,6 +936,32 @@ public function testPost() {
         $this->assertFalse($response->hasHeader('X-Drupal-Cache'));
     
    +
    +    if ($this->entity->getEntityType()->getStorageClass() !== ContentEntityNullStorage::class && $this->entity->getEntityType()->hasKey('uuid')) {
    

    One \n too many. I introduced this in my previous patch, sorry ☺️

  2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -906,6 +936,32 @@ public function testPost() {
    +      $new_entity = \Drupal::service('entity.repository')->loadEntityByUuid($created_entity->getEntityType()->id(), $new_uuid);
    

    This should use $this->entityStorage.

  3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -1283,6 +1354,24 @@ protected function makeNormalizationInvalid(array $normalization) {
    +  protected function makePatchInsecure(array $normalization, $key) {
    

    This is very similar to makeNormalizationInvalid(). It's better to make that existing method a bit more powerful than adding yet another method.

    (This method also has a fairly strange name, so refactoring it away would solve that too.)

  4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Feed/FeedResourceTestBase.php
    @@ -3,10 +3,10 @@
    -use Drupal\Tests\rest\Functional\EntityResource\EntityTest\EntityTestResourceTestBase;
    +use Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase;
     use Drupal\aggregator\Entity\Feed;
     
    -abstract class FeedResourceTestBase extends EntityTestResourceTestBase {
    +abstract class FeedResourceTestBase extends EntityResourceTestBase {
    

    This could be done in a separate issue, but that's probably not worth the overhead.

  5. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Item/ItemResourceTestBase.php
    @@ -78,6 +78,17 @@ protected function createEntity() {
    +  protected function createAnotherEntity() {
    

    Missing inheritdoc.

  6. +++ b/core/modules/rest/tests/src/Functional/EntityResource/User/UserResourceTestBase.php
    @@ -244,6 +255,49 @@ protected function assertRpcLogin($username, $password) {
    +    // The anonymous user is never allowed to modify itself.
    

    This comment is c/p'ed from \Drupal\Tests\rest\Functional\EntityResource\User\UserResourceTestBase::testPatchDxForSecuritySensitiveBaseFields(), but here we're not trying to modify ourselves, we're trying to modify another user.

  7. +++ b/core/modules/rest/tests/src/Functional/EntityResource/User/UserResourceTestBase.php
    @@ -244,6 +255,49 @@ protected function assertRpcLogin($username, $password) {
    +    if (!static::$auth) {
    +      $this->markTestSkipped();
    +    }
    ...
    +    $user = static::$auth ? $this->account : User::load(0);
    

    This doesn't run for anonymous users, yet that last line does take that into account. That last line can be simplified.

  8. +++ b/core/modules/rest/tests/src/Functional/EntityResource/User/UserResourceTestBase.php
    @@ -244,6 +255,49 @@ protected function assertRpcLogin($username, $password) {
    +    $user1 = [
    +        'mail' => [['value' => 'another_email_address@example.com']],
    +        'uid' => [['value' => 1]],
    +        'name' => [['value' => 'another_user_name']],
    +        'pass' => [['existing' => $this->account->passRaw]],
    +        'uuid' => [['value' => '2e9403a4-d8af-4096-a116-624710140be0']],
    +      ] + $original_normalization;
    

    Indentation is off.

Addressed all my remarks.

The last submitted patch, 72: 2885469-72.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 74: 2885469-73.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
903 bytes
71.03 KB

One small oversight.

Status: Needs review » Needs work

The last submitted patch, 77: 2885469-77.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
743 bytes
71.26 KB
Berdir’s picture

  1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -738,7 +767,7 @@ public function testPost() {
         $unparseable_request_body = '!{>}<';
         $parseable_valid_request_body   = $this->serializer->encode($this->getNormalizedPostEntity(), static::$format);
         $parseable_valid_request_body_2 = $this->serializer->encode($this->getNormalizedPostEntity(), static::$format);
    -    $parseable_invalid_request_body   = $this->serializer->encode($this->makeNormalizationInvalid($this->getNormalizedPostEntity()), static::$format);
    +    $parseable_invalid_request_body   = $this->serializer->encode($this->makeNormalizationInvalid($this->getNormalizedPostEntity(), 'label'), static::$format);
         $parseable_invalid_request_body_2 = $this->serializer->encode($this->getNormalizedPostEntity() + ['uuid' => [$this->randomMachineName(129)]], static::$format);
         $parseable_invalid_request_body_3 = $this->serializer->encode($this->getNormalizedPostEntity() + ['field_rest_test' => [['value' => $this->randomString()]]], static::$format);
    

    we don't do alignment of = and it's different alignments anyway, should we remove the double space on the line we're touching

  2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -906,6 +936,32 @@ public function testPost() {
    +    if ($this->entity->getEntityType()->getStorageClass() !== ContentEntityNullStorage::class && $this->entity->getEntityType()->hasKey('uuid')) {
    +      // 500 when creating an entity with a duplicate UUID.
    +      $normalized_entity = $this->getNormalizedPostEntity(2);
    +      $normalized_entity[$created_entity->getEntityType()->getKey('uuid')] = [['value' => $created_entity->uuid()]];
    +      $normalized_entity[$label_field] = [['value' => $this->randomMachineName()]];
    +      $request_options[RequestOptions::BODY] = $this->serializer->encode($normalized_entity, static::$format);
    +
    +      $response = $this->request('POST', $url, $request_options);
    +      $this->assertSame(500, $response->getStatusCode());
    +      $this->assertContains('Internal Server Error', (string) $response->getBody());
    

    We're basically adding test coverage for a bug here, which is IMHO a bit weird, as it's not directly tied to this issue, is it?

    Don't we have an issue to add uuid validation to make this a validation error instead of an exception.

  3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -906,6 +936,32 @@ public function testPost() {
    +      $response = $this->request('POST', $url, $request_options);
    +      $this->assertResourceResponse(201, FALSE, $response);
    +      $entities = $this->entityStorage->loadByProperties([$created_entity->getEntityType()->getKey('uuid') => $new_uuid]);
    +      $new_entity = reset($entities);
    +      $this->assertNotNull($new_entity);
    +      $new_entity->delete();
    

    why not use loadEntityByUuid()?

  4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -1274,11 +1345,19 @@ protected function getEntityResourcePostUrl() {
    +      case 'id':
    +      case 'uuid':
    +        $key_field = $this->entity->getEntityType()->getKey($key);
    +        $normalization[$key_field][0]['value'] = $this->anotherEntity->$key();
    +        break;
    

    the $key variable is named fairly strange and not documented. Took me a while, but I guess it refers to an *entity key*, then lets name it $entity_key. I also don't really like "$key()", it's pretty random that we happen to have the same name for the keys and the method names, maybe go with $normalization[$key_field] = $this->anotherEntity->get($key_field)->toValue().

  5. +++ b/core/modules/rest/tests/src/Functional/EntityResource/User/UserResourceTestBase.php
    @@ -244,6 +255,49 @@ protected function assertRpcLogin($username, $password) {
    +    // Ensure the email address has not changed.
    +    $this->assertEquals('admin@example.com', User::load(1)->getEmail());
    +    $this->assertResourceErrorResponse(403, "Access denied on updating field 'uid'.", $response);
    

    if the user was already loaded on this page then it might be in the static cache, to be extra sure, it makes sense to call resetCache() first.

  6. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityAccessControlHandlerTest.php
    @@ -293,4 +296,73 @@ public function testHooks() {
    +
    +  public function providerTestFieldAccess() {
    +    return [
    

    not sure what our rule on documenting provider methods is.

Wim Leers’s picture

FileSize
417 bytes
71.54 KB

Thanks for the review!

  1. This is already in HEAD… but sure, done.
  2. This entire issue is about fixing a regression introduced by a security bugfix. Core committer @xjm un-RTBC'd this in #54, and said that we should include the test coverage that was written for that security issue in this patch. To ensure we don't regress on the security front while fixing the regression reporting in this issue. That's why this (security vulnerability) test coverage is being added here.
  3. Because loadEntityByUuid() doesn't exist on \Drupal\Core\Entity\EntityStorageInterface. It only exists on the separate entity.repository service.
  4. I didn't write this — I think it was named like that because this can be either of those two "keying" fields: "id" and "uuid". I agree this attempt to save a few LoC is adding more confusion than it is preventing, so I just converted this to explicit code :)
  5. Good call — used loadUnchanged() instead.
  6. Discussed this in another issue recently with Alex Pott, he agreed that it makes no sense to add boilerplate docs for them. So should be fine like this.

Status: Needs review » Needs work

The last submitted patch, 81: 2885469-81.patch, failed testing. View results

Wim Leers’s picture

FileSize
3.69 KB

#81's interdiff is obviously the wrong one. Here's the right one.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.15 KB
71.62 KB

The problem was indeed in #81's interdiff (now posted at #83). Fixed.

Berdir’s picture

2. Right, I remember. Should we have a follow-up then to use the Unique validation constraint on uuid fields and reference that so we can eventually replace that assert-that-php-died-with-a-500-exception with a validation error?
3. I know, but why not use that service then?
4. You renamed it to $field_name now, but that's not correctl. 'label' is not a field name. It is an $entity_key, then we use the entity key info to get the actual field name.

Wim Leers’s picture

FileSize
1.36 KB
71.62 KB
  1. Yes please! 👌
  2. Why add a new service when the existing service that we have available works just fine? It's conceptually clear that \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase only has one service injected for "entity" REST tests: \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::$entityStorage. Why add more?
  3. Oops! 👍 Fixed.
Wim Leers’s picture

Title: Regression: manually setting the ID field for newly-created content entEities is not possible anymore (public followup to SA-2017-002) » Regression: manually setting the ID field for newly-created content entities is not possible anymore (public follow-up to SA-2017-002)

Title nits.

Berdir’s picture

2. #2932009: Add a Unique constraint to uuid fields

3. Well, the point of entity.repository/that method is to simplify the code when loading an entity by uuid, it would just look like this:

$new_entity = \Drupal::service('entity.repository')->loadEntityByUuid($created_entity->getEntityTypeId(), $new_uuid);

Also, technically, $this->entityStorage isn't "injected", just indirectly accessed through a property that is set in the constructor. We can't actually inject anything in test classes, nor do I think it is necessary (we don't test tests). But different people have very different opinions (and strong ones) on that stuff: #2066993: Use \Drupal consistently in tests

This still leaves us with #60/62 IMHO, at the very least we need to update the issue description (because the use case explained there is *not* fixed with this patch) and have a change record that explains what exactly we allow now and provide hints on how to change that if really desired by a custom entity type, including mentioning the possible problems with it? Or maybe explore my ideas in #62, not sure.

Wim Leers’s picture

Assigned: Unassigned » alexpott
  1. Excellent! Thank you! I posted an observation and a patch.
  2. This still leaves us with #60/62 IMHO, at the very least we need to update the issue description (because the use case explained there is *not* fixed with this patch) and have a change record that explains what exactly we allow now and provide hints on how to change that if really desired by a custom entity type, including mentioning the possible problems with it? Or maybe explore my ideas in #62, not sure.

    I don't have an informed opinion about that … assigning to @alexpott for that, per #61.

alexpott’s picture

Assigned: alexpott » Unassigned

If the field is an integer auto-increment we shouldn't allow rest to set the ID. As explained in #31 this opens a really simple way to break a site. REST needs to treat all incoming data as untrusted.

Berdir’s picture

Status: Needs review » Needs work

@alexpott: "If the field is an integer auto-increment" is interesting. We always make int fields auto-increment in \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::processIdentifierSchema() which for example user overwrites in \Drupal\user\UserStorageSchema::processIdentifierSchema() (Although user is interesting because it would still break)

But I'm fine to look into that in another issue and add an explicit way of opting-out of auto-increment that we could also check in regards to access then.

But setting to needs work then for a change record and issue summary updates to document what exactly we allow.

alexpott’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
    @@ -316,13 +316,17 @@ public function fieldAccess($operation, FieldDefinitionInterface $field_definiti
    +        // String IDs can be set when creating the entity.
    +        if (!($entity->isNew() && $field_definition->getType() === 'string')) {
    +          return $return_as_object ? AccessResult::forbidden('The entity ID cannot be changed')->addCacheableDependency($entity) : FALSE;
    +        }
    

    So looking here we are still denying access on create for integer IDs as far as I can see - regardless of auto-create so my security concerns are solved whilst this restriction is in place. Nice.

  2. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonTestBase.php
    @@ -117,7 +117,7 @@ protected function getExpectedNormalizedEntity() {
         return parent::getNormalizedPostEntity() + [
    
    +++ b/core/modules/hal/tests/src/Functional/EntityResource/EntityTest/EntityTestHalJsonAnonTest.php
    @@ -78,7 +78,7 @@ protected function getExpectedNormalizedEntity() {
         return parent::getNormalizedPostEntity() + [
    
    +++ b/core/modules/hal/tests/src/Functional/EntityResource/EntityTest/EntityTestHalJsonInternalPropertyNormalizerTest.php
    @@ -73,7 +73,7 @@ protected function createEntity() {
         return parent::getNormalizedPostEntity() + [
    

    Should we be passing on $which? (There are quite a few instances when we don't).

  3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -230,9 +253,15 @@ public function setUp() {
    +   * @param int|null $which
    +   *   An integer indicating which POST entity to generate. (Some entity types
    +   *   have a required field that must contain a unique value. To test certain
    +   *   edge cases, multiple distinct entities need to be created. This integer
    +   *   then indicates which set of entity field values to create.)
    +   *
        * @return array
        */
    -  abstract protected function getNormalizedPostEntity();
    +  abstract protected function getNormalizedPostEntity($which = NULL);
    

    $which has to be one of the oddest variable names around.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
15.63 KB
74.6 KB

#92:

  1. Yes, we should in principle/for consistency. Fixed. (It's okay to not do that for entity types where we're not testing multiple entities, which is why this passed tests.)
  2. I'm open to suggestions :)

This also needed a reroll for #2626924: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata anyway (note the changes to \Drupal\Tests\rest\Functional\EntityResource\EntityTest\EntityTestTextItemNormalizerTest, which that issue added).

amateescu’s picture

Wim Leers’s picture

Thanks! 👍

At this point, so many of us touched this patch. Who is still able to RTBC this?

Berdir’s picture

I think I can once we have the issue summary update + change record :)

alexpott’s picture

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -235,9 +258,15 @@ public function setUp() {
+   * @param int|null $which
+   *   An integer indicating which POST entity to generate. (Some entity types
+   *   have a required field that must contain a unique value. To test certain
+   *   edge cases, multiple distinct entities need to be created. This integer
+   *   then indicates which set of entity field values to create.)

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Feed/FeedResourceTestBase.php
@@ -143,7 +148,7 @@ protected function getNormalizedPostEntity() {
-          'value' => 'http://example.com/feed'
+          'value' => 'http://example.com/feed' . $which

This the only use of $which I can find. So it's not really being used to indicate a set of entity field values - it's being used as modifier of field values. To ensure uniqueness. So $unique_value_modifier might be better? Or something along those lines.

Wim Leers’s picture

#97: But "unique value modifier" is highly specific to this one use case. In this case, that $which value can indeed be used as a modifier. But for other use cases, that may not be possible. The general case looks like this:

switch ($which) {
  case NULL:
   // return default field values to POST
  case 2:
   // return an alternative set of field values to POST
…
  case N:
   // return …

Right now, this is only being applied to FeedResourceTestBase, for the Feed entity type. But there's quite a few fields lacking validation constraints, it's likely more cases of this will be encountered in the future.

That is being used in the great test coverage you (Alex Pott) added (see #54, and which didn't take the Feed case into account yet, because its test coverage didn't exist yet at the time:

+    if ($this->entity->getEntityType()->getStorageClass() !== ContentEntityNullStorage::class && $this->entity->getEntityType()->hasKey('uuid')) {
+      // 500 when creating an entity with a duplicate UUID.
+      $normalized_entity = $this->getNormalizedPostEntity(2);
+      $normalized_entity[$created_entity->getEntityType()->getKey('uuid')] = [['value' => $created_entity->uuid()]];
+      $normalized_entity[$label_field] = [['value' => $this->randomMachineName()]];
+      $request_options[RequestOptions::BODY] = $this->serializer->encode($normalized_entity, static::$format);
+
+      $response = $this->request('POST', $url, $request_options);
+      $this->assertSame(500, $response->getStatusCode());
+      $this->assertContains('Internal Server Error', (string) $response->getBody());
+
+      // 201 when successfully creating an entity with a new UUID.
+      $normalized_entity = $this->getNormalizedPostEntity(2);
+      $new_uuid = \Drupal::service('uuid')->generate();
+      $normalized_entity[$created_entity->getEntityType()->getKey('uuid')] = [['value' => $new_uuid]];
+      $normalized_entity[$label_field] = [['value' => $this->randomMachineName()]];
+      $request_options[RequestOptions::BODY] = $this->serializer->encode($normalized_entity, static::$format);
+
+      $response = $this->request('POST', $url, $request_options);
+      $this->assertResourceResponse(201, FALSE, $response);
+      $entities = $this->entityStorage->loadByProperties([$created_entity->getEntityType()->getKey('uuid') => $new_uuid]);
+      $new_entity = reset($entities);
+      $this->assertNotNull($new_entity);
+      $new_entity->delete();
+    }

So I think $unique_value_modifier is actually more confusing? Maybe that's just me though. I provided some extra detail in this comment to hopefully help land on the best possible solution.

Wim Leers’s picture

Just after posting #98, I realized something: this whole $which business is necessary because we're trying to reuse $this->getNormalizedPostEntity() to POST an entity which already has been created, just to test duplicate UUID behavior.

But … #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior already added logic to deal with something quite similar: \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::getModifiedEntityForPatchTesting() is able to take an entity and replace values of fields we know to be problematic and replace them with random values. What if we do something similar here?

What if we add protected static $uniqueFieldNames (similar to protected static $patchProtectedFieldNames;) and in the test coverage quoted in #98 iterate over those unique field names and generate random values for those? Wouldn't that be much simpler?

alexpott’s picture

@Wim Leers #99 that sounds great.

amateescu’s picture

Implemented #99, re-wrote the issue summary and added a change record: https://www.drupal.org/node/2934705

Berdir’s picture

  1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -254,6 +285,29 @@ protected function getNormalizedPatchEntity() {
    +      $field_definition = $this->entity->{$field_name}->getFieldDefinition();
    

    $this->entity->getFieldDefinition($field_name) is a bit cleaner than this I'd say?

  2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -1308,15 +1404,26 @@ protected static function getModifiedEntityForPatchTesting(EntityInterface $enti
    +      case 'label':
    +        // 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';
    +        break;
    +      case 'id':
    +        $normalization[$this->entity->getEntityType()->getKey('id')][0]['value'] = $this->anotherEntity->id();
    +        break;
    +      case 'uuid':
    +        $normalization[$this->entity->getEntityType()->getKey('uuid')][0]['value'] = $this->anotherEntity->uuid();
    +        break;
    

    Would it make sense to do a $entity_type = $this->entity->getEntityType(); above, could make especially the line with the two calls quite a bit shorter and easier to read?

  3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Item/ItemResourceTestBase.php
    @@ -81,6 +81,20 @@ protected function createEntity() {
    +    $entity->setLink('https://www.exmaple.org/');
    

    Is this a typo or done on purpose? :)

Nice how much shorter this got :)

Issue summary and CR looks pretty good to me.

> A solution for entity types with numeric (auto-incremented) IDs does not exist yet.

Well, there is a solution, that's to completely override the fieldAccess() method and do whatever you want. Not sure how much of that we want to mention, considering the possible security issues with that.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.16 KB
23.65 KB

#101: 🎉👏 Such a short comment, such a big interdiff, much smaller patch — so much better!

All points in #102 are great nits. Since this is blocking important Workflow Initiative patches, I just fixed those nits for @amateescu :)

alexpott’s picture

@amateescu++
@Wim Leers++
lovely to see this patch become way smaller.

alexpott’s picture

Adding credits for reviews and reviews on the related security issue.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 4fbf997792 to 8.6.x and dafa5f1176 to 8.5.x. Thanks!

Wim Leers’s picture

Issue tags: +8.5.0 release notes

Not a highlight of this release, but worth mentioning in the release notes.

  • alexpott committed 4fbf997 on 8.6.x
    Issue #2885469 by Wim Leers, amateescu, timmillwood, alexpott,...

  • alexpott committed dafa5f1 on 8.5.x
    Issue #2885469 by Wim Leers, amateescu, timmillwood, alexpott,...

Status: Fixed » Closed (fixed)

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