diff --git a/Gemfile b/Gemfile index 1c822a40fc6cda96e78159ead695a00335ae4054..6d820e1f88782ab77e2794b202fb69838b34374f 100644 --- a/Gemfile +++ b/Gemfile @@ -82,7 +82,7 @@ gem 'net-ldap' # Git Wiki # Required manually in config/initializers/gollum.rb to control load order -gem 'gitlab-gollum-lib', '~> 4.2' +gem 'gitlab-gollum-lib', '~> 4.2', require: false gem 'gitlab-gollum-rugged_adapter', '~> 0.4.4', require: false @@ -415,7 +415,7 @@ group :ed25519 do end # Gitaly GRPC client -gem 'gitaly-proto', '~> 0.94.0', require: 'gitaly' +gem 'gitaly-proto', '~> 0.97.0', require: 'gitaly' gem 'grpc', '~> 1.10.0' # Locked until https://github.com/google/protobuf/issues/4210 is closed diff --git a/Gemfile.lock b/Gemfile.lock index 7f243491c90eb5e231ba592cf8b83ef0af2cf5a5..3076fbee6a88dea99a92dc0d5be5eec6342edbdd 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -290,9 +290,9 @@ GEM po_to_json (>= 1.0.0) rails (>= 3.2.0) gherkin-ruby (0.3.2) - gitaly-proto (0.94.0) + gitaly-proto (0.97.0) google-protobuf (~> 3.1) - grpc (~> 1.0) + grpc (~> 1.10) github-linguist (5.3.3) charlock_holmes (~> 0.7.5) escape_utils (~> 1.1.0) @@ -1064,7 +1064,7 @@ DEPENDENCIES gettext (~> 3.2.2) gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails_js (~> 1.3) - gitaly-proto (~> 0.94.0) + gitaly-proto (~> 0.97.0) github-linguist (~> 5.3.3) gitlab-flowdock-git-hook (~> 1.0.1) gitlab-gollum-lib (~> 4.2) diff --git a/app/controllers/projects/wikis_controller.rb b/app/controllers/projects/wikis_controller.rb index c4930d3d18d736d36504bdaf8a0602ddc1662b4e..1b0751f48c57006456e41f1e06ad0dcfe3b83490 100644 --- a/app/controllers/projects/wikis_controller.rb +++ b/app/controllers/projects/wikis_controller.rb @@ -29,8 +29,7 @@ class Projects::WikisController < Projects::ApplicationController else return render('empty') unless can?(current_user, :create_wiki, @project) - @page = WikiPage.new(@project_wiki) - @page.title = params[:id] + @page = build_page(title: params[:id]) render 'edit' end @@ -54,7 +53,7 @@ class Projects::WikisController < Projects::ApplicationController else render 'edit' end - rescue WikiPage::PageChangedError, WikiPage::PageRenameError => e + rescue WikiPage::PageChangedError, WikiPage::PageRenameError, Gitlab::Git::Wiki::OperationError => e @error = e render 'edit' end @@ -70,6 +69,11 @@ class Projects::WikisController < Projects::ApplicationController else render action: "edit" end + rescue Gitlab::Git::Wiki::OperationError => e + @page = build_page(wiki_params) + @error = e + + render 'edit' end def history @@ -94,6 +98,9 @@ class Projects::WikisController < Projects::ApplicationController redirect_to project_wiki_path(@project, :home), status: 302, notice: "Page was successfully deleted" + rescue Gitlab::Git::Wiki::OperationError => e + @error = e + render 'edit' end def git_access @@ -116,4 +123,10 @@ class Projects::WikisController < Projects::ApplicationController def wiki_params params.require(:wiki).permit(:title, :content, :format, :message, :last_commit_sha) end + + def build_page(args) + WikiPage.new(@project_wiki).tap do |page| + page.update_attributes(args) + end + end end diff --git a/app/helpers/nav_helper.rb b/app/helpers/nav_helper.rb index 56c88e6eab0db7b9a92651faf22cb0a1fe62fdb3..7754c34d6f03c6c8d4931a679d29d7f7332a157d 100644 --- a/app/helpers/nav_helper.rb +++ b/app/helpers/nav_helper.rb @@ -28,7 +28,7 @@ module NavHelper end elsif current_path?('jobs#show') %w[page-gutter build-sidebar right-sidebar-expanded] - elsif current_controller?('wikis') && current_action?('show', 'create', 'edit', 'update', 'history', 'git_access') + elsif current_controller?('wikis') && current_action?('show', 'create', 'edit', 'update', 'history', 'git_access', 'destroy') %w[page-gutter wiki-sidebar right-sidebar-expanded] else [] diff --git a/app/models/project_wiki.rb b/app/models/project_wiki.rb index 52e067cb44c5ffddcaa591792d9e739bcb83de71..b7e38ceb5021a0dfdab7269914b528b9396166af 100644 --- a/app/models/project_wiki.rb +++ b/app/models/project_wiki.rb @@ -179,7 +179,11 @@ class ProjectWiki def commit_details(action, message = nil, title = nil) commit_message = message || default_message(action, title) - Gitlab::Git::Wiki::CommitDetails.new(@user.name, @user.email, commit_message) + Gitlab::Git::Wiki::CommitDetails.new(@user.id, + @user.username, + @user.name, + @user.email, + commit_message) end def default_message(action, title) diff --git a/app/models/wiki_page.rb b/app/models/wiki_page.rb index 0f5536415f79ceea274c468cc0da5f14694d2799..cde79b95062373e177ccd37503267477bb1e9930 100644 --- a/app/models/wiki_page.rb +++ b/app/models/wiki_page.rb @@ -265,6 +265,15 @@ class WikiPage title.present? && self.class.unhyphenize(@page.url_path) != title end + # Updates the current @attributes hash by merging a hash of params + def update_attributes(attrs) + attrs[:title] = process_title(attrs[:title]) if attrs[:title].present? + + attrs.slice!(:content, :format, :message, :title) + + @attributes.merge!(attrs) + end + private # Process and format the title based on the user input. @@ -290,15 +299,6 @@ class WikiPage File.join(components) end - # Updates the current @attributes hash by merging a hash of params - def update_attributes(attrs) - attrs[:title] = process_title(attrs[:title]) if attrs[:title].present? - - attrs.slice!(:content, :format, :message, :title) - - @attributes.merge!(attrs) - end - def set_attributes attributes[:slug] = @page.url_path attributes[:title] = @page.title diff --git a/changelogs/unreleased/fj-42354-custom-hooks-not-triggered-by-UI-wiki-edit.yml b/changelogs/unreleased/fj-42354-custom-hooks-not-triggered-by-UI-wiki-edit.yml new file mode 100644 index 0000000000000000000000000000000000000000..9fe458aba4a4937bab91efce7d1027cc36f2d67a --- /dev/null +++ b/changelogs/unreleased/fj-42354-custom-hooks-not-triggered-by-UI-wiki-edit.yml @@ -0,0 +1,5 @@ +--- +title: Triggering custom hooks by Wiki UI edit +merge_request: 18251 +author: +type: fixed diff --git a/lib/gitlab/git/wiki.rb b/lib/gitlab/git/wiki.rb index 8d82820915db63adbab1f7e957adba6f13ffdbc8..821436911ab50bd2abe9b7ff3f604c0d2ae712cf 100644 --- a/lib/gitlab/git/wiki.rb +++ b/lib/gitlab/git/wiki.rb @@ -2,10 +2,11 @@ module Gitlab module Git class Wiki DuplicatePageError = Class.new(StandardError) + OperationError = Class.new(StandardError) - CommitDetails = Struct.new(:name, :email, :message) do + CommitDetails = Struct.new(:user_id, :username, :name, :email, :message) do def to_h - { name: name, email: email, message: message } + { user_id: user_id, username: username, name: name, email: email, message: message } end end PageBlob = Struct.new(:name) @@ -140,6 +141,10 @@ module Gitlab end end + def gollum_wiki + @gollum_wiki ||= Gollum::Wiki.new(@repository.path) + end + private # options: @@ -158,10 +163,6 @@ module Gitlab offset: options[:offset]) end - def gollum_wiki - @gollum_wiki ||= Gollum::Wiki.new(@repository.path) - end - def gollum_page_by_path(page_path) page_name = Gollum::Page.canonicalize_filename(page_path) page_dir = File.split(page_path).first @@ -201,12 +202,12 @@ module Gitlab assert_type!(format, Symbol) assert_type!(commit_details, CommitDetails) - filename = File.basename(name) - dir = (tmp_dir = File.dirname(name)) == '.' ? '' : tmp_dir - - gollum_wiki.write_page(filename, format, content, commit_details.to_h, dir) + with_committer_with_hooks(commit_details) do |committer| + filename = File.basename(name) + dir = (tmp_dir = File.dirname(name)) == '.' ? '' : tmp_dir - nil + gollum_wiki.write_page(filename, format, content, { committer: committer }, dir) + end rescue Gollum::DuplicatePageError => e raise Gitlab::Git::Wiki::DuplicatePageError, e.message end @@ -214,24 +215,23 @@ module Gitlab def gollum_delete_page(page_path, commit_details) assert_type!(commit_details, CommitDetails) - gollum_wiki.delete_page(gollum_page_by_path(page_path), commit_details.to_h) - nil + with_committer_with_hooks(commit_details) do |committer| + gollum_wiki.delete_page(gollum_page_by_path(page_path), committer: committer) + end end def gollum_update_page(page_path, title, format, content, commit_details) assert_type!(format, Symbol) assert_type!(commit_details, CommitDetails) - page = gollum_page_by_path(page_path) - committer = Gollum::Committer.new(page.wiki, commit_details.to_h) - - # Instead of performing two renames if the title has changed, - # the update_page will only update the format and content and - # the rename_page will do anything related to moving/renaming - gollum_wiki.update_page(page, page.name, format, content, committer: committer) - gollum_wiki.rename_page(page, title, committer: committer) - committer.commit - nil + with_committer_with_hooks(commit_details) do |committer| + page = gollum_page_by_path(page_path) + # Instead of performing two renames if the title has changed, + # the update_page will only update the format and content and + # the rename_page will do anything related to moving/renaming + gollum_wiki.update_page(page, page.name, format, content, committer: committer) + gollum_wiki.rename_page(page, title, committer: committer) + end end def gollum_find_page(title:, version: nil, dir: nil) @@ -288,6 +288,20 @@ module Gitlab Gitlab::Git::WikiPage.new(wiki_page, version) end end + + def committer_with_hooks(commit_details) + Gitlab::Wiki::CommitterWithHooks.new(self, commit_details.to_h) + end + + def with_committer_with_hooks(commit_details, &block) + committer = committer_with_hooks(commit_details) + + yield committer + + committer.commit + + nil + end end end end diff --git a/lib/gitlab/gitaly_client/wiki_service.rb b/lib/gitlab/gitaly_client/wiki_service.rb index 7a698e4b3f39e58609128c438dc5f468ba157b74..2dfe055a496b9548ee851b0c2d32e2c13959ba81 100644 --- a/lib/gitlab/gitaly_client/wiki_service.rb +++ b/lib/gitlab/gitaly_client/wiki_service.rb @@ -200,6 +200,8 @@ module Gitlab def gitaly_commit_details(commit_details) Gitaly::WikiCommitDetails.new( + user_id: commit_details.user_id, + user_name: encode_binary(commit_details.username), name: encode_binary(commit_details.name), email: encode_binary(commit_details.email), message: encode_binary(commit_details.message) diff --git a/lib/gitlab/gl_id.rb b/lib/gitlab/gl_id.rb index 624fd00367e0f5c128497449630242824b62e416..a53d156b41f3fa7e28204d5b301c4fed62329153 100644 --- a/lib/gitlab/gl_id.rb +++ b/lib/gitlab/gl_id.rb @@ -2,10 +2,14 @@ module Gitlab module GlId def self.gl_id(user) if user.present? - "user-#{user.id}" + gl_id_from_id_value(user.id) else - "" + '' end end + + def self.gl_id_from_id_value(id) + "user-#{id}" + end end end diff --git a/lib/gitlab/wiki/committer_with_hooks.rb b/lib/gitlab/wiki/committer_with_hooks.rb new file mode 100644 index 0000000000000000000000000000000000000000..19f0b3814fd48e998043274d197a33faef2edaf0 --- /dev/null +++ b/lib/gitlab/wiki/committer_with_hooks.rb @@ -0,0 +1,39 @@ +module Gitlab + module Wiki + class CommitterWithHooks < Gollum::Committer + attr_reader :gl_wiki + + def initialize(gl_wiki, options = {}) + @gl_wiki = gl_wiki + super(gl_wiki.gollum_wiki, options) + end + + def commit + result = Gitlab::Git::OperationService.new(git_user, gl_wiki.repository).with_branch( + @wiki.ref, + start_branch_name: @wiki.ref + ) do |start_commit| + super(false) + end + + result[:newrev] + rescue Gitlab::Git::HooksService::PreReceiveError => e + message = "Custom Hook failed: #{e.message}" + raise Gitlab::Git::Wiki::OperationError, message + end + + private + + def git_user + @git_user ||= Gitlab::Git::User.new(@options[:username], + @options[:name], + @options[:email], + gitlab_id) + end + + def gitlab_id + Gitlab::GlId.gl_id_from_id_value(@options[:user_id]) + end + end + end +end diff --git a/spec/lib/gitlab/git/wiki_spec.rb b/spec/lib/gitlab/git/wiki_spec.rb index 761f77320365b18bdd3f0b6986d3dbb4c7a5dc37..722d697c28e8cbdaaa43a52a47af5098dab1c65e 100644 --- a/spec/lib/gitlab/git/wiki_spec.rb +++ b/spec/lib/gitlab/git/wiki_spec.rb @@ -30,7 +30,7 @@ describe Gitlab::Git::Wiki do end def commit_details(name) - Gitlab::Git::Wiki::CommitDetails.new(user.name, user.email, "created page #{name}") + Gitlab::Git::Wiki::CommitDetails.new(user.id, user.username, user.name, user.email, "created page #{name}") end def destroy_page(title, dir = '') diff --git a/spec/lib/gitlab/wiki/committer_with_hooks_spec.rb b/spec/lib/gitlab/wiki/committer_with_hooks_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..830fb8a85983f64287942ce5d211876edaac230b --- /dev/null +++ b/spec/lib/gitlab/wiki/committer_with_hooks_spec.rb @@ -0,0 +1,154 @@ +require 'spec_helper' + +describe Gitlab::Wiki::CommitterWithHooks, seed_helper: true do + shared_examples 'calling wiki hooks' do + let(:project) { create(:project) } + let(:user) { project.owner } + let(:project_wiki) { ProjectWiki.new(project, user) } + let(:wiki) { project_wiki.wiki } + let(:options) do + { + id: user.id, + username: user.username, + name: user.name, + email: user.email, + message: 'commit message' + } + end + + subject { described_class.new(wiki, options) } + + before do + project_wiki.create_page('home', 'test content') + end + + shared_examples 'failing pre-receive hook' do + before do + expect_any_instance_of(Gitlab::Git::HooksService).to receive(:run_hook).with('pre-receive').and_return([false, '']) + expect_any_instance_of(Gitlab::Git::HooksService).not_to receive(:run_hook).with('update') + expect_any_instance_of(Gitlab::Git::HooksService).not_to receive(:run_hook).with('post-receive') + end + + it 'raises exception' do + expect { subject.commit }.to raise_error(Gitlab::Git::Wiki::OperationError) + end + + it 'does not create a new commit inside the repository' do + current_rev = find_current_rev + + expect { subject.commit }.to raise_error(Gitlab::Git::Wiki::OperationError) + + expect(current_rev).to eq find_current_rev + end + end + + shared_examples 'failing update hook' do + before do + expect_any_instance_of(Gitlab::Git::HooksService).to receive(:run_hook).with('pre-receive').and_return([true, '']) + expect_any_instance_of(Gitlab::Git::HooksService).to receive(:run_hook).with('update').and_return([false, '']) + expect_any_instance_of(Gitlab::Git::HooksService).not_to receive(:run_hook).with('post-receive') + end + + it 'raises exception' do + expect { subject.commit }.to raise_error(Gitlab::Git::Wiki::OperationError) + end + + it 'does not create a new commit inside the repository' do + current_rev = find_current_rev + + expect { subject.commit }.to raise_error(Gitlab::Git::Wiki::OperationError) + + expect(current_rev).to eq find_current_rev + end + end + + shared_examples 'failing post-receive hook' do + before do + expect_any_instance_of(Gitlab::Git::HooksService).to receive(:run_hook).with('pre-receive').and_return([true, '']) + expect_any_instance_of(Gitlab::Git::HooksService).to receive(:run_hook).with('update').and_return([true, '']) + expect_any_instance_of(Gitlab::Git::HooksService).to receive(:run_hook).with('post-receive').and_return([false, '']) + end + + it 'does not raise exception' do + expect { subject.commit }.not_to raise_error + end + + it 'creates the commit' do + current_rev = find_current_rev + + subject.commit + + expect(current_rev).not_to eq find_current_rev + end + end + + shared_examples 'when hooks call succceeds' do + let(:hook) { double(:hook) } + + it 'calls the three hooks' do + expect(Gitlab::Git::Hook).to receive(:new).exactly(3).times.and_return(hook) + expect(hook).to receive(:trigger).exactly(3).times.and_return([true, nil]) + + subject.commit + end + + it 'creates the commit' do + current_rev = find_current_rev + + subject.commit + + expect(current_rev).not_to eq find_current_rev + end + end + + context 'when creating a page' do + before do + project_wiki.create_page('index', 'test content') + end + + it_behaves_like 'failing pre-receive hook' + it_behaves_like 'failing update hook' + it_behaves_like 'failing post-receive hook' + it_behaves_like 'when hooks call succceeds' + end + + context 'when updating a page' do + before do + project_wiki.update_page(find_page('home'), content: 'some other content', format: :markdown) + end + + it_behaves_like 'failing pre-receive hook' + it_behaves_like 'failing update hook' + it_behaves_like 'failing post-receive hook' + it_behaves_like 'when hooks call succceeds' + end + + context 'when deleting a page' do + before do + project_wiki.delete_page(find_page('home')) + end + + it_behaves_like 'failing pre-receive hook' + it_behaves_like 'failing update hook' + it_behaves_like 'failing post-receive hook' + it_behaves_like 'when hooks call succceeds' + end + + def find_current_rev + wiki.gollum_wiki.repo.commits.first&.sha + end + + def find_page(name) + wiki.page(title: name) + end + end + + # TODO: Uncomment once Gitaly updates the ruby vendor code + # context 'when Gitaly is enabled' do + # it_behaves_like 'calling wiki hooks' + # end + + context 'when Gitaly is disabled', :skip_gitaly_mock do + it_behaves_like 'calling wiki hooks' + end +end diff --git a/spec/models/project_wiki_spec.rb b/spec/models/project_wiki_spec.rb index 374a157bec007147d6a61a2a1b4cb9c09e8b2e8a..4e83f4353cf8540d29c8518f89b4b9c85b8438bb 100644 --- a/spec/models/project_wiki_spec.rb +++ b/spec/models/project_wiki_spec.rb @@ -377,7 +377,7 @@ describe ProjectWiki do end def commit_details - Gitlab::Git::Wiki::CommitDetails.new(user.name, user.email, "test commit") + Gitlab::Git::Wiki::CommitDetails.new(user.id, user.username, user.name, user.email, "test commit") end def create_page(name, content) diff --git a/spec/models/wiki_page_spec.rb b/spec/models/wiki_page_spec.rb index b2b7721674c5797efcf92018fd30912ed058385d..90b7e7715a84da0a51051d274dc1fc870225b80b 100644 --- a/spec/models/wiki_page_spec.rb +++ b/spec/models/wiki_page_spec.rb @@ -561,7 +561,7 @@ describe WikiPage do end def commit_details - Gitlab::Git::Wiki::CommitDetails.new(user.name, user.email, "test commit") + Gitlab::Git::Wiki::CommitDetails.new(user.id, user.username, user.name, user.email, "test commit") end def create_page(name, content)