Merge pull request ManageIQ#211 from ailisp/bug-1430701

Fix bug: existed v2_key deleted even if fetch/create new v2_key fails

lib/gems/pending/appliance_console/key_configuration.rb

@@ -6,7 +6,8 @@
 
 module ApplianceConsole
   CERT_DIR = ENV['KEY_ROOT'] || RAILS_ROOT.join("certs")
-  KEY_FILE = "#{CERT_DIR}/v2_key"
+  KEY_FILE = "#{CERT_DIR}/v2_key".freeze
+  NEW_KEY_FILE = "#{KEY_FILE}.tmp".freeze
 
   class KeyConfiguration
     attr_accessor :host, :login, :password, :key_path, :action, :force
@@ -45,11 +46,12 @@ def ask_question_loop
     end
 
     def activate
-      if remove_key(force)
-        if fetch_key?
-          fetch_key
+      if !key_exist? || force
+        if get_new_key
+          save_new_key
         else
-          create_key
+          remove_new_key_if_any
+          false
         end
       else
         # probably only got here via the cli
@@ -63,6 +65,17 @@ def activate
       end
     end
 
+    def save_new_key
+      FileUtils.mv(NEW_KEY_FILE, KEY_FILE, :force => true)
+    rescue => e
+      say("Failed to overwrite original key, original key kept. #{e.message}")
+      return false
+    end
+
+    def remove_new_key_if_any
+      FileUtils.rm(NEW_KEY_FILE) if File.exist?(NEW_KEY_FILE)
+    end
+
     def key_exist?
       File.exist?(KEY_FILE)
     end
@@ -72,15 +85,15 @@ def fetch_key?
     end
 
     def create_key
-      MiqPassword.generate_symmetric(KEY_FILE) && true
+      MiqPassword.generate_symmetric(NEW_KEY_FILE) && true
     end
 
     def fetch_key
       # use :verbose => 1 (or :debug for later versions) to see actual errors
       Net::SCP.start(host, login, :password => password) do |scp|
-        scp.download!(key_path, KEY_FILE)
+        scp.download!(key_path, NEW_KEY_FILE)
       end
-      File.exist?(KEY_FILE)
+      File.exist?(NEW_KEY_FILE)
     rescue => e
       say("Failed to fetch key: #{e.message}")
       false
@@ -93,10 +106,8 @@ def ask_for_action(default_action)
       ask_with_menu("Encryption Key", options, default_action, false)
     end
 
-    # return true if key is gone, otherwise false (and we should probably abort)
-    # throws an exception if rm fails e.g.: Errno::EACCES
-    def remove_key(force)
-      !key_exist? || (force && FileUtils.rm(KEY_FILE))
+    def get_new_key
+      fetch_key? ? fetch_key : create_key
     end
   end
 end

spec/appliance_console/key_configuration_spec.rb

@@ -64,13 +64,15 @@
           v2_exists(false) # before download
           v2_exists(true)  # after downloaded
           expect(Net::SCP).to receive(:start).with(host, "root", :password => password)
+          expect(FileUtils).to receive(:mv).with(/v2_key\.tmp/, /v2_key$/, :force=>true).and_return(true)
           expect(subject.activate).to be_truthy
         end
 
         it "creates key" do
           subject.action = :create
           v2_exists(false)
           expect(MiqPassword).to receive(:generate_symmetric).and_return(154)
+          expect(FileUtils).to receive(:mv).with(/v2_key\.tmp/, /v2_key$/, :force=>true).and_return(true)
           expect(subject.activate).to be_truthy
         end
       end
@@ -80,21 +82,36 @@
           subject.force = true
           v2_exists(true) # before downloaded
           v2_exists(true) # after downloaded
-          expect(FileUtils).to receive(:rm).with(/v2_key/).and_return(["v2_key"])
           scp = double('scp')
           expect(scp).to receive(:download!).with(subject.key_path, /v2_key/).and_return(:result)
           expect(Net::SCP).to receive(:start).with(host, "root", :password => password).and_yield(scp).and_return(true)
+          expect(FileUtils).to receive(:mv).with(/v2_key\.tmp/, /v2_key$/, :force=>true).and_return(true)
           expect(subject.activate).to be_truthy
         end
 
         it "fails if key exists (no force)" do
           expect($stderr).to receive(:puts).at_least(2).times
           subject.force = false
           v2_exists(true)
-          expect(FileUtils).not_to receive(:rm)
+          expect(FileUtils).not_to receive(:mv)
           expect(Net::SCP).not_to receive(:start)
           expect(subject.activate).to be_falsey
         end
+
+        it "keeps original v2_key if fetch new fails" do
+          subject.force = true
+          key_content = "The v2_key is abc"
+          mock_key = Tempfile.new('v2_key')
+          mock_key.print(key_content)
+          mock_key.close
+          stub_const("ApplianceConsole::KEY_FILE", mock_key.path)
+          stub_const("ApplianceConsole::NEW_KEY_FILE", mock_key.path + ".tmp")
+          expect(subject).to receive(:fetch_key).and_return(false)
+          expect(subject.activate).to be_falsey
+          expect(FileUtils).not_to receive(:mv)
+          expect(File.open(ApplianceConsole::KEY_FILE).read).to eq(key_content)
+          mock_key.unlink
+        end
       end
     end
   end