Merge pull request #36 from dentarg/fix-permissions-tracking

Only flip group writable bit if permissions differ

Author: strzibny

lib/rubygems/comparator/monitor.rb

@@ -47,7 +47,7 @@ def self.files_permissions_changes(prev_file, curr_file, ignore_group_writable=f
       curr_permissions = File.stat(curr_file).mode
 
       diff = prev_permissions ^ curr_permissions
-      diff ^= 020 if ignore_group_writable
+      diff ^= 020 if diff != 0 && ignore_group_writable
 
       if diff != 0
         "  (!) New permissions: " +
@@ -59,13 +59,21 @@ def self.files_permissions_changes(prev_file, curr_file, ignore_group_writable=f
 
     def self.new_file_permissions(file, ignore_group_writable=false)
       file_permissions = File.stat(file).mode
-      formatted_file_permissions = format_permissions(file_permissions)
 
-      file_permissions ^= 020 if ignore_group_writable
+      expected_permissions = if DirUtils.gem_bin_file?(file)
+        0100755
+      else
+        0100644
+      end
+
+      permissions_to_compare = if ignore_group_writable && file_permissions != expected_permissions
+        file_permissions ^ 020
+      else
+        file_permissions
+      end
 
-      unless file_permissions == 0100644 || \
-          (DirUtils.gem_bin_file?(file) && file_permissions == 0100755)
-        return "  (!) Unexpected permissions: #{formatted_file_permissions}"
+      unless permissions_to_compare == expected_permissions
+        return "  (!) Unexpected permissions: #{format_permissions(file_permissions)}"
       end
       ''
     end

test/rubygems/comparator/test_file_list_comparator.rb

@@ -9,9 +9,10 @@ def test_files_comparison
     assert_equal [], @report['files']['0.0.1->0.0.2']['deleted'].messages
     assert_equal [], @report['files']['0.0.1->0.0.2']['updated'].messages
     assert_equal "bin/lorem +3/-0", @report['files']['0.0.2->0.0.3']['added'].lines(1)
-    assert_equal "(!) File is not executable", @report['files']['0.0.2->0.0.3']['added'].lines(2).strip
-    assert_equal "(!) Shebang found: #!/usr/bin/ruby", @report['files']['0.0.2->0.0.3']['added'].lines(3).strip
-    assert_equal "lib/lorem/utils.rb +7/-0", @report['files']['0.0.2->0.0.3']['added'].lines(4).strip
+    assert_equal "(!) Unexpected permissions: 100664", @report['files']['0.0.2->0.0.3']['added'].lines(2).strip
+    assert_equal "(!) File is not executable", @report['files']['0.0.2->0.0.3']['added'].lines(3).strip
+    assert_equal "(!) Shebang found: #!/usr/bin/ruby", @report['files']['0.0.2->0.0.3']['added'].lines(4).strip
+    assert_equal "lib/lorem/utils.rb +7/-0", @report['files']['0.0.2->0.0.3']['added'].lines(5).strip
     assert_nil @report['files']['0.0.2->0.0.3']['added'].lines(6)
     assert_equal [], @report['files']['0.0.2->0.0.3']['deleted'].messages
     assert_equal [], @report['files']['0.0.2->0.0.3']['updated'].messages

test/rubygems/comparator/test_monitor.rb

@@ -49,6 +49,21 @@ def test_files_permissions_changes
     file1 = File.join(@v003, 'bin/lorem')
     file2 = File.join(@v004, 'bin/lorem')
     assert_equal '(!) New permissions: 100664 -> 100775', Gem::Comparator::Monitor.files_permissions_changes(file1, file2).strip 
+    assert_equal '(!) New permissions: 100664 -> 100775', Gem::Comparator::Monitor.files_permissions_changes(file1, file2, true).strip
+  end
+
+  def test_files_permissions_changes_no_change
+    file1 = Tempfile.new
+    file2 = Tempfile.new
+    begin
+      File.chmod(0644, file1)
+      File.chmod(0644, file2)
+      assert_equal '', Gem::Comparator::Monitor.files_permissions_changes(file1.path, file2.path)
+      assert_equal '', Gem::Comparator::Monitor.files_permissions_changes(file1.path, file2.path, true)
+    ensure
+      file1.unlink
+      file2.unlink
+    end
   end
 
   def test_files_permissions_changes_ignores_group_writable_added
@@ -94,16 +109,39 @@ def test_new_file_permissions
     file1 = File.join(@v004, 'bin/lorem')
     file2 = File.join(@v004, 'lib/lorem.rb')
     assert_equal '(!) Unexpected permissions: 100775', Gem::Comparator::Monitor.new_file_permissions(file1).strip 
-    assert_equal '(!) Unexpected permissions: 100664', Gem::Comparator::Monitor.new_file_permissions(file2).strip 
+    assert_equal '(!) Unexpected permissions: 100664', Gem::Comparator::Monitor.new_file_permissions(file2).strip
+    ignore_group_writable = true
+    assert_equal '', Gem::Comparator::Monitor.new_file_permissions(file1, ignore_group_writable).strip
+    assert_equal '', Gem::Comparator::Monitor.new_file_permissions(file2, ignore_group_writable).strip
+  end
+
+  def test_new_file_permissions_ignore_group_writable_when_not_group_writable
+    file = Tempfile.new
+    bin_file = temp_bin_file
+    begin
+      File.chmod(0644, file)
+      assert_equal '', Gem::Comparator::Monitor.new_file_permissions(file.path, true)
+
+      File.chmod(0755, bin_file)
+      assert_equal '', Gem::Comparator::Monitor.new_file_permissions(bin_file.path, true)
+    ensure
+      file.unlink
+      bin_file.unlink
+    end
   end
 
   def test_new_file_permissions_ignore_group_writable
     file = Tempfile.new
+    bin_file = temp_bin_file
     begin
       File.chmod(0664, file)
-      assert_equal '', Gem::Comparator::Monitor.new_file_permissions(file, true)
+      assert_equal '', Gem::Comparator::Monitor.new_file_permissions(file.path, true)
+
+      File.chmod(0775, bin_file)
+      assert_equal '', Gem::Comparator::Monitor.new_file_permissions(bin_file.path, true)
     ensure
       file.unlink
+      bin_file.unlink
     end
   end
 

test/test_helper.rb

@@ -11,6 +11,12 @@ def setup_file_permissions
   File.chmod(0775, File.join(gemfiles_path, 'lorem-0.0.4', 'bin', 'lorem'))
 end
 
+def temp_bin_file
+  bin_dir = File.join(Dir.mktmpdir, "bin")
+  Dir.mkdir(bin_dir)
+  Tempfile.new("", bin_dir)
+end
+
 class TestGemComparator < Minitest::Test
   def setup
     super