[D7] Private file download returns access denied, when file attached to node revision other than current

I spotted this issue using D7 to create a controlled documents system for an intranet.
The goal was to have users upload a file as the current revision of a document. When this document is changed in the future, a new revision is uploaded. The user should then have access to the latest revision by default, however users with correct permissions should also be able to view older revisions if required.
Our only solution was to move the whole system out of drupal nodes and code a custom module to handle revision control of document copies through an external database.
Introducing this kind of flexibility into the core nodes and revisions system would be a useful upgrade.

Has there been any progress on this?

I'd love to be able to download a private file attached to a revision. This seems like an obvious use case - why wouldn't you still want access to the files attached to revisions?

Any hacks / patches / suggestions?

Besides the access denied on downloads, if you use the private file field to upload and display images the images wont render either.
Any updates on this?

Setting Upload destination of File field to private causes Access Denied to User 1.

Changing File System ->Default download method has not effect.

Looking forward disperately for any progress. Its D7.17 now

Any update?
If I revert to a revision, I get access to the file but not as a revision.

I don't see any change for 'Permission denied' upon committed changes for http://drupal.org/node/992674 instead of that what I found while debugging with FIELD_LOAD_CURRENT it returns empty $reference not so in case of FIELD_LOAD_REVISION which atleast return reference to revision(s). Due to which it enters the following condition at line 150 (file.module|file_file_download())

   if (empty($references) && ($file->status == FILE_STATUS_PERMANENT || $file->uid != $user->uid)) {
      return;
  }

Still with FIELD_LOAD_REVISION skipping this condition when we proceed further at Line 162-217 the three for loops check sequentially all references of the file and on basis of that it set access to that file by setting $denied TRUE/FALSE. Currently it is failing at Line 188 because of returning empty entity by entity_load() function. Likewise there are series of checkpoint which sets $denied TRUE which leads to current issue. On the otherhand by manually overriding $denied to FALSE temporarily fixes the problem. I think the possible spot for is in between Line 162-217 for D7.x

Any progress on this, or a working patch?

I can confirm this problem still exists and it's a clear and heavy bug. It makes private files in revisions inaccessible.
This is a clear disfunction in Drupal 7 Core and it would be great to have the eyes of a core developer on it. How can we proceed?

I also tried to debug this a bit and there seem to be several hard locical problems:
1. FIELD_LOAD_REVISION should be used to check all revisions instead of FIELD_LOAD_CURRENT in line 143:

  // Find out which (if any) fields of this type contain the file.
  $references = file_get_file_references($file, NULL, FIELD_LOAD_REVISION, $field_type);

2. In the three foreach loops the entity_load man not use the $id from the foreach key, but must use the nid of the $reference, because the results have a different format based on (1.):

  // Try to load $entity and $field.
  $entity = entity_load($entity_type, array($reference->nid));

3. The next thing is that the inner foreach loop only runs ONE TIME currently, because the "break;" in the following condition breaks it:

// Check that $entity, $field and $field_item were loaded successfully
        // and check if access to that field is not disallowed. If any of these
        // checks fail, stop checking access for this reference.
        if (empty($entity) || empty($field) || empty($field_item) || !field_access('view', $field, $entity_type, $entity)) {
          $denied = TRUE;
          break;
        }

Instead a "continue" might help, but it also requires to think about the whole logic. The reason behind this is, that we have to check EVERY result in the $type_references array and not only the first (because we have to check each revision)!

At this point I stopped trying because the change is quite heavy and the uri of the file I tried to check was still not part of the checked array... lots of problems and it clearly points out that nobody yet thought about revisions in this part of code, as it seems. We need help!

Due to the priority (https://www.drupal.org/core/issue-priority) levels I will now set this major, please correct me if I'm wrong.

Having same issue. Code will not progress past line 150 in file.module.
Both the user id check and the file-status == FILE_STATUS_PERMANENT seem to return true in my instance.

Quickly going throug hthe code, not sure why this check is here as the field_access and file_download_access checks later in the code would be sufficient? I'm not clear on all the use cases here so could easily be mistaken.

Update: I still have some hacks to unravel, but it appears my issue was caused by File Entity https://www.drupal.org/node/2351691

using the more common tag, so the duplicate less common one can be deleted and wont confuse people

I'm also experiencing this issue.
I'll look into a solution soon.

Easiest seems to be to use FIELD_LOAD_REVISION and entity_revision_load. entity_revision_load isn't part of core though, so we can't use that.

I can confirm that the issue still persists in 7.42!

That's really sad. Based on #13 we should find out if this is a Drupal Core or File Entity issue. We should get this basic issue fixed.

@Anybody, this is a problem of the file module.
2 things should be fixed:

$references = file_get_file_references($file, NULL, FIELD_LOAD_CURRENT, $field_type);

Should be:

$references = file_get_file_references($file, NULL, FIELD_LOAD_REVISION, $field_type);

foreach ($references as $field_name => $field_references) {
  foreach ($field_references as $entity_type => $type_references) {
    foreach ($type_references as $id => $reference) {
      // Try to load $entity and $field.
      $entity = entity_load($entity_type, array($id));
    }
  }
}

Should be:

foreach ($references as $field_name => $field_references) {
  foreach ($field_references as $entity_type => $type_references) {
    foreach ($type_references as $id => $reference) {
      // Try to load $entity and $field.
      $entity = entity_revision_load($entity_type, array($id));
    }
  }
}

However, core does not have/support entity_revision_load, so it should probably be added to core.
I probably missed some minor things to pass the revision id from file_get_file_references, but this is the major part of the fix.

Thanks jeroen.b for pointing this out. How can we contact a core maintainer to have a look at this? I think this can hardly be decided by community?

congratulations to us all, today is the 4th birthday of the bug reported here!!! :-)

an additional observation: the problem does not 'only' affect file fields but in addition also image fields, i.e. images attached to nodes and included e.g. into a ckeditor textarea only show for the current version but not for older ones due to the access denied issue. reverting to an earlier version gives access again. however, this 'process' completely messes up the revision history especially if users should not be able to delete revisions say out of regulatory compliance reasons :-/

@anybody: i'm wondering too how mere mortals like us can summon the gods of core maintenance? after 4 years it is probably safe to assume that at least this issue is not monitored too closely from above.

happy birthday!

Happy birthday!
I'd be happy do work on this issue when some core maintainer gives me some direction in the desired approach.
Might be best to contact them on IRC?

There definitely needs to be configurable permissions for this because you generally don't want anonymous users accessing private files attached to old revisions. Usually only content editors or admins will want to be seeing those.

As an example, you don't want old revisions of legal documents to be publicly available.

@rooby, isn't that already the case?
file.module already checks for field_access, so you can only view the file when you have view access on the revision of that node, which is already defined in permissions.

Also:
the node module contains:

/**
 * Implements hook_file_download_access().
 */
function node_file_download_access($field, $entity_type, $entity) {
  if ($entity_type == 'node') {
    return node_access('view', $entity);
  }
}

Ah true. I think I initially misunderstood the purpose of this issue a little.

Duplicate of #1452100: Private file download returns access denied, when file attached to revision other than current. That issue is assigned to Drupal 8. I think it's still an issue there.

This one can stay open for D7, as per the new policy at https://www.drupal.org/core/backport-policy

Postponing until it is resolved in D8.

I ran into this issue. I think this is at least major and maybe even critical because the core function to view old node revisions does not work. The only work around is to use public files, which can have security implications.

Attached is a screencast video (3:05) demonstrating this issue on simplytest.me with a clean install of Drupal 7.54 core.

Thanks @arlinsandbulte for the video which shows the problem clearly - I agree this should be critical so I'm going to go ahead and flag it as such (I'm sure someone else will correct this if it is not!)

I should probably leave this as postponed as I believe it was done so waiting for this to be fixed in Drupal 8 but switching to needs review anyway.

1. Attached is a patch to address the issue, I've returned to looking for FIELD_LOAD_REVISION references in file_file_download so we can see the revisions of content the file is attached to
2. Made sure the revision of the entity gets loaded later, since I couldn't find a function in core Drupal to do this I copied the entity_revision_load() function from the entity_api module and called it _entity_revision_load() in includes/common.inc - there may be a better, more standards compliant way of generically loading an entity revision I'm not aware of.
3. Altered the function node_file_download_access() to properly check access if the given node is not the current revision as previously it was only checking the current revision of the node regardless of what node you passed it. This means that private files attached to revisions of nodes will get access checked based on access to revisions.
4. To make part 3 cleaner (in my opinion) I created a generic function called _node_is_current_revision($node) which returns TRUE if the node passed in is the current revision.

The previous patch was not taking into account that some entity types have revisions (e.g. node) and some do not (e.g. comment).

I've made a check for if it is a revisionable entity and changed the entity load function accordingly. Again I couldn't find a core function to determine if an entity_type has or does not have revisions so I've added one but I'm not sure it follows any conventions.

Ready for automated testing and then review.

Thanks @johnennew.
Due to Drupal's backport policy, this probably needs to be fixed in D8 first before it will get any attention here for D7.

The same D8 Issue is here: https://www.drupal.org/node/1452100.
It has a patch that needs review. So, if you are able, review the patch there to help push this along for D7.

Shell we keep this issue for D7 or move johnennew's patch over to #1452100: Private file download returns access denied, when file attached to revision other than current and close this issue? Hopefully this will ever be resolved then?

THIS issue (1453138) is for D7.
The other issue (1452100) is for D8.

Both D7 & D8 have the same bug.
Therefore, according to policy, two issues should be created: one for each major drupal version.
And, the earlier version should be postponed until the latest version is fixed (I'm doing so now).
This policy prevents regressions from happening when an earlier version of Drupal (D7) is fixed, but the fix is lost or never gets applied to the later version (D8).

So, the only way to move this issue forward is to first resolve the D8 issue.
That issue has a patch & needs review.

Thank you for the quick reply and information.

DRUPL 8 work arround

/**
 * Implements hook_file_access().
 */
function mymodule_common_file_access($entity, $operation, $account) {
  if ($operation == 'download') {
    $roles = $account->getRoles();
    if (in_array('specal-role', $roles)) {
      return AccessResult::allowed();
    }
  
}

Hi If you are facing this problem in D7 then you can use this module. I had also faced this problem but, did not get any solution without core modification, So I have created a module to resolve this issue, you can use this module for D7 only.

I tried using hook_file_download_alter() to grant access, but it didn't work. file_file_download() never reaches the call to drupal_alter() because of this, which precedes it:

        // Check that $entity, $field and $field_item were loaded successfully
        // and check if access to that field is not disallowed. If any of these
        // checks fail, stop checking access for this reference.
        if (empty($entity) || empty($field) || empty($field_item) || !field_access('view', $field, $entity_type, $entity)) {
          $denied = TRUE;
          break;
        }

That code, along with the calls to module_implements() and drupal_alter(), are nested inside a foreach loop that examines each file reference. If a file reference belongs to an unpublished revision, $field_item is undefined, so access is denied and the loop ends. Shouldn't the hooks still be invoked?

Reset issue status.