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

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

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

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

changelogs/unreleased/42682-merge-request-project-members-access.yml

+---
+title: Do not allow non-members to create MRs via forked projects when MRs are private
+merge_request:
+author:
+type: security

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

lib/api/merge_requests.rb

@@ -194,6 +194,7 @@ module API
         mr_params = declared_params(include_missing: false)
         mr_params[:force_remove_source_branch] = mr_params.delete(:remove_source_branch)
 
+        begin
           merge_request = ::MergeRequests::CreateService.new(user_project, current_user, mr_params).execute
 
           if merge_request.valid?
@@ -201,6 +202,11 @@ module API
           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
 
       desc 'Delete a merge request'

lib/api/v3/merge_requests.rb

@@ -98,6 +98,7 @@ 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?
 
+          begin
             merge_request = ::MergeRequests::CreateService.new(user_project, current_user, mr_params).execute
 
             if merge_request.valid?
@@ -105,6 +106,11 @@ module API
             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
 
         desc 'Delete a merge request'

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

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