Files from private stream cannot be downloaded

How would we check if the file is accessible? Are there any permissions assigned directly to the file itself that we could check?

Steps:

1. Installed File Entity (latest from 7.x-2.x-dev).
2. Role authenticated user receive view own files, delete own files, view own private files, create files, edit own files permissions.
3. At admin/config/media/file-system set the Default download method to Private local files served by Drupal.
4. At admin/structure/file-types/manage/image/edit set the Allowed streams only to Private files.
5. As UID == 1 add an image (file/add).
6. Go to view (file/[file_id]). You can see that the image is not accessible.

Sorry. Accidentally moved the status.

Right now I only added a test that should fail, The test demonstrates that a user having view own private files permission is not able to download his own file.

Here's a patch with an improved test case.

Fixed some comments.

Small improvements. This is (really) the final one :)

i like this approach.
only thing that remains is to remove all references from file_entity_file_entity_access to $grants, since grants wont be used now

Looking a little bit at permission descriptions I found:

'view own private files' => array(
      'title' => t('View own private file details'),
      'description' => t('For viewing file details, not for downloading files.'),
    ),

This looks like we want to have separate permissions for view file details (aka file/%file) and file download (the file itself).

Well, this I think it's not a good idea. We're dealing here with file entity, not other entities that are bundling files as field or property. IMO the the whole entity (file + meta + fields) is a single thing: the File Entity. If we want different access level for file and fields, just bundle a file into a node and use an appropriate module to fine tune permissions into the bundle. That's why I won't separate access for file itself from file details (meta+fields).

Here's a patch according to #12.

Fixing some typos.

+++ b/tests/file_entity.testundefined
@@ -418,4 +418,57 @@ class FileEntityAccessTestCase extends FileEntityTestHelper {
+    $cases = array(
+      "Owner not granted with 'view own private files' cannot download his files." => array('perms' => array(), 'expect' => 403, 'owner' => TRUE),
+      'Owner can download his files.' => array('perms' => array('view own private files'), 'expect' => 200, 'owner' => TRUE),
+      'Anonymous cannot download private files.' => array('perms' => NULL, 'expect' => 403),
+      "Regular user cannot download other's private files." => array('perms' => array(), 'expect' => 403),
+      "User able to see everyone's public file details cannot download private files." => array('perms' => array('view files'), 'expect' => 403),
+      "Superuser can download everyone's files." => array('perms' => array('bypass file access'), 'expect' => 200),
+    );

This is hard to read.
Why dont you add the message in the array as well with a key message and format the array nicely?

+++ b/tests/file_entity.testundefined
@@ -418,4 +418,57 @@ class FileEntityAccessTestCase extends FileEntityTestHelper {
+      $this->drupalGet($url);
+      $this->assertResponse($case['expect'], $message . $message_file_info);

doesnt this trigger two notices/warnings since if owner is empty both variables ($url and $message_file_info) are not initialized?

Everything else looks good

This is hard to read.
Why dont you add the message in the array as well with a key message and format the array nicely?

Well, the only unique thing that can act as a key is the message itself. Adding a new key is useless but we can make something in between by having the array as a vector array. Example:

$cases = array(
  array(
    'message' => "Owner not granted with 'view own private files' cannot download his files.",
    'perms' => array(),
    'expect' => 403,
    'owner' => TRUE
  ),
  array(
    'message' => ...
    ...
  ),
  ...
);

doesnt this trigger two notices/warnings since if owner is empty both variables ($url and $message_file_info) are not initialized?

No. That's how it was designed. The first test case will always create $url and $message_file_info. Same the second. The others will reuse $url and $message_file_info already created by the file owners. The meaning is: Non-owner access is tested against files owned by somebody else.

Reworking according to #17...

Here's only the test which should fail.

Here's the patch with test.

Fix a small wrapping issue in phpdoc.

#21: file_entity-private-file-download-1420812-21.patch queued for re-testing.

#21: file_entity-private-file-download-1420812-21.patch queued for re-testing.

Well, the only unique thing that can act as a key is the message itself. Adding a new key is useless but we can make something in between by having the array as a vector array

Yeap, thats what i actually meant..sorry for typing it the wrong way, but we got in the same page eventually, much better now

Seems good to me.
Final points:

No. That's how it was designed. The first test case will always create $url and $message_file_info. Same the second. The others will reuse $url and $message_file_info already created by the file owners. The meaning is: Non-owner access is tested against files owned by somebody else.

Yeah well, this is not documented in getPrivateDownloadAccessCases(). So if someone in the future adds a case in the top without owner it will trigger the error.
Instead you should just initialize those variables before foreach and make sure you dont get any problems no matter what

Well, this I think it's not a good idea. We're dealing here with file entity, not other entities that are bundling files as field or property. IMO the the whole entity (file + meta + fields) is a single thing: the File Entity. If we want different access level for file and fields, just bundle a file into a node and use an appropriate module to fine tune permissions into the bundle. That's why I won't separate access for file itself from file details (meta+fields).

I have to agree here. Care removing , not for downloading files from t('For viewing file details, not for downloading files.'),

Besides those minors everything else looks good.

No. That's how it was designed. The first test case will always create $url and $message_file_info. Same the second. The others will reuse $url and $message_file_info already created by the file owners. The meaning is: Non-owner access is tested against files owned by somebody else.

Yeah well, this is not documented in getPrivateDownloadAccessCases(). So if someone in the future adds a case in the top without owner it will trigger the error. Instead you should just initialize those variables before foreach and make sure you dont get any problems no matter what.

My point of view:

• There's nothing to initialize here. Non-owners should use previous created files.
• Developers that are creating tests should know what they are doing, better than other developers. They will see the notices and they will fix.
• Test cases are just cases. They are special, specific, custom cases and should be treated so (like manual testing).

Anyway, I will sort that array on owner assuring that "owner" cases will be always on top. IMO that is superfluous but I will fix it in that way so the order will have no relevance because every time owners will create files (and $url, $message_file_info already).

Well, this I think it's not a good idea. We're dealing here with file entity, not other entities that are bundling files as field or property. IMO the the whole entity (file + meta + fields) is a single thing: the File Entity. If we want different access level for file and fields, just bundle a file into a node and use an appropriate module to fine tune permissions into the bundle. That's why I won't separate access for file itself from file details (meta+fields).

I have to agree here. Care removing , not for downloading files from t('For viewing file details, not for downloading files.'),

I will not touch this in this issue. The whole hook_permission() implementation needs some attention because:

• Permissions description lacks
• Per-filetype permissions need to be implemented (like for nodes).

A separate ticket should be opened for hook_permission() refactoring.

Here is a re-roll with some changes.

• Since a hook_file_download is being added I added support for the download param $op in file_access.
• Cleaned up a strict error in file_entity_download.

I noticed in #13 you say that we shouldn't separate download vs. view. So I guess I do that in the patch I posted. I completely understand where you are coming from and I'm trying to come up with a use case where the separation makes a difference. Even if we can't come up with a solid use case what's the downside of separating the two?

Tried to apply #29 against 7.x-dev, but faild. Updated it manually and created a new patch.

Updated patch.

Changes:

• Left out "cleanup" changes.
• Minor language changes to various comments.
• Used file_uri_to_object() over file_load_multiple().
• Returned file_get_content_headers() instead of a "static" array of headers.
• Used the "view" operation instead of the "download" operation for checking access in file_entity_file_download(). I believe that "download" is reserved for file/%fid/download.

I'm pretty sure view is if you went to file/fid and download is if you went to system/files/my_file_path/my_file_name.jpg

system/files/my_file_path/my_file_name.jpg

i guess thats indeed download

I believe that it's actually a bit more tricky than that. Visiting system/files/my_file_path/my_file_name.jpg with the appropriate permissions would open up the image in your browser while visiting the equivalent file/%fid/download would actually trigger the file to be downloaded to your machine.

This is the behavior provided by Drupal core through file_get_content_headers() but we could always force a file to be downloaded by returning the same headers used by file/%fid/download.

Also, I'm not sure about permissions. If we use file_entity_access('download') in hook_file_download() users must have the "view own private files" permission in order to view a private file but they could actually "download" the file if they went to the appropriate "system/files/*" path and had the more general "download own files".

Marked #1919722: Files cannot be accessed by admin with file system set to private as a duplicate.

#34: 1420812-file_download-34.patch queued for re-testing.

Updated patch.

I can confirm that this patch fixes the issue I have with private content not working with the current Media 2.x module. Thanks for the hard work, this has been a nasty critical bug with private files that has persisted for far too long. I'm tempted to mark it as RTBC but not sure I've tested for any other side effects. So I'll run with this patch for a week or so and then post back if all looks good. Hopefully other people can also test.

This looks great to me. I don't see any issues with it.

Committed to 7.x-2.x.

Marked #1936064: Upload using file/add uploads file but does not display it / Access denied / Private upload as a duplicate.

now that this is done, what should happen to http://drupal.org/node/1414990

Regarding the hook_file_entity_access() implementation, the committed patch is not quite correct. The function returns:

return !empty($grants) ? FILE_ENTITY_ACCESS_ALLOW : FILE_ENTITY_ACCESS_IGNORE;

while there's no $grants defined anywhere. So we need to simply return FILE_ENTITY_ACCESS_IGNORE there:

return FILE_ENTITY_ACCESS_IGNORE;

Also for code readability I like more the approach from #21 that wraps long lines and prevent code duplication. Using

$is_own_file = is_object($file) && ($account->uid == $file->uid);
if ($op == 'update') {
  if (user_access('edit any files', $account) || ($is_own_file && user_access('edit own files', $account))) {
    return FILE_ENTITY_ACCESS_ALLOW;
  }
}

instead of the long, unreadable and duplicated:

if (user_access('edit any files', $account) || (is_object($file) && user_access('edit own files', $account) && ($account->uid == $file->uid))) {

is preferred (IMO).

Anyway the first issue (unused $grants) is the reason I switched back to needs work.

A patch to simply return FILE_ENTITY_ACCESS_IGNORE.

Committed to 7.x-2.x.