diff options
20 files changed, 106 insertions, 45 deletions
diff --git a/app/policies/base_policy.rb b/app/policies/base_policy.rb index 5dd2279ef99..82bf9bf8bf6 100644 --- a/app/policies/base_policy.rb +++ b/app/policies/base_policy.rb @@ -7,6 +7,10 @@ class BasePolicy < DeclarativePolicy::Base with_options scope: :user, score: 0 condition(:admin) { @user&.admin? } + desc "User is blocked" + with_options scope: :user, score: 0 + condition(:blocked) { @user&.blocked? } + desc "User has access to all private groups & projects" with_options scope: :user, score: 0 condition(:full_private_access) { @user&.full_private_access? } diff --git a/app/policies/global_policy.rb b/app/policies/global_policy.rb index e85397422e6..134de1c9ace 100644 --- a/app/policies/global_policy.rb +++ b/app/policies/global_policy.rb @@ -1,10 +1,6 @@ # frozen_string_literal: true class GlobalPolicy < BasePolicy - desc "User is blocked" - with_options scope: :user, score: 0 - condition(:blocked) { @user&.blocked? } - desc "User is an internal user" with_options scope: :user, score: 0 condition(:internal) { @user&.internal? } diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index a3632640ede..b3e29e775fc 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -447,6 +447,10 @@ class ProjectPolicy < BasePolicy prevent :owner_access end + rule { blocked }.policy do + prevent :create_pipeline + end + private def team_member? diff --git a/changelogs/unreleased/error-pipelines-for-blocked-users.yml b/changelogs/unreleased/error-pipelines-for-blocked-users.yml new file mode 100644 index 00000000000..3ace28b6cfd --- /dev/null +++ b/changelogs/unreleased/error-pipelines-for-blocked-users.yml @@ -0,0 +1,5 @@ +--- +title: preventing blocked users and their PipelineSchdules from creating new Pipelines +merge_request: 27318 +author: +type: fixed diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 1d2b724a5e5..4f3392cdcbf 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -66,6 +66,16 @@ FactoryBot.define do end end + transient do + developer_projects [] + end + + after(:create) do |user, evaluator| + evaluator.developer_projects.each do |project| + project.add_developer(user) + end + end + factory :omniauth_user do transient do extern_uid '123456' diff --git a/spec/features/commits_spec.rb b/spec/features/commits_spec.rb index e6f44aa7d20..a2dd34e7f7c 100644 --- a/spec/features/commits_spec.rb +++ b/spec/features/commits_spec.rb @@ -10,8 +10,7 @@ describe 'Commits' do stub_ci_pipeline_to_return_yaml_file end - let(:creator) { create(:user) } - + let(:creator) { create(:user, developer_projects: [project]) } let!(:pipeline) do create(:ci_pipeline, project: project, @@ -77,10 +76,11 @@ describe 'Commits' do describe 'Commit builds', :js do before do + project.add_developer(user) visit pipeline_path(pipeline) end - it 'shows pipeline`s data' do + it 'shows pipeline data' do expect(page).to have_content pipeline.sha[0..7] expect(page).to have_content pipeline.git_commit_message.gsub!(/\s+/, ' ') expect(page).to have_content pipeline.user.name diff --git a/spec/features/projects/pipelines/pipeline_spec.rb b/spec/features/projects/pipelines/pipeline_spec.rb index 77f0f237d0a..9759fd04ad2 100644 --- a/spec/features/projects/pipelines/pipeline_spec.rb +++ b/spec/features/projects/pipelines/pipeline_spec.rb @@ -5,6 +5,7 @@ require 'spec_helper' describe 'Pipeline', :js do include RoutesHelpers include ProjectForksHelper + include ::ExclusiveLeaseHelpers let(:project) { create(:project) } let(:user) { create(:user) } @@ -539,6 +540,44 @@ describe 'Pipeline', :js do expect(page).to have_selector('.pipeline-visualization') expect(page).to have_content('cross-build') end + + context 'when a scheduled pipeline is created by a blocked user' do + let(:project) { create(:project, :repository) } + + let(:schedule) do + create(:ci_pipeline_schedule, + project: project, + owner: project.owner, + description: 'blocked user schedule' + ).tap do |schedule| + schedule.update_column(:next_run_at, 1.minute.ago) + end + end + + before do + schedule.owner.block! + + begin + PipelineScheduleWorker.new.perform + rescue Ci::CreatePipelineService::CreateError + # Do nothing, assert view code after the Pipeline failed to create. + end + end + + it 'displays the PipelineSchedule in an active state' do + visit project_pipeline_schedules_path(project) + page.click_link('Active') + + expect(page).to have_selector('table.ci-table > tbody > tr > td', text: 'blocked user schedule') + end + + it 'does not create a new Pipeline' do + visit project_pipelines_path(project) + + expect(page).not_to have_selector('.ci-table') + expect(schedule.last_pipeline).to be_nil + end + end end describe 'GET /:project/pipelines/:id/builds' do diff --git a/spec/features/projects/pipelines/pipelines_spec.rb b/spec/features/projects/pipelines/pipelines_spec.rb index de780f13681..885d5f85989 100644 --- a/spec/features/projects/pipelines/pipelines_spec.rb +++ b/spec/features/projects/pipelines/pipelines_spec.rb @@ -469,7 +469,7 @@ describe 'Pipelines', :js do visit_project_pipelines end - it 'has artifats' do + it 'has artifacts' do expect(page).to have_selector('.build-artifacts') end diff --git a/spec/finders/pipelines_finder_spec.rb b/spec/finders/pipelines_finder_spec.rb index c2c304589c9..b23fd8ccdc6 100644 --- a/spec/finders/pipelines_finder_spec.rb +++ b/spec/finders/pipelines_finder_spec.rb @@ -170,8 +170,9 @@ describe PipelinesFinder do context 'when order_by and sort are specified' do context 'when order_by user_id' do - let(:params) { { order_by: 'user_id', sort: 'asc' } } - let!(:pipelines) { Array.new(2) { create(:ci_pipeline, project: project, user: create(:user)) } } + let(:params) { { order_by: 'user_id', sort: 'asc' } } + let(:users) { Array.new(2) { create(:user, developer_projects: [project]) } } + let!(:pipelines) { users.map { |user| create(:ci_pipeline, project: project, user: user) } } it 'sorts as user_id: :asc' do is_expected.to match_array(pipelines) diff --git a/spec/javascripts/fixtures/pipelines.rb b/spec/javascripts/fixtures/pipelines.rb index de6fcfe10f4..6b6b0eefab9 100644 --- a/spec/javascripts/fixtures/pipelines.rb +++ b/spec/javascripts/fixtures/pipelines.rb @@ -8,7 +8,7 @@ describe Projects::PipelinesController, '(JavaScript fixtures)', type: :controll let(:project) { create(:project, :repository, namespace: namespace, path: 'pipelines-project') } let(:commit) { create(:commit, project: project) } let(:commit_without_author) { RepoHelpers.another_sample_commit } - let!(:user) { create(:user, email: commit.author_email) } + let!(:user) { create(:user, developer_projects: [project], email: commit.author_email) } let!(:pipeline) { create(:ci_pipeline, project: project, sha: commit.id, user: user) } let!(:pipeline_without_author) { create(:ci_pipeline, project: project, sha: commit_without_author.id) } let!(:pipeline_without_commit) { create(:ci_pipeline, project: project, sha: '0000') } diff --git a/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb index 3debd42ac65..50cb45c39d1 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe Gitlab::Ci::Pipeline::Chain::Build do set(:project) { create(:project, :repository) } - set(:user) { create(:user) } + set(:user) { create(:user, developer_projects: [project]) } let(:pipeline) { Ci::Pipeline.new } let(:variables_attributes) do diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index a8701f0efa4..c4e54be673f 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -2736,7 +2736,7 @@ describe Ci::Pipeline, :mailer do create(:ci_pipeline, project: project, sha: project.commit('master').sha, - user: create(:user)) + user: project.owner) end before do diff --git a/spec/models/project_services/hipchat_service_spec.rb b/spec/models/project_services/hipchat_service_spec.rb index a04b984c1f6..a1bd0855708 100644 --- a/spec/models/project_services/hipchat_service_spec.rb +++ b/spec/models/project_services/hipchat_service_spec.rb @@ -301,7 +301,7 @@ describe HipchatService do end context 'pipeline events' do - let(:pipeline) { create(:ci_empty_pipeline, user: create(:user)) } + let(:pipeline) { create(:ci_empty_pipeline, user: project.owner) } let(:data) { Gitlab::DataBuilder::Pipeline.build(pipeline) } context 'for failed' do diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 867692d4d64..d9b61dfe503 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -1132,5 +1132,17 @@ describe Ci::CreatePipelineService do .with_message('Insufficient permissions to create a new pipeline') end end + + context 'when a user with permissions has been blocked' do + before do + user.block! + end + + it 'raises an error' do + expect { subject } + .to raise_error(described_class::CreateError) + .with_message('Insufficient permissions to create a new pipeline') + end + end end end diff --git a/spec/services/ci/play_build_service_spec.rb b/spec/services/ci/play_build_service_spec.rb index 634f865e2d8..1e68b7956ea 100644 --- a/spec/services/ci/play_build_service_spec.rb +++ b/spec/services/ci/play_build_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Ci::PlayBuildService, '#execute' do - let(:user) { create(:user) } + let(:user) { create(:user, developer_projects: [project]) } let(:project) { create(:project) } let(:pipeline) { create(:ci_pipeline, project: project) } let(:build) { create(:ci_build, :manual, pipeline: pipeline) } @@ -16,8 +16,6 @@ describe Ci::PlayBuildService, '#execute' do let(:project) { create(:project) } it 'allows user to play build if protected branch rules are met' do - project.add_developer(user) - create(:protected_branch, :developers_can_merge, name: build.ref, project: project) @@ -27,8 +25,6 @@ describe Ci::PlayBuildService, '#execute' do end it 'does not allow user with developer role to play build' do - project.add_developer(user) - expect { service.execute(build) } .to raise_error Gitlab::Access::AccessDeniedError end @@ -38,23 +34,21 @@ describe Ci::PlayBuildService, '#execute' do let(:project) { create(:project, :repository) } it 'allows user with developer role to play a build' do - project.add_developer(user) - service.execute(build) expect(build.reload).to be_pending end + + it 'prevents a blocked developer from playing a build' do + user.block! + + expect { service.execute(build) }.to raise_error(Gitlab::Access::AccessDeniedError) + end end context 'when build is a playable manual action' do let(:build) { create(:ci_build, :manual, pipeline: pipeline) } - - before do - project.add_developer(user) - - create(:protected_branch, :developers_can_merge, - name: build.ref, project: project) - end + let!(:branch) { create(:protected_branch, :developers_can_merge, name: build.ref, project: project) } it 'enqueues the build' do expect(service.execute(build)).to eq build @@ -70,13 +64,7 @@ describe Ci::PlayBuildService, '#execute' do context 'when build is not a playable manual action' do let(:build) { create(:ci_build, when: :manual, pipeline: pipeline) } - - before do - project.add_developer(user) - - create(:protected_branch, :developers_can_merge, - name: build.ref, project: project) - end + let!(:branch) { create(:protected_branch, :developers_can_merge, name: build.ref, project: project) } it 'duplicates the build' do duplicate = service.execute(build) @@ -94,6 +82,7 @@ describe Ci::PlayBuildService, '#execute' do end context 'when build is not action' do + let(:user) { create(:user) } let(:build) { create(:ci_build, :success, pipeline: pipeline) } it 'raises an error' do @@ -103,10 +92,8 @@ describe Ci::PlayBuildService, '#execute' do end context 'when user does not have ability to trigger action' do - before do - create(:protected_branch, :no_one_can_push, - name: build.ref, project: project) - end + let(:user) { create(:user) } + let!(:branch) { create(:protected_branch, :developers_can_merge, name: build.ref, project: project) } it 'raises an error' do expect { service.execute(build) } diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 4b40c86574f..f25e2fe5e2b 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -2217,10 +2217,12 @@ describe NotificationService, :mailer do let(:pipeline) { create(:ci_pipeline, :failed, project: project, user: pipeline_user) } it 'emails project owner and user that triggered the pipeline' do + project.add_developer(pipeline_user) + notification.autodevops_disabled(pipeline, [owner.email, pipeline_user.email]) - should_email(owner) - should_email(pipeline_user) + should_email(owner, times: 1) # Once for the disable pipeline. + should_email(pipeline_user, times: 2) # Once for the new permission, once for the disable. end end end diff --git a/spec/views/notify/pipeline_failed_email.html.haml_spec.rb b/spec/views/notify/pipeline_failed_email.html.haml_spec.rb index d04affc7df1..623237b111a 100644 --- a/spec/views/notify/pipeline_failed_email.html.haml_spec.rb +++ b/spec/views/notify/pipeline_failed_email.html.haml_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe 'notify/pipeline_failed_email.html.haml' do include Devise::Test::ControllerHelpers - let(:user) { create(:user) } + let(:user) { create(:user, developer_projects: [project]) } let(:project) { create(:project, :repository) } let(:merge_request) { create(:merge_request, :simple, source_project: project) } diff --git a/spec/views/notify/pipeline_failed_email.text.erb_spec.rb b/spec/views/notify/pipeline_failed_email.text.erb_spec.rb index 079fb865d7b..81245239eba 100644 --- a/spec/views/notify/pipeline_failed_email.text.erb_spec.rb +++ b/spec/views/notify/pipeline_failed_email.text.erb_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' describe 'notify/pipeline_failed_email.text.erb' do include Devise::Test::ControllerHelpers - let(:user) { create(:user) } + let(:user) { create(:user, developer_projects: [project]) } let(:project) { create(:project, :repository) } let(:merge_request) { create(:merge_request, :simple, source_project: project) } diff --git a/spec/views/notify/pipeline_success_email.html.haml_spec.rb b/spec/views/notify/pipeline_success_email.html.haml_spec.rb index 8ee7f954d70..a876bf13e11 100644 --- a/spec/views/notify/pipeline_success_email.html.haml_spec.rb +++ b/spec/views/notify/pipeline_success_email.html.haml_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe 'notify/pipeline_success_email.html.haml' do include Devise::Test::ControllerHelpers - let(:user) { create(:user) } + let(:user) { create(:user, developer_projects: [project]) } let(:project) { create(:project, :repository) } let(:merge_request) { create(:merge_request, :simple, source_project: project) } diff --git a/spec/workers/auto_devops/disable_worker_spec.rb b/spec/workers/auto_devops/disable_worker_spec.rb index 800a07e41a5..53113f286ba 100644 --- a/spec/workers/auto_devops/disable_worker_spec.rb +++ b/spec/workers/auto_devops/disable_worker_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe AutoDevops::DisableWorker, '#perform' do - let(:user) { create(:user) } + let(:user) { create(:user, developer_projects: [project]) } let(:project) { create(:project, :repository, :auto_devops) } let(:auto_devops) { project.auto_devops } let(:pipeline) { create(:ci_pipeline, :failed, :auto_devops_source, project: project, user: user) } @@ -10,6 +10,7 @@ describe AutoDevops::DisableWorker, '#perform' do subject { described_class.new } before do + project.add_developer(user) stub_application_setting(auto_devops_enabled: true) auto_devops.update_attribute(:enabled, nil) end |