Merge branch '41567-projectfix' into 'security-10-3'

check project access on MR create

See merge request gitlab/gitlabhq!2273

app/services/merge_requests/create_service.rb

 module MergeRequests
   class CreateService < MergeRequests::BaseService
     def execute
-      # @project is used to determine whether the user can set the merge request's
-      # assignee, milestone and labels. Whether they can depends on their
-      # permissions on the target project.
-      source_project = @project
-      @project = Project.find(params[:target_project_id]) if params[:target_project_id]
+      set_projects!
 
       merge_request = MergeRequest.new
       merge_request.target_project = @project
-      merge_request.source_project = source_project
+      merge_request.source_project = @source_project
       merge_request.source_branch = params[:source_branch]
       merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch)
 
@@ -58,5 +54,25 @@ module MergeRequests
 
       pipelines.order(id: :desc).first
     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
+      # permissions on the target project.
+      @source_project = @project
+      @project = Project.find(params[:target_project_id]) if params[:target_project_id]
+
+      # make sure that source/target project ids are not in
+      # params so it can't be overridden later when updating attributes
+      # from params when applying quick actions
+      params.delete(:source_project_id)
+      params.delete(:target_project_id)
+
+      unless can?(current_user, :read_project, @source_project) &&
+          can?(current_user, :read_project, @project)
+
+        raise Gitlab::Access::AccessDeniedError
+      end
+    end
   end
 end

changelogs/unreleased/projectfix.yml

+---
+title: Check user authorization for source and target projects when creating a merge
+  request.
+merge_request:
+author:
+type: security

spec/features/cycle_analytics_spec.rb

@@ -113,6 +113,7 @@ feature 'Cycle Analytics', :js do
 
   context "as a guest" do
     before do
+      project.add_developer(user)
       project.add_guest(guest)
 
       allow_any_instance_of(Gitlab::ReferenceExtractor).to receive(:issues).and_return([issue])

spec/models/project_services/microsoft_teams_service_spec.rb

@@ -92,6 +92,10 @@ describe MicrosoftTeamsService do
         service.hook_data(merge_request, 'open')
       end
 
+      before do
+        project.add_developer(user)
+      end
+
       it "calls Microsoft Teams API" do
         chat_service.execute(merge_sample_data)
 

spec/requests/api/merge_requests_spec.rb

@@ -757,16 +757,28 @@ describe API::MergeRequests do
         expect(response).to have_gitlab_http_status(400)
       end
 
-      context 'when target_branch is specified' do
+      context 'when target_branch and target_project_id is specified' do
+        let(:params) do
+          { title: 'Test merge_request',
+            target_branch: 'master',
+            source_branch: 'markdown',
+            author: user2,
+            target_project_id: unrelated_project.id }
+        end
+
         it 'returns 422 if targeting a different fork' do
-          post api("/projects/#{forked_project.id}/merge_requests", user2),
-               title: 'Test merge_request',
-               target_branch: 'master',
-               source_branch: 'markdown',
-               author: user2,
-               target_project_id: unrelated_project.id
+          unrelated_project.add_developer(user2)
+
+          post api("/projects/#{forked_project.id}/merge_requests", user2), params
+
           expect(response).to have_gitlab_http_status(422)
         end
+
+        it 'returns 403 if targeting a different fork which user can not access' do
+          post api("/projects/#{forked_project.id}/merge_requests", user2), params
+
+          expect(response).to have_gitlab_http_status(403)
+        end
       end
 
       it "returns 201 when target_branch is specified and for the same project" do

spec/requests/api/v3/merge_requests_spec.rb

@@ -374,16 +374,28 @@ describe API::MergeRequests do
         expect(response).to have_gitlab_http_status(400)
       end
 
-      context 'when target_branch is specified' do
+      context 'when target_branch and target_project_id is specified' do
+        let(:params) do
+          { title: 'Test merge_request',
+            target_branch: 'master',
+            source_branch: 'markdown',
+            author: user2,
+            target_project_id: unrelated_project.id }
+        end
+
         it 'returns 422 if targeting a different fork' do
-          post v3_api("/projects/#{forked_project.id}/merge_requests", user2),
-               title: 'Test merge_request',
-               target_branch: 'master',
-               source_branch: 'markdown',
-               author: user2,
-               target_project_id: unrelated_project.id
+          unrelated_project.add_developer(user2)
+
+          post v3_api("/projects/#{forked_project.id}/merge_requests", user2), params
+
           expect(response).to have_gitlab_http_status(422)
         end
+
+        it 'returns 403 if targeting a different fork which user can not access' do
+          post v3_api("/projects/#{forked_project.id}/merge_requests", user2), params
+
+          expect(response).to have_gitlab_http_status(403)
+        end
       end
 
       it "returns 201 when target_branch is specified and for the same project" do

spec/services/merge_requests/create_service_spec.rb

@@ -263,5 +263,66 @@ describe MergeRequests::CreateService do
         expect(issue_ids).to match_array([first_issue.id, second_issue.id])
       end
     end
+
+    context 'when source and target projects are different' do
+      let(:target_project) { create(:project) }
+
+      let(:opts) do
+        {
+          title: 'Awesome merge_request',
+          source_branch: 'feature',
+          target_branch: 'master',
+          target_project_id: target_project.id
+        }
+      end
+
+      context 'when user can not access source project' do
+        before do
+          target_project.add_developer(assignee)
+          target_project.add_master(user)
+        end
+
+        it 'raises an error' do
+          expect { described_class.new(project, user, opts).execute }
+            .to raise_error Gitlab::Access::AccessDeniedError
+        end
+      end
+
+      context 'when user can not access target project' do
+        before do
+          target_project.add_developer(assignee)
+          target_project.add_master(user)
+        end
+
+        it 'raises an error' do
+          expect { described_class.new(project, user, opts).execute }
+            .to raise_error Gitlab::Access::AccessDeniedError
+        end
+      end
+    end
+
+    context 'when user sets source project id' do
+      let(:another_project) { create(:project) }
+
+      let(:opts) do
+        {
+          title: 'Awesome merge_request',
+          source_branch: 'feature',
+          target_branch: 'master',
+          source_project_id: another_project.id
+        }
+      end
+
+      before do
+        project.add_developer(assignee)
+        project.add_master(user)
+      end
+
+      it 'ignores source_project_id' do
+        merge_request = described_class.new(project, user, opts).execute
+
+        expect(merge_request.source_project_id).to eq(project.id)
+      end
+    end
   end
 end

spec/support/slack_mattermost_notifications_shared_examples.rb

@@ -57,6 +57,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do
       @issue = issue_service.execute
       @issues_sample_data = issue_service.hook_data(@issue, 'open')
 
+      project.add_developer(user)
       opts = {
         title: 'Awesome merge_request',
         description: 'please fix',