Validate projects in MR build service

This validates the correct abilities for both projects. Only
`read_project` isn't enough:

For the `source_project` we validate `create_merge_request_from` this
also validates that the user has developer access to the project.

For the `target_project` we validate `create_merge_reqeust_in` this
also validates that the user has access to the project's repository.

To avoid generating diffs for unrelated projects we also validate that
the projects are in the same fork network now.

2 files changed, 89 insertions, 3 deletions

diff --git a/spec/features/merge_request/user_tries_to_access_private_repository_through_new_mr_spec.rb b/spec/features/merge_request/user_tries_to_access_private_repository_through_new_mr_spec.rb
new file mode 100644
index 00000000000..9318b5f1ebb
--- /dev/null
+++ b/spec/features/merge_request/user_tries_to_access_private_repository_through_new_mr_spec.rb
@@ -0,0 +1,37 @@
+require 'spec_helper'
+
+describe 'Merge Request > Tries to access private repo of public project' do
+  let(:current_user) { create(:user) }
+  let(:private_project) do
+    create(:project, :public, :repository,
+           path: 'nothing-to-see-here',
+           name: 'nothing to see here',
+           repository_access_level: ProjectFeature::PRIVATE)
+  end
+  let(:owned_project) do
+    create(:project, :public, :repository,
+           namespace: current_user.namespace,
+           creator: current_user)
+  end
+
+  context 'when the user enters the querystring info for the other project' do
+    let(:mr_path) do
+      project_new_merge_request_diffs_path(
+        owned_project,
+        merge_request: {
+          source_project_id: private_project.id,
+          source_branch: 'feature'
+        }
+      )
+    end
+
+    before do
+      sign_in current_user
+      visit mr_path
+    end
+
+    it "does not mention the project the user can't see the repo of" do
+      expect(page).not_to have_content('nothing-to-see-here')
+    end
+  end
+end
diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb
index 1894d8c8d0e..536d0d345a4 100644
--- a/spec/services/merge_requests/build_service_spec.rb
+++ b/spec/services/merge_requests/build_service_spec.rb
@@ -3,6 +3,7 @@ require 'spec_helper'
 describe MergeRequests::BuildService do
   using RSpec::Parameterized::TableSyntax
   include RepoHelpers
+  include ProjectForksHelper
 
   let(:project) { create(:project, :repository) }
   let(:source_project) { nil }
@@ -49,7 +50,7 @@ describe MergeRequests::BuildService do
 
   describe '#execute' do
     it 'calls the compare service with the correct arguments' do
-      allow_any_instance_of(described_class).to receive(:branches_valid?).and_return(true)
+      allow_any_instance_of(described_class).to receive(:projects_and_branches_valid?).and_return(true)
       expect(CompareService).to receive(:new)
                                   .with(project, Gitlab::Git::BRANCH_REF_PREFIX + source_branch)
                                   .and_call_original
@@ -393,11 +394,27 @@ describe MergeRequests::BuildService do
       end
     end
 
+    context 'target_project is set but repo is not accessible by current_user' do
+      let(:target_project) do
+        create(:project, :public, :repository, repository_access_level: ProjectFeature::PRIVATE)
+      end
+
+      it 'sets target project correctly' do
+        expect(merge_request.target_project).to eq(project)
+      end
+    end
+
     context 'source_project is set and accessible by current_user' do
       let(:source_project) { create(:project, :public, :repository)}
       let(:commits) { Commit.decorate([commit_1], project) }
 
-      it 'sets target project correctly' do
+      before do
+        # To create merge requests _from_ a project the user needs at least
+        # developer access
+        source_project.add_developer(user)
+      end
+
+      it 'sets source project correctly' do
         expect(merge_request.source_project).to eq(source_project)
       end
     end
@@ -406,11 +423,43 @@ describe MergeRequests::BuildService do
       let(:source_project) { create(:project, :private, :repository)}
       let(:commits) { Commit.decorate([commit_1], project) }
 
-      it 'sets target project correctly' do
+      it 'sets source project correctly' do
+        expect(merge_request.source_project).to eq(project)
+      end
+    end
+
+    context 'source_project is set but the user cannot create merge requests from the project' do
+      let(:source_project) do
+        create(:project, :public, :repository, merge_requests_access_level: ProjectFeature::PRIVATE)
+      end
+
+      it 'sets the source_project correctly' do
         expect(merge_request.source_project).to eq(project)
       end
     end
 
+    context 'target_project is not in the fork network of source_project' do
+      let(:target_project) { create(:project, :public, :repository) }
+
+      it 'adds an error to the merge request' do
+        expect(merge_request.errors[:validate_fork]).to contain_exactly('Source project is not a fork of the target project')
+      end
+    end
+
+    context 'target_project is in the fork network of source project but no longer accessible' do
+      let!(:project) { fork_project(target_project, user, namespace: user.namespace, repository: true) }
+      let(:source_project) { project }
+      let(:target_project) { create(:project, :public, :repository) }
+
+      before do
+        target_project.update(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+      end
+
+      it 'sets the target_project correctly' do
+        expect(merge_request.target_project).to eq(project)
+      end
+    end
+
     context 'when specifying target branch in the description' do
       let(:description) { "A merge request targeting another branch\n\n/target_branch with-codeowners" }