diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index b646b32fc64476fc43d79c2b2b0c9ec2fa50e0aa..e5b615a7cc0709ad734737fe53a9cfca56851782 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -21,7 +21,7 @@ module Ci has_many :merge_requests, foreign_key: "head_pipeline_id" has_many :pending_builds, -> { pending }, foreign_key: :commit_id, class_name: 'Ci::Build' - has_many :retryable_builds, -> { latest.failed_or_canceled }, foreign_key: :commit_id, class_name: 'Ci::Build' + has_many :retryable_builds, -> { latest.failed_or_canceled.includes(:project) }, foreign_key: :commit_id, class_name: 'Ci::Build' has_many :cancelable_statuses, -> { cancelable }, foreign_key: :commit_id, class_name: 'CommitStatus' has_many :manual_actions, -> { latest.manual_actions.includes(:project) }, foreign_key: :commit_id, class_name: 'Ci::Build' has_many :artifacts, -> { latest.with_artifacts_not_expired.includes(:project) }, foreign_key: :commit_id, class_name: 'Ci::Build' diff --git a/app/models/ci/pipeline_schedule.rb b/app/models/ci/pipeline_schedule.rb index e4ae1b35f66f6e1782b7dde22ec31d3dea5a3ed4..085eeeae1572599de51025017f6a26d0b4be8f8f 100644 --- a/app/models/ci/pipeline_schedule.rb +++ b/app/models/ci/pipeline_schedule.rb @@ -40,10 +40,6 @@ module Ci update_attribute(:active, false) end - def runnable_by_owner? - Ability.allowed?(owner, :create_pipeline, project) - end - def set_next_run_at self.next_run_at = Gitlab::Ci::CronParser.new(cron, cron_timezone).next_time_from(Time.now) end diff --git a/app/models/concerns/protected_ref.rb b/app/models/concerns/protected_ref.rb index 5dd43c362223ecb247ec7dbb6be996b93b125c3c..ef95d6b0f9880a722215217b5dabfe8ada4522b7 100644 --- a/app/models/concerns/protected_ref.rb +++ b/app/models/concerns/protected_ref.rb @@ -31,8 +31,8 @@ module ProtectedRef end end - def protected_ref_accessible_to?(ref, user, action:) - access_levels_for_ref(ref, action: action).any? do |access_level| + def protected_ref_accessible_to?(ref, user, action:, protected_refs: nil) + access_levels_for_ref(ref, action: action, protected_refs: protected_refs).any? do |access_level| access_level.check_access(user) end end @@ -43,8 +43,9 @@ module ProtectedRef end end - def access_levels_for_ref(ref, action:) - self.matching(ref).map(&:"#{action}_access_levels").flatten + def access_levels_for_ref(ref, action:, protected_refs: nil) + self.matching(ref, protected_refs: protected_refs) + .map(&:"#{action}_access_levels").flatten end def matching(ref_name, protected_refs: nil) diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb index 386822d3ff6dd881d2012f3edf5341dc1028ed73..984e5482288c3975bfeb16cdf6e2946d9d6daa02 100644 --- a/app/policies/ci/build_policy.rb +++ b/app/policies/ci/build_policy.rb @@ -1,17 +1,15 @@ module Ci class BuildPolicy < CommitStatusPolicy - condition(:protected_action) do - next false unless @subject.action? - + condition(:protected_ref) do access = ::Gitlab::UserAccess.new(@user, project: @subject.project) if @subject.tag? !access.can_create_tag?(@subject.ref) else - !access.can_merge_to_branch?(@subject.ref) + !access.can_update_branch?(@subject.ref) end end - rule { protected_action }.prevent :update_build + rule { protected_ref }.prevent :update_build end end diff --git a/app/policies/ci/pipeline_policy.rb b/app/policies/ci/pipeline_policy.rb index a2dde95dbc8a1897229383b6e8a82eb8d15fd122..4e689a9efd502cad88bdc7d33bcde26afb922cd5 100644 --- a/app/policies/ci/pipeline_policy.rb +++ b/app/policies/ci/pipeline_policy.rb @@ -1,5 +1,17 @@ module Ci class PipelinePolicy < BasePolicy delegate { @subject.project } + + condition(:protected_ref) do + access = ::Gitlab::UserAccess.new(@user, project: @subject.project) + + if @subject.tag? + !access.can_create_tag?(@subject.ref) + else + !access.can_update_branch?(@subject.ref) + end + end + + rule { protected_ref }.prevent :update_pipeline end end diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index 273386776faa818bb4d9b0eed7688eb08134a8c5..21e2ef153dec230900dca26eb2e33fb897477abf 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -15,12 +15,40 @@ module Ci pipeline_schedule: schedule ) + result = validate(current_user || trigger_request.trigger.owner, + ignore_skip_ci: ignore_skip_ci, + save_on_errors: save_on_errors) + + return result if result + + Ci::Pipeline.transaction do + update_merge_requests_head_pipeline if pipeline.save + + Ci::CreatePipelineStagesService + .new(project, current_user) + .execute(pipeline) + end + + cancel_pending_pipelines if project.auto_cancel_pending_pipelines? + + pipeline_created_counter.increment(source: source) + + pipeline.tap(&:process!) + end + + private + + def validate(triggering_user, ignore_skip_ci:, save_on_errors:) unless project.builds_enabled? return error('Pipeline is disabled') end - unless trigger_request || can?(current_user, :create_pipeline, project) - return error('Insufficient permissions to create a new pipeline') + unless allowed_to_trigger_pipeline?(triggering_user) + if can?(triggering_user, :create_pipeline, project) + return error("Insufficient permissions for protected ref '#{ref}'") + else + return error('Insufficient permissions to create a new pipeline') + end end unless branch? || tag? @@ -46,24 +74,29 @@ module Ci unless pipeline.has_stage_seeds? return error('No stages / jobs for this pipeline.') end + end - Ci::Pipeline.transaction do - update_merge_requests_head_pipeline if pipeline.save - - Ci::CreatePipelineStagesService - .new(project, current_user) - .execute(pipeline) + def allowed_to_trigger_pipeline?(triggering_user) + if triggering_user + allowed_to_create?(triggering_user) + else # legacy triggers don't have a corresponding user + !project.protected_for?(ref) end + end - cancel_pending_pipelines if project.auto_cancel_pending_pipelines? - - pipeline_created_counter.increment(source: source) + def allowed_to_create?(triggering_user) + access = Gitlab::UserAccess.new(triggering_user, project: project) - pipeline.tap(&:process!) + can?(triggering_user, :create_pipeline, project) && + if branch? + access.can_update_branch?(ref) + elsif tag? + access.can_create_tag?(ref) + else + true # Allow it for now and we'll reject when we check ref existence + end end - private - def update_merge_requests_head_pipeline return unless pipeline.latest? @@ -113,15 +146,21 @@ module Ci end def branch? - project.repository.ref_exists?(Gitlab::Git::BRANCH_REF_PREFIX + ref) + return @is_branch if defined?(@is_branch) + + @is_branch = + project.repository.ref_exists?(Gitlab::Git::BRANCH_REF_PREFIX + ref) end def tag? - project.repository.ref_exists?(Gitlab::Git::TAG_REF_PREFIX + ref) + return @is_tag if defined?(@is_tag) + + @is_tag = + project.repository.ref_exists?(Gitlab::Git::TAG_REF_PREFIX + ref) end def ref - Gitlab::Git.ref_name(origin_ref) + @ref ||= Gitlab::Git.ref_name(origin_ref) end def valid_sha? diff --git a/app/services/ci/create_trigger_request_service.rb b/app/services/ci/create_trigger_request_service.rb index cf3d4aee2bcff92681a12a24a6bb1aa8ed8383fe..a43d0e4593c01d2fa1954959a040e5c2346b2dc3 100644 --- a/app/services/ci/create_trigger_request_service.rb +++ b/app/services/ci/create_trigger_request_service.rb @@ -1,12 +1,14 @@ module Ci - class CreateTriggerRequestService - def execute(project, trigger, ref, variables = nil) + module CreateTriggerRequestService + Result = Struct.new(:trigger_request, :pipeline) + + def self.execute(project, trigger, ref, variables = nil) trigger_request = trigger.trigger_requests.create(variables: variables) pipeline = Ci::CreatePipelineService.new(project, trigger.owner, ref: ref) .execute(:trigger, ignore_skip_ci: true, trigger_request: trigger_request) - trigger_request if pipeline.persisted? + Result.new(trigger_request, pipeline) end end end diff --git a/app/workers/pipeline_schedule_worker.rb b/app/workers/pipeline_schedule_worker.rb index 7b485b3363c5d1ffd364352ad6f3bb370a127410..d7087f20dfc3ada944c64f396058a3d3c973b587 100644 --- a/app/workers/pipeline_schedule_worker.rb +++ b/app/workers/pipeline_schedule_worker.rb @@ -6,15 +6,12 @@ class PipelineScheduleWorker Ci::PipelineSchedule.active.where("next_run_at < ?", Time.now) .preload(:owner, :project).find_each do |schedule| begin - unless schedule.runnable_by_owner? - schedule.deactivate! - next - end - - Ci::CreatePipelineService.new(schedule.project, - schedule.owner, - ref: schedule.ref) + pipeline = Ci::CreatePipelineService.new(schedule.project, + schedule.owner, + ref: schedule.ref) .execute(:schedule, save_on_errors: false, schedule: schedule) + + schedule.deactivate! unless pipeline.persisted? rescue => e Rails.logger.error "#{schedule.id}: Failed to create a scheduled pipeline: #{e.message}" ensure diff --git a/changelogs/unreleased/30634-protected-pipeline.yml b/changelogs/unreleased/30634-protected-pipeline.yml new file mode 100644 index 0000000000000000000000000000000000000000..e46538e5b4698f12f71a995b4b444f8bbff03db7 --- /dev/null +++ b/changelogs/unreleased/30634-protected-pipeline.yml @@ -0,0 +1,5 @@ +--- +title: Disallow running the pipeline if ref is protected and user cannot merge the + branch or create the tag +merge_request: 11910 +author: diff --git a/doc/user/project/pipelines/schedules.md b/doc/user/project/pipelines/schedules.md index 258b3a2f9555f771d8746fc5001e5e97a9f1d174..9ad15a12c3c0f1cf8102534f09f4e77ca3c80b68 100644 --- a/doc/user/project/pipelines/schedules.md +++ b/doc/user/project/pipelines/schedules.md @@ -71,9 +71,10 @@ The next time a pipeline is scheduled, your credentials will be used. >**Note:** When the owner of the schedule doesn't have the ability to create pipelines -anymore, due to e.g., being blocked or removed from the project, the schedule -is deactivated. Another user can take ownership and activate it, so the -schedule can be run again. +anymore, due to e.g., being blocked or removed from the project, or lacking +the permission to run on protected branches or tags. When this happened, the +schedule is deactivated. Another user can take ownership and activate it, so +the schedule can be run again. ## Advanced admin configuration diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index cb0619105e1b0bb522043b4dfbcb70399a21e9ce..9375e7eb7685c721182626c2dc3eb41f98058cc8 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -27,12 +27,13 @@ module API end # create request and trigger builds - trigger_request = Ci::CreateTriggerRequestService.new.execute(project, trigger, params[:ref].to_s, variables) - if trigger_request - present trigger_request.pipeline, with: Entities::Pipeline + result = Ci::CreateTriggerRequestService.execute(project, trigger, params[:ref].to_s, variables) + pipeline = result.pipeline + + if pipeline.persisted? + present pipeline, with: Entities::Pipeline else - errors = 'No pipeline created' - render_api_error!(errors, 400) + render_validation_error!(pipeline) end end diff --git a/lib/api/v3/triggers.rb b/lib/api/v3/triggers.rb index a23d6b6b48cb1546c7a6dd696c7cc53a00a03e46..e9d4c35307bbea624de51d1e62368f18511327fe 100644 --- a/lib/api/v3/triggers.rb +++ b/lib/api/v3/triggers.rb @@ -28,12 +28,13 @@ module API end # create request and trigger builds - trigger_request = Ci::CreateTriggerRequestService.new.execute(project, trigger, params[:ref].to_s, variables) - if trigger_request - present trigger_request, with: ::API::V3::Entities::TriggerRequest + result = Ci::CreateTriggerRequestService.execute(project, trigger, params[:ref].to_s, variables) + pipeline = result.pipeline + + if pipeline.persisted? + present result.trigger_request, with: ::API::V3::Entities::TriggerRequest else - errors = 'No builds created' - render_api_error!(errors, 400) + render_validation_error!(pipeline) end end diff --git a/lib/ci/api/triggers.rb b/lib/ci/api/triggers.rb index 6e622601680ff02f772d9e36c454e012d8783560..6225203f223b23d3ab393044536d213c1667c341 100644 --- a/lib/ci/api/triggers.rb +++ b/lib/ci/api/triggers.rb @@ -24,12 +24,13 @@ module Ci end # create request and trigger builds - trigger_request = Ci::CreateTriggerRequestService.new.execute(project, trigger, params[:ref], variables) - if trigger_request - present trigger_request, with: Entities::TriggerRequest + result = Ci::CreateTriggerRequestService.execute(project, trigger, params[:ref], variables) + pipeline = result.pipeline + + if pipeline.persisted? + present result.trigger_request, with: Entities::TriggerRequest else - errors = 'No builds created' - render_api_error!(errors, 400) + render_validation_error!(pipeline) end end end diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index 8e91ee7287c9bbded0f75f31e846acfaa5efc367..d9a5af09f08c76555d4751b27bafcc18bff34aa5 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -37,8 +37,8 @@ module Gitlab request_cache def can_create_tag?(ref) return false unless can_access_git? - if ProtectedTag.protected?(project, ref) - project.protected_tags.protected_ref_accessible_to?(ref, user, action: :create) + if protected?(ProtectedTag, project, ref) + protected_tag_accessible_to?(ref, action: :create) else user.can?(:push_code, project) end @@ -47,20 +47,24 @@ module Gitlab request_cache def can_delete_branch?(ref) return false unless can_access_git? - if ProtectedBranch.protected?(project, ref) + if protected?(ProtectedBranch, project, ref) user.can?(:delete_protected_branch, project) else user.can?(:push_code, project) end end + def can_update_branch?(ref) + can_push_to_branch?(ref) || can_merge_to_branch?(ref) + end + request_cache def can_push_to_branch?(ref) return false unless can_access_git? - if ProtectedBranch.protected?(project, ref) + if protected?(ProtectedBranch, project, ref) return true if project.empty_repo? && project.user_can_push_to_empty_repo?(user) - project.protected_branches.protected_ref_accessible_to?(ref, user, action: :push) + protected_branch_accessible_to?(ref, action: :push) else user.can?(:push_code, project) end @@ -69,8 +73,8 @@ module Gitlab request_cache def can_merge_to_branch?(ref) return false unless can_access_git? - if ProtectedBranch.protected?(project, ref) - project.protected_branches.protected_ref_accessible_to?(ref, user, action: :merge) + if protected?(ProtectedBranch, project, ref) + protected_branch_accessible_to?(ref, action: :merge) else user.can?(:push_code, project) end @@ -87,5 +91,23 @@ module Gitlab def can_access_git? user && user.can?(:access_git) end + + def protected_branch_accessible_to?(ref, action:) + ProtectedBranch.protected_ref_accessible_to?( + ref, user, + action: action, + protected_refs: project.protected_branches) + end + + def protected_tag_accessible_to?(ref, action:) + ProtectedTag.protected_ref_accessible_to?( + ref, user, + action: action, + protected_refs: project.protected_tags) + end + + request_cache def protected?(kind, project, ref) + kind.protected?(project, ref) + end end end diff --git a/spec/controllers/projects/jobs_controller_spec.rb b/spec/controllers/projects/jobs_controller_spec.rb index 472e5fc51a09d1175cf93c830c9e662ae70e271b..5a295ae47a655f601bc1c2cce45942685bcccc01 100644 --- a/spec/controllers/projects/jobs_controller_spec.rb +++ b/spec/controllers/projects/jobs_controller_spec.rb @@ -7,6 +7,10 @@ describe Projects::JobsController do let(:pipeline) { create(:ci_pipeline, project: project) } let(:user) { create(:user) } + before do + stub_not_protect_default_branch + end + describe 'GET index' do context 'when scope is pending' do before do diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb index 734532668d38d9af4a76184733fb23d24a35baae..c8de275ca3e2e800a9e6ada9883a2e1bc9a6c940 100644 --- a/spec/controllers/projects/pipelines_controller_spec.rb +++ b/spec/controllers/projects/pipelines_controller_spec.rb @@ -8,6 +8,7 @@ describe Projects::PipelinesController do let(:feature) { ProjectFeature::DISABLED } before do + stub_not_protect_default_branch project.add_developer(user) project.project_feature.update( builds_access_level: feature) @@ -158,7 +159,7 @@ describe Projects::PipelinesController do context 'when builds are enabled' do let(:feature) { ProjectFeature::ENABLED } - + it 'retries a pipeline without returning any content' do expect(response).to have_http_status(:no_content) expect(build.reload).to be_retried @@ -175,7 +176,7 @@ describe Projects::PipelinesController do describe 'POST cancel.json' do let!(:pipeline) { create(:ci_pipeline, project: project) } let!(:build) { create(:ci_build, :running, pipeline: pipeline) } - + before do post :cancel, namespace_id: project.namespace, project_id: project, @@ -185,7 +186,7 @@ describe Projects::PipelinesController do context 'when builds are enabled' do let(:feature) { ProjectFeature::ENABLED } - + it 'cancels a pipeline without returning any content' do expect(response).to have_http_status(:no_content) expect(pipeline.reload).to be_canceled diff --git a/spec/lib/gitlab/ci/status/build/cancelable_spec.rb b/spec/lib/gitlab/ci/status/build/cancelable_spec.rb index 114d24904908b1d43fece227725a58da62bca5e7..5a7a42d84c04a4e44a4a2c37cd392ac059a3db2f 100644 --- a/spec/lib/gitlab/ci/status/build/cancelable_spec.rb +++ b/spec/lib/gitlab/ci/status/build/cancelable_spec.rb @@ -48,7 +48,9 @@ describe Gitlab::Ci::Status::Build::Cancelable do describe '#has_action?' do context 'when user is allowed to update build' do before do - build.project.team << [user, :developer] + stub_not_protect_default_branch + + build.project.add_developer(user) end it { is_expected.to have_action } diff --git a/spec/lib/gitlab/ci/status/build/factory_spec.rb b/spec/lib/gitlab/ci/status/build/factory_spec.rb index c8a97016f207bae5af27917bb660398259d477aa..8768302eda159790fdcea966fd8bcc37eaba746a 100644 --- a/spec/lib/gitlab/ci/status/build/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/build/factory_spec.rb @@ -7,7 +7,9 @@ describe Gitlab::Ci::Status::Build::Factory do let(:factory) { described_class.new(build, user) } before do - project.team << [user, :developer] + stub_not_protect_default_branch + + project.add_developer(user) end context 'when build is successful' do @@ -225,19 +227,19 @@ describe Gitlab::Ci::Status::Build::Factory do end context 'when user has ability to play action' do - before do - project.add_developer(user) - - create(:protected_branch, :developers_can_merge, - name: build.ref, project: project) - end - it 'fabricates status that has action' do expect(status).to have_action end end context 'when user does not have ability to play action' do + before do + allow(build.project).to receive(:empty_repo?).and_return(false) + + create(:protected_branch, :no_one_can_push, + name: build.ref, project: build.project) + end + it 'fabricates status that has no action' do expect(status).not_to have_action end @@ -262,6 +264,13 @@ describe Gitlab::Ci::Status::Build::Factory do end context 'when user is not allowed to execute manual action' do + before do + allow(build.project).to receive(:empty_repo?).and_return(false) + + create(:protected_branch, :no_one_can_push, + name: build.ref, project: build.project) + end + it 'fabricates status with correct details' do expect(status.text).to eq 'manual' expect(status.group).to eq 'manual' diff --git a/spec/lib/gitlab/ci/status/build/retryable_spec.rb b/spec/lib/gitlab/ci/status/build/retryable_spec.rb index 099d873fc01de089a9c0a0a24355015f17779bd2..21026f2c968d7f4b25eee64e9c49b4a70c7b1e5b 100644 --- a/spec/lib/gitlab/ci/status/build/retryable_spec.rb +++ b/spec/lib/gitlab/ci/status/build/retryable_spec.rb @@ -48,7 +48,9 @@ describe Gitlab::Ci::Status::Build::Retryable do describe '#has_action?' do context 'when user is allowed to update build' do before do - build.project.team << [user, :developer] + stub_not_protect_default_branch + + build.project.add_developer(user) end it { is_expected.to have_action } diff --git a/spec/lib/gitlab/ci/status/build/stop_spec.rb b/spec/lib/gitlab/ci/status/build/stop_spec.rb index 23902f26b1a718db864ba90fbe5a1616e9b67b19..e0425103f4114413c3c1f7e298d7357f1ec5f06f 100644 --- a/spec/lib/gitlab/ci/status/build/stop_spec.rb +++ b/spec/lib/gitlab/ci/status/build/stop_spec.rb @@ -20,7 +20,9 @@ describe Gitlab::Ci::Status::Build::Stop do describe '#has_action?' do context 'when user is allowed to update build' do before do - build.project.team << [user, :developer] + stub_not_protect_default_branch + + build.project.add_developer(user) end it { is_expected.to have_action } diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index ba0696fa2108c53350e6b374ca04b0b97cf0e01f..bbd45f10b1b6d1f292f9f6d18ca864e4864a59f9 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -734,6 +734,8 @@ describe Ci::Pipeline, models: true do context 'on failure and build retry' do before do + stub_not_protect_default_branch + build.drop project.add_developer(user) @@ -999,6 +1001,8 @@ describe Ci::Pipeline, models: true do let(:latest_status) { pipeline.statuses.latest.pluck(:status) } before do + stub_not_protect_default_branch + project.add_developer(user) end diff --git a/spec/policies/ci/build_policy_spec.rb b/spec/policies/ci/build_policy_spec.rb index 9f3212b1a63a505690b0d95bb8a765696c1abc50..e3ea3c960a48e8dda37f362c2379fc61fd70d70d 100644 --- a/spec/policies/ci/build_policy_spec.rb +++ b/spec/policies/ci/build_policy_spec.rb @@ -96,87 +96,57 @@ describe Ci::BuildPolicy, :models do end end - describe 'rules for manual actions' do + describe 'rules for protected ref' do let(:project) { create(:project) } + let(:build) { create(:ci_build, ref: 'some-ref', pipeline: pipeline) } before do project.add_developer(user) end - shared_examples 'protected ref' do - context 'when build is a manual action' do - let(:build) do - create(:ci_build, :manual, ref: 'some-ref', pipeline: pipeline) - end - - it 'does not include ability to update build' do - expect(policy).to be_disallowed :update_build - end + context 'when no one can push or merge to the branch' do + before do + create(:protected_branch, :no_one_can_push, + name: build.ref, project: project) end - context 'when build is not a manual action' do - let(:build) do - create(:ci_build, ref: 'some-ref', pipeline: pipeline) - end - - it 'includes ability to update build' do - expect(policy).to be_allowed :update_build - end + it 'does not include ability to update build' do + expect(policy).to be_disallowed :update_build end end - context 'when build is against a protected branch' do + context 'when developers can push to the branch' do before do - create(:protected_branch, :no_one_can_push, - name: 'some-ref', project: project) + create(:protected_branch, :developers_can_merge, + name: build.ref, project: project) end - it_behaves_like 'protected ref' + it 'includes ability to update build' do + expect(policy).to be_allowed :update_build + end end - context 'when build is against a protected tag' do + context 'when no one can create the tag' do before do create(:protected_tag, :no_one_can_create, - name: 'some-ref', project: project) + name: build.ref, project: project) build.update(tag: true) end - it_behaves_like 'protected ref' + it 'does not include ability to update build' do + expect(policy).to be_disallowed :update_build + end end - context 'when build is against a protected tag but it is not a tag' do + context 'when no one can create the tag but it is not a tag' do before do create(:protected_tag, :no_one_can_create, - name: 'some-ref', project: project) + name: build.ref, project: project) end - context 'when build is a manual action' do - let(:build) do - create(:ci_build, :manual, ref: 'some-ref', pipeline: pipeline) - end - - it 'includes ability to update build' do - expect(policy).to be_allowed :update_build - end - end - end - - context 'when branch build is assigned to is not protected' do - context 'when build is a manual action' do - let(:build) { create(:ci_build, :manual, pipeline: pipeline) } - - it 'includes ability to update build' do - expect(policy).to be_allowed :update_build - end - end - - context 'when build is not a manual action' do - let(:build) { create(:ci_build, pipeline: pipeline) } - - it 'includes ability to update build' do - expect(policy).to be_allowed :update_build - end + it 'includes ability to update build' do + expect(policy).to be_allowed :update_build end end end diff --git a/spec/policies/ci/pipeline_policy_spec.rb b/spec/policies/ci/pipeline_policy_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..b11b06d301f1a26d39dcd2b5f4e4b6e53f6b9479 --- /dev/null +++ b/spec/policies/ci/pipeline_policy_spec.rb @@ -0,0 +1,66 @@ +require 'spec_helper' + +describe Ci::PipelinePolicy, :models do + let(:user) { create(:user) } + let(:pipeline) { create(:ci_empty_pipeline, project: project) } + + let(:policy) do + described_class.new(user, pipeline) + end + + describe 'rules' do + describe 'rules for protected ref' do + let(:project) { create(:project) } + + before do + project.add_developer(user) + end + + context 'when no one can push or merge to the branch' do + before do + create(:protected_branch, :no_one_can_push, + name: pipeline.ref, project: project) + end + + it 'does not include ability to update pipeline' do + expect(policy).to be_disallowed :update_pipeline + end + end + + context 'when developers can push to the branch' do + before do + create(:protected_branch, :developers_can_merge, + name: pipeline.ref, project: project) + end + + it 'includes ability to update pipeline' do + expect(policy).to be_allowed :update_pipeline + end + end + + context 'when no one can create the tag' do + before do + create(:protected_tag, :no_one_can_create, + name: pipeline.ref, project: project) + + pipeline.update(tag: true) + end + + it 'does not include ability to update pipeline' do + expect(policy).to be_disallowed :update_pipeline + end + end + + context 'when no one can create the tag but it is not a tag' do + before do + create(:protected_tag, :no_one_can_create, + name: pipeline.ref, project: project) + end + + it 'includes ability to update pipeline' do + expect(policy).to be_allowed :update_pipeline + end + end + end + end +end diff --git a/spec/requests/api/triggers_spec.rb b/spec/requests/api/triggers_spec.rb index 16ddade27d99026dd4ef83e2bf31dc7e2f219289..c2636b6614e899598e3fe443fe09adbcceb4a255 100644 --- a/spec/requests/api/triggers_spec.rb +++ b/spec/requests/api/triggers_spec.rb @@ -61,7 +61,8 @@ describe API::Triggers do post api("/projects/#{project.id}/trigger/pipeline"), options.merge(ref: 'other-branch') expect(response).to have_http_status(400) - expect(json_response['message']).to eq('No pipeline created') + expect(json_response['message']['base']) + .to contain_exactly('Reference not found') end context 'Validates variables' do diff --git a/spec/requests/api/v3/triggers_spec.rb b/spec/requests/api/v3/triggers_spec.rb index d3de6bf13bc79da463dc5eeaed274eb99af4693e..60212660fb60ced1100282535f1095fd27ed3cbb 100644 --- a/spec/requests/api/v3/triggers_spec.rb +++ b/spec/requests/api/v3/triggers_spec.rb @@ -52,7 +52,8 @@ describe API::V3::Triggers do it 'returns bad request with no builds created if there\'s no commit for that ref' do post v3_api("/projects/#{project.id}/trigger/builds"), options.merge(ref: 'other-branch') expect(response).to have_http_status(400) - expect(json_response['message']).to eq('No builds created') + expect(json_response['message']['base']) + .to contain_exactly('Reference not found') end context 'Validates variables' do diff --git a/spec/requests/ci/api/triggers_spec.rb b/spec/requests/ci/api/triggers_spec.rb index 26b03c0f14820ed5cff4cd5697f1cd625edbe629..e481ca916aba87b076aa916150a1286c279579ef 100644 --- a/spec/requests/ci/api/triggers_spec.rb +++ b/spec/requests/ci/api/triggers_spec.rb @@ -5,7 +5,14 @@ describe Ci::API::Triggers do let!(:trigger_token) { 'secure token' } let!(:project) { create(:project, :repository, ci_id: 10) } let!(:project2) { create(:empty_project, ci_id: 11) } - let!(:trigger) { create(:ci_trigger, project: project, token: trigger_token) } + + let!(:trigger) do + create(:ci_trigger, + project: project, + token: trigger_token, + owner: create(:user)) + end + let(:options) do { token: trigger_token @@ -14,6 +21,8 @@ describe Ci::API::Triggers do before do stub_ci_pipeline_to_return_yaml_file + + project.add_developer(trigger.owner) end context 'Handles errors' do @@ -47,7 +56,8 @@ describe Ci::API::Triggers do it 'returns bad request with no builds created if there\'s no commit for that ref' do post ci_api("/projects/#{project.ci_id}/refs/other-branch/trigger"), options expect(response).to have_http_status(400) - expect(json_response['message']).to eq('No builds created') + expect(json_response['message']['base']) + .to contain_exactly('Reference not found') end context 'Validates variables' do diff --git a/spec/serializers/job_entity_spec.rb b/spec/serializers/job_entity_spec.rb index 5ca7bf2fcaf14ba99afe952ba01bcceed05c8e7c..026360e91a342589695f64eca522cf35c56fd153 100644 --- a/spec/serializers/job_entity_spec.rb +++ b/spec/serializers/job_entity_spec.rb @@ -7,7 +7,9 @@ describe JobEntity do let(:request) { double('request') } before do + stub_not_protect_default_branch allow(request).to receive(:current_user).and_return(user) + project.add_developer(user) end @@ -77,7 +79,7 @@ describe JobEntity do project.add_developer(user) create(:protected_branch, :developers_can_merge, - name: 'master', project: project) + name: job.ref, project: job.project) end it 'contains path to play action' do @@ -90,6 +92,13 @@ describe JobEntity do end context 'when user is not allowed to trigger action' do + before do + allow(job.project).to receive(:empty_repo?).and_return(false) + + create(:protected_branch, :no_one_can_push, + name: job.ref, project: job.project) + end + it 'does not contain path to play action' do expect(subject).not_to include(:play_path) end diff --git a/spec/serializers/pipeline_details_entity_spec.rb b/spec/serializers/pipeline_details_entity_spec.rb index d28dec9592ac470a24bae21d3ef21bd180612d50..b990370a2712f3883b5a58d255c71daa71e647f5 100644 --- a/spec/serializers/pipeline_details_entity_spec.rb +++ b/spec/serializers/pipeline_details_entity_spec.rb @@ -9,6 +9,8 @@ describe PipelineDetailsEntity do end before do + stub_not_protect_default_branch + allow(request).to receive(:current_user).and_return(user) end @@ -52,7 +54,7 @@ describe PipelineDetailsEntity do context 'user has ability to retry pipeline' do before do - project.team << [user, :developer] + project.add_developer(user) end it 'retryable flag is true' do @@ -97,7 +99,7 @@ describe PipelineDetailsEntity do context 'when pipeline has commit statuses' do let(:pipeline) { create(:ci_empty_pipeline) } - + before do create(:generic_commit_status, pipeline: pipeline) end diff --git a/spec/serializers/pipeline_entity_spec.rb b/spec/serializers/pipeline_entity_spec.rb index 46650f3a80d2eb2e8db5d9b95a5f5f7eb8962454..5b01cc4fc9eb8970a167a270ab012f8775502c52 100644 --- a/spec/serializers/pipeline_entity_spec.rb +++ b/spec/serializers/pipeline_entity_spec.rb @@ -5,6 +5,8 @@ describe PipelineEntity do let(:request) { double('request') } before do + stub_not_protect_default_branch + allow(request).to receive(:current_user).and_return(user) end @@ -52,7 +54,7 @@ describe PipelineEntity do context 'user has ability to retry pipeline' do before do - project.team << [user, :developer] + project.add_developer(user) end it 'contains retry path' do diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb index 44813656aff15d2e5ffb923bac77facdf4e5edd2..262bc4acb69d2dc32d75258b4ec9eaaa64791664 100644 --- a/spec/serializers/pipeline_serializer_spec.rb +++ b/spec/serializers/pipeline_serializer_spec.rb @@ -108,14 +108,35 @@ describe PipelineSerializer do end end - it 'verifies number of queries', :request_store do - recorded = ActiveRecord::QueryRecorder.new { subject } - expect(recorded.count).to be_within(1).of(57) - expect(recorded.cached_count).to eq(0) + shared_examples 'no N+1 queries' do + it 'verifies number of queries', :request_store do + recorded = ActiveRecord::QueryRecorder.new { subject } + expect(recorded.count).to be_within(1).of(59) + expect(recorded.cached_count).to eq(0) + end + end + + context 'with the same ref' do + let(:ref) { 'feature' } + + it_behaves_like 'no N+1 queries' + end + + context 'with different refs' do + def ref + @sequence ||= 0 + @sequence += 1 + "feature-#{@sequence}" + end + + it_behaves_like 'no N+1 queries' end def create_pipeline(status) - create(:ci_empty_pipeline, project: project, status: status).tap do |pipeline| + create(:ci_empty_pipeline, + project: project, + status: status, + ref: ref).tap do |pipeline| Ci::Build::AVAILABLE_STATUSES.each do |status| create_build(pipeline, status, status) end @@ -125,7 +146,7 @@ describe PipelineSerializer do def create_build(pipeline, stage, status) create(:ci_build, :tags, :triggered, :artifacts, pipeline: pipeline, stage: stage, - name: stage, status: status) + name: stage, status: status, ref: pipeline.ref) end end end diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index ba07c01d43f1eff6d08d8b539f206031c0f96cb8..146d25daba3e9897ddf78966b466329d128bef2d 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -3,19 +3,26 @@ require 'spec_helper' describe Ci::CreatePipelineService, :services do let(:project) { create(:project, :repository) } let(:user) { create(:admin) } + let(:ref_name) { 'refs/heads/master' } before do stub_ci_pipeline_to_return_yaml_file end describe '#execute' do - def execute_service(source: :push, after: project.commit.id, message: 'Message', ref: 'refs/heads/master') + def execute_service( + source: :push, + after: project.commit.id, + message: 'Message', + ref: ref_name, + trigger_request: nil) params = { ref: ref, before: '00000000', after: after, commits: [{ message: message }] } - described_class.new(project, user, params).execute(source) + described_class.new(project, user, params).execute( + source, trigger_request: trigger_request) end context 'valid params' do @@ -334,5 +341,209 @@ describe Ci::CreatePipelineService, :services do expect(pipeline.builds.find_by(name: 'rspec').retries_max).to eq 2 end end + + shared_examples 'when ref is protected' do + let(:user) { create(:user) } + + context 'when user is developer' do + before do + project.add_developer(user) + end + + it 'does not create a pipeline' do + expect(execute_service).not_to be_persisted + expect(Ci::Pipeline.count).to eq(0) + end + end + + context 'when user is master' do + before do + project.add_master(user) + end + + it 'creates a pipeline' do + expect(execute_service).to be_persisted + expect(Ci::Pipeline.count).to eq(1) + end + end + + context 'when trigger belongs to no one' do + let(:user) {} + let(:trigger_request) { create(:ci_trigger_request) } + + it 'does not create a pipeline' do + expect(execute_service(trigger_request: trigger_request)) + .not_to be_persisted + expect(Ci::Pipeline.count).to eq(0) + end + end + + context 'when trigger belongs to a developer' do + let(:user) {} + + let(:trigger_request) do + create(:ci_trigger_request).tap do |request| + user = create(:user) + project.add_developer(user) + request.trigger.update(owner: user) + end + end + + it 'does not create a pipeline' do + expect(execute_service(trigger_request: trigger_request)) + .not_to be_persisted + expect(Ci::Pipeline.count).to eq(0) + end + end + + context 'when trigger belongs to a master' do + let(:user) {} + + let(:trigger_request) do + create(:ci_trigger_request).tap do |request| + user = create(:user) + project.add_master(user) + request.trigger.update(owner: user) + end + end + + it 'does not create a pipeline' do + expect(execute_service(trigger_request: trigger_request)) + .to be_persisted + expect(Ci::Pipeline.count).to eq(1) + end + end + end + + context 'when ref is a protected branch' do + before do + create(:protected_branch, project: project, name: 'master') + end + + it_behaves_like 'when ref is protected' + end + + context 'when ref is a protected tag' do + let(:ref_name) { 'refs/tags/v1.0.0' } + + before do + create(:protected_tag, project: project, name: '*') + end + + it_behaves_like 'when ref is protected' + end + + context 'when ref is not protected' do + context 'when trigger belongs to no one' do + let(:user) {} + let(:trigger_request) { create(:ci_trigger_request) } + + it 'creates a pipeline' do + expect(execute_service(trigger_request: trigger_request)) + .to be_persisted + expect(Ci::Pipeline.count).to eq(1) + end + end + end + end + + describe '#allowed_to_create?' do + let(:user) { create(:user) } + let(:project) { create(:project, :repository) } + let(:ref) { 'master' } + + subject do + described_class.new(project, user, ref: ref) + .send(:allowed_to_create?, user) + 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/services/ci/create_trigger_request_service_spec.rb b/spec/services/ci/create_trigger_request_service_spec.rb index f2956262f4b0d3fc8ceef2b99ecef1eb37340d59..37ca9804f568fa9eff3bd0a488bd94a825d75f98 100644 --- a/spec/services/ci/create_trigger_request_service_spec.rb +++ b/spec/services/ci/create_trigger_request_service_spec.rb @@ -1,12 +1,15 @@ require 'spec_helper' describe Ci::CreateTriggerRequestService, services: true do - let(:service) { described_class.new } + let(:service) { described_class } let(:project) { create(:project, :repository) } - let(:trigger) { create(:ci_trigger, project: project) } + let(:trigger) { create(:ci_trigger, project: project, owner: owner) } + let(:owner) { create(:user) } before do stub_ci_pipeline_to_return_yaml_file + + project.add_developer(owner) end describe '#execute' do @@ -14,29 +17,26 @@ describe Ci::CreateTriggerRequestService, services: true do subject { service.execute(project, trigger, 'master') } context 'without owner' do - it { expect(subject).to be_kind_of(Ci::TriggerRequest) } + it { expect(subject.trigger_request).to be_kind_of(Ci::TriggerRequest) } + it { expect(subject.trigger_request.builds.first).to be_kind_of(Ci::Build) } it { expect(subject.pipeline).to be_kind_of(Ci::Pipeline) } it { expect(subject.pipeline).to be_trigger } - it { expect(subject.builds.first).to be_kind_of(Ci::Build) } end context 'with owner' do - let(:owner) { create(:user) } - let(:trigger) { create(:ci_trigger, project: project, owner: owner) } - - it { expect(subject).to be_kind_of(Ci::TriggerRequest) } + it { expect(subject.trigger_request).to be_kind_of(Ci::TriggerRequest) } + it { expect(subject.trigger_request.builds.first).to be_kind_of(Ci::Build) } + it { expect(subject.trigger_request.builds.first.user).to eq(owner) } it { expect(subject.pipeline).to be_kind_of(Ci::Pipeline) } it { expect(subject.pipeline).to be_trigger } it { expect(subject.pipeline.user).to eq(owner) } - it { expect(subject.builds.first).to be_kind_of(Ci::Build) } - it { expect(subject.builds.first.user).to eq(owner) } end end context 'no commit for ref' do subject { service.execute(project, trigger, 'other-branch') } - it { expect(subject).to be_nil } + it { expect(subject.pipeline).not_to be_persisted } end context 'no builds created' do @@ -46,7 +46,7 @@ describe Ci::CreateTriggerRequestService, services: true do stub_ci_pipeline_yaml_file('script: { only: [develop], script: hello World }') end - it { expect(subject).to be_nil } + it { expect(subject.pipeline).not_to be_persisted } end end end diff --git a/spec/services/ci/process_pipeline_service_spec.rb b/spec/services/ci/process_pipeline_service_spec.rb index 0934833a4fa3561f3d7cd0f470da3ef3cc1da73e..6346f3116962a1ea498d9c979c237601dc5de272 100644 --- a/spec/services/ci/process_pipeline_service_spec.rb +++ b/spec/services/ci/process_pipeline_service_spec.rb @@ -9,6 +9,8 @@ describe Ci::ProcessPipelineService, '#execute', :services do end before do + stub_not_protect_default_branch + project.add_developer(user) end diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index ef9927c59693041075d5986603a3c5b6e4cdb2f7..2cf62b54666b00ac72953348056bf7cf8596db21 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -85,6 +85,8 @@ describe Ci::RetryBuildService, :services do context 'when user has ability to execute build' do before do + stub_not_protect_default_branch + project.add_developer(user) end @@ -131,6 +133,8 @@ describe Ci::RetryBuildService, :services do context 'when user has ability to execute build' do before do + stub_not_protect_default_branch + project.add_developer(user) end diff --git a/spec/services/ci/retry_pipeline_service_spec.rb b/spec/services/ci/retry_pipeline_service_spec.rb index 3e860203063cecbe794ff83b86d5cbb26330eb04..7798db3f3b987b4d6ce206d337312ff3cd7d945c 100644 --- a/spec/services/ci/retry_pipeline_service_spec.rb +++ b/spec/services/ci/retry_pipeline_service_spec.rb @@ -244,13 +244,9 @@ describe Ci::RetryPipelineService, '#execute', :services do create_build('verify', :canceled, 1) end - it 'does not reprocess manual action' do - service.execute(pipeline) - - expect(build('test')).to be_pending - expect(build('deploy')).to be_failed - expect(build('verify')).to be_created - expect(pipeline.reload).to be_running + it 'raises an error' do + expect { service.execute(pipeline) } + .to raise_error Gitlab::Access::AccessDeniedError end end @@ -261,13 +257,9 @@ describe Ci::RetryPipelineService, '#execute', :services do create_build('verify', :canceled, 2) end - it 'does not reprocess manual action' do - service.execute(pipeline) - - expect(build('test')).to be_pending - expect(build('deploy')).to be_failed - expect(build('verify')).to be_created - expect(pipeline.reload).to be_running + it 'raises an error' do + expect { service.execute(pipeline) } + .to raise_error Gitlab::Access::AccessDeniedError end end end diff --git a/spec/services/create_deployment_service_spec.rb b/spec/services/create_deployment_service_spec.rb index dfab6ebf372a8a9d86c4dd8825ebba07f0983f48..2794721e1575faa25c20dc46c362e09532e191a1 100644 --- a/spec/services/create_deployment_service_spec.rb +++ b/spec/services/create_deployment_service_spec.rb @@ -244,6 +244,8 @@ describe CreateDeploymentService, services: true do context 'when job is retried' do it_behaves_like 'creates deployment' do before do + stub_not_protect_default_branch + project.add_developer(user) end diff --git a/spec/support/stub_configuration.rb b/spec/support/stub_configuration.rb index 48f454c71876b00eb9c9be967d877b383f163717..80ecce92dc1111c66e810915e9de5ef0e3de276d 100644 --- a/spec/support/stub_configuration.rb +++ b/spec/support/stub_configuration.rb @@ -9,6 +9,11 @@ module StubConfiguration .to receive_messages(messages) end + def stub_not_protect_default_branch + stub_application_setting( + default_branch_protection: Gitlab::Access::PROTECTION_NONE) + end + def stub_config_setting(messages) allow(Gitlab.config.gitlab).to receive_messages(messages) end diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb index a8f4bb72acf50d4a779654a729bb89fdb7ed0140..74a9f90195ce78d5def9c3ac36f72f2ae5ded0aa 100644 --- a/spec/workers/post_receive_spec.rb +++ b/spec/workers/post_receive_spec.rb @@ -74,6 +74,7 @@ describe PostReceive do OpenStruct.new(id: '123456') end allow_any_instance_of(Ci::CreatePipelineService).to receive(:branch?).and_return(true) + allow_any_instance_of(Repository).to receive(:ref_exists?).and_return(true) stub_ci_pipeline_to_return_yaml_file end