Automatic merge of gitlab-org/gitlab-ce master

app/models/ci/trigger.rb

@@ -14,6 +14,7 @@ module Ci
     has_many :trigger_requests
 
     validates :token, presence: true, uniqueness: true
+    validates :owner, presence: true, unless: :supports_legacy_tokens?
 
     before_validation :set_default_values
 
@@ -37,8 +38,13 @@ module Ci
       self.owner_id.blank?
     end
 
+    def supports_legacy_tokens?
+      Feature.enabled?(:use_legacy_pipeline_triggers, project)
+    end
+
     def can_access_project?
-      self.owner_id.blank? || Ability.allowed?(self.owner, :create_build, project)
+      supports_legacy_tokens? && legacy? ||
+        Ability.allowed?(self.owner, :create_build, project)
     end
   end
 end

app/policies/ci/trigger_policy.rb

@@ -5,7 +5,7 @@ module Ci
     delegate { @subject.project }
 
     with_options scope: :subject, score: 0
-    condition(:legacy) { @subject.legacy? }
+    condition(:legacy) { @subject.supports_legacy_tokens? && @subject.legacy? }
 
     with_score 0
     condition(:is_owner) { @user && @subject.owner_id == @user.id }

app/views/projects/triggers/_content.html.haml

-%p.append-bottom-default
+- if Feature.enabled?(:use_legacy_pipeline_triggers, @project)
+  %p.append-bottom-default
     Triggers with the
     %span.badge.badge-primary legacy
     label do not have an associated user and only have access to the current project.

app/views/projects/triggers/_trigger.html.haml

@@ -8,8 +8,11 @@
 
     .label-container
       - if trigger.legacy?
+        - if trigger.supports_legacy_tokens?
           %span.badge.badge-primary.has-tooltip{ title: "Trigger makes use of deprecated functionality" } legacy
-      - if !trigger.can_access_project?
+        - else
+          %span.badge.badge-danger.has-tooltip{ title: "Trigger is invalid due to being a legacy trigger. We recommend replacing it with a new trigger" } invalid
+      - elsif !trigger.can_access_project?
         %span.badge.badge-danger.has-tooltip{ title: "Trigger user has insufficient permissions to project" } invalid
 
   %td

changelogs/unreleased/remove-support-for-legacy-pipeline-triggers.yml

+---
+title: Remove support for legacy pipeline triggers
+merge_request: 30133
+author:
+type: removed

lib/gitlab/ci/pipeline/chain/command.rb

@@ -20,6 +20,12 @@ module Gitlab
             end
           end
 
+          def uses_unsupported_legacy_trigger?
+            trigger_request.present? &&
+              trigger_request.trigger.legacy? &&
+              !trigger_request.trigger.supports_legacy_tokens?
+          end
+
           def branch_exists?
             strong_memoize(:is_branch) do
               project.repository.branch_exists?(ref)

lib/gitlab/ci/pipeline/chain/validate/abilities.rb

@@ -14,6 +14,10 @@ module Gitlab
                 return error('Pipelines are disabled!')
               end
 
+              if @command.uses_unsupported_legacy_trigger?
+                return error('Trigger token is invalid because is not owned by any user')
+              end
+
               unless allowed_to_trigger_pipeline?
                 if can?(current_user, :create_pipeline, project)
                   return error("Insufficient permissions for protected ref '#{command.ref}'")

lib/gitlab/import_export/relation_factory.rb

@@ -30,7 +30,7 @@ module Gitlab
                     links: 'Releases::Link',
                     metrics_setting: 'ProjectMetricsSetting' }.freeze
 
-      USER_REFERENCES = %w[author_id assignee_id updated_by_id merged_by_id latest_closed_by_id user_id created_by_id last_edited_by_id merge_user_id resolved_by_id closed_by_id].freeze
+      USER_REFERENCES = %w[author_id assignee_id updated_by_id merged_by_id latest_closed_by_id user_id created_by_id last_edited_by_id merge_user_id resolved_by_id closed_by_id owner_id].freeze
 
       PROJECT_REFERENCES = %w[project_id source_project_id target_project_id].freeze
 
@@ -80,6 +80,9 @@ module Gitlab
       def create
         return if unknown_service?
 
+        # Do not import legacy triggers
+        return if !Feature.enabled?(:use_legacy_pipeline_triggers, @project) && legacy_trigger?
+
         setup_models
 
         generate_imported_object
@@ -280,6 +283,10 @@ module Gitlab
           !Object.const_defined?(parsed_relation_hash['type'])
       end
 
+      def legacy_trigger?
+        @relation_name == 'Ci::Trigger' && @relation_hash['owner_id'].nil?
+      end
+
       def find_or_create_object!
         return relation_class.find_or_create_by(project_id: @project.id) if @relation_name == :project_feature
 

spec/features/triggers_spec.rb

@@ -66,7 +66,7 @@ describe 'Triggers', :js do
 
     it 'edit "legacy" trigger and save' do
       # Create new trigger without owner association, i.e. Legacy trigger
-      create(:ci_trigger, owner: nil, project: @project)
+      create(:ci_trigger, owner: user, project: @project).update_attribute(:owner, nil)
       visit project_settings_ci_cd_path(@project)
 
       # See if the trigger can be edited and description is blank
@@ -127,10 +127,75 @@ describe 'Triggers', :js do
   end
 
   describe 'show triggers workflow' do
+    before do
+      stub_feature_flags(use_legacy_pipeline_triggers: false)
+    end
+
     it 'contains trigger description placeholder' do
       expect(page.find('#trigger_description')['placeholder']).to eq 'Trigger description'
     end
 
+    it 'show "invalid" badge for legacy trigger' do
+      create(:ci_trigger, owner: user, project: @project).update_attribute(:owner, nil)
+      visit project_settings_ci_cd_path(@project)
+
+      expect(page.find('.triggers-list')).to have_content 'invalid'
+    end
+
+    it 'show "invalid" badge for trigger with owner having insufficient permissions' do
+      create(:ci_trigger, owner: guest_user, project: @project, description: trigger_title)
+      visit project_settings_ci_cd_path(@project)
+
+      # See if trigger without owner (i.e. legacy) shows "legacy" badge and is non-editable
+      expect(page.find('.triggers-list')).to have_content 'invalid'
+      expect(page.find('.triggers-list')).not_to have_selector('a[title="Edit"]')
+    end
+
+    it 'do not show "Edit" or full token for legacy trigger' do
+      create(:ci_trigger, owner: user, project: @project, description: trigger_title)
+        .update_attribute(:owner, nil)
+      visit project_settings_ci_cd_path(@project)
+
+      # See if trigger not owned shows only first few token chars and doesn't have copy-to-clipboard button
+      expect(page.find('.triggers-list')).to have_content(@project.triggers.first.token[0..3])
+      expect(page.find('.triggers-list')).not_to have_selector('button.btn-clipboard')
+
+      # See if trigger is non-editable
+      expect(page.find('.triggers-list')).not_to have_selector('a[title="Edit"]')
+    end
+
+    it 'do not show "Edit" or full token for not owned trigger' do
+      # Create trigger with user different from current_user
+      create(:ci_trigger, owner: user2, project: @project, description: trigger_title)
+      visit project_settings_ci_cd_path(@project)
+
+      # See if trigger not owned by current_user shows only first few token chars and doesn't have copy-to-clipboard button
+      expect(page.find('.triggers-list')).to have_content(@project.triggers.first.token[0..3])
+      expect(page.find('.triggers-list')).not_to have_selector('button.btn-clipboard')
+
+      # See if trigger owner name doesn't match with current_user and trigger is non-editable
+      expect(page.find('.triggers-list .trigger-owner')).not_to have_content user.name
+      expect(page.find('.triggers-list')).not_to have_selector('a[title="Edit"]')
+    end
+
+    it 'show "Edit" and full token for owned trigger' do
+      create(:ci_trigger, owner: user, project: @project, description: trigger_title)
+      visit project_settings_ci_cd_path(@project)
+
+      # See if trigger shows full token and has copy-to-clipboard button
+      expect(page.find('.triggers-list')).to have_content @project.triggers.first.token
+      expect(page.find('.triggers-list')).to have_selector('button.btn-clipboard')
+
+      # See if trigger owner name matches with current_user and is editable
+      expect(page.find('.triggers-list .trigger-owner')).to have_content user.name
+      expect(page.find('.triggers-list')).to have_selector('a[title="Edit"]')
+    end
+
+    context 'when :use_legacy_pipeline_triggers feature flag is enabled' do
+      before do
+        stub_feature_flags(use_legacy_pipeline_triggers: true)
+      end
+
       it 'show "legacy" badge for legacy trigger' do
         create(:ci_trigger, owner: nil, project: @project)
         visit project_settings_ci_cd_path(@project)
@@ -176,4 +241,5 @@ describe 'Triggers', :js do
         expect(page.find('.triggers-list')).to have_selector('a[title="Edit"]')
       end
     end
+  end
 end

spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb

@@ -10,7 +10,11 @@ describe Gitlab::Ci::Pipeline::Chain::Validate::Abilities do
 
   let(:command) do
     Gitlab::Ci::Pipeline::Chain::Command.new(
-      project: project, current_user: user, origin_ref: origin_ref, merge_request: merge_request)
+      project: project,
+      current_user: user,
+      origin_ref: origin_ref,
+      merge_request: merge_request,
+      trigger_request: trigger_request)
   end
 
   let(:step) { described_class.new(pipeline, command) }
@@ -18,6 +22,7 @@ describe Gitlab::Ci::Pipeline::Chain::Validate::Abilities do
   let(:ref) { 'master' }
   let(:origin_ref) { ref }
   let(:merge_request) { nil }
+  let(:trigger_request) { nil }
 
   shared_context 'detached merge request pipeline' do
     let(:merge_request) do
@@ -69,6 +74,43 @@ describe Gitlab::Ci::Pipeline::Chain::Validate::Abilities do
     end
   end
 
+  context 'when pipeline triggered by legacy trigger' do
+    let(:user) { nil }
+    let(:trigger_request) do
+      build_stubbed(:ci_trigger_request, trigger: build_stubbed(:ci_trigger, owner: nil))
+    end
+
+    context 'when :use_legacy_pipeline_triggers feature flag is enabled' do
+      before do
+        stub_feature_flags(use_legacy_pipeline_triggers: true)
+        step.perform!
+      end
+
+      it 'allows legacy triggers to create a pipeline' do
+        expect(pipeline).to be_valid
+      end
+
+      it 'does not break the chain' do
+        expect(step.break?).to eq false
+      end
+    end
+
+    context 'when :use_legacy_pipeline_triggers feature flag is disabled' do
+      before do
+        stub_feature_flags(use_legacy_pipeline_triggers: false)
+        step.perform!
+      end
+
+      it 'prevents legacy triggers from creating a pipeline' do
+        expect(pipeline.errors.to_a).to include /Trigger token is invalid/
+      end
+
+      it 'breaks the pipeline builder chain' do
+        expect(step.break?).to eq true
+      end
+    end
+  end
+
   describe '#allowed_to_create?' do
     subject { step.allowed_to_create? }
 

spec/lib/gitlab/import_export/project.json

@@ -6631,8 +6631,16 @@
       "id": 123,
       "token": "cdbfasdf44a5958c83654733449e585",
       "project_id": 5,
+      "owner_id": 1,
       "created_at": "2017-01-16T15:25:28.637Z",
       "updated_at": "2017-01-16T15:25:28.637Z"
+    },
+    {
+      "id": 456,
+      "token": "33a66349b5ad01fc00174af87804e40",
+      "project_id": 5,
+      "created_at": "2017-01-16T15:25:29.637Z",
+      "updated_at": "2017-01-16T15:25:29.637Z"
     }
   ],
   "deploy_keys": [],

spec/lib/gitlab/import_export/project_tree_restorer_spec.rb

@@ -32,6 +32,10 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
     end
 
     context 'JSON' do
+      before do
+        stub_feature_flags(use_legacy_pipeline_triggers: false)
+      end
+
       it 'restores models based on JSON' do
         expect(@restored_project_json).to be_truthy
       end
@@ -198,8 +202,9 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
       end
 
       context 'tokens are regenerated' do
-        it 'has a new CI trigger token' do
-          expect(Ci::Trigger.where(token: 'cdbfasdf44a5958c83654733449e585')).to be_empty
+        it 'has new CI trigger tokens' do
+          expect(Ci::Trigger.where(token: %w[cdbfasdf44a5958c83654733449e585 33a66349b5ad01fc00174af87804e40]))
+            .to be_empty
         end
 
         it 'has a new CI build token' do
@@ -212,7 +217,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
           expect(@project.merge_requests.size).to eq(9)
         end
 
-        it 'has the correct number of triggers' do
+        it 'only restores valid triggers' do
           expect(@project.triggers.size).to eq(1)
         end
 

spec/models/ci/trigger_spec.rb

@@ -54,19 +54,31 @@ describe Ci::Trigger do
   end
 
   describe '#can_access_project?' do
+    let(:owner) { create(:user) }
     let(:trigger) { create(:ci_trigger, owner: owner, project: project) }
 
     context 'when owner is blank' do
-      let(:owner) { nil }
+      before do
+        stub_feature_flags(use_legacy_pipeline_triggers: false)
+        trigger.update_attribute(:owner, nil)
+      end
+
+      subject { trigger.can_access_project? }
+
+      it { is_expected.to eq(false) }
+
+      context 'when :use_legacy_pipeline_triggers feature flag is enabled' do
+        before do
+          stub_feature_flags(use_legacy_pipeline_triggers: true)
+        end
 
         subject { trigger.can_access_project? }
 
         it { is_expected.to eq(true) }
       end
+    end
 
     context 'when owner is set' do
-      let(:owner) { create(:user) }
-
       subject { trigger.can_access_project? }
 
       context 'and is member of the project' do

spec/policies/ci/trigger_policy_spec.rb

@@ -3,52 +3,47 @@ require 'spec_helper'
 describe Ci::TriggerPolicy do
   let(:user) { create(:user) }
   let(:project) { create(:project) }
-  let(:trigger) { create(:ci_trigger, project: project, owner: owner) }
+  let(:trigger) { create(:ci_trigger, project: project, owner: create(:user)) }
 
-  let(:policies) do
-    described_class.new(user, trigger)
-  end
+  subject { described_class.new(user, trigger) }
 
-  shared_examples 'allows to admin and manage trigger' do
-    it 'does include ability to admin trigger' do
-      expect(policies).to be_allowed :admin_trigger
+  describe '#rules' do
+    context 'when owner is undefined' do
+      before do
+        stub_feature_flags(use_legacy_pipeline_triggers: false)
+        trigger.update_attribute(:owner, nil)
       end
 
-    it 'does include ability to manage trigger' do
-      expect(policies).to be_allowed :manage_trigger
-    end
+      context 'when user is maintainer of the project' do
+        before do
+          project.add_maintainer(user)
         end
 
-  shared_examples 'allows to manage trigger' do
-    it 'does not include ability to admin trigger' do
-      expect(policies).not_to be_allowed :admin_trigger
+        it { is_expected.to be_allowed(:manage_trigger) }
+        it { is_expected.not_to be_allowed(:admin_trigger) }
       end
 
-    it 'does include ability to manage trigger' do
-      expect(policies).to be_allowed :manage_trigger
-    end
+      context 'when user is developer of the project' do
+        before do
+          project.add_developer(user)
         end
 
-  shared_examples 'disallows to admin and manage trigger' do
-    it 'does not include ability to admin trigger' do
-      expect(policies).not_to be_allowed :admin_trigger
+        it { is_expected.not_to be_allowed(:manage_trigger) }
+        it { is_expected.not_to be_allowed(:admin_trigger) }
       end
 
-    it 'does not include ability to manage trigger' do
-      expect(policies).not_to be_allowed :manage_trigger
-    end
+      context 'when :use_legacy_pipeline_triggers feature flag is enabled' do
+        before do
+          stub_feature_flags(use_legacy_pipeline_triggers: true)
         end
 
-  describe '#rules' do
-    context 'when owner is undefined' do
-      let(:owner) { nil }
-
         context 'when user is maintainer of the project' do
           before do
             project.add_maintainer(user)
           end
 
-        it_behaves_like 'allows to admin and manage trigger'
+          it { is_expected.to be_allowed(:manage_trigger) }
+          it { is_expected.to be_allowed(:admin_trigger) }
         end
 
         context 'when user is developer of the project' do
@@ -56,35 +51,40 @@ describe Ci::TriggerPolicy do
             project.add_developer(user)
           end
 
-        it_behaves_like 'disallows to admin and manage trigger'
+          it { is_expected.not_to be_allowed(:manage_trigger) }
+          it { is_expected.not_to be_allowed(:admin_trigger) }
         end
 
         context 'when user is not member of the project' do
-        it_behaves_like 'disallows to admin and manage trigger'
+          it { is_expected.not_to be_allowed(:manage_trigger) }
+          it { is_expected.not_to be_allowed(:admin_trigger) }
+        end
       end
     end
 
     context 'when owner is an user' do
-      let(:owner) { user }
+      before do
+        trigger.update!(owner: user)
+      end
 
       context 'when user is maintainer of the project' do
         before do
           project.add_maintainer(user)
         end
 
-        it_behaves_like 'allows to admin and manage trigger'
+        it { is_expected.to be_allowed(:manage_trigger) }
+        it { is_expected.to be_allowed(:admin_trigger) }
       end
     end
 
     context 'when owner is another user' do
-      let(:owner) { create(:user) }
-
       context 'when user is maintainer of the project' do
         before do
           project.add_maintainer(user)
         end
 
-        it_behaves_like 'allows to manage trigger'
+        it { is_expected.to be_allowed(:manage_trigger) }
+        it { is_expected.not_to be_allowed(:admin_trigger) }
       end
 
       context 'when user is developer of the project' do
@@ -92,11 +92,13 @@ describe Ci::TriggerPolicy do
           project.add_developer(user)
         end
 
-        it_behaves_like 'disallows to admin and manage trigger'
+        it { is_expected.not_to be_allowed(:manage_trigger) }
+        it { is_expected.not_to be_allowed(:admin_trigger) }
       end
 
       context 'when user is not member of the project' do
-        it_behaves_like 'disallows to admin and manage trigger'
+        it { is_expected.not_to be_allowed(:manage_trigger) }
+        it { is_expected.not_to be_allowed(:admin_trigger) }
       end
     end
   end