Merge branch '1827-prevent-concurrent-editing-wiki' into 'master'

Prevent concurrent editing wiki

Closes #1827

See merge request !9707

9 files changed, 81 insertions, 9 deletions

diff --git a/app/controllers/projects/wikis_controller.rb b/app/controllers/projects/wikis_controller.rb
index ac98470c2b1..968d880886c 100644
--- a/app/controllers/projects/wikis_controller.rb
+++ b/app/controllers/projects/wikis_controller.rb
@@ -55,6 +55,9 @@ class Projects::WikisController < Projects::ApplicationController
     else
       render 'edit'
     end
+  rescue WikiPage::PageChangedError
+    @conflict = true
+    render 'edit'
   end
 
   def create
@@ -119,6 +122,6 @@ class Projects::WikisController < Projects::ApplicationController
   end
 
   def wiki_params
-    params.require(:wiki).permit(:title, :content, :format, :message)
+    params.require(:wiki).permit(:title, :content, :format, :message, :last_commit_sha)
   end
 end
diff --git a/app/models/wiki_page.rb b/app/models/wiki_page.rb
index 224eb3cd4d0..148998bc9be 100644
--- a/app/models/wiki_page.rb
+++ b/app/models/wiki_page.rb
@@ -1,4 +1,6 @@
 class WikiPage
+  PageChangedError = Class.new(StandardError)
+
   include ActiveModel::Validations
   include ActiveModel::Conversion
   include StaticModel
@@ -136,6 +138,10 @@ class WikiPage
     versions.first
   end
 
+  def last_commit_sha
+    commit&.sha
+  end
+
   # Returns the Date that this latest version was
   # created on.
   def created_at
@@ -182,17 +188,22 @@ class WikiPage
 
   # Updates an existing Wiki Page, creating a new version.
   #
-  # new_content - The raw markup content to replace the existing.
-  # format      - Optional symbol representing the content format.
-  #               See ProjectWiki::MARKUPS Hash for available formats.
-  # message     - Optional commit message to set on the new version.
+  # new_content     - The raw markup content to replace the existing.
+  # format          - Optional symbol representing the content format.
+  #                   See ProjectWiki::MARKUPS Hash for available formats.
+  # message         - Optional commit message to set on the new version.
+  # last_commit_sha - Optional last commit sha to validate the page unchanged.
   #
   # Returns the String SHA1 of the newly created page
   # or False if the save was unsuccessful.
-  def update(new_content = "", format = :markdown, message = nil)
+  def update(new_content, format: :markdown, message: nil, last_commit_sha: nil)
     @attributes[:content] = new_content
     @attributes[:format] = format
 
+    if last_commit_sha && last_commit_sha != self.last_commit_sha
+      raise PageChangedError.new("You are attempting to update a page that has changed since you started editing it.")
+    end
+
     save :update_page, @page, content, format, message
   end
 
diff --git a/app/services/wiki_pages/update_service.rb b/app/services/wiki_pages/update_service.rb
index 8f6a50da838..c628e6781af 100644
--- a/app/services/wiki_pages/update_service.rb
+++ b/app/services/wiki_pages/update_service.rb
@@ -1,7 +1,7 @@
 module WikiPages
   class UpdateService < WikiPages::BaseService
     def execute(page)
-      if page.update(@params[:content], @params[:format], @params[:message])
+      if page.update(@params[:content], format: @params[:format], message: @params[:message], last_commit_sha: @params[:last_commit_sha])
         execute_hooks(page, 'update')
       end
 
diff --git a/app/views/projects/wikis/_form.html.haml b/app/views/projects/wikis/_form.html.haml
index fc6b7a33943..adb8d5aaecb 100644
--- a/app/views/projects/wikis/_form.html.haml
+++ b/app/views/projects/wikis/_form.html.haml
@@ -4,6 +4,8 @@
   = form_errors(@page)
 
   = f.hidden_field :title, value: @page.title
+  - if @page.persisted?
+    = f.hidden_field :last_commit_sha, value: @page.last_commit_sha
   .form-group
     .col-sm-12= f.label :format, class: 'control-label-full-width'
     .col-sm-12
diff --git a/app/views/projects/wikis/edit.html.haml b/app/views/projects/wikis/edit.html.haml
index df0ec14eb3b..8fd60216536 100644
--- a/app/views/projects/wikis/edit.html.haml
+++ b/app/views/projects/wikis/edit.html.haml
@@ -1,6 +1,12 @@
 - @content_class = "limit-container-width limit-container-width-sm" unless fluid_layout
 - page_title "Edit", @page.title.capitalize, "Wiki"
 
+- if @conflict
+  .alert.alert-danger
+    Someone edited the page the same time you did. Please check out
+    = link_to "the page", project_wiki_path(@project, @page), target: "_blank"
+    and make sure your changes will not unintentionally remove theirs.
+
 .wiki-page-header.has-sidebar-toggle
   %button.btn.btn-default.sidebar-toggle.js-sidebar-wiki-toggle{ role: "button", type: "button" }
     = icon('angle-double-left')
diff --git a/changelogs/unreleased/1827-prevent-concurrent-editing-wiki.yml b/changelogs/unreleased/1827-prevent-concurrent-editing-wiki.yml
new file mode 100644
index 00000000000..c8c2bb3eb4c
--- /dev/null
+++ b/changelogs/unreleased/1827-prevent-concurrent-editing-wiki.yml
@@ -0,0 +1,4 @@
+---
+title: Alert the user if a Wiki page changed while they were editing it in order to prevent overwriting changes.
+merge_request: 9707
+author: Hiroyuki Sato
diff --git a/features/steps/project/wiki.rb b/features/steps/project/wiki.rb
index 9d38939378d..6a478c50e5e 100644
--- a/features/steps/project/wiki.rb
+++ b/features/steps/project/wiki.rb
@@ -63,7 +63,7 @@ class Spinach::Features::ProjectWiki < Spinach::FeatureSteps
   end
 
   step 'That page has two revisions' do
-    @page.update("new content", :markdown, "second commit")
+    @page.update("new content", message: "second commit")
   end
 
   step 'I click the History button' do
diff --git a/spec/features/projects/wiki/user_updates_wiki_page_spec.rb b/spec/features/projects/wiki/user_updates_wiki_page_spec.rb
index 6ebf59cba98..e3739a705bf 100644
--- a/spec/features/projects/wiki/user_updates_wiki_page_spec.rb
+++ b/spec/features/projects/wiki/user_updates_wiki_page_spec.rb
@@ -2,10 +2,10 @@ require 'spec_helper'
 
 feature 'Projects > Wiki > User updates wiki page' do
   let(:user) { create(:user) }
+  let!(:wiki_page) { WikiPages::CreateService.new(project, user, title: 'home', content: 'Home page').execute }
 
   background do
     project.team << [user, :master]
-    WikiPages::CreateService.new(project, user, title: 'home', content: 'Home page').execute
     sign_in(user)
 
     visit project_wikis_path(project)
@@ -51,6 +51,16 @@ feature 'Projects > Wiki > User updates wiki page' do
         expect(page).to have_selector('.atwho-view')
       end
     end
+
+    scenario 'page has been updated since the user opened the edit page' do
+      click_link 'Edit'
+
+      wiki_page.update('Update')
+
+      click_button 'Save changes'
+
+      expect(page).to have_content 'Someone edited the page the same time you did.'
+    end
   end
 
   context 'in a group namespace' do
diff --git a/spec/models/wiki_page_spec.rb b/spec/models/wiki_page_spec.rb
index 90ad3cdeb93..55d9c4ddd7b 100644
--- a/spec/models/wiki_page_spec.rb
+++ b/spec/models/wiki_page_spec.rb
@@ -208,6 +208,18 @@ describe WikiPage do
         expect(@page.update("more content")).to be_truthy
       end
     end
+
+    context 'with same last commit sha' do
+      it 'returns true' do
+        expect(@page.update('more content', last_commit_sha: @page.last_commit_sha)).to be_truthy
+      end
+    end
+
+    context 'with different last commit sha' do
+      it 'raises exception' do
+        expect { @page.update('more content', last_commit_sha: 'xxx') }.to raise_error(WikiPage::PageChangedError)
+      end
+    end
   end
 
   describe "#destroy" do
@@ -331,6 +343,30 @@ describe WikiPage do
     end
   end
 
+  describe '#last_commit_sha' do
+    before do
+      create_page("Update", "content")
+      @page = wiki.find_page("Update")
+    end
+
+    after do
+      destroy_page("Update")
+    end
+
+    it 'returns commit sha' do
+      expect(@page.last_commit_sha).to eq @page.commit.sha
+    end
+
+    it 'is changed after page updated' do
+      last_commit_sha_before_update = @page.last_commit_sha
+
+      @page.update("new content")
+      @page = wiki.find_page("Update")
+
+      expect(@page.last_commit_sha).not_to eq last_commit_sha_before_update
+    end
+  end
+
   private
 
   def remove_temp_repo(path)