Problem/Motivation

#2808233: REST 403 responses don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out introduced the infrastructure to be able to surface the reason for why access is not being allowed. Over there, we used that to provide meaningful, actionable 403 responses when entity access is denied. But … we never did the same for field access!

(I noticed this while working on JSON API test coverage. See #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only.)

Proposed resolution

When generating a 403 response due to field access not being allowed, check if the access result object contains a reason, and expose it in the error response.

Remaining tasks

Implement!

User interface changes

None.

API changes

None.

Data model changes

CommentFileSizeAuthor
#56 2938035-56.patch21.31 KBWim Leers
#52 2938035-52.patch21.98 KBWim Leers
#52 interdiff.txt764 bytesWim Leers
#50 2938035-50.patch21.3 KBWim Leers
#50 interdiff.txt1.4 KBWim Leers
#48 2938035-48.patch21.3 KBWim Leers
#48 interdiff.txt2.63 KBWim Leers
#44 2938035-44.patch20.74 KBWim Leers
#44 interdiff.txt2.02 KBWim Leers
#42 2938035-42.patch20.71 KBWim Leers
#42 interdiff.txt1.7 KBWim Leers
#34 2938035-34-8.5.x.patch17.51 KBWim Leers
#34 2938035-34-8.6.x.patch19.7 KBWim Leers
#34 interdiff.txt757 bytesWim Leers
#28 2938035-28-8.5.x.patch17.25 KBWim Leers
#28 2938035-28-8.6.x.patch19.44 KBWim Leers
#26 2938035-26-8.6.x.patch22 KBWim Leers
#26 2938035-26-8.5.x.patch19.81 KBWim Leers
#26 interdiff-23-26-8.6.x.txt2.26 KBWim Leers
#23 2938035-21-8.6.x.patch19.81 KBWim Leers
#21 interdiff-18-21.txt4.58 KBWim Leers
#21 2938035-21.patch19.81 KBWim Leers
#18 2938035-18.patch16.21 KBWim Leers
#18 interdiff-16-18.txt4.4 KBWim Leers
#16 2938035-16.patch11.92 KBWim Leers
#16 interdiff-8-16.txt1.42 KBWim Leers
#9 2938035-8.patch10.56 KBWim Leers
#9 interdiff-7-8.txt3.19 KBWim Leers
#7 2938035-7.patch7.86 KBWim Leers
#7 interdiff-6-7.txt1.3 KBWim Leers
#6 2938035-6.patch6.59 KBWim Leers
#6 interdiff-4-6.txt1.34 KBWim Leers
#4 2938035-4.patch5.28 KBWim Leers
#4 interdiff-3-4.txt3.43 KBWim Leers
#3 2938035-3.patch3.84 KBWim Leers
#3 interdiff-2-3.txt1.92 KBWim Leers
#2 2938035-2.patch1.98 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Status: Active » Needs review
FileSize
1.98 KB

This is all that's necessary! Building on all that previous work is paying off 😍 🎉

Wim Leers’s picture

FileSize
1.92 KB
3.84 KB

Well, actually, thanks to #2885469: Regression: manually setting the ID field for newly-created content entities is not possible anymore (public follow-up to SA-2017-002) we now also have two fields (id and uuid) that can never be changed. But that's also a super helpful reason to list!

Wim Leers’s picture

FileSize
3.43 KB
5.28 KB

Those non-trailing periods in #3 look sloppy.

Wim Leers’s picture

Wim Leers’s picture

FileSize
1.34 KB
6.59 KB

… but #3 and #4 are passing, even though they should not.

The reason they're passing is that:

  • for REST tests, we also have the rest_test module installed, which implements rest_test_entity_field_access() and results AccessResult::neutral() by default (indicating it has no opinion, and hence not providing a reason)
  • … and \Drupal\Core\Entity\EntityAccessControlHandler::fieldAccess() merges both the default access result (that for the entity type) and those of hooks, and in doing so it uses AccessResult::orIf(). Turns out AccessResult::orIf() contains a bug. 😞

Here's a failing test to prove that bug. (We should move this to a separate issue.)

Wim Leers’s picture

FileSize
1.3 KB
7.86 KB

This fixes #6.

(Again, this should be moved to a separate bugfix issue.)

This should make the test added in #6 pass, and the REST tests should now fail.

The last submitted patch, 2: 2938035-2.patch, failed testing. View results

Wim Leers’s picture

Issue tags: -Needs tests
FileSize
3.19 KB
10.56 KB

… and this then finally is adding that test coverage that proves that 403 responses for trying to modify fields that the user is not allowed to modify now contain the reason *why*.

(This patch is functionally equivalent to #2930028-30: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only for JSON API, and in fact derived from it!)

Wim Leers’s picture

Issue tags: +Needs change record

This probably merits a change record to announce it to the world.

The last submitted patch, 3: 2938035-3.patch, failed testing. View results

Wim Leers’s picture

Title: When PATCHing a field is disallowed, no reason is given for *why* this happens » [PP-1] When PATCHing a field is disallowed, no reason is given for *why* this happens
Related issues: +#2938053: AccessResult::orIf() fails to retain the reason if both operands are neutral or forbidden, but the first contains a reason and the second one does not

The last submitted patch, 4: 2938035-4.patch, failed testing. View results

The last submitted patch, 6: 2938035-6.patch, failed testing. View results

The last submitted patch, 7: 2938035-7.patch, failed testing. View results

Wim Leers’s picture

FileSize
1.42 KB
11.92 KB

#4 failed to update a test.

Status: Needs review » Needs work

The last submitted patch, 16: 2938035-16.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
4.4 KB
16.21 KB

Patches so far only updated expectations for Node. This now also updates expectations for:

  • BlockContent
  • Comment
  • Media
  • MenuLinkContent
  • Term
  • User

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

Status: Needs review » Needs work

The last submitted patch, 18: 2938035-18.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
19.81 KB
4.58 KB

Still missed a few spots in #18.

Status: Needs review » Needs work

The last submitted patch, 21: 2938035-21.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
19.81 KB

Status: Needs review » Needs work

The last submitted patch, 23: 2938035-21-8.6.x.patch, failed testing. View results

Wim Leers’s picture

So the 8.5 patch passed, the 8.6 patch didn't just yet. This is going to be a bit of a pain…

Wim Leers’s picture

8.6.x diverged even more from 8.5.x already than I previously thought.

(I've renamed the 8.5.x patch from #21 that passed, it didn't change at all.)

Wim Leers’s picture

Status: Needs review » Postponed
Wim Leers’s picture

Title: [PP-1] When PATCHing a field is disallowed, no reason is given for *why* this happens » When PATCHing a field is disallowed, no reason is given for *why* this happens
Status: Postponed » Needs review
FileSize
19.44 KB
17.25 KB
borisson_’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Translation updates needed

Not sure if Translation updates needed is the right tag for this, but we're changing some strings, by adding a period, and that should be announced.

Otherwise this looks very solid.

For the next reviewer, the only changes that are done to actual code are in 2 files:

core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
core/modules/rest/src/Plugin/rest/resource/EntityResource.php

The rest seems to be test changes, to test the new behavior.

Wim Leers’s picture

Thanks for the review! :)

but we're changing some strings, by adding a period, and that should be announced.

The string we're changing is not a translatable string, it's a developer-only, English-only string — equivalent to an exception message. Hence removing that issue tag.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: 2938035-28-8.5.x.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Composer failed when re-testing the 8.5 patch. Retesting.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonAnonTest.php
@@ -25,11 +25,11 @@ class CommentHalJsonAnonTest extends CommentHalJsonTestBase {
   protected static $patchProtectedFieldNames = [
-    'entity_id',
-    'changed',
-    'thread',
-    'entity_type',
-    'field_name',
+    'entity_id' => NULL,
+    'changed' => NULL,
+    'thread' => NULL,
+    'entity_type' => NULL,
+    'field_name' => NULL,
   ];

I think this change is problematic. Any contrib or custom entity that extends \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase is going to have to change.

At the very least the docs for \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::$patchProtectedFieldNames need updating but I think we should implement this with addition rather than change. Maybe add a new accessDeniedReasonFieldMap property or something.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
757 bytes
19.7 KB
17.51 KB

Any contrib or custom entity that extends \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase is going to have to change.

The tests continue to evolve to become more strict. We've made similar test coverage breaks in the past, and we will need to continue to do so. It is absolutely necessary to have evolvability of these tests, otherwise we can't bring the same guarantees to all entity types' REST resources. Also: https://www.drupal.org/core/d8-bc-policy#tests.

I completely understand where you're coming from, and ideally we'd do that, but the unfortunate reality is that we need to stabilize a REST API that should not have shipped with 8.0.0, which includes writing test coverage that should have been there from the very start.

I updated the docs for the property.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: 2938035-34-8.5.x.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

I tested the 8.5.x patch against 8.6.x. Of course that does not apply. 🙄

Retesting.

Sorry, getting back into it!

alexpott’s picture

@Wim Leers I agree that these tests need to evolve but why do unnecessary breakages? This can be done in a BC manner that if no reasons are provided that the test doesn't need to change. Whereas with the current fix every contrib / custom test has to change.

Wim Leers’s picture

This can be done in a BC manner that if no reasons are provided that the test doesn't need to change.

Likely 100% of entity type's access control handlers use \Drupal\Core\Access\AccessResult::allowedIfHasPermission(), which means that likely 100% of the tests would need to define this additional property. Otherwise, their tests would still fail. It's all coupled!

The last submitted patch, 34: 2938035-34-8.6.x.patch, failed testing. View results

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

@Wim Leers okay so what do you think about helping people and produce a nice error that helps people upgrade if the array is not as expected. At the moment they'll probably get so quite hard to understand error?

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.7 KB
20.71 KB

Sure, done! Manually tested:

#!/usr/bin/env php
PHPUnit 6.5.8 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\rest\Functional\EntityResource\Node\NodeJsonBasicAuthTest
F                                                                   1 / 1 (100%)

Time: 8.1 seconds, Memory: 6.00MB

There was 1 failure:

1) Drupal\Tests\rest\Functional\EntityResource\Node\NodeJsonBasicAuthTest::testPatch
In Drupal 8.6, the structure of $patchProtectectedFieldNames changed. It used to be an array with field names as values. Now those values are the keys, and their values should be either NULL or a string: a string containing the reason for why the field cannot be PATCHed, or NULL otherwise. in /Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:1112

FAILURES!
Tests: 1, Assertions: 90, Failures: 1.

Process finished with exit code 1
alexpott’s picture

The new anonymous function is failing style checks.

----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------
 1112 | ERROR | [x] Opening brace must be the last content on the
      |       |     line
 1112 | ERROR | [x] There should be no white space after an opening
      |       |     "{"
 1112 | ERROR | [x] There should be no white space before a closing
      |       |     "}"
 1112 | ERROR | [x] Closing brace must be on a line by itself
----------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Because it's not simple it'd be great to have a patch on issue that doesn't introduce this.

Wim Leers’s picture

There's no way to fix this unless we spread it across multiple lines. Assuming we don't want that, // @codingStandardsIgnoreLine is the only solution.

If you prefer to spread an assert function across multiple lines instead, happy to oblige!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -1106,15 +1109,15 @@ public function testPatch() {
     // DX: 403 when sending PATCH request with updated read-only fields.
+    assert(Inspector::assertAllStrings(array_keys(static::$patchProtectedFieldNames)) && Inspector::assertAll(function ($value) { return is_null($value) || is_string($value); }, static::$patchProtectedFieldNames), 'In Drupal 8.6, the structure of $patchProtectectedFieldNames changed. It used to be an array with field names as values. Now those values are the keys, and their values should be either NULL or a string: a string containing the reason for why the field cannot be PATCHed, or NULL otherwise.'); // @codingStandardsIgnoreLine

Doing some more thinking about this. Why not just use regular test assertions here and fail the test if the array is not the expected format? I'm not sure why we need to use the runtime assert() system here.

Wim Leers’s picture

I think using test assertions in a test base class to test the test (subclass) itself would be a first. That's why: because test assertions should only be used to test application behavior, not test implementation? Happy to oblige, but I'm honestly a bit surprised, so double-checking before doing it :)

alexpott’s picture

Well for me the runtime assert system is something that is disable-able and this is not an error you'd want disabled. I think it is okay for us to trigger a test error here. After all the test is going to fail.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.63 KB
21.3 KB

Works for me :) Done!

Status: Needs review » Needs work

The last submitted patch, 48: 2938035-48.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.4 KB
21.3 KB

It helps if I run at least one test locally first 😅

Status: Needs review » Needs work

The last submitted patch, 50: 2938035-50.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
764 bytes
21.98 KB

Yay #50 worked as expected!

This issue was last RTBC on April 23. #2784921: Add Workspaces experimental module was committed on May 4. So the 3 failures make sense :)

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

Can we get the change record updated to include the developer changes required in tests that extend the base class - i.e. changing the values to keys and adding expected messages?

Other than that, this looks ready to go.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 52: 2938035-52.patch, failed testing. View results

Wim Leers’s picture

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

Needed a rebase now that #2910883: Move all entity type REST tests to the providing modules landed. Straight rebase. Identical diffstat. So fairly certain I didn't make mistake.

larowlan’s picture

Adding @alexpott to review credits

  • larowlan committed 585e633 on 8.6.x
    Issue #2938035 by Wim Leers, alexpott: When PATCHing a field is...
larowlan’s picture

Committed as 585e633 and pushed to 8.6.x.

Published change record

Wim Leers’s picture

Status: Reviewed & tested by the community » Fixed

WOOT!

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture

Issue tags: +8.6.0 highlights