From 609fa45f0ed273414eac2798f76daf088e287b30 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 26 Sep 2017 11:50:47 +0200 Subject: [PATCH] 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 ------------------ .../ci/pipeline/chain/validate_abilities.rb | 52 +++++++++ .../ci/pipeline/chain/validate_config.rb | 33 ++++++ .../ci/pipeline/chain/validate_repository.rb | 30 ++++++ ...ate_spec.rb => validate_abilities_spec.rb} | 2 +- 8 files changed, 147 insertions(+), 104 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 rename spec/lib/gitlab/ci/pipeline/chain/{validate_spec.rb => validate_abilities_spec.rb} (97%) 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_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate_abilities_spec.rb similarity index 97% rename from spec/lib/gitlab/ci/pipeline/chain/validate_spec.rb rename to spec/lib/gitlab/ci/pipeline/chain/validate_abilities_spec.rb index 117f55fd8a7..1613df395d1 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/validate_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/validate_abilities_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Gitlab::Ci::Pipeline::Chain::Validate do +describe Gitlab::Ci::Pipeline::Chain::ValidateAbilities do describe '#allowed_to_create?' do let(:user) { create(:user) } let(:project) { create(:project, :repository) } -- GitLab