From ad2bfeb85756db8c4cea9290be743665efd1c918 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Thu, 11 May 2017 16:23:02 +0100
Subject: [PATCH] Fix conflict resolution from corrupted upstream

I don't know why this happens exactly, but given an upstream and fork repository
from a customer, both of which required GC, resolving conflicts would corrupt
the fork so badly that it couldn't be cloned.

This isn't a perfect fix for that case, because the MR may still need to be
merged manually, but it does ensure that the repository is at least usable.

My best guess is that when we generate the index for the conflict
resolution (which we previously did in the target project), we obtain a
reference to an OID that doesn't exist in the source, even though we already
fetch the refs from the target into the source.

Explicitly setting the source project as the place to get the merge index from
seems to prevent repository corruption in this way.
---
 .../projects/merge_requests_controller.rb     | 18 +++--
 app/models/merge_request.rb                   | 28 -------
 app/presenters/merge_request_presenter.rb     |  6 +-
 .../merge_requests/conflicts/base_service.rb  | 11 +++
 .../merge_requests/conflicts/list_service.rb  | 35 +++++++++
 .../conflicts/resolve_service.rb              | 53 ++++++++++++++
 .../merge_requests/resolve_service.rb         | 65 -----------------
 ...conflict-resolution-with-corrupt-repos.yml |  5 ++
 lib/gitlab/conflict/file_collection.rb        | 42 +++++++++--
 .../merge_requests_controller_spec.rb         | 10 ++-
 .../gitlab/conflict/file_collection_spec.rb   |  2 +-
 spec/models/merge_request_spec.rb             | 65 -----------------
 .../merge_request_presenter_spec.rb           | 14 ++--
 .../conflicts/list_service_spec.rb            | 73 +++++++++++++++++++
 .../{ => conflicts}/resolve_service_spec.rb   | 41 +++++++----
 15 files changed, 268 insertions(+), 200 deletions(-)
 create mode 100644 app/services/merge_requests/conflicts/base_service.rb
 create mode 100644 app/services/merge_requests/conflicts/list_service.rb
 create mode 100644 app/services/merge_requests/conflicts/resolve_service.rb
 delete mode 100644 app/services/merge_requests/resolve_service.rb
 create mode 100644 changelogs/unreleased/fix-conflict-resolution-with-corrupt-repos.yml
 create mode 100644 spec/services/merge_requests/conflicts/list_service_spec.rb
 rename spec/services/merge_requests/{ => conflicts}/resolve_service_spec.rb (85%)

diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 207fbad7856..b99ccd453b8 100755
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -155,8 +155,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       format.html { define_discussion_vars }
 
       format.json do
-        if @merge_request.conflicts_can_be_resolved_in_ui?
-          render json: @merge_request.conflicts
+        if @conflicts_list.can_be_resolved_in_ui?
+          render json: @conflicts_list
         elsif @merge_request.can_be_merged?
           render json: {
             message: 'The merge conflicts for this merge request have already been resolved. Please return to the merge request.',
@@ -173,9 +173,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def conflict_for_path
-    return render_404 unless @merge_request.conflicts_can_be_resolved_in_ui?
+    return render_404 unless @conflicts_list.can_be_resolved_in_ui?
 
-    file = @merge_request.conflicts.file_for_path(params[:old_path], params[:new_path])
+    file = @conflicts_list.file_for_path(params[:old_path], params[:new_path])
 
     return render_404 unless file
 
@@ -183,7 +183,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def resolve_conflicts
-    return render_404 unless @merge_request.conflicts_can_be_resolved_in_ui?
+    return render_404 unless @conflicts_list.can_be_resolved_in_ui?
 
     if @merge_request.can_be_merged?
       render status: :bad_request, json: { message: 'The merge conflicts for this merge request have already been resolved.' }
@@ -191,7 +191,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     end
 
     begin
-      MergeRequests::ResolveService.new(@merge_request.source_project, current_user, params).execute(@merge_request)
+      MergeRequests::Conflicts::ResolveService.
+        new(merge_request).
+        execute(current_user, params)
 
       flash[:notice] = 'All merge conflicts were resolved. The merge request can now be merged.'
 
@@ -459,7 +461,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def authorize_can_resolve_conflicts!
-    return render_404 unless @merge_request.conflicts_can_be_resolved_by?(current_user)
+    @conflicts_list = MergeRequests::Conflicts::ListService.new(@merge_request)
+
+    return render_404 unless @conflicts_list.can_be_resolved_by?(current_user)
   end
 
   def module_enabled
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 59736f70f24..5f65636ba63 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -899,34 +899,6 @@ class MergeRequest < ActiveRecord::Base
     project.repository.keep_around(self.merge_commit_sha)
   end
 
-  def conflicts
-    @conflicts ||= Gitlab::Conflict::FileCollection.new(self)
-  end
-
-  def conflicts_can_be_resolved_by?(user)
-    return false unless source_project
-
-    access = ::Gitlab::UserAccess.new(user, project: source_project)
-    access.can_push_to_branch?(source_branch)
-  end
-
-  def conflicts_can_be_resolved_in_ui?
-    return @conflicts_can_be_resolved_in_ui if defined?(@conflicts_can_be_resolved_in_ui)
-
-    return @conflicts_can_be_resolved_in_ui = false unless cannot_be_merged?
-    return @conflicts_can_be_resolved_in_ui = false unless has_complete_diff_refs?
-
-    begin
-      # Try to parse each conflict. If the MR's mergeable status hasn't been updated,
-      # ensure that we don't say there are conflicts to resolve when there are no conflict
-      # files.
-      conflicts.files.each(&:lines)
-      @conflicts_can_be_resolved_in_ui = conflicts.files.length > 0
-    rescue Rugged::OdbError, Gitlab::Conflict::Parser::UnresolvableError, Gitlab::Conflict::FileCollection::ConflictSideMissing
-      @conflicts_can_be_resolved_in_ui = false
-    end
-  end
-
   def has_commits?
     merge_request_diff && commits_count > 0
   end
diff --git a/app/presenters/merge_request_presenter.rb b/app/presenters/merge_request_presenter.rb
index 255f63db5c2..0db9e31031c 100644
--- a/app/presenters/merge_request_presenter.rb
+++ b/app/presenters/merge_request_presenter.rb
@@ -76,7 +76,7 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
   end
 
   def conflict_resolution_path
-    if conflicts_can_be_resolved_in_ui? && conflicts_can_be_resolved_by?(current_user)
+    if conflicts.can_be_resolved_in_ui? && conflicts.can_be_resolved_by?(current_user)
       conflicts_namespace_project_merge_request_path(project.namespace, project, merge_request)
     end
   end
@@ -141,6 +141,10 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
 
   private
 
+  def conflicts
+    @conflicts ||= MergeRequests::Conflicts::ListService.new(merge_request)
+  end
+
   def closing_issues
     @closing_issues ||= closes_issues(current_user)
   end
diff --git a/app/services/merge_requests/conflicts/base_service.rb b/app/services/merge_requests/conflicts/base_service.rb
new file mode 100644
index 00000000000..b50875347d9
--- /dev/null
+++ b/app/services/merge_requests/conflicts/base_service.rb
@@ -0,0 +1,11 @@
+module MergeRequests
+  module Conflicts
+    class BaseService
+      attr_reader :merge_request
+
+      def initialize(merge_request)
+        @merge_request = merge_request
+      end
+    end
+  end
+end
diff --git a/app/services/merge_requests/conflicts/list_service.rb b/app/services/merge_requests/conflicts/list_service.rb
new file mode 100644
index 00000000000..9bf82518643
--- /dev/null
+++ b/app/services/merge_requests/conflicts/list_service.rb
@@ -0,0 +1,35 @@
+module MergeRequests
+  module Conflicts
+    class ListService < MergeRequests::Conflicts::BaseService
+      delegate :file_for_path, :to_json, to: :conflicts
+
+      def can_be_resolved_by?(user)
+        return false unless merge_request.source_project
+
+        access = ::Gitlab::UserAccess.new(user, project: merge_request.source_project)
+        access.can_push_to_branch?(merge_request.source_branch)
+      end
+
+      def can_be_resolved_in_ui?
+        return @conflicts_can_be_resolved_in_ui if defined?(@conflicts_can_be_resolved_in_ui)
+
+        return @conflicts_can_be_resolved_in_ui = false unless merge_request.cannot_be_merged?
+        return @conflicts_can_be_resolved_in_ui = false unless merge_request.has_complete_diff_refs?
+
+        begin
+          # Try to parse each conflict. If the MR's mergeable status hasn't been
+          # updated, ensure that we don't say there are conflicts to resolve
+          # when there are no conflict files.
+          conflicts.files.each(&:lines)
+          @conflicts_can_be_resolved_in_ui = conflicts.files.length > 0
+        rescue Rugged::OdbError, Gitlab::Conflict::Parser::UnresolvableError, Gitlab::Conflict::FileCollection::ConflictSideMissing
+          @conflicts_can_be_resolved_in_ui = false
+        end
+      end
+
+      def conflicts
+        @conflicts ||= Gitlab::Conflict::FileCollection.read_only(merge_request)
+      end
+    end
+  end
+end
diff --git a/app/services/merge_requests/conflicts/resolve_service.rb b/app/services/merge_requests/conflicts/resolve_service.rb
new file mode 100644
index 00000000000..d74a82effd6
--- /dev/null
+++ b/app/services/merge_requests/conflicts/resolve_service.rb
@@ -0,0 +1,53 @@
+module MergeRequests
+  module Conflicts
+    class ResolveService < MergeRequests::Conflicts::BaseService
+      MissingFiles = Class.new(Gitlab::Conflict::ResolutionError)
+
+      def execute(current_user, params)
+        rugged = merge_request.source_project.repository.rugged
+
+        Gitlab::Conflict::FileCollection.for_resolution(merge_request) do |conflicts_for_resolution|
+          merge_index = conflicts_for_resolution.merge_index
+
+          params[:files].each do |file_params|
+            conflict_file = conflicts_for_resolution.file_for_path(file_params[:old_path], file_params[:new_path])
+
+            write_resolved_file_to_index(merge_index, rugged, conflict_file, file_params)
+          end
+
+          unless merge_index.conflicts.empty?
+            missing_files = merge_index.conflicts.map { |file| file[:ours][:path] }
+
+            raise MissingFiles, "Missing resolutions for the following files: #{missing_files.join(', ')}"
+          end
+
+          commit_params = {
+            message: params[:commit_message] || conflicts_for_resolution.default_commit_message,
+            parents: [conflicts_for_resolution.our_commit, conflicts_for_resolution.their_commit].map(&:oid),
+            tree: merge_index.write_tree(rugged)
+          }
+
+          conflicts_for_resolution.
+            project.
+            repository.
+            resolve_conflicts(current_user, merge_request.source_branch, commit_params)
+        end
+      end
+
+      private
+
+      def write_resolved_file_to_index(merge_index, rugged, file, params)
+        new_file = if params[:sections]
+                     file.resolve_lines(params[:sections]).map(&:text).join("\n")
+                   elsif params[:content]
+                     file.resolve_content(params[:content])
+                   end
+
+        our_path = file.our_path
+
+        merge_index.add(path: our_path, oid: rugged.write(new_file, :blob), mode: file.our_mode)
+        merge_index.conflict_remove(our_path)
+      end
+    end
+  end
+end
diff --git a/app/services/merge_requests/resolve_service.rb b/app/services/merge_requests/resolve_service.rb
deleted file mode 100644
index 82cd89d9a0b..00000000000
--- a/app/services/merge_requests/resolve_service.rb
+++ /dev/null
@@ -1,65 +0,0 @@
-module MergeRequests
-  class ResolveService < MergeRequests::BaseService
-    MissingFiles = Class.new(Gitlab::Conflict::ResolutionError)
-
-    attr_accessor :conflicts, :rugged, :merge_index, :merge_request
-
-    def execute(merge_request)
-      @conflicts = merge_request.conflicts
-      @rugged = project.repository.rugged
-      @merge_index = conflicts.merge_index
-      @merge_request = merge_request
-
-      fetch_their_commit!
-
-      params[:files].each do |file_params|
-        conflict_file = merge_request.conflicts.file_for_path(file_params[:old_path], file_params[:new_path])
-
-        write_resolved_file_to_index(conflict_file, file_params)
-      end
-
-      unless merge_index.conflicts.empty?
-        missing_files = merge_index.conflicts.map { |file| file[:ours][:path] }
-
-        raise MissingFiles, "Missing resolutions for the following files: #{missing_files.join(', ')}"
-      end
-
-      commit_params = {
-        message: params[:commit_message] || conflicts.default_commit_message,
-        parents: [conflicts.our_commit, conflicts.their_commit].map(&:oid),
-        tree: merge_index.write_tree(rugged)
-      }
-
-      project.repository.resolve_conflicts(current_user, merge_request.source_branch, commit_params)
-    end
-
-    def write_resolved_file_to_index(file, params)
-      new_file = if params[:sections]
-                   file.resolve_lines(params[:sections]).map(&:text).join("\n")
-                 elsif params[:content]
-                   file.resolve_content(params[:content])
-                 end
-
-      our_path = file.our_path
-
-      merge_index.add(path: our_path, oid: rugged.write(new_file, :blob), mode: file.our_mode)
-      merge_index.conflict_remove(our_path)
-    end
-
-    # If their commit (in the target project) doesn't exist in the source project, it
-    # can't be a parent for the merge commit we're about to create. If that's the case,
-    # fetch the target branch ref into the source project so the commit exists in both.
-    #
-    def fetch_their_commit!
-      return if rugged.include?(conflicts.their_commit.oid)
-
-      random_string = SecureRandom.hex
-
-      project.repository.fetch_ref(
-        merge_request.target_project.repository.path_to_repo,
-        "refs/heads/#{merge_request.target_branch}",
-        "refs/tmp/#{random_string}/head"
-      )
-    end
-  end
-end
diff --git a/changelogs/unreleased/fix-conflict-resolution-with-corrupt-repos.yml b/changelogs/unreleased/fix-conflict-resolution-with-corrupt-repos.yml
new file mode 100644
index 00000000000..19a3c56e478
--- /dev/null
+++ b/changelogs/unreleased/fix-conflict-resolution-with-corrupt-repos.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent further repository corruption when resolving conflicts from a fork
+  where both the fork and upstream projects require housekeeping
+merge_request:
+author:
diff --git a/lib/gitlab/conflict/file_collection.rb b/lib/gitlab/conflict/file_collection.rb
index 990b719ecfd..6e73361cad1 100644
--- a/lib/gitlab/conflict/file_collection.rb
+++ b/lib/gitlab/conflict/file_collection.rb
@@ -3,16 +3,33 @@ module Gitlab
     class FileCollection
       ConflictSideMissing = Class.new(StandardError)
 
-      attr_reader :merge_request, :our_commit, :their_commit
+      attr_reader :merge_request, :our_commit, :their_commit, :project
 
-      def initialize(merge_request)
-        @merge_request = merge_request
-        @our_commit = merge_request.source_branch_head.raw.raw_commit
-        @their_commit = merge_request.target_branch_head.raw.raw_commit
-      end
+      delegate :repository, to: :project
 
-      def repository
-        merge_request.project.repository
+      class << self
+        # We can only write when getting the merge index from the source
+        # project, because we will write to that project. We don't use this all
+        # the time because this fetches a ref into the source project, which
+        # isn't needed for reading.
+        def for_resolution(merge_request)
+          project = merge_request.source_project
+
+          new(merge_request, project).tap do |file_collection|
+            project.
+              repository.
+              with_repo_branch_commit(merge_request.target_project.repository, merge_request.target_branch) do
+
+              yield file_collection
+            end
+          end
+        end
+
+        # We don't need to do `with_repo_branch_commit` here, because the target
+        # project always fetches source refs when creating merge request diffs.
+        def read_only(merge_request)
+          new(merge_request, merge_request.target_project)
+        end
       end
 
       def merge_index
@@ -55,6 +72,15 @@ Merge branch '#{merge_request.target_branch}' into '#{merge_request.source_branc
 #{conflict_filenames.join("\n")}
 EOM
       end
+
+      private
+
+      def initialize(merge_request, project)
+        @merge_request = merge_request
+        @our_commit = merge_request.source_branch_head.raw.raw_commit
+        @their_commit = merge_request.target_branch_head.raw.raw_commit
+        @project = project
+      end
     end
   end
 end
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index 37a253fde9b..0b3492a8fed 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -916,7 +916,9 @@ describe Projects::MergeRequestsController do
       end
 
       it 'returns the file in JSON format' do
-        content = merge_request_with_conflicts.conflicts.file_for_path(path, path).content
+        content = MergeRequests::Conflicts::ListService.new(merge_request_with_conflicts).
+                    file_for_path(path, path).
+                    content
 
         expect(json_response).to include('old_path' => path,
                                          'new_path' => path,
@@ -1040,11 +1042,15 @@ describe Projects::MergeRequestsController do
 
     context 'when a file has identical content to the conflict' do
       before do
+        content = MergeRequests::Conflicts::ListService.new(merge_request_with_conflicts).
+                    file_for_path('files/ruby/popen.rb', 'files/ruby/popen.rb').
+                    content
+
         resolved_files = [
           {
             'new_path' => 'files/ruby/popen.rb',
             'old_path' => 'files/ruby/popen.rb',
-            'content' => merge_request_with_conflicts.conflicts.file_for_path('files/ruby/popen.rb', 'files/ruby/popen.rb').content
+            'content' => content
           }, {
             'new_path' => 'files/ruby/regex.rb',
             'old_path' => 'files/ruby/regex.rb',
diff --git a/spec/lib/gitlab/conflict/file_collection_spec.rb b/spec/lib/gitlab/conflict/file_collection_spec.rb
index 39d892c18c0..27f23ea70dc 100644
--- a/spec/lib/gitlab/conflict/file_collection_spec.rb
+++ b/spec/lib/gitlab/conflict/file_collection_spec.rb
@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Gitlab::Conflict::FileCollection, lib: true do
   let(:merge_request) { create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start') }
-  let(:file_collection) { Gitlab::Conflict::FileCollection.new(merge_request) }
+  let(:file_collection) { described_class.read_only(merge_request) }
 
   describe '#files' do
     it 'returns an array of Conflict::Files' do
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index ef349530761..67f715ee25e 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1310,71 +1310,6 @@ describe MergeRequest, models: true do
     end
   end
 
-  describe '#conflicts_can_be_resolved_in_ui?' do
-    def create_merge_request(source_branch)
-      create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start') do |mr|
-        mr.mark_as_unmergeable
-      end
-    end
-
-    it 'returns a falsey value when the MR can be merged without conflicts' do
-      merge_request = create_merge_request('master')
-      merge_request.mark_as_mergeable
-
-      expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_falsey
-    end
-
-    it 'returns a falsey value when the MR is marked as having conflicts, but has none' do
-      merge_request = create_merge_request('master')
-
-      expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_falsey
-    end
-
-    it 'returns a falsey value when the MR has a missing ref after a force push' do
-      merge_request = create_merge_request('conflict-resolvable')
-      allow(merge_request.conflicts).to receive(:merge_index).and_raise(Rugged::OdbError)
-
-      expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_falsey
-    end
-
-    it 'returns a falsey value when the MR does not support new diff notes' do
-      merge_request = create_merge_request('conflict-resolvable')
-      merge_request.merge_request_diff.update_attributes(start_commit_sha: nil)
-
-      expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_falsey
-    end
-
-    it 'returns a falsey value when the conflicts contain a large file' do
-      merge_request = create_merge_request('conflict-too-large')
-
-      expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_falsey
-    end
-
-    it 'returns a falsey value when the conflicts contain a binary file' do
-      merge_request = create_merge_request('conflict-binary-file')
-
-      expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_falsey
-    end
-
-    it 'returns a falsey value when the conflicts contain a file edited in one branch and deleted in another' do
-      merge_request = create_merge_request('conflict-missing-side')
-
-      expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_falsey
-    end
-
-    it 'returns a truthy value when the conflicts are resolvable in the UI' do
-      merge_request = create_merge_request('conflict-resolvable')
-
-      expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_truthy
-    end
-
-    it 'returns a truthy value when the conflicts have to be resolved in an editor' do
-      merge_request = create_merge_request('conflict-contains-conflict-markers')
-
-      expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_truthy
-    end
-  end
-
   describe "#source_project_missing?" do
     let(:project)      { create(:empty_project) }
     let(:fork_project) { create(:empty_project, forked_from_project: project) }
diff --git a/spec/presenters/merge_request_presenter_spec.rb b/spec/presenters/merge_request_presenter_spec.rb
index e599ddaf943..44720fc4448 100644
--- a/spec/presenters/merge_request_presenter_spec.rb
+++ b/spec/presenters/merge_request_presenter_spec.rb
@@ -73,12 +73,12 @@ describe MergeRequestPresenter do
   describe '#conflict_resolution_path' do
     let(:project) { create :empty_project }
     let(:user) { create :user }
-    let(:path) { described_class.new(resource, current_user: user).conflict_resolution_path }
+    let(:presenter) { described_class.new(resource, current_user: user) }
+    let(:path) { presenter.conflict_resolution_path }
 
     context 'when MR cannot be resolved in UI' do
       it 'does not return conflict resolution path' do
-        allow(resource).to receive(:conflicts_can_be_resolved_in_ui?) { true }
-        allow(resource).to receive(:conflicts_can_be_resolved_by?).with(user) { false }
+        allow(presenter).to receive_message_chain(:conflicts, :can_be_resolved_in_ui?) { false }
 
         expect(path).to be_nil
       end
@@ -86,8 +86,8 @@ describe MergeRequestPresenter do
 
     context 'when conflicts cannot be resolved by user' do
       it 'does not return conflict resolution path' do
-        allow(resource).to receive(:conflicts_can_be_resolved_in_ui?) { false }
-        allow(resource).to receive(:conflicts_can_be_resolved_by?).with(user) { true }
+        allow(presenter).to receive_message_chain(:conflicts, :can_be_resolved_in_ui?) { true }
+        allow(presenter).to receive_message_chain(:conflicts, :can_be_resolved_by?).with(user) { false }
 
         expect(path).to be_nil
       end
@@ -95,8 +95,8 @@ describe MergeRequestPresenter do
 
     context 'when able to access conflict resolution UI' do
       it 'does return conflict resolution path' do
-        allow(resource).to receive(:conflicts_can_be_resolved_in_ui?) { true }
-        allow(resource).to receive(:conflicts_can_be_resolved_by?).with(user) { true }
+        allow(presenter).to receive_message_chain(:conflicts, :can_be_resolved_in_ui?) { true }
+        allow(presenter).to receive_message_chain(:conflicts, :can_be_resolved_by?).with(user) { true }
 
         expect(path)
           .to eq("/#{project.full_path}/merge_requests/#{resource.iid}/conflicts")
diff --git a/spec/services/merge_requests/conflicts/list_service_spec.rb b/spec/services/merge_requests/conflicts/list_service_spec.rb
new file mode 100644
index 00000000000..e8a305d6130
--- /dev/null
+++ b/spec/services/merge_requests/conflicts/list_service_spec.rb
@@ -0,0 +1,73 @@
+require 'spec_helper'
+
+describe MergeRequests::Conflicts::ListService do
+  describe '#can_be_resolved_in_ui?' do
+    def create_merge_request(source_branch)
+      create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start') do |mr|
+        mr.mark_as_unmergeable
+      end
+    end
+
+    def conflicts_service(merge_request)
+      described_class.new(merge_request)
+    end
+
+    it 'returns a falsey value when the MR can be merged without conflicts' do
+      merge_request = create_merge_request('master')
+      merge_request.mark_as_mergeable
+
+      expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
+    end
+
+    it 'returns a falsey value when the MR is marked as having conflicts, but has none' do
+      merge_request = create_merge_request('master')
+
+      expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
+    end
+
+    it 'returns a falsey value when the MR has a missing ref after a force push' do
+      merge_request = create_merge_request('conflict-resolvable')
+      service = conflicts_service(merge_request)
+      allow(service.conflicts).to receive(:merge_index).and_raise(Rugged::OdbError)
+
+      expect(service.can_be_resolved_in_ui?).to be_falsey
+    end
+
+    it 'returns a falsey value when the MR does not support new diff notes' do
+      merge_request = create_merge_request('conflict-resolvable')
+      merge_request.merge_request_diff.update_attributes(start_commit_sha: nil)
+
+      expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
+    end
+
+    it 'returns a falsey value when the conflicts contain a large file' do
+      merge_request = create_merge_request('conflict-too-large')
+
+      expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
+    end
+
+    it 'returns a falsey value when the conflicts contain a binary file' do
+      merge_request = create_merge_request('conflict-binary-file')
+
+      expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
+    end
+
+    it 'returns a falsey value when the conflicts contain a file edited in one branch and deleted in another' do
+      merge_request = create_merge_request('conflict-missing-side')
+
+      expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
+    end
+
+    it 'returns a truthy value when the conflicts are resolvable in the UI' do
+      merge_request = create_merge_request('conflict-resolvable')
+
+      expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_truthy
+    end
+
+    it 'returns a truthy value when the conflicts have to be resolved in an editor' do
+      merge_request = create_merge_request('conflict-contains-conflict-markers')
+
+      expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_truthy
+    end
+  end
+end
diff --git a/spec/services/merge_requests/resolve_service_spec.rb b/spec/services/merge_requests/conflicts/resolve_service_spec.rb
similarity index 85%
rename from spec/services/merge_requests/resolve_service_spec.rb
rename to spec/services/merge_requests/conflicts/resolve_service_spec.rb
index 3afd6b92900..19e8d5cc5f1 100644
--- a/spec/services/merge_requests/resolve_service_spec.rb
+++ b/spec/services/merge_requests/conflicts/resolve_service_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe MergeRequests::ResolveService do
+describe MergeRequests::Conflicts::ResolveService do
   let(:user) { create(:user) }
   let(:project) { create(:project, :repository) }
 
@@ -24,6 +24,8 @@ describe MergeRequests::ResolveService do
   end
 
   describe '#execute' do
+    let(:service) { described_class.new(merge_request) }
+
     context 'with section params' do
       let(:params) do
         {
@@ -50,7 +52,7 @@ describe MergeRequests::ResolveService do
 
       context 'when the source and target project are the same' do
         before do
-          described_class.new(project, user, params).execute(merge_request)
+          service.execute(user, params)
         end
 
         it 'creates a commit with the message' do
@@ -74,15 +76,26 @@ describe MergeRequests::ResolveService do
             branch_name: 'conflict-start')
         end
 
-        before do
-          described_class.new(fork_project, user, params).execute(merge_request_from_fork)
+        def resolve_conflicts
+          described_class.new(merge_request_from_fork).execute(user, params)
+        end
+
+        it 'gets conflicts from the source project' do
+          expect(fork_project.repository.rugged).to receive(:merge_commits).and_call_original
+          expect(project.repository.rugged).not_to receive(:merge_commits)
+
+          resolve_conflicts
         end
 
         it 'creates a commit with the message' do
+          resolve_conflicts
+
           expect(merge_request_from_fork.source_branch_head.message).to eq(params[:commit_message])
         end
 
         it 'creates a commit with the correct parents' do
+          resolve_conflicts
+
           expect(merge_request_from_fork.source_branch_head.parents.map(&:id)).
             to eq(['404fa3fc7c2c9b5dacff102f353bdf55b1be2813',
                    target_head])
@@ -115,7 +128,7 @@ describe MergeRequests::ResolveService do
       end
 
       before do
-        described_class.new(project, user, params).execute(merge_request)
+        service.execute(user, params)
       end
 
       it 'creates a commit with the message' do
@@ -154,15 +167,15 @@ describe MergeRequests::ResolveService do
         }
       end
 
-      let(:service) { described_class.new(project, user, invalid_params) }
-
       it 'raises a MissingResolution error' do
-        expect { service.execute(merge_request) }.
+        expect { service.execute(user, invalid_params) }.
           to raise_error(Gitlab::Conflict::File::MissingResolution)
       end
     end
 
     context 'when the content of a file is unchanged' do
+      let(:list_service) { MergeRequests::Conflicts::ListService.new(merge_request) }
+
       let(:invalid_params) do
         {
           files: [
@@ -173,17 +186,15 @@ describe MergeRequests::ResolveService do
             }, {
               old_path: 'files/ruby/regex.rb',
               new_path: 'files/ruby/regex.rb',
-              content: merge_request.conflicts.file_for_path('files/ruby/regex.rb', 'files/ruby/regex.rb').content
+              content: list_service.conflicts.file_for_path('files/ruby/regex.rb', 'files/ruby/regex.rb').content
             }
           ],
           commit_message: 'This is a commit message!'
         }
       end
 
-      let(:service) { described_class.new(project, user, invalid_params) }
-
       it 'raises a MissingResolution error' do
-        expect { service.execute(merge_request) }.
+        expect { service.execute(user, invalid_params) }.
           to raise_error(Gitlab::Conflict::File::MissingResolution)
       end
     end
@@ -202,11 +213,9 @@ describe MergeRequests::ResolveService do
         }
       end
 
-      let(:service) { described_class.new(project, user, invalid_params) }
-
       it 'raises a MissingFiles error' do
-        expect { service.execute(merge_request) }.
-          to raise_error(MergeRequests::ResolveService::MissingFiles)
+        expect { service.execute(user, invalid_params) }.
+          to raise_error(described_class::MissingFiles)
       end
     end
   end