Destroy repo mirrors instead of disabling them

It is important to destroy data related to repo mirrors when they are
disabled.
Use `_destroy` nested attribute instead of `enabled` for push mirrors.
Call `remove_import_data` after saving a project if its pull mirror is
disabled.

app/assets/javascripts/mirrors/mirror_repos.js

@@ -87,7 +87,7 @@ export default class MirrorRepos {
         project: {
           remote_mirrors_attributes: {
             id: $target.data('mirrorId'),
-            enabled: 0,
+            _destroy: 1,
           },
         },
       };

app/assets/javascripts/mirrors/ssh_mirror.js

@@ -24,12 +24,6 @@ export default class SSHMirror {
 
     this.$wellAuthTypeChanging = this.$form.find('.js-well-changing-auth');
     this.$wellPasswordAuth = this.$form.find('.js-well-password-auth');
-    this.$wellSSHAuth = this.$form.find('.js-well-ssh-auth');
-    this.$sshPublicKeyWrap = this.$form.find('.js-ssh-public-key-wrap');
-    this.$regeneratePublicSshKeyButton = this.$wellSSHAuth.find('.js-btn-regenerate-ssh-key');
-    this.$regeneratePublicSshKeyModal = this.$wellSSHAuth.find(
-      '.js-regenerate-public-ssh-key-confirm-modal',
-    );
   }
 
   init() {
@@ -40,15 +34,6 @@ export default class SSHMirror {
     this.$dropdownAuthType.on('change', e => this.handleAuthTypeChange(e));
     this.$btnDetectHostKeys.on('click', e => this.handleDetectHostKeys(e));
     this.$btnSSHHostsShowAdvanced.on('click', e => this.handleSSHHostsAdvanced(e));
-    this.$regeneratePublicSshKeyButton.on('click', () =>
-      this.$regeneratePublicSshKeyModal.toggle(true),
-    );
-    $('.js-confirm', this.$regeneratePublicSshKeyModal).on('click', e =>
-      this.regeneratePublicSshKey(e),
-    );
-    $('.js-cancel', this.$regeneratePublicSshKeyModal).on('click', () =>
-      this.$regeneratePublicSshKeyModal.toggle(false),
-    );
   }
 
   /**
@@ -162,54 +147,11 @@ export default class SSHMirror {
    * Authentication method dropdown change event listener
    */
   handleAuthTypeChange() {
-    const projectMirrorAuthTypeEndpoint = `${this.$form.attr('action')}.json`;
-    const $sshPublicKey = this.$sshPublicKeyWrap.find('.ssh-public-key');
     const selectedAuthType = this.$dropdownAuthType.val();
 
     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
-    if (selectedAuthType === AUTH_METHOD.SSH && !$sshPublicKey.text().trim()) {
-      if (!this.$wellSSHAuth.length) return;
-
-      // Construct request body
-      const authTypeData = {
-        project: {
-          ...this.$regeneratePublicSshKeyButton.data().projectData,
-        },
-      };
-
-      this.$wellAuthTypeChanging.collapse('show');
-      this.$dropdownAuthType.disable();
-
-      axios
-        .put(projectMirrorAuthTypeEndpoint, JSON.stringify(authTypeData), {
-          headers: {
-            'Content-Type': 'application/json; charset=utf-8',
-          },
-        })
-        .then(({ data }) => {
-          // Show SSH public key container and fill in public key
-          this.toggleAuthWell(selectedAuthType);
-          this.toggleSSHAuthWellMessage(true);
-          this.setSSHPublicKey(data.import_data_attributes.ssh_public_key);
-
-          this.$wellAuthTypeChanging.collapse('hide');
-          this.$dropdownAuthType.enable();
-        })
-        .catch(() => {
-          Flash(__('Something went wrong on our end.'));
-
-          this.$wellAuthTypeChanging.collapse('hide');
-          this.$dropdownAuthType.enable();
-        });
-    } else {
-      this.toggleAuthWell(selectedAuthType);
-      this.$wellSSHAuth.find('.js-ssh-public-key-present').collapse('show');
-    }
+    this.toggleAuthWell(selectedAuthType);
   }
 
   /**
@@ -235,7 +177,6 @@ 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);
   }
 
@@ -244,64 +185,11 @@ export default class SSHMirror {
     this.$hiddenAuthType.prop('disabled', authType === AUTH_METHOD.SSH);
   }
 
-  /**
-   * Toggle SSH auth information message
-   */
-  toggleSSHAuthWellMessage(sshKeyPresent) {
-    this.$sshPublicKeyWrap.collapse(sshKeyPresent ? 'show' : 'hide');
-    this.$wellSSHAuth.find('.js-ssh-public-key-present').collapse(sshKeyPresent ? 'show' : 'hide');
-    this.$regeneratePublicSshKeyButton.collapse(sshKeyPresent ? 'show' : 'hide');
-    this.$wellSSHAuth.find('.js-ssh-public-key-pending').collapse(sshKeyPresent ? 'hide' : 'show');
-  }
-
-  /**
-   * Sets SSH Public key to Clipboard button and shows it on UI.
-   */
-  setSSHPublicKey(sshPublicKey) {
-    this.$sshPublicKeyWrap.find('.ssh-public-key').text(sshPublicKey);
-    this.$sshPublicKeyWrap
-      .find('.btn-copy-ssh-public-key')
-      .attr('data-clipboard-text', sshPublicKey);
-  }
-
-  regeneratePublicSshKey(event) {
-    event.preventDefault();
-
-    this.$regeneratePublicSshKeyModal.toggle(false);
-
-    const button = this.$regeneratePublicSshKeyButton;
-    const spinner = $('.js-spinner', button);
-    const endpoint = button.data('endpoint');
-    const authTypeData = {
-      project: {
-        ...this.$regeneratePublicSshKeyButton.data().projectData,
-      },
-    };
-
-    button.attr('disabled', 'disabled');
-    spinner.removeClass('d-none');
-
-    axios
-      .patch(endpoint, authTypeData)
-      .then(({ data }) => {
-        button.removeAttr('disabled');
-        spinner.addClass('d-none');
-
-        this.setSSHPublicKey(data.import_data_attributes.ssh_public_key);
-      })
-      .catch(() => {
-        Flash(__('Unable to regenerate public ssh key.'));
-      });
-  }
-
   destroy() {
     this.$repositoryUrl.off('keyup');
     this.$form.find('.js-known-hosts').off('keyup');
     this.$dropdownAuthType.off('change');
     this.$btnDetectHostKeys.off('click');
     this.$btnSSHHostsShowAdvanced.off('click');
-    this.$regeneratePublicSshKeyButton.off('click');
-    $('.js-confirm', this.$regeneratePublicSshKeyModal).off('click');
-    $('.js-cancel', this.$regeneratePublicSshKeyModal).off('click');
   }
 }

app/controllers/projects/mirrors_controller.rb

@@ -81,6 +81,7 @@ class Projects::MirrorsController < Projects::ApplicationController
         password
         ssh_known_hosts
         regenerate_ssh_private_key
+        _destroy
       ]
     ]
   end

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

 - mirror = f.object
-- is_push = local_assigns.fetch(:is_push, false)
 - auth_options = [[_('Password'), 'password'], [_('SSH public key'), 'ssh_public_key']]
-- regen_data = { auth_method: 'ssh_public_key', regenerate_ssh_private_key: true }
-- ssh_public_key_present = mirror.ssh_public_key.present?
 
 .form-group
   = f.label :auth_method, _('Authentication method'), class: 'label-bold'
@@ -17,21 +14,3 @@
   .well-password-auth.collapse.js-well-password-auth
     = f.label :password, _("Password"), class: "label-bold"
     = f.password_field :password, value: mirror.password, class: 'form-control qa-password', autocomplete: 'new-password'
-  - unless is_push
-    .well-ssh-auth.collapse.js-well-ssh-auth
-      %p.js-ssh-public-key-present{ class: ('collapse' unless ssh_public_key_present) }
-        = _('Here is the public SSH key that needs to be added to the remote server. For more information, please refer to the documentation.')
-      %p.js-ssh-public-key-pending{ class: ('collapse' if ssh_public_key_present) }
-        = _('An SSH key will be automatically generated when the form is submitted. For more information, please refer to the documentation.')
-
-      .clearfix.js-ssh-public-key-wrap{ class: ('collapse' unless ssh_public_key_present) }
-        %code.prepend-top-10.ssh-public-key
-          = mirror.ssh_public_key
-        = clipboard_button(text: mirror.ssh_public_key, title: _("Copy SSH public key to clipboard"), class: 'prepend-top-10 btn-copy-ssh-public-key')
-
-      = button_tag type: 'button',
-        data: { endpoint: project_mirror_path(@project), project_data: { import_data_attributes: regen_data } },
-        class: "btn btn-inverted btn-warning prepend-top-10 js-btn-regenerate-ssh-key#{ ' collapse' unless ssh_public_key_present }" do
-        = icon('spinner spin', class: 'js-spinner d-none')
-        = _('Regenerate key')
-      = render 'projects/mirrors/regenerate_public_ssh_key_confirm_modal'

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

@@ -5,4 +5,4 @@
   = rm_f.hidden_field :url, class: 'js-mirror-url-hidden', required: true, pattern: "(#{protocols}):\/\/.+"
   = rm_f.hidden_field :only_protected_branches, class: 'js-mirror-protected-hidden'
   = render partial: 'projects/mirrors/ssh_host_keys', locals: { f: rm_f }
-  = render partial: 'projects/mirrors/authentication_method', locals: { f: rm_f, is_push: true }
+  = render partial: 'projects/mirrors/authentication_method', locals: { f: rm_f }

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

@@ -3,7 +3,7 @@
 - verified_at = mirror.ssh_known_hosts_verified_at
 
 .form-group.js-ssh-host-keys-section{ class: ('collapse' unless mirror.ssh_mirror_url?) }
-  %button.btn.btn-inverted.btn-success.inline.js-detect-host-keys.append-right-10{ type: 'button' }
+  %button.btn.btn-inverted.btn-secondary.inline.js-detect-host-keys.append-right-10{ type: 'button' }
     = icon('spinner spin', class: 'js-spinner d-none')
     = _('Detect host keys')
   .fingerprint-ssh-info.js-fingerprint-ssh-info.prepend-top-10.append-bottom-10{ class: ('collapse' unless mirror.ssh_mirror_url?) }

changelogs/unreleased/ce-its-simple-just-destroy-the-mirrors.yml

+---
+title: Destroy project remote mirrors instead of disabling
+merge_request: 27087
+author:
+type: security

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

@@ -34,6 +34,7 @@ module EE
 
           sync_wiki_on_enable if !wiki_was_enabled && project.wiki_enabled?
           project.import_state.force_import_job! if params[:mirror].present? && project.mirror?
+          project.remove_import_data if project.previous_changes.include?('mirror') && !project.mirror?
 
           sync_approval_rules
         end

ee/app/views/projects/mirrors/_mirror_repos_form.html.haml

 - import_data = @project.import_data || @project.build_import_data
 - is_one_user_option = default_mirror_users.count == 1
 - protocols = Gitlab::UrlSanitizer::ALLOWED_SCHEMES.join('|')
-- options = [[_('Push'), 'push']]
+- direction_options = [[_('Push'), 'push']]
 - has_existing_pull_mirror = @project.mirror.present?
+
 - if can?(current_user, :admin_mirror, @project)
-  - pull_addition_method = has_existing_pull_mirror ? options.method(:push) : options.method(:unshift)
+  - pull_addition_method = has_existing_pull_mirror ? direction_options.method(:push) : direction_options.method(:unshift)
   - pull_addition_method.call([_('Pull'), 'pull'])
 
 .form-group
   = label_tag :mirror_direction, _('Mirror direction'), class: 'label-light'
-  = select_tag :mirror_direction, options_for_select(options), class: 'form-control js-mirror-direction qa-mirror-direction', disabled: (options.count == 1) || has_existing_pull_mirror
+  = select_tag :mirror_direction, options_for_select(direction_options), class: 'form-control js-mirror-direction qa-mirror-direction', disabled: (direction_options.count == 1) || has_existing_pull_mirror
 
 .js-form-insertion-point
 
@@ -21,7 +22,7 @@
   = f.hidden_field :username_only_import_url, class: 'js-mirror-url-hidden', required: true, pattern: "(#{protocols}):\/\/.+"
   = f.hidden_field :only_mirror_protected_branches, class: 'js-mirror-protected-hidden'
 
-  = f.fields_for :import_data, import_data do |import_form|
+  = f.fields_for :import_data, import_data, include_id: false do |import_form|
     = render partial: 'projects/mirrors/ssh_host_keys', locals: { f: import_form }
     = render partial: 'projects/mirrors/authentication_method', locals: { f: import_form }
 

ee/app/views/projects/mirrors/_table_pull_row.html.haml

@@ -12,6 +12,9 @@
   %td.mirror-action-buttons
     .btn-group.mirror-actions-group.pull-right{ role: 'group' }
       - if @project.mirror?
+        - ssh_public_key = @project.import_data.ssh_public_key
+        - if ssh_public_key
+          = clipboard_button(text: ssh_public_key, class: 'btn btn-default qa-copy-ssh-public-key', title: _('Copy SSH public key'))
         - if import_state.mirror_update_due? || import_state.updating_mirror?
           %button.btn.disabled{ type: 'button', data: { container: 'body', toggle: 'tooltip' }, title: _('Updating') }= icon("refresh spin")
         - else

ee/changelogs/unreleased/its-simple-just-destroy-the-mirrors.yml

+---
+title: Destroy project remote pull mirrors instead of disabling
+merge_request: 10355
+author:
+type: security

ee/spec/features/projects/mirror_spec.rb

@@ -142,41 +142,17 @@ describe 'Project mirror', :js do
           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
-
-          expect(import_data.ssh_public_key).not_to be_nil
-          expect(page).to have_content(import_data.ssh_public_key)
-
           click_without_sidekiq 'Mirror repository'
         end
+        project.reload
 
-        # We didn't set any host keys
         expect(page).to have_content('Mirroring settings were successfully updated')
         expect(page).not_to have_content('Verified by')
-
-        project.reload
-        import_data.reload
+        expect(find('.qa-copy-ssh-public-key')['data-clipboard-text']).to eq(import_data.ssh_public_key)
         expect(project.mirror?).to be_truthy
         expect(project.username_only_import_url).to eq('ssh://user@example.com')
         expect(import_data.auth_method).to eq('ssh_public_key')
         expect(import_data.password).to be_blank
-
-        find('.js-delete-mirror').click
-        fill_in 'Git repository URL', with: 'ssh://user@example.com'
-        select('Pull', from: 'Mirror direction')
-
-        first_key = import_data.ssh_public_key
-        expect(page).to have_content(first_key)
-
-        # Check regenerating the public key works
-        click_without_sidekiq 'Regenerate key'
-        find('.js-regenerate-public-ssh-key-confirm-modal .js-confirm').click
-
-        wait_for_requests
-
-        expect(page).not_to have_content(first_key)
-        expect(page).to have_content(import_data.reload.ssh_public_key)
       end
     end
 
@@ -204,27 +180,6 @@ 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/features/projects/settings/ee/repository_mirrors_settings_spec.rb

@@ -66,5 +66,20 @@ describe 'Project settings > [EE] repository' do
         expect(mirror_url).to include('https://*****@github.com/')
       end
     end
+
+    context 'with an existing pull mirror', :js do
+      let(:mirrored_project) { create(:project, :repository, :mirror, namespace: user.namespace) }
+
+      it 'deletes the mirror' do
+        visit project_settings_repository_path(mirrored_project)
+
+        find('.js-delete-mirror').click
+        wait_for_requests
+        mirrored_project.reload
+
+        expect(mirrored_project.import_data).to be_nil
+        expect(mirrored_project).not_to be_mirror
+      end
+    end
   end
 end

ee/spec/services/projects/update_service_spec.rb

@@ -277,6 +277,13 @@ describe Projects::UpdateService, '#execute' do
     expect(result).to eq({ status: :error, message: "Name can contain only letters, digits, emojis, '_', '.', dash, space. It must start with letter, digit, emoji or '_'." })
   end
 
+  it 'calls remove_import_data if mirror was disabled in previous change' do
+    update_project(project, user, { mirror: false })
+
+    expect(project.import_data).to be_nil
+    expect(project).not_to be_mirror
+  end
+
   def update_project(project, user, opts)
     Projects::UpdateService.new(project, user, opts).execute
   end

locale/gitlab.pot

@@ -977,9 +977,6 @@ msgstr ""
 msgid "Amount of time (in hours) that users are allowed to skip forced configuration of two-factor authentication"
 msgstr ""
 
-msgid "An SSH key will be automatically generated when the form is submitted. For more information, please refer to the documentation."
-msgstr ""
-
 msgid "An application called %{link_to_client} is requesting access to your GitLab account."
 msgstr ""
 
@@ -3402,9 +3399,6 @@ msgstr ""
 msgid "Copy SSH public key"
 msgstr ""
 
-msgid "Copy SSH public key to clipboard"
-msgstr ""
-
 msgid "Copy URL to clipboard"
 msgstr ""
 
@@ -6316,9 +6310,6 @@ msgstr ""
 msgid "Help page text and support page url."
 msgstr ""
 
-msgid "Here is the public SSH key that needs to be added to the remote server. For more information, please refer to the documentation."
-msgstr ""
-
 msgid "Hide file browser"
 msgstr ""
 
@@ -13094,9 +13085,6 @@ msgstr ""
 msgid "Unable to load the diff. %{button_try_again}"
 msgstr ""
 
-msgid "Unable to regenerate public ssh key."
-msgstr ""
-
 msgid "Unable to resolve"
 msgstr ""
 

spec/features/projects/settings/repository_settings_spec.rb

@@ -217,5 +217,24 @@ describe 'Projects > Settings > Repository settings' do
         expect(RepositoryCleanupWorker.jobs.count).to eq(1)
       end
     end
+
+    context 'with an existing mirror', :js do
+      let(:mirrored_project) { create(:project, :repository, :remote_mirror) }
+
+      before do
+        mirrored_project.add_maintainer(user)
+
+        visit project_settings_repository_path(mirrored_project)
+      end
+
+      it 'delete remote mirrors' do
+        expect(mirrored_project.remote_mirrors.count).to eq(1)
+
+        find('.js-delete-mirror').click
+        wait_for_requests
+
+        expect(mirrored_project.remote_mirrors.count).to eq(0)
+      end
+    end
   end
 end