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:
- 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.
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.
Comment | File | Size | Author |
---|---|---|---|
#73 | 2310307-73_TEST_ONLY.patch | 2.69 KB | tedbow |
#73 | interdiff-2310307-69-73.txt | 1.07 KB | tedbow |
#73 | file-access-2310307-73.patch | 5.16 KB | tedbow |
#69 | interdiff-2310307-63-69.txt | 1.39 KB | tedbow |
#69 | file-access-2310307-69.patch | 4.77 KB | tedbow |
Comments
Comment #1
clemens.tolboomGood 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.
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe 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.
Comment #3
clemens.tolboom@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?
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commented@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.
Comment #5
ParisLiakos CreditAttribution: ParisLiakos commentedComment #6
juampynr CreditAttribution: juampynr commentedI 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 headerAccept: application/hal+json
: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.
Comment #7
clemens.tolboomThe 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
Comment #8
juampynr CreditAttribution: juampynr commentedI 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.
Comment #9
clemens.tolboom@juampy guess #6 needs some condensation ;)
Questions to be answered
Comment #10
clemens.tolboomAttached patch is tiny step forward. How do I declare permissions for the file entity?
Comment #11
juampynr CreditAttribution: juampynr commentedUsing 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.
Comment #12
clemens.tolboom@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 availableThis issue needs permissions added.
Comment #13
vivekvpandya CreditAttribution: vivekvpandya commentedI am not able to apply this patch. I got following error
error: core/modules/file/src/FileAccessControlHandler.php: No such file or directory
Comment #14
clemens.tolboom@vivekvpandya I can apply the patch #10
The file exists
#2154435: Rename EntityAccessController to EntityAccessControlHandler
Comment #15
juampynr CreditAttribution: juampynr commentedSame here.
Comment #16
clemens.tolboomAttached 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
Comment #17
clemens.tolboomChanged title to reflect the CRUD ops on File entity
Comment #18
BerdirThe 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.
Comment #19
clemens.tolboom@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
Comment #20
Berdirfile 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?
Comment #21
Damien Tournoud CreditAttribution: Damien Tournoud commentedIt seems like the alternative is between:
file
a first-class entity in Drupal 8, that includes REST support, proper access control for queries, proper rendering and form modes support;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:
file_entity
what we want for core?Comment #22
BerdirYeah, 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.
Comment #23
juampynr CreditAttribution: juampynr commentedBased on the following by @berdir:
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?
Comment #24
clemens.tolboom@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.
Comment #25
dawehneryes, file entity is somehow in a sad state.
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.
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.
Comment #26
clemens.tolboomThis needs at least splitting permissions (#19) into
* own file entities
and* all file entities
.Comment #27
clemens.tolboomComment #28
BerdirBased 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.
Comment #29
queenvictoria CreditAttribution: queenvictoria commentedApologies 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.
Comment #30
queenvictoria CreditAttribution: queenvictoria commentedI've added the 'file' module to each test otherwise we get a missing entity definition message running tests.
Comment #31
marthinal CreditAttribution: marthinal commentedWorking 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.
Comment #32
Dave ReidComment #33
vivekvpandya CreditAttribution: vivekvpandya as a volunteer commentedI 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 :
Comment #34
vivekvpandya CreditAttribution: vivekvpandya as a volunteer commentedGET works as expected with localhost/drupal-8.0.x/drupal/entity/file/2?_format=hal_json
Comment #35
marthinal CreditAttribution: marthinal commented@vivekvpandya thanks for reviewing. Let me take a look at this.
Comment #36
marthinal CreditAttribution: marthinal commentedChanging 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.
Comment #37
stefan.r CreditAttribution: stefan.r commentedmissing space before the opening parenthesis.
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?
extraneous newline
Comment #39
marthinal CreditAttribution: marthinal commented1. 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!
Comment #40
stefan.r CreditAttribution: stefan.r commentedThis could use a comment explaining why we allow create access for everybody
Comment #41
vivekvpandya CreditAttribution: vivekvpandya as a volunteer commentedGET 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 .
Comment #42
marthinal CreditAttribution: marthinal commentedI'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.
Comment #43
vivekvpandya CreditAttribution: vivekvpandya as a volunteer commentedI 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 .
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 ?
Comment #44
marthinal CreditAttribution: marthinal commentedYou 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.
Comment #45
vivekvpandya CreditAttribution: vivekvpandya as a volunteer commentedsorry 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 :
and hal_json gives as shown below:
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 .
Comment #46
vivekvpandya CreditAttribution: vivekvpandya as a volunteer commentedOne 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 ?Comment #47
marthinal CreditAttribution: marthinal commented@vivekvpandya Ok I see :)
Well let's try to fix then. I was testing manually and looks correct. Anyways missing tests.
Comment #48
vivekvpandya CreditAttribution: vivekvpandya as a volunteer commented@marthinal Does this latest patch solves DELETE related problem ??
Comment #49
marthinal CreditAttribution: marthinal commented@vivekvpandya Yes. I only had time to check DELETE problems not sure about #46.
Comment #50
vivekvpandya CreditAttribution: vivekvpandya as a volunteer commented@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 " ?
Comment #52
vivekvpandya CreditAttribution: vivekvpandya as a volunteer commentedRe-rolling...
Comment #54
vivekvpandya CreditAttribution: vivekvpandya as a volunteer commentedComment #55
vivekvpandya CreditAttribution: vivekvpandya as a volunteer commented@marthinal please upload re-rolled patch .
Comment #57
Wim LeersThis can't go in without test coverage.
Also, the issue title & summary do not at all match the patch.
Comment #58
dawehnerI also cannot imagine that putting 'administer nodes' into files itself is a good idea in the first place, but rather just a workaround.
Comment #59
marthinal CreditAttribution: marthinal commentedAdding 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.
Comment #62
dawehnerNeeds a newline
{@inheritdoc}
We are missing test coverage for the create case ...
Comment #63
marthinal CreditAttribution: marthinal commented1) Done
2) Done
3) Done
Comment #65
dawehnerThank you @marthinal!
Comment #68
alexpottIs this really the right behaviour? Should we return ::neutral() so that something has to specifically allow? #40 has not been addressed.
Comment #69
tedbowYes nuetral seems right.
Updated tests and added comment
Comment #71
Wim LeersI'd say:
… 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 :)
Comment #72
BerdirThe 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.
Comment #73
tedbowOk 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 ;)
Comment #75
dawehnerThat is a good description of what needs to happen.
@tedbow If you upload the test file first it doesn't fail anylonger.
Comment #76
alexpottCommitted e164976 and pushed to 8.1.x and 8.2.x. Thanks!
Comment #80
webchickThis 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.
Comment #81
BerdirNot 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.
Comment #82
joelpittetOld 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.