Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.The function checks view access for the view, if that is not granted, then access is explicitly denied.
file_file_download() already checks and supports download access operation. If you implement a custom file access check, it only works if you return access allowed for both view *and* download.
download vs view is weird anyway, but having to support both is even weirder :)
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | 2799897-n13.patch | 887 bytes | hanoii |
| #7 | file_entitiy-hook_file_download-2799897-7.patch | 729 bytes | dwkitchen |
| #2 | remove-2799897-2.patch | 821 bytes | Berdir |
Comments
Comment #2
BerdirComment #3
BerdirComment #6
manarth CreditAttribution: manarth as a volunteer commentedRelated in 7.x-2.x: #2351691: Access denied when downloading private files - decide if file_entity_file_download() should deny access by default or not
The current behavior in 7.x-2.x is to return NULL ("no opinion"), rather than -1 ("deny access"), so the behaviour of the 8.x-2.x branch is different to the 7.x-2.x branch.
The change in 7.x-2.x occurred in commit 5b5e5a5.
It might be worth initially switching the 8.x behavior to simply match the 7.x branch, then taking a longer view on whether a more complete sweeping change should be applied to both branches. The comments in #2351691: Access denied when downloading private files - decide if file_entity_file_download() should deny access by default or not suggest that there are quite a few differing opinions on the best way forward.
Comment #7
dwkitchen CreditAttribution: dwkitchen at Johnson & Johnson commentedWhat I think we need to remove here is the access check; this is managed by FileEntityAccessControlHandler. We can still add the file_entitiy information to the headers though.
On an aside FileEntityAccessControlHandler checks access based on 'download' permission and this uses 'view' causing a conflict.
Comment #8
BerdirI don't think we should remove it completely, that's definitely not secure.
file_file_download() uses 'view', so possibly we should use the same. I honestly don't know what the difference between those two operations is. It could be view file/ID vs. downloading the file but the problem is that core already does it in the wrong way and we can't easily change that.
Comment #9
dwkitchen CreditAttribution: dwkitchen at Johnson & Johnson commentedHi @Berdir
I created another patch with a link to this one but forgot to back link.
https://www.drupal.org/node/2892628
I'll have a look a file_file_download()
Comment #10
joseph.olstadrequeue
Comment #11
Wim LeersThat's not true since #2078473: Use entity access API for checking access to private files, which was committed July 24, 2014:
In fact, zero mentions of the string
'view'infile.module.EDIT: I'm talking about D8 of course. In D7, it is indeed using
'view', because before #2078473, D8 was too.Comment #12
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedSeems like a bug to me...
Note that the related issue (#2892628: Improve file download access control) is currently marked as a bug.
Comment #13
hanoiiBased on #11, #12 and after reviewing this and #2892628: Improve file download access control I do think that we are better off removing the hook and let
file_file_downloadhandle that part, this module's access control handler will take precedence.The headers are not necessary, they are also added by that hook.
Re-rolling #2 mostly so that it takes more relevance in the discussion.
Also uploaded an alternative to the mentioned issue: #2892628-6: Improve file download access control.