Merge pull request #10084 from chrisroberts/f-host-nfs-sudo

Fix sudo usage in exports write within linux host cap

plugins/hosts/linux/cap/nfs.rb

@@ -173,16 +173,14 @@ module VagrantPlugins
         def self.nfs_write_exports(new_exports_content)
           if(nfs_exports_content != new_exports_content.strip)
             begin
+              exports_path = Pathname.new(NFS_EXPORTS_PATH)
+
               # Write contents out to temporary file
               new_exports_file = Tempfile.create('vagrant')
               new_exports_file.puts(new_exports_content)
               new_exports_file.close
               new_exports_path = new_exports_file.path
 
-              # Only use "sudo" if we can't write to /etc/exports directly
-              sudo_command = ""
-              sudo_command = "sudo " if !File.writable?(NFS_EXPORTS_PATH)
-
               # Ensure new file mode and uid/gid match existing file to replace
               existing_stat = File.stat(NFS_EXPORTS_PATH)
               new_stat = File.stat(new_exports_path)
@@ -190,7 +188,7 @@ module VagrantPlugins
                 File.chmod(existing_stat.mode, new_exports_path)
               end
               if existing_stat.uid != new_stat.uid || existing_stat.gid != new_stat.gid
-                chown_cmd = "#{sudo_command}chown #{existing_stat.uid}:#{existing_stat.gid} #{new_exports_path}"
+                chown_cmd = "sudo chown #{existing_stat.uid}:#{existing_stat.gid} #{new_exports_path}"
                 result = Vagrant::Util::Subprocess.execute(*Shellwords.split(chown_cmd))
                 if result.exit_code != 0
                   raise Vagrant::Errors::NFSExportsFailed,
@@ -200,6 +198,7 @@ module VagrantPlugins
                 end
               end
               # Always force move the file to prevent overwrite prompting
+              sudo_command = "sudo " if !exports_path.writable? || !exports_path.dirname.writable?
               mv_cmd = "#{sudo_command}mv -f #{new_exports_path} #{NFS_EXPORTS_PATH}"
               result = Vagrant::Util::Subprocess.execute(*Shellwords.split(mv_cmd))
               if result.exit_code != 0

test/unit/plugins/hosts/linux/cap/nfs_test.rb

@@ -269,14 +269,72 @@ EOH
       expect{ described_class.nfs_write_exports("new content") }.to raise_error(Vagrant::Errors::NFSExportsFailed)
     end
 
-    it "should retain existing file owner and group IDs" do
-      pending("investigate using a simulated FS to test")
-      test_with_simulated_fs
-    end
+    context "exports file modification" do
+      let(:tmp_stat) { double("tmp_stat", uid: 100, gid: 100, mode: tmp_mode) }
+      let(:tmp_mode) { 0 }
+      let(:exports_stat) { double("stat", uid: exports_uid, gid: exports_gid, mode: exports_mode) }
+      let(:exports_uid) { -1 }
+      let(:exports_gid) { -1 }
+      let(:exports_mode) { 0 }
+      let(:new_exports_file) { double("new_exports_file", path: "/dev/null/exports") }
 
-    it "should raise custom exception when chown fails" do
-      pending("investigate using a simulated FS to test")
-      test_with_simulated_fs
+      before do
+        allow(File).to receive(:stat).with(new_exports_file.path).and_return(tmp_stat)
+        allow(File).to receive(:stat).with(tmp_exports_path.to_s).and_return(exports_stat)
+        allow(new_exports_file).to receive(:puts)
+        allow(new_exports_file).to receive(:close)
+        allow(Vagrant::Util::Subprocess).to receive(:execute).and_return(Vagrant::Util::Subprocess::Result.new(0, "", ""))
+        allow(Tempfile).to receive(:create).with("vagrant").and_return(new_exports_file)
+      end
+
+      it "should retain existing file owner and group IDs" do
+        expect(Vagrant::Util::Subprocess).to receive(:execute) { |*args|
+          expect(args).to include("sudo")
+          expect(args).to include("chown")
+        }.and_return(Vagrant::Util::Subprocess::Result.new(0, "", ""))
+        described_class.nfs_write_exports("new content")
+      end
+
+      it "should raise custom exception when chown fails" do
+        expect(Vagrant::Util::Subprocess).to receive(:execute) { |*args|
+          expect(args).to include("sudo")
+          expect(args).to include("chown")
+        }.and_return(Vagrant::Util::Subprocess::Result.new(1, "", ""))
+        expect { described_class.nfs_write_exports("new content") }.to raise_error(Vagrant::Errors::NFSExportsFailed)
+      end
+
+      context "when user has write access to exports file" do
+        let(:file_writable?) { true }
+        let(:dir_writable?) { false }
+        let(:exports_pathname) { double("exports_pathname", writable?: file_writable?, dirname: exports_dir_pathname) }
+        let(:exports_dir_pathname) { double("exports_dir_pathname", writable?: dir_writable?) }
+
+        before do
+          allow(File).to receive(:stat).and_return(exports_stat)
+          allow(File).to receive(:exist?).and_return(false)
+          allow(Pathname).to receive(:new).with(tmp_exports_path.to_s).and_return(exports_pathname)
+        end
+
+        it "should use sudo when moving new file" do
+          expect(Vagrant::Util::Subprocess).to receive(:execute) { |*args|
+            expect(args).to include("sudo")
+            expect(args).to include("mv")
+          }.and_return(Vagrant::Util::Subprocess::Result.new(0, "", ""))
+          described_class.nfs_write_exports("new content")
+        end
+
+        context "and write access to exports parent directory" do
+          let(:dir_writable?) { true }
+
+          it "should not use sudo when moving new file" do
+            expect(Vagrant::Util::Subprocess).to receive(:execute) { |*args|
+              expect(args).not_to include("sudo")
+              expect(args).to include("mv")
+            }.and_return(Vagrant::Util::Subprocess::Result.new(0, "", ""))
+            described_class.nfs_write_exports("new content")
+          end
+        end
+      end
     end
   end
 end