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
Comment #1
andreiashu CreditAttribution: andreiashu commentedIf #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.
Comment #2
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedLooks like this is duplicate now
Comment #3
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedActually, also marking won't fix since it seems this is not a critical bug.