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
Comment | File | Size | Author |
---|---|---|---|
#56 | 2938035-56.patch | 21.31 KB | Wim Leers |
#52 | 2938035-52.patch | 21.98 KB | Wim Leers |
#52 | interdiff.txt | 764 bytes | Wim Leers |
#50 | 2938035-50.patch | 21.3 KB | Wim Leers |
#50 | interdiff.txt | 1.4 KB | Wim Leers |
Comments
Comment #2
Wim LeersThis is all that's necessary! Building on all that previous work is paying off 😍 🎉
Comment #3
Wim LeersWell, 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
anduuid
) that can never be changed. But that's also a super helpful reason to list!Comment #4
Wim LeersThose non-trailing periods in #3 look sloppy.
Comment #5
Wim Leers(For #3.)
Comment #6
Wim Leers… but #3 and #4 are passing, even though they should not.
The reason they're passing is that:
rest_test
module installed, which implementsrest_test_entity_field_access()
and resultsAccessResult::neutral()
by default (indicating it has no opinion, and hence not providing a reason)\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 usesAccessResult::orIf()
. Turns outAccessResult::orIf()
contains a bug. 😞Here's a failing test to prove that bug. (We should move this to a separate issue.)
Comment #7
Wim LeersThis 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.
Comment #9
Wim Leers… 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!)
Comment #10
Wim LeersThis probably merits a change record to announce it to the world.
Comment #12
Wim LeersMoved #6 + #7 into its own issue: #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.
Comment #16
Wim Leers#4 failed to update a test.
Comment #18
Wim LeersPatches so far only updated expectations for
Node
. This now also updates expectations for:BlockContent
Comment
Media
MenuLinkContent
Term
User
Comment #21
Wim LeersStill missed a few spots in #18.
Comment #23
Wim Leers#21 applied to 8.5. Here's a 8.6 rebase.
(They diverge because #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration was committed only to 8.6.x.)
Comment #25
Wim LeersSo the 8.5 patch passed, the 8.6 patch didn't just yet. This is going to be a bit of a pain…
Comment #26
Wim Leers8.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.)
Comment #27
Wim Leers#26 should have green patches for 8.5.x and 8.6.x. Now marking this postponed, because #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 is a hard blocker.
Comment #28
Wim Leers#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 landed!
Rebased.
Comment #29
borisson_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.
Comment #30
Wim LeersThanks for the review! :)
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.
Comment #32
Wim LeersComposer failed when re-testing the 8.5 patch. Retesting.
Comment #33
alexpottI 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.Comment #34
Wim LeersThe 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.
Comment #35
Wim LeersCR created: https://www.drupal.org/node/2958395.
Comment #37
Wim LeersI tested the 8.5.x patch against 8.6.x. Of course that does not apply. 🙄
Retesting.
Sorry, getting back into it!
Comment #38
alexpott@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.
Comment #39
Wim LeersLikely 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!Comment #41
alexpott@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?
Comment #42
Wim LeersSure, done! Manually tested:
Comment #43
alexpottThe new anonymous function is failing style checks.
Because it's not simple it'd be great to have a patch on issue that doesn't introduce this.
Comment #44
Wim LeersThere'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!
Comment #45
alexpottDoing 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.
Comment #46
Wim LeersI 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 :)
Comment #47
alexpottWell 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.
Comment #48
Wim LeersWorks for me :) Done!
Comment #50
Wim LeersIt helps if I run at least one test locally first 😅
Comment #52
Wim LeersYay #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 :)
Comment #53
larowlanCan 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.
Comment #54
Wim LeersCR updated: https://www.drupal.org/node/2958395 — changes: https://www.drupal.org/node/2958395/revisions/view/10908423/10967278
Comment #56
Wim LeersNeeded 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.
Comment #57
larowlanAdding @alexpott to review credits
Comment #59
larowlanCommitted as 585e633 and pushed to 8.6.x.
Published change record
Comment #60
Wim LeersWOOT!
Comment #62
Wim Leers