Merge branch 'sh-fix-project-mirrors-mix-http-ssh-creds' into 'master'

Clear our import data credentials when adding new mirrors

Closes gitlab-ce#55729

See merge request gitlab-org/gitlab-ee!9134

app/models/project_import_data.rb

@@ -32,4 +32,8 @@ class ProjectImportData < ActiveRecord::Base
   def merge_credentials(hash)
     self.credentials = credentials.to_h.merge(hash) unless hash.empty?
   end
+
+  def clear_credentials
+    self.credentials = {}
+  end
 end

ee/app/controllers/ee/projects/mirrors_controller.rb

@@ -82,10 +82,20 @@ module EE
         params = mirror_params
 
         import_data = params[:import_data_attributes]
+
         if import_data.present?
           # Prevent Rails from destroying the existing import data
           import_data[:id] ||= project.import_data&.id
 
+          # Since we reuse the existing import data row, we'll need to
+          # clear out the credentials each time to ensure we don't
+          # erroneously mix and match SSH and HTTPS credentials. We assume
+          # this is safe to do because we only support creation of new
+          # mirrors and do not support updating them until
+          # https://gitlab.com/gitlab-org/gitlab-ee/issues/7320 is
+          # supported.
+          params[:clear_import_data_credentials] = true
+
           # If the known hosts data is being set, store details about who and when
           if import_data[:ssh_known_hosts].present?
             import_data[:ssh_known_hosts_verified_at] = Time.now

ee/app/services/ee/projects/update_service.rb

@@ -10,6 +10,7 @@ module EE
       override :execute
       def execute
         should_remove_old_approvers = params.delete(:remove_old_approvers)
+        should_clear_import_data_credentials = params.delete(:clear_import_data_credentials)
         wiki_was_enabled = project.wiki_enabled?
 
         limit = params.delete(:repository_size_limit)
@@ -19,6 +20,10 @@ module EE
           return project
         end
 
+        # Existing import data may have SSH or HTTP credentials. Often we won't want
+        # to merge both credentials, so clear them out if requested.
+        project.import_data&.clear_credentials if should_clear_import_data_credentials
+
         result = super do
           # Repository size limit comes as MB from the view
           project.repository_size_limit = ::Gitlab::Utils.try_megabytes_to_bytes(limit) if limit

ee/changelogs/unreleased/sh-fix-project-mirrors-mix-http-ssh-creds.yml

+---
+title: Clear our import data credentials when adding new mirrors
+merge_request: 24339
+author:
+type: fixed

ee/spec/controllers/projects/mirrors_controller_spec.rb

@@ -115,6 +115,51 @@ describe Projects::MirrorsController do
     end
   end
 
+  describe 'recreating a pull mirror' do
+    let(:import_url) { 'http://username:password@local.dev' }
+    let(:project) { create(:project, :mirror, import_url: import_url) }
+
+    before do
+      sign_in(project.owner)
+    end
+
+    it 'retains the right credentials' do
+      expect(project.mirror).to eq(true)
+      expect(project.import_url).to eq(import_url)
+      expect(project.import_data.auth_method).to eq('password')
+      expect(project.import_data.user).to eq('username')
+      expect(project.import_data.password).to eq('password')
+
+      do_put(project,
+             mirror: true,
+             mirror_user_id: project.owner.id,
+             import_data_attributes: {
+               auth_method: 'ssh_public_key'
+             })
+
+      project.reload
+      expect(project.mirror).to eq(true)
+      expect(project.import_data.auth_method).to eq('ssh_public_key')
+      expect(project.import_data.ssh_public_key).to be_present
+      expect(project.import_data.ssh_private_key).to be_present
+
+      do_put(project,
+             mirror: true,
+             import_url: 'http://newuser@local.dev',
+             import_data_attributes: {
+               password: 'newpassword'
+             })
+
+      project.reload
+      expect(project.mirror).to eq(true)
+      expect(project.import_data.auth_method).to eq('password')
+      expect(project.import_data.user).to eq('newuser')
+      expect(project.import_data.password).to eq('newpassword')
+      expect(project.import_data.ssh_public_key).to be_nil
+      expect(project.import_data.ssh_private_key).to be_nil
+    end
+  end
+
   describe 'forcing an update on a pull mirror' do
     it 'forces update' do
       project = create(:project, :mirror)

ee/spec/services/projects/update_service_spec.rb

@@ -13,6 +13,10 @@ describe Projects::UpdateService, '#execute' do
       }
     end
 
+    before do
+      stub_licensed_features(repository_mirrors: true)
+    end
+
     it 'forces an import job' do
       opts = {
         import_url: 'http://foo.com',
@@ -21,11 +25,21 @@ describe Projects::UpdateService, '#execute' do
         mirror_trigger_builds: true
       }
 
-      stub_licensed_features(repository_mirrors: true)
       expect_any_instance_of(EE::ProjectImportState).to receive(:force_import_job!).once
 
       update_project(project, user, opts)
     end
+
+    it 'clears credentials' do
+      project = create(:project, :mirror, import_url: 'https://username:password@github.com/vbim/vim.git')
+
+      expect(project.import_data.credentials[:user]).to eq('username')
+      expect(project.import_data.credentials[:password]).to eq('password')
+
+      project.reload
+      update_project(project, user, clear_import_data_credentials: true)
+      expect(project.import_data.credentials).to eq({})
+    end
   end
 
   context 'audit events' do

spec/models/project_import_data_spec.rb

@@ -39,4 +39,15 @@ describe ProjectImportData do
       expect(row.credentials).to eq({ 'number' => 10, 'foo' => 'bar' })
     end
   end
+
+  describe '#clear_credentials' do
+    it 'clears out the Hash' do
+      row = described_class.new
+
+      row.merge_credentials('number' => 10)
+      row.clear_credentials
+
+      expect(row.credentials).to eq({})
+    end
+  end
 end