#898036 by Berdir: Fixed Private images broken. (with tests)

includes/file.inc

@@ -1808,9 +1808,19 @@ function file_download() {
   $uri = $scheme . '://' . $target;
   if (file_stream_wrapper_valid_scheme($scheme) && file_exists($uri)) {
     // Let other modules provide headers and controls access to the file.
-    $headers = module_invoke_all('file_download', $uri);
-    if (in_array(-1, $headers)) {
-      return drupal_access_denied();
+    // module_invoke_all() uses array_merge_recursive() which merges header
+    // values into a new array. To avoid that and allow modules to override
+    // headers instead, use array_merge() to merge the returned arrays.
+    $headers = array();
+    foreach (module_implements('file_download') as $module) {
+      $function = $module . '_file_download';
+      $result = $function($uri);
+      if ($result == -1) {
+        return drupal_access_denied();
+      }
+      if (isset($result) && is_array($result)) {
+        $headers = array_merge($headers, $result);
+      }
     }
     if (count($headers)) {
       file_transfer($uri, $headers);

modules/file/file.module

@@ -138,9 +138,15 @@ function file_file_download($uri, $field_type = 'file') {
     return;
   }
 
-  // Find out which (if any) file fields contain this file.
+  // Find out which (if any) fields of this type contain the file.
   $references = file_get_file_references($file, NULL, FIELD_LOAD_REVISION, $field_type);
 
+  // If there are no references, stop processing, to avoid returning headers
+  // for files controlled by other modules.
+  if (empty($references)) {
+    return;
+  }
+
   // Default to allow access.
   $denied = FALSE;
   // Loop through all references of this file. If a reference explicitly allows

modules/image/image.module

@@ -288,6 +288,9 @@ function image_file_download($uri) {
     array_shift($args);
     // Get the style name from the second part.
     $style_name = array_shift($args);
+    // Remove the scheme from the path.
+    array_shift($args);
+
     // Then the remaining parts are the path to the image.
     $original_uri = file_uri_scheme($uri) . '://' . implode('/', $args);
 

modules/image/image.test

@@ -619,13 +619,28 @@ class ImageFieldDisplayTestCase extends ImageFieldTestCase {
     );
   }
 
+  /**
+   * Test image formatters on node display for public files.
+   */
+  function testImageFieldFormattersPublic() {
+    $this->_testImageFieldFormatters('public');
+  }
+
+  /**
+   * Test image formatters on node display for private files.
+   */
+  function testImageFieldFormattersPrivate() {
+    // Remove access content permission from anonymous users.
+    user_role_change_permissions(DRUPAL_ANONYMOUS_RID, array('access content' => FALSE));
+    $this->_testImageFieldFormatters('private');
+  }
+
   /**
    * Test image formatters on node display.
    */
-  function testImageFieldFormatters() {
+  function _testImageFieldFormatters($scheme) {
     $field_name = strtolower($this->randomName());
-    $this->createImageField($field_name, 'article');
-
+    $this->createImageField($field_name, 'article', array('uri_scheme' => $scheme));
     // Create a new node with an image attached.
     $test_image = current($this->drupalGetTestFiles('image'));
     $nid = $this->uploadNodeImage($test_image, $field_name, 'article');
@@ -648,6 +663,23 @@ class ImageFieldDisplayTestCase extends ImageFieldTestCase {
     $default_output = l(theme('image', $image_info), file_create_url($image_uri), array('html' => TRUE));
     $this->drupalGet('node/' . $nid);
     $this->assertRaw($default_output, t('Image linked to file formatter displaying correctly on full node view.'));
+    // Verify that the image can be downloaded.
+    $this->assertEqual(file_get_contents($test_image->uri), $this->drupalGet(file_create_url($image_uri)), t('File was downloaded successfully.'));
+    if ($scheme == 'private') {
+      // Only verify HTTP headers when using private scheme and the headers are
+      // sent by Drupal.
+      $this->assertEqual($this->drupalGetHeader('Content-Type'), 'image/png; name="' . $test_image->filename . '"', t('Content-Type header was sent.'));
+      $this->assertEqual($this->drupalGetHeader('Content-Disposition'), 'inline; filename="' . $test_image->filename . '"', t('Content-Disposition header was sent.'));
+      $this->assertEqual($this->drupalGetHeader('Cache-Control'), 'private', t('Cache-Control header was sent.'));
+
+      // Log out and try to access the file.
+      $this->drupalLogout();
+      $this->drupalGet(file_create_url($image_uri));
+      $this->assertResponse('403', t('Access denied to original image as anonymous user.'));
+
+      // Log in again.
+      $this->drupalLogin($this->admin_user);
+    }
 
     // Test the image linked to content formatter.
     $instance['display']['default']['settings']['image_link'] = 'content';
@@ -660,7 +692,7 @@ class ImageFieldDisplayTestCase extends ImageFieldTestCase {
     $instance['display']['default']['settings']['image_link'] = '';
     $instance['display']['default']['settings']['image_style'] = 'thumbnail';
     field_update_instance($instance);
-    // Ensure the derrivative image is generated so we do not have to deal with
+    // Ensure the derivative image is generated so we do not have to deal with
     // image style callback paths.
     $this->drupalGet(image_style_url('thumbnail', $image_uri));
     $image_info['path'] = image_style_path('thumbnail', $image_uri);
@@ -668,6 +700,13 @@ class ImageFieldDisplayTestCase extends ImageFieldTestCase {
     $default_output = theme('image', $image_info);
     $this->drupalGet('node/' . $nid);
     $this->assertRaw($default_output, t('Image style thumbnail formatter displaying correctly on full node view.'));
+
+    if ($scheme == 'private') {
+      // Log out and try to access the file.
+      $this->drupalLogout();
+      $this->drupalGet(image_style_url('thumbnail', $image_uri));
+      $this->assertResponse('403', t('Access denied to image style thumbnail as anonymous user.'));
+    }
   }
 
   /**