From 09d8c77ab2338593a303d73360dc9e5db706394b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Wed, 16 Jan 2019 20:51:41 +0100 Subject: Prevent checking protected_ref? for ambiguous refs --- lib/gitlab/ci/pipeline/chain/command.rb | 2 ++ spec/lib/gitlab/ci/pipeline/chain/command_spec.rb | 11 +++++++++++ 2 files changed, 13 insertions(+) diff --git a/lib/gitlab/ci/pipeline/chain/command.rb b/lib/gitlab/ci/pipeline/chain/command.rb index e62d547d862..e0172e56f56 100644 --- a/lib/gitlab/ci/pipeline/chain/command.rb +++ b/lib/gitlab/ci/pipeline/chain/command.rb @@ -53,6 +53,8 @@ module Gitlab end def protected_ref? + return if ambiguous_ref? + strong_memoize(:protected_ref) do project.protected_for?(origin_ref) end diff --git a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb index 6aa802ce6fd..a00d4339828 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb @@ -181,6 +181,17 @@ describe Gitlab::Ci::Pipeline::Chain::Command do it { is_expected.to eq(false) } end + + context 'when ref is ambiguous' do + before do + project.repository.add_tag(project.creator, 'ref', 'master') + project.repository.add_branch(project.creator, 'ref', 'master') + end + + it 'does not raise an error' do + expect { subject }.not_to raise_error + end + end end describe '#ambiguous_ref' do -- cgit v1.2.1 From c739efa9d3f7662fe0006e8739efe5b076dc5db4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Wed, 16 Jan 2019 21:01:16 +0100 Subject: Add CHANGELOG entry --- ...6-when-ref-is-ambiguous-createpipelineservice-raises-an-error.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/55966-when-ref-is-ambiguous-createpipelineservice-raises-an-error.yml diff --git a/changelogs/unreleased/55966-when-ref-is-ambiguous-createpipelineservice-raises-an-error.yml b/changelogs/unreleased/55966-when-ref-is-ambiguous-createpipelineservice-raises-an-error.yml new file mode 100644 index 00000000000..01a162944d3 --- /dev/null +++ b/changelogs/unreleased/55966-when-ref-is-ambiguous-createpipelineservice-raises-an-error.yml @@ -0,0 +1,5 @@ +--- +title: Prevent checking protected_ref? for ambiguous refs. +merge_request: 24437 +author: +type: fixed -- cgit v1.2.1 From 673b80977542e1f8725a288e84287b0e5606b466 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 18 Jan 2019 14:41:49 +0100 Subject: Move assignment to protected to separate step This moves setting the protected attribute of a pipeline to a separate pipeline chain step in order to perform the assignment after validation. --- app/services/ci/create_pipeline_service.rb | 1 + lib/gitlab/ci/pipeline/chain/build.rb | 1 - lib/gitlab/ci/pipeline/chain/command.rb | 2 -- lib/gitlab/ci/pipeline/chain/protect.rb | 19 ++++++++++ spec/lib/gitlab/ci/pipeline/chain/command_spec.rb | 11 ------ spec/lib/gitlab/ci/pipeline/chain/protect_spec.rb | 43 +++++++++++++++++++++++ 6 files changed, 63 insertions(+), 14 deletions(-) create mode 100644 lib/gitlab/ci/pipeline/chain/protect.rb create mode 100644 spec/lib/gitlab/ci/pipeline/chain/protect_spec.rb diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index f8d8ef04001..ead6c99b4c7 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -10,6 +10,7 @@ module Ci Gitlab::Ci::Pipeline::Chain::Validate::Abilities, Gitlab::Ci::Pipeline::Chain::Validate::Repository, Gitlab::Ci::Pipeline::Chain::Validate::Config, + Gitlab::Ci::Pipeline::Chain::Protect, Gitlab::Ci::Pipeline::Chain::Skip, Gitlab::Ci::Pipeline::Chain::Populate, Gitlab::Ci::Pipeline::Chain::Create].freeze diff --git a/lib/gitlab/ci/pipeline/chain/build.rb b/lib/gitlab/ci/pipeline/chain/build.rb index d33d1edfe35..41632211374 100644 --- a/lib/gitlab/ci/pipeline/chain/build.rb +++ b/lib/gitlab/ci/pipeline/chain/build.rb @@ -17,7 +17,6 @@ module Gitlab user: @command.current_user, pipeline_schedule: @command.schedule, merge_request: @command.merge_request, - protected: @command.protected_ref?, variables_attributes: Array(@command.variables_attributes) ) diff --git a/lib/gitlab/ci/pipeline/chain/command.rb b/lib/gitlab/ci/pipeline/chain/command.rb index e0172e56f56..e62d547d862 100644 --- a/lib/gitlab/ci/pipeline/chain/command.rb +++ b/lib/gitlab/ci/pipeline/chain/command.rb @@ -53,8 +53,6 @@ module Gitlab end def protected_ref? - return if ambiguous_ref? - strong_memoize(:protected_ref) do project.protected_for?(origin_ref) end diff --git a/lib/gitlab/ci/pipeline/chain/protect.rb b/lib/gitlab/ci/pipeline/chain/protect.rb new file mode 100644 index 00000000000..4c743d6b988 --- /dev/null +++ b/lib/gitlab/ci/pipeline/chain/protect.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module Gitlab + module Ci + module Pipeline + module Chain + class Protect < Chain::Base + def perform! + @pipeline.protected = @command.protected_ref? + end + + def break? + @pipeline.protected? != @command.protected_ref? + end + end + end + end + end +end diff --git a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb index a00d4339828..6aa802ce6fd 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb @@ -181,17 +181,6 @@ describe Gitlab::Ci::Pipeline::Chain::Command do it { is_expected.to eq(false) } end - - context 'when ref is ambiguous' do - before do - project.repository.add_tag(project.creator, 'ref', 'master') - project.repository.add_branch(project.creator, 'ref', 'master') - end - - it 'does not raise an error' do - expect { subject }.not_to raise_error - end - end end describe '#ambiguous_ref' do diff --git a/spec/lib/gitlab/ci/pipeline/chain/protect_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/protect_spec.rb new file mode 100644 index 00000000000..8523ebb83fc --- /dev/null +++ b/spec/lib/gitlab/ci/pipeline/chain/protect_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Ci::Pipeline::Chain::Protect do + set(:project) { create(:project) } + set(:user) { create(:user) } + + let(:pipeline) do + build(:ci_empty_pipeline, project: project, ref: 'master') + end + + let(:command) do + Gitlab::Ci::Pipeline::Chain::Command.new( + project: project, current_user: user, origin_ref: 'master') + end + + let(:step) { described_class.new(pipeline, command) } + + context 'when the ref is protected' do + before do + allow(project).to receive(:protected_for?).with('master').and_return(true) + + step.perform! + end + + it 'protects the pipeline' do + expect(pipeline.protected).to eq(true) + end + end + + context 'when the ref is not protected' do + before do + allow(project).to receive(:protected_for?).with('master').and_return(false) + + step.perform! + end + + it 'does not protect the pipeline' do + expect(pipeline.protected).to eq(false) + end + end +end -- cgit v1.2.1 From a2477ec2e363c58857e5f8f08a370282bffa57cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 21 Jan 2019 13:30:27 +0100 Subject: Assign pipeline protected attribute in Populate This removes the Protect pipeline chain step and assigns the protected attribute in the Populate step instead. --- app/services/ci/create_pipeline_service.rb | 1 - lib/gitlab/ci/pipeline/chain/populate.rb | 4 ++ lib/gitlab/ci/pipeline/chain/protect.rb | 19 ---------- spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb | 25 +++++++++++++ spec/lib/gitlab/ci/pipeline/chain/protect_spec.rb | 43 ---------------------- 5 files changed, 29 insertions(+), 63 deletions(-) delete mode 100644 lib/gitlab/ci/pipeline/chain/protect.rb delete mode 100644 spec/lib/gitlab/ci/pipeline/chain/protect_spec.rb diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index ead6c99b4c7..f8d8ef04001 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -10,7 +10,6 @@ module Ci Gitlab::Ci::Pipeline::Chain::Validate::Abilities, Gitlab::Ci::Pipeline::Chain::Validate::Repository, Gitlab::Ci::Pipeline::Chain::Validate::Config, - Gitlab::Ci::Pipeline::Chain::Protect, Gitlab::Ci::Pipeline::Chain::Skip, Gitlab::Ci::Pipeline::Chain::Populate, Gitlab::Ci::Pipeline::Chain::Create].freeze diff --git a/lib/gitlab/ci/pipeline/chain/populate.rb b/lib/gitlab/ci/pipeline/chain/populate.rb index 633d3cd4f6b..0405292a25b 100644 --- a/lib/gitlab/ci/pipeline/chain/populate.rb +++ b/lib/gitlab/ci/pipeline/chain/populate.rb @@ -13,6 +13,10 @@ module Gitlab # Allocate next IID. This operation must be outside of transactions of pipeline creations. pipeline.ensure_project_iid! + # Protect the pipeline. This is assigned in Populate instead of + # Build to prevent erroring out on ambiguous refs. + pipeline.protected = @command.protected_ref? + ## # Populate pipeline with block argument of CreatePipelineService#execute. # diff --git a/lib/gitlab/ci/pipeline/chain/protect.rb b/lib/gitlab/ci/pipeline/chain/protect.rb deleted file mode 100644 index 4c743d6b988..00000000000 --- a/lib/gitlab/ci/pipeline/chain/protect.rb +++ /dev/null @@ -1,19 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Ci - module Pipeline - module Chain - class Protect < Chain::Base - def perform! - @pipeline.protected = @command.protected_ref? - end - - def break? - @pipeline.protected? != @command.protected_ref? - end - end - end - end - end -end diff --git a/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb index 1b014ecfaa4..3459939267a 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb @@ -79,6 +79,31 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do end end + describe 'pipeline protect' do + subject { step.perform! } + + context 'when ref is protected' do + before do + allow(project).to receive(:protected_for?).with('master').and_return(true) + allow(project).to receive(:protected_for?).with('refs/heads/master').and_return(true) + end + + it 'does not protect the pipeline' do + subject + + expect(pipeline.protected).to eq(true) + end + end + + context 'when ref is not protected' do + it 'does not protect the pipeline' do + subject + + expect(pipeline.protected).to eq(false) + end + end + end + context 'when pipeline has validation errors' do let(:pipeline) do build(:ci_pipeline, project: project, ref: nil) diff --git a/spec/lib/gitlab/ci/pipeline/chain/protect_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/protect_spec.rb deleted file mode 100644 index 8523ebb83fc..00000000000 --- a/spec/lib/gitlab/ci/pipeline/chain/protect_spec.rb +++ /dev/null @@ -1,43 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Gitlab::Ci::Pipeline::Chain::Protect do - set(:project) { create(:project) } - set(:user) { create(:user) } - - let(:pipeline) do - build(:ci_empty_pipeline, project: project, ref: 'master') - end - - let(:command) do - Gitlab::Ci::Pipeline::Chain::Command.new( - project: project, current_user: user, origin_ref: 'master') - end - - let(:step) { described_class.new(pipeline, command) } - - context 'when the ref is protected' do - before do - allow(project).to receive(:protected_for?).with('master').and_return(true) - - step.perform! - end - - it 'protects the pipeline' do - expect(pipeline.protected).to eq(true) - end - end - - context 'when the ref is not protected' do - before do - allow(project).to receive(:protected_for?).with('master').and_return(false) - - step.perform! - end - - it 'does not protect the pipeline' do - expect(pipeline.protected).to eq(false) - end - end -end -- cgit v1.2.1