file_file_download() only implements access checks for nodes and users

I haven't researched this, but it sounds like field_access('view') should tell you for any entity if the file is viewable.

Just need to track this issue since I filed the original Private Message bug report....

Shouldn't node.module pass on permissions from the node type to the attached fields via a node_field_access()?
And shouldn't user.module do the same, ie grant users with the 'view user profiles' permissions the field-level 'view' permission?
I actually have no clue, but I read the PHPDoc of the related functions, and, going from there, that would IMO make the most sense conceptually.

Using field_access() then having hook_field_access() implementations would get around the hard coding of node and user modules, but it's going to add a fair bit of PHP overhead for every single field - 20 instances on a node, 10 nodes in a list, now you're running node_field_access() 200 times where previously nothing, and that overhead is going to be on sites without private files too.

I found what is probably a pretty nasty bug in file_get_file_references() where it is improperly calling drupal_static(). It is calling it like so...
$references = drupal_static(__FUNCTION__, array());
but it is supposed to call it with the =& reference operator like so...
$references = &drupal_static(__FUNCTION__, array());
I'm not familiar with the file end of the drupal code base here so I have no idea what the ramifications of this fix might be, but this may resolve this issue. Attached is a patch that corrects this error. Let's see if this fix passes automatic testing...

Not sure why the patch from the previous post did not get submitted for testing. Trying again with a different filename...

Ok. Could someone please explain why the above two (identical except for filename) patches were ignored?

Ok, found the problem. Looks like the status needs to be "Needs Review" to trigger the bot...

Sorry for the interruption. Carry on.

Here are (at least some of) the options then:

1. Make the switch to hook_field_access(), node_field_access(), and user_field_access(); eat the performance hit
2. Force developers to implement hook_file_download()
3. adding: Create hook_file_download_access(); implement node_file_download_access() and user_file_download_access()
4. and: Make "public/private download" control a field-level setting.

As noted in the original post, file and image downloads just don't work currently. See #867928: Files and Images don't work in private filesystem mode.

Making node and user implement hook_field_access() sounds like the way to go to me.

This said, it will have a performance hit, which proves that field access control is not performed at the correct place. It is currently processed field-by-field into field_default_view(), while it should probably be a multiple hook processed in field_attach_view(). One additional argument to make this change is that field_view_field() currently calls field_default_view(), so that fields displayed by other modules (for example: Views) will *also* be subjected to access control.

Having node_field_access() and user_field_access() introduces that performance hit even for sites not using private files - that's a regression from D6 for all sites which don't use private files (which is still likely to be the majority even with the separation). Making it a multiple hook is a good idea to ameliorate this somewhat but that's a proper API change, whereas hook_file_download_access() wouldn't have any performance penalty, and is only an API addition. Either multiple hook_field_access() if it worked out alright or hook_file_download_access() I can live with though.

Since we have a module_implements cache, I don't really see a significant performance hit for adding a 100-200 (for the sake of argument) no-op function calls. I would prefer that to adding a new hook which is special for files. Not a huge deal either day.

I guess I prefer whatever patch anyone comes up with to close the critical. Keep existing hook or make a new one.

Re: #19

1. default to true (allow access)
2. best i can find is http://api.drupal.org/api/function/hook_entity_info/7

At #851878: serve image derivatives from the same url they are generated from, justin notes that image derivitives of private files are not currently access controlled. Might be fixable here.

This also looks like THE place/function to solve 1/4th of #881578: Solve SA-CORE-2010-002 issues, i.e., the "File download access bypass". Looks like the files list returned could be multiples and the upper/lower case trick also applies. I'll submit a patch over there, just cross-referencing.

Code and approach are sound IMO.

- If you have implement multiple in hook_field_access, thats ideal. But I am happy to go with this patch approach as well.

- current hook name is fine

- those args look good to me

- agree, d8 stuff

- thanks for including privatemsg example code. it is helpful when considering the patch.

I do think that comment logic is right and belongs in this hook. As you said, there is no protection currently for term viewing.

About your 'additional question': I agree with the behavior you have coded.

Code looks ready save for the the doxygen you mentioned.

No tests in this patch. You mentioned those in #28

Perhaps the comment module hunk needs minor change since #881578: Solve SA-CORE-2010-002 issues was committed.

I'm satisfied with the existing test on this. We're just down to doxygen tweaks

+ * Note that hook_field_access() should be implemented instead to provide
+ * global access restrictions for fields even though $field is passed to this
+ * hook as well.

Not sure what we mean to convey here. Perhaps omit it?

If a reference denies access, eventually existing additional references are checked.

This seems to imply that we are not short circuiting a denial when the code says that we are. Or I am missing the meaning.

OK, that logic makes sense.

Now that I think of it, I think we should add a drupal_alter('file_download_access', $grants) after our first loop. This gives flexibility to site specific modules to get the behavior they want. This is what we added for node access (see http://api.drupal.org/api/function/node_access_acquire_grants/7) so we should do same here. This does mean that we can't short circuit anymore. Not a big loss.

OK, thats exactly what I meant. RTBC. I'll summarize.

If you add a file or image Field to comments or terms or a custom entity, you currently have no good way to deny to the file if the user has no access to view that entity. Here is example code for privatemsg which uses entities for its messages in D7:

/**
* Implements hook_file_download_acces().
*/
function privatemsg_file_download_access($field, $entity_type, $entity) {
  global $user;

  if ($entity_type == 'privatemsg_message') {
    // Users with read all private messages permission can view all files too.
    if (user_access('read all private messages')) {
      return TRUE;
    }

    // Check if user is a recipient of this message.
    return (bool)db_query_range("SELECT 1 FROM {pm_index} WHERE recipient = :uid AND type IN ('user', 'hidden') AND mid = :mid", 0, 1, array(':uid' => $user->uid, ':mid' => $entity->mid))->fetchField();
  }
}

Coming to you LIVE from Drupalcon CPH!

Reviewed this patch with Dries, and we both agreed it looks good. But it seems we need expanded test coverage for the original bug? Something like adding a file field to a comment and verifying that access carries over properly.

With that change, looks like this is good to go.

This is also an API change, so tagging accordingly.

Oh. We also need docs for hook_file_download_access_alter(), too.

I do like the parity here with the node access system. +1.

great. just found one typo: authenticed

I found a bunch of problems, discussed them to Joakim, Berdir and webchick. New patch attached. Summary:

-- Added return documentation for hook_file_download_access().
-- Added documentation for hook_file_download_access_alter().
-- Made the $grants array passed to the alter hook contain some meaningful context. As written before, the alter hook was passed a positional array of Boolean values. Not enough context to 'alter'. Now the $grants array is keyed by the module that controls the entity.
-- Default access set to FALSE (assigned to system module), so that at least one entity must assert TRUE for access to be granted.

Berdir's tests are incorporated here.

Forgot that file.api.php is a new file.

OK, I think we are ready.

@agentrickard - nice catch in #43 with extra context in hook and change to $grants deny logic.

Great work, folks! Committed to HEAD.

Randy, the summary of this API change is in #57 (but cross-reference with the example docs in file.api.php because I think the function signature changed in the latest patch).

Oh man, the dsm() made poor Andrew re-test the patch all morning. D'oh!

OK, looking for the issue and API change summary. Not finding it in #57 :-) Can somebody point me to it or write it?

Thanks!

#37 and #43 have the revelant features. There is also a new file.api.inc that explains the new hooks,

Looks like there's no longer an API change here, just needs a documentation fix.

Can someone clarify what documentation needs to be fixed? Webchick mentioned a summary above, but she said it was on comment #57, which doesn't exist (and neither does #47).

And does this change need to be put into the module 6/7 upgrade guide? If so, please tag "Needs Update Documentation". Thanks.

I think we still need a taxonomy_file_download_access() for files attached to terms and vocabularies. Try adding an image field from the private filesystem to a term. If there are other field data visible, you should see a broken image. Since there's an alter hook already, just returning TRUE when the $entity_type is 'taxonomy_term' should be enough for core.

Looks like an x-post cleared the Needs Documentation tag.

3 weeks is a long time for a cross-post. Sorry, but I meant to take it off because I wanted to indicate that we need some more code. I guess what we really need are more tests.

I encountered the error as described in #56 and posted this issue: Private files broken for Filefield/Imagefield in Taxonomy Term.
Then I found this issue and I applied the patch in #58, but with no results. Do I need to apply more patches?

That has become a second nature of me, but in this case, I didn't. :-( Thanks for the tip!
Works, like a charm, and I hope it gets in a next -dev version! --> RTBC

There seems to be an issue with this in the Drupal 7 release code.

/modules/file/file.module Line 143

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

  // If there are no references, stop processing, to avoid returning headers
  // for files controlled by other modules.
  if (empty($references)) {
    return;
  }

The functionality spelt out in the commments is sound.
Problem is, if a field of $field_type is defined but doesn't contain a reference to the file what's returned isn't empty().

Example return:

array(2) {
  ["field_file"]=>
  array(0) {
  }
  ["field_file2"]=>
  array(0) {
  }
}

This means that files where there is no reference node (or the reference node isn't accessible to the user) are always accessible.

I haven't got GIT setup yet otherwise I'd submit a patch. :(

Right. This issue has not been marked "fixed", because it hasn't been fixed. Please leave the version as 7.x-dev, since it hasn't been fixed in the development version, much less the released code.

And there is already a patch in #58, and in fact it has already been reviewed. Please don't change the status.

#58: 846296_taxonomy_files.patch queued for re-testing.

Hi!
I've been fighting with this for a couple of days already. I must say that i consider this a very (security?) seriousone. I did extenses tests and i came up with the following clonclusion:
As long as the file access is managed by Drupal core functionalies (no contributed AC module acting over node_access table) the behavior is as expected but in the presence of such a AC module the query that is constructed in file_get_file_references:

      $query = new EntityFieldQuery();
      $query
        ->fieldCondition($file_field, 'fid', $file->fid)
        ->age($age);
      $references[$field_name] = $query->execute();

returns populated arrays (by reference, the query is executed as many times as fild instances of the type 'file' are) of only nodes the user can access, wich IMO shouldn't be becouse then we are NOT going to have the posibility of calling the hook_file_download_access or _alter for files attached to that node and according with the documentation that is exactly what this hook is for:

Control download access to files.

The hook is typically implemented to limit access based on the entity the file is referenced, e.g., only users with access to a node should be allowed to download files attached to that node.

On the other hand it seems that for other types of entities than nodes, it should work fine, the following is an example of the query executed by file_get_file_references on a node with a file attached and in the presence of the Tac Lite (AC module) (note the field_data_field_datei0.entity_type <> 'node'):

SELECT DISTINCT field_data_field_datei0.entity_type AS entity_type, field_data_field_datei0.entity_id AS entity_id, field_data_field_datei0.revision_id AS revision_id, field_data_field_datei0.bundle AS bundle
FROM 
field_data_field_datei field_data_field_datei0
LEFT OUTER JOIN node_access na ON na.nid = field_data_field_datei0.entity_id
WHERE  (field_data_field_datei0.field_datei_fid = '208') AND (field_data_field_datei0.deleted = '0') AND(((( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '0') AND (na.realm = 'tac_lite') ))AND (na.grant_view >= '1') AND (field_data_field_datei0.entity_type = 'node') )OR (field_data_field_datei0.entity_type <> 'node') )

Here we see that the query checks (when the entity is 'node') the access on the node to return the result, delivering an empty array and therefore bannig file_file_download to call the corresponding hooks.
I'm posting a patch to resolve the issue when the $references array is an array of empty values but i really think references should always be returned and not only when the entities they are attatched to are accessible by the user.
The patch is against the 7.0 -> changing Version and Status to schedule tha patch test.
I consider this major becouse it limits the posibility to use contributed AC modules, exposing the private files attached to a nodes, even when nodes are not accessible.

...Ups, i'm sorry, it's the first time i'm posting a patch..

Hm, this seems similar to what I've been doing. I haven't compared your patch with mine in detail yet though. Just throwing this out there for more eyes to see.

The problem is that file.module :: file_get_file_references() sets $references[$field_name] to an empty array if the EntityFieldQuery fails to find the file. EntityFieldQuery returns an empty array because it actually does honor node_access and finds the file restricted. Unfortunately, setting $references (even to an empty array) leads the calling function, file_file_download(), to try to traverse the empty array looking for access denial. Since it's empty, the function assumes access is granted. Attached is my patch to fix the problem by checking if the EntityFieldQuery returned an empty array.

It appears that my patch tackles the bug more upstream, which is good if code elsewhere uses file_get_file_references(), whereas #68 only works within file_file_download().

@nonzero I DO really think that would be a nice code to patch file.module (it adresses a more general issue) but the question is:
Should file_get_file_references() honor node_access or other access call on the entities, files are attached to ??
V.S.
Should implement file access checks regardless of node_access or other entities' access check??
As long as i understand the answer is affirmative to the second question (see the blockquote i posted) and in that case neither your nor my patch adresses the issue, the query is just wrong constructed.
I repeat:
references should always be returned and not only when the entities they are attatched to are accessible by the user.
It's just a matter of Business Logic.
This issue is assigned to Berdir, opinions from him (and the community of course) are wellcome.

@neoglez

I disagree, simply because file_get_file_references() is merely a helper function for file_file_download() and is designed to force an access check. No other core functions call this code.

I think you want a new function or query to return all files attached to an entity, which is valid but not required in this context.

Otherwise, the solution would have to be an API change that passes file information and access information back to the caller.

Marking #69 RTBC.

D8 first. Let's get these fixed on D8 and into D7.

This is a duplicate of [#1168756] afaik.

This issue was unpublished a few years ago, because the followups discussed here touched on a security issue. However, that security issue was fixed a long time ago in SA-CORE-2011-001. I'm republishing it now because I want to refer to it in #2305017: Regression: Files or images attached to certain core and non-core entities are lost when the entity is edited and saved, which relates to some of the code that was originally added here.

I'm also moving this issue to "Fixed" because the original patch here was committed to Drupal 7 a long time ago. Besides the security issue, the two other followups I see are:

1. Possible missing implementation of hook_file_download_access() in the Taxonomy module. This is a separate issue at #1327224: Access denied to taxonomy term image.
2. Inconsistency between the code and code comments (#53). However, that ties directly into #2305017: Regression: Files or images attached to certain core and non-core entities are lost when the entity is edited and saved, so we can continue that discussion there. Basically, at first glance it looks to me like the code is doing the wrong thing (not the code comments), but I need more feedback on that issue.