diff options
author | Tiago Botelho <tiagonbotelho@hotmail.com> | 2018-10-22 14:18:45 +0100 |
---|---|---|
committer | Tiago Botelho <tiagonbotelho@hotmail.com> | 2018-10-25 11:23:34 +0100 |
commit | f8ee07d9ee6d01b255902ad976a07beef59a95fb (patch) | |
tree | b72f437ffe28b36d99fe2b24713127ad27f191e3 | |
parent | f5d71ad8f3d1899a29bc12c4fcc8847b14195e3a (diff) | |
download | gitlab-ce-f8ee07d9ee6d01b255902ad976a07beef59a95fb.tar.gz |
User not defined in PostReceive#process_project_changes
When Gitlab::GitPostReceive#changes_refs is empty
user would not get defined and nil would be passed
to PostReceive#after_project_changes_hooks which would
then throw an error.
-rw-r--r-- | app/workers/post_receive.rb | 12 | ||||
-rw-r--r-- | changelogs/unreleased/51335-fail-early-when-user-cannot-be-identified.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/git_post_receive.rb | 2 | ||||
-rw-r--r-- | spec/workers/post_receive_spec.rb | 124 |
4 files changed, 81 insertions, 62 deletions
diff --git a/app/workers/post_receive.rb b/app/workers/post_receive.rb index 09a594cdb4e..207eef6229f 100644 --- a/app/workers/post_receive.rb +++ b/app/workers/post_receive.rb @@ -29,14 +29,11 @@ class PostReceive def process_project_changes(post_received) changes = [] refs = Set.new + @user = post_received.identify post_received.changes_refs do |oldrev, newrev, ref| @user ||= post_received.identify(newrev) - - unless @user - log("Triggered hook for non-existing user \"#{post_received.identifier}\"") - return false # rubocop:disable Cop/AvoidReturnFromBlocks - end + break unless @user if Gitlab::Git.tag_ref?(ref) GitTagPushService.new(post_received.project, @user, oldrev: oldrev, newrev: newrev, ref: ref).execute @@ -48,6 +45,11 @@ class PostReceive refs << ref end + unless @user + log("Triggered hook for non-existing user \"#{post_received.identifier}\"") + return false + end + after_project_changes_hooks(post_received, @user, refs.to_a, changes) end diff --git a/changelogs/unreleased/51335-fail-early-when-user-cannot-be-identified.yml b/changelogs/unreleased/51335-fail-early-when-user-cannot-be-identified.yml new file mode 100644 index 00000000000..a042debc28f --- /dev/null +++ b/changelogs/unreleased/51335-fail-early-when-user-cannot-be-identified.yml @@ -0,0 +1,5 @@ +--- +title: If user was not found, service hooks won't run on post receive background job +merge_request: 22519 +author: +type: fixed diff --git a/lib/gitlab/git_post_receive.rb b/lib/gitlab/git_post_receive.rb index e731e654f3c..f5470ffa2da 100644 --- a/lib/gitlab/git_post_receive.rb +++ b/lib/gitlab/git_post_receive.rb @@ -11,7 +11,7 @@ module Gitlab @changes = deserialize_changes(changes) end - def identify(revision) + def identify(revision = nil) super(identifier, project, revision) end diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb index cd6661f09a1..1184bd9492a 100644 --- a/spec/workers/post_receive_spec.rb +++ b/spec/workers/post_receive_spec.rb @@ -6,7 +6,7 @@ describe PostReceive do let(:base64_changes) { Base64.encode64(wrongly_encoded_changes) } let(:gl_repository) { "project-#{project.id}" } let(:key) { create(:key, user: project.owner) } - let(:key_id) { key.shell_id } + let!(:key_id) { key.shell_id } let(:project) do create(:project, :repository, auto_cancel_pending_pipelines: 'disabled') @@ -31,85 +31,97 @@ describe PostReceive do end describe "#process_project_changes" do - before do - allow_any_instance_of(Gitlab::GitPostReceive).to receive(:identify).and_return(project.owner) + context 'empty changes' do + it "does not call any PushService but runs after project hooks" do + expect(GitPushService).not_to receive(:new) + expect(GitTagPushService).not_to receive(:new) + expect_next_instance_of(SystemHooksService) { |service| expect(service).to receive(:execute_hooks) } + + described_class.new.perform(gl_repository, key_id, "") + end end - context "branches" do - let(:changes) { "123456 789012 refs/heads/tést" } + context 'with changes' do + before do + allow_any_instance_of(Gitlab::GitPostReceive).to receive(:identify).and_return(project.owner) + end + + context "branches" do + let(:changes) { "123456 789012 refs/heads/tést" } - it "calls GitTagPushService" do - expect_any_instance_of(GitPushService).to receive(:execute).and_return(true) - expect_any_instance_of(GitTagPushService).not_to receive(:execute) - described_class.new.perform(gl_repository, key_id, base64_changes) + it "calls GitPushService" do + expect_any_instance_of(GitPushService).to receive(:execute).and_return(true) + expect_any_instance_of(GitTagPushService).not_to receive(:execute) + described_class.new.perform(gl_repository, key_id, base64_changes) + end end - end - context "tags" do - let(:changes) { "123456 789012 refs/tags/tag" } + context "tags" do + let(:changes) { "123456 789012 refs/tags/tag" } - it "calls GitTagPushService" do - expect_any_instance_of(GitPushService).not_to receive(:execute) - expect_any_instance_of(GitTagPushService).to receive(:execute).and_return(true) - described_class.new.perform(gl_repository, key_id, base64_changes) + it "calls GitTagPushService" do + expect_any_instance_of(GitPushService).not_to receive(:execute) + expect_any_instance_of(GitTagPushService).to receive(:execute).and_return(true) + described_class.new.perform(gl_repository, key_id, base64_changes) + end end - end - context "merge-requests" do - let(:changes) { "123456 789012 refs/merge-requests/123" } + context "merge-requests" do + let(:changes) { "123456 789012 refs/merge-requests/123" } - it "does not call any of the services" do - expect_any_instance_of(GitPushService).not_to receive(:execute) - expect_any_instance_of(GitTagPushService).not_to receive(:execute) - described_class.new.perform(gl_repository, key_id, base64_changes) + it "does not call any of the services" do + expect_any_instance_of(GitPushService).not_to receive(:execute) + expect_any_instance_of(GitTagPushService).not_to receive(:execute) + described_class.new.perform(gl_repository, key_id, base64_changes) + end end - end - context "gitlab-ci.yml" do - let(:changes) { "123456 789012 refs/heads/feature\n654321 210987 refs/tags/tag" } + context "gitlab-ci.yml" do + let(:changes) { "123456 789012 refs/heads/feature\n654321 210987 refs/tags/tag" } - subject { described_class.new.perform(gl_repository, key_id, base64_changes) } + subject { described_class.new.perform(gl_repository, key_id, base64_changes) } - context "creates a Ci::Pipeline for every change" do - before do - stub_ci_pipeline_to_return_yaml_file + context "creates a Ci::Pipeline for every change" do + before do + stub_ci_pipeline_to_return_yaml_file + + allow_any_instance_of(Project) + .to receive(:commit) + .and_return(project.commit) - allow_any_instance_of(Project) - .to receive(:commit) - .and_return(project.commit) + allow_any_instance_of(Repository) + .to receive(:branch_exists?) + .and_return(true) + end - allow_any_instance_of(Repository) - .to receive(:branch_exists?) - .and_return(true) + it { expect { subject }.to change { Ci::Pipeline.count }.by(2) } end - it { expect { subject }.to change { Ci::Pipeline.count }.by(2) } - end + context "does not create a Ci::Pipeline" do + before do + stub_ci_pipeline_yaml_file(nil) + end - context "does not create a Ci::Pipeline" do - before do - stub_ci_pipeline_yaml_file(nil) + it { expect { subject }.not_to change { Ci::Pipeline.count } } end - - it { expect { subject }.not_to change { Ci::Pipeline.count } } end - end - context 'after project changes hooks' do - let(:changes) { '123456 789012 refs/heads/tést' } - let(:fake_hook_data) { Hash.new(event_name: 'repository_update') } + context 'after project changes hooks' do + let(:changes) { '123456 789012 refs/heads/tést' } + let(:fake_hook_data) { Hash.new(event_name: 'repository_update') } - before do - allow_any_instance_of(Gitlab::DataBuilder::Repository).to receive(:update).and_return(fake_hook_data) - # silence hooks so we can isolate - allow_any_instance_of(Key).to receive(:post_create_hook).and_return(true) - allow_any_instance_of(GitPushService).to receive(:execute).and_return(true) - end + before do + allow_any_instance_of(Gitlab::DataBuilder::Repository).to receive(:update).and_return(fake_hook_data) + # silence hooks so we can isolate + allow_any_instance_of(Key).to receive(:post_create_hook).and_return(true) + allow_any_instance_of(GitPushService).to receive(:execute).and_return(true) + end - it 'calls SystemHooksService' do - expect_any_instance_of(SystemHooksService).to receive(:execute_hooks).with(fake_hook_data, :repository_update_hooks).and_return(true) + it 'calls SystemHooksService' do + expect_any_instance_of(SystemHooksService).to receive(:execute_hooks).with(fake_hook_data, :repository_update_hooks).and_return(true) - described_class.new.perform(gl_repository, key_id, base64_changes) + described_class.new.perform(gl_repository, key_id, base64_changes) + end end end end |