Commit 17af6be9 authored by Fabio Pitino's avatar Fabio Pitino

Merge branch 'ci-validate-uniquness-of-pipeline-variables' into 'master'

Validate the uniqueness of pipeline variables

See merge request gitlab-org/gitlab!66556
parents 51d976ae a143ad44
...@@ -5,9 +5,6 @@ module Gitlab ...@@ -5,9 +5,6 @@ module Gitlab
module Pipeline module Pipeline
module Chain module Chain
class Build < Chain::Base class Build < Chain::Base
include Gitlab::Allowable
include Chain::Helpers
def perform! def perform!
@pipeline.assign_attributes( @pipeline.assign_attributes(
source: @command.source, source: @command.source,
...@@ -23,35 +20,12 @@ module Gitlab ...@@ -23,35 +20,12 @@ module Gitlab
pipeline_schedule: @command.schedule, pipeline_schedule: @command.schedule,
merge_request: @command.merge_request, merge_request: @command.merge_request,
external_pull_request: @command.external_pull_request, external_pull_request: @command.external_pull_request,
locked: @command.project.default_pipeline_lock, locked: @command.project.default_pipeline_lock)
variables_attributes: variables_attributes
)
end end
def break? def break?
@pipeline.errors.any? @pipeline.errors.any?
end end
private
def variables_attributes
variables = Array(@command.variables_attributes)
# We allow parent pipelines to pass variables to child pipelines since
# these variables are coming from internal configurations. We will check
# permissions to :set_pipeline_variables when those are injected upstream,
# to the parent pipeline.
# In other scenarios (e.g. multi-project pipelines or run pipeline via UI)
# the variables are provided from the outside and those should be guarded.
return variables if @command.creates_child_pipeline?
if variables.present? && !can?(@command.current_user, :set_pipeline_variables, @command.project)
error("Insufficient permissions to set pipeline variables")
variables = []
end
variables
end
end end
end end
end end
......
...@@ -6,7 +6,25 @@ module Gitlab ...@@ -6,7 +6,25 @@ module Gitlab
module Chain module Chain
class Build class Build
class Associations < Chain::Base class Associations < Chain::Base
include Gitlab::Allowable
include Chain::Helpers
def perform! def perform!
assign_pipeline_variables
assign_source_pipeline
end
def break?
@pipeline.errors.any?
end
private
def assign_pipeline_variables
@pipeline.variables_attributes = variables_attributes
end
def assign_source_pipeline
return unless @command.bridge return unless @command.bridge
@pipeline.build_source_pipeline( @pipeline.build_source_pipeline(
...@@ -17,8 +35,45 @@ module Gitlab ...@@ -17,8 +35,45 @@ module Gitlab
) )
end end
def break? def variables_attributes
false variables = Array(@command.variables_attributes)
variables = apply_permissions(variables)
validate_uniqueness(variables)
end
def apply_permissions(variables)
# We allow parent pipelines to pass variables to child pipelines since
# these variables are coming from internal configurations. We will check
# permissions to :set_pipeline_variables when those are injected upstream,
# to the parent pipeline.
# In other scenarios (e.g. multi-project pipelines or run pipeline via UI)
# the variables are provided from the outside and those should be guarded.
return variables if @command.creates_child_pipeline?
if variables.present? && !can?(@command.current_user, :set_pipeline_variables, @command.project)
error("Insufficient permissions to set pipeline variables")
variables = []
end
variables
end
def validate_uniqueness(variables)
duplicated_keys = variables
.map { |var| var[:key] }
.tally
.filter_map { |key, count| key if count > 1 }
if duplicated_keys.empty?
variables
else
error(duplicate_variables_message(duplicated_keys), config_error: true)
[]
end
end
def duplicate_variables_message(keys)
"Duplicate variable #{'name'.pluralize(keys.size)}: #{keys.join(', ')}"
end end
end end
end end
......
...@@ -3,10 +3,16 @@ ...@@ -3,10 +3,16 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe Gitlab::Ci::Pipeline::Chain::Build::Associations do RSpec.describe Gitlab::Ci::Pipeline::Chain::Build::Associations do
let(:project) { create(:project, :repository) } let_it_be_with_reload(:project) { create(:project, :repository) }
let(:user) { create(:user, developer_projects: [project]) } let_it_be(:user) { create(:user, developer_projects: [project]) }
let(:pipeline) { Ci::Pipeline.new } let(:pipeline) { Ci::Pipeline.new }
let(:step) { described_class.new(pipeline, command) } let(:bridge) { nil }
let(:variables_attributes) do
[{ key: 'first', secret_value: 'world' },
{ key: 'second', secret_value: 'second_world' }]
end
let(:command) do let(:command) do
Gitlab::Ci::Pipeline::Chain::Command.new( Gitlab::Ci::Pipeline::Chain::Command.new(
...@@ -20,7 +26,26 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Build::Associations do ...@@ -20,7 +26,26 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Build::Associations do
merge_request: nil, merge_request: nil,
project: project, project: project,
current_user: user, current_user: user,
bridge: bridge) bridge: bridge,
variables_attributes: variables_attributes)
end
let(:step) { described_class.new(pipeline, command) }
shared_examples 'breaks the chain' do
it 'returns true' do
step.perform!
expect(step.break?).to be true
end
end
shared_examples 'does not break the chain' do
it 'returns false' do
step.perform!
expect(step.break?).to be false
end
end end
context 'when a bridge is passed in to the pipeline creation' do context 'when a bridge is passed in to the pipeline creation' do
...@@ -37,26 +62,83 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Build::Associations do ...@@ -37,26 +62,83 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Build::Associations do
) )
end end
it 'never breaks the chain' do it_behaves_like 'does not break the chain'
end
context 'when a bridge is not passed in to the pipeline creation' do
it 'leaves the source pipeline empty' do
step.perform! step.perform!
expect(step.break?).to eq(false) expect(pipeline.source_pipeline).to be_nil
end end
it_behaves_like 'does not break the chain'
end end
context 'when a bridge is not passed in to the pipeline creation' do it 'sets pipeline variables' do
let(:bridge) { nil } step.perform!
it 'leaves the source pipeline empty' do expect(pipeline.variables.map { |var| var.slice(:key, :secret_value) })
.to eq variables_attributes.map(&:with_indifferent_access)
end
context 'when project setting restrict_user_defined_variables is enabled' do
before do
project.update!(restrict_user_defined_variables: true)
end
context 'when user is developer' do
it_behaves_like 'breaks the chain'
it 'returns an error on variables_attributes', :aggregate_failures do
step.perform! step.perform!
expect(pipeline.source_pipeline).to be_nil expect(pipeline.errors.full_messages).to eq(['Insufficient permissions to set pipeline variables'])
expect(pipeline.variables).to be_empty
end
context 'when variables_attributes is not specified' do
let(:variables_attributes) { nil }
it_behaves_like 'does not break the chain'
it 'assigns empty variables' do
step.perform!
expect(pipeline.variables).to be_empty
end
end end
end
context 'when user is maintainer' do
before do
project.add_maintainer(user)
end
it_behaves_like 'does not break the chain'
it 'assigns variables_attributes' do
step.perform!
expect(pipeline.variables.map { |var| var.slice(:key, :secret_value) })
.to eq variables_attributes.map(&:with_indifferent_access)
end
end
end
context 'with duplicate pipeline variables' do
let(:variables_attributes) do
[{ key: 'first', secret_value: 'world' },
{ key: 'first', secret_value: 'second_world' }]
end
it_behaves_like 'breaks the chain'
it 'never breaks the chain' do it 'returns an error for variables_attributes' do
step.perform! step.perform!
expect(step.break?).to eq(false) expect(pipeline.errors.full_messages).to eq(['Duplicate variable name: first'])
expect(pipeline.variables).to be_empty
end end
end end
end end
...@@ -8,11 +8,6 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Build do ...@@ -8,11 +8,6 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Build do
let(:pipeline) { Ci::Pipeline.new } let(:pipeline) { Ci::Pipeline.new }
let(:variables_attributes) do
[{ key: 'first', secret_value: 'world' },
{ key: 'second', secret_value: 'second_world' }]
end
let(:command) do let(:command) do
Gitlab::Ci::Pipeline::Chain::Command.new( Gitlab::Ci::Pipeline::Chain::Command.new(
source: :push, source: :push,
...@@ -24,13 +19,17 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Build do ...@@ -24,13 +19,17 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Build do
schedule: nil, schedule: nil,
merge_request: nil, merge_request: nil,
project: project, project: project,
current_user: user, current_user: user)
variables_attributes: variables_attributes)
end end
let(:step) { described_class.new(pipeline, command) } let(:step) { described_class.new(pipeline, command) }
shared_examples 'builds pipeline' do it 'does not break the chain' do
step.perform!
expect(step.break?).to be false
end
it 'builds a pipeline with the expected attributes' do it 'builds a pipeline with the expected attributes' do
step.perform! step.perform!
...@@ -41,84 +40,6 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Build do ...@@ -41,84 +40,6 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Build do
expect(pipeline.user).to eq user expect(pipeline.user).to eq user
expect(pipeline.project).to eq project expect(pipeline.project).to eq project
end end
end
shared_examples 'breaks the chain' do
it 'returns true' do
step.perform!
expect(step.break?).to be true
end
end
shared_examples 'does not break the chain' do
it 'returns false' do
step.perform!
expect(step.break?).to be false
end
end
before do
stub_ci_pipeline_yaml_file(gitlab_ci_yaml)
end
it_behaves_like 'does not break the chain'
it_behaves_like 'builds pipeline'
it 'sets pipeline variables' do
step.perform!
expect(pipeline.variables.map { |var| var.slice(:key, :secret_value) })
.to eq variables_attributes.map(&:with_indifferent_access)
end
context 'when project setting restrict_user_defined_variables is enabled' do
before do
project.update!(restrict_user_defined_variables: true)
end
context 'when user is developer' do
it_behaves_like 'breaks the chain'
it_behaves_like 'builds pipeline'
it 'returns an error on variables_attributes', :aggregate_failures do
step.perform!
expect(pipeline.errors.full_messages).to eq(['Insufficient permissions to set pipeline variables'])
expect(pipeline.variables).to be_empty
end
context 'when variables_attributes is not specified' do
let(:variables_attributes) { nil }
it_behaves_like 'does not break the chain'
it_behaves_like 'builds pipeline'
it 'assigns empty variables' do
step.perform!
expect(pipeline.variables).to be_empty
end
end
end
context 'when user is maintainer' do
before do
project.add_maintainer(user)
end
it_behaves_like 'does not break the chain'
it_behaves_like 'builds pipeline'
it 'assigns variables_attributes' do
step.perform!
expect(pipeline.variables.map { |var| var.slice(:key, :secret_value) })
.to eq variables_attributes.map(&:with_indifferent_access)
end
end
end
it 'returns a valid pipeline' do it 'returns a valid pipeline' do
step.perform! step.perform!
......
...@@ -1248,19 +1248,50 @@ RSpec.describe Ci::CreatePipelineService do ...@@ -1248,19 +1248,50 @@ RSpec.describe Ci::CreatePipelineService do
end end
context 'when pipeline variables are specified' do context 'when pipeline variables are specified' do
subject(:pipeline) { execute_service(variables_attributes: variables_attributes).payload }
context 'with valid pipeline variables' do
let(:variables_attributes) do let(:variables_attributes) do
[{ key: 'first', secret_value: 'world' }, [{ key: 'first', secret_value: 'world' },
{ key: 'second', secret_value: 'second_world' }] { key: 'second', secret_value: 'second_world' }]
end end
subject(:pipeline) { execute_service(variables_attributes: variables_attributes).payload }
it 'creates a pipeline with specified variables' do it 'creates a pipeline with specified variables' do
expect(pipeline.variables.map { |var| var.slice(:key, :secret_value) }) expect(pipeline.variables.map { |var| var.slice(:key, :secret_value) })
.to eq variables_attributes.map(&:with_indifferent_access) .to eq variables_attributes.map(&:with_indifferent_access)
end end
end end
context 'with duplicate pipeline variables' do
let(:variables_attributes) do
[{ key: 'hello', secret_value: 'world' },
{ key: 'hello', secret_value: 'second_world' }]
end
it 'fails to create the pipeline' do
expect(pipeline).to be_failed
expect(pipeline.variables).to be_empty
expect(pipeline.errors[:base]).to eq(['Duplicate variable name: hello'])
end
end
context 'with more than one duplicate pipeline variable' do
let(:variables_attributes) do
[{ key: 'hello', secret_value: 'world' },
{ key: 'hello', secret_value: 'second_world' },
{ key: 'single', secret_value: 'variable' },
{ key: 'other', secret_value: 'value' },
{ key: 'other', secret_value: 'other value' }]
end
it 'fails to create the pipeline' do
expect(pipeline).to be_failed
expect(pipeline.variables).to be_empty
expect(pipeline.errors[:base]).to eq(['Duplicate variable names: hello, other'])
end
end
end
context 'when pipeline has a job with environment' do context 'when pipeline has a job with environment' do
let(:pipeline) { execute_service.payload } let(:pipeline) { execute_service.payload }
......
...@@ -103,6 +103,17 @@ RSpec.describe Ci::PipelineTriggerService do ...@@ -103,6 +103,17 @@ RSpec.describe Ci::PipelineTriggerService do
end end
end end
context 'when params have duplicate variables' do
let(:params) { { token: trigger.token, ref: 'master', variables: variables } }
let(:variables) { { 'TRIGGER_PAYLOAD' => 'duplicate value' } }
it 'creates a failed pipeline without variables' do
expect { result }.to change { Ci::Pipeline.count }
expect(result).to be_error
expect(result.message[:base]).to eq(['Duplicate variable name: TRIGGER_PAYLOAD'])
end
end
it_behaves_like 'detecting an unprocessable pipeline trigger' it_behaves_like 'detecting an unprocessable pipeline trigger'
end end
...@@ -201,6 +212,17 @@ RSpec.describe Ci::PipelineTriggerService do ...@@ -201,6 +212,17 @@ RSpec.describe Ci::PipelineTriggerService do
end end
end end
context 'when params have duplicate variables' do
let(:params) { { token: job.token, ref: 'master', variables: variables } }
let(:variables) { { 'TRIGGER_PAYLOAD' => 'duplicate value' } }
it 'creates a failed pipeline without variables' do
expect { result }.to change { Ci::Pipeline.count }
expect(result).to be_error
expect(result.message[:base]).to eq(['Duplicate variable name: TRIGGER_PAYLOAD'])
end
end
it_behaves_like 'detecting an unprocessable pipeline trigger' it_behaves_like 'detecting an unprocessable pipeline trigger'
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