Merge pull request #300 from hainesr/fix-create-perms

Fix permissions on new zip files (#294)

lib/zip/file.rb

@@ -43,7 +43,7 @@ module Zip
   # interface for accessing the filesystem, ie. the File and Dir classes.
 
   class File < CentralDirectory
-    CREATE               = 1
+    CREATE               = true
     SPLIT_SIGNATURE      = 0x08074b50
     ZIP64_EOCD_SIGNATURE = 0x06064b50
     MAX_SEGMENT_SIZE     = 3_221_225_472
@@ -64,20 +64,19 @@ module Zip
 
     # Opens a zip archive. Pass true as the second parameter to create
     # a new archive if it doesn't exist already.
-    def initialize(file_name, create = nil, buffer = false, options = {})
+    def initialize(file_name, create = false, buffer = false, options = {})
       super()
       @name    = file_name
       @comment = ''
-      @create  = create
+      @create  = create ? true : false # allow any truthy value to mean true
       case
       when !buffer && ::File.size?(file_name)
-        @create = nil
+        @create = false
         @file_permissions = ::File.stat(file_name).mode
         ::File.open(name, 'rb') do |f|
           read_from_stream(f)
         end
-      when create
+      when @create
-        @file_permissions = create_file_permissions
         @entry_set = EntrySet.new
       when ::File.zero?(file_name)
         raise Error, "File #{file_name} has zero size. Did you mean to pass the create flag?"
@@ -95,7 +94,7 @@ module Zip
       # Same as #new. If a block is passed the ZipFile object is passed
       # to the block and is automatically closed afterwards just as with
       # ruby's builtin File.open method.
-      def open(file_name, create = nil)
+      def open(file_name, create = false)
         zf = ::Zip::File.new(file_name, create)
         return zf unless block_given?
         begin
@@ -340,7 +339,7 @@ module Zip
       @entry_set.each do |e|
         return true if e.dirty
       end
-      @comment != @stored_comment || @entry_set != @stored_entries || @create == ::Zip::File::CREATE
+      @comment != @stored_comment || @entry_set != @stored_entries || @create
     end
 
     # Searches for entry with the specified name. Returns nil if
@@ -403,27 +402,18 @@ module Zip
     end
 
     def on_success_replace
-      tmp_filename = create_tmpname
+      dirname, basename = ::File.split(name)
+      ::Dir::Tmpname.create(basename, dirname) do |tmp_filename|
+        begin
           if yield tmp_filename
             ::File.rename(tmp_filename, name)
-        ::File.chmod(@file_permissions, name) if defined?(@file_permissions)
+            ::File.chmod(@file_permissions, name) unless @create
           end
         ensure
           ::File.unlink(tmp_filename) if ::File.exist?(tmp_filename)
         end
-
-    def create_tmpname
-      dirname, basename = ::File.split(name)
-      ::Dir::Tmpname.create(basename, dirname) do |tmpname|
-        opts = {perm: 0600, mode: ::File::CREAT | ::File::WRONLY | ::File::EXCL}
-        f = File.open(tmpname, opts)
-        f.close
       end
     end
-
-    def create_file_permissions
-      ::Zip::RUNNING_ON_WINDOWS ? 0644 : 0666 - ::File.umask
-    end
   end
 end
 

test/file_permissions_test.rb

@@ -2,58 +2,58 @@ require 'test_helper'
 
 class FilePermissionsTest < MiniTest::Test
 
-  FILENAME = File.join(File.dirname(__FILE__), "umask.zip")
+  ZIPNAME = File.join(File.dirname(__FILE__), "umask.zip")
+  FILENAME = File.join(File.dirname(__FILE__), "umask.txt")
 
   def teardown
+    ::File.unlink(ZIPNAME)
     ::File.unlink(FILENAME)
   end
 
-  if ::Zip::RUNNING_ON_WINDOWS
-    # Windows tests
-
-    DEFAULT_PERMS = 0644
-
-    def test_windows_perms
-      create_file
-
-      assert_equal DEFAULT_PERMS, ::File.stat(FILENAME).mode
-    end
-
-  else
-    # Unix tests
-
-    DEFAULT_PERMS = 0100666
-
   def test_current_umask
-      umask = DEFAULT_PERMS - ::File.umask
+    create_files
-      create_file
+    assert_matching_permissions FILENAME, ZIPNAME
-
-      assert_equal umask, ::File.stat(FILENAME).mode
   end
 
   def test_umask_000
     set_umask(0000) do
-        create_file
+      create_files
     end
 
-      assert_equal DEFAULT_PERMS, ::File.stat(FILENAME).mode
+    assert_matching_permissions FILENAME, ZIPNAME
   end
 
   def test_umask_066
-      umask = 0066
+    set_umask(0066) do
-      set_umask(umask) do
+      create_files
-        create_file
     end
 
-      assert_equal((DEFAULT_PERMS - umask), ::File.stat(FILENAME).mode)
+    assert_matching_permissions FILENAME, ZIPNAME
   end
 
+  def test_umask_027
+    set_umask(0027) do
+      create_files
     end
 
-  def create_file
+    assert_matching_permissions FILENAME, ZIPNAME
-    ::Zip::File.open(FILENAME, ::Zip::File::CREATE) do |zip|
+  end
+
+  def assert_matching_permissions(expected_file, actual_file)
+    assert_equal(
+      ::File.stat(expected_file).mode.to_s(8).rjust(4, '0'),
+      ::File.stat(actual_file).mode.to_s(8).rjust(4, '0')
+    )
+  end
+
+  def create_files
+    ::Zip::File.open(ZIPNAME, ::Zip::File::CREATE) do |zip|
       zip.comment = "test"
     end
+
+    ::File.open(FILENAME, 'w') do |file|
+      file << 'test'
+    end
   end
 
   # If anything goes wrong, make sure the umask is restored.

test/file_test.rb

@@ -41,6 +41,20 @@ class ZipFileTest < MiniTest::Test
     assert_equal(2, zfRead.entries.length)
   end
 
+  def test_create_from_scratch_with_old_create_parameter
+    comment = 'a short comment'
+
+    zf = ::Zip::File.new(EMPTY_FILENAME, 1)
+    zf.get_output_stream('myFile') { |os| os.write 'myFile contains just this' }
+    zf.mkdir('dir1')
+    zf.comment = comment
+    zf.close
+
+    zfRead = ::Zip::File.new(EMPTY_FILENAME)
+    assert_equal(comment, zfRead.comment)
+    assert_equal(2, zfRead.entries.length)
+  end
+
   def test_get_output_stream
     entryCount = nil
     ::Zip::File.open(TEST_ZIP.zip_name) do |zf|