Discovered in #2869426-71: EntityResource should add _entity_access requirement to REST routes.

CommentFileSizeAuthor
#20 interdiff-19-20.txt1.8 KBmsankhala
#20 2950127-20.patch1.8 KBmsankhala
#19 interdiff-2950127-16-19.txt1.58 KByogeshmpawar
#19 2950127-19.patch1.8 KByogeshmpawar
#16 interdiff-2950127-12-16.txt1.51 KByogeshmpawar
#16 core-file_access_messsaging-2950127-16.patch1.5 KByogeshmpawar
#12 interdiff.txt939 bytesowenbush
#12 core-file_access_messsaging-2950127-11.patch1.5 KBowenbush
#9 interdiff.txt810 bytesowenbush
#9 core-file_access_messsaging-2950127-9.patch1.42 KBowenbush
#5 core-file_access_messsaging-2950127-5.patch645 bytesowenbush

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2950125: Add helpful reason for 'update' and 'delete' access not being allowed to CommentAccessControlHandler, +#2950129: Add helpful reason for 'update' and 'delete' access not being allowed to MediaAccessControlHandler
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Issue tags: +dcruhr18
owenbush’s picture

owenbush commented

Looking at this at DrupalCon Nashville.

owenbush’s picture

owenbush commented
StatusFileSize
new core-file_access_messsaging-2950127-5.patch645 bytes

I have added a message to the forbidden AccessResult call which details that only file owners can update or delete file entities.

owenbush’s picture

owenbush commented
Status: Active » Needs review

Needs review.

Status: Needs review » Needs work

The last submitted patch, 5: core-file_access_messsaging-2950127-5.patch, failed testing. View results

josephdpurcell’s picture

josephdpurcell commented
Status: Needs work » Active

Looking at the failed tests, this looks a similar state to https://www.drupal.org/project/drupal/issues/2950125#comment-12570703

The tests that are failing should be updated with the new error message you've set.

owenbush’s picture

owenbush commented
StatusFileSize
new core-file_access_messsaging-2950127-9.patch1.42 KB
new interdiff.txt810 bytes

Thanks for that. I have updated the test case. I've provided a new patch and an interdiff.

owenbush’s picture

owenbush commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 9: core-file_access_messsaging-2950127-9.patch, failed testing. View results

owenbush’s picture

owenbush commented
StatusFileSize
new core-file_access_messsaging-2950127-11.patch1.5 KB
new interdiff.txt939 bytes

I guess in this instance it should be a DELETE type, so I added that to the test case. Attached is new patch and a new interdiff.

owenbush’s picture

owenbush commented
Status: Needs work » Needs review
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Needs work 
+++ b/core/modules/file/src/FileAccessControlHandler.php
@@ -68,7 +68,7 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
+      return AccessResult::forbidden('Only the file owner can delete and update the file entity.');

I'm not sure about and here.

I think update or delete the file entity is a better description, because you can't update and delete at the same time.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Assigned: Unassigned » yogeshmpawar
yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Assigned: yogeshmpawar » Unassigned
Status: Needs work » Needs review
StatusFileSize
new core-file_access_messsaging-2950127-16.patch1.5 KB
new interdiff-2950127-12-16.txt1.51 KB

Agree with #14 comment it should be update or delete the file entity rather than update and delete the file entity so added updated patch with an interdiff.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work 
+++ b/core/modules/file/src/FileAccessControlHandler.php
@@ -68,7 +68,7 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
+      return AccessResult::forbidden('Only the file owner can delete or update the file entity.');

+++ b/core/modules/rest/tests/src/Functional/EntityResource/File/FileResourceTestBase.php
@@ -224,8 +224,8 @@ protected function getExpectedUnauthorizedAccessMessage($method) {
+    if ($method === 'PATCH' || $method == 'DELETE') {
+      return 'Only the file owner can delete or update the file entity.';

One of the implicit, but important point of @borisson_ is:

update (first) or delete (second)

The title of issue also comply with this order.

The code check with this order (PATCH is 'update').
if ($method === 'PATCH' || $method == 'DELETE') {
Therefore it will be good if the message will also comply with this order.

Also we use strict compare, so:

-  $method == 'DELETE'
+  $method === 'DELETE'
yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Assigned: Unassigned » yogeshmpawar
yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Assigned: yogeshmpawar » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 2950127-19.patch1.8 KB
new interdiff-2950127-16-19.txt1.58 KB

Agree with @vaplas, but currently adding only strict compare for "DELETE" method. Interdiff is also added.

msankhala’s picture

msankhala commented
StatusFileSize
new 2950127-20.patch1.8 KB
new interdiff-19-20.txt1.8 KB

Here is updated patch as per code review suggestions.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Reviewed & tested by the community

Looks nice!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

😍👍 Perfect!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/core/modules/file/src/FileAccessControlHandler.php
@@ -64,11 +64,11 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
-      // Only the file owner can delete and update the file entity.
+      // Only the file owner can update or delete the file entity.
       if ($account->id() == $file_uid[0]['target_id']) {
         return AccessResult::allowed();
       }
-      return AccessResult::forbidden();
+      return AccessResult::forbidden('Only the file owner can update or delete the file entity.');

I'm not sure the comment adds anything anymore because it's now part of the code too. But I guess this doesn't really matter.

Creditting @Wim Leers and @vaplas for reviews.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed ec8b4f8 and pushed to 8.6.x. Thanks!

  • alexpott committed ec8b4f8 on 8.6.x 
    Issue #2950127 by owenbush, Yogesh Pawar, msankhala, Wim Leers, vaplas:...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2971277: FileTest::testPatchIndividual() and FileTest::testDeleteIndividual() failing on 8.6

Created #2971277: FileTest::testPatchIndividual() and FileTest::testDeleteIndividual() failing on 8.6 to update JSON API's test coverage accordingly.

Status: Fixed » Closed (fixed)

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