Editing username for EmbeddedAnsible SCM credential cause key to be nil'd out

[NickLaMuro]

Steps to reproduce

1. Enable EmbeddedAnsible (if it is not already)

2. Go to Automation -> Ansible -> Credentials

3. Add a new SCM key, with a username (example: "git") and private key (example: "totally fake")

4. In a Rails console, check the credentials is there and the username and key exist:

   irb> Authentication.pluck(:userid, :auth_key).last
   #=> ["git", "totally fake"]

5. Edit the new credential, and change only the userid (example: "got")

6. Observe the change in the :auth_key in the Rails console:

   irb> Authentication.pluck(:userid, :auth_key).last
   #=> ["got", nil]

[NickLaMuro]

I will do a quick look into this to see if I can fix it on my own, but it is something we stumbled on while troubleshooting this ticket with a customer:

https://bugzilla.redhat.com/show_bug.cgi?id=1869885

Could be frontend or backend code that is the culprit, but not sure yet.

[NickLaMuro]

Did a bit of Debugging locally, and rails log seems suggest the UI calls a API action for this and that queues a backend task:

Log output

DEBUG -- : PostgreSQLAdapter#log_after_checkin, connection_pool: size: 5, connections: 4, in use: 1, waiting_in_queue: 0
 INFO -- : NotificationChannel is transmitting the subscription confirmation
 INFO -- : NotificationChannel is streaming from notifications_1
 INFO -- : Started PUT "/api/authentications/2" for 127.0.0.1 at 2020-09-02 20:10:58 -0500
 INFO -- : Processing by Api::AuthenticationsController#update as JSON
 INFO -- :   Parameters: {"c_id"=>"2"}

*** LOG OUTPUT TRIMMED ***

DEBUG -- :    (0.1ms)  BEGIN
DEBUG -- :   MiqTask Create (0.8ms)  INSERT INTO "miq_tasks" ("name", "state", "status", "message", "userid", "created_on", "updated_on")
																		 VALUES ($1, $2, $3, $4, $5, $6, $7) RETURNING "id"
                                     [
                                       ["name", "Updating Embedded Ansible Credential (name=test)"],
                                       ["state", "Queued"],
                                       ["status", "Ok"],
                                       ["message", "Queued the action: [Updating Embedded Ansible Credential (name=test)]being run for user: [system]"],
                                       ["userid", "system"],
                                       ["created_on", "2020-09-03 01:10:58.773569"],
                                       ["updated_on", "2020-09-03 01:10:58.773569"]
                                     ]
DEBUG -- :    (0.8ms)  COMMIT
DEBUG -- :    (0.1ms)  BEGIN
DEBUG -- :   MiqQueue Create (0.5ms)  INSERT INTO "miq_queue" ("priority", "method_name", "state", "created_on", "updated_on", "queue_name", "class_name", "instance_id", "args", "miq_callback", "role", "msg_timeout", "miq_task_id", "lock_version")
                                      VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14) RETURNING "id" 
                                      [
                                        ["priority", 20],
                                        ["method_name", "update_in_provider"],
                                        ["state", "ready"],
                                        ["created_on", "2020-09-03 01:10:58.777244"],
                                        ["updated_on", "2020-09-03 01:10:58.777244"],
                                        ["queue_name", "generic"],
                                        ["class_name", "ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ScmCredential"],
                                        ["instance_id", 2],
                                        ["args", "---\n- :id: '2'\n  :name: test\n  :type: ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ScmCredential\n :userid: got\n  :task_id: 2\n"],
                                        ["miq_callback", "---\n:class_name: MiqTask\n:instance_id: 2\n:method_name: :queue_callback\n:args:\n- Finished\n"],
                                        ["role", "embedded_ansible"],
                                        ["msg_timeout", 600],
                                        ["miq_task_id", 2],
                                        ["lock_version", 0]
                                      ]

Unless it is sending bad data to said task, I think this is most likely NOT a UI problem, but I will keep the issue open here until I know for sure (and ultimately will not open an issue elsewhere just to avoid this issue being tracked across multiple issues).

[NickLaMuro]

Okay, the issue is in Core. The following change fixes is for scm_credential.rb:

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 9dc2f5ca0e..7e0b88bef3 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 @@ class ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ScmCredential < M
   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

But there will need to be probably changes to all credential types. I will work on a PR for this tomorrow.

[Fryguy]

Yeah, since we don't return the auth_key and auth_key_password for security reasons then the UI is presenting nothing. After that the form sends back nil and we write it. @NickLaMuro Does the above patch work all the way from the UI? I would have expected the UI form to send the key with a literal null value.

Not sure what the right answer is here... I would assume we should follow the same pattern as we do for any other password field that is part of form (like with provider credentials, for example)

[NickLaMuro]

If you look at the log output from this #7286 (comment) you will see it just include userid: got for the update yaml and none of the other non-changed fields, so I think we are good.

[NickLaMuro]

Fixed via ManageIQ/manageiq#20494

Closing...

[NickLaMuro]

Woops, not fixed via that PR, but this one:

ManageIQ/manageiq#20524

Which is still open. Reopening...