From 7bbdf70a00cac12ff7cd869c0f082e6d3045ca20 Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Thu, 3 Sep 2020 17:42:52 -0500 Subject: [PATCH] [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. --- .../automation_manager/amazon_credential.rb | 2 +- .../automation_manager/azure_credential.rb | 11 ++-- .../automation_manager/credential.rb | 8 ++- .../automation_manager/google_credential.rb | 2 +- .../automation_manager/machine_credential.rb | 4 +- .../automation_manager/network_credential.rb | 6 +-- .../openstack_credential.rb | 10 ++-- .../automation_manager/scm_credential.rb | 4 +- .../automation_manager/vault_credential.rb | 2 +- .../automation_manager/credential_spec.rb | 51 ++++++++++++++----- 10 files changed, 63 insertions(+), 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 index f45180b3fab..a78ddf5af06 100644 --- 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 index 4067593df90..f504e360b71 100644 --- 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 index 1970b70a69e..1fa9c3a970f 100644 --- 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 index c1322e8e2e5..f75b8f640b8 100644 --- 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 index 1a4c8cd4032..d64548883b4 100644 --- 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 index 4238de66139..c57ab1f83dd 100644 --- 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 index 913df0bb8ae..46cdd20a86f 100644 --- 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 index 9dc2f5ca0e2..7e0b88bef30 100644 --- 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 index 0cc81be832c..d578dd72ae7 100644 --- 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 index a7290df1ec7..44f80fc9c11 100644 --- 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",