Restrict mirroring changes to admins only when mirroring is disabled

When admin disables mirroring from admin settings, we say only admin can
make changes to mirroring setting, but it seems that maintainers are
able to make changes currently. This commit fixes that and only admin
can make changes to project mirroring settings.

app/controllers/projects/mirrors_controller.rb

@@ -67,7 +67,7 @@ class Projects::MirrorsController < Projects::ApplicationController
   end
 
   def check_mirror_available!
-    Gitlab::CurrentSettings.current_application_settings.mirror_available || current_user&.admin?
+    render_404 unless can?(current_user, :admin_remote_mirror, project)
   end
 
   def mirror_params_attributes

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

 - expanded = expanded_by_default?
 - protocols = Gitlab::UrlSanitizer::ALLOWED_SCHEMES.join('|')
+- mirror_settings_enabled = can?(current_user, :admin_remote_mirror, @project)
+- mirror_settings_class = "#{'expanded' if expanded} #{'js-mirror-settings' if mirror_settings_enabled}".strip
 
-%section.settings.project-mirror-settings.js-mirror-settings.no-animate#js-push-remote-settings{ class: ('expanded' if expanded), data: { qa_selector: 'mirroring_repositories_settings_section' } }
+%section.settings.project-mirror-settings.no-animate#js-push-remote-settings{ class: mirror_settings_class, data: { qa_selector: 'mirroring_repositories_settings_section' } }
   .settings-header
     %h4= _('Mirroring repositories')
     %button.btn.js-settings-toggle
@@ -11,26 +13,32 @@
       = 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: 'new-password', data: mirrors_form_data_attributes } do |f|
-      .panel.panel-default
-        .panel-body
-          %div= form_errors(@project)
+    - if mirror_settings_enabled
+      = form_for @project, url: project_mirror_path(@project), html: { class: 'gl-show-field-errors js-mirror-form', autocomplete: 'new-password', data: mirrors_form_data_attributes } do |f|
+        .panel.panel-default
+          .panel-body
+            %div= form_errors(@project)
 
-          .form-group.has-feedback
-            = label_tag :url, _('Git repository URL'), class: 'label-light'
-            = text_field_tag :url, nil, class: 'form-control js-mirror-url js-repo-url qa-mirror-repository-url-input', placeholder: _('Input your repository URL'), required: true, pattern: "(#{protocols}):\/\/.+", autocomplete: 'new-password'
+            .form-group.has-feedback
+              = label_tag :url, _('Git repository URL'), class: 'label-light'
+              = text_field_tag :url, nil, class: 'form-control js-mirror-url js-repo-url qa-mirror-repository-url-input', placeholder: _('Input your repository URL'), required: true, pattern: "(#{protocols}):\/\/.+", autocomplete: 'new-password'
 
-          = render 'projects/mirrors/instructions'
+            = render 'projects/mirrors/instructions'
 
-          = render 'projects/mirrors/mirror_repos_form', f: f
+            = render 'projects/mirrors/mirror_repos_form', f: f
 
-          .form-check.append-bottom-10
-            = check_box_tag :only_protected_branches, '1', false, class: 'js-mirror-protected form-check-input'
-            = label_tag :only_protected_branches, _('Only mirror protected branches'), class: 'form-check-label'
-            = link_to icon('question-circle'), help_page_path('user/project/protected_branches'), target: '_blank'
+            .form-check.append-bottom-10
+              = check_box_tag :only_protected_branches, '1', false, class: 'js-mirror-protected form-check-input'
+              = label_tag :only_protected_branches, _('Only mirror protected branches'), class: 'form-check-label'
+              = link_to icon('question-circle'), help_page_path('user/project/protected_branches'), target: '_blank'
 
-        .panel-footer
-          = f.submit _('Mirror repository'), class: 'btn btn-success js-mirror-submit qa-mirror-repository-button', name: :update_remote_mirror
+          .panel-footer
+            = f.submit _('Mirror repository'), class: 'btn btn-success js-mirror-submit qa-mirror-repository-button', name: :update_remote_mirror
+    - else
+      .gl-alert.gl-alert-info{ role: 'alert' }
+        = sprite_icon('information-o', size: 16, css_class: 'gl-icon gl-alert-icon gl-alert-icon-no-title')
+        .gl-alert-body
+          = _('Mirror settings are only available to GitLab administrators.')
 
     .panel.panel-default
       .table-responsive
@@ -61,8 +69,10 @@
                   - if mirror.last_error.present?
                     .badge.mirror-error-badge{ data: { toggle: 'tooltip', html: 'true', qa_selector: 'mirror_error_badge' }, title: html_escape(mirror.last_error.try(:strip)) }= _('Error')
                 %td
-                  .btn-group.mirror-actions-group.pull-right{ role: 'group' }
-                    - if mirror.ssh_key_auth?
-                      = clipboard_button(text: mirror.ssh_public_key, class: 'btn btn-default', title: _('Copy SSH public key'), qa_selector: 'copy_public_key_button')
-                    = render 'shared/remote_mirror_update_button', remote_mirror: mirror
+                  - if mirror_settings_enabled
+                    .btn-group.mirror-actions-group.pull-right{ role: 'group' }
+                      - if mirror.ssh_key_auth?
+                        = clipboard_button(text: mirror.ssh_public_key, class: 'btn btn-default', title: _('Copy SSH public key'), qa_selector: 'copy_public_key_button')
+                      = render 'shared/remote_mirror_update_button', remote_mirror: mirror
                     %button.js-delete-mirror.qa-delete-mirror.rspec-delete-mirror.btn.btn-danger{ type: 'button', data: { mirror_id: mirror.id, toggle: 'tooltip', container: 'body' }, title: _('Remove') }= icon('trash-o')
+

changelogs/unreleased/security-disable-mirroring-fix.yml

+---
+title: Restrict mirroring changes to admins only when mirroring is disabled
+merge_request:
+author:
+type: security

locale/gitlab.pot

@@ -12699,6 +12699,9 @@ msgstr ""
 msgid "Mirror repository"
 msgstr ""
 
+msgid "Mirror settings are only available to GitLab administrators."
+msgstr ""
+
 msgid "Mirror user"
 msgstr ""
 

spec/controllers/projects/mirrors_controller_spec.rb

@@ -5,6 +5,72 @@ require 'spec_helper'
 describe Projects::MirrorsController do
   include ReactiveCachingHelpers
 
+  shared_examples 'only admin is allowed when mirroring is disabled' do
+    let(:subject_action) { raise 'subject_action is required' }
+    let(:user) { project.owner }
+    let(:project_settings_path) { project_settings_repository_path(project, anchor: 'js-push-remote-settings') }
+
+    context 'when project mirroring is enabled' do
+      it 'allows requests from a maintainer' do
+        sign_in(user)
+
+        subject_action
+        expect(response).to redirect_to(project_settings_path)
+      end
+
+      it 'allows requests from an admin user' do
+        user.update!(admin: true)
+        sign_in(user)
+
+        subject_action
+        expect(response).to redirect_to(project_settings_path)
+      end
+    end
+
+    context 'when project mirroring is disabled' do
+      before do
+        stub_application_setting(mirror_available: false)
+      end
+
+      it 'disallows requests from a maintainer' do
+        sign_in(user)
+
+        subject_action
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+
+      it 'allows requests from an admin user' do
+        user.update!(admin: true)
+        sign_in(user)
+
+        subject_action
+        expect(response).to redirect_to(project_settings_path)
+      end
+    end
+  end
+
+  describe 'Access control' do
+    let(:project) { create(:project, :repository) }
+
+    describe '#update' do
+      include_examples 'only admin is allowed when mirroring is disabled' do
+        let(:subject_action) do
+          do_put(project, remote_mirrors_attributes: { '0' => { 'enabled' => 1, 'url' => 'http://foo.com' } })
+        end
+      end
+    end
+
+    describe '#update_now' do
+      include_examples 'only admin is allowed when mirroring is disabled' do
+        let(:options) { { namespace_id: project.namespace, project_id: project } }
+
+        let(:subject_action) do
+          get :update_now, params: options.merge(sync_remote: true)
+        end
+      end
+    end
+  end
+
   describe 'setting up a remote mirror' do
     let_it_be(:project) { create(:project, :repository) }
 

spec/features/projects/settings/repository_settings_spec.rb

@@ -26,11 +26,7 @@ describe 'Projects > Settings > Repository settings' do
     let(:role) { :maintainer }
 
     context 'remote mirror settings' do
-      let(:user2) { create(:user) }
-
       before do
-        project.add_maintainer(user2)
-
         visit project_settings_repository_path(project)
       end
 
@@ -90,6 +86,18 @@ describe 'Projects > Settings > Repository settings' do
         expect(page).to have_selector('[title="Copy SSH public key"]')
       end
 
+      context 'when project mirroring is disabled' do
+        before do
+          stub_application_setting(mirror_available: false)
+          visit project_settings_repository_path(project)
+        end
+
+        it 'hides remote mirror settings' do
+          expect(page.find('.project-mirror-settings')).not_to have_selector('form')
+          expect(page).to have_content('Mirror settings are only available to GitLab administrators.')
+        end
+      end
+
       def select_direction(direction = 'push')
         direction_select = find('#mirror_direction')
 
@@ -154,4 +162,31 @@ describe 'Projects > Settings > Repository settings' do
       expect(mirror).not_to have_selector('.rspec-update-now-button')
     end
   end
+
+  context 'for admin' do
+    shared_examples_for 'shows mirror settings' do
+      it 'shows mirror settings' do
+        expect(page.find('.project-mirror-settings')).to have_selector('form')
+        expect(page).not_to have_content('Changing mirroring setting is disabled for non-admin users.')
+      end
+    end
+
+    before do
+      stub_application_setting(mirror_available: mirror_available)
+      user.update!(admin: true)
+      visit project_settings_repository_path(project)
+    end
+
+    context 'when project mirroring is enabled' do
+      let(:mirror_available) { true }
+
+      include_examples 'shows mirror settings'
+    end
+
+    context 'when project mirroring is disabled' do
+      let(:mirror_available) { false }
+
+      include_examples 'shows mirror settings'
+    end
+  end
 end