[EmbeddedAnsible] Fix edit credentials

There was a bug when editing credentials for EmbeddedAnsible where if you edited an attribute and there are other attributes on the particular credential class' params_to_attributes method which have to be converted, those previously would be `nil`'d out. This fixes that, and modifies tests to confirm that this is now working.
NickLaMuro · Sep 8, 2020 · 7bbdf70 · 7bbdf70
1 parent a3a8e62
commit 7bbdf70
Show file tree

Hide file tree

Showing 10 changed files with 63 additions and 37 deletions.
diff --git a/app/models/manageiq/providers/embedded_ansible/automation_manager/amazon_credential.rb b/app/models/manageiq/providers/embedded_ansible/automation_manager/amazon_credential.rb
@@ -38,7 +38,7 @@ def self.display_name(number = 1)
 
   def self.params_to_attributes(params)
     attrs = params.dup
-    attrs[:auth_key] = attrs.delete(:security_token)
+    attrs[:auth_key] = attrs.delete(:security_token) if attrs.key?(:security_token)
     attrs
   end
 end
diff --git a/app/models/manageiq/providers/embedded_ansible/automation_manager/azure_credential.rb b/app/models/manageiq/providers/embedded_ansible/automation_manager/azure_credential.rb
@@ -55,14 +55,13 @@ def self.display_name(number = 1)
 
   def self.params_to_attributes(params)
     attrs            = params.dup
-    attrs[:auth_key] = attrs.delete(:secret)
+    attrs[:auth_key] = attrs.delete(:secret) if attrs.key?(:secret)
 
     if %i[client tenant subscription].any? {|opt| attrs.has_key? opt }
-      attrs[:options] = {
-        :client       => attrs.delete(:client),
-        :tenant       => attrs.delete(:tenant),
-        :subscription => attrs.delete(:subscription)
-      }
+      attrs[:options]              ||= {}
+      attrs[:options][:client]       = attrs.delete(:client)       if attrs.key?(:client)
+      attrs[:options][:tenant]       = attrs.delete(:tenant)       if attrs.key?(:tenant)
+      attrs[:options][:subscription] = attrs.delete(:subscription) if attrs.key?(:subscription)
     end
 
     attrs

diff --git a/app/models/manageiq/providers/embedded_ansible/automation_manager/credential.rb b/app/models/manageiq/providers/embedded_ansible/automation_manager/credential.rb
@@ -39,13 +39,19 @@ def self.encrypt_queue_params(params)
   end
 
   def raw_update_in_provider(params)
-    update!(self.class.params_to_attributes(params.except(:task_id, :miq_task_id)))
+    update!(params_for_update(params))
   end
 
   def raw_delete_in_provider
     destroy!
   end
 
+  def params_for_update(params)
+    update_params           = params.dup
+    update_params[:options] = options.merge(update_params[:options] || {}) if options
+    self.class.params_to_attributes(update_params.except(:task_id, :miq_task_id))
+  end
+
   def native_ref
     Integer(manager_ref)
   end

diff --git a/app/models/manageiq/providers/embedded_ansible/automation_manager/google_credential.rb b/app/models/manageiq/providers/embedded_ansible/automation_manager/google_credential.rb
@@ -46,7 +46,7 @@ def self.display_name(number = 1)
   def self.params_to_attributes(params)
     attrs = params.dup
 
-    attrs[:auth_key] = attrs.delete(:ssh_key_data)
+    attrs[:auth_key] = attrs.delete(:ssh_key_data)            if attrs.key?(:ssh_key_data)
     attrs[:options]  = { :project => attrs.delete(:project) } if attrs[:project]
 
     attrs

diff --git a/app/models/manageiq/providers/embedded_ansible/automation_manager/machine_credential.rb b/app/models/manageiq/providers/embedded_ansible/automation_manager/machine_credential.rb
@@ -67,8 +67,8 @@ def self.display_name(number = 1)
   def self.params_to_attributes(params)
     attrs = params.dup
 
-    attrs[:auth_key]          = attrs.delete(:ssh_key_data)
-    attrs[:auth_key_password] = attrs.delete(:ssh_key_unlock)
+    attrs[:auth_key]          = attrs.delete(:ssh_key_data)   if attrs.key?(:ssh_key_data)
+    attrs[:auth_key_password] = attrs.delete(:ssh_key_unlock) if attrs.key?(:ssh_key_unlock)
 
     if attrs[:become_method]
       attrs[:options] = { :become_method => attrs.delete(:become_method) }

diff --git a/app/models/manageiq/providers/embedded_ansible/automation_manager/network_credential.rb b/app/models/manageiq/providers/embedded_ansible/automation_manager/network_credential.rb
@@ -62,9 +62,9 @@ def self.display_name(number = 1)
   def self.params_to_attributes(params)
     attrs = params.dup
 
-    attrs[:auth_key]          = attrs.delete(:ssh_key_data)
-    attrs[:auth_key_password] = attrs.delete(:ssh_key_unlock)
-    attrs[:become_password]   = attrs.delete(:authorize_password)
+    attrs[:auth_key]          = attrs.delete(:ssh_key_data)       if attrs.key?(:ssh_key_data)
+    attrs[:auth_key_password] = attrs.delete(:ssh_key_unlock)     if attrs.key?(:ssh_key_unlock)
+    attrs[:become_password]   = attrs.delete(:authorize_password) if attrs.key?(:authorize_password)
 
     if attrs[:authorize]
       attrs[:options] = { :authorize => attrs.delete(:authorize) }

diff --git a/app/models/manageiq/providers/embedded_ansible/automation_manager/openstack_credential.rb b/app/models/manageiq/providers/embedded_ansible/automation_manager/openstack_credential.rb
@@ -52,14 +52,12 @@ def self.params_to_attributes(params)
     attrs = params.dup
 
     if %i[host domain project].any? {|opt| attrs.has_key? opt }
-      attrs[:options] = {
-        :host    => attrs.delete(:host),
-        :domain  => attrs.delete(:domain),
-        :project => attrs.delete(:project)
-      }
+      attrs[:options]         ||= {}
+      attrs[:options][:host]    = attrs.delete(:host)    if attrs.key?(:host)
+      attrs[:options][:domain]  = attrs.delete(:domain)  if attrs.key?(:domain)
+      attrs[:options][:project] = attrs.delete(:project) if attrs.key?(:project)
     end
 
-
     attrs
   end
 

diff --git a/app/models/manageiq/providers/embedded_ansible/automation_manager/scm_credential.rb b/app/models/manageiq/providers/embedded_ansible/automation_manager/scm_credential.rb
@@ -49,8 +49,8 @@ def self.display_name(number = 1)
   def self.params_to_attributes(params)
     attrs = params.dup
 
-    attrs[:auth_key]          = attrs.delete(:ssh_key_data)
-    attrs[:auth_key_password] = attrs.delete(:ssh_key_unlock)
+    attrs[:auth_key]          = attrs.delete(:ssh_key_data)    if attrs.key?(:ssh_key_data)
+    attrs[:auth_key_password] = attrs.delete(:ssh_key_unlock)  if attrs.key?(:ssh_key_unlock)
 
     attrs
   end

diff --git a/app/models/manageiq/providers/embedded_ansible/automation_manager/vault_credential.rb b/app/models/manageiq/providers/embedded_ansible/automation_manager/vault_credential.rb
@@ -26,7 +26,7 @@ def self.display_name(number = 1)
 
   def self.params_to_attributes(params)
     attrs = params.dup
-    attrs[:password] = attrs.delete(:vault_password)
+    attrs[:password] = attrs.delete(:vault_password) if attrs.key?(:vault_password)
     attrs
   end
 end
diff --git a/spec/models/manageiq/providers/embedded_ansible/automation_manager/credential_spec.rb b/spec/models/manageiq/providers/embedded_ansible/automation_manager/credential_spec.rb
@@ -88,10 +88,19 @@
       it "#update_in_provider to succeed" do
         expect(Notification).to receive(:create!).never
 
+        previous_params_to_attrs = params_to_attrs.each_with_object({}) do |key, attrs|
+                                     attrs[key] = ansible_cred.send(key)
+                                   end
+
         result = ansible_cred.update_in_provider update_params
 
         expect(result).to be_a(credential_class)
         expect(result.name).to eq("Updated Credential")
+
+        # Doesn't muck up old attrs
+        previous_params_to_attrs.each do |attr, value|
+          expect(result.send(attr)).to eq(value)
+        end
       end
 
       it "#update_in_provider_queue" do
@@ -200,6 +209,7 @@
           }
         }
       end
+      let(:params_to_attrs) { [:auth_key, :auth_key_password, :become_method] }
       let(:update_params) do
         {
           :name     => "Updated Credential",
@@ -272,6 +282,7 @@
           }
         }
       end
+      let(:params_to_attrs) { [:authorize, :auth_key, :auth_key_password, :become_password] }
       let(:update_params) do
         {
           :name     => "Updated Credential",
@@ -330,6 +341,7 @@
           :auth_key_password_encrypted => ManageIQ::Password.try_encrypt("secret3")
         }
       end
+      let(:params_to_attrs) { [:auth_key, :auth_key_password] }
       let(:update_params) do
         {
           :name     => "Updated Credential",
@@ -369,16 +381,15 @@
           :password_encrypted => ManageIQ::Password.try_encrypt("secret1")
         }
       end
+      let(:params_to_attrs) { [:password] }
       let(:update_params) do
         {
           :name           => "Updated Credential",
-          :vault_password => "supersecret"
         }
       end
       let(:update_queue_params) do
         {
           :name           => "Updated Credential",
-          :vault_password => ManageIQ::Password.encrypt("supersecret")
         }
       end
     end
@@ -415,6 +426,7 @@
           :auth_key_encrypted => ManageIQ::Password.try_encrypt("secret2")
         }
       end
+      let(:params_to_attrs) { [:auth_key] }
       let(:update_params) do
         {
           :name     => "Updated Credential",
@@ -487,6 +499,7 @@
           }
         }
       end
+      let(:params_to_attrs) { [:auth_key, :client, :tenant, :subscription] }
       let(:update_params) do
         {
           :name     => "Updated Credential",
@@ -499,6 +512,22 @@
           :password => ManageIQ::Password.encrypt("supersecret")
         }
       end
+
+      it "#update_in_provider updating a single option" do
+        ansible_cred = credential_class.raw_create_in_provider(manager, params)
+        expect(Notification).to receive(:create!).never
+        expect(ansible_cred.client).to eq("client")
+        expect(ansible_cred.tenant).to eq("tenant")
+        expect(ansible_cred.subscription).to eq("subscription")
+
+        result = ansible_cred.update_in_provider(:name => "Updated Credential", :client => "foo")
+
+        expect(result).to be_a(credential_class)
+        expect(result.name).to eq("Updated Credential")
+        expect(result.client).to eq("foo")
+        expect(result.tenant).to eq("tenant")
+        expect(result.subscription).to eq("subscription")
+      end
     end
   end
 
@@ -544,18 +573,9 @@
           }
         }
       end
-      let(:update_params) do
-        {
-          :name         => "Updated Credential",
-          :ssh_key_data => "supersecret"
-        }
-      end
-      let(:update_queue_params) do
-        {
-          :name         => "Updated Credential",
-          :ssh_key_data => ManageIQ::Password.encrypt("supersecret")
-        }
-      end
+      let(:params_to_attrs)     { [:auth_key, :project] }
+      let(:update_params)       { {:name => "Updated Credential"} }
+      let(:update_queue_params) { {:name => "Updated Credential"} }
     end
   end
 
@@ -611,6 +631,7 @@
           }
         }
       end
+      let(:params_to_attrs) { [:host, :domain, :project] }
       let(:update_params) do
         {
           :name     => "Updated Credential",
@@ -668,6 +689,7 @@
           }
         }
       end
+      let(:params_to_attrs) { [:host] }
       let(:update_params) do
         {
           :name     => "Updated Credential",
@@ -725,6 +747,7 @@
           }
         }
       end
+      let(:params_to_attrs) { [:host] }
       let(:update_params) do
         {
           :name     => "Updated Credential",