From 9ecb85a4f36669fa05c961eef84cf46d7bf7f39c Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Mon, 5 Jun 2017 23:38:06 +0800
Subject: Forbid creating pipeline if it's protected and

cannot create the tag if it's a tag, and
cannot merge the branch if it's a branch.
---
 app/services/ci/create_pipeline_service.rb | 10 ++++++++++
 1 file changed, 10 insertions(+)

(limited to 'app')

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 13baa63220d..a54af4749ac 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -27,6 +27,12 @@ module Ci
         return error('Reference not found')
       end
 
+      if tag?
+        return error("#{ref} is protected") unless access.can_create_tag?(ref)
+      else
+        return error("#{ref} is protected") unless access.can_merge_to_branch?(ref)
+      end
+
       unless commit
         return error('Commit not found')
       end
@@ -94,6 +100,10 @@ module Ci
       @commit ||= project.commit(origin_sha || origin_ref)
     end
 
+    def access
+      @access ||= Gitlab::UserAccess.new(current_user, project: project)
+    end
+
     def sha
       commit.try(:id)
     end
-- 
cgit v1.2.1


From 4408da47b8462055612548b8d43a679c861595e8 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 6 Jun 2017 00:56:38 +0800
Subject: Move the check to Pipeline.allowed_to_create?

So that we could use it for the schedule before
trying to use CreatePipelineService
---
 app/models/ci/pipeline.rb                  | 14 ++++++++++++++
 app/models/ci/pipeline_schedule.rb         |  2 +-
 app/services/ci/create_pipeline_service.rb | 28 ++++++++++++++++------------
 3 files changed, 31 insertions(+), 13 deletions(-)

(limited to 'app')

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 425ca9278eb..e2caeda2289 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -162,6 +162,20 @@ 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_merge_to_branch?(ref)
+        elsif repo.ref_exists?("#{Gitlab::Git::TAG_REF_PREFIX}#{ref}")
+          access.can_create_tag?(ref)
+        else
+          false
+        end
+    end
+
     def stage(name)
       stage = Ci::Stage.new(self, name: name)
       stage unless stage.statuses_count.zero?
diff --git a/app/models/ci/pipeline_schedule.rb b/app/models/ci/pipeline_schedule.rb
index 45d8cd34359..eaca2774bf9 100644
--- a/app/models/ci/pipeline_schedule.rb
+++ b/app/models/ci/pipeline_schedule.rb
@@ -37,7 +37,7 @@ module Ci
     end
 
     def runnable_by_owner?
-      Ability.allowed?(owner, :create_pipeline, project)
+      Ci::Pipeline.allowed_to_create?(owner, project, ref)
     end
 
     def set_next_run_at
diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index a54af4749ac..5ed9d1aa517 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -27,10 +27,8 @@ module Ci
         return error('Reference not found')
       end
 
-      if tag?
-        return error("#{ref} is protected") unless access.can_create_tag?(ref)
-      else
-        return error("#{ref} is protected") unless access.can_merge_to_branch?(ref)
+      unless Ci::Pipeline.allowed_to_create?(current_user, project, ref)
+        return error("Insufficient permissions for protected #{ref}")
       end
 
       unless commit
@@ -53,6 +51,12 @@ module Ci
         return error('No builds for this pipeline.')
       end
 
+      process!
+    end
+
+    private
+
+    def process!
       Ci::Pipeline.transaction do
         update_merge_requests_head_pipeline if pipeline.save
 
@@ -66,8 +70,6 @@ module Ci
       pipeline.tap(&:process!)
     end
 
-    private
-
     def update_merge_requests_head_pipeline
       return unless pipeline.latest?
 
@@ -100,10 +102,6 @@ module Ci
       @commit ||= project.commit(origin_sha || origin_ref)
     end
 
-    def access
-      @access ||= Gitlab::UserAccess.new(current_user, project: project)
-    end
-
     def sha
       commit.try(:id)
     end
@@ -121,11 +119,17 @@ 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
-- 
cgit v1.2.1


From 47b93fd76138ce24ec78926647497e52c5101dd8 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 6 Jun 2017 02:19:47 +0800
Subject: Don't check permission, only protected ref if no user

---
 app/services/ci/create_pipeline_service.rb | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

(limited to 'app')

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 5ed9d1aa517..7efea564ba6 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 Ci::Pipeline.allowed_to_create?(current_user, project, ref)
+      unless triggering_user_allowed_for_ref?(trigger_request, ref)
         return error("Insufficient permissions for protected #{ref}")
       end
 
@@ -56,6 +56,14 @@ module Ci
 
     private
 
+    def triggering_user_allowed_for_ref?(trigger_request, ref)
+      triggering_user = current_user || trigger_request.trigger.owner
+
+      (triggering_user &&
+        Ci::Pipeline.allowed_to_create?(triggering_user, project, ref)) ||
+        !project.protected_for?(ref)
+    end
+
     def process!
       Ci::Pipeline.transaction do
         update_merge_requests_head_pipeline if pipeline.save
-- 
cgit v1.2.1


From 9984f07a28273035d6c989913cb76c9c371965d0 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 6 Jun 2017 18:00:34 +0800
Subject: Disallow legacy trigger without a owner

Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11910#note_31594492
https://gitlab.com/gitlab-org/gitlab-ce/issues/30634#note_31601001
---
 app/services/ci/create_pipeline_service.rb | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

(limited to 'app')

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 7efea564ba6..a51c52b3f91 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -23,6 +23,10 @@ module Ci
         return error('Insufficient permissions to create a new pipeline')
       end
 
+      unless trigger_request && trigger_request.trigger.owner
+        return error('Legacy trigger without a owner is not allowed')
+      end
+
       unless branch? || tag?
         return error('Reference not found')
       end
@@ -59,9 +63,7 @@ module Ci
     def triggering_user_allowed_for_ref?(trigger_request, ref)
       triggering_user = current_user || trigger_request.trigger.owner
 
-      (triggering_user &&
-        Ci::Pipeline.allowed_to_create?(triggering_user, project, ref)) ||
-        !project.protected_for?(ref)
+      Ci::Pipeline.allowed_to_create?(triggering_user, project, ref)
     end
 
     def process!
-- 
cgit v1.2.1


From e86e1e515a7a4e4e1ee53d3d33bdfebfddd226a6 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 6 Jun 2017 20:23:19 +0800
Subject: Try to report why it's failing and fix tests

---
 app/services/ci/create_pipeline_service.rb        | 2 +-
 app/services/ci/create_trigger_request_service.rb | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

(limited to 'app')

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index a51c52b3f91..b3dbb548454 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -23,7 +23,7 @@ module Ci
         return error('Insufficient permissions to create a new pipeline')
       end
 
-      unless trigger_request && trigger_request.trigger.owner
+      if trigger_request && !trigger_request.trigger.owner
         return error('Legacy trigger without a owner is not allowed')
       end
 
diff --git a/app/services/ci/create_trigger_request_service.rb b/app/services/ci/create_trigger_request_service.rb
index beb27a5a597..e4f55c27f61 100644
--- a/app/services/ci/create_trigger_request_service.rb
+++ b/app/services/ci/create_trigger_request_service.rb
@@ -6,7 +6,8 @@ module Ci
       pipeline = Ci::CreatePipelineService.new(project, trigger.owner, ref: ref).
         execute(:trigger, ignore_skip_ci: true, trigger_request: trigger_request)
 
-      trigger_request if pipeline.persisted?
+      trigger_request.pipeline = pipeline
+      trigger_request
     end
   end
 end
-- 
cgit v1.2.1


From 6d17ddac5aaf6c178a13c1e371b072780e7fd049 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 6 Jun 2017 23:52:57 +0800
Subject: Still allow legacy triggers, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11910#note_31632911
---
 app/services/ci/create_pipeline_service.rb | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

(limited to 'app')

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index b3dbb548454..7efea564ba6 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -23,10 +23,6 @@ module Ci
         return error('Insufficient permissions to create a new pipeline')
       end
 
-      if trigger_request && !trigger_request.trigger.owner
-        return error('Legacy trigger without a owner is not allowed')
-      end
-
       unless branch? || tag?
         return error('Reference not found')
       end
@@ -63,7 +59,9 @@ module Ci
     def triggering_user_allowed_for_ref?(trigger_request, ref)
       triggering_user = current_user || trigger_request.trigger.owner
 
-      Ci::Pipeline.allowed_to_create?(triggering_user, project, ref)
+      (triggering_user &&
+        Ci::Pipeline.allowed_to_create?(triggering_user, project, ref)) ||
+        !project.protected_for?(ref)
     end
 
     def process!
-- 
cgit v1.2.1


From 23bfd8c13c803f4efdb9eaf8e6e3c1ffd17640e8 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 4 Jul 2017 05:01:05 +0800
Subject: Consistently check permission for creating pipelines,

updating builds and updating pipelines. We check against
being able to merge or push if the ref is protected.
---
 app/models/ci/pipeline.rb          |  2 +-
 app/policies/ci/build_policy.rb    | 11 ++++++-----
 app/policies/ci/pipeline_policy.rb | 19 ++++++++++++++++++-
 3 files changed, 25 insertions(+), 7 deletions(-)

(limited to 'app')

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index a46c1304667..06ce01095ea 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -169,7 +169,7 @@ module Ci
 
       Ability.allowed?(user, :create_pipeline, project) &&
         if repo.ref_exists?("#{Gitlab::Git::BRANCH_REF_PREFIX}#{ref}")
-          access.can_merge_to_branch?(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
diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb
index 2d7405dc240..85245528602 100644
--- a/app/policies/ci/build_policy.rb
+++ b/app/policies/ci/build_policy.rb
@@ -11,19 +11,20 @@ module Ci
         cannot! :"#{rule}_commit_status" unless can? :"#{rule}_build"
       end
 
-      if can?(:update_build) && protected_action?
+      if can?(:update_build) && !can_user_update?
         cannot! :update_build
       end
     end
 
     private
 
-    def protected_action?
-      return false unless build.action?
+    def can_user_update?
+      user_access.can_push_or_merge_to_branch?(build.ref)
+    end
 
-      !::Gitlab::UserAccess
+    def user_access
+      @user_access ||= ::Gitlab::UserAccess
         .new(user, project: build.project)
-        .can_merge_to_branch?(build.ref)
     end
   end
 end
diff --git a/app/policies/ci/pipeline_policy.rb b/app/policies/ci/pipeline_policy.rb
index 10aa2d3e72a..e71cc358353 100644
--- a/app/policies/ci/pipeline_policy.rb
+++ b/app/policies/ci/pipeline_policy.rb
@@ -1,7 +1,24 @@
 module Ci
   class PipelinePolicy < BasePolicy
+    alias_method :pipeline, :subject
+
     def rules
-      delegate! @subject.project
+      delegate! pipeline.project
+
+      if can?(:update_pipeline) && !can_user_update?
+        cannot! :update_pipeline
+      end
+    end
+
+    private
+
+    def can_user_update?
+      user_access.can_push_or_merge_to_branch?(pipeline.ref)
+    end
+
+    def user_access
+      @user_access ||= ::Gitlab::UserAccess
+        .new(user, project: pipeline.project)
     end
   end
 end
-- 
cgit v1.2.1


From 005870d5ce1a00b3405d0ae3a639d0c4befcb7a2 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 4 Jul 2017 05:20:44 +0800
Subject: Fix bad conflict resolution

---
 app/policies/ci/pipeline_policy.rb         |  2 +-
 app/services/ci/create_pipeline_service.rb | 22 ++++++++++++----------
 2 files changed, 13 insertions(+), 11 deletions(-)

(limited to 'app')

diff --git a/app/policies/ci/pipeline_policy.rb b/app/policies/ci/pipeline_policy.rb
index 73b5a40c7fc..8dba28b8d97 100644
--- a/app/policies/ci/pipeline_policy.rb
+++ b/app/policies/ci/pipeline_policy.rb
@@ -1,6 +1,6 @@
 module Ci
   class PipelinePolicy < BasePolicy
-    delegate { pipeline.project }
+    delegate { @subject.project }
 
     condition(:user_cannot_update) do
       !::Gitlab::UserAccess
diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index db12116b3ae..e487b7d5f30 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -51,19 +51,13 @@ module Ci
         return error('No stages / jobs for this pipeline.')
       end
 
-      process!
+      process! do
+        pipeline_created_counter.increment(source: source)
+      end
     end
 
     private
 
-    def triggering_user_allowed_for_ref?(trigger_request, ref)
-      triggering_user = current_user || trigger_request.trigger.owner
-
-      (triggering_user &&
-        Ci::Pipeline.allowed_to_create?(triggering_user, project, ref)) ||
-        !project.protected_for?(ref)
-    end
-
     def process!
       Ci::Pipeline.transaction do
         update_merge_requests_head_pipeline if pipeline.save
@@ -75,11 +69,19 @@ module Ci
 
       cancel_pending_pipelines if project.auto_cancel_pending_pipelines?
 
-      pipeline_created_counter.increment(source: source)
+      yield
 
       pipeline.tap(&:process!)
     end
 
+    def triggering_user_allowed_for_ref?(trigger_request, ref)
+      triggering_user = current_user || trigger_request.trigger.owner
+
+      (triggering_user &&
+        Ci::Pipeline.allowed_to_create?(triggering_user, project, ref)) ||
+        !project.protected_for?(ref)
+    end
+
     def update_merge_requests_head_pipeline
       return unless pipeline.latest?
 
-- 
cgit v1.2.1


From a4dd3ea168d19d2b65b7e55ed0043c7e7dcac77c Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 4 Jul 2017 18:00:39 +0800
Subject: Make sure that retryable_builds would preload project

---
 app/models/ci/pipeline.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(limited to 'app')

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index bea2ec1e18c..7963386bdb1 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'
-- 
cgit v1.2.1


From 56ea7a0cfe0fcdff33de80fd4602f463367914b2 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 5 Jul 2017 21:55:35 +0800
Subject: Merge allowed_to_create? into CreatePipelineService

---
 app/models/ci/pipeline.rb                  | 14 --------------
 app/models/ci/pipeline_schedule.rb         |  4 ----
 app/services/ci/create_pipeline_service.rb | 22 +++++++++++++++++-----
 app/workers/pipeline_schedule_worker.rb    | 13 +++++--------
 4 files changed, 22 insertions(+), 31 deletions(-)

(limited to 'app')

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
-- 
cgit v1.2.1


From 550ccf443059412a26adfcba15fbe9d05d39a5f9 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 6 Jul 2017 17:37:27 +0800
Subject: Make message and code more clear

---
 app/services/ci/create_pipeline_service.rb | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

(limited to 'app')

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 485161e5f3f..a8034e30a85 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -28,7 +28,7 @@ module Ci
       end
 
       unless triggering_user_allowed_for_ref?(trigger_request)
-        return error("Insufficient permissions for protected #{ref}")
+        return error("Insufficient permissions for protected ref '#{ref}'")
       end
 
       unless commit
@@ -77,8 +77,11 @@ module Ci
     def triggering_user_allowed_for_ref?(trigger_request)
       triggering_user = current_user || trigger_request.trigger.owner
 
-      (triggering_user && allowed_to_create?(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
 
     def allowed_to_create?(triggering_user)
-- 
cgit v1.2.1


From 679789ee93b0e5db3863bfcd539e20074c140984 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 18 Jul 2017 21:56:28 +0800
Subject: Rename can_push_or_merge_to_branch? to can_update_branch?

Also make sure pipeline would also check against tag as well
---
 app/policies/ci/build_policy.rb            |  2 +-
 app/policies/ci/pipeline_policy.rb         | 10 +++++++---
 app/services/ci/create_pipeline_service.rb |  2 +-
 3 files changed, 9 insertions(+), 5 deletions(-)

(limited to 'app')

diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb
index 00adb51e7de..00f18d0155b 100644
--- a/app/policies/ci/build_policy.rb
+++ b/app/policies/ci/build_policy.rb
@@ -6,7 +6,7 @@ module Ci
       if @subject.tag?
         !access.can_create_tag?(@subject.ref)
       else
-        !access.can_push_or_merge_to_branch?(@subject.ref)
+        !access.can_update_branch?(@subject.ref)
       end
     end
 
diff --git a/app/policies/ci/pipeline_policy.rb b/app/policies/ci/pipeline_policy.rb
index 8dba28b8d97..07d724c9cfd 100644
--- a/app/policies/ci/pipeline_policy.rb
+++ b/app/policies/ci/pipeline_policy.rb
@@ -3,9 +3,13 @@ module Ci
     delegate { @subject.project }
 
     condition(:user_cannot_update) do
-      !::Gitlab::UserAccess
-        .new(@user, project: @subject.project)
-        .can_push_or_merge_to_branch?(@subject.ref)
+      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 { user_cannot_update }.prevent :update_pipeline
diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 8e2184a1f19..8b689968895 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -89,7 +89,7 @@ module Ci
 
       Ability.allowed?(triggering_user, :create_pipeline, project) &&
         if branch?
-          access.can_push_or_merge_to_branch?(ref)
+          access.can_update_branch?(ref)
         elsif tag?
           access.can_create_tag?(ref)
         else
-- 
cgit v1.2.1


From 1ed6d1541c7973c08cdd4c1906ddcc0c3db893e3 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 18 Jul 2017 22:13:57 +0800
Subject: Rename :user_cannot_update to :protected_ref

---
 app/policies/ci/build_policy.rb    | 4 ++--
 app/policies/ci/pipeline_policy.rb | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

(limited to 'app')

diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb
index 00f18d0155b..984e5482288 100644
--- a/app/policies/ci/build_policy.rb
+++ b/app/policies/ci/build_policy.rb
@@ -1,6 +1,6 @@
 module Ci
   class BuildPolicy < CommitStatusPolicy
-    condition(:user_cannot_update) do
+    condition(:protected_ref) do
       access = ::Gitlab::UserAccess.new(@user, project: @subject.project)
 
       if @subject.tag?
@@ -10,6 +10,6 @@ module Ci
       end
     end
 
-    rule { user_cannot_update }.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 07d724c9cfd..4e689a9efd5 100644
--- a/app/policies/ci/pipeline_policy.rb
+++ b/app/policies/ci/pipeline_policy.rb
@@ -2,7 +2,7 @@ module Ci
   class PipelinePolicy < BasePolicy
     delegate { @subject.project }
 
-    condition(:user_cannot_update) do
+    condition(:protected_ref) do
       access = ::Gitlab::UserAccess.new(@user, project: @subject.project)
 
       if @subject.tag?
@@ -12,6 +12,6 @@ module Ci
       end
     end
 
-    rule { user_cannot_update }.prevent :update_pipeline
+    rule { protected_ref }.prevent :update_pipeline
   end
 end
-- 
cgit v1.2.1


From b84eb3434d0493cd594eade68d344a9675d72b8a Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 19 Jul 2017 16:42:47 +0800
Subject: Try to merge permission checks into one

---
 app/services/ci/create_pipeline_service.rb | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

(limited to 'app')

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 8b689968895..f331f86e622 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -19,18 +19,20 @@ module Ci
         return error('Pipeline is disabled')
       end
 
-      unless trigger_request || can?(current_user, :create_pipeline, project)
-        return error('Insufficient permissions to create a new pipeline')
+      triggering_user = current_user || trigger_request.trigger.owner
+
+      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?
         return error('Reference not found')
       end
 
-      unless triggering_user_allowed_for_ref?(trigger_request)
-        return error("Insufficient permissions for protected ref '#{ref}'")
-      end
-
       unless commit
         return error('Commit not found')
       end
@@ -74,9 +76,7 @@ module Ci
       pipeline.tap(&:process!)
     end
 
-    def triggering_user_allowed_for_ref?(trigger_request)
-      triggering_user = current_user || trigger_request.trigger.owner
-
+    def allowed_to_trigger_pipeline?(triggering_user)
       if triggering_user
         allowed_to_create?(triggering_user)
       else # legacy triggers don't have a corresponding user
@@ -87,7 +87,7 @@ module Ci
     def allowed_to_create?(triggering_user)
       access = Gitlab::UserAccess.new(triggering_user, project: project)
 
-      Ability.allowed?(triggering_user, :create_pipeline, project) &&
+      can?(triggering_user, :create_pipeline, project) &&
         if branch?
           access.can_update_branch?(ref)
         elsif tag?
-- 
cgit v1.2.1


From a397a0eb1a4c34c27175e2c4e68e7ceb43a81f02 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 19 Jul 2017 19:12:11 +0800
Subject: Eliminate N+1 queries on checking different protected refs

I realized where the N+1 queries were actually coming from
project.protected_branches, but how come we cannot preload this,
or cache this at all?

Then I found that this is somehow a Rails limitation. What we're
doing before, eventually come to:

    project.protected_branches.matching

But why it's not cached? (project.protected_branches.loaded? is always
false) It's because matching is a class method, which is called on
the proxy. In this case, Rails cannot cache the result. I don't know
if this is possible to implement or not, because clearly this would
require some tricks to implement class methods on associations.

So instead, we could just pass project.protected_branches to
ProtectedRef.matching, then it would work regularly.

With this change, there's no more N+1 queries.
---
 app/models/concerns/protected_ref.rb | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

(limited to 'app')

diff --git a/app/models/concerns/protected_ref.rb b/app/models/concerns/protected_ref.rb
index fc6b840f7a8..ca9ef2b9375 100644
--- a/app/models/concerns/protected_ref.rb
+++ b/app/models/concerns/protected_ref.rb
@@ -25,8 +25,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
@@ -37,8 +37,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)
-- 
cgit v1.2.1


From d035d735242a47bee7cd5973c9daa7d984800700 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 19 Jul 2017 22:37:38 +0800
Subject: Fix tests and fine tweak permission error message

---
 app/services/ci/create_pipeline_service.rb | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

(limited to 'app')

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index f331f86e622..700ac42d56e 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -23,16 +23,16 @@ module Ci
 
       unless allowed_to_trigger_pipeline?(triggering_user)
         if can?(triggering_user, :create_pipeline, project)
-          return error("Insufficient permissions for protected ref '#{ref}'")
+          if branch? || tag?
+            return error("Insufficient permissions for protected ref '#{ref}'")
+          else
+            return error('Reference not found')
+          end
         else
           return error('Insufficient permissions to create a new pipeline')
         end
       end
 
-      unless branch? || tag?
-        return error('Reference not found')
-      end
-
       unless commit
         return error('Commit not found')
       end
-- 
cgit v1.2.1


From a05bc477b99500fa919295e1086f7a8de903e3c4 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 20 Jul 2017 00:08:34 +0800
Subject: Use hash to return multiple objects

---
 app/services/ci/create_trigger_request_service.rb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

(limited to 'app')

diff --git a/app/services/ci/create_trigger_request_service.rb b/app/services/ci/create_trigger_request_service.rb
index 90f75606ddf..1674830a41a 100644
--- a/app/services/ci/create_trigger_request_service.rb
+++ b/app/services/ci/create_trigger_request_service.rb
@@ -1,13 +1,13 @@
 module Ci
-  class CreateTriggerRequestService
-    def execute(project, trigger, ref, variables = nil)
+  module CreateTriggerRequestService
+    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.pipeline = pipeline
-      trigger_request
+      { trigger_request: trigger_request,
+        pipeline: pipeline }
     end
   end
 end
-- 
cgit v1.2.1


From c9c715cd5510456d83da5272f28b7ce7f248c77f Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 20 Jul 2017 01:31:20 +0800
Subject: Make permission checks easier to understand

---
 app/services/ci/create_pipeline_service.rb | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

(limited to 'app')

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 700ac42d56e..5da70ba87e9 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -23,16 +23,16 @@ module Ci
 
       unless allowed_to_trigger_pipeline?(triggering_user)
         if can?(triggering_user, :create_pipeline, project)
-          if branch? || tag?
-            return error("Insufficient permissions for protected ref '#{ref}'")
-          else
-            return error('Reference not found')
-          end
+          return error("Insufficient permissions for protected ref '#{ref}'")
         else
           return error('Insufficient permissions to create a new pipeline')
         end
       end
 
+      unless branch? || tag?
+        return error('Reference not found')
+      end
+
       unless commit
         return error('Commit not found')
       end
@@ -93,7 +93,7 @@ module Ci
         elsif tag?
           access.can_create_tag?(ref)
         else
-          false
+          true # Allow it for now and we'll reject when we check ref existence
         end
     end
 
-- 
cgit v1.2.1


From e9862a9900c6269a41b65ca543035e57b49fede3 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 20 Jul 2017 20:17:42 +0800
Subject: Use struct instead of hash

---
 app/services/ci/create_trigger_request_service.rb | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

(limited to 'app')

diff --git a/app/services/ci/create_trigger_request_service.rb b/app/services/ci/create_trigger_request_service.rb
index 1674830a41a..a43d0e4593c 100644
--- a/app/services/ci/create_trigger_request_service.rb
+++ b/app/services/ci/create_trigger_request_service.rb
@@ -1,13 +1,14 @@
 module Ci
   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: trigger_request,
-        pipeline: pipeline }
+      Result.new(trigger_request, pipeline)
     end
   end
 end
-- 
cgit v1.2.1


From 8a444484345806dcbc0312d770b185edde1edb67 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 21 Jul 2017 17:49:32 +0800
Subject: Extract validations

---
 app/services/ci/create_pipeline_service.rb | 48 +++++++++++++++---------------
 1 file changed, 24 insertions(+), 24 deletions(-)

(limited to 'app')

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 3ff698b6437..21e2ef153de 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -15,12 +15,34 @@ 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
 
-      triggering_user = current_user || trigger_request.trigger.owner
-
       unless allowed_to_trigger_pipeline?(triggering_user)
         if can?(triggering_user, :create_pipeline, project)
           return error("Insufficient permissions for protected ref '#{ref}'")
@@ -52,28 +74,6 @@ module Ci
       unless pipeline.has_stage_seeds?
         return error('No stages / jobs for this pipeline.')
       end
-
-      process! do
-        pipeline_created_counter.increment(source: source)
-      end
-    end
-
-    private
-
-    def process!
-      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?
-
-      yield
-
-      pipeline.tap(&:process!)
     end
 
     def allowed_to_trigger_pipeline?(triggering_user)
-- 
cgit v1.2.1