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

[10.1] backport - check project access on MR create

See merge request gitlab/gitlabhq!2280

(cherry picked from commit 6ca3de3c1e97590f62677227c7eef2f000db766c)

285551b9 check project access on MR create

8 files changed, 133 insertions, 20 deletions

diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb
index 820709583fa..2e98e0a78b7 100644
--- a/app/services/merge_requests/create_service.rb
+++ b/app/services/merge_requests/create_service.rb
@@ -1,15 +1,11 @@
 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)
 
@@ -52,5 +48,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
diff --git a/changelogs/unreleased/projectfix.yml b/changelogs/unreleased/projectfix.yml
new file mode 100644
index 00000000000..4d8ebe6194a
--- /dev/null
+++ b/changelogs/unreleased/projectfix.yml
@@ -0,0 +1,6 @@
+---
+title: Check user authorization for source and target projects when creating a merge
+  request.
+merge_request:
+author:
+type: security
diff --git a/spec/features/cycle_analytics_spec.rb b/spec/features/cycle_analytics_spec.rb
index 177cd50dd72..2dac3502209 100644
--- a/spec/features/cycle_analytics_spec.rb
+++ b/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])
diff --git a/spec/models/project_services/microsoft_teams_service_spec.rb b/spec/models/project_services/microsoft_teams_service_spec.rb
index f89be20ad78..3b260d66a2a 100644
--- a/spec/models/project_services/microsoft_teams_service_spec.rb
+++ b/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)
 
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index 5e66e1607ba..971e522076b 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/spec/requests/api/merge_requests_spec.rb
@@ -677,16 +677,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
diff --git a/spec/requests/api/v3/merge_requests_spec.rb b/spec/requests/api/v3/merge_requests_spec.rb
index df73c731c96..bb327604be4 100644
--- a/spec/requests/api/v3/merge_requests_spec.rb
+++ b/spec/requests/api/v3/merge_requests_spec.rb
@@ -373,16 +373,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
diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb
index a047f891ab2..8f35c8c42f6 100644
--- a/spec/services/merge_requests/create_service_spec.rb
+++ b/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
diff --git a/spec/support/slack_mattermost_notifications_shared_examples.rb b/spec/support/slack_mattermost_notifications_shared_examples.rb
index 6accf16bea4..9fcaf04fb3d 100644
--- a/spec/support/slack_mattermost_notifications_shared_examples.rb
+++ b/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',