Problem/Motivation

File Entity has no proper access checks like create, view, update, delete. It only has a 'download'. It checks for it's parent to allow viewing.

Rest module checks only for create, view, update, delete on entity type.

(#21)

It seems like the alternative is between:

  1. Have a solid plan to make file a first-class entity in Drupal 8, that includes REST support, proper access control for queries, proper rendering and form modes support;
  2. Just accept that all of this is going to be done in contrib, and as a consequence accept that the file entity in core doesn't support REST.

The latter has a few hard questions that need to be answered. Some of them:

  • What permissions do you need to "view" a file entity? How is that different from "download"ing the file that it represents?
  • How to implement access control for "list"ing file entities. Do those apply to formatters for the file reference field?
  • How should the file entity implement view modes? Is the implementation in file_entity what we want for core?
  • How do form modes apply to the file entity?

Proposed resolution

Remaining tasks

Split permissions into own / all from #16

User interface changes

API changes

Original report by @vivekvpandya

I want to download any file attached to a node from my iOS device.
I created a node with a file attached to it.
I enabled REST endpoint for file with REST UI module.
I gave permission to admin user to access GET on file REST endpoint.
I used GET method with application/json as accept type on /node/id and I got

"field_file":[
{
"target_id":"12",
"display":"1",
"description":""
}
] 

for file field then I used GET on entity/file/12 with proper authentication though it gave me 403 Forbidden . Same is true for DELETE method.

I think here expected output for GET is actual file path and file type or other meta data related to file in json format so that a client can use it to download file.

Other interesting thing is if you use application/hal+json with GET on /node/id then it will return a full path for file So we can easily use that path to download files.

CommentFileSizeAuthor
#73 2310307-73_TEST_ONLY.patch2.69 KBtedbow
#73 interdiff-2310307-69-73.txt1.07 KBtedbow
#73 file-access-2310307-73.patch5.16 KBtedbow
#69 interdiff-2310307-63-69.txt1.39 KBtedbow
#69 file-access-2310307-69.patch4.77 KBtedbow
#69 file-access-2310307-69_TEST_ONLY.patch0 bytestedbow
#63 interdiff-file-access-2310307-59-63.txt1.59 KBmarthinal
#63 file-access-2310307-63.patch4.69 KBmarthinal
#63 2310307-63-only-test-should-fail.patch2.69 KBmarthinal
#59 2310307-59.patch4.29 KBmarthinal
#59 2310307-59-only-test-should-fail.patch2.29 KBmarthinal
#52 2310307-47-rerolled2.patch1.58 KBvivekvpandya
#47 2310307-47.patch1.65 KBmarthinal
#47 interdiff-2310307-39-47.txt836 bytesmarthinal
#43 Not wroking.png53.13 KBvivekvpandya
#42 Screen Shot 2015-08-10 at 18.30.59.png267.47 KBmarthinal
#39 2310307-39.patch1.69 KBmarthinal
#39 rest-interdiff-36-39-2310307.txt1.19 KBmarthinal
#36 2310307-36.patch1.52 KBmarthinal
#36 2310307-19-36-interdiff.txt1.15 KBmarthinal
#33 file not deleted.png130.31 KBvivekvpandya
#31 2310307-31.patch638 bytesmarthinal
#30 get_and_delete_not-2310307-30.patch7.02 KBqueenvictoria
#29 get_and_delete_not-2310307-29.patch1.79 KBqueenvictoria
#23 Selection_001.png1.48 MBjuampynr
#16 get_and_delete_not-2310307-15.patch1.87 KBclemens.tolboom
#10 get_and_delete_not-2310307-10.patch917 bytesclemens.tolboom
#1 get_and_delete_is_not-2310307-1.patch837 bytesclemens.tolboom
fileGet.png78.4 KBvivekvpandya
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

clemens.tolboom’s picture

Issue summary: View changes
Status: Active » Needs review
Related issues: +#1927648: Allow creation of file entities from binary data via REST requests
FileSize
837 bytes

Good finding. I created a quick patch.

Probably worth to check with #1927648: Allow creation of file entities from binary data via REST requests too.

I removed the image from your text.

Damien Tournoud’s picture

Component: rest.module » file.module
Status: Needs review » Needs work

The problem is in the File entity that doesn't implement correct access control. Introducing special cases in the Rest module is not the way to go.

clemens.tolboom’s picture

@Damien Tournoud thanks for the quick answer.

Do you mind to make it a little longer as it seems File is always connected to a parent entity as calling if (!$entity->access('download')) { seems to do.

What steps are feasible?

Damien Tournoud’s picture

@clemens: Rest doesn't need to know that some specific entity type has some specific access requirements. The File entity obviously don't implement access control properly or it would just work out of the box.

ParisLiakos’s picture

juampynr’s picture

I think that we fixed the issue described at the top at #2277705: Files don't have URI nor href.

Here is an example: Given that I have created a node with nid 36 with an image attached to it and REST is configured to access nodes, this is what I get when I request http://d8.local/node/36 with header Accept: application/hal+json:

{
   "_links":{
      "self":{
         "href":"http:\/\/d8.local\/node\/36"
      },
      "type":{
         "href":"http:\/\/d8.local\/rest\/type\/node\/page"
      },
      "http:\/\/d8.local\/rest\/relation\/node\/page\/uid":[
         {
            "href":"http:\/\/d8.local\/user\/0",
            "lang":"und"
         }
      ],
      "http:\/\/d8.local\/rest\/relation\/node\/page\/revision_uid":[
         {
            "href":"http:\/\/d8.local\/user\/0"
         }
      ],
      "http:\/\/d8.local\/rest\/relation\/node\/page\/field_image":[
         {
            "href":"http:\/\/d8.local\/sites\/default\/files\/images\/391412_10151380611596403_150295933_n.jpg",
            "lang":"und"
         }
      ]
   },
   "uuid":[
      {
         "value":"4e25471b-cdcc-44ab-a007-7e29c6b69ee1"
      }
   ],
   "type":[
      {
         "target_id":"page"
      }
   ],
   "langcode":[
      {
         "value":"und"
      }
   ],
   "title":[
      {
         "value":"New node title 120",
         "lang":"und"
      }
   ],
   "_embedded":{
      "http:\/\/d8.local\/rest\/relation\/node\/page\/uid":[
         {
            "_links":{
               "self":{
                  "href":"http:\/\/d8.local\/user\/0"
               },
               "type":{
                  "href":"http:\/\/d8.local\/rest\/type\/user\/user"
               }
            },
            "uuid":[
               {
                  "value":"47cddc06-113e-4fb5-a64e-78b75605953a"
               }
            ],
            "lang":"und"
         }
      ],
      "http:\/\/d8.local\/rest\/relation\/node\/page\/revision_uid":[
         {
            "_links":{
               "self":{
                  "href":"http:\/\/d8.local\/user\/0"
               },
               "type":{
                  "href":"http:\/\/d8.local\/rest\/type\/user\/user"
               }
            },
            "uuid":[
               {
                  "value":"47cddc06-113e-4fb5-a64e-78b75605953a"
               }
            ]
         }
      ],
      "http:\/\/d8.local\/rest\/relation\/node\/page\/field_image":[
         {
            "_links":{
               "self":{
                  "href":"http:\/\/d8.local\/sites\/default\/files\/images\/391412_10151380611596403_150295933_n.jpg"
               },
               "type":{
                  "href":"http:\/\/d8.local\/rest\/type\/file\/file"
               }
            },
            "uuid":[
               {
                  "value":"1065df18-3d9c-4f53-9d28-4046fc7cc69b"
               }
            ],
            "uri":[
               {
                  "value":"http:\/\/d8.local\/sites\/default\/files\/images\/391412_10151380611596403_150295933_n.jpg"
               }
            ],
            "lang":"und"
         }
      ]
   },
   "status":[
      {
         "value":"1",
         "lang":"und"
      }
   ],
   "created":[
      {
         "value":"1407602124",
         "lang":"und"
      }
   ],
   "changed":[
      {
         "value":"1407602124",
         "lang":"und"
      }
   ],
   "promote":[
      {
         "value":"0",
         "lang":"und"
      }
   ],
   "sticky":[
      {
         "value":"0",
         "lang":"und"
      }
   ],
   "revision_timestamp":[
      {
         "value":"1407602124"
      }
   ]
}

As you can see above, the file field and the embedded file entity at the top contain the full URIs and therefore the file can be accessed and downloaded if needed.

clemens.tolboom’s picture

The file entity cannot be accessed. The file itself is downloadable.

According to @Damien Tournoud and my current hunch we need proper crud ops on the file entity as badly demonstrated in #1

juampynr’s picture

I remember working on this issue some time ago and provided an access controller to it #2128791: File resource needs an access controller. At some point my patch would not apply anymore and the embedded file entity seemed to change in the REST output.

clemens.tolboom’s picture

Title: GET and DELETE is not working with entity/file/{id} » GET and DELETE not working with REST on entity/file/{id}
Issue tags: +REST
Related issues: +#2128791: File resource needs an access controller, +#2078473: Use entity access API for checking access to private files

@juampy guess #6 needs some condensation ;)

Questions to be answered

  1. Is File entity a standalone entity (by reference) or is File entity an embedded (like comment)?
  2. Is File entity deletable on it's own?
  3. Are we currently in line with what was invented by https://www.drupal.org/project/file_entity ?
clemens.tolboom’s picture

Attached patch is tiny step forward. How do I declare permissions for the file entity?

juampynr’s picture

Status: Needs work » Needs review

Using this patch I can access to a file at the path entity/file/1, but not with file/1. This is really weird since in theory we got rid of entity/* paths.

clemens.tolboom’s picture

Issue summary: View changes

@juampy afaik not all entities do have their canonical path used for rest. Running drush $DRUSH_ALIAS sql-query "SELECT name, path FROM router WHERE name LIKE 'rest.entity.%';" shows _ALL_ POST like rest.entity.comment.POST /entity/comment and for FILE all REST ops. See #2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available

This issue needs permissions added.

vivekvpandya’s picture

I am not able to apply this patch. I got following error
error: core/modules/file/src/FileAccessControlHandler.php: No such file or directory

clemens.tolboom’s picture

@vivekvpandya I can apply the patch #10

The file exists

$ git log core/modules/file/src/FileAccessControlHandler.php
commit 258856aee9940b0d59077d62eb7a3c8f155dd7f4
Author: Alex Pott <alex.a.pott@googlemail.com>
Date:   Thu Aug 7 23:27:28 2014 +0100

    Issue #2154435 by andypost, Berdir, mparker17: Rename EntityAccessController to EntityAccessControlHandler.

#2154435: Rename EntityAccessController to EntityAccessControlHandler

juampynr’s picture

Same here.

clemens.tolboom’s picture

Attached patch add permissions but these do not work for ie anonymous. This fails:

curl --user anonymous: --header "Accept: application/hal+json" --request GET http://drupal.d8/entity/file/1

while this works:

curl --user admin:admin --header "Accept: application/hal+json" --request GET http://drupal.d8/entity/file/1

clemens.tolboom’s picture

Title: GET and DELETE not working with REST on entity/file/{id} » File needs CRUD permissions to make REST work on entity/file/{id}

Changed title to reflect the CRUD ops on File entity

Berdir’s picture

+++ b/core/modules/file/src/FileAccessControlHandler.php
@@ -33,9 +33,24 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
     }
+    else {
+      $account = $this->prepareUser($account);
+
+      if ($operation == 'view') {
+        return $account->hasPermission('view file');
+      }

The common pattern is to make this a switch statement, including the existing download operation.

I'm not sure how to solve this in a good way, because adding generic permissions in file.module that are always checked but then not actually checking them anywhere is going to be very very confusing for users. And at the same time, I have no idea how we could check it in a useful way. What does it mean to be able to "create a file" in 8.x core?

It will also overlap with what file_entity in 7.x and our 8.x port is doing.

clemens.tolboom’s picture

@Berdir

I somehow expected File entity being a real (independent) entity until playing with Rest to discover it is not even accessible. So I'm following hint from #4 to add access checks.

In #1927648: Allow creation of file entities from binary data via REST requests we are trying to create a file entity which needs permissions.

I now realize I had to split permissions into '* all/own file' permissions which makes more sense.

imho permissions is a must have for #1927648: Allow creation of file entities from binary data via REST requests

Berdir’s picture

file entity in core always was a second class citizen, yes, which is unfortunate. Only file_entity.module changes that.

Yes, I'm perfectly aware what you are doing and it makes sense in the context of rest... but not anywhere else, as far as core is concerned. So site builders will see those permissions but it will not be clear to them what it actually means for the normal UI, like creating a node with file attachments? Would you expect that a user who does not have "create file" permission to be able to upload files? Or not? How can we even check that in a way that makes sense?

Damien Tournoud’s picture

It seems like the alternative is between:

  • Have a solid plan to make file a first-class entity in Drupal 8, that includes REST support, proper access control for queries, proper rendering and form modes support;
  • Just accept that all of this is going to be done in contrib, and as a consequence accept that the file entity in core doesn't support REST.

I'm ready to live with the latter, but if we don't think it's acceptable then we need a solid plan. Let's not go with a piece-meal approach, there are a few hard questions that need to be answered. Some of them:

  • What permissions do you need to "view" a file entity? How is that different from "download"ing the file that it represents?
  • How to implement access control for "list"ing file entities. Do those apply to formatters for the file reference field?
  • How should the file entity implement view modes? Is the implementation in file_entity what we want for core?
  • How do form modes apply to the file entity?
Berdir’s picture

Yeah, the problem with not supporting it in REST would also imply no support for files/images on nodes and other entity types. At least with our current suggestions in #1927648: Allow creation of file entities from binary data via REST requests. And doing that on the field item level as the earlier patches tried would IMHO be a bad idea.

juampynr’s picture

FileSize
1.48 MB

Based on the following by @berdir:

...What does it mean to be able to "create a file" in 8.x core?...

...it makes sense in the context of rest... but not anywhere else...

By the time we get to the access controller we already did meet the REST permissions:

Does it make sense if REST module implements a permissive access controller for files?

clemens.tolboom’s picture

Issue summary: View changes

@Damien Tournoud I totally agree.

I've updated the summary according to #21

@juampy I did that in #1 (sort of) but I agree with @Damien Tournoud in #2

My patches up to #16 were mere about sharing hot fixes then real ones. So hiding all.

dawehner’s picture

yes, file entity is somehow in a sad state.

File Entity has no proper access checks like create, view, update, delete. It only has a 'download'. It checks for it's parent to allow viewing.

This patch at least is a step forward, even maybe confusing to the site builder. Based on it we can improve the descriptions and work more on it later.

+++ b/core/modules/file/file.module
@@ -1898,6 +1898,22 @@ function file_permission() {
+      'description' => t('Useful for services like REST.'),
...
+      'description' => t('Useful for services like REST.'),
...
+      'description' => t('Useful for services like REST.'),
...
+      'description' => t('Useful for services like REST.'),

We should explain that these permissions are not used on fields, at least now, on top of the current description? This would probably fix some of the confusion.

clemens.tolboom’s picture

Issue summary: View changes
Status: Needs review » Needs work

This needs at least splitting permissions (#19) into * own file entities and * all file entities.

clemens.tolboom’s picture

Berdir’s picture

Based on a discussion in IRC, what if we try to solve this in a way that doesn't require new permissions?

We already have view access logic for private files now, based on to whom they are attached to. So private files should actually be accessible now. What if we just add a check for that and make all public files accessible by default?

To actually be able to view those files over REST, you still need the rest specific permissions and file_entity can still build on this and add more permissions, just like it already does.

Then we have one of three scenarios solved. For creating new files, I'm not sure. We could implement rest_file_create_access() and allow create access if the user has the rest POST permission. Then we don't need a new permission, just the single one that is automatically created would be enough. But we can't make it specific to REST requests, so it might mess with file_entity.

I will try to ask to ask @WimLeers if we could get a neutral response from the access controller and in that case assume that no opinion + the specific REST permission means creating is allowed? I'm not sure if that would be safe?

For editing, I actually have no problem with saying that is not supported. Drupal core does not provide the ability to update files in the UI either, so it is not a bug that REST integration can't do it either. If you want that, use file_entity. I have no problem with that answer.

queenvictoria’s picture

Apologies if I'm heading in the wrong direction here. I'm just trying to kick along REST PUT support for file entities and it seems like this is a blocker (as it prevents GET at least). I've re-rolled the patch in #15 against 8.0.x and added install default settings (which is the only way I could get the permissions to appear). For an existing site one would need to uninstall the rest module and then reenable hal and rest to see the permissions in the UI.

If we're trying to do this without special permissions for files what would we need to do? I'd like to be doing this the *right* way.

queenvictoria’s picture

I've added the 'file' module to each test otherwise we get a missing entity definition message running tests.

marthinal’s picture

Status: Needs work » Needs review
FileSize
638 bytes

Working on #1927648: Allow creation of file entities from binary data via REST requests and as suggested by @berdir (IRC) we can simply allow to upload files. So, if you have access to the file entity resource then you can upload a file.

If you take a look at #1927648: Allow creation of file entities from binary data via REST requests, I'm validating the file when denormalizing.

Dave Reid’s picture

Issue tags: +D8Media
vivekvpandya’s picture

FileSize
130.31 KB

I applied the patch, enable file resource on REST module and added permission on DELETE to authenticated user and tried to DELETE file with proper authentication it is still not working .
Consider image below :
File note deleted.png

vivekvpandya’s picture

GET works as expected with localhost/drupal-8.0.x/drupal/entity/file/2?_format=hal_json

marthinal’s picture

Assigned: Unassigned » marthinal
Status: Needs review » Needs work

@vivekvpandya thanks for reviewing. Let me take a look at this.

marthinal’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
1.15 KB
1.52 KB

Changing status to Major because we need CRUD for file if we want to attach files to nodes from rest.

From the UI, when you remove a file from a node, you are not deleting the file entity. So basically you remove the entity reference from the node. Later the file entity is removed automatically AFAIK. So probably the current behavior is correct but if we want to remove a file entity from rest we need, at least, to verify that you are an admin user or owner/creator. So maybe we can do something like this.

Patch attached. Missing tests but from the rest client it works as expected.

stefan.r’s picture

  1. +++ b/core/modules/file/src/FileAccessControlHandler.php
    @@ -21,6 +21,23 @@ class FileAccessControlHandler extends EntityAccessControlHandler {
    +    if($operation == 'delete') {
    

    missing space before the opening parenthesis.

  2. +++ b/core/modules/file/src/FileAccessControlHandler.php
    @@ -21,6 +21,23 @@ class FileAccessControlHandler extends EntityAccessControlHandler {
    +      // Only admin users and the file owner can delete the file entity.
    +      if ($account->hasPermission('administer nodes') || $account->id() == $file_uid[0]['target_id']) {
    

    Just for reference, in D7 file_entity the access check is:
    (user_access('delete any ' . $type . ' files', $account) || (is_object($file) && user_access('delete own ' . $type . ' files', $account) && ($account->uid == $file->uid)))

    Do we want to add "delete own files" and "delete any files" permissions?

  3. +++ b/core/modules/file/src/FileAccessControlHandler.php
    @@ -21,6 +21,23 @@ class FileAccessControlHandler extends EntityAccessControlHandler {
    +
    

    extraneous newline

Status: Needs review » Needs work

The last submitted patch, 36: 2310307-36.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
1.19 KB
1.69 KB

1. Done.

2. AFAIK we are trying to avoid create new permissions and thats why we allow create files if you have the file rest resource enabled.(#31)

So this kind of permissions will be covered by file_entity module for the moment. So... I would like the opinion of @berdir here again :)

3. Looks correct to me ...

@stefan.r many thanks for reviewing!

stefan.r’s picture

Issue tags: +Needs tests
+++ b/core/modules/file/src/FileAccessControlHandler.php
@@ -65,4 +83,11 @@ protected function getFileReferences(FileInterface $file) {
+  /**
+   * {@inheritdoc}
+   */
+  protected function checkCreateAccess(AccountInterface $account, array $context, $entity_bundle = NULL) {
+    return AccessResult::allowed();
+  }

This could use a comment explaining why we allow create access for everybody

vivekvpandya’s picture

GET is not working . It give 405 Method not allowed ( though I have all necessary permission and settings for GET ).
DELETE works file.
For DELELE is there any way by which we can allowed to DELETE the file only to uploader of that file ?
And for GET also previously JSON does not include uid for the file .

marthinal’s picture

Issue summary: View changes
FileSize
267.47 KB

I'm trying GET and it works as expected...

Well with the current patch only the creator and the admin can remove a file entity.

Yep I have detected that fid is missing too. Probably we avoid to show some fields. Not sure why and if we need to avoid it in this case.

vivekvpandya’s picture

FileSize
53.13 KB

I have applied the patch to beta 12 version , as 8.0.x branch is not working at all.
I am not able to do GET . Screenshot attached .

Not working

And for the DELETE it allows any administrator and authenticated user to delete the file , and which is true as we can only give those role based permissions form the menu.
Is there any way to give permission or relation like ' owner of the file ' ?
Or you have coded it like only owner of the file is allowed to DELETE it ?

marthinal’s picture

You need to use '_format' instead of 'format'.

Only owners and admin can delete the file. Try with an authenticated(not the owner :) ) and you should receive a 403.

vivekvpandya’s picture

sorry for GET . It was my bad otherwise it is working fine .
one more thing I noticed is that in GET _format=json gives uid as shown below :

"uid":[
{
"target_id": "1"
}
],

and hal_json gives as shown below:

"http://localhost/dr8b12/rest/relation/file/file/uid":[
{
"href": "http://localhost/dr8b12/user/1?_format=hal_json"
}
]

And for DELETE I have uploaded one file with admin account and I am able to delete it with not admin non owner authorized account .

vivekvpandya’s picture

One more interesting thing I noticed is that there are some inline images that get stored in sites/default/files/inline-images/ and are used in article by a user , for these images GET only allows owner of the file to fetch details admin credentials also fails to GET on those. However other authenticated user is able to delete those files . Is it involving some permissions from file module like 'file is in public space' etc ?

marthinal’s picture

@vivekvpandya Ok I see :)

Well let's try to fix then. I was testing manually and looks correct. Anyways missing tests.

vivekvpandya’s picture

@marthinal Does this latest patch solves DELETE related problem ??

marthinal’s picture

@vivekvpandya Yes. I only had time to check DELETE problems not sure about #46.

vivekvpandya’s picture

@marthinal great work , only owner is able to DELETE the file.
But for non owner user is Drupal planning to add some more descriptive error message like "Only owner is allowed to delete " ?

vivekvpandya queued 47: 2310307-47.patch for re-testing.

vivekvpandya’s picture

Status: Needs review » Needs work

The last submitted patch, 52: 2310307-47-rerolled2.patch, failed testing.

vivekvpandya’s picture

vivekvpandya’s picture

@marthinal please upload re-rolled patch .

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.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

This can't go in without test coverage.

Also, the issue title & summary do not at all match the patch.

dawehner’s picture

I also cannot imagine that putting 'administer nodes' into files itself is a good idea in the first place, but rather just a workaround.

marthinal’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs work » Needs review
Issue tags: +neworleans2016
FileSize
2.29 KB
4.29 KB

Adding tests to cover that only the owner can edit/delete the file + status field is not editable. Not sure if it makes sense to test that everybody can create a File entity. For the moment this is the only way to have the possibility to create a File from REST before create a node for example.

The last submitted patch, 59: 2310307-59-only-test-should-fail.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 59: 2310307-59.patch, failed testing.

dawehner’s picture

  1. +++ b/core/modules/file/tests/src/Kernel/AccessTest.php
    @@ -0,0 +1,90 @@
    +class AccessTest extends KernelTestBase {
    +  /**
    

    Needs a newline

  2. +++ b/core/modules/file/tests/src/Kernel/AccessTest.php
    @@ -0,0 +1,90 @@
    +
    +  protected function setUp() {
    

    {@inheritdoc}

  3. +++ b/core/modules/file/tests/src/Kernel/AccessTest.php
    @@ -0,0 +1,90 @@
    +
    +  /**
    +   * Tests that the status field is not editable.
    +   */
    +  public function testStatusFieldIsNotEditable() {
    +    \Drupal::currentUser()->setAccount($this->user1);
    +    $this->assertFalse($this->file->get('status')->access('edit'));
    +  }
    +}
    

    We are missing test coverage for the create case ...

marthinal’s picture

The last submitted patch, 63: 2310307-63-only-test-should-fail.patch, failed testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Thank you @marthinal!

The last submitted patch, 30: get_and_delete_not-2310307-30.patch, failed testing.

The last submitted patch, 29: get_and_delete_not-2310307-29.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/file/src/FileAccessControlHandler.php
@@ -63,4 +75,23 @@ protected function getFileReferences(FileInterface $file) {
+  protected function checkCreateAccess(AccountInterface $account, array $context, $entity_bundle = NULL) {
+    return AccessResult::allowed();
+  }

Is this really the right behaviour? Should we return ::neutral() so that something has to specifically allow? #40 has not been addressed.

tedbow’s picture

Is this really the right behaviour? Should we return ::neutral() so that something has to specifically allow?

Yes nuetral seems right.

Updated tests and added comment

The last submitted patch, 69: file-access-2310307-69_TEST_ONLY.patch, failed testing.

Wim Leers’s picture

+++ b/core/modules/file/src/FileAccessControlHandler.php
@@ -91,7 +91,8 @@ protected function checkFieldAccess($operation, FieldDefinitionInterface $field_
+    // The file entity has no create permission so we are neutral here.

I'd say: The file entity has no "create" permission because by default Drupal core does not allow to create individual file entities. It allows you to create file entities that are referenced from another entity (e.g. an image for a blog post). A contributed module is free to alter this to allow file entities to be create directly.

… or something like that. This is what I understood from reading this issue.

I'd like Berdir to confirm that what I wrote is correct. Or better yet, I'd like Berdir to write this comment :)

Berdir’s picture

The comment makes sense. But it contradicts what #1927648: Allow creation of file entities from binary data via REST requests wants to do, because there we want to do exactly that. Create a file on its own. Which goes back to my comment in #18.

The only way to upload files using REST then is as part of an entity that you are allowed to create (see #2721489-6: REST: top priorities for Drupal 8.2.x), but that is a big direction change for #2721489: REST: top priorities for Drupal 8.2.x.

tedbow’s picture

Ok update the comment based @Wim Leers suggestion #71

Also added a @todo to update the comment when #1927648: Allow creation of file entities from binary data via REST requests is committed.

Also thanks for not noticing/commenting on my 0 bytes TEST_ONLY file in #69 ;)

Status: Needs review » Needs work

The last submitted patch, 73: 2310307-73_TEST_ONLY.patch, failed testing.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

That is a good description of what needs to happen.

@tedbow If you upload the test file first it doesn't fail anylonger.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed e164976 and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed b6861fe on 8.2.x
    Issue #2310307 by marthinal, tedbow, clemens.tolboom, queenvictoria,...

  • alexpott committed e164976 on 8.1.x
    Issue #2310307 by marthinal, tedbow, clemens.tolboom, queenvictoria,...

Status: Fixed » Closed (fixed)

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

webchick’s picture

Issue tags: +8.2.0 release notes

This sounds like it allows files to be managed via the REST API and if so, that seems worthy of calling out in the release notes.

Berdir’s picture

Not really, it's just one part of that problem space and in fact now kind of prevents #1927648: Allow creation of file entities from binary data via REST requests from working as previously planned. So not worth mentioning for now I'd say.

joelpittet’s picture

Old issue but this seems strange that there is nobody allowed to delete a file other than the original owner(what if they are no longer at the org?), not even an admin user at present.

@stefan.r's suggestion in #37.2 should be given a bit more consideration?

An issue has been created #2949017: There is no way to delete file entities of other users to address this.