Merge branch '42682-merge-request-internal-access' into 'security-10-6'

Do not allow non-members to create MRs via forked projects when MRs are private

See merge request gitlab/gitlabhq!2368

9 files changed, 97 insertions, 12 deletions

diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb
index a90030a8312..b08f0e7a790 100644
--- a/app/controllers/projects/merge_requests/creations_controller.rb
+++ b/app/controllers/projects/merge_requests/creations_controller.rb
@@ -29,6 +29,19 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
       define_new_vars
       render action: "new"
     end
+  rescue Gitlab::Access::AccessDeniedError
+    flash[:alert] = 'You do not have permission to create a Merge Request on the target project'
+
+    params = merge_request_params
+    params.delete(:force_remove_source_branch)
+    @merge_request = ::MergeRequests::BuildService.new(project, current_user, params).execute
+
+    @source_project = @merge_request.source_project
+    @target_project = @merge_request.target_project
+
+    define_new_vars
+
+    render action: 'new'
   end
 
   def pipelines
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index 57ab0c23dcd..894c3ea3a61 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -127,6 +127,8 @@ class ProjectPolicy < BasePolicy
   rule { guest & can?(:download_code) }.enable :build_download_code
   rule { guest & can?(:read_container_image) }.enable :build_read_container_image
 
+  rule { ~anonymous & ~merge_requests_disabled }.enable :create_forked_merge_request
+
   rule { can?(:reporter_access) }.policy do
     enable :download_code
     enable :download_wiki_code
diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb
index c57a2445341..ba3b2672ca3 100644
--- a/app/services/merge_requests/create_service.rb
+++ b/app/services/merge_requests/create_service.rb
@@ -58,6 +58,10 @@ module MergeRequests
       pipelines.order(id: :desc).first
     end
 
+    def forked?
+      @project != @source_project
+    end
+
     def set_projects!
       # @project is used to determine whether the user can set the merge request's
       # assignee, milestone and labels. Whether they can depends on their
@@ -71,8 +75,11 @@ module MergeRequests
       params.delete(:source_project_id)
       params.delete(:target_project_id)
 
+      create_ability = forked? ? :create_forked_merge_request : :create_merge_request
+
       unless can?(current_user, :read_project, @source_project) &&
-          can?(current_user, :read_project, @project)
+          can?(current_user, :read_project, @project) &&
+          can?(current_user, create_ability, @project)
 
         raise Gitlab::Access::AccessDeniedError
       end
diff --git a/changelogs/unreleased/42682-merge-request-project-members-access.yml b/changelogs/unreleased/42682-merge-request-project-members-access.yml
new file mode 100644
index 00000000000..ed7275c6345
--- /dev/null
+++ b/changelogs/unreleased/42682-merge-request-project-members-access.yml
@@ -0,0 +1,5 @@
+---
+title: Do not allow non-members to create MRs via forked projects when MRs are private
+merge_request:
+author:
+type: security
diff --git a/db/fixtures/development/10_merge_requests.rb b/db/fixtures/development/10_merge_requests.rb
index 30244ee4431..6886234ef92 100644
--- a/db/fixtures/development/10_merge_requests.rb
+++ b/db/fixtures/development/10_merge_requests.rb
@@ -21,7 +21,7 @@ Gitlab::Seeder.quiet do
         assignee: project.team.users.sample
       }
 
-      MergeRequests::CreateService.new(project, project.team.users.sample, params).execute
+      MergeRequests::CreateService.new(project, project.team.developers.sample, params).execute
       print '.'
     end
   end
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index 3264a26f7d2..3a570a56af9 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -194,12 +194,18 @@ module API
         mr_params = declared_params(include_missing: false)
         mr_params[:force_remove_source_branch] = mr_params.delete(:remove_source_branch)
 
-        merge_request = ::MergeRequests::CreateService.new(user_project, current_user, mr_params).execute
+        begin
+          merge_request = ::MergeRequests::CreateService.new(user_project, current_user, mr_params).execute
 
-        if merge_request.valid?
-          present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project
-        else
-          handle_merge_request_errors! merge_request.errors
+          if merge_request.valid?
+            present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project
+          else
+            handle_merge_request_errors! merge_request.errors
+          end
+        rescue Gitlab::Access::AccessDeniedError => ex
+          raise ex if Project.find(mr_params[:target_project_id]).merge_requests_enabled?
+
+          error!('Target project has disabled merge requests', 422)
         end
       end
 
diff --git a/lib/api/v3/merge_requests.rb b/lib/api/v3/merge_requests.rb
index ce216497996..c7a14de9135 100644
--- a/lib/api/v3/merge_requests.rb
+++ b/lib/api/v3/merge_requests.rb
@@ -98,12 +98,18 @@ module API
           mr_params = declared_params(include_missing: false)
           mr_params[:force_remove_source_branch] = mr_params.delete(:remove_source_branch) if mr_params[:remove_source_branch].present?
 
-          merge_request = ::MergeRequests::CreateService.new(user_project, current_user, mr_params).execute
+          begin
+            merge_request = ::MergeRequests::CreateService.new(user_project, current_user, mr_params).execute
 
-          if merge_request.valid?
-            present merge_request, with: ::API::V3::Entities::MergeRequest, current_user: current_user, project: user_project
-          else
-            handle_merge_request_errors! merge_request.errors
+            if merge_request.valid?
+              present merge_request, with: ::API::V3::Entities::MergeRequest, current_user: current_user, project: user_project
+            else
+              handle_merge_request_errors! merge_request.errors
+            end
+          rescue Gitlab::Access::AccessDeniedError => ex
+            raise ex if Project.find(mr_params[:target_project_id]).merge_requests_enabled?
+
+            error!('Target project has disabled merge requests', 422)
           end
         end
 
diff --git a/spec/controllers/projects/merge_requests/creations_controller_spec.rb b/spec/controllers/projects/merge_requests/creations_controller_spec.rb
index 24310b847e8..05d2b2f4b88 100644
--- a/spec/controllers/projects/merge_requests/creations_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests/creations_controller_spec.rb
@@ -31,6 +31,30 @@ describe Projects::MergeRequests::CreationsController do
     end
   end
 
+  describe 'POST create' do
+    context 'merge request to a project the user has no access to' do
+      let(:project) { create(:project, :internal, :merge_requests_private) }
+      let(:user) { create(:user) }
+      let(:opts) do
+        {
+          target_project_id: project.id,
+          source_project_id: fork_project.id,
+          source_branch: 'remove-submodule',
+          target_branch: 'master'
+        }
+      end
+
+      it 'shows a proper error message' do
+        expect do
+          post :create, get_diff_params.merge(merge_request: opts)
+        end.not_to change { MergeRequest.count }
+
+        expect(response).to be_success
+        expect(flash[:alert]).to eq('You do not have permission to create a Merge Request on the target project')
+      end
+    end
+  end
+
   describe 'GET diffs' do
     context 'when merge request cannot be created' do
       it 'does not assign diffs var' do
diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb
index 44a83c436cb..3f6e7734609 100644
--- a/spec/services/merge_requests/create_service_spec.rb
+++ b/spec/services/merge_requests/create_service_spec.rb
@@ -334,6 +334,28 @@ describe MergeRequests::CreateService do
             .to raise_error Gitlab::Access::AccessDeniedError
         end
       end
+
+      context 'when the user is not a member of target project and MR feature is restricted' do
+        let(:target_project) { create(:project, :internal, :merge_requests_private) }
+        let(:service) { described_class.new(project, user, opts) }
+
+        before do
+          project.add_master(user)
+          target_project.add_developer(assignee)
+          allow(service).to receive(:execute_hooks)
+        end
+
+        it 'raises an error' do
+          expect { service.execute }
+            .to raise_error(Gitlab::Access::AccessDeniedError)
+        end
+
+        it 'does not create the MR' do
+          expect do
+            service.execute rescue nil
+          end.not_to change { MergeRequest.count }
+        end
+      end
     end
 
     context 'when user sets source project id' do