I was looking at the following code in filefield_file_download() and I think there is at least one problem with it.

// So the overall field view permissions are not denied, but if access is
// denied for ALL nodes containing the file, deny the download as well.
// Node access checks also include checking for 'access content'.
$nodes = array();
$denied = TRUE;
foreach ($cck_files as $field_name => $field_files) {
  foreach ($field_files as $revision_id => $content) {
    // Checking separately for each revision is probably not the best idea -
    // what if 'view revisions' is disabled? So, let's just check for the
    // current revision of that node.
    if (isset($nodes[$content['nid']])) {
      continue; // Don't check the same node twice.
    }
    if (($node = node_load($content['nid'])) && (node_access('view', $node) && filefield_view_access($field_name, $node))) {
      $denied = FALSE;
      break 2;
    }
    $nodes[$content['nid']] = $node;
  }
}

The first issue is the way we load the nodes: node_load($content['nid']). Shouldn't it clear the internal node cache of node_load (eg. node_load($content['nid'], NULL, TRUE))?

The second thing is that $nodes[$content['nid']] = $node; can be rewritten as $nodes[$content['nid']] = TRUE; as we don't need the node itself (the code just does isset() to check if we already looped through this node: isset($nodes[$content['nid']])

I can obviously provide a patch to this but I wanted to know first if there were any reasons behind the way it is being done now.

Comments

andreiashu’s picture

If #1154324: In filefield_file_download, use a query + db_rewrite_sql to first check access, avoid extra node_load's gets in then this issue will be obsolete otherwise the changes suggested are still valid.

pwolanin’s picture

Status: Active » Closed (duplicate)

Looks like this is duplicate now

pwolanin’s picture

Status: Closed (duplicate) » Closed (won't fix)

Actually, also marking won't fix since it seems this is not a critical bug.