Finishing off the sftp unit tests. Added some checks to the sftp provider to maintain the precedent set by the ftp provider

3 files changed, 133 insertions, 19 deletions

diff --git a/lib/chef/provider/remote_file/fetcher.rb b/lib/chef/provider/remote_file/fetcher.rb
index c138ee5d6b..563d135d6a 100644
--- a/lib/chef/provider/remote_file/fetcher.rb
+++ b/lib/chef/provider/remote_file/fetcher.rb
@@ -32,7 +32,7 @@ class Chef
             when "ftp"
               Chef::Provider::RemoteFile::FTP.new(uri, new_resource, current_resource)
             when "sftp"
-              Chef::Provider::RemoteFile::SFTP.net(uri, new_resource, current_resource)
+              Chef::Provider::RemoteFile::SFTP.new(uri, new_resource, current_resource)
             when "file"
               Chef::Provider::RemoteFile::LocalFile.new(uri, new_resource, current_resource)
             else
diff --git a/lib/chef/provider/remote_file/sftp.rb b/lib/chef/provider/remote_file/sftp.rb
index 4e2c0d2c9a..680e69c675 100644
--- a/lib/chef/provider/remote_file/sftp.rb
+++ b/lib/chef/provider/remote_file/sftp.rb
@@ -36,26 +36,19 @@ class Chef
           @new_resource = new_resource
           @current_resource = current_resource
           validate_path!
+          validate_userinfo!
         end
 
         def hostname
           @uri.host
         end
 
-        def user
-          if uri.userinfo
-            URI.unescape(uri.user)
-          else
-            'anonymous'
-          end
+        def port
+          @uri.port
         end
 
-        def pass
-          if uri.userinfo
-            URI.unescape(uri.password)
-          else
-            nil
-          end
+        def user
+          URI.unescape(uri.user)
         end
 
         def filename
@@ -67,16 +60,34 @@ class Chef
           get
         end
 
+        private
+
         def sftp
-          @sftp ||= Net::SFTP.start(hostname, user, :password => pass)
+          host = port ? "#{hostname}:#{port}" : hostname
+          @sftp ||= Net::SFTP.start(host, user, :password => pass)
         end
 
-        private
+        def pass
+          URI.unescape(uri.password)
+        end
 
         def validate_path!
           parse_path
         end
 
+        def validate_userinfo!
+          if uri.userinfo
+            if !(uri.user)
+              raise ArgumentError, "no user name provided in the sftp URI"
+            end
+            if !(uri.password)
+              raise ArgumentError, "no password provided in the sftp URI"
+            end
+          else
+            raise ArgumentError, "no userinfo provided in the sftp URI"
+          end
+        end
+
         # Fetches using Net::FTP, returns a Tempfile with the content
         def get
           tempfile = Chef::FileContentManagement::Tempfile.new(@new_resource).tempfile
diff --git a/spec/unit/provider/remote_file/sftp_spec.rb b/spec/unit/provider/remote_file/sftp_spec.rb
index 2cc473ae6b..c030ff83c1 100644
--- a/spec/unit/provider/remote_file/sftp_spec.rb
+++ b/spec/unit/provider/remote_file/sftp_spec.rb
@@ -28,23 +28,126 @@ describe Chef::Provider::RemoteFile::SFTP do
   }
 
   let(:new_resource) do
-    r = Chef::Resource::RemoteFile.new("remote file ftp backend test (new resource)")
+    r = Chef::Resource::RemoteFile.new("remote file sftp backend test (new resource)")
     r.path(resource_path)
     r
   end
 
   let(:current_resource) do
-    Chef::Resource::RemoteFile.new("remote file ftp backend test (current resource)'")
+    Chef::Resource::RemoteFile.new("remote file sftp backend test (current resource)'")
   end
 
-  let(:uri) { URI.parse("sftp://opscode.com/seattle.txt") }
 
-  describe "on initialization" do
+
+  let(:uri) { URI.parse("sftp://conan:cthu1hu@opscode.com/seattle.txt") }
+
+  let(:sftp) do
+    sftp = double(Net::SFTP, {})
+    allow(sftp).to receive(:download!)
+    sftp
+  end
+
+  let(:tempfile_path) { "/tmp/somedir/remote-file-sftp-backend-spec-test" }
+
+  let(:tempfile) do
+    t = StringIO.new
+    allow(t).to receive(:path).and_return(tempfile_path)
+    t
+  end
+
+  before(:each) do
+    allow(Net::SFTP).to receive(:start).with(any_args).and_return(sftp)
+    allow(Tempfile).to receive(:new).and_return(tempfile)
+  end
+  describe "on initialization without user and password provided in the URI" do
+    it "throws an argument exception with no userinfo is given" do
+      uri.userinfo = nil
+      uri.password = nil
+      uri.user = nil
+      expect { Chef::Provider::RemoteFile::SFTP.new(uri, new_resource, current_resource) }.to raise_error(ArgumentError)
+    end
+
+    it "throws an argument exception with no user name is given" do
+      uri.userinfo = ":cthu1hu"
+      uri.password = "cthu1hu"
+      uri.user = nil
+      expect { Chef::Provider::RemoteFile::SFTP.new(uri, new_resource, current_resource) }.to raise_error(ArgumentError)
+    end
+
+    it "throws an argument exception with no password is given" do
+      uri.userinfo = "conan:"
+      uri.password = nil
+      uri.user = "conan"
+      expect { Chef::Provider::RemoteFile::SFTP.new(uri, new_resource, current_resource) }.to raise_error(ArgumentError)
+    end
+
+  end
+
+  describe "on initialization with user and password provided in the URI" do
 
     it "throws an argument exception when no path is given" do
       uri.path = ""
       expect { Chef::Provider::RemoteFile::SFTP.new(uri, new_resource, current_resource) }.to raise_error(ArgumentError)
     end
 
+    it "throws an argument exception when only a / is given" do
+      uri.path = "/"
+      expect { Chef::Provider::RemoteFile::SFTP.new(uri, new_resource, current_resource) }.to raise_error(ArgumentError)
+    end
+
+    it "throws an argument exception when no filename is given" do
+      uri.path = "/the/whole/path/"
+      expect { Chef::Provider::RemoteFile::SFTP.new(uri, new_resource, current_resource) }.to raise_error(ArgumentError)
+    end
+
+
+  end
+
+  describe "when fetching the object" do
+
+    let(:cache_control_data) { Chef::Provider::RemoteFile::CacheControlData.new(uri) }
+    let(:current_resource_checksum) { "e2a8938cc31754f6c067b35aab1d0d4864272e9bf8504536ef3e79ebf8432305" }
+
+    subject(:fetcher) { Chef::Provider::RemoteFile::SFTP.new(uri, new_resource, current_resource) }
+
+    before do
+      current_resource.checksum(current_resource_checksum)
+    end
+
+    it "should attempt to download a file from the provided url and path" do
+      expect(sftp).to receive(:download!).with("/seattle.txt", "/tmp/somedir/remote-file-sftp-backend-spec-test")
+      fetcher.fetch
+    end
+
+    context "and the URI specifies an alternate port" do
+      let(:uri) { URI.parse("ftp://conan:cthu1hu@opscode.com:8021/seattle.txt") }
+
+      it "should connect on an alternate port when one is provided" do
+        expect(Net::SFTP).to receive(:start).with("opscode.com:8021", "conan", :password => "cthu1hu")
+        fetcher.fetch
+      end
+
+    end
+
+    context "and the uri specifies a nested path" do
+      let(:uri) { URI.parse("ftp://conan:cthu1hu@opscode.com/the/whole/path/seattle.txt") }
+
+      it "should fetch the file from the correct path" do
+        expect(sftp).to receive(:download!).with("the/whole/path/seattle.txt", "/tmp/somedir/remote-file-sftp-backend-spec-test")
+        fetcher.fetch
+      end
+    end
+
+    context "when not using last modified based conditional fetching" do
+      before do
+        new_resource.use_last_modified(false)
+      end
+
+      it "should return a tempfile in the result" do
+        result = fetcher.fetch
+        expect(result).to equal(tempfile)
+      end
+
+    end
   end
 end