From 9d029a05f1a6e35228bfb094ad2bd8369b7ee45f Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> Date: Tue, 11 Aug 2015 13:04:38 +0200 Subject: [PATCH] Revert "Merge branch 'web-editor-rugged' into 'master'" This reverts commit 5a1aa49b5533593dc4c6de82279fe44f5f15616c, reversing changes made to a675bea2c1c1d5d6923cb97b8714eb72d4e4ff9b. Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> --- app/services/files/delete_service.rb | 13 ++-- app/views/projects/blob/_editor.html.haml | 4 +- .../satellite/files/delete_file_action.rb | 50 ++++++++++++++ .../satellite/files/edit_file_action.rb | 68 +++++++++++++++++++ lib/gitlab/satellite/files/file_action.rb | 25 +++++++ lib/gitlab/satellite/files/new_file_action.rb | 67 ++++++++++++++++++ spec/requests/api/files_spec.rb | 52 ++++++++++++-- 7 files changed, 266 insertions(+), 13 deletions(-) create mode 100644 lib/gitlab/satellite/files/delete_file_action.rb create mode 100644 lib/gitlab/satellite/files/edit_file_action.rb create mode 100644 lib/gitlab/satellite/files/file_action.rb create mode 100644 lib/gitlab/satellite/files/new_file_action.rb diff --git a/app/services/files/delete_service.rb b/app/services/files/delete_service.rb index fabcdc196..1497a0f88 100644 --- a/app/services/files/delete_service.rb +++ b/app/services/files/delete_service.rb @@ -19,15 +19,14 @@ module Files return error("You can only edit text files") end - sha = repository.remove_file( - current_user, - path, - params[:commit_message], - ref + delete_file_action = Gitlab::Satellite::DeleteFileAction.new(current_user, project, ref, path) + + deleted_successfully = delete_file_action.commit!( + nil, + params[:commit_message] ) - if sha - after_commit(sha) + if deleted_successfully success else error("Your changes could not be committed, because the file has been changed") diff --git a/app/views/projects/blob/_editor.html.haml b/app/views/projects/blob/_editor.html.haml index 9c3e1703c..96f188e4a 100644 --- a/app/views/projects/blob/_editor.html.haml +++ b/app/views/projects/blob/_editor.html.haml @@ -12,8 +12,8 @@ \/ = text_field_tag 'file_name', params[:file_name], placeholder: "File name", required: true, class: 'form-control new-file-name' - .pull-right - = select_tag :encoding, options_for_select([ "base64", "text" ], "text"), class: 'form-control' + .pull-right + = select_tag :encoding, options_for_select([ "base64", "text" ], "text"), class: 'form-control' .file-content.code %pre.js-edit-mode-pane#editor diff --git a/lib/gitlab/satellite/files/delete_file_action.rb b/lib/gitlab/satellite/files/delete_file_action.rb new file mode 100644 index 000000000..0d37b9dea --- /dev/null +++ b/lib/gitlab/satellite/files/delete_file_action.rb @@ -0,0 +1,50 @@ +require_relative 'file_action' + +module Gitlab + module Satellite + class DeleteFileAction < FileAction + # Deletes file and creates a new commit for it + # + # Returns false if committing the change fails + # Returns false if pushing from the satellite to bare repo failed or was rejected + # Returns true otherwise + def commit!(content, commit_message) + in_locked_and_timed_satellite do |repo| + prepare_satellite!(repo) + + # create target branch in satellite at the corresponding commit from bare repo + repo.git.checkout({ raise: true, timeout: true, b: true }, ref, "origin/#{ref}") + + # update the file in the satellite's working dir + file_path_in_satellite = File.join(repo.working_dir, file_path) + + # Prevent relative links + unless safe_path?(file_path_in_satellite) + Gitlab::GitLogger.error("FileAction: Relative path not allowed") + return false + end + + File.delete(file_path_in_satellite) + + # add removed file + repo.remove(file_path_in_satellite) + + # commit the changes + # will raise CommandFailed when commit fails + repo.git.commit(raise: true, timeout: true, a: true, m: commit_message) + + + # push commit back to bare repo + # will raise CommandFailed when push fails + repo.git.push({ raise: true, timeout: true }, :origin, ref) + + # everything worked + true + end + rescue Grit::Git::CommandFailed => ex + Gitlab::GitLogger.error(ex.message) + false + end + end + end +end diff --git a/lib/gitlab/satellite/files/edit_file_action.rb b/lib/gitlab/satellite/files/edit_file_action.rb new file mode 100644 index 000000000..3cb9c0b5e --- /dev/null +++ b/lib/gitlab/satellite/files/edit_file_action.rb @@ -0,0 +1,68 @@ +require_relative 'file_action' + +module Gitlab + module Satellite + # GitLab server-side file update and commit + class EditFileAction < FileAction + # Updates the files content and creates a new commit for it + # + # Returns false if the ref has been updated while editing the file + # Returns false if committing the change fails + # Returns false if pushing from the satellite to bare repo failed or was rejected + # Returns true otherwise + def commit!(content, commit_message, encoding, new_branch = nil) + in_locked_and_timed_satellite do |repo| + prepare_satellite!(repo) + + # create target branch in satellite at the corresponding commit from bare repo + begin + repo.git.checkout({ raise: true, timeout: true, b: true }, ref, "origin/#{ref}") + rescue Grit::Git::CommandFailed => ex + log_and_raise(CheckoutFailed, ex.message) + end + + # update the file in the satellite's working dir + file_path_in_satellite = File.join(repo.working_dir, file_path) + + # Prevent relative links + unless safe_path?(file_path_in_satellite) + Gitlab::GitLogger.error("FileAction: Relative path not allowed") + return false + end + + # Write file + write_file(file_path_in_satellite, content, encoding) + + # commit the changes + # will raise CommandFailed when commit fails + begin + repo.git.commit(raise: true, timeout: true, a: true, m: commit_message) + rescue Grit::Git::CommandFailed => ex + log_and_raise(CommitFailed, ex.message) + end + + + target_branch = new_branch.present? ? "#{ref}:#{new_branch}" : ref + + # push commit back to bare repo + # will raise CommandFailed when push fails + begin + repo.git.push({ raise: true, timeout: true }, :origin, target_branch) + rescue Grit::Git::CommandFailed => ex + log_and_raise(PushFailed, ex.message) + end + + # everything worked + true + end + end + + private + + def log_and_raise(errorClass, message) + Gitlab::GitLogger.error(message) + raise(errorClass, message) + end + end + end +end diff --git a/lib/gitlab/satellite/files/file_action.rb b/lib/gitlab/satellite/files/file_action.rb new file mode 100644 index 000000000..6446b1456 --- /dev/null +++ b/lib/gitlab/satellite/files/file_action.rb @@ -0,0 +1,25 @@ +module Gitlab + module Satellite + class FileAction < Action + attr_accessor :file_path, :ref + + def initialize(user, project, ref, file_path) + super user, project + @file_path = file_path + @ref = ref + end + + def safe_path?(path) + File.absolute_path(path) == path + end + + def write_file(abs_file_path, content, file_encoding = 'text') + if file_encoding == 'base64' + File.open(abs_file_path, 'wb') { |f| f.write(Base64.decode64(content)) } + else + File.open(abs_file_path, 'w') { |f| f.write(content) } + end + end + end + end +end diff --git a/lib/gitlab/satellite/files/new_file_action.rb b/lib/gitlab/satellite/files/new_file_action.rb new file mode 100644 index 000000000..724dfa0d0 --- /dev/null +++ b/lib/gitlab/satellite/files/new_file_action.rb @@ -0,0 +1,67 @@ +require_relative 'file_action' + +module Gitlab + module Satellite + class NewFileAction < FileAction + # Updates the files content and creates a new commit for it + # + # Returns false if the ref has been updated while editing the file + # Returns false if committing the change fails + # Returns false if pushing from the satellite to bare repo failed or was rejected + # Returns true otherwise + def commit!(content, commit_message, encoding, new_branch = nil) + in_locked_and_timed_satellite do |repo| + prepare_satellite!(repo) + + # create target branch in satellite at the corresponding commit from bare repo + current_ref = + if @project.empty_repo? + # skip this step if we want to add first file to empty repo + Satellite::PARKING_BRANCH + else + repo.git.checkout({ raise: true, timeout: true, b: true }, ref, "origin/#{ref}") + ref + end + + file_path_in_satellite = File.join(repo.working_dir, file_path) + dir_name_in_satellite = File.dirname(file_path_in_satellite) + + # Prevent relative links + unless safe_path?(file_path_in_satellite) + Gitlab::GitLogger.error("FileAction: Relative path not allowed") + return false + end + + # Create dir if not exists + FileUtils.mkdir_p(dir_name_in_satellite) + + # Write file + write_file(file_path_in_satellite, content, encoding) + + # add new file + repo.add(file_path_in_satellite) + + # commit the changes + # will raise CommandFailed when commit fails + repo.git.commit(raise: true, timeout: true, a: true, m: commit_message) + + target_branch = if new_branch.present? && !@project.empty_repo? + "#{ref}:#{new_branch}" + else + "#{current_ref}:#{ref}" + end + + # push commit back to bare repo + # will raise CommandFailed when push fails + repo.git.push({ raise: true, timeout: true }, :origin, target_branch) + + # everything worked + true + end + rescue Grit::Git::CommandFailed => ex + Gitlab::GitLogger.error(ex.message) + false + end + end + end +end diff --git a/spec/requests/api/files_spec.rb b/spec/requests/api/files_spec.rb index 6c7860511..346f1e29d 100644 --- a/spec/requests/api/files_spec.rb +++ b/spec/requests/api/files_spec.rb @@ -49,6 +49,10 @@ describe API::API, api: true do end it "should create a new file in project repo" do + Gitlab::Satellite::NewFileAction.any_instance.stub( + commit!: true, + ) + post api("/projects/#{project.id}/repository/files", user), valid_params expect(response.status).to eq(201) expect(json_response['file_path']).to eq('newfile.rb') @@ -59,9 +63,10 @@ describe API::API, api: true do expect(response.status).to eq(400) end - it "should return a 400 if editor fails to create file" do - allow_any_instance_of(Repository).to receive(:commit_file). - and_return(false) + it "should return a 400 if satellite fails to create file" do + Gitlab::Satellite::NewFileAction.any_instance.stub( + commit!: false, + ) post api("/projects/#{project.id}/repository/files", user), valid_params expect(response.status).to eq(400) @@ -79,6 +84,10 @@ describe API::API, api: true do end it "should update existing file in project repo" do + Gitlab::Satellite::EditFileAction.any_instance.stub( + commit!: true, + ) + put api("/projects/#{project.id}/repository/files", user), valid_params expect(response.status).to eq(200) expect(json_response['file_path']).to eq(file_path) @@ -88,6 +97,35 @@ describe API::API, api: true do put api("/projects/#{project.id}/repository/files", user) expect(response.status).to eq(400) end + + it 'should return a 400 if the checkout fails' do + Gitlab::Satellite::EditFileAction.any_instance.stub(:commit!) + .and_raise(Gitlab::Satellite::CheckoutFailed) + + put api("/projects/#{project.id}/repository/files", user), valid_params + expect(response.status).to eq(400) + + ref = valid_params[:branch_name] + expect(response.body).to match("ref '#{ref}' could not be checked out") + end + + it 'should return a 409 if the file was not modified' do + Gitlab::Satellite::EditFileAction.any_instance.stub(:commit!) + .and_raise(Gitlab::Satellite::CommitFailed) + + put api("/projects/#{project.id}/repository/files", user), valid_params + expect(response.status).to eq(409) + expect(response.body).to match("Maybe there was nothing to commit?") + end + + it 'should return a 409 if the push fails' do + Gitlab::Satellite::EditFileAction.any_instance.stub(:commit!) + .and_raise(Gitlab::Satellite::PushFailed) + + put api("/projects/#{project.id}/repository/files", user), valid_params + expect(response.status).to eq(409) + expect(response.body).to match("Maybe the file was changed by another process?") + end end describe "DELETE /projects/:id/repository/files" do @@ -100,6 +138,10 @@ describe API::API, api: true do end it "should delete existing file in project repo" do + Gitlab::Satellite::DeleteFileAction.any_instance.stub( + commit!: true, + ) + delete api("/projects/#{project.id}/repository/files", user), valid_params expect(response.status).to eq(200) expect(json_response['file_path']).to eq(file_path) @@ -111,7 +153,9 @@ describe API::API, api: true do end it "should return a 400 if satellite fails to create file" do - allow_any_instance_of(Repository).to receive(:remove_file).and_return(false) + Gitlab::Satellite::DeleteFileAction.any_instance.stub( + commit!: false, + ) delete api("/projects/#{project.id}/repository/files", user), valid_params expect(response.status).to eq(400) -- 2.30.9