From 609fa45f0ed273414eac2798f76daf088e287b30 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 26 Sep 2017 11:50:47 +0200 Subject: Split pipeline chain builder validation class --- app/services/ci/create_pipeline_service.rb | 4 +- lib/gitlab/ci/pipeline/chain/create.rb | 4 +- lib/gitlab/ci/pipeline/chain/helpers.rb | 25 +++++ lib/gitlab/ci/pipeline/chain/validate.rb | 101 ------------------- lib/gitlab/ci/pipeline/chain/validate_abilities.rb | 52 ++++++++++ lib/gitlab/ci/pipeline/chain/validate_config.rb | 33 +++++++ .../ci/pipeline/chain/validate_repository.rb | 30 ++++++ .../ci/pipeline/chain/validate_abilities_spec.rb | 110 +++++++++++++++++++++ spec/lib/gitlab/ci/pipeline/chain/validate_spec.rb | 110 --------------------- 9 files changed, 256 insertions(+), 213 deletions(-) create mode 100644 lib/gitlab/ci/pipeline/chain/helpers.rb delete mode 100644 lib/gitlab/ci/pipeline/chain/validate.rb create mode 100644 lib/gitlab/ci/pipeline/chain/validate_abilities.rb create mode 100644 lib/gitlab/ci/pipeline/chain/validate_config.rb create mode 100644 lib/gitlab/ci/pipeline/chain/validate_repository.rb create mode 100644 spec/lib/gitlab/ci/pipeline/chain/validate_abilities_spec.rb delete mode 100644 spec/lib/gitlab/ci/pipeline/chain/validate_spec.rb diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index b22904fa4f1..ecf9be6600c 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -2,7 +2,9 @@ module Ci class CreatePipelineService < BaseService attr_reader :pipeline - SEQUENCE = [Gitlab::Ci::Pipeline::Chain::Validate, + SEQUENCE = [Gitlab::Ci::Pipeline::Chain::ValidateAbilities, + Gitlab::Ci::Pipeline::Chain::ValidateRepository, + Gitlab::Ci::Pipeline::Chain::ValidateConfig, Gitlab::Ci::Pipeline::Chain::Skip, Gitlab::Ci::Pipeline::Chain::Create].freeze diff --git a/lib/gitlab/ci/pipeline/chain/create.rb b/lib/gitlab/ci/pipeline/chain/create.rb index 3b067ca6ace..8bd658b3069 100644 --- a/lib/gitlab/ci/pipeline/chain/create.rb +++ b/lib/gitlab/ci/pipeline/chain/create.rb @@ -3,6 +3,8 @@ module Gitlab module Pipeline module Chain class Create < Chain::Base + include Chain::Helpers + def perform! ::Ci::Pipeline.transaction do pipeline.save! @@ -16,7 +18,7 @@ module Gitlab .execute(pipeline) end rescue ActiveRecord::RecordInvalid => e - pipeline.erros.add(:base, "Failed to persist the pipeline: #{e}") + error("Failed to persist the pipeline: #{e}") end def break? diff --git a/lib/gitlab/ci/pipeline/chain/helpers.rb b/lib/gitlab/ci/pipeline/chain/helpers.rb new file mode 100644 index 00000000000..02d81286f21 --- /dev/null +++ b/lib/gitlab/ci/pipeline/chain/helpers.rb @@ -0,0 +1,25 @@ +module Gitlab + module Ci + module Pipeline + module Chain + module Helpers + def branch_exists? + return @is_branch if defined?(@is_branch) + + @is_branch = project.repository.branch_exists?(pipeline.ref) + end + + def tag_exists? + return @is_tag if defined?(@is_tag) + + @is_tag = project.repository.tag_exists?(pipeline.ref) + end + + def error(message) + pipeline.errors.add(:base, message) + end + end + end + end + end +end diff --git a/lib/gitlab/ci/pipeline/chain/validate.rb b/lib/gitlab/ci/pipeline/chain/validate.rb deleted file mode 100644 index ec06b0f76c5..00000000000 --- a/lib/gitlab/ci/pipeline/chain/validate.rb +++ /dev/null @@ -1,101 +0,0 @@ -module Gitlab - module Ci - module Pipeline - module Chain - class Validate < Chain::Base - include Gitlab::Allowable - - def perform! - validate_project! || validate_repository! || validate_pipeline! - end - - def break? - @pipeline.errors.any? || @pipeline.persisted? - end - - def allowed_to_trigger_pipeline? - if current_user - allowed_to_create? - else # legacy triggers don't have a corresponding user - !project.protected_for?(@pipeline.ref) - end - end - - def allowed_to_create? - return unless can?(current_user, :create_pipeline, project) - - access = Gitlab::UserAccess.new(current_user, project: project) - - if branch? - access.can_update_branch?(@pipeline.ref) - elsif tag? - access.can_create_tag?(@pipeline.ref) - else - true # Allow it for now and we'll reject when we check ref existence - end - end - - private - - def validate_project! - unless project.builds_enabled? - return error('Pipeline is disabled') - end - - unless allowed_to_trigger_pipeline? - if can?(current_user, :create_pipeline, project) - return error("Insufficient permissions for protected ref '#{pipeline.ref}'") - else - return error('Insufficient permissions to create a new pipeline') - end - end - end - - def validate_repository! - unless branch? || tag? - return error('Reference not found') - end - - unless pipeline.sha - return error('Commit not found') - end - end - - def validate_pipeline! - unless @pipeline.config_processor - unless @pipeline.ci_yaml_file - return error("Missing #{@pipeline.ci_yaml_file_path} file") - end - - if @command.save_incompleted && @pipeline.has_yaml_errors? - @pipeline.drop - end - - return error(@pipeline.yaml_errors) - end - - unless @pipeline.has_stage_seeds? - return error('No stages / jobs for this pipeline.') - end - end - - def branch? - return @is_branch if defined?(@is_branch) - - @is_branch = project.repository.branch_exists?(pipeline.ref) - end - - def tag? - return @is_tag if defined?(@is_tag) - - @is_tag = project.repository.tag_exists?(pipeline.ref) - end - - def error(message) - pipeline.errors.add(:base, message) - end - end - end - end - end -end diff --git a/lib/gitlab/ci/pipeline/chain/validate_abilities.rb b/lib/gitlab/ci/pipeline/chain/validate_abilities.rb new file mode 100644 index 00000000000..28a9c0ba999 --- /dev/null +++ b/lib/gitlab/ci/pipeline/chain/validate_abilities.rb @@ -0,0 +1,52 @@ +module Gitlab + module Ci + module Pipeline + module Chain + class ValidateAbilities < Chain::Base + include Gitlab::Allowable + include Chain::Helpers + + def perform! + unless project.builds_enabled? + return error('Pipelines are disabled!') + end + + unless allowed_to_trigger_pipeline? + if can?(current_user, :create_pipeline, project) + return error("Insufficient permissions for protected ref '#{pipeline.ref}'") + else + return error('Insufficient permissions to create a new pipeline') + end + end + end + + def break? + @pipeline.errors.any? + end + + def allowed_to_trigger_pipeline? + if current_user + allowed_to_create? + else # legacy triggers don't have a corresponding user + !project.protected_for?(@pipeline.ref) + end + end + + def allowed_to_create? + return unless can?(current_user, :create_pipeline, project) + + access = Gitlab::UserAccess.new(current_user, project: project) + + if branch_exists? + access.can_update_branch?(@pipeline.ref) + elsif tag_exists? + access.can_create_tag?(@pipeline.ref) + else + true # Allow it for now and we'll reject when we check ref existence + end + end + end + end + end + end +end diff --git a/lib/gitlab/ci/pipeline/chain/validate_config.rb b/lib/gitlab/ci/pipeline/chain/validate_config.rb new file mode 100644 index 00000000000..0dba8731438 --- /dev/null +++ b/lib/gitlab/ci/pipeline/chain/validate_config.rb @@ -0,0 +1,33 @@ +module Gitlab + module Ci + module Pipeline + module Chain + class ValidateConfig < Chain::Base + include Chain::Helpers + + def perform! + unless @pipeline.config_processor + unless @pipeline.ci_yaml_file + return error("Missing #{@pipeline.ci_yaml_file_path} file") + end + + if @command.save_incompleted && @pipeline.has_yaml_errors? + @pipeline.drop + end + + return error(@pipeline.yaml_errors) + end + + unless @pipeline.has_stage_seeds? + return error('No stages / jobs for this pipeline.') + end + end + + def break? + @pipeline.errors.any? || @pipeline.persisted? + end + end + end + end + end +end diff --git a/lib/gitlab/ci/pipeline/chain/validate_repository.rb b/lib/gitlab/ci/pipeline/chain/validate_repository.rb new file mode 100644 index 00000000000..4d1b88a7065 --- /dev/null +++ b/lib/gitlab/ci/pipeline/chain/validate_repository.rb @@ -0,0 +1,30 @@ +module Gitlab + module Ci + module Pipeline + module Chain + class ValidateRepository < Chain::Base + include Chain::Helpers + + def perform! + unless branch_exists? || tag_exists? + return error('Reference not found') + end + + ## TODO, we check commit in the service, that is why + # there is no repository access here. + # + # Should we validate repository before building a pipeline? + # + unless pipeline.sha + return error('Commit not found') + end + end + + def break? + @pipeline.errors.any? + end + end + end + end + end +end diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate_abilities_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate_abilities_spec.rb new file mode 100644 index 00000000000..1613df395d1 --- /dev/null +++ b/spec/lib/gitlab/ci/pipeline/chain/validate_abilities_spec.rb @@ -0,0 +1,110 @@ +require 'spec_helper' + +describe Gitlab::Ci::Pipeline::Chain::ValidateAbilities do + describe '#allowed_to_create?' do + let(:user) { create(:user) } + let(:project) { create(:project, :repository) } + let(:ref) { 'master' } + + let(:pipeline) do + build_stubbed(:ci_pipeline, ref: ref, project: project) + end + + let(:command) do + double('command', project: project, current_user: user) + end + + subject do + described_class.new(pipeline, command).allowed_to_create? + end + + context 'when user is a developer' do + before do + project.add_developer(user) + end + + it { is_expected.to be_truthy } + + context 'when the branch is protected' do + let!(:protected_branch) do + create(:protected_branch, project: project, name: ref) + end + + it { is_expected.to be_falsey } + + context 'when developers are allowed to merge' do + let!(:protected_branch) do + create(:protected_branch, + :developers_can_merge, + project: project, + name: ref) + end + + it { is_expected.to be_truthy } + end + end + + context 'when the tag is protected' do + let(:ref) { 'v1.0.0' } + + let!(:protected_tag) do + create(:protected_tag, project: project, name: ref) + end + + it { is_expected.to be_falsey } + + context 'when developers are allowed to create the tag' do + let!(:protected_tag) do + create(:protected_tag, + :developers_can_create, + project: project, + name: ref) + end + + it { is_expected.to be_truthy } + end + end + end + + context 'when user is a master' do + before do + project.add_master(user) + end + + it { is_expected.to be_truthy } + + context 'when the branch is protected' do + let!(:protected_branch) do + create(:protected_branch, project: project, name: ref) + end + + it { is_expected.to be_truthy } + end + + context 'when the tag is protected' do + let(:ref) { 'v1.0.0' } + + let!(:protected_tag) do + create(:protected_tag, project: project, name: ref) + end + + it { is_expected.to be_truthy } + + context 'when no one can create the tag' do + let!(:protected_tag) do + create(:protected_tag, + :no_one_can_create, + project: project, + name: ref) + end + + it { is_expected.to be_falsey } + end + end + end + + context 'when owner cannot create pipeline' do + it { is_expected.to be_falsey } + end + end +end diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate_spec.rb deleted file mode 100644 index 117f55fd8a7..00000000000 --- a/spec/lib/gitlab/ci/pipeline/chain/validate_spec.rb +++ /dev/null @@ -1,110 +0,0 @@ -require 'spec_helper' - -describe Gitlab::Ci::Pipeline::Chain::Validate do - describe '#allowed_to_create?' do - let(:user) { create(:user) } - let(:project) { create(:project, :repository) } - let(:ref) { 'master' } - - let(:pipeline) do - build_stubbed(:ci_pipeline, ref: ref, project: project) - end - - let(:command) do - double('command', project: project, current_user: user) - end - - subject do - described_class.new(pipeline, command).allowed_to_create? - end - - context 'when user is a developer' do - before do - project.add_developer(user) - end - - it { is_expected.to be_truthy } - - context 'when the branch is protected' do - let!(:protected_branch) do - create(:protected_branch, project: project, name: ref) - end - - it { is_expected.to be_falsey } - - context 'when developers are allowed to merge' do - let!(:protected_branch) do - create(:protected_branch, - :developers_can_merge, - project: project, - name: ref) - end - - it { is_expected.to be_truthy } - end - end - - context 'when the tag is protected' do - let(:ref) { 'v1.0.0' } - - let!(:protected_tag) do - create(:protected_tag, project: project, name: ref) - end - - it { is_expected.to be_falsey } - - context 'when developers are allowed to create the tag' do - let!(:protected_tag) do - create(:protected_tag, - :developers_can_create, - project: project, - name: ref) - end - - it { is_expected.to be_truthy } - end - end - end - - context 'when user is a master' do - before do - project.add_master(user) - end - - it { is_expected.to be_truthy } - - context 'when the branch is protected' do - let!(:protected_branch) do - create(:protected_branch, project: project, name: ref) - end - - it { is_expected.to be_truthy } - end - - context 'when the tag is protected' do - let(:ref) { 'v1.0.0' } - - let!(:protected_tag) do - create(:protected_tag, project: project, name: ref) - end - - it { is_expected.to be_truthy } - - context 'when no one can create the tag' do - let!(:protected_tag) do - create(:protected_tag, - :no_one_can_create, - project: project, - name: ref) - end - - it { is_expected.to be_falsey } - end - end - end - - context 'when owner cannot create pipeline' do - it { is_expected.to be_falsey } - end - end -end -- cgit v1.2.1