Merge branch 'fix-pull-mirror-setting-nil' into 'master'

Fix unsetting credentials data for pull mirrors

Closes #3308

See merge request !2810

changelogs/unreleased-ee/fix-pull-mirror-setting-nil.yml

+---
+title: Fix unsetting credentials data for pull mirrors
+merge_request: 2810
+author:
+type: fixed

ee/app/models/ee/project_import_data.rb

@@ -5,6 +5,16 @@ module EE
       bits: 4096
     }.freeze
 
+    CREDENTIALS_FIELDS = %i[
+      auth_method
+      password
+      ssh_known_hosts
+      ssh_known_hosts_verified_at
+      ssh_known_hosts_verified_by_id
+      ssh_private_key
+      user
+    ].freeze
+
     extend ActiveSupport::Concern
 
     included do
@@ -26,14 +36,22 @@ module EE
       project&.import_url&.start_with?('ssh://')
     end
 
-    %i[auth_method user password ssh_private_key ssh_known_hosts ssh_known_hosts_verified_at ssh_known_hosts_verified_by_id].each do |name|
+    CREDENTIALS_FIELDS.each do |name|
       define_method(name) do
         credentials[name] if credentials.present?
       end
 
       define_method("#{name}=") do |value|
         self.credentials ||= {}
+
+        # Removal of the password, username, etc, generally causes an update of
+        # the value to the empty string. Detect and gracefully handle this case.
+        if value.present?
           self.credentials[name] = value
+        else
+          self.credentials.delete(name)
+          nil
+        end
       end
     end
 

spec/ee/spec/models/ee/project_import_data_spec.rb

@@ -62,6 +62,34 @@ describe ProjectImportData do
     end
   end
 
+  describe 'credential fields accessors' do
+    let(:accessors) { %i[auth_method password ssh_known_hosts ssh_known_hosts_verified_at ssh_known_hosts_verified_by_id ssh_private_key user] }
+
+    it { expect(described_class::CREDENTIALS_FIELDS).to contain_exactly(*accessors) }
+
+    where(:field) { described_class::CREDENTIALS_FIELDS }
+
+    with_them do
+      it 'sets the value in the credentials hash' do
+        import_data.send("#{field}=", 'foo')
+
+        expect(import_data.credentials[field]).to eq('foo')
+      end
+
+      it 'sets a not-present value to nil' do
+        import_data.send("#{field}=", '')
+
+        expect(import_data.credentials[field]).to be_nil
+      end
+
+      it 'returns the data in the credentials hash' do
+        import_data.credentials[field] = 'foo'
+
+        expect(import_data.send(field)).to eq('foo')
+      end
+    end
+  end
+
   describe '#ssh_import?' do
     where(:import_url, :expected) do
       'ssh://example.com'   | true