Merge branch '3730-allow-admins-to-disable-mirroring' into 'master'

Allows admins to disable mirroring

Closes #3730

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

app/views/admin/application_settings/_form.html.haml

@@ -128,7 +128,6 @@
 
   - if License.feature_available?(:repository_mirrors)
     = render partial: 'repository_mirrors_form', locals: { f: f }
-    = render partial: 'repository_remote_mirrors_form', locals: { f: f }
 
   %fieldset
     %legend Sign-up Restrictions

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

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

db/migrate/20171129150921_rename_remote_mirror_available_to_mirror_available.rb

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

db/migrate/20171129152659_add_pull_mirror_available_overridden_to_projects.rb

+class AddPullMirrorAvailableOverriddenToProjects < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def change
+    add_column :projects, :pull_mirror_available_overridden, :boolean
+  end
+end

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

@@ -162,7 +162,6 @@ ActiveRecord::Schema.define(version: 20171205190711) do
     t.integer "circuitbreaker_failure_wait_time", default: 30
     t.integer "circuitbreaker_failure_reset_time", default: 1800
     t.integer "circuitbreaker_storage_timeout", default: 30
-    t.boolean "remote_mirror_available", default: true, null: false
     t.integer "circuitbreaker_access_retries", default: 3
     t.integer "circuitbreaker_backoff_threshold", default: 80
     t.boolean "throttle_unauthenticated_enabled", default: false, null: false
@@ -179,6 +178,7 @@ ActiveRecord::Schema.define(version: 20171205190711) do
     t.integer "gitaly_timeout_default", default: 55, null: false
     t.integer "gitaly_timeout_medium", default: 30, null: false
     t.integer "gitaly_timeout_fast", default: 10, null: false
+    t.boolean "mirror_available", default: true, null: false
   end
 
   create_table "approvals", force: :cascade do |t|
@@ -1856,6 +1856,7 @@ ActiveRecord::Schema.define(version: 20171205190711) do
     t.boolean "resolve_outdated_diff_discussions"
     t.boolean "remote_mirror_available_overridden"
     t.boolean "only_mirror_protected_branches"
+    t.boolean "pull_mirror_available_overridden"
   end
 
   add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree

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,7 +2,7 @@ class Projects::MirrorsController < Projects::ApplicationController
   include RepositorySettingsRedirect
   include SafeMirrorParams
   # Authorize
-  before_action :authorize_admin_project!
+  before_action :authorize_admin_mirror!
   before_action :remote_mirror, only: [:update]
   before_action :check_repository_mirrors_available!
 
@@ -73,12 +73,8 @@ class Projects::MirrorsController < Projects::ApplicationController
     @remote_mirror = @project.remote_mirrors.first_or_initialize
   end
 
-  def remote_mirror_attributes
-    { remote_mirrors_attributes: %i[url id enabled only_protected_branches] }
-  end
-
   def mirror_params_attributes
-    attributes = [
+    [
       :mirror,
       :import_url,
       :username_only_import_url,
@@ -92,14 +88,15 @@ class Projects::MirrorsController < Projects::ApplicationController
         password
         ssh_known_hosts
         regenerate_ssh_private_key
+      ],
+
+      remote_mirrors_attributes: %i[
+        url
+        id
+        enabled
+        only_protected_branches
       ]
     ]
-
-    if can?(current_user, :admin_remote_mirror, project)
-      attributes << remote_mirror_attributes
-    end
-
-    attributes
   end
 
   def mirror_params

ee/app/helpers/ee/application_settings_helper.rb

@@ -23,7 +23,7 @@ module EE
         :slack_app_secret,
         :slack_app_verification_token,
         :allow_group_owners_to_manage_ldap,
-        :remote_mirror_available
+        :mirror_available
       ]
     end
 

ee/app/models/ee/application_setting.rb

@@ -41,7 +41,7 @@ module EE
           mirror_max_capacity: Settings.gitlab['mirror_max_capacity'],
           mirror_capacity_threshold: Settings.gitlab['mirror_capacity_threshold'],
           allow_group_owners_to_manage_ldap: true,
-          remote_mirror_available: true
+          mirror_available: true
         )
       end
     end

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
 
@@ -500,11 +501,21 @@ module EE
 
     def remote_mirror_available?
       remote_mirror_available_overridden ||
-        current_application_settings.remote_mirror_available
+        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/models/remote_mirror.rb

@@ -21,7 +21,7 @@ class RemoteMirror < ActiveRecord::Base
   validate  :url_availability, if: -> (mirror) { mirror.url_changed? || mirror.enabled? }
   validates :url, addressable_url: true, if: :url_changed?
 
-  after_save :set_override_remote_mirror_available, unless: -> { Gitlab::CurrentSettings.current_application_settings.remote_mirror_available }
+  after_save :set_override_remote_mirror_available, unless: -> { Gitlab::CurrentSettings.current_application_settings.mirror_available }
   after_save :refresh_remote, if: :mirror_url_changed?
   after_update :reset_fields, if: :mirror_url_changed?
   after_destroy :remove_remote

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,8 +29,8 @@ module EE
       end
 
       with_scope :global
-      condition(:remote_mirror_available) do
-        ::Gitlab::CurrentSettings.current_application_settings.remote_mirror_available
+      condition(:mirror_available, score: 0) do
+        ::Gitlab::CurrentSettings.current_application_settings.mirror_available
       end
 
       rule { admin }.enable :change_repository_storage
@@ -60,7 +63,7 @@ module EE
 
       rule { can?(:developer_access) }.enable :admin_board
 
-      rule { (remote_mirror_available & can?(:admin_project)) | admin }.enable :admin_remote_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.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/admin/application_settings/_repository_remote_mirrors_form.html.haml

-%fieldset
-  %legend Repository Remote mirror settings
-  .form-group
-    = f.label :remote_mirror_available, 'Enable remote mirror configuration', class: 'control-label col-sm-2'
-    .col-sm-10
-      .checkbox
-        = f.label :remote_mirror_available do
-          = f.check_box :remote_mirror_available
-          Allow remote mirrors to be setup for projects
-      %span.help-block
-        If disabled, only admins will be able to setup remote mirrors in projects.
-        = link_to icon('question-circle'), help_page_path('workflow/repository_mirroring', anchor: 'pushing-to-a-remote-repository')

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'
-  - if can?(current_user, :admin_remote_mirror, @project)
-    = render 'projects/mirrors/push'
+  = 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

lib/gitlab/import_export/import_export.yml

@@ -108,6 +108,7 @@ excluded_attributes:
     - :storage_version
     - :remote_mirror_available_overridden
     - :only_mirror_protected_branches
+    - :pull_mirror_available_overridden
   snippets:
     - :expired_at
   merge_request_diff:

spec/controllers/projects/mirrors_controller_spec.rb

@@ -9,7 +9,7 @@ describe Projects::MirrorsController do
 
     context 'when remote mirrors are disabled' do
       before do
-        stub_application_setting(remote_mirror_available: false)
+        stub_application_setting(mirror_available: false)
       end
 
       context 'when user is admin' do
@@ -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!)
 
-      it 'is able to disable the mirror' do
-        expect { do_put(project, mirror: false) }.to change { Project.mirror.count }.to(0)
+          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)
+        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(remote_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

@@ -451,7 +451,7 @@ describe Project do
     end
 
     it 'does nothing when remote mirror is disabled globally and not overridden' do
-      stub_application_setting(remote_mirror_available: false)
+      stub_application_setting(mirror_available: false)
       project.remote_mirror_available_overridden = false
 
       expect_any_instance_of(RemoteMirror).not_to receive(:sync)
@@ -1051,7 +1051,7 @@ describe Project do
 
     context 'when remote mirror global setting is disabled' do
       before do
-        stub_application_setting(remote_mirror_available: false)
+        stub_application_setting(mirror_available: false)
       end
 
       it 'returns true when overridden' do
@@ -1066,6 +1066,32 @@ describe Project do
     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
+
   describe '#username_only_import_url' do
     where(:import_url, :username, :expected_import_url) do
       '' | 'foo' | ''

spec/ee/spec/policies/project_policy_spec.rb

@@ -10,7 +10,7 @@ describe ProjectPolicy do
     project.add_developer(developer)
   end
 
-  context 'admin_remote_mirror' do
+  context 'admin_mirror' do
     context 'with remote mirror setting enabled' do
       context 'with admin' do
         subject do
@@ -18,7 +18,7 @@ describe ProjectPolicy do
         end
 
         it do
-          is_expected.to be_allowed(:admin_remote_mirror)
+          is_expected.to be_allowed(:admin_mirror)
         end
       end
 
@@ -28,7 +28,7 @@ describe ProjectPolicy do
         end
 
         it do
-          is_expected.to be_allowed(:admin_remote_mirror)
+          is_expected.to be_allowed(:admin_mirror)
         end
       end
 
@@ -38,14 +38,14 @@ describe ProjectPolicy do
         end
 
         it do
-          is_expected.to be_disallowed(:admin_remote_mirror)
+          is_expected.to be_disallowed(:admin_mirror)
         end
       end
     end
 
     context 'with remote mirror setting disabled' do
       before do
-        stub_application_setting(remote_mirror_available: false)
+        stub_application_setting(mirror_available: false)
       end
 
       context 'with admin' do
@@ -54,7 +54,7 @@ describe ProjectPolicy do
         end
 
         it do
-          is_expected.to be_allowed(:admin_remote_mirror)
+          is_expected.to be_allowed(:admin_mirror)
         end
       end
 
@@ -64,7 +64,59 @@ describe ProjectPolicy do
         end
 
         it do
-          is_expected.to be_disallowed(:admin_remote_mirror)
+          is_expected.to be_disallowed(:admin_mirror)
+        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

spec/services/git_push_service_spec.rb

@@ -38,7 +38,7 @@ describe GitPushService do
 
     context 'when remote mirror feature is disabled' do
       before do
-        stub_application_setting(remote_mirror_available: false)
+        stub_application_setting(mirror_available: false)
       end
 
       context 'with remote mirrors global setting overridden' do