Resolves feedback round requests

ee/app/controllers/ee/projects/mirrors_controller.rb

@@ -4,10 +4,6 @@ module EE
       extend ::Gitlab::Utils::Override
       extend ActiveSupport::Concern
 
-      prepended do
-        include SafeMirrorParams
-      end
-
       def ssh_host_keys
         lookup = SshHostKey.new(project: project, url: params[:ssh_url])
 
@@ -99,8 +95,6 @@ module EE
       def safe_mirror_params
         params = mirror_params
 
-        params[:mirror_user_id] = current_user.id unless valid_mirror_user?(params)
-
         import_data = params[:import_data_attributes]
         if import_data.present?
           # Prevent Rails from destroying the existing import data

ee/app/controllers/ee/projects_controller.rb

@@ -9,7 +9,7 @@ module EE
     private
 
     def project_params_ee
-      %i[
+      attrs = %i[
         approvals_before_merge
         approver_group_ids
         approver_ids
@@ -19,11 +19,22 @@ module EE
         repository_size_limit
         reset_approvals_on_push
         service_desk_enabled
+        external_authorization_classification_label
+        ci_cd_only
+      ]
+
+      if allow_mirror_params?
+        attrs + mirror_params
+      else
+        attrs
+      end
+    end
+
+    def mirror_params
+      %i[
         mirror
         mirror_trigger_builds
         mirror_user_id
-        external_authorization_classification_label
-        ci_cd_only
       ]
     end
 
@@ -40,5 +51,13 @@ module EE
     def active_new_project_tab
       project_params[:ci_cd_only] == 'true' ? 'ci_cd_only' : super
     end
+
+    def allow_mirror_params?
+      if @project # rubocop:disable Gitlab/ModuleWithInstanceVariables
+        can?(current_user, :admin_mirror, @project) # rubocop:disable Gitlab/ModuleWithInstanceVariables
+      else
+        ::Gitlab::CurrentSettings.current_application_settings.mirror_available || current_user&.admin?
+      end
+    end
   end
 end

ee/app/services/ee/projects/create_service.rb

@@ -7,8 +7,8 @@ module EE
       override :execute
       def execute
         limit = params.delete(:repository_size_limit)
-        mirror = params.delete(:mirror)
-        mirror_user_id = params.delete(:mirror_user_id)
+        mirror = ::Gitlab::Utils.to_boolean(params.delete(:mirror))
+        mirror_user_id = current_user.id if mirror
         mirror_trigger_builds = params.delete(:mirror_trigger_builds)
         ci_cd_only = ::Gitlab::Utils.to_boolean(params.delete(:ci_cd_only))
 
@@ -16,7 +16,7 @@ module EE
           # Repository size limit comes as MB from the view
           project.repository_size_limit = ::Gitlab::Utils.try_megabytes_to_bytes(limit) if limit
 
-          if mirror && project.feature_available?(:repository_mirrors)
+          if mirror && can?(current_user, :admin_mirror, project)
             project.mirror = mirror unless mirror.nil?
             project.mirror_trigger_builds = mirror_trigger_builds unless mirror_trigger_builds.nil?
             project.mirror_user_id = mirror_user_id

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

@@ -7,19 +7,16 @@ module EE
 
       override :execute
       def execute
-        unless project.feature_available?(:repository_mirrors)
-          params.delete(:mirror)
-          params.delete(:mirror_user_id)
-          params.delete(:mirror_trigger_builds)
-          params.delete(:only_mirror_protected_branches)
-          params.delete(:mirror_overwrites_diverged_branches)
-          params.delete(:import_data_attributes)
-        end
-
         should_remove_old_approvers = params.delete(:remove_old_approvers)
         wiki_was_enabled = project.wiki_enabled?
 
         limit = params.delete(:repository_size_limit)
+
+        unless valid_mirror_user?
+          project.errors.add(:mirror_user_id, 'is invalid')
+          return project
+        end
+
         result = super do
           # Repository size limit comes as MB from the view
           project.repository_size_limit = ::Gitlab::Utils.try_megabytes_to_bytes(limit) if limit
@@ -52,6 +49,15 @@ module EE
 
       private
 
+      def valid_mirror_user?
+        return true unless params[:mirror_user_id].present?
+
+        mirror_user_id = params[:mirror_user_id].to_i
+
+        mirror_user_id == current_user.id ||
+          mirror_user_id == project.mirror_user&.id
+      end
+
       def log_audit_events
         EE::Audit::ProjectChangesAuditor.new(current_user, project).execute
       end

ee/app/views/shared/ee/_import_form.html.haml

@@ -10,5 +10,3 @@
     - if Gitlab::CurrentSettings.should_check_namespace_plan?
       .form-text.text-muted
         Mirroring will only be available if the feature is included in the plan of the selected group or user.
-
-  = f.hidden_field :mirror_user_id, value: current_user.id

ee/changelogs/unreleased/1499-api-endpoint-for-configuring-pull-mirroring-via-http.yml

+---
+title: Enables configuration of pull mirroring through API
+merge_request: 6485
+author:
+type: added

ee/lib/ee/api/entities.rb

@@ -19,18 +19,10 @@ module EE
           expose :repository_storage, if: ->(_project, options) { options[:current_user].try(:admin?) }
           expose :approvals_before_merge, if: ->(project, _) { project.feature_available?(:merge_request_approvers) }
           expose :mirror, if: ->(project, _) { project.feature_available?(:repository_mirrors) }
-          expose :mirror_user_id, if: ->(project, _) { mirroring_available? }
-          expose :mirror_trigger_builds, if: ->(project, _) { mirroring_available? }
-          expose :only_mirror_protected_branches, if: ->(project, _) { mirroring_available? }
-          expose :mirror_overwrites_diverged_branches, if: ->(project, _) { mirroring_available? }
-
-          private
-
-          alias_method :project, :object
-
-          def mirroring_available?
-            project.mirror? && project.feature_available?(:repository_mirrors)
-          end
+          expose :mirror_user_id, if: ->(project, _) { project.mirror? }
+          expose :mirror_trigger_builds, if: ->(project, _) { project.mirror? }
+          expose :only_mirror_protected_branches, if: ->(project, _) { project.mirror? }
+          expose :mirror_overwrites_diverged_branches, if: ->(project, _) { project.mirror? }
         end
       end
 

ee/lib/ee/api/helpers.rb

@@ -61,20 +61,6 @@ module EE
             ::Gitlab::CurrentSettings.current_application_settings
               .allow_group_owners_to_manage_ldap
       end
-
-      def mirroring_available?
-        ::Gitlab::CurrentSettings.current_application_settings.mirror_available ||
-          current_user&.admin?
-      end
-
-      def valid_mirror_user?(mirror_params)
-        return true unless mirror_params[:mirror_user_id].present?
-
-        mirror_user_id = mirror_params[:mirror_user_id].to_i
-
-        mirror_user_id == current_user.id ||
-          mirror_user_id == user_project.mirror_user&.id
-      end
     end
   end
 end

ee/lib/ee/api/projects.rb

@@ -29,31 +29,21 @@ module EE
             projects
           end
 
-          override :verify_create_projects_attrs!
-          def verify_create_projects_attrs!(attrs)
-            super
-
-            verify_mirror_attrs!(attrs)
-
-            if attrs[:mirror]
-              attrs[:mirror_user_id] = current_user.id
-            end
-          end
-
           override :verify_update_project_attrs!
-          def verify_update_project_attrs!(attrs)
+          def verify_update_project_attrs!(project, attrs)
             super
 
-            verify_mirror_attrs!(attrs)
-
-            unless valid_mirror_user?(attrs)
-              render_api_error!("Invalid mirror user", 400)
-            end
+            verify_mirror_attrs!(project, attrs)
           end
 
-          def verify_mirror_attrs!(attrs)
-            if attrs[:mirror].present? && !mirroring_available?
-              render_api_error!("Pull mirroring is not available", 403)
+          def verify_mirror_attrs!(project, attrs)
+            unless can?(current_user, :admin_mirror, project)
+              attrs.delete(:mirror)
+              attrs.delete(:mirror_user_id)
+              attrs.delete(:mirror_trigger_builds)
+              attrs.delete(:only_mirror_protected_branches)
+              attrs.delete(:mirror_overwrites_diverged_branches)
+              attrs.delete(:import_data_attributes)
             end
           end
         end

ee/spec/controllers/projects/mirrors_controller_spec.rb

@@ -29,7 +29,7 @@ describe Projects::MirrorsController do
       end
 
       context 'when trying to create a mirror with the same URL' do
-        it 'should not setup the mirror' do
+        it 'does not setup the mirror' do
           do_put(project, mirror: true, import_url: remote_mirror.url)
 
           expect(project.reload.mirror).to be_falsey
@@ -38,7 +38,7 @@ describe Projects::MirrorsController do
       end
 
       context 'when trying to create a mirror with a different URL' do
-        it 'should setup the mirror' do
+        it 'sets up the mirror' do
           do_put(project, mirror: true, mirror_user_id: project.owner.id, import_url: 'http://local.dev')
 
           expect(project.reload.mirror).to eq(true)
@@ -46,14 +46,14 @@ describe Projects::MirrorsController do
         end
 
         context 'mirror user is not the current user' do
-          it 'should only assign the current user' do
+          it 'does not setup the mirror' do
             new_user = create(:user)
             project.add_maintainer(new_user)
 
             do_put(project, mirror: true, mirror_user_id: new_user.id, import_url: 'http://local.dev')
 
-            expect(project.reload.mirror).to eq(true)
-            expect(project.reload.mirror_user.id).to eq(project.owner.id)
+            expect(project.reload.mirror).to be_falsey
+            expect(project.reload.import_url).to be_blank
           end
         end
       end
@@ -185,15 +185,13 @@ describe Projects::MirrorsController do
       end
 
       it 'only allows the current user to be the mirror user' do
-        mirror_user = project.mirror_user
-
         other_user = create(:user)
         project.add_maintainer(other_user)
 
         do_put(project, { mirror_user_id: other_user.id }, format: :json)
 
-        expect(response).to have_gitlab_http_status(200)
-        expect(project.mirror_user(true)).to eq(mirror_user)
+        expect(response).to have_gitlab_http_status(422)
+        expect(json_response['mirror_user_id'].first).to eq("is invalid")
       end
     end
 

ee/spec/controllers/projects_controller_spec.rb

@@ -20,7 +20,6 @@ describe ProjectsController do
         namespace_id: user.namespace.id,
         visibility_level: Gitlab::VisibilityLevel::PUBLIC,
         mirror: true,
-        mirror_user_id: user.id,
         mirror_trigger_builds: true
       }
     end

ee/spec/requests/api/projects_spec.rb

@@ -50,11 +50,11 @@ describe API::Projects do
           stub_ee_application_setting(mirror_available: false)
         end
 
-        it 'returns a 403' do
+        it 'ignores the mirroring options' do
           post api('/projects', user), mirror_params
 
-          expect(response).to have_gitlab_http_status(403)
-          expect(json_response["message"]).to eq("Pull mirroring is not available")
+          expect(response).to have_gitlab_http_status(201)
+          expect(Project.first.mirror?).to be false
         end
 
         it 'creates project with mirror settings' do
@@ -107,11 +107,11 @@ describe API::Projects do
           stub_ee_application_setting(mirror_available: false)
         end
 
-        it 'raises an API error' do
+        it 'does not update mirror related attributes' do
           put(api("/projects/#{project.id}", user), mirror_params)
 
-          expect(response).to have_gitlab_http_status(403)
-          expect(json_response["message"]).to eq("Pull mirroring is not available")
+          expect(response).to have_gitlab_http_status(200)
+          expect(project.reload.mirror).to be false
         end
 
         it 'updates mirror related attributes when user is admin' do
@@ -168,7 +168,7 @@ describe API::Projects do
         put(api("/projects/#{project.id}", user), mirror_params)
 
         expect(response).to have_gitlab_http_status(400)
-        expect(json_response["message"]).to eq("Invalid mirror user")
+        expect(json_response["message"]["mirror_user_id"].first).to eq("is invalid")
       end
 
       it 'returns 403 when the user does not have access to mirror settings' do

ee/spec/services/projects/create_service_spec.rb

@@ -85,8 +85,7 @@ describe Projects::CreateService, '#execute' do
   context 'with repository mirror' do
     before do
       opts.merge!(import_url: 'http://foo.com',
-                  mirror: true,
-                  mirror_user_id: user.id)
+                  mirror: true)
     end
 
     context 'when licensed' do
@@ -229,7 +228,6 @@ describe Projects::CreateService, '#execute' do
           visibility_level: Gitlab::VisibilityLevel::PRIVATE,
           namespace_id: user.namespace.id,
           mirror: true,
-          mirror_user_id: user.id,
           mirror_trigger_builds: true
         }
 

ee/spec/services/projects/update_service_spec.rb

@@ -10,58 +10,24 @@ describe Projects::UpdateService, '#execute' do
   context 'repository mirror' do
     let!(:opts) do
       {
+      }
+    end
+
+    it 'forces an import job' do
+      opts = {
         import_url: 'http://foo.com',
         mirror: true,
         mirror_user_id: user.id,
         mirror_trigger_builds: true
       }
-    end
 
-    context 'when licensed' do
-      before do
       stub_licensed_features(repository_mirrors: true)
-      end
-
-      it 'updates the correct attributes' do
-        update_project(project, user, opts)
-
-        updated_project = project.reload
-        expect(updated_project).to be_valid
-        expect(updated_project.mirror).to be true
-        expect(updated_project.mirror_user_id).to eq(user.id)
-        expect(updated_project.mirror_trigger_builds).to be true
-      end
-
-      it 'forces an import job' do
       expect(project).to receive(:force_import_job!).once
 
       update_project(project, user, opts)
     end
   end
 
-    context 'when unlicensed' do
-      before do
-        stub_licensed_features(repository_mirrors: false)
-      end
-
-      it 'does not update mirror attributes' do
-        update_project(project, user, opts)
-
-        updated_project = project.reload
-        expect(updated_project).to be_valid
-        expect(updated_project.mirror).to be false
-        expect(updated_project.mirror_user_id).to be_nil
-        expect(updated_project.mirror_trigger_builds).to be false
-      end
-
-      it 'does not force an import job' do
-        expect(project).not_to receive(:force_import_job!)
-
-        update_project(project, user, opts)
-      end
-    end
-  end
-
   context 'audit events' do
     let(:audit_event_params) do
       {

lib/api/projects.rb

@@ -14,6 +14,7 @@ module API
       end
 
       params :optional_update_params_ee do
+        # EE::API::Projects would override this helper
       end
 
       # EE::API::Projects would override this method
@@ -25,10 +26,7 @@ module API
         projects
       end
 
-      def verify_create_projects_attrs!(attrs)
-      end
-
-      def verify_update_project_attrs!(attrs)
+      def verify_update_project_attrs!(project, attrs)
       end
     end
 
@@ -177,9 +175,6 @@ module API
       post do
         attrs = declared_params(include_missing: false)
         attrs = translate_params_for_compatibility(attrs)
-
-        verify_create_projects_attrs!(attrs)
-
         project = ::Projects::CreateService.new(current_user, attrs).execute
 
         if project.saved?
@@ -212,9 +207,6 @@ module API
 
         attrs = declared_params(include_missing: false)
         attrs = translate_params_for_compatibility(attrs)
-
-        verify_create_projects_attrs!(attrs)
-
         project = ::Projects::CreateService.new(user, attrs).execute
 
         if project.saved?
@@ -314,7 +306,7 @@ module API
 
         attrs = translate_params_for_compatibility(attrs)
 
-        verify_update_project_attrs!(attrs)
+        verify_update_project_attrs!(user_project, attrs)
 
         result = ::Projects::UpdateService.new(user_project, current_user, attrs).execute