From f40f36a53e78e1bb2afa5baa76c1bd1d35d97c5b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0imon=20Luka=C5=A1=C3=ADk?= <isimluk@fedoraproject.org>
Date: Fri, 3 Jun 2016 11:02:50 +0200
Subject: [PATCH 1/2] AnonFTP: Create directory structure by descending down

https://bugzilla.redhat.com/show_bug.cgi?id=1341502

The problem is that we cannot rely on fpt.nlst to give exception when
file is not available. With vsftpd 3.0.2-14.fc23 and Ruby-2.2.0 Net::FTP
I get behaviour like the following

    ftp.nlst('/')
    => ["/incoming", "/pub"]
    ftp.nlst('/incomming')
    => []
    ftp.nlst('/non-existent')
    => []
---
 app/models/file_depot_ftp.rb | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/app/models/file_depot_ftp.rb b/app/models/file_depot_ftp.rb
index 4a1d29e1f7a..0e300d0522c 100644
--- a/app/models/file_depot_ftp.rb
+++ b/app/models/file_depot_ftp.rb
@@ -93,12 +93,15 @@ def file_exists?(file_or_directory)
   private
 
   def create_directory_structure(directory_path)
-    Pathname.new(directory_path).descend do |path|
-      next if file_exists?(path)
-
-      _log.info("creating #{path}")
-      ftp.mkdir(path.to_s)
+    pwd = ftp.pwd
+    directory_path.to_s.split('/').each do |directory|
+      unless ftp.nlst.include?(directory)
+        _log.info("creating #{directory}")
+        ftp.mkdir(directory)
+      end
+      ftp.chdir(directory)
     end
+    ftp.chdir(pwd)
   end
 
   def upload(source, destination)

From f87471adff09ee5316165ae906fc8e3705796868 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0imon=20Luka=C5=A1=C3=ADk?= <isimluk@fedoraproject.org>
Date: Fri, 3 Jun 2016 14:53:43 +0200
Subject: [PATCH 2/2] Start mocking specific behavior of remote ftp servers

I feel we need some better mocking then just rspec's double. There are
differences between various ftp servers and we keep touching
FileDepotFtp code as we see saw these differences comming.

The tests based on rspec's double are not the useful in this case
as they need to be rewritten each time we change the code logic. At that
point we may re-introduce bugs for certain ftp servers.

Let's build the knowledge about specific servers behavior in the spec.

https://bugzilla.redhat.com/show_bug.cgi?id=1341502
---
 spec/models/file_depot_ftp_spec.rb | 76 ++++++++++++++++++++++++++----
 1 file changed, 67 insertions(+), 9 deletions(-)

diff --git a/spec/models/file_depot_ftp_spec.rb b/spec/models/file_depot_ftp_spec.rb
index 326fc3a9c77..1b8025335c5 100644
--- a/spec/models/file_depot_ftp_spec.rb
+++ b/spec/models/file_depot_ftp_spec.rb
@@ -29,17 +29,28 @@
   end
 
   context "#upload_file" do
-    it "does not already exist" do
-      expect(file_depot_ftp).to receive(:connect).and_return(connection)
-      expect(file_depot_ftp).to receive(:file_exists?).exactly(4).times.and_return(false)
-      expect(connection).to receive(:mkdir).with("uploads")
-      expect(connection).to receive(:mkdir).with("uploads/#{@zone.name}_#{@zone.id}")
-      expect(connection).to receive(:mkdir).with("uploads/#{@zone.name}_#{@zone.id}/#{@miq_server.name}_#{@miq_server.id}")
-      expect(connection).to receive(:putbinaryfile)
-      expect(log_file).to receive(:post_upload_tasks)
-      expect(connection).to receive(:close)
+    it 'uploads file to vsftpd with existing directory structure' do
+      vsftpd = VsftpdMock.new('uploads' =>
+                              {"#{@zone.name}_#{@zone.id}" =>
+                              {"#{@miq_server.name}_#{@miq_server.id}" => {}}})
+      expect(file_depot_ftp).to receive(:connect).and_return(vsftpd)
+      file_depot_ftp.upload_file(log_file)
+      expect(vsftpd.content).to eq('uploads' =>
+                                   {"#{@zone.name}_#{@zone.id}" =>
+                                   {"#{@miq_server.name}_#{@miq_server.id}" =>
+                                   {"Current_region_unknown_#{@zone.name}_#{@zone.id}_#{@miq_server.name}_#{@miq_server.id}_unknown_unknown.txt" =>
+                                   log_file.local_file}}})
+    end
 
+    it 'uploads file to vsftpd with empty /uploads directory' do
+      vsftpd = VsftpdMock.new('uploads' => {})
+      expect(file_depot_ftp).to receive(:connect).and_return(vsftpd)
       file_depot_ftp.upload_file(log_file)
+      expect(vsftpd.content).to eq('uploads' =>
+                                   {"#{@zone.name}_#{@zone.id}" =>
+                                   {"#{@miq_server.name}_#{@miq_server.id}" =>
+                                   {"Current_region_unknown_#{@zone.name}_#{@zone.id}_#{@miq_server.name}_#{@miq_server.id}_unknown_unknown.txt" =>
+                                   log_file.local_file}}})
     end
 
     it "already exists" do
@@ -53,4 +64,51 @@
       file_depot_ftp.upload_file(log_file)
     end
   end
+
+  class FtpMock
+    attr_reader :pwd, :content
+    def initialize(content = {})
+      @pwd = '/'
+      @content = content
+    end
+
+    def chdir(dir)
+      newpath = (Pathname.new(pwd) + dir).to_s
+      if local(newpath).kind_of? Hash
+        @pwd = newpath
+      end
+    end
+
+    private
+
+    def local(path)
+      local = @content
+      path.split('/').each do |dir|
+        next if dir.empty?
+        local = local[dir]
+        raise Net::FTPPermError, '550 Failed to change directory.' if local.nil?
+      end
+      local
+    end
+  end
+
+  class VsftpdMock < FtpMock
+    def nlst(path = '')
+      l = local(pwd + path)
+      l.respond_to?(:keys) ? l.keys : []
+    rescue
+      return []
+    end
+
+    def mkdir(dir)
+      l = local(pwd)
+      l[dir] = {}
+    end
+
+    def putbinaryfile(local_path, remote_path)
+      dir, base = Pathname.new(remote_path).split
+      l = local(dir.to_s)
+      l[base.to_s] = local_path
+    end
+  end
 end