Fix issue where deleting a remote mirror doesnt remove it from databasefix-destroy-remote-mirror

Remote mirrors were simply being disabled when the user clicks
the UI delete button. This commit adds a remote mirrors delete
endpoint that correctly destroys the database record.
It is important to correctly delete remote mirrors when the user
requests it as their credentials are present in
`remote_mirrors_attributes`.

14 files changed, 213 insertions, 27 deletions

diff --git a/app/assets/javascripts/api.js b/app/assets/javascripts/api.js
index 85eb08cc97d..093762e823e 100644
--- a/app/assets/javascripts/api.js
+++ b/app/assets/javascripts/api.js
@@ -31,6 +31,7 @@ const Api = {
   branchSinglePath: '/api/:version/projects/:id/repository/branches/:branch',
   createBranchPath: '/api/:version/projects/:id/repository/branches',
   releasesPath: '/api/:version/projects/:id/releases',
+  projectRemoteMirrorPath: '/api/:version/projects/:project_id/remote_mirrors/:remote_mirror_id',
 
   group(groupId, callback) {
     const url = Api.buildUrl(Api.groupPath).replace(':id', groupId);
@@ -321,6 +322,14 @@ const Api = {
     return axios.get(url);
   },
 
+  deleteProjectRemoteMirror(projectId, remoteMirrorId) {
+    const url = Api.buildUrl(this.projectRemoteMirrorPath)
+      .replace(':project_id', encodeURIComponent(projectId))
+      .replace(':remote_mirror_id', remoteMirrorId);
+
+    return axios.delete(url).then(({ data }) => data);
+  },
+
   buildUrl(url) {
     let urlRoot = '';
     if (gon.relative_url_root != null) {
diff --git a/app/assets/javascripts/mirrors/mirror_repos.js b/app/assets/javascripts/mirrors/mirror_repos.js
index 196b84621b6..ae19f182c58 100644
--- a/app/assets/javascripts/mirrors/mirror_repos.js
+++ b/app/assets/javascripts/mirrors/mirror_repos.js
@@ -2,8 +2,8 @@ import $ from 'jquery';
 import _ from 'underscore';
 import { __ } from '~/locale';
 import Flash from '~/flash';
-import axios from '~/lib/utils/axios_utils';
 import SSHMirror from './ssh_mirror';
+import Api from '~/api';
 
 export default class MirrorRepos {
   constructor(container) {
@@ -13,7 +13,6 @@ export default class MirrorRepos {
     this.$urlInput = $('.js-mirror-url', this.$form);
     this.$protectedBranchesInput = $('.js-mirror-protected', this.$form);
     this.$table = $('.js-mirrors-table-body', this.$container);
-    this.mirrorEndpoint = this.$form.data('projectMirrorEndpoint');
   }
 
   init() {
@@ -78,25 +77,16 @@ export default class MirrorRepos {
     this.$passwordGroup.collapse(isPassword ? 'show' : 'hide');
   }
 
-  deleteMirror(event, existingPayload) {
+  deleteMirror(event) {
     const $target = $(event.currentTarget);
-    let payload = existingPayload;
-
-    if (!payload) {
-      payload = {
-        project: {
-          remote_mirrors_attributes: {
-            id: $target.data('mirrorId'),
-            enabled: 0,
-          },
-        },
-      };
-    }
 
-    return axios
-      .put(this.mirrorEndpoint, payload)
+    return Api.deleteProjectRemoteMirror(this.$form.data('projectId'), $target.data('mirrorId'))
       .then(() => this.removeRow($target))
-      .catch(() => Flash(__('Failed to remove mirror.')));
+      .catch(({ response }) => {
+        Flash(
+          response.status === 404 ? __('Remote mirror not found.') : __('Failed to remove mirror.'),
+        );
+      });
   }
 
   /* eslint-disable class-methods-use-this */
diff --git a/app/helpers/mirror_helper.rb b/app/helpers/mirror_helper.rb
index 65c7cd82832..6fff418b293 100644
--- a/app/helpers/mirror_helper.rb
+++ b/app/helpers/mirror_helper.rb
@@ -1,10 +1,11 @@
 # frozen_string_literal: true
 
 module MirrorHelper
-  def mirrors_form_data_attributes
+  def mirrors_form_data_attributes(project)
     {
-      project_mirror_ssh_endpoint: ssh_host_keys_project_mirror_path(@project, :json),
-      project_mirror_endpoint: project_mirror_path(@project, :json)
+      project_mirror_ssh_endpoint: ssh_host_keys_project_mirror_path(project, :json),
+      project_mirror_endpoint: project_mirror_path(project, :json),
+      project_id: project.id
     }
   end
 end
diff --git a/app/views/projects/mirrors/_mirror_repos.html.haml b/app/views/projects/mirrors/_mirror_repos.html.haml
index 21b105e6f80..7f139a794ae 100644
--- a/app/views/projects/mirrors/_mirror_repos.html.haml
+++ b/app/views/projects/mirrors/_mirror_repos.html.haml
@@ -11,7 +11,7 @@
       = link_to _('Read more'), help_page_path('workflow/repository_mirroring'), target: '_blank'
 
   .settings-content
-    = form_for @project, url: project_mirror_path(@project), html: { class: 'gl-show-field-errors js-mirror-form', autocomplete: 'false', data: mirrors_form_data_attributes } do |f|
+    = form_for @project, url: project_mirror_path(@project), html: { class: 'gl-show-field-errors js-mirror-form', autocomplete: 'false', data: mirrors_form_data_attributes(@project) } do |f|
       .panel.panel-default
         .panel-heading
           %h3.panel-title= _('Mirror a repository')
diff --git a/changelogs/unreleased/fix-destroy-remote-mirror.yml b/changelogs/unreleased/fix-destroy-remote-mirror.yml
new file mode 100644
index 00000000000..66287f28f07
--- /dev/null
+++ b/changelogs/unreleased/fix-destroy-remote-mirror.yml
@@ -0,0 +1,6 @@
+---
+title: Fix issue where deleting a project's remote mirror doesnt remove the mirror
+  from the database
+merge_request: 25224
+author:
+type: security
diff --git a/doc/api/README.md b/doc/api/README.md
index 89069fe60e1..f14b4a6e53e 100644
--- a/doc/api/README.md
+++ b/doc/api/README.md
@@ -52,6 +52,7 @@ The following API resources are available in the project context:
 | [Project-level variables](project_level_variables.md) | `/projects/:id/variables`                                                                                                                                                 |
 | [Project import/export](project_import_export.md)     | `/projects/:id/export`, `/projects/import`, `/projects/:id/import`                                                                                                        |
 | [Project milestones](milestones.md)                   | `/projects/:id/milestones`                                                                                                                                                |
+| [Project remote mirrors](project_remote_mirrors.md)   | `/projects/:id/remote_mirrors`                                                                                                                                            |
 | [Project snippets](project_snippets.md)               | `/projects/:id/snippets`                                                                                                                                                  |
 | [Project templates](project_templates.md)             | `/projects/:id/templates`                                                                                                                                                 |
 | [Protected branches](protected_branches.md)           | `/projects/:id/protected_branches`                                                                                                                                        |
diff --git a/doc/api/project_remote_mirrors.md b/doc/api/project_remote_mirrors.md
new file mode 100644
index 00000000000..bcf268bd421
--- /dev/null
+++ b/doc/api/project_remote_mirrors.md
@@ -0,0 +1,24 @@
+# Project remote mirrors API
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/54574) in GitLab 11.8
+
+A project's remote mirrors are its push mirrors. Remote mirrors are not pull mirrors.
+
+There is
+[an issue](https://gitlab.com/gitlab-org/gitlab-ce/issues/51763)
+to improve the naming of push and pull mirrors.
+
+## Delete remote mirror
+
+Deletes an existing project remote mirror. This returns a `204 No Content` status code if the operation was successful or `404` if the resource was not found.
+
+```
+DELETE /projects/:id/remote_mirrors/:remote_mirror_id
+```
+
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user |
+| `remote_mirror_id` | integer | yes | The id of the project's remote mirror |
+
+
diff --git a/lib/api/api.rb b/lib/api/api.rb
index 4dd1b459554..2719ef9baa2 100644
--- a/lib/api/api.rb
+++ b/lib/api/api.rb
@@ -138,6 +138,7 @@ module API
     mount ::API::ProjectImport
     mount ::API::ProjectHooks
     mount ::API::ProjectMilestones
+    mount ::API::ProjectRemoteMirrors
     mount ::API::Projects
     mount ::API::ProjectSnapshots
     mount ::API::ProjectSnippets
diff --git a/lib/api/project_remote_mirrors.rb b/lib/api/project_remote_mirrors.rb
new file mode 100644
index 00000000000..0a2c22d2974
--- /dev/null
+++ b/lib/api/project_remote_mirrors.rb
@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+module API
+  class ProjectRemoteMirrors < Grape::API
+    before do
+      authenticate!
+      authorize_admin_project
+    end
+
+    params do
+      requires :id, type: String, desc: 'The ID of a project'
+    end
+    resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
+      desc 'Delete a project remote mirror'
+      params do
+        requires :remote_mirror_id, type: Integer, desc: 'The ID of a project remote mirror'
+      end
+      delete ":id/remote_mirrors/:remote_mirror_id" do
+        remote_mirror = RemoteMirrorFinder.new(params.merge(id: params[:remote_mirror_id])).execute
+        not_found!('Remote Mirror') unless remote_mirror
+        destroy_conditionally!(remote_mirror)
+      end
+    end
+  end
+end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 3c739759979..c87a717d207 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -6132,6 +6132,9 @@ msgstr ""
 msgid "Remind later"
 msgstr ""
 
+msgid "Remote mirror not found."
+msgstr ""
+
 msgid "Remove"
 msgstr ""
 
diff --git a/spec/features/projects/settings/repository_settings_spec.rb b/spec/features/projects/settings/repository_settings_spec.rb
index 1259ad45791..1c80596fbe8 100644
--- a/spec/features/projects/settings/repository_settings_spec.rb
+++ b/spec/features/projects/settings/repository_settings_spec.rb
@@ -121,15 +121,11 @@ describe 'Projects > Settings > Repository settings' do
     end
 
     context 'remote mirror settings' do
-      let(:user2) { create(:user) }
-
       before do
-        project.add_maintainer(user2)
-
         visit project_settings_repository_path(project)
       end
 
-      it 'shows push mirror settings', :js do
+      it 'shows push mirror settings' do
         expect(page).to have_selector('#mirror_direction')
       end
 
@@ -217,5 +213,37 @@ 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)
+
+        click_delete_mirror
+        wait_for_requests
+
+        expect(mirrored_project.remote_mirrors.count).to eq(0)
+      end
+
+      it 'displays an error when the mirror does not exist' do
+        mirrored_project.remote_mirrors.first.destroy
+
+        click_delete_mirror
+        wait_for_requests
+
+        expect(page).to have_content('Remote mirror not found.')
+      end
+
+      def click_delete_mirror
+        find('.js-delete-mirror').click
+      end
+    end
   end
 end
diff --git a/spec/helpers/mirror_helper_spec.rb b/spec/helpers/mirror_helper_spec.rb
new file mode 100644
index 00000000000..3288bbcf45a
--- /dev/null
+++ b/spec/helpers/mirror_helper_spec.rb
@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe MirrorHelper do
+  let(:project) { build(:project) }
+
+  describe '#mirrors_form_data_attributes' do
+    it 'has required properties for repository mirroring' do
+      expect(helper.mirrors_form_data_attributes(project)).to include(project_id: project.id,
+                                                                      project_mirror_endpoint: project_mirror_path(project, :json),
+                                                                      project_mirror_ssh_endpoint: ssh_host_keys_project_mirror_path(project, :json))
+    end
+  end
+end
diff --git a/spec/requests/api/project_remote_mirrors_spec.rb b/spec/requests/api/project_remote_mirrors_spec.rb
new file mode 100644
index 00000000000..88dff7f569b
--- /dev/null
+++ b/spec/requests/api/project_remote_mirrors_spec.rb
@@ -0,0 +1,71 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe API::ProjectRemoteMirrors do
+  set(:project) { create(:project, :repository, :remote_mirror) }
+
+  describe 'DELETE /projects/:project_id/remote_mirrors/:id/' do
+    let(:remote_mirror) { project.remote_mirrors.first }
+    let(:endpoint) { endpoint_path(project.id, remote_mirror.id) }
+
+    case_name = lambda {|user_type| "like a project #{user_type}"}
+
+    context 'as an authorized user' do
+      let(:owner) { project.owner }
+      let(:maintainer) { project.add_maintainer(create(:user)).user }
+      let(:authorized_users) { { owner: owner, maintainer: maintainer } }
+
+      where(case_names: case_name, user_type: [:owner, :maintainer])
+
+      with_them do
+        let(:user) { authorized_users[user_type] }
+
+        it 'deletes remote mirror' do
+          delete api(endpoint, user)
+
+          expect(response).to have_gitlab_http_status(204)
+          expect(project.remote_mirrors.count).to eq(0)
+        end
+
+        it_behaves_like '412 response' do
+          let(:request) { api(endpoint, user) }
+        end
+
+        context 'for an invalid remote mirror id' do
+          it_behaves_like '404 response' do
+            let(:message) { '404 Remote Mirror Not Found' }
+            let(:request) { delete api(endpoint_path(project.id, '1234'), user) }
+          end
+        end
+      end
+    end
+
+    context 'as an unauthorized user' do
+      let(:developer) { project.add_developer(create(:user)).user }
+      let(:reporter) { project.add_reporter(create(:user)).user }
+      let(:guest) { project.add_guest(create(:user)).user }
+      let(:unauthorized_users) { { developer: developer, reporter: reporter, guest: guest } }
+
+      where(case_names: case_name, user_type: [:developer, :reporter, :guest])
+
+      with_them do
+        let(:user) { unauthorized_users[user_type] }
+
+        it_behaves_like '403 response' do
+          let(:request) { delete api(endpoint, user) }
+        end
+      end
+
+      context 'as an anonymous user' do
+        it_behaves_like '401 response' do
+          let(:request) { delete api(endpoint, nil) }
+        end
+      end
+    end
+  end
+
+  def endpoint_path(project_id, remote_mirror_id)
+    "/projects/#{project_id}/remote_mirrors/#{remote_mirror_id}"
+  end
+end
diff --git a/spec/support/shared_examples/requests/api/status_shared_examples.rb b/spec/support/shared_examples/requests/api/status_shared_examples.rb
index ebfc5fed3bb..c88ad02909c 100644
--- a/spec/support/shared_examples/requests/api/status_shared_examples.rb
+++ b/spec/support/shared_examples/requests/api/status_shared_examples.rb
@@ -19,6 +19,18 @@ shared_examples_for '400 response' do
   end
 end
 
+shared_examples_for '401 response' do
+  before do
+    # Fires the request
+    request
+  end
+
+  it 'returns 401' do
+    expect(response).to have_gitlab_http_status(401)
+    expect(json_response['message']).to eq('401 Unauthorized')
+  end
+end
+
 shared_examples_for '403 response' do
   before do
     # Fires the request