Commit d9b9aace authored by Kamil Trzciński's avatar Kamil Trzciński

Merge branch 'remove-support-for-legacy-pipeline-triggers' into 'master'

Remove support for legacy pipeline triggers

Closes #30231

See merge request gitlab-org/gitlab-ce!30133
parents 1e99c1b0 c2396ce0
......@@ -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
......@@ -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 }
......
%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'
- 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'
......@@ -8,8 +8,11 @@
.label-container
- if trigger.legacy?
%span.badge.badge-primary.has-tooltip{ title: "Trigger makes use of deprecated functionality" } legacy
- if !trigger.can_access_project?
- 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?
%span.badge.badge-danger.has-tooltip{ title: "Trigger user has insufficient permissions to project" } invalid
%td
......
---
title: Remove support for legacy pipeline triggers
merge_request: 30133
author:
type: removed
......@@ -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)
......
......@@ -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}'")
......
......@@ -28,7 +28,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
......@@ -78,6 +78,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
......@@ -278,6 +281,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
......
......@@ -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,17 +127,19 @@ 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 "legacy" badge for legacy trigger' do
create(:ci_trigger, owner: nil, project: @project)
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)
# 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"]')
expect(page.find('.triggers-list')).to have_content 'invalid'
end
it 'show "invalid" badge for trigger with owner having insufficient permissions' do
......@@ -149,6 +151,19 @@ describe 'Triggers', :js do
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)
......@@ -175,5 +190,56 @@ 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
......@@ -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? }
......
......@@ -6630,8 +6630,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": [],
......
......@@ -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
......
......@@ -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(true) }
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
......
......@@ -3,52 +3,24 @@ 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
shared_examples 'allows to admin and manage trigger' do
it 'does include ability to admin trigger' do
expect(policies).to be_allowed :admin_trigger
end
it 'does include ability to manage trigger' do
expect(policies).to be_allowed :manage_trigger
end
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
end
it 'does include ability to manage trigger' do
expect(policies).to be_allowed :manage_trigger
end
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
end
it 'does not include ability to manage trigger' do
expect(policies).not_to be_allowed :manage_trigger
end
end
subject { described_class.new(user, trigger) }
describe '#rules' do
context 'when owner is undefined' do
let(:owner) { nil }
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_behaves_like 'allows to admin and 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
......@@ -56,35 +28,63 @@ 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'
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
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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment