#846296 by Berdir, ridgerunner, agentrickard: Fixed file_file_download() only...

#846296 by Berdir, ridgerunner, agentrickard: Fixed file_file_download() only implements access checks for nodes and users.

modules/comment/comment.module

@@ -2615,3 +2615,12 @@ function comment_rdf_mapping() {
     ),
   );
 }
+
+/**
+ * Implements hook_file_download_access().
+ */
+function comment_file_download_access($field, $entity_type, $entity) {
+  if ($entity_type == 'comment') {
+    return user_access('access comments') && $entity->status == COMMENT_PUBLISHED || user_access('administer comments');
+  }
+}

modules/file/file.api.php

+<?php
+// $Id$
+
+/**
+ * @file
+ * Hooks for file module.
+ */
+
+/**
+ * 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.
+ *
+ * @param $field
+ *   The field to which the file belongs.
+ * @param $entity_type
+ *   The type of $entity; for example, 'node' or 'user'.
+ * @param $entity
+ *   The $entity to which $file is referenced.
+ *
+ * @return
+ *   TRUE is access should be allowed by this entity or FALSE if denied. Note
+ *   that denial may be overridden by another entity controller, making this
+ *   grant permissive rather than restrictive.
+ *
+ * @see hook_field_access().
+ */
+function hook_file_download_access($field, $entity_type, $entity) {
+  if ($entity_type == 'node') {
+    return node_access('view', $entity);
+  }
+}
+
+/**
+ * Alter the access rules applied to a file download.
+ *
+ * Entities that implement file management set the access rules for their
+ * individual files. Module may use this hook to create custom access rules
+ * for file downloads.
+ *
+ * @see hook_file_download_access().
+ *
+ * @param &$grants
+ *   An array of grants gathered by hook_file_download_access(). The array is
+ *   keyed by the module that defines the entity type's access control; the
+ *   values are Boolean grant responses for each module.
+ * @param $field
+ *   The field to which the file belongs.
+ * @param $entity_type
+ *   The type of $entity; for example, 'node' or 'user'.
+ * @param $entity
+ *   The $entity to which $file is referenced.
+ *
+ * @return
+ *   An array of grants, keyed by module name, each with a Boolean grant value.
+ *   Return an empty array to assert FALSE. You may choose to return your own
+ *   module's value in addition to other grants or to overwrite the values set by
+ *   other modules.
+ */
+function hook_file_download_access_alter(&$grants, $field, $entity_type, $entity) {
+  // For our example module, we always enforce the rules set by node module.
+  if (isset($grants['node'])) {
+    $grants = array('node' => $grants['node']);
+  }
+}

modules/file/file.module

@@ -141,46 +141,69 @@ function file_file_download($uri, $field_type = 'file') {
   // Find out which (if any) file fields contain this file.
   $references = file_get_file_references($file, NULL, FIELD_LOAD_REVISION, $field_type);
 
-  // TODO: Check field-level access if available here.
-
-  $denied = $file->status ? NULL : FALSE;
-  // Check access to content containing the file fields. If access is allowed
-  // to any of this content, allow the download.
+  // Default to allow access.
+  $denied = FALSE;
+  // Loop through all references of this file. If a reference explicitly allows
+  // access to the field to which this file belongs, no further checks are done
+  // and download access is granted. If a reference denies access, eventually
+  // existing additional references are checked. If all references were checked
+  // and no reference denied access, access is granted as well. If at least one
+  // reference denied access, access is denied.
   foreach ($references as $field_name => $field_references) {
     foreach ($field_references as $entity_type => $type_references) {
-      foreach ($type_references as $reference) {
-        // If access is allowed to any object, immediately stop and grant
-        // access. If access is denied, continue through in case another object
-        // grants access.
-        // TODO: Switch this to a universal access check mechanism if available.
-        if ($entity_type == 'node' && ($node = node_load($reference->nid))) {
-          if (node_access('view', $node)) {
-            $denied = FALSE;
-            break 3;
-          }
-          else {
-            $denied = TRUE;
+      foreach ($type_references as $id => $reference) {
+        // Try to load $entity and $field.
+        $entity = reset(entity_load($entity_type, array($id)));
+        $field = NULL;
+        if ($entity) {
+          // Load all fields for that entity.
+          $field_items = field_get_items($entity_type, $entity, $field_name);
+
+          // Find the field item with the matching URI.
+          foreach ($field_items as $field_item) {
+            if ($field_item['uri'] == $uri) {
+              $field = $field_item;
+              break;
+            }
           }
         }
-        if ($entity_type == 'user') {
-          if (user_access('access user profiles') || $user->uid == $reference->uid) {
-            $denied = FALSE;
-            break 3;
-          }
-          else {
-            $denied = TRUE;
-          }
+
+        // Check that $entity and $field were loaded successfully and check if
+        // access to that field is not disallowed. If any of these checks fail,
+        // stop checking access for this reference.
+        if (empty($entity) || empty($field) || !field_access('view', $field, $entity_type, $entity)) {
+          $denied = TRUE;
+          break;
+        }
+
+        // Invoke hook and collect grants/denies for download access.
+        // Default to FALSE and let entities overrule this ruling.
+        $grants = array('system' => FALSE);
+        foreach (module_implements('file_download_access') as $module) {
+          $grants = array_merge($grants, array($module => module_invoke($module, 'file_download_access', $field, $entity_type, $entity)));
+        }
+        // Allow other modules to alter the returned grants/denies.
+        drupal_alter('file_download_access', $grants, $field, $entity_type, $entity);
+
+        if (in_array(TRUE, $grants)) {
+          // If TRUE is returned, access is granted and no further checks are
+          // necessary.
+          $denied = FALSE;
+          break 3;
+        }
+
+        if (in_array(FALSE, $grants)) {
+          // If an implementation returns FALSE, access to this entity is denied
+          // but the file could belong to another entity to which the user might
+          // have access. Continue with these.
+          $denied = TRUE;
         }
       }
     }
   }
 
-  // No access was denied or granted.
-  if (!isset($denied)) {
-    return;
-  }
-  // Access specifically denied and not granted elsewhere.
-  elseif ($denied == TRUE) {
+  // Access specifically denied.
+  if ($denied) {
     return -1;
   }
 

modules/file/tests/file.test

@@ -14,7 +14,7 @@ class FileFieldTestCase extends DrupalWebTestCase {
 
   function setUp() {
     parent::setUp('file');
-    $this->admin_user = $this->drupalCreateUser(array('access content', 'access administration pages', 'administer site configuration', 'administer users', 'administer content types', 'administer nodes', 'bypass node access'));
+    $this->admin_user = $this->drupalCreateUser(array('access content', 'access administration pages', 'administer site configuration', 'administer users', 'administer permissions', 'administer content types', 'administer nodes', 'bypass node access'));
     $this->drupalLogin($this->admin_user);
   }
 
@@ -301,6 +301,68 @@ class FileFieldWidgetTestCase extends FileFieldTestCase {
     $this->drupalGet("admin/structure/types/manage/$type_name/fields/$field_name");
     $this->assertFieldByXpath('//input[@id="edit-field-settings-uri-scheme-public" and not(@disabled)]', 'public', t('Upload destination setting enabled.'));
   }
+
+  /**
+   * Tests that download restrictions on private files work on comments.
+   */
+  function testPrivateFileComment() {
+    $user = $this->drupalCreateUser(array('access comments'));
+
+    // Remove access comments permission from anon user.
+    $edit = array(
+      '1[access comments]' => FALSE,
+    );
+    $this->drupalPost('admin/people/permissions', $edit, t('Save permissions'));
+
+    // Create a new field.
+    $edit = array(
+      '_add_new_field[label]' => $label = $this->randomName(),
+      '_add_new_field[field_name]' => $name = strtolower($this->randomName()),
+      '_add_new_field[type]' => 'file',
+      '_add_new_field[widget_type]' => 'file_generic',
+    );
+    $this->drupalPost('admin/structure/types/manage/article/comment/fields', $edit, t('Save'));
+    $edit = array('field[settings][uri_scheme]' => 'private');
+    $this->drupalPost(NULL, $edit, t('Save field settings'));
+    $this->drupalPost(NULL, array(), t('Save settings'));
+
+    // Create node.
+    $text_file = $this->getTestFile('text');
+    $edit = array(
+      'title' => $this->randomName(),
+    );
+    $this->drupalPost('node/add/article', $edit, t('Save'));
+
+    // Add a comment with a file.
+    $text_file = $this->getTestFile('text');
+    $edit = array(
+      'files[field_' . $name . '_' . LANGUAGE_NONE . '_' . 0 . ']' => realpath($text_file->uri),
+      'comment_body[' . LANGUAGE_NONE . '][0][value]' => $comment_body = $this->randomName(),
+    );
+    $this->drupalPost(NULL, $edit, t('Save'));
+
+    // Get the comment ID.
+    preg_match('/comment-([0-9]+)/', $this->getUrl(), $matches);
+    $cid = $matches[1];
+
+    // Log in as normal user.
+    $this->drupalLogin($user);
+   
+    $comment = comment_load($cid);
+    $comment_file = (object) $comment->{'field_' . $name}[LANGUAGE_NONE][0];
+    $this->assertFileExists($comment_file, t('New file saved to disk on node creation.'));
+    // Test authenticated file download.
+    $url = file_create_url($comment_file->uri);
+    $this->assertNotEqual($url, NULL, t('Confirmed that the URL is valid'));
+    $this->drupalGet(file_create_url($comment_file->uri));
+    $this->assertResponse(200, t('Confirmed that the generated URL is correct by downloading the shipped file.'));
+ 
+    // Test anonymous file download.
+    $this->drupalLogout();
+    $this->drupalGet(file_create_url($comment_file->uri));
+    $this->assertResponse(403, t('Confirmed that access is denied for the file without the needed permission.'));
+  }
+
 }
 
 /**

modules/node/node.module

@@ -3763,3 +3763,12 @@ protected function buildQuery($ids, $conditions = array(), $revision_id = FALSE)
     return $query;
   }
 }
+
+/**
+ * Implements hook_file_download_access().
+ */
+function node_file_download_access($field, $entity_type, $entity) {
+  if ($entity_type == 'node') {
+    return node_access('view', $entity);
+  }
+}

modules/user/user.module

@@ -3684,3 +3684,12 @@ function user_rdf_mapping() {
     ),
   );
 }
+
+/**
+ * Implements hook_file_download_access().
+ */
+function user_file_download_access($field, $entity_type, $entity) {
+  if ($entity_type == 'user') {
+    return user_view_access($entity);
+  }
+}