Merge allowed_to_create? into CreatePipelineService

6 files changed, 122 insertions, 128 deletions

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 7963386bdb1..8d1beca9771 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -164,20 +164,6 @@ module Ci
       where.not(duration: nil).sum(:duration)
     end
 
-    def self.allowed_to_create?(user, project, ref)
-      repo = project.repository
-      access = Gitlab::UserAccess.new(user, project: project)
-
-      Ability.allowed?(user, :create_pipeline, project) &&
-        if repo.ref_exists?("#{Gitlab::Git::BRANCH_REF_PREFIX}#{ref}")
-          access.can_push_or_merge_to_branch?(ref)
-        elsif repo.ref_exists?("#{Gitlab::Git::TAG_REF_PREFIX}#{ref}")
-          access.can_create_tag?(ref)
-        else
-          false
-        end
-    end
-
     def self.internal_sources
       sources.reject { |source| source == "external" }.values
     end
diff --git a/app/models/ci/pipeline_schedule.rb b/app/models/ci/pipeline_schedule.rb
index eaca2774bf9..49455e79c15 100644
--- a/app/models/ci/pipeline_schedule.rb
+++ b/app/models/ci/pipeline_schedule.rb
@@ -36,10 +36,6 @@ module Ci
       update_attribute(:active, false)
     end
 
-    def runnable_by_owner?
-      Ci::Pipeline.allowed_to_create?(owner, project, ref)
-    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/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index e487b7d5f30..485161e5f3f 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -27,7 +27,7 @@ module Ci
         return error('Reference not found')
       end
 
-      unless triggering_user_allowed_for_ref?(trigger_request, ref)
+      unless triggering_user_allowed_for_ref?(trigger_request)
         return error("Insufficient permissions for protected #{ref}")
       end
 
@@ -74,14 +74,26 @@ module Ci
       pipeline.tap(&:process!)
     end
 
-    def triggering_user_allowed_for_ref?(trigger_request, ref)
+    def triggering_user_allowed_for_ref?(trigger_request)
       triggering_user = current_user || trigger_request.trigger.owner
 
-      (triggering_user &&
-        Ci::Pipeline.allowed_to_create?(triggering_user, project, ref)) ||
+      (triggering_user && allowed_to_create?(triggering_user)) ||
         !project.protected_for?(ref)
     end
 
+    def allowed_to_create?(triggering_user)
+      access = Gitlab::UserAccess.new(triggering_user, project: project)
+
+      Ability.allowed?(triggering_user, :create_pipeline, project) &&
+        if branch?
+          access.can_push_or_merge_to_branch?(ref)
+        elsif tag?
+          access.can_create_tag?(ref)
+        else
+          false
+        end
+    end
+
     def update_merge_requests_head_pipeline
       return unless pipeline.latest?
 
@@ -145,7 +157,7 @@ module Ci
     end
 
     def ref
-      Gitlab::Git.ref_name(origin_ref)
+      @ref ||= Gitlab::Git.ref_name(origin_ref)
     end
 
     def valid_sha?
diff --git a/app/workers/pipeline_schedule_worker.rb b/app/workers/pipeline_schedule_worker.rb
index 7b485b3363c..d7087f20dfc 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/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 7463fb3d379..d400bdfe8f8 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -28,103 +28,6 @@ describe Ci::Pipeline, models: true do
   it { is_expected.to respond_to :git_author_email }
   it { is_expected.to respond_to :short_sha }
 
-  describe '.allowed_to_create?' do
-    let(:user) { create(:user) }
-    let(:project) { create(:project, :repository) }
-    let(:ref) { 'master' }
-
-    subject { described_class.allowed_to_create?(user, project, ref) }
-
-    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
-
   describe '#source' do
     context 'when creating new pipeline' do
       let(:pipeline) do
diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb
index 7d960dc411f..66218772084 100644
--- a/spec/services/ci/create_pipeline_service_spec.rb
+++ b/spec/services/ci/create_pipeline_service_spec.rb
@@ -432,4 +432,104 @@ describe Ci::CreatePipelineService, :services do
       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