Prevents accidental overwrites of commits from UI

Currently when a user performs an update of a file through the UI  and there
has already been a change committed to the file the previous commits will be
overwritten without a check to see if the file has been changed.

This commit uses the last commit sha at the time the user starts editing the
file and compares it with the current sha of the file being edited to ensure
they are the same before committing the file. If the shas do not match we
throw an exception preventing the commit from the commit from occurring.

Fixes #5857

7 files changed, 162 insertions, 4 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 6e096b480c0..d424d6aebc6 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -108,6 +108,7 @@ v 8.11.0 (unreleased)
   - Sort folders with submodules in Files view !5521
   - Each `File::exists?` replaced to `File::exist?` because of deprecate since ruby version 2.2.0
   - Add auto-completition in pipeline (Katarzyna Kobierska Ula Budziszewska)
+  - Ensure file editing in UI does not overwrite commited changes without warning user
 
 v 8.10.5
   - Add a data migration to fix some missing timestamps in the members table. !5670
diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb
index 19d051720e9..cdf9a04bacf 100644
--- a/app/controllers/projects/blob_controller.rb
+++ b/app/controllers/projects/blob_controller.rb
@@ -17,6 +17,7 @@ class Projects::BlobController < Projects::ApplicationController
   before_action :require_branch_head, only: [:edit, :update]
   before_action :editor_variables, except: [:show, :preview, :diff]
   before_action :validate_diff_params, only: :diff
+  before_action :set_last_commit_sha, only: [:edit, :update]
 
   def new
     commit unless @repository.empty?
@@ -33,7 +34,6 @@ class Projects::BlobController < Projects::ApplicationController
   end
 
   def edit
-    @last_commit = Gitlab::Git::Commit.last_for_path(@repository, @ref, @path).sha
     blob.load_all_data!(@repository)
   end
 
@@ -55,6 +55,10 @@ class Projects::BlobController < Projects::ApplicationController
     create_commit(Files::UpdateService, success_path: after_edit_path,
                                         failure_view: :edit,
                                         failure_path: namespace_project_blob_path(@project.namespace, @project, @id))
+
+  rescue Files::UpdateService::FileChangedError
+    @conflict = true
+    render :edit
   end
 
   def preview
@@ -152,7 +156,8 @@ class Projects::BlobController < Projects::ApplicationController
       file_path: @file_path,
       commit_message: params[:commit_message],
       file_content: params[:content],
-      file_content_encoding: params[:encoding]
+      file_content_encoding: params[:encoding],
+      last_commit_sha: params[:last_commit_sha]
     }
   end
 
@@ -161,4 +166,9 @@ class Projects::BlobController < Projects::ApplicationController
       render nothing: true
     end
   end
+
+  def set_last_commit_sha
+    @last_commit_sha = Gitlab::Git::Commit.
+      last_for_path(@repository, @ref, @path).sha
+  end
 end
diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb
index c4a206f785e..ea94818713b 100644
--- a/app/services/files/base_service.rb
+++ b/app/services/files/base_service.rb
@@ -15,6 +15,7 @@ module Files
                         else
                           params[:file_content]
                         end
+      @last_commit_sha = params[:last_commit_sha]
 
       # Validate parameters
       validate
diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb
index 8d2b5083179..4fc3b640799 100644
--- a/app/services/files/update_service.rb
+++ b/app/services/files/update_service.rb
@@ -2,11 +2,34 @@ require_relative "base_service"
 
 module Files
   class UpdateService < Files::BaseService
+    class FileChangedError < StandardError; end
+
     def commit
       repository.update_file(current_user, @file_path, @file_content,
                              branch: @target_branch,
                              previous_path: @previous_path,
                              message: @commit_message)
     end
+
+    private
+
+    def validate
+      super
+
+      if file_has_changed?
+        raise FileChangedError.new("You are attempting to update a file that has changed since you started editing it.")
+      end
+    end
+
+    def file_has_changed?
+      return false unless @last_commit_sha && last_commit
+
+      @last_commit_sha != last_commit.sha
+    end
+
+    def last_commit
+      @last_commit ||= Gitlab::Git::Commit.
+        last_for_path(@source_project.repository, @source_branch, @file_path)
+    end
   end
 end
diff --git a/app/views/projects/blob/edit.html.haml b/app/views/projects/blob/edit.html.haml
index b1c9895f43e..7b0621f9401 100644
--- a/app/views/projects/blob/edit.html.haml
+++ b/app/views/projects/blob/edit.html.haml
@@ -1,5 +1,11 @@
 - page_title "Edit", @blob.path, @ref
 
+- if @conflict
+  .alert.alert-danger
+    Someone edited the file the same time you did. Please check out
+    = link_to "the file", namespace_project_blob_path(@project.namespace, @project, tree_join(@target_branch, @file_path)), target: "_blank"
+    and make sure your changes will not unintentionally remove theirs.
+
 .file-editor
   %ul.nav-links.no-bottom.js-edit-mode
     %li.active
@@ -13,8 +19,7 @@
   = form_tag(namespace_project_update_blob_path(@project.namespace, @project, @id), method: :put, class: 'form-horizontal js-quick-submit js-requires-input js-edit-blob-form') do
     = render 'projects/blob/editor', ref: @ref, path: @path, blob_data: @blob.data
     = render 'shared/new_commit_form', placeholder: "Update #{@blob.name}"
-
-    = hidden_field_tag 'last_commit', @last_commit
+    = hidden_field_tag 'last_commit_sha', @last_commit_sha
     = hidden_field_tag 'content', '', id: "file-content"
     = hidden_field_tag 'from_merge_request_id', params[:from_merge_request_id]
     = render 'projects/commit_button', ref: @ref, cancel_path: namespace_project_blob_path(@project.namespace, @project, @id)
diff --git a/spec/features/projects/files/editing_a_file_spec.rb b/spec/features/projects/files/editing_a_file_spec.rb
new file mode 100644
index 00000000000..fe047e00409
--- /dev/null
+++ b/spec/features/projects/files/editing_a_file_spec.rb
@@ -0,0 +1,34 @@
+require 'spec_helper'
+
+feature 'User wants to edit a file', feature: true do
+  include WaitForAjax
+
+  let(:project) { create(:project) }
+  let(:user) { create(:user) }
+  let(:commit_params) do
+    {
+      source_branch: project.default_branch,
+      target_branch: project.default_branch,
+      commit_message: "Committing First Update",
+      file_path: ".gitignore",
+      file_content: "First Update",
+      last_commit_sha: Gitlab::Git::Commit.last_for_path(project.repository, project.default_branch,
+                                                         ".gitignore").sha
+    }
+  end
+
+  background do
+    project.team << [user, :master]
+    login_as user
+    visit namespace_project_edit_blob_path(project.namespace, project,
+                                           File.join(project.default_branch, '.gitignore'))
+  end
+
+  scenario 'file has been updated since the user opened the edit page' do
+    Files::UpdateService.new(project, user, commit_params).execute
+
+    click_button 'Commit Changes'
+
+    expect(page).to have_content 'Someone edited the file the same time you did.'
+  end
+end
diff --git a/spec/services/files/update_service_spec.rb b/spec/services/files/update_service_spec.rb
new file mode 100644
index 00000000000..d019e50649f
--- /dev/null
+++ b/spec/services/files/update_service_spec.rb
@@ -0,0 +1,84 @@
+require "spec_helper"
+
+describe Files::UpdateService do
+  subject { described_class.new(project, user, commit_params) }
+
+  let(:project) { create(:project) }
+  let(:user) { create(:user) }
+  let(:file_path) { 'files/ruby/popen.rb' }
+  let(:new_contents) { "New Content" }
+  let(:commit_params) do
+    {
+      file_path: file_path,
+      commit_message: "Update File",
+      file_content: new_contents,
+      file_content_encoding: "text",
+      last_commit_sha: last_commit_sha,
+      source_project: project,
+      source_branch: project.default_branch,
+      target_branch: project.default_branch,
+    }
+  end
+
+  before do
+    project.team << [user, :master]
+  end
+
+  describe "#execute" do
+    context "when the file's last commit sha does not match the supplied last_commit_sha" do
+      let(:last_commit_sha) { "foo" }
+
+      it "returns a hash with the correct error message and a :error status " do
+        expect { subject.execute }.
+          to raise_error(Files::UpdateService::FileChangedError,
+                         "You are attempting to update a file that has changed since you started editing it.")
+      end
+    end
+
+    context "when the file's last commit sha does match the supplied last_commit_sha" do
+      let(:last_commit_sha) { Gitlab::Git::Commit.last_for_path(project.repository, project.default_branch, file_path).sha }
+
+      it "returns a hash with the :success status " do
+        results = subject.execute
+
+        expect(results).to match({ status: :success })
+      end
+
+      it "updates the file with the new contents" do
+        subject.execute
+
+        results = project.repository.blob_at_branch(project.default_branch, file_path)
+
+        expect(results.data).to eq(new_contents)
+      end
+    end
+
+    context "when the last_commit_sha is not supplied" do
+      let(:commit_params) do
+        {
+          file_path: file_path,
+          commit_message: "Update File",
+          file_content: new_contents,
+          file_content_encoding: "text",
+          source_project: project,
+          source_branch: project.default_branch,
+          target_branch: project.default_branch,
+        }
+      end
+
+      it "returns a hash with the :success status " do
+        results = subject.execute
+
+        expect(results).to match({ status: :success })
+      end
+
+      it "updates the file with the new contents" do
+        subject.execute
+
+        results = project.repository.blob_at_branch(project.default_branch, file_path)
+
+        expect(results.data).to eq(new_contents)
+      end
+    end
+  end
+end