After upgrading to 7.x-2.0-alpha1, private files (images) cannot be downloaded anymore, even with appropriate permissions (download any images) set. I think file_entity_file_download should call file_entity_access() with $op = 'download'.

And also: should file_entity_file_download() rather not return -1? The image module still be able to grant access do download images based on node access, right?

CommentFileSizeAuthor
#5 2064129-5-file-download-op-download.patch290 bytesdevin carlson
#1 2064129-file-download-op-download.patch429 bytesdave reid

Comments

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Active » Needs review
StatusFileSize
new 2064129-file-download-op-download.patch429 bytes

Let's try the following patch. Does this resolve the issue for you?

Status: Needs review » Needs work

The last submitted patch, 2064129-file-download-op-download.patch, failed testing.

morenstrat’s picture

morenstrat
he/him
Primary language German
Location Bochum, Germany
commented

No, it does not resolve the issue because I additionally have to grant the "download any [file type] files" permission. I think the other problem is that file_entity_file_download() returns -1 and thereby does not allow other modules to grant file download access (e.g. the image module which should be able to grant download access of images based on node access).

However, I'm not really familiar with the file_entity code and I don't know if not returning anything will break the file_entity access logic.

Another problem I just encountered is that my EntityFieldQueries also break with this version of file_entity. Nodes with files in private fields are not returned by EntityFieldQueries anymore.

drupalnuts’s picture

drupalnuts commented

That seems correct.

https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func...

When it returns -1 it will prevent other hooks from running.

devin carlson’s picture

devin carlson commented
Version: 7.x-2.0-alpha1 » 7.x-2.x-dev
Status: Needs work » Needs review
StatusFileSize
new 2064129-5-file-download-op-download.patch290 bytes

A patch to make file_entity_file_download() return NULL if it doesn't find any reason to approve/deny access.

Also, since core's idea of "downloading" a file is the same as File entity's ability to view a file, I think that the "view" op is correct. Talking to some other file/media management folks, it sounds like we could just remove the "download *" permissions in the future.

morenstrat’s picture

morenstrat
he/him
Primary language German
Location Bochum, Germany
commented
Status: Needs review » Reviewed & tested by the community

I can confirm that returning NULL or not returning anything solved the problem for me.

As for the op, I don't know. I thought that "downloading" a file means to get the file contents and "viewing" a file means to see file's field data...

devin carlson’s picture

devin carlson commented
Status: Reviewed & tested by the community » Fixed

Committed #5 to File entity 7.x-2.x.

@dunix you're correct about view vs. download but it can get confusing when a user has permission to "view" a file (by going to file/fid/view), lets say an image, but when they go to the page they get a missing image because they don't have "download" access.

I personally prefer unifying view + download but that's for another issue.

Status: Fixed » Closed (fixed)

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

silkogelman’s picture

silkogelman commented
Issue summary: View changes

FYI: I had a similar file permission problem blocking access to private files even when they had the right view permissions.
(private files, file access based on Organic Group membership with og_access and og_field_access.)

upgrading from 7.x-2.0-beta1 to 7.x-2.x-dev (2014-Nov-14) seemed to fix the problem.

Update: Related issue #2351691: Access denied when downloading private files - decide if file_entity_file_download() should deny access by default or not