From 42d6d3187fb7305daead326bfdf56a09c249f829 Mon Sep 17 00:00:00 2001
From: drew cimino <dcimino@gitlab.com>
Date: Thu, 11 Apr 2019 16:21:18 +0100
Subject: [PATCH] preventing blocked users and their PipelineSchdules from
 creating new Pipelines

updated several specs and factories to accomodate new permissions
---
 app/policies/base_policy.rb                   |  4 ++
 app/policies/global_policy.rb                 |  4 --
 app/policies/project_policy.rb                |  4 ++
 .../error-pipelines-for-blocked-users.yml     |  5 +++
 spec/factories/users.rb                       | 10 +++++
 spec/features/commits_spec.rb                 |  6 +--
 .../projects/pipelines/pipeline_spec.rb       | 39 +++++++++++++++++++
 .../projects/pipelines/pipelines_spec.rb      |  2 +-
 spec/finders/pipelines_finder_spec.rb         |  5 ++-
 spec/javascripts/fixtures/pipelines.rb        |  2 +-
 .../gitlab/ci/pipeline/chain/build_spec.rb    |  2 +-
 spec/models/ci/pipeline_spec.rb               |  2 +-
 .../project_services/hipchat_service_spec.rb  |  2 +-
 .../ci/create_pipeline_service_spec.rb        | 12 ++++++
 spec/services/ci/play_build_service_spec.rb   | 37 ++++++------------
 spec/services/notification_service_spec.rb    |  6 ++-
 .../pipeline_failed_email.html.haml_spec.rb   |  2 +-
 .../pipeline_failed_email.text.erb_spec.rb    |  2 +-
 .../pipeline_success_email.html.haml_spec.rb  |  2 +-
 .../auto_devops/disable_worker_spec.rb        |  3 +-
 20 files changed, 106 insertions(+), 45 deletions(-)
 create mode 100644 changelogs/unreleased/error-pipelines-for-blocked-users.yml

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 3218c04b219..35b9bf2d6a3 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -445,6 +445,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 2adeb37c98a..d6da04171e5 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
             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 506aa867490..7db84568e86 100644
--- a/spec/features/projects/pipelines/pipeline_spec.rb
+++ b/spec/features/projects/pipelines/pipeline_spec.rb
@@ -3,6 +3,7 @@ require 'spec_helper'
 describe 'Pipeline', :js do
   include RoutesHelpers
   include ProjectForksHelper
+  include ::ExclusiveLeaseHelpers
 
   let(:project) { create(:project) }
   let(:user) { create(:user) }
@@ -537,6 +538,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 d9d73f789c5..979e78a11f9 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 a7d3dc09fd4..63b164e3c0e 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 a793b37e412..88013d2908b 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