Merge branch 'error-pipelines-for-blocked-users' into 'master'

Preventing blocked users and their PipelineSchdules from creating new Pipelines

Closes #47756

See merge request gitlab-org/gitlab-ce!27318

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? }

app/policies/global_policy.rb

 # 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? }

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?

changelogs/unreleased/error-pipelines-for-blocked-users.yml

+---
+title: preventing blocked users and their PipelineSchdules from creating new Pipelines
+merge_request: 27318
+author:
+type: fixed

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'

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

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

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
 

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)

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') }

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

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

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

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

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) }

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

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) }
 

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) }
 

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) }
 

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