From ade0c2c8922c0838ba85cf69419cbb109453d6b2 Mon Sep 17 00:00:00 2001 From: Frank West Date: Mon, 8 Aug 2016 03:29:23 +0000 Subject: 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 --- app/controllers/projects/blob_controller.rb | 14 ++++++++++++-- app/services/files/base_service.rb | 1 + app/services/files/update_service.rb | 23 +++++++++++++++++++++++ app/views/projects/blob/edit.html.haml | 9 +++++++-- 4 files changed, 43 insertions(+), 4 deletions(-) (limited to 'app') 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) -- cgit v1.2.1