Skip pull mirrors if they are globally disabled and not overridden.

changelogs/unreleased-ee/3730-allow-admins-to-disable-mirroring.yml

+---
+title: Allow admins to disable mirroring
+merge_request: 3586
+author:
+type: added

db/post_migrate/20171201140229_cleanup_remote_mirror_available_rename.rb

+class CleanupRemoteMirrorAvailableRename < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    cleanup_concurrent_column_rename :application_settings, :remote_mirror_available, :mirror_available
+  end
+
+  def down
+    rename_column_concurrently :application_settings, :mirror_available, :remote_mirror_available
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20171129152659) do
+ActiveRecord::Schema.define(version: 20171201140229) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"

doc/user/admin_area/settings/visibility_and_access_controls.md

@@ -37,14 +37,14 @@ not selected.
   HTTP, will still be accessible. What GitLab does is restrict access on the
   application level.
 
-## Allow remote mirrors to be setup for projects
+## Allow mirrors to be setup for projects
 
-> [Introduced][ee-3130] in Gitlab 10.2.
+> [Introduced][ee-3586] in Gitlab 10.3.
 
-This option is enabled by default. By disabling it, push mirroring will no longer
+This option is enabled by default. By disabling it, both pull and push mirroring will no longer
 work in every repository and can only be re-enabled on a per-project basis by an admin.
 
-![Remote mirror settings](img/remote_mirror_settings.png)
+![Mirror settings](img/mirror_settings.png)
 
 [ce-4696]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4696
-[ee-3130]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/3130
+[ee-3586]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/3586

ee/app/controllers/projects/mirrors_controller.rb

@@ -2,8 +2,7 @@ class Projects::MirrorsController < Projects::ApplicationController
   include RepositorySettingsRedirect
   include SafeMirrorParams
   # Authorize
-  before_action :authorize_admin_mirror
-  before_action :authorize_admin_project!
+  before_action :authorize_admin_mirror!
   before_action :remote_mirror, only: [:update]
   before_action :check_repository_mirrors_available!
 
@@ -91,7 +90,7 @@ class Projects::MirrorsController < Projects::ApplicationController
         regenerate_ssh_private_key
       ],
 
-      remote_mirror_attributes: %i[
+      remote_mirrors_attributes: %i[
         url
         id
         enabled

ee/app/models/ee/project.rb

@@ -13,6 +13,7 @@ module EE
 
       before_validation :mark_remote_mirrors_for_removal
 
+      before_save :set_override_pull_mirror_available, unless: -> { ::Gitlab::CurrentSettings.current_application_settings.mirror_available }
       after_save :create_mirror_data, if: ->(project) { project.mirror? && project.mirror_changed? }
       after_save :destroy_mirror_data, if: ->(project) { !project.mirror? && project.mirror_changed? }
 
@@ -91,7 +92,7 @@ module EE
     end
 
     def mirror
-      super && feature_available?(:repository_mirrors)
+      super && feature_available?(:repository_mirrors) && pull_mirror_available?
     end
     alias_method :mirror?, :mirror
 
@@ -503,8 +504,18 @@ module EE
         current_application_settings.mirror_available
     end
 
+    def pull_mirror_available?
+      pull_mirror_available_overridden ||
+        current_application_settings.mirror_available
+    end
+
     private
 
+    def set_override_pull_mirror_available
+      self.pull_mirror_available_overridden = read_attribute(:mirror)
+      true
+    end
+
     def licensed_feature_available?(feature)
       @licensed_feature_available ||= Hash.new do |h, feature|
         h[feature] = load_licensed_feature_available(feature)

ee/app/policies/ee/project_policy.rb

@@ -9,6 +9,9 @@ module EE
       with_scope :subject
       condition(:related_issues_disabled) { !@subject.feature_available?(:related_issues) }
 
+      with_scope :subject
+      condition(:repository_mirrors_enabled) { @subject.feature_available?(:repository_mirrors) }
+
       with_scope :subject
       condition(:deploy_board_disabled) { !@subject.feature_available?(:deploy_board) }
 
@@ -26,7 +29,7 @@ module EE
       end
 
       with_scope :global
-      condition(:mirror_available) do
+      condition(:mirror_available, score: 0) do
         ::Gitlab::CurrentSettings.current_application_settings.mirror_available
       end
 
@@ -60,7 +63,7 @@ module EE
 
       rule { can?(:developer_access) }.enable :admin_board
 
-      rule { (mirror_available & can?(:admin_project)) | admin }.enable :admin_mirror
+      rule { repository_mirrors_enabled & ((mirror_available & can?(:admin_project)) | admin) }.enable :admin_mirror
 
       rule { deploy_board_disabled & ~is_development }.prevent :read_deploy_board
 

ee/app/views/admin/application_settings/_repository_mirrors_form.html.haml

-- if Gitlab.com? && License.feature_available?(:repository_mirrors)
+- if License.feature_available?(:repository_mirrors)
   %fieldset
     %legend Repository mirror settings
     .form-group
       = f.label :mirror_available, 'Enable mirror configuration', class: 'control-label col-sm-2'
       .col-sm-10
         .checkbox
-          = f.label :mirror_available, do
+          = f.label :mirror_available do
             = f.check_box :mirror_available
             Allow mirrors to be setup for projects
         %span.help-block
           If disabled, only admins will be able to setup mirrors in projects.
           = link_to icon('question-circle'), help_page_path('workflow/repository_mirroring')
+
+  - if Gitlab.com?
     .form-group
       = f.label :mirror_max_delay, class: 'control-label col-sm-2' do
         Maximum delay (Minutes)

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

 - content_for :page_specific_javascripts do
   = webpack_bundle_tag 'mirrors'
 
-- if @project.feature_available?(:repository_mirrors)
-  - if can?(current_user, :admin_mirror, @project)
-    = render 'projects/mirrors/pull'
-    = render 'projects/mirrors/push'
+- if can?(current_user, :admin_mirror, @project)
+  = render 'projects/mirrors/pull'
+  = render 'projects/mirrors/push'

ee/app/workers/update_all_mirrors_worker.rb

@@ -31,7 +31,7 @@ class UpdateAllMirrorsWorker
       last = projects.last.mirror_data.next_execution_timestamp
 
       projects.each do |project|
-        next unless project.feature_available?(:repository_mirrors)
+        next unless project.mirror?
 
         capacity -= 1
         project.import_schedule

spec/controllers/projects/mirrors_controller_spec.rb

@@ -46,7 +46,7 @@ describe Projects::MirrorsController do
         expect_any_instance_of(EE::Project).to receive(:force_import_job!)
 
         expect do
-          do_put(project, remote_mirrors_attributes: { '0' => { 'enabled' => 1, 'url' => 'http://foo.com' } })
+          do_put(project, remote_mirrors_attributes: { '0' => { 'enabled' => 1, 'url' => url } })
         end.to change { RemoteMirror.count }.to(1)
       end
 
@@ -121,27 +121,59 @@ describe Projects::MirrorsController do
   end
 
   describe 'setting up a mirror' do
-    before do
-      sign_in(project.owner)
-    end
+    let(:url) { 'http://foo.com' }
+    let(:project) { create(:project, :repository) }
 
-    context 'when project does not have a mirror' do
-      let(:project) { create(:project) }
+    context 'when mirrors are disabled' do
+      before do
+        stub_application_setting(mirror_available: false)
+      end
 
-      it 'allows to create a mirror' do
-        expect_any_instance_of(EE::Project).to receive(:force_import_job!)
+      context 'when user is admin' do
+        let(:admin) { create(:user, :admin) }
 
-        expect do
-          do_put(project, mirror: true, mirror_user_id: project.owner.id, import_url: 'http://foo.com')
-        end.to change { Project.mirror.count }.to(1)
+        it 'creates a new mirror' do
+          sign_in(admin)
+          expect_any_instance_of(EE::Project).to receive(:force_import_job!)
+
+          expect do
+            do_put(project, mirror: true, mirror_user_id: admin.id, import_url: url)
+          end.to change { Project.mirror.count }.to(1)
+        end
+      end
+
+      context 'when user is not an admin' do
+        it 'does not create a new mirror' do
+          sign_in(project.owner)
+
+          expect do
+            do_put(project, mirror: true, mirror_user_id: project.owner.id, import_url: url)
+          end.not_to change { Project.mirror.count }
+        end
       end
     end
 
-    context 'when project has a mirror' do
-      let(:project) { create(:project, :mirror, :import_finished) }
+    context 'when mirrors are enabled' do
+      before do
+        sign_in(project.owner)
+      end
+
+      context 'when project does not have a mirror' do
+        it 'allows to create a mirror' do
+          expect_any_instance_of(EE::Project).to receive(:force_import_job!)
+
+          expect do
+            do_put(project, mirror: true, mirror_user_id: project.owner.id, import_url: url)
+          end.to change { Project.mirror.count }.to(1)
+        end
+      end
+
+      context 'when project has a mirror' do
+        let(:project) { create(:project, :mirror, :import_finished) }
 
-      it 'is able to disable the mirror' do
-        expect { do_put(project, mirror: false) }.to change { Project.mirror.count }.to(0)
+        it 'is able to disable the mirror' do
+          expect { do_put(project, mirror: false) }.to change { Project.mirror.count }.to(0)
+        end
       end
     end
   end
@@ -157,25 +189,6 @@ describe Projects::MirrorsController do
     end
   end
 
-  describe 'forcing an update on a push mirror' do
-    context 'when remote mirrors are disabled' do
-      let(:project) { create(:project, :repository, :remote_mirror) }
-
-      before do
-        stub_application_setting(mirror_available: false)
-        sign_in(project.owner)
-      end
-
-      it 'updates now when overridden' do
-        project.update(remote_mirror_available_overridden: true)
-
-        expect_any_instance_of(EE::Project).to receive(:update_remote_mirrors)
-
-        put :update_now, { namespace_id: project.namespace.to_param, project_id: project.to_param, sync_remote: 1 }
-      end
-    end
-  end
-
   describe '#update' do
     let(:project) { create(:project, :repository, :mirror, :remote_mirror) }
     let(:attributes) { { project: { mirror_user_id: project.owner.id, mirror_trigger_builds: 0 }, namespace_id: project.namespace.to_param, project_id: project.to_param } }

spec/ee/spec/models/ee/project_spec.rb

@@ -1040,12 +1040,12 @@ describe Project do
     end
   end
 
-  describe '#mirror_available?' do
+  describe '#remote_mirror_available?' do
     let(:project) { create(:project) }
 
     context 'when remote mirror global setting is enabled' do
       it 'returns true' do
-        expect(project.mirror_available?).to be(true)
+        expect(project.remote_mirror_available?).to be(true)
       end
     end
 
@@ -1057,11 +1057,37 @@ describe Project do
       it 'returns true when overridden' do
         project.remote_mirror_available_overridden = true
 
-        expect(project.mirror_available?).to be(true)
+        expect(project.remote_mirror_available?).to be(true)
       end
 
       it 'returns false when not overridden' do
-        expect(project.mirror_available?).to be(false)
+        expect(project.remote_mirror_available?).to be(false)
+      end
+    end
+  end
+
+  describe '#pull_mirror_available?' do
+    let(:project) { create(:project) }
+
+    context 'when mirror global setting is enabled' do
+      it 'returns true' do
+        expect(project.pull_mirror_available?).to be(true)
+      end
+    end
+
+    context 'when mirror global setting is disabled' do
+      before do
+        stub_application_setting(mirror_available: false)
+      end
+
+      it 'returns true when overridden' do
+        project.pull_mirror_available_overridden = true
+
+        expect(project.pull_mirror_available?).to be(true)
+      end
+
+      it 'returns false when not overridden' do
+        expect(project.pull_mirror_available?).to be(false)
       end
     end
   end

spec/ee/spec/policies/project_policy_spec.rb

@@ -68,5 +68,57 @@ describe ProjectPolicy do
         end
       end
     end
+
+    context 'with remote mirrors feature disabled' do
+      before do
+        stub_licensed_features(repository_mirrors: false)
+      end
+
+      context 'with admin' do
+        subject do
+          described_class.new(admin, project)
+        end
+
+        it do
+          is_expected.to be_disallowed(:admin_mirror)
+        end
+      end
+
+      context 'with owner' do
+        subject do
+          described_class.new(owner, project)
+        end
+
+        it do
+          is_expected.to be_disallowed(:admin_mirror)
+        end
+      end
+    end
+
+    context 'with remote mirrors feature enabled' do
+      before do
+        stub_licensed_features(repository_mirrors: true)
+      end
+
+      context 'with admin' do
+        subject do
+          described_class.new(admin, project)
+        end
+
+        it do
+          is_expected.to be_allowed(:admin_mirror)
+        end
+      end
+
+      context 'with owner' do
+        subject do
+          described_class.new(owner, project)
+        end
+
+        it do
+          is_expected.to be_allowed(:admin_mirror)
+        end
+      end
+    end
   end
 end