Currently filefield_file_download currently goes and gather all nodes attached to a field, then loops through the nids, loading each node, doing a node_access then a field access check (though a bug it is only checking the first field that file was assigned to, so if it was assigned to two where first is private but second is public... not that is ever going to happen >.O.).

However, if you have many nodes (say, thousands) and only the last node the user actually has access to, this will degrade.

So my solution is to do a db_query to filter out nodes the user doesn;'t have access to.

I needed an array of nids to query on and to be able to easily remove nids from it. However, when looking into this, I noticed that the code is old; it was getting information it was not using (vid), it was using logic that had been made better elsewhere, so by looking filefield_get_file_references, updated those part also.

SInce logic needed to be reworked for easily nid-fetchable array, changed the double foreach's also

The disadvantage to this I see is:
1) the query is aqward (is there a better way to do it?)
2) extra query for users that do have access (since a user that doesn't have access will likely be catched with the new query, it avoids a full node_load for them).

One thing is could check how many files currently are before doing this query, via some arbiatary number. shrug.

CommentFileSizeAuthor
filefield_file_download_mucho_files.patch3.96 KBhefox

Comments

quicksketch’s picture

quicksketch
he/him
commented

Regarding this change:

+    // Now that know that we're handling this file, can assume if fails
+    // basic access content should not have access to file.
+    if (!user_access('access content')) {
+      return -1;
+    }

That (apparently) is not a safe check. Considering your edge-case situation you probably will respect this issue's similarly edgy request that made it so that users could not only view files without "access content" but actually upload content too: #1126198: Allow users without "access content" permission to upload files.

hefox’s picture

hefox commented

Looking at the code, I suspect it's not an issue, but if it is an issue, it's a pre-existing issue.

From filefield_file_download

  // See if this is a file on a newly created node, on which the user who
  // uploaded it will immediately have access.
  $new_node_file = $file->status == 0 && isset($_SESSION['filefield_access']) && in_array($file->fid, $_SESSION['filefield_access']);
  if ($new_node_file) {
    $denied = FALSE;
  }

If those conditions match, the parts the patch is modifying are never reached.

In the current filefield_file_download, if node_access returns false, it denies access; the user_access('access content' check was added right before -- I don't think it's changing functionality there, but should be tested. (I don't actually have a true private file system set up (modifying filefield_private) and need to to correctly test this).

andreiashu’s picture

andreiashu commented

#1175362: Potential memory leak in filefield_file_download() is somewhat related to this.

pwolanin’s picture

pwolanin commented
Status: Needs review » Closed (won't fix)

bug fixes only now