Use full ref when possible to avoid ambiguity

7 files changed, 70 insertions, 8 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index d86a6eceb59..f5c21481f63 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -10,6 +10,7 @@ module Ci
     include Importable
     include Gitlab::Utils::StrongMemoize
     include Deployable
+    include HasRef
 
     belongs_to :project, inverse_of: :builds
     belongs_to :runner
@@ -640,11 +641,11 @@ module Ci
     def secret_group_variables
       return [] unless project.group
 
-      project.group.ci_variables_for(ref, project)
+      project.group.ci_variables_for(git_ref, project)
     end
 
     def secret_project_variables(environment: persisted_environment)
-      project.ci_variables_for(ref: ref, environment: environment)
+      project.ci_variables_for(ref: git_ref, environment: environment)
     end
 
     def steps
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index d06022a0fb7..a1a11d2f7ab 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -11,6 +11,7 @@ module Ci
     include Gitlab::Utils::StrongMemoize
     include AtomicInternalId
     include EnumWithNil
+    include HasRef
 
     belongs_to :project, inverse_of: :all_pipelines
     belongs_to :user
@@ -588,7 +589,7 @@ module Ci
     end
 
     def protected_ref?
-      strong_memoize(:protected_ref) { project.protected_for?(ref) }
+      strong_memoize(:protected_ref) { project.protected_for?(git_ref) }
     end
 
     def legacy_trigger
diff --git a/app/models/concerns/has_ref.rb b/app/models/concerns/has_ref.rb
new file mode 100644
index 00000000000..79816841f7f
--- /dev/null
+++ b/app/models/concerns/has_ref.rb
@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+module HasRef
+  extend ActiveSupport::Concern
+
+  def branch?
+    !tag?
+  end
+
+  private
+
+  def git_ref
+    if branch?
+      Gitlab::Git::BRANCH_REF_PREFIX + ref.to_s
+    elsif tag?
+      Gitlab::Git::TAG_REF_PREFIX + ref.to_s
+    else
+      raise ArgumentError, 'Invalid pipeline type!'
+    end
+  end
+end
diff --git a/lib/gitlab/ci/pipeline/chain/command.rb b/lib/gitlab/ci/pipeline/chain/command.rb
index 100b9521412..316c283d90b 100644
--- a/lib/gitlab/ci/pipeline/chain/command.rb
+++ b/lib/gitlab/ci/pipeline/chain/command.rb
@@ -54,7 +54,7 @@ module Gitlab
 
           def protected_ref?
             strong_memoize(:protected_ref) do
-              project.protected_for?(ref)
+              project.protected_for?(origin_ref)
             end
           end
         end
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 89f78f629d4..d4056b4f7f1 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -2385,6 +2385,8 @@ describe Ci::Build do
     end
 
     context 'when protected variable is defined' do
+      let(:ref) { Gitlab::Git::BRANCH_REF_PREFIX + build.ref }
+
       let(:protected_variable) do
         { key: 'PROTECTED_KEY', value: 'protected_value', public: false }
       end
@@ -2397,7 +2399,7 @@ describe Ci::Build do
 
       context 'when the branch is protected' do
         before do
-          allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true)
+          allow(build.project).to receive(:protected_for?).with(ref).and_return(true)
         end
 
         it { is_expected.to include(protected_variable) }
@@ -2405,7 +2407,7 @@ describe Ci::Build do
 
       context 'when the tag is protected' do
         before do
-          allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true)
+          allow(build.project).to receive(:protected_for?).with(ref).and_return(true)
         end
 
         it { is_expected.to include(protected_variable) }
@@ -2430,6 +2432,8 @@ describe Ci::Build do
     end
 
     context 'when group protected variable is defined' do
+      let(:ref) { Gitlab::Git::BRANCH_REF_PREFIX + build.ref }
+
       let(:protected_variable) do
         { key: 'PROTECTED_KEY', value: 'protected_value', public: false }
       end
@@ -2442,7 +2446,7 @@ describe Ci::Build do
 
       context 'when the branch is protected' do
         before do
-          allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true)
+          allow(build.project).to receive(:protected_for?).with(ref).and_return(true)
         end
 
         it { is_expected.to include(protected_variable) }
@@ -2450,7 +2454,7 @@ describe Ci::Build do
 
       context 'when the tag is protected' do
         before do
-          allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true)
+          allow(build.project).to receive(:protected_for?).with(ref).and_return(true)
         end
 
         it { is_expected.to include(protected_variable) }
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index b67c6a4cffa..17f33785fda 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -397,6 +397,10 @@ describe Ci::Pipeline, :mailer do
   end
 
   describe '#protected_ref?' do
+    before do
+      pipeline.project = create(:project, :repository)
+    end
+
     it 'delegates method to project' do
       expect(pipeline).not_to be_protected_ref
     end
diff --git a/spec/models/concerns/has_ref_spec.rb b/spec/models/concerns/has_ref_spec.rb
new file mode 100644
index 00000000000..ec3cf9a8580
--- /dev/null
+++ b/spec/models/concerns/has_ref_spec.rb
@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe HasRef do
+  describe '#branch?' do
+    let(:pipeline) { create(:ci_pipeline) }
+
+    subject { pipeline.branch? }
+
+    context 'is not a tag' do
+      before do
+        pipeline.tag = false
+      end
+
+      it 'return true when tag is set to false' do
+        is_expected.to be_truthy
+      end
+    end
+
+    context 'is not a tag' do
+      before do
+        pipeline.tag = true
+      end
+
+      it 'return false when tag is set to true' do
+        is_expected.to be_falsey
+      end
+    end
+  end
+end