It's good that filefield_file_download() prevents a file from being seen when the user doesn't have permission to view the node it is attached to. However, if a file is attached to several nodes, and only one of them is restricted, then the file is unviewable in all scenarios. Allowing files to be reused on nodes is the explicit purpose of filefield_sources, but the same thing can also happen with imported data or other uses of files like taxonomy_image.

I wonder if the node_access() check really belongs in this hook. The context of the file can't really be gained just from the filepath. Instead, it comes from whatever is causing the file to be viewed, whether it's through a link, or whatever. It's not enforceable except by calling it a best practice, but the calling context should call node_access() before displaying the file.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Island Usurper’s picture

Quick update to say that the comment on that section says that it should deny "if access is denied for ALL nodes containing the file", but this doesn't look to be accurate. Once $denied is set to TRUE, the function must return -1.

quicksketch’s picture

Status: Active » Needs review
FileSize
1.46 KB

I've confirmed that this seems to be the case. FileField is too restrictive when the same file has been shared across nodes. I'd have to argue that knowing where a file is used is plenty information to determine whether access should be granted. Since FileField files are by nature attached to a node, it only makes sense that access to the file should be based upon access to the node. If you've directly linked to a file, access shouldn't always be granted. If this were the case we'd just return TRUE on access to all files.

I put together this patch, but I haven't tested it. I think it should correct the problem, but I dislike making changes like this since they often result in needing to tighten things down again later (with a security release to boot). If you can give this a shot and see if it allows only what's expected, that'd be appreciated.

quicksketch’s picture

Status: Needs review » Fixed
FileSize
1.47 KB

Okay here's a new patch that I've tested and it implements access correctly. It should allow access to files that are shared between multiple nodes as long as a user has access to view that field and node on at least one node.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

donquixote’s picture

Status: Closed (fixed) » Needs work

What if there are duplicates in the files table?
Yes, I should probably clean this up, but this is a big site, and I am not 100% sure about the implications.

The current behavior in this case is quite arbitrary. It will simply pick the first fid it finds.

The desired logic should be:
- If any node with this file as a filefield value is visible to the user, then the file should be visible as well.
- Otherwise, if any node with this file attached or as a filefield is hidden to the user, return -1.
- Otherwise, return NULL.

In case of a files table without duplicates, this will show the old behavior.
In case of a files table with duplicates, it will have a well-defined behavior, instead of a wonky behavior.

If you agree, I can produce a patch.
Otherwise, I will have to do this in a custom module for this particular site..