Ensure missing credentials are populated (#4)

This change ensures that cached credentials are only used if they have a
defined user and password. This is necessary in situations where the
cached credential may have been incorrectly populated prior to usage in
an SSH or WinRM connection.

CHANGELOG.md

@@ -1,5 +1,8 @@
 ## [Unreleased]
 
+## [0.2.2] - 2021-03-23
+* Ensure both user and password are populated before using a cached credential
+
 ## [0.2.1] - 2021-03-15
 * Fix connection pooling metadata sharing
 * Fix caching of pooled metadata

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    remotus (0.2.1)
+    remotus (0.2.2)
       connection_pool (~> 2.2)
       net-scp (~> 3.0)
       net-ssh (~> 6.1)

lib/remotus/auth.rb

@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
+require "remotus"
 require "remotus/auth/credential"
 require "remotus/auth/store"
 require "remotus/auth/hash_store"
@@ -17,15 +18,12 @@ module Auth
     # @return [Remotus::Auth::Credential] found credential
     #
     def self.credential(connection, **options)
-      return cache[connection.host] if cache.key?(connection.host)
+      # Only return cached credentials that have a populated user and password, otherwise attempt retrieval
+      return cache[connection.host] if cache.key?(connection.host) && cache[connection.host].user && cache[connection.host].password
+
+      found_credential = credential_from_stores(connection, **options)
+      return found_credential if found_credential
 
-      stores.each do |store|
-        host_cred = store.credential(connection, **options)
-        if host_cred
-          cache[connection.host] = host_cred
-          return host_cred
-        end
-      end
       raise Remotus::MissingCredential, "Could not find credential for #{connection.host} in any credential store (#{stores.join(", ")})."
     end
 
@@ -62,5 +60,31 @@ def self.stores
     def self.stores=(stores)
       @stores = stores
     end
+
+    class << self
+      private
+
+      #
+      # Gets authentication credentials for the given connection and options from one of the credential stores
+      #
+      # @param [Remotus::SshConnection, Remotus::WinrmConnection, #host] connection remote connection
+      # @param [Hash] options options hash
+      #                       options may be used by different credential stores.
+      #
+      # @return [Remotus::Auth::Credential, nil] found credential or nil if the credential cannot be found
+      #
+      def credential_from_stores(connection, **options)
+        stores.each do |store|
+          Remotus.logger.debug { "Gathering #{connection.host} credentials from #{store} credential store" }
+          host_cred = store.credential(connection, **options)
+          next unless host_cred
+
+          Remotus.logger.debug { "#{connection.host} credentials found in #{store} credential store" }
+          cache[connection.host] = host_cred
+          return host_cred
+        end
+        nil
+      end
+    end
   end
 end

lib/remotus/version.rb

@@ -2,5 +2,5 @@
 
 module Remotus
   # Remotus gem version
-  VERSION = "0.2.1"
+  VERSION = "0.2.2"
 end

spec/remotus/auth_spec.rb

@@ -12,6 +12,33 @@
   end
 
   describe "#credential" do
+    context "when the cache contains a host credential without a username or password" do
+      it "retrieves and caches a new credential" do
+        described_class.cache[host] = described_class::Credential.new(nil, nil)
+        described_class.stores = [hash_store]
+        hash_store.add(connection, cred)
+        expect(described_class.credential(connection)).to eq(cred)
+      end
+    end
+
+    context "when the cache contains a host credential without a username" do
+      it "retrieves and caches a new credential" do
+        described_class.cache[host] = described_class::Credential.new(nil, "password")
+        described_class.stores = [hash_store]
+        hash_store.add(connection, cred)
+        expect(described_class.credential(connection)).to eq(cred)
+      end
+    end
+
+    context "when the cache contains a host credential without a password" do
+      it "retrieves and caches a new credential" do
+        described_class.cache[host] = described_class::Credential.new("diff_user", nil)
+        described_class.stores = [hash_store]
+        hash_store.add(connection, cred)
+        expect(described_class.credential(connection)).to eq(cred)
+      end
+    end
+
     context "when the cache contains the host credential" do
       it "returns the credential" do
         described_class.cache[host] = cred