Merge branch '29772-completely-removing-use_legacy_pipeline_triggers' into 'master'

Resolve "Remove feature flag :use_legacy_pipeline_triggers"

See merge request gitlab-org/gitlab!21732

app/models/ci/trigger.rb

@@ -11,7 +11,7 @@ module Ci
     has_many :trigger_requests
 
     validates :token, presence: true, uniqueness: true
-    validates :owner, presence: true, unless: :supports_legacy_tokens?
+    validates :owner, presence: true
 
     before_validation :set_default_values
 
@@ -31,17 +31,8 @@ module Ci
       token[0...4] if token.present?
     end
 
-    def legacy?
-      self.owner_id.blank?
-    end
-
-    def supports_legacy_tokens?
-      Feature.enabled?(:use_legacy_pipeline_triggers, project)
-    end
-
     def can_access_project?
-      supports_legacy_tokens? && legacy? ||
-        Ability.allowed?(self.owner, :create_build, project)
+      Ability.allowed?(self.owner, :create_build, project)
     end
   end
 end

app/policies/ci/trigger_policy.rb

@@ -5,13 +5,12 @@ module Ci
     delegate { @subject.project }
 
     with_options scope: :subject, score: 0
-    condition(:legacy) { @subject.supports_legacy_tokens? && @subject.legacy? }
 
     with_score 0
     condition(:is_owner) { @user && @subject.owner_id == @user.id }
 
     rule { ~can?(:admin_build) }.prevent :admin_trigger
-    rule { legacy | is_owner }.enable :admin_trigger
+    rule { is_owner }.enable :admin_trigger
 
     rule { can?(:admin_build) }.enable :manage_trigger
   end

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

-- 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.
-    %br
-    = succeed '.' do
-      Learn more in the
-      = link_to 'triggers documentation', help_page_path('ci/triggers/README'), target: '_blank'

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

 .row.prepend-top-default.append-bottom-default.triggers-container
   .col-lg-12
-    = render "projects/triggers/content"
     .card
       .card-header
         Manage your project's triggers

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

@@ -7,12 +7,7 @@
       %span= trigger.short_token
 
     .label-container
-      - if trigger.legacy?
-        - if trigger.supports_legacy_tokens?
-          %span.badge.badge-primary.has-tooltip{ title: "Trigger makes use of deprecated functionality" } legacy
-        - 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?
+      - unless trigger.can_access_project?
         %span.badge.badge-danger.has-tooltip{ title: "Trigger user has insufficient permissions to project" } invalid
 
   %td

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

 - page_title "Trigger"
 
 .row.prepend-top-default.append-bottom-default
-  .col-lg-3
-    = render "content"
-  .col-lg-9
+  .col-lg-12
     %h4.prepend-top-0
       Update trigger
     = render "form", btn_text: "Save trigger"

changelogs/unreleased/29772-completely-removing-use_legacy_pipeline_triggers.yml

+---
+title: Remove feature flag 'use_legacy_pipeline_triggers' and remove legacy tokens
+merge_request: 21732
+author:
+type: removed

db/post_migrate/20191204114127_delete_legacy_triggers.rb

+# frozen_string_literal: true
+
+class DeleteLegacyTriggers < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def up
+    execute <<~SQL
+      DELETE FROM ci_triggers WHERE owner_id IS NULL
+    SQL
+
+    change_column_null :ci_triggers, :owner_id, false
+  end
+
+  def down
+    change_column_null :ci_triggers, :owner_id, true
+  end
+end

db/schema.rb

@@ -994,7 +994,7 @@ ActiveRecord::Schema.define(version: 2020_01_08_233040) do
     t.datetime "created_at"
     t.datetime "updated_at"
     t.integer "project_id"
-    t.integer "owner_id"
+    t.integer "owner_id", null: false
     t.string "description"
     t.string "ref"
     t.index ["owner_id"], name: "index_ci_triggers_on_owner_id"

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

@@ -22,12 +22,6 @@ 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,16 +14,12 @@ 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')
+              unless allowed_to_create_pipeline?
+                return error('Insufficient permissions to create a new pipeline')
               end
 
-              unless allowed_to_trigger_pipeline?
-                if can?(current_user, :create_pipeline, project)
-                  return error("Insufficient permissions for protected ref '#{command.ref}'")
-                else
-                  return error('Insufficient permissions to create a new pipeline')
-                end
+              unless allowed_to_write_ref?
+                return error("Insufficient permissions for protected ref '#{command.ref}'")
               end
             end
 
@@ -31,17 +27,13 @@ module Gitlab
               @pipeline.errors.any?
             end
 
-            def allowed_to_trigger_pipeline?
-              if current_user
-                allowed_to_create?
-              else # legacy triggers don't have a corresponding user
-                !@command.protected_ref?
-              end
-            end
+            private
 
-            def allowed_to_create?
-              return unless can?(current_user, :create_pipeline, project)
+            def allowed_to_create_pipeline?
+              can?(current_user, :create_pipeline, project)
+            end
 
+            def allowed_to_write_ref?
               access = Gitlab::UserAccess.new(current_user, project: project)
 
               if @command.branch_exists?

lib/gitlab/import_export/relation_factory.rb

@@ -100,9 +100,6 @@ 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
 
         object = generate_imported_object
@@ -359,10 +356,6 @@ 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!
         if UNIQUE_RELATIONS.include?(@relation_name)
           unique_relation_object = relation_class.find_or_create_by(project_id: @project.id)

spec/features/triggers_spec.rb

@@ -65,22 +65,6 @@ describe 'Triggers', :js do
       expect(page.find('.triggers-list')).to have_content new_trigger_title
       expect(page.find('.triggers-list .trigger-owner')).to have_content user.name
     end
-
-    it 'edit "legacy" trigger and save' do
-      # Create new trigger without owner association, i.e. Legacy trigger
-      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
-      find('a[title="Edit"]').send_keys(:return)
-      expect(page.find('#trigger_description').value).to have_content ''
-
-      # See if trigger can be updated with description and saved successfully
-      fill_in 'trigger_description', with: new_trigger_title
-      click_button 'Save trigger'
-      expect(page.find('.flash-notice')).to have_content 'Trigger was successfully updated.'
-      expect(page.find('.triggers-list')).to have_content new_trigger_title
-    end
   end
 
   describe 'trigger "Revoke" workflow' do
@@ -106,43 +90,18 @@ 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)
@@ -169,56 +128,5 @@ describe 'Triggers', :js do
       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)
-
-        # See if trigger without owner (i.e. legacy) shows "legacy" badge and is editable
-        expect(page.find('.triggers-list')).to have_content 'legacy'
-        expect(page.find('.triggers-list')).to have_selector('a[title="Edit"]')
-      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 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
-    end
   end
 end

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

@@ -76,45 +76,8 @@ 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? }
+  describe '#allowed_to_write_ref?' do
+    subject { step.send(:allowed_to_write_ref?) }
 
     context 'when user is a developer' do
       before do

spec/lib/gitlab/import_export/project_tree_restorer_spec.rb

@@ -36,10 +36,6 @@ 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

spec/migrations/20191204114127_delete_legacy_triggers_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20191204114127_delete_legacy_triggers.rb')
+
+describe DeleteLegacyTriggers, :migration, schema: 2019_11_25_140458 do
+  let(:ci_trigger_table) { table(:ci_triggers) }
+  let(:user) { table(:users).create!(name: 'test', email: 'test@example.com', projects_limit: 1) }
+
+  before do
+    @trigger_with_user = ci_trigger_table.create!(owner_id: user.id)
+    ci_trigger_table.create!(owner_id: nil)
+    ci_trigger_table.create!(owner_id: nil)
+  end
+
+  it 'removes legacy triggers which has null owner_id' do
+    expect do
+      migrate!
+    end.to change(ci_trigger_table, :count).by(-2)
+
+    expect(ci_trigger_table.all).to eq([@trigger_with_user])
+  end
+end

spec/models/ci/trigger_spec.rb

@@ -11,6 +11,10 @@ describe Ci::Trigger do
     it { is_expected.to have_many(:trigger_requests) }
   end
 
+  describe 'validations' do
+    it { is_expected.to validate_presence_of(:owner) }
+  end
+
   describe 'before_validation' do
     it 'sets an random token if none provided' do
       trigger = create(:ci_trigger_without_token, project: project)
@@ -35,63 +39,22 @@ describe Ci::Trigger do
     end
   end
 
-  describe '#legacy?' do
-    let(:trigger) { create(:ci_trigger, owner: owner, project: project) }
-
-    subject { trigger }
-
-    context 'when owner is blank' do
-      let(:owner) { nil }
-
-      it { is_expected.to be_legacy }
-    end
-
-    context 'when owner is set' do
-      let(:owner) { create(:user) }
-
-      it { is_expected.not_to be_legacy }
-    end
-  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
+    subject { trigger.can_access_project? }
+
+    context 'and is member of the project' do
       before do
-        stub_feature_flags(use_legacy_pipeline_triggers: false)
-        trigger.update_attribute(:owner, nil)
+        project.add_developer(owner)
       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
+      it { is_expected.to eq(true) }
     end
 
-    context 'when owner is set' do
-      subject { trigger.can_access_project? }
-
-      context 'and is member of the project' do
-        before do
-          project.add_developer(owner)
-        end
-
-        it { is_expected.to eq(true) }
-      end
-
-      context 'and is not member of the project' do
-        it { is_expected.to eq(false) }
-      end
+    context 'and is not member of the project' do
+      it { is_expected.to eq(false) }
     end
   end
 end

spec/policies/ci/trigger_policy_spec.rb

@@ -10,60 +10,6 @@ describe Ci::TriggerPolicy do
   subject { described_class.new(user, 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
-
-      context 'when user is maintainer of the project' do
-        before do
-          project.add_maintainer(user)
-        end
-
-        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
-        before do
-          project.add_developer(user)
-        end
-
-        it { is_expected.not_to be_allowed(:manage_trigger) }
-        it { is_expected.not_to be_allowed(:admin_trigger) }
-      end
-
-      context 'when :use_legacy_pipeline_triggers feature flag is enabled' do
-        before do
-          stub_feature_flags(use_legacy_pipeline_triggers: true)
-        end
-
-        context 'when user is maintainer of the project' do
-          before do
-            project.add_maintainer(user)
-          end
-
-          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
-          before do
-            project.add_developer(user)
-          end
-
-          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 { 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
       before do
         trigger.update!(owner: user)

spec/requests/api/triggers_spec.rb

@@ -87,22 +87,6 @@ describe API::Triggers do
           expect(pipeline.variables.map { |v| { v.key => v.value } }.last).to eq(variables)
         end
       end
-
-      context 'when legacy trigger' do
-        before do
-          trigger.update(owner: nil)
-        end
-
-        it 'creates pipeline' do
-          post api("/projects/#{project.id}/trigger/pipeline"), params: options.merge(ref: 'master')
-
-          expect(response).to have_gitlab_http_status(201)
-          expect(json_response).to include('id' => pipeline.id)
-          pipeline.builds.reload
-          expect(pipeline.builds.pending.size).to eq(2)
-          expect(pipeline.builds.size).to eq(5)
-        end
-      end
     end
 
     context 'when triggering a pipeline from a trigger token' do

spec/services/ci/create_pipeline_service_spec.rb

@@ -1123,21 +1123,6 @@ describe Ci::CreatePipelineService do
       it_behaves_like 'when ref is protected'
     end
 
-    context 'when ref is not protected' do
-      context 'when trigger belongs to no one' do
-        let(:user) {}
-        let(:trigger) { create(:ci_trigger, owner: nil) }
-        let(:trigger_request) { create(:ci_trigger_request, trigger: trigger) }
-        let(:pipeline) { execute_service(trigger_request: trigger_request) }
-
-        it 'creates an unprotected pipeline' do
-          expect(pipeline).to be_persisted
-          expect(pipeline).not_to be_protected
-          expect(Ci::Pipeline.count).to eq(1)
-        end
-      end
-    end
-
     context 'when pipeline is running for a tag' do
       before do
         config = YAML.dump(test: { script: 'test', only: ['branches'] },