Break out the gpg key validation into its own methodapt_repo_cleanup

This logic was entirely different than the key validation and doing them in the same block didn't make any sense. This makes it a lot easier to follow as well since there isn't the cmd variable that requires searching the code to see what we're actually running

Signed-off-by: Tim Smith <tsmith@chef.io>

2 files changed, 38 insertions, 8 deletions

diff --git a/lib/chef/provider/apt_repository.rb b/lib/chef/provider/apt_repository.rb
index a8db5088d3..1909ed8034 100644
--- a/lib/chef/provider/apt_repository.rb
+++ b/lib/chef/provider/apt_repository.rb
@@ -29,7 +29,6 @@ class Chef
 
       provides :apt_repository, platform_family: "debian"
 
-      LIST_APT_KEYS = "apt-key list".freeze
       LIST_APT_KEY_FINGERPRINTS = "apt-key adv --list-public-keys --with-fingerprint --with-colons".freeze
 
       def load_current_resource
@@ -120,15 +119,23 @@ class Chef
         end.compact
       end
 
-      # validate the key
-      # @param [String] cmd
+      # see if the keyfile is invalid such as a text file that is not actually a gpg key
+      # @param [String] keyfile the path to the keyfile
+      #
+      # @return [Boolean] is the key file invalid
+      def keyfile_is_invalid?(keyfile)
+        so = shell_out("gpg #{keyfile}")
+        so.error?
+      end
+
+      # validate the key against the apt keystore to see if that version is expired
       # @param [String] key
       #
       # @return [Boolean] is the key valid or not
-      def key_is_valid?(cmd, key)
+      def key_is_valid?(key)
         valid = true
 
-        so = shell_out(cmd)
+        so = shell_out("apt-key list")
         so.stdout.split(/\n/).map do |t|
           if t =~ %r{^\/#{key}.*\[expired: .*\]$}
             Chef::Log.debug "Found expired key: #{t}"
@@ -205,7 +212,7 @@ class Chef
           action :create
         end
 
-        raise "The key #{cached_keyfile} is invalid and cannot be used to verify an apt repository." unless key_is_valid?("gpg #{cached_keyfile}", "")
+        raise "The key #{cached_keyfile} is invalid and cannot be used to verify an apt repository." if keyfile_is_invalid?(cached_keyfile)
 
         declare_resource(:execute, "apt-key add #{cached_keyfile}") do
           sensitive new_resource.sensitive
@@ -248,12 +255,12 @@ class Chef
             present = extract_fingerprints_from_cmd(LIST_APT_KEY_FINGERPRINTS).any? do |fp|
               fp.end_with? key.upcase
             end
-            present && key_is_valid?(LIST_APT_KEYS, key.upcase)
+            present && key_is_valid?(key.upcase)
           end
           notifies :run, "execute[apt-cache gencaches]", :immediately
         end
 
-        raise "The key #{key} is invalid and cannot be used to verify an apt repository." unless key_is_valid?(LIST_APT_KEYS, key.upcase)
+        raise "The key #{key} is invalid and cannot be used to verify an apt repository." unless key_is_valid?(key.upcase)
       end
 
       # @param [String] owner
diff --git a/spec/unit/provider/apt_repository_spec.rb b/spec/unit/provider/apt_repository_spec.rb
index 05eba821dd..ca85831a06 100644
--- a/spec/unit/provider/apt_repository_spec.rb
+++ b/spec/unit/provider/apt_repository_spec.rb
@@ -66,6 +66,17 @@ describe Chef::Provider::AptRepository do
     double("shell_out", stdout: GPG_FINGER, exitstatus: 0, error?: false)
   end
 
+  let(:gpg_shell_out_success) do
+    double("shell_out", :stdout => "pub  2048R/7BD9BF62 2011-08-19 nginx signing key <signing-key@nginx.com>",
+                        :exitstatus => 0, :error? => false)
+  end
+
+  let(:gpg_shell_out_failure) do
+    double("shell_out", :stderr => "gpg: no valid OpenPGP data found.\n
+                                    gpg: processing message failed: eof",
+                        :exitstatus => 1, :error? => true)
+  end
+
   let(:apt_fingerprints) do
     %w{630239CC130E1A7FD81A27B140976EAF437D05B5
 C5986B4F1257FFA86632CBA746181433FBB75451
@@ -240,4 +251,16 @@ C5986B4F1257FFA86632CBA746181433FBB75451
       expect(provider.build_repo("ppa:chef/main", "unstable", "main", false, nil)).to eql(target)
     end
   end
+
+  describe "#keyfile_is_invalid?" do
+    it "returns true if the file is invalid" do
+      expect(provider).to receive(:shell_out).and_return(gpg_shell_out_failure)
+      expect(provider.keyfile_is_invalid?("/foo/bar.key")).to be_truthy
+    end
+
+    it "returns false if the file is valid" do
+      expect(provider).to receive(:shell_out).and_return(gpg_shell_out_success)
+      expect(provider.keyfile_is_invalid?("/foo/bar.key")).to be_falsey
+    end
+  end
 end