Merge branch 'sh-fix-ssh-pull-mirorrs' into 'master'

Fix SSH pull mirrors not working

Closes gitaly#1550

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

app/assets/javascripts/mirrors/ssh_mirror.js

@@ -20,6 +20,7 @@ export default class SSHMirror {
     this.$btnDetectHostKeys = this.$form.find('.js-detect-host-keys');
     this.$btnSSHHostsShowAdvanced = this.$form.find('.btn-show-advanced');
     this.$dropdownAuthType = this.$form.find('.js-mirror-auth-type');
+    this.$hiddenAuthType = this.$form.find('.js-hidden-mirror-auth-type');
 
     this.$wellAuthTypeChanging = this.$form.find('.js-well-changing-auth');
     this.$wellPasswordAuth = this.$form.find('.js-well-password-auth');
@@ -167,6 +168,7 @@ export default class SSHMirror {
 
     this.$wellPasswordAuth.collapse('hide');
     this.$wellSSHAuth.collapse('hide');
+    this.updateHiddenAuthType(selectedAuthType);
 
     // This request should happen only if selected Auth type was SSH
     // and SSH Public key was not present on page load
@@ -234,6 +236,12 @@ export default class SSHMirror {
   toggleAuthWell(authType) {
     this.$wellPasswordAuth.collapse(authType === AUTH_METHOD.PASSWORD ? 'show' : 'hide');
     this.$wellSSHAuth.collapse(authType === AUTH_METHOD.SSH ? 'show' : 'hide');
+    this.updateHiddenAuthType(authType);
+  }
+
+  updateHiddenAuthType(authType) {
+    this.$hiddenAuthType.val(authType);
+    this.$hiddenAuthType.prop('disabled', authType === AUTH_METHOD.SSH);
   }
 
   /**

app/views/projects/mirrors/_authentication_method.html.haml

@@ -9,6 +9,7 @@
   = f.select :auth_method,
       options_for_select(auth_options, mirror.auth_method),
       {}, { class: "form-control js-mirror-auth-type qa-authentication-method" }
+  = f.hidden_field :auth_method, value: "password", class: "js-hidden-mirror-auth-type"
 
 .form-group
   .collapse.js-well-changing-auth

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

@@ -87,15 +87,6 @@ module EE
           # 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,7 +10,6 @@ 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)
@@ -20,10 +19,6 @@ 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-ssh-pull-mirorrs.yml

+---
+title: Fix SSH pull mirrors not working
+merge_request: 10272
+author:
+type: fixed

ee/spec/controllers/projects/mirrors_controller_spec.rb

@@ -115,51 +115,6 @@ 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/features/projects/mirror_spec.rb

@@ -98,6 +98,39 @@ describe 'Project mirror', :js do
         expect(import_data.auth_method).to eq('password')
         expect(project.import_url).to eq('http://2.example.com')
       end
+
+      it 'can be recreated after an SSH mirror is set' do
+        visit project_settings_repository_path(project)
+
+        page.within('.project-mirror-settings') do
+          fill_in 'Git repository URL', with: 'ssh://user@example.com'
+          select('Pull', from: 'Mirror direction')
+          select 'SSH public key', from: 'Authentication method'
+
+          # Generates an SSH public key with an asynchronous PUT and displays it
+          wait_for_requests
+
+          click_without_sidekiq 'Mirror repository'
+        end
+
+        expect(page).to have_content('Mirroring settings were successfully updated')
+
+        find('.js-delete-pull-mirror').click
+
+        page.within('.project-mirror-settings') do
+          fill_in 'Git repository URL', with: 'http://git@example.com'
+          select('Pull', from: 'Mirror direction')
+          fill_in 'Password', with: 'test_password'
+          click_without_sidekiq 'Mirror repository'
+        end
+
+        expect(page).to have_content('Mirroring settings were successfully updated')
+
+        project.reload
+        expect(import_data.auth_method).to eq('password')
+        expect(import_data.password).to eq('test_password')
+        expect(project.import_url).to eq('http://git:test_password@example.com')
+      end
     end
 
     describe 'SSH public key authentication' do
@@ -171,6 +204,27 @@ describe 'Project mirror', :js do
         end
       end
 
+      it 'preserves the existing SSH key after generating it once' do
+        stub_reactive_cache(cache, known_hosts: key.key_text)
+
+        visit project_settings_repository_path(project)
+
+        page.within('.project-mirror-settings') do
+          fill_in 'Git repository URL', with: 'ssh://example.com'
+          select('Pull', from: 'Mirror direction')
+          select 'SSH public key', from: 'Authentication method'
+          click_on 'Detect host keys'
+
+          wait_for_requests
+
+          expect(page).to have_content(key.fingerprint)
+
+          wait_for_requests
+
+          expect { click_on 'Mirror repository' }.not_to change { import_data.reload.ssh_public_key }
+        end
+      end
+
       it 'displays error if detection fails' do
         stub_reactive_cache(cache, error: 'Some error text here')
 

ee/spec/services/projects/update_service_spec.rb

@@ -29,17 +29,6 @@ describe Projects::UpdateService, '#execute' do
 
       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