Validate the uniqueness of pipeline variables

The model validations are all executed before we start inserting into
the database, so the scoped uniqueness validaiton does not work.

Changelog: fixed

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

@@ -5,9 +5,6 @@ module Gitlab
     module Pipeline
       module Chain
         class Build < Chain::Base
-          include Gitlab::Allowable
-          include Chain::Helpers
-
           def perform!
             @pipeline.assign_attributes(
               source: @command.source,
@@ -23,35 +20,12 @@ module Gitlab
               pipeline_schedule: @command.schedule,
               merge_request: @command.merge_request,
               external_pull_request: @command.external_pull_request,
-              locked: @command.project.default_pipeline_lock,
-              variables_attributes: variables_attributes
-            )
+              locked: @command.project.default_pipeline_lock)
           end
 
           def break?
             @pipeline.errors.any?
           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

lib/gitlab/ci/pipeline/chain/build/associations.rb

@@ -6,7 +6,25 @@ module Gitlab
       module Chain
         class Build
           class Associations < Chain::Base
+            include Gitlab::Allowable
+            include Chain::Helpers
+
             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
 
               @pipeline.build_source_pipeline(
@@ -17,8 +35,45 @@ module Gitlab
               )
             end
 
-            def break?
-              false
+            def variables_attributes
+              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

spec/lib/gitlab/ci/pipeline/chain/build/associations_spec.rb

@@ -3,10 +3,16 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Ci::Pipeline::Chain::Build::Associations do
-  let(:project) { create(:project, :repository) }
-  let(:user) { create(:user, developer_projects: [project]) }
+  let_it_be_with_reload(:project) { create(:project, :repository) }
+  let_it_be(:user) { create(:user, developer_projects: [project]) }
+
   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
     Gitlab::Ci::Pipeline::Chain::Command.new(
@@ -20,7 +26,26 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Build::Associations do
       merge_request: nil,
       project: project,
       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
 
   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
       )
     end
 
-    it 'never breaks the chain' do
-      step.perform!
-
-      expect(step.break?).to eq(false)
-    end
+    it_behaves_like 'does not break the chain'
   end
 
   context 'when a bridge is not passed in to the pipeline creation' do
-    let(:bridge) { nil }
-
     it 'leaves the source pipeline empty' do
       step.perform!
 
       expect(pipeline.source_pipeline).to be_nil
     end
 
-    it 'never breaks the chain' do
+    it_behaves_like 'does not break the chain'
+  end
+
+  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 '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 '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 '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 'returns an error for variables_attributes' do
       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

spec/lib/gitlab/ci/pipeline/chain/build_spec.rb

@@ -8,11 +8,6 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Build do
 
   let(:pipeline) { Ci::Pipeline.new }
 
-  let(:variables_attributes) do
-    [{ key: 'first', secret_value: 'world' },
-     { key: 'second', secret_value: 'second_world' }]
-  end
-
   let(:command) do
     Gitlab::Ci::Pipeline::Chain::Command.new(
       source: :push,
@@ -24,100 +19,26 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Build do
       schedule: nil,
       merge_request: nil,
       project: project,
-      current_user: user,
-      variables_attributes: variables_attributes)
+      current_user: user)
   end
 
   let(:step) { described_class.new(pipeline, command) }
 
-  shared_examples 'builds pipeline' do
-    it 'builds a pipeline with the expected attributes' do
-      step.perform!
-
-      expect(pipeline.sha).not_to be_empty
-      expect(pipeline.sha).to eq project.commit.id
-      expect(pipeline.ref).to eq 'master'
-      expect(pipeline.tag).to be false
-      expect(pipeline.user).to eq user
-      expect(pipeline.project).to eq project
-    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
+  it 'does not break the chain' do
     step.perform!
 
-    expect(pipeline.variables.map { |var| var.slice(:key, :secret_value) })
-      .to eq variables_attributes.map(&:with_indifferent_access)
+    expect(step.break?).to be false
   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!
+  it 'builds a pipeline with the expected attributes' do
+    step.perform!
 
-        expect(pipeline.variables.map { |var| var.slice(:key, :secret_value) })
-          .to eq variables_attributes.map(&:with_indifferent_access)
-      end
-    end
+    expect(pipeline.sha).not_to be_empty
+    expect(pipeline.sha).to eq project.commit.id
+    expect(pipeline.ref).to eq 'master'
+    expect(pipeline.tag).to be false
+    expect(pipeline.user).to eq user
+    expect(pipeline.project).to eq project
   end
 
   it 'returns a valid pipeline' do

spec/services/ci/create_pipeline_service_spec.rb

@@ -1277,16 +1277,47 @@ RSpec.describe Ci::CreatePipelineService do
     end
 
     context 'when pipeline variables are specified' do
-      let(:variables_attributes) do
-        [{ key: 'first', secret_value: 'world' },
-         { key: 'second', secret_value: 'second_world' }]
+      subject(:pipeline) { execute_service(variables_attributes: variables_attributes).payload }
+
+      context 'with valid pipeline variables' do
+        let(:variables_attributes) do
+          [{ key: 'first', secret_value: 'world' },
+           { key: 'second', secret_value: 'second_world' }]
+        end
+
+        it 'creates a pipeline with specified variables' do
+          expect(pipeline.variables.map { |var| var.slice(:key, :secret_value) })
+            .to eq variables_attributes.map(&:with_indifferent_access)
+        end
       end
 
-      subject(:pipeline) { execute_service(variables_attributes: variables_attributes).payload }
+      context 'with duplicate pipeline variables' do
+        let(:variables_attributes) do
+          [{ key: 'hello', secret_value: 'world' },
+           { key: 'hello', secret_value: 'second_world' }]
+        end
 
-      it 'creates a pipeline with specified variables' do
-        expect(pipeline.variables.map { |var| var.slice(:key, :secret_value) })
-          .to eq variables_attributes.map(&:with_indifferent_access)
+        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
 

spec/services/ci/pipeline_trigger_service_spec.rb

@@ -103,6 +103,17 @@ RSpec.describe Ci::PipelineTriggerService do
             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'
         end
 
@@ -201,6 +212,17 @@ RSpec.describe Ci::PipelineTriggerService do
             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'
         end