EntityResource::patchIndividual() should allow fields that the user is not allowed to change, as long as they match the current value [#2939810]

Problem/Motivation

#2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior recently made very important fixes to the core REST module. In doing so, it introduced an important change, which follows https://en.wikipedia.org/wiki/Robustness_principle.

It allows one to send values in PATCH requests, even for fields that the user is not allowed to modify, as long as they match the currently stored values.

This means developers can GET an entity, decode it from JSON into an object/array structure in JS, change a value, encode it again into JSON, and have it work. Today, they cannot do that: they have to explicitly omit fields that the user is not allowed to modify, otherwise they'll get a 403!

Proposed resolution

Implement the same change in JSON API.

Remaining tasks

Roll patch, which should copy/paste the logic from core, plus #2938035: When PATCHing a field is disallowed, no reason is given for *why* this happens to ensure we still send the same excellent field access 403 responses that JSON API already does better than core REST!
~~Wait for #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only to land.~~

User interface changes

None.

API changes

None.

Data model changes

None.

Comment	File	Size	Author
#12	2939810-12.patch	6.73 KB	Wim Leers
#12	8.x-1.x: PHP 7 & MySQL 5.5, D8.6 534 pass
#12	interdiff-9-12.txt	3.37 KB	Wim Leers
#9	2939810-9.patch	5.25 KB	Wim Leers
#9	8.x-1.x: PHP 7 & MySQL 5.5, D8.6 531 pass, 1 fail
#7	2939810-7.patch	5.96 KB	Wim Leers
#7
#3	2939810-3.patch	5.85 KB	Wim Leers
#3

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

26 January 2018 at 15:30

Wim Leers created an issue. See original summary.

Comment #2

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 26 January 2018 at 15:32

Issue summary:	View changes
Related issues:		+#2938035: When PATCHing a field is disallowed, no reason is given for why this happens

Comment #3

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 26 January 2018 at 15:37

File	Size
2939810-3.patch	5.85 KB

Comment #4

e0ipso

Catalan

Can Picafort

CreditAttribution: e0ipso as a volunteer and at Lullabot commented 28 January 2018 at 20:32

Should we add a "warning" or an "error" in the response now that we have support for the Partial Success extension?

We could highlight the fields that would be un-editable, and that are recommended to be dropped from the payload.

Comment #5

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 29 January 2018 at 11:05

There's no need for any warning at all when sending unchanged values for fields. That means you don't intend to change anything. Receiving warnings for things that are fine and safe seems unnecessarily scary?

Comment #6

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 7 February 2018 at 11:22

In that same issue, we ran into #2824851-157: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior:

[…] UserResourceTestBase::testPatchDxForSecuritySensitiveBaseFields() REST tests are failing […]

This is because \Drupal\user\Plugin\Validation\Constraint\ProtectedUserFieldConstraintValidator::validate() calls \Drupal\user\Entity\User::checkExistingPassword() which checks $user->get('pass')->existing … which … no longer gets set since #148 because it now only sets a value if it is different from the current value. Before, it was always being set.

And then in #2824851-194: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior:

[…]
Well … I actually investigated it a bit more, but still wanted to post that. The original reason for introducing that is at #157. And that reasoning was correct: the pass field was no longer being set for fields whose value hadn't changed.
But! Thanks to @cburschka's analysis, we ended up with a different implementation, which does always set the value. So actually, we didn't need the special casing of User's pass field anymore!

The patch in #3 also fixes this case: it ensures that User's pass field always gets set.

This issue should therefore also re-activate the test coverage that #2930028-54: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only is adding in \Drupal\Tests\jsonapi\Functional\UserTest::testPatchSecurityOtherUser()

Comment #7

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 8 February 2018 at 09:40

File	Size
2939810-7.patch	5.96 KB

Rebased.

Comment #8

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 16 February 2018 at 15:44

Title:	[PP-1] EntityResource::patchIndividual() should allow fields that the user is not allowed to change, as long as they match the current value	» EntityResource::patchIndividual() should allow fields that the user is not allowed to change, as long as they match the current value
Assigned:	Unassigned	» Wim Leers
Issue summary:	View changes
Status:	Postponed	» Active

#2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only (and #2932031: Comprehensive JSON API integration test coverage phase 2: for every entity type, complex use-cases) landed, this can now be worked on.

Comment #9

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 16 February 2018 at 15:49

Status:

Active

» Needs review

File	Size
2939810-9.patch	5.25 KB
8.x-1.x: PHP 7 & MySQL 5.5, D8.6 531 pass, 1 fail

2 files were hidden/shown/deleted

File	Size
2939810-3.patch	5.85 KB

2939810-7.patch	5.96 KB

Rebased #7 (had to fix some conflicts).

Comment #10

16 February 2018 at 15:52

Status:

Needs review

» Needs work

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

Comment #11

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 16 February 2018 at 15:57

That worked … for everything except File. Same exact failure on 8.4 and 8.6. Investigating…

Comment #12

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 16 February 2018 at 16:24

Status:

Needs work

» Needs review

File	Size
interdiff-9-12.txt	3.37 KB
2939810-12.patch	6.73 KB
8.x-1.x: PHP 7 & MySQL 5.5, D8.6 534 pass

1 file was hidden/shown/deleted

File	Size
2939810-9.patch	5.25 KB
8.x-1.x: PHP 7 & MySQL 5.5, D8.6 531 pass, 1 fail

That failed because jsonapi is adding a url computed field (which we'll deprecate in #2926463: [>=8.5] Remove JSON API's "file URL" field work-around now that Drupal core 8.5 fixed it), and the class corresponding to it does not compute it at all necessary times. This is similar to what we saw in Drupal core with Node's path field. To solve that, we did #2392845: Add a trait to standardize handling of computed item lists, which introduced a trait to standardize how computed field classes should behave, to prevent the same class of bugs from appearing everywhere.

In this case, it's failing because comparing the stored File entity's url field value with the value in the PATCH request concluded that the stored and received values were not the same. But they really are.

The solution is simple: updating \Drupal\jsonapi\Field\FileDownloadUrl to use the trait that #2392845 introduced.

Comment #13

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 16 February 2018 at 16:43

Assigned:

Wim Leers

» Unassigned

#12 is green on both 8.4 and 8.6!

Comment #14

gabesullice

English

CreditAttribution: gabesullice at Acquia commented 19 February 2018 at 10:26

Status:

Needs review

» Reviewed & tested by the community

Looks good. Super job watching out for the security pitfalls.

Comment #15

24 February 2018 at 09:51

e0ipso committed 1eafaf8 on 8.x-1.x authored by Wim Leers

Issue #2939810 by Wim Leers, e0ipso, gabesullice: EntityResource::...

Comment #16

e0ipso

Catalan

Can Picafort

CreditAttribution: e0ipso as a volunteer and at Lullabot commented 24 February 2018 at 09:57

There was a minor issue applying the patch. I think all went well after all.

 curl https://www.drupal.org/files/issues/2939810-12.patch | git apply -3
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  6890  100  6890    0     0  29880      0 --:--:-- --:--:-- --:--:-- 29826
error: patch failed: tests/src/Functional/ResourceTestBase.php:1181
Falling back to three-way merge...
Applied patch to 'tests/src/Functional/ResourceTestBase.php' cleanly.