Fix that bulk download raises Errno::EACCES on Windows (#7056).

Patch by Yuichi HARADA.


git-svn-id: http://svn.redmine.org/redmine/trunk@19688 e93f8b46-1217-0410-a6f0-8f06a7374b81

3 files changed, 33 insertions, 26 deletions

diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb
index b69dcf983..0c76490b2 100644
--- a/app/controllers/attachments_controller.rb
+++ b/app/controllers/attachments_controller.rb
@@ -137,16 +137,16 @@ class AttachmentsController < ApplicationController
   end
 
   def download_all
-    Tempfile.create('attachments_zip-', Rails.root.join('tmp')) do |tempfile|
-      zip_file = Attachment.archive_attachments(tempfile, @attachments)
-      if zip_file
-        send_data(
-          File.read(zip_file.path),
-          :type => 'application/zip',
-          :filename => "#{@container.class.to_s.downcase}-#{@container.id}-attachments.zip")
-      else
-        render_404
-      end
+    zip_data = Attachment.archive_attachments(@attachments)
+    if zip_data
+      file_name = "#{@container.class.to_s.downcase}-#{@container.id}-attachments.zip"
+      send_data(
+        zip_data,
+        :type => Redmine::MimeType.of(file_name),
+        :filename => file_name
+      )
+    else
+      render_404
     end
   end
 
diff --git a/app/models/attachment.rb b/app/models/attachment.rb
index b5a3332ac..b0cc94cb2 100644
--- a/app/models/attachment.rb
+++ b/app/models/attachment.rb
@@ -346,28 +346,31 @@ class Attachment < ActiveRecord::Base
     Attachment.where("created_on < ? AND (container_type IS NULL OR container_type = '')", Time.now - age).destroy_all
   end
 
-  def self.archive_attachments(out_file, attachments)
+  def self.archive_attachments(attachments)
     attachments = attachments.select(&:readable?)
     return nil if attachments.blank?
 
     Zip.unicode_names = true
     archived_file_names = []
-    Zip::File.open(out_file.path, Zip::File::CREATE) do |zip|
+    buffer = Zip::OutputStream.write_buffer do |zos|
       attachments.each do |attachment|
         filename = attachment.filename
         # rename the file if a file with the same name already exists
         dup_count = 0
         while archived_file_names.include?(filename)
           dup_count += 1
-          basename = File.basename(attachment.filename, '.*')
           extname = File.extname(attachment.filename)
+          basename = File.basename(attachment.filename, extname)
           filename = "#{basename}(#{dup_count})#{extname}"
         end
-        zip.add(filename, attachment.diskfile)
+        zos.put_next_entry(filename)
+        zos << IO.binread(attachment.diskfile)
         archived_file_names << filename
       end
     end
-    out_file
+    buffer.string
+  ensure
+    buffer&.close
   end
 
   # Moves an existing attachment to its target directory
diff --git a/test/unit/attachment_test.rb b/test/unit/attachment_test.rb
index 209775514..5adfc5116 100644
--- a/test/unit/attachment_test.rb
+++ b/test/unit/attachment_test.rb
@@ -280,28 +280,32 @@ class AttachmentTest < ActiveSupport::TestCase
 
   def test_archive_attachments
     attachment = Attachment.create!(:file => uploaded_test_file("testfile.txt", ""), :author_id => 1)
-    Tempfile.create('attachments_zip', Rails.root.join('tmp')) do |tempfile|
-      zip_file = Attachment.archive_attachments(tempfile, [attachment])
-      assert_instance_of File, zip_file
+    zip_data = Attachment.archive_attachments([attachment])
+    file_names = []
+    Zip::InputStream.open(StringIO.new(zip_data)) do |io|
+      while (entry = io.get_next_entry)
+        file_names << entry.name
+      end
     end
+    assert_equal ['testfile.txt'], file_names
   end
 
   def test_archive_attachments_without_attachments
-    Tempfile.create('attachments_zip', Rails.root.join('tmp')) do |tempfile|
-      zip_file = Attachment.archive_attachments(tempfile, [])
-      assert_nil zip_file
-    end
+    zip_data = Attachment.archive_attachments([])
+    assert_nil zip_data
   end
 
   def test_archive_attachments_should_rename_duplicate_file_names
     attachment1 = Attachment.create!(:file => uploaded_test_file("testfile.txt", ""), :author_id => 1)
     attachment2 = Attachment.create!(:file => uploaded_test_file("testfile.txt", ""), :author_id => 1)
-    Tempfile.create('attachments_zip', Rails.root.join('tmp')) do |tempfile|
-      zip_file = Attachment.archive_attachments(tempfile, [attachment1, attachment2])
-      Zip::File.open(zip_file.path) do |z|
-        assert_equal ['testfile.txt', 'testfile(1).txt'], z.map(&:name)
+    zip_data = Attachment.archive_attachments([attachment1, attachment2])
+    file_names = []
+    Zip::InputStream.open(StringIO.new(zip_data)) do |io|
+      while (entry = io.get_next_entry)
+        file_names << entry.name
       end
     end
+    assert_equal ['testfile.txt', 'testfile(1).txt'], file_names
   end
 
   def test_move_from_root_to_target_directory_should_move_root_files