From 56418e85ac6b667d19495665860092ce4d74f55d Mon Sep 17 00:00:00 2001
From: Shinya Maeda <shinya@gitlab.com>
Date: Wed, 26 Jul 2017 18:31:09 +0900
Subject: [PATCH] init

---
 app/models/ci/build.rb                        |  1 +
 app/models/ci/pipeline.rb                     |  3 +-
 app/models/ci/pipeline_variable.rb            | 10 +++
 app/services/ci/create_pipeline_service.rb    | 18 +++--
 .../ci/create_trigger_request_service.rb      |  5 ++
 app/services/ci/pipeline_trigger_service.rb   | 44 +++++++++++
 ...0720130522_create_ci_pipeline_variables.rb | 20 +++++
 ...dd_foreign_key_to_ci_pipeline_variables.rb | 15 ++++
 db/schema.rb                                  | 12 +++
 lib/api/triggers.rb                           | 23 +++---
 .../ci/pipeline_variable_variables.rb         |  8 ++
 spec/lib/gitlab/import_export/all_models.yml  |  3 +
 spec/models/ci/build_spec.rb                  |  6 ++
 spec/models/ci/pipeline_spec.rb               |  5 +-
 spec/models/ci/pipeline_variable_spec.rb      |  8 ++
 spec/requests/api/triggers_spec.rb            | 19 ++---
 .../ci/pipeline_trigger_service_spec.rb       | 79 +++++++++++++++++++
 17 files changed, 247 insertions(+), 32 deletions(-)
 create mode 100644 app/models/ci/pipeline_variable.rb
 create mode 100644 app/services/ci/pipeline_trigger_service.rb
 create mode 100644 db/migrate/20170720130522_create_ci_pipeline_variables.rb
 create mode 100644 db/migrate/20170720130749_add_foreign_key_to_ci_pipeline_variables.rb
 create mode 100644 spec/factories/ci/pipeline_variable_variables.rb
 create mode 100644 spec/models/ci/pipeline_variable_spec.rb
 create mode 100644 spec/services/ci/pipeline_trigger_service_spec.rb

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 416a2a33378..d9029e5fbca 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -219,6 +219,7 @@ module Ci
       variables += project.group.secret_variables_for(ref, project).map(&:to_runner_variable) if project.group
       variables += secret_variables(environment: environment)
       variables += trigger_request.user_variables if trigger_request
+      variables += pipeline.variables.map(&:to_runner_variable) if pipeline.variables
       variables += pipeline.pipeline_schedule.job_variables if pipeline.pipeline_schedule
       variables += persisted_environment_variables if environment
 
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index e5b615a7cc0..148ff6d025a 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -15,13 +15,14 @@ module Ci
     has_many :statuses, class_name: 'CommitStatus', foreign_key: :commit_id
     has_many :builds, foreign_key: :commit_id
     has_many :trigger_requests, dependent: :destroy, foreign_key: :commit_id # rubocop:disable Cop/ActiveRecordDependent
+    has_many :variables, class_name: 'Ci::PipelineVariable'
 
     # Merge requests for which the current pipeline is running against
     # the merge request's latest commit.
     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.includes(:project) }, 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 :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'
diff --git a/app/models/ci/pipeline_variable.rb b/app/models/ci/pipeline_variable.rb
new file mode 100644
index 00000000000..00b419c3efa
--- /dev/null
+++ b/app/models/ci/pipeline_variable.rb
@@ -0,0 +1,10 @@
+module Ci
+  class PipelineVariable < ActiveRecord::Base
+    extend Ci::Model
+    include HasVariable
+
+    belongs_to :pipeline
+
+    validates :key, uniqueness: { scope: :pipeline_id }
+  end
+end
diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 21e2ef153de..884b681ff81 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -21,14 +21,22 @@ module Ci
 
       return result if result
 
-      Ci::Pipeline.transaction do
-        update_merge_requests_head_pipeline if pipeline.save
+      begin
+        Ci::Pipeline.transaction do
+          pipeline.save!
 
-        Ci::CreatePipelineStagesService
-          .new(project, current_user)
-          .execute(pipeline)
+          yield(pipeline) if block_given?
+
+          Ci::CreatePipelineStagesService
+            .new(project, current_user)
+            .execute(pipeline)
+        end
+      rescue ActiveRecord::RecordInvalid => e
+        return error("Failed to persist the pipeline: #{e}")
       end
 
+      update_merge_requests_head_pipeline
+
       cancel_pending_pipelines if project.auto_cancel_pending_pipelines?
 
       pipeline_created_counter.increment(source: source)
diff --git a/app/services/ci/create_trigger_request_service.rb b/app/services/ci/create_trigger_request_service.rb
index a43d0e4593c..b2aa457bbd5 100644
--- a/app/services/ci/create_trigger_request_service.rb
+++ b/app/services/ci/create_trigger_request_service.rb
@@ -1,3 +1,8 @@
+# This class is deprecated because we're closing Ci::TriggerRequest.
+# New class is PipelineTriggerService (app/services/ci/pipeline_trigger_service.rb)
+# which is integrated with Ci::PipelineVariable instaed of Ci::TriggerRequest.
+# We remove this class after we removed v1 and v3 API. This class is still being
+# referred by such legacy code.
 module Ci
   module CreateTriggerRequestService
     Result = Struct.new(:trigger_request, :pipeline)
diff --git a/app/services/ci/pipeline_trigger_service.rb b/app/services/ci/pipeline_trigger_service.rb
new file mode 100644
index 00000000000..af680d2f953
--- /dev/null
+++ b/app/services/ci/pipeline_trigger_service.rb
@@ -0,0 +1,44 @@
+module Ci
+  class PipelineTriggerService < BaseService
+    def execute
+      if trigger_from_token
+        create_pipeline_from_trigger(trigger_from_token)
+      end
+    end
+
+    private
+
+    def create_pipeline_from_trigger(trigger)
+      # this check is to not leak the presence of the project if user cannot read it
+      return unless trigger.project == project
+
+      pipeline = Ci::CreatePipelineService.new(project, trigger.owner, ref: params[:ref])
+        .execute(:trigger, ignore_skip_ci: true) do |pipeline|
+          trigger.trigger_requests.create(pipeline: pipeline)
+          create_pipeline_variables!(pipeline)
+        end
+
+      if pipeline.persisted?
+        success(pipeline: pipeline)
+      else
+        error(pipeline.errors.messages, 400)
+      end
+    end
+
+    def trigger_from_token
+      return @trigger if defined?(@trigger)
+
+      @trigger = Ci::Trigger.find_by_token(params[:token].to_s)
+    end
+
+    def create_pipeline_variables!(pipeline)
+      return unless params[:variables].is_a?(Hash)
+
+      variables = params[:variables].map do |key, value|
+        { key: key, value: value }
+      end
+
+      pipeline.variables.create!(variables)
+    end
+  end
+end
diff --git a/db/migrate/20170720130522_create_ci_pipeline_variables.rb b/db/migrate/20170720130522_create_ci_pipeline_variables.rb
new file mode 100644
index 00000000000..a784f5dd142
--- /dev/null
+++ b/db/migrate/20170720130522_create_ci_pipeline_variables.rb
@@ -0,0 +1,20 @@
+class CreateCiPipelineVariables < ActiveRecord::Migration
+  DOWNTIME = false
+
+  def up
+    create_table :ci_pipeline_variables do |t|
+      t.string :key, null: false
+      t.text :value
+      t.text :encrypted_value
+      t.string :encrypted_value_salt
+      t.string :encrypted_value_iv
+      t.integer :pipeline_id, null: false
+    end
+
+    add_index :ci_pipeline_variables, [:pipeline_id, :key], unique: true
+  end
+
+  def down
+    drop_table :ci_pipeline_variables
+  end
+end
diff --git a/db/migrate/20170720130749_add_foreign_key_to_ci_pipeline_variables.rb b/db/migrate/20170720130749_add_foreign_key_to_ci_pipeline_variables.rb
new file mode 100644
index 00000000000..550b8a88f02
--- /dev/null
+++ b/db/migrate/20170720130749_add_foreign_key_to_ci_pipeline_variables.rb
@@ -0,0 +1,15 @@
+class AddForeignKeyToCiPipelineVariables < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key(:ci_pipeline_variables, :ci_pipelines, column: :pipeline_id)
+  end
+
+  def down
+    remove_foreign_key(:ci_pipeline_variables, column: :pipeline_id)
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 93ab79e14e0..0abdb987b77 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -298,6 +298,17 @@ ActiveRecord::Schema.define(version: 20170725145659) do
   add_index "ci_pipeline_schedules", ["next_run_at", "active"], name: "index_ci_pipeline_schedules_on_next_run_at_and_active", using: :btree
   add_index "ci_pipeline_schedules", ["project_id"], name: "index_ci_pipeline_schedules_on_project_id", using: :btree
 
+  create_table "ci_pipeline_variables", force: :cascade do |t|
+    t.string "key", null: false
+    t.text "value"
+    t.text "encrypted_value"
+    t.string "encrypted_value_salt"
+    t.string "encrypted_value_iv"
+    t.integer "pipeline_id", null: false
+  end
+
+  add_index "ci_pipeline_variables", ["pipeline_id", "key"], name: "index_ci_pipeline_variables_on_pipeline_id_and_key", unique: true, using: :btree
+
   create_table "ci_pipelines", force: :cascade do |t|
     t.string "ref"
     t.string "sha"
@@ -1617,6 +1628,7 @@ ActiveRecord::Schema.define(version: 20170725145659) do
   add_foreign_key "ci_group_variables", "namespaces", column: "group_id", name: "fk_33ae4d58d8", on_delete: :cascade
   add_foreign_key "ci_pipeline_schedules", "projects", name: "fk_8ead60fcc4", on_delete: :cascade
   add_foreign_key "ci_pipeline_schedules", "users", column: "owner_id", name: "fk_9ea99f58d2", on_delete: :nullify
+  add_foreign_key "ci_pipeline_variables", "ci_pipelines", column: "pipeline_id", name: "fk_f29c5f4380", on_delete: :cascade
   add_foreign_key "ci_pipelines", "ci_pipeline_schedules", column: "pipeline_schedule_id", name: "fk_3d34ab2e06", on_delete: :nullify
   add_foreign_key "ci_pipelines", "ci_pipelines", column: "auto_canceled_by_id", name: "fk_262d4c2d19", on_delete: :nullify
   add_foreign_key "ci_pipelines", "projects", name: "fk_86635dbd80", on_delete: :cascade
diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb
index 9375e7eb768..edfdb63d183 100644
--- a/lib/api/triggers.rb
+++ b/lib/api/triggers.rb
@@ -15,25 +15,22 @@ module API
         optional :variables, type: Hash, desc: 'The list of variables to be injected into build'
       end
       post ":id/(ref/:ref/)trigger/pipeline", requirements: { ref: /.+/ } do
-        project = find_project(params[:id])
-        trigger = Ci::Trigger.find_by_token(params[:token].to_s)
-        not_found! unless project && trigger
-        unauthorized! unless trigger.project == project
-
         # validate variables
-        variables = params[:variables].to_h
-        unless variables.all? { |key, value| key.is_a?(String) && value.is_a?(String) }
+        params[:variables] = params[:variables].to_h
+        unless params[:variables].all? { |key, value| key.is_a?(String) && value.is_a?(String) }
           render_api_error!('variables needs to be a map of key-valued strings', 400)
         end
 
-        # create request and trigger builds
-        result = Ci::CreateTriggerRequestService.execute(project, trigger, params[:ref].to_s, variables)
-        pipeline = result.pipeline
+        project = find_project(params[:id])
+        not_found! unless project
 
-        if pipeline.persisted?
-          present pipeline, with: Entities::Pipeline
+        result = Ci::PipelineTriggerService.new(project, nil, params).execute
+        not_found! unless result
+
+        if result[:http_status]
+          render_api_error!(result[:message], result[:http_status])
         else
-          render_validation_error!(pipeline)
+          present result[:pipeline], with: Entities::Pipeline
         end
       end
 
diff --git a/spec/factories/ci/pipeline_variable_variables.rb b/spec/factories/ci/pipeline_variable_variables.rb
new file mode 100644
index 00000000000..7c1a7faec08
--- /dev/null
+++ b/spec/factories/ci/pipeline_variable_variables.rb
@@ -0,0 +1,8 @@
+FactoryGirl.define do
+  factory :ci_pipeline_variable, class: Ci::PipelineVariable do
+    sequence(:key) { |n| "VARIABLE_#{n}" }
+    value 'VARIABLE_VALUE'
+
+    pipeline factory: :ci_empty_pipeline
+  end
+end
diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index 977174a5fd2..6a41afe0c25 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/spec/lib/gitlab/import_export/all_models.yml
@@ -102,6 +102,7 @@ pipelines:
 - statuses
 - builds
 - trigger_requests
+- variables
 - auto_canceled_by
 - auto_canceled_pipelines
 - auto_canceled_jobs
@@ -112,6 +113,8 @@ pipelines:
 - artifacts
 - pipeline_schedule
 - merge_requests
+pipeline_variables:
+- pipeline
 stages:
 - project
 - pipeline
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index a18da3768d5..86afa856ea7 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -1468,6 +1468,12 @@ describe Ci::Build do
       it { is_expected.to include(predefined_trigger_variable) }
     end
 
+    context 'when pipeline has a variable' do
+      let!(:pipeline_variable) { create(:ci_pipeline_variable, pipeline: pipeline) }
+
+      it { is_expected.to include(pipeline_variable.to_runner_variable) }
+    end
+
     context 'when a job was triggered by a pipeline schedule' do
       let(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project) }
 
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 9461905c787..406608256e0 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -17,6 +17,7 @@ describe Ci::Pipeline do
 
   it { is_expected.to have_many(:statuses) }
   it { is_expected.to have_many(:trigger_requests) }
+  it { is_expected.to have_many(:variables) }
   it { is_expected.to have_many(:builds) }
   it { is_expected.to have_many(:auto_canceled_pipelines) }
   it { is_expected.to have_many(:auto_canceled_jobs) }
@@ -734,8 +735,6 @@ describe Ci::Pipeline do
 
     context 'on failure and build retry' do
       before do
-        stub_not_protect_default_branch
-
         build.drop
         project.add_developer(user)
 
@@ -1001,8 +1000,6 @@ describe Ci::Pipeline do
     let(:latest_status) { pipeline.statuses.latest.pluck(:status) }
 
     before do
-      stub_not_protect_default_branch
-
       project.add_developer(user)
     end
 
diff --git a/spec/models/ci/pipeline_variable_spec.rb b/spec/models/ci/pipeline_variable_spec.rb
new file mode 100644
index 00000000000..2ce78e34b0c
--- /dev/null
+++ b/spec/models/ci/pipeline_variable_spec.rb
@@ -0,0 +1,8 @@
+require 'spec_helper'
+
+describe Ci::PipelineVariable, models: true do
+  subject { build(:ci_pipeline_variable) }
+
+  it { is_expected.to include_module(HasVariable) }
+  it { is_expected.to validate_uniqueness_of(:key).scoped_to(:pipeline_id) }
+end
diff --git a/spec/requests/api/triggers_spec.rb b/spec/requests/api/triggers_spec.rb
index c2636b6614e..0b42a603885 100644
--- a/spec/requests/api/triggers_spec.rb
+++ b/spec/requests/api/triggers_spec.rb
@@ -22,6 +22,7 @@ describe API::Triggers do
 
     before do
       stub_ci_pipeline_to_return_yaml_file
+      trigger.update(owner: user)
     end
 
     context 'Handles errors' do
@@ -36,12 +37,6 @@ describe API::Triggers do
 
         expect(response).to have_http_status(404)
       end
-
-      it 'returns unauthorized if token is for different project' do
-        post api("/projects/#{project2.id}/trigger/pipeline"), options.merge(ref: 'master')
-
-        expect(response).to have_http_status(401)
-      end
     end
 
     context 'Have a commit' do
@@ -61,8 +56,7 @@ describe API::Triggers do
         post api("/projects/#{project.id}/trigger/pipeline"), options.merge(ref: 'other-branch')
 
         expect(response).to have_http_status(400)
-        expect(json_response['message']['base'])
-          .to contain_exactly('Reference not found')
+        expect(json_response['message']).to eq('base' => ["Reference not found"])
       end
 
       context 'Validates variables' do
@@ -88,12 +82,19 @@ describe API::Triggers do
           post api("/projects/#{project.id}/trigger/pipeline"), options.merge(variables: variables, ref: 'master')
 
           expect(response).to have_http_status(201)
-          expect(pipeline.builds.reload.first.trigger_request.variables).to eq(variables)
+          expect(Ci::Pipeline.last.variables.first.key).to eq(variables.keys.first)
+          expect(Ci::Pipeline.last.variables.first.value).to eq(variables.values.first)
         end
       end
     end
 
     context 'when triggering a pipeline from a trigger token' do
+      it 'does not leak the presence of project when token is for different project' do
+        post api("/projects/#{project2.id}/ref/master/trigger/pipeline?token=#{trigger_token}"), { ref: 'refs/heads/other-branch' }
+
+        expect(response).to have_http_status(404)
+      end
+
       it 'creates builds from the ref given in the URL, not in the body' do
         expect do
           post api("/projects/#{project.id}/ref/master/trigger/pipeline?token=#{trigger_token}"), { ref: 'refs/heads/other-branch' }
diff --git a/spec/services/ci/pipeline_trigger_service_spec.rb b/spec/services/ci/pipeline_trigger_service_spec.rb
new file mode 100644
index 00000000000..914ec4844d0
--- /dev/null
+++ b/spec/services/ci/pipeline_trigger_service_spec.rb
@@ -0,0 +1,79 @@
+require 'spec_helper'
+
+describe Ci::PipelineTriggerService, services: true do
+  let(:project) { create(:project, :repository) }
+
+  before do
+    stub_ci_pipeline_to_return_yaml_file
+  end
+
+  describe '#execute' do
+    let(:user) { create(:user) }
+
+    before do
+      project.add_developer(user)
+    end
+
+    let(:result) { described_class.new(project, user, params).execute }
+    let(:trigger) { create(:ci_trigger, project: project, owner: user) }
+
+    context 'when params have an existsed trigger token' do
+      let(:token) { trigger.token }
+
+      context 'when params have an existsed ref' do
+        let(:ref) { 'master' }
+
+        it 'triggers a pipeline' do
+          expect { result }.to change { Ci::Pipeline.count }.by(1)
+          expect(result[:pipeline].ref).to eq(ref)
+          expect(result[:pipeline].project).to eq(project)
+          expect(result[:pipeline].user).to eq(trigger.owner)
+          expect(result[:status]).to eq(:success)
+        end
+
+        context 'when params have a variable' do
+          let(:variables) { { 'AAA' => 'AAA123' } }
+
+          it 'has a variable' do
+            expect { result }.to change { Ci::PipelineVariable.count }.by(1)
+            expect(result[:pipeline].variables.first.key).to eq(variables.keys.first)
+            expect(result[:pipeline].variables.first.value).to eq(variables.values.first)
+          end
+        end
+
+        context 'when params have two variables and keys are duplicated' do
+          let(:variables) { [{ key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' }] }
+
+          it 'returns error' do
+            expect { result }.not_to change { Ci::Pipeline.count }
+            expect(result[:http_status]).to eq(400)
+          end
+        end
+      end
+
+      context 'when params have a non-existsed ref' do
+        let(:ref) { 'invalid-ref' }
+
+        it 'does not trigger a pipeline' do
+          expect { result }.not_to change { Ci::Pipeline.count }
+          expect(result[:http_status]).to eq(400)
+        end
+      end
+    end
+
+    context 'when params have a non-existsed trigger token' do
+      let(:token) { 'invalid-token' }
+
+      it 'does not trigger a pipeline' do
+        expect { result }.not_to change { Ci::Pipeline.count }
+        expect(result).to be_nil
+      end
+    end
+  end
+
+  def params
+    { token: defined?(token) ? token : nil,
+      ref: defined?(ref) ? ref : nil,
+      variables: defined?(variables) ? variables : nil }
+  end
+end