Access denied when downloading private files - decide if file_entity_file_download() should deny access by default or not

Thank you for this patch, which just bailed me out of hours of heartache.

Thanks so much! This issue caused me major problems today when I upgraded Commons to 3.18. We use FileDepot and it completely prevented access to hundreds of non-admin users to any file on our system. The patch took care of it. Marking as Major since this is a huge issue on a site like ours. Marking RTBC as the patch worked for me and apparently @wmfinck.

I wonder if it was changed to fix some other issue though?!

Thanks again @dunix.

Committed to http://drupalcode.org/project/file_entity.git/commit/5b5e5a5.

This patch reverts #2164527: Cannot deny access to private files. My understanding is that if you want to server private files, then you should grant View private files permission to the roles in question.

file_file_download always grants access and returns the headers for files linked from file fields. This is expected behavior and not caused by some strange core/contrib module.

Well, let's rephrase that. file_file_download first determines if the file to be downloaded is controlled by the file module, i.e. is referenced from a file field. If it is not, then it returns NULL. If it is it continues with checking permissions and if the user has access to the field, then it allows the download by returning the headers. This is what I meant in #12, sorry for the sloppy comment.

The important thing to note is the sentence in hook_file_download documentation for the return value:

If the file is not controlled by the current module, the return value should be NULL.

It is expected behavior that some module claims ownership of a file download and that it returns headers from its hook_file_download implementation if the user has access according to the permissions respected by that module. So if the core file module decides that according to field access a file should be downloadable by a user, then the file entity module only can prevent that by returning -1 from its hook_file_download.

Thus hook_file_download needs to return -1 for private files when users are lacking the View private files permission. Returning NULL in that case will not have any effect.

Thanks for this!

It solved this usecase for me:
private file access based on Organic Group membership with og_access and og_field_access.

• Private file permissions where working correctly without file_entity module enabled.
• file_entity 7.x-2.0-beta1 did not give the users access to private files when they where supposed to have access.
• file_entity 7.x-2.x-dev (2014-Nov-14) seems to fix that problem. Organic Group members can now access the private files that they are supposed to be able to download.

If the View private files permission cannot be implemented cleanly in file entity, then it should be removed completely. Flip-flopping the return value in every release is not only a nuisance for site owners but also may have security implications for some sites.

Hello,

First of all thank you for the patch it fixes the issue I was having with Webform Protected downloads.

Before the patch I had to implement my own solution by having to integrate the Webform Protected Downloads hook_file_download logic into my own module that can determine which module to delegate it to.

Also this required the use of the view private files (so that the hook is triggered).

I agree with dunix on in the sense that the way File Entity is returning currently just makes it impossible to play nice with any module that deals with granting access to private files to users (say anonymous), as we are seeing with the current thread (Organic Groups, FileDepot, Webform Protected Downloads).

Hi @all,

why not simple implement hook_file_entity_access() instead of hook_file_download from now? If you allow the access to the file entiy u got access to the private file also.
So the expected behavior is the same as the entity view.

But one thing is irritate me... why check for "view" in file_entity_file_download and not check for "download" ?

file_entity_file_download:

  // Allow the user to download the file if they have appropriate permissions.
  if (file_entity_access('view', $file)) {
    return file_get_content_headers($file);
  }

Here a example how i implemented it currently (very specific but could be help someone):

function evt_document_file_entity_access($op, $file, $account) {
  if ($op == 'view' && !empty($file->fid)) {
    $sw = file_stream_wrapper_get_instance_by_uri($file->uri);
    if ($sw instanceof DrupalPrivateStreamWrapper) {
      $used_by = file_usage_list($file);
      foreach($used_by['file'] as $entity_type => $entity_ids) {
        $entites = entity_load($entity_type, array_keys($entity_ids));
        foreach($entites as $entity) {
          if ($entity->type = "evt_downloads" && entity_access('view', $entity_type, $entity, $account)) {
            return FILE_ENTITY_ACCESS_ALLOW;
          }
        }
      }
    }
  }

  return FILE_ENTITY_ACCESS_IGNORE;
}

I just ran some updates on my site two weeks ago and lost access to my purchased files. even as user 1. any idea if the patch above will resolve the current issue? I've had file downloads working fine since Sept until the recent updates.

+1 for #17

It would be good to get some stability in file permissions.

It appears this small patch was rolled into DEV at some point, but we are now getting Access Denied errors on all private files again even as User 1 and with all "View private files" permissions set correctly. It has been awhile but if I recall correctly:

a. We originally applied this patch and it fixed our issue

b. We updated module to most recent DEV (which appears to include said patch) but we are now back to access denied on all private file links

Not sure where things stand or if anybody else is seeing this but thought I'l post a report -- if/when we fix I will update.

This fixes it for me.

I also tried the latest DEV version which seems to have this committed and found no issues with accessing private files.

@Blackice2999 - thank you for posting your code at #19

I implemented this code but hit the following error when uploading files using the media module: "Upload a new file field is required."

The problem is described here: https://www.drupal.org/node/2308737

The File module assumes that files without a public:// schema need to be checked for download permission, which leads to file_entity_access() being called unnecessarily on the temporary file that's been uploaded as part of the media upload workflow.

As a result, I needed to amend the code as follows.

/**
 * Implements hook_file_entity_access().
 *
 * Fixes a problem with file_entity access described in:
 * https://www.drupal.org/node/2351691
 */
function myproject_privatize_file_entity_access($op, $file, $account) {
  if (user_is_logged_in()) {
    if ($op == 'view' && !empty($file->fid)) {
      $sw = file_stream_wrapper_get_instance_by_uri($file->uri);
      if ($sw instanceof DrupalPrivateStreamWrapper) {
        if ($file->status == 1) {
          // $used_by is the list of entities using this file entity.
          $used_by = file_usage_list($file);
          foreach ($used_by['file'] as $entity_type => $entity_ids) {
            $entities = entity_load($entity_type, array_keys($entity_ids));
            foreach ($entities as $entity) {
              if (entity_access('view', $entity_type, $entity, $account)) {
                return FILE_ENTITY_ACCESS_ALLOW;
              }
            }
          }
        }
        else {
          // Special case when file->status == 0.
          // Allow access, otherwise we get the error when uploading a file
          // using the media module file browser:
          // "Upload a new file field is required."
          // See: https://www.drupal.org/node/2308737
          // The File module assumes that files without a public:// schema need
          // to be checked for download permission, which leads to
          // file_entity_access() being called unnecessarily on the temporary
          // file that's been uploaded as part of the media upload workflow.
          return FILE_ENTITY_ACCESS_ALLOW;
        }
      }
    }
  }
  return FILE_ENTITY_ACCESS_IGNORE;
}

So, when I attempt to apply this patch is states it already has been. Is this issue closed and the patch applied already?

I'm still having issues with preview images not showing up from my private file system but I don't know if this module is the culprit or if it's Drupal 7.41.

@deanflory - the patch at #1 was indeed committed to on 2014-10-29 (see #7)
http://cgit.drupalcode.org/file_entity/commit/?id=5b5e5a5

The rest of the discussion in this thread is about subsequent permissions issues that people encountered, even with the change at #1 in place.

"I'm still having issues with preview images not showing up from my private file system but I don't know if this module is the culprit or if it's Drupal 7.41"

It may be that you need do something to implement hook_file_entity_access() to get around this.

I can see both arguments here personally... that said, the patch was committed to Drupal 7 a long time ago and it is probably a little late to revert it again. Right now, the Drupal 7 and Drupal 8 versions of the module actually have different behaviors, so maybe should discuss here what is the correct behavior going forward and whether this should be changed in Drupal 8 too or not.