Merge pull request #45 from javiplx/hardening/branch_matching

Hardening/branch matching

Conflicts:
	models/values/project.rb
	spec/values/project_spec.rb

Bundled commits:
	f3b9380 Don't prefix with remote name when simple branch names are configured on SCM
	6baf7db Allow proper matching if using refs style names for branch specifiers
	8082e25 Allow exact matches when configured branch starts with wildcard (Resolves #42)

models/values/project.rb

@@ -98,6 +98,22 @@ def matching_scms(details_uri)
       end
     end
 
+    # Maybe all this stuff could get delegated to an SCM poll, but on the meantime
+    # we need to clarify the behaviour. From the available BranchSpec tests on the
+    # git plugin, we seen that when there is no slash on the branch specification,
+    # the first token of the supplied string is discarded, thus producing a false
+    # match when the string neither has a slash and is equal to the branchspec. And
+    # when there is a slash on configured BranchSpec, an standard matching is done,
+    # with no extra work on the supplied string.
+    # This means that the git plugin expects the supplied branch to be always prefixed
+    # with the remote name.
+    # Adding 'remotes' or 'refs/remotes' to the string does not change the match
+    # behaviour except when by chance the mismatching portion is discarded by git plugin.
+    # The results obtained when using any kind of 'refs/' prefix on configured branchspec
+    # lead us to supose that a simple ant-alike path wildcard matching is done among
+    # the configured refspec and the supplied string, except for the removal of the first
+    # path portion when refspec has no slash.
+    #
     def matches_branch?(details, branch = false, exactly = false)
       refspec = details.full_branch_reference
       branch = details.branch unless branch
@@ -107,10 +123,21 @@ def matches_branch?(details, branch = false, exactly = false)
       matched_scm = @matching_scms.find do |scm|
         matched_branch = scm.branches.find do |scm_branch|
           scm.repositories.find do |repo|
-            token = "#{repo.name}/#{branch}"
+            # When BranchSpec seems to be a 'refs' style, we use the reference supplied by
+            # gitlab, which is the reference on its local repository. In any other case, we
+            # follow the classic gitlab-hook processing.
+            if scm_branch.name.start_with?('refs/')
+              token = refspec
+            elsif scm_branch.name.start_with?('*/')
+              token = "*/#{branch}"
+            else
+              # if scm_branch.name has no slash, repo.name will be filtered on 'matches' call,
+              token = "#{repo.name}/#{branch}"
+            end
             scm_refspecs = repo.getFetchRefSpecs().select { |scm_refspec| scm_refspec.matchSource(refspec) }
             matched_refspecs.concat(scm_refspecs)
-            scm_refspecs.any? && (exactly ? scm_branch.name == token : scm_branch.matches(token))
+            scm_branch_name = scm_branch.name.match('/') ? scm_branch.name : "#{repo.name}/#{scm_branch.name}"
+            scm_refspecs.any? && (exactly ? scm_branch_name == token : scm_branch.matches(token))
           end
         end
       end

spec/values/matches_branch_spec.rb

@@ -0,0 +1,310 @@
+require 'spec_helper'
+
+describe "BranchSpect matching" do
+
+  context "when branchspec is master" do
+    let(:subject) { BranchSpec.new('master') }
+
+    it "will not match when string is 'master'" do
+      expect(subject.matches('master')).to be(false)
+    end
+
+    context "will match" do
+      it "when string is 'origin/master'" do
+        expect(subject.matches('origin/master')).to be(true)
+      end
+      it "when string is 'other/master'" do
+        expect(subject.matches('other/master')).to be(true)
+      end
+      it "when string is '*/master'" do
+        expect(subject.matches('*/master')).to be(true)
+      end
+      it "when string is 'remotes/master'" do
+        expect(subject.matches('remotes/master')).to be(true)
+      end
+    end
+
+    context "will not match" do
+      it "when string is 'refs/remotes/master'" do
+        expect(subject.matches('refs/remotes/master')).to be(false)
+      end
+      it "when string is 'refs/remotes/origin/master'" do
+        expect(subject.matches('refs/remotes/origin/master')).to be(false)
+      end
+      it "when string is 'remotes/origin/master'" do
+        expect(subject.matches('remotes/origin/master')).to be(false)
+      end
+      it "when string is 'origin/otherbranch'" do
+        expect(subject.matches('origin/otherbranch')).to be(false)
+      end
+      it "when string is '*/otherbranch'" do
+        expect(subject.matches('*/otherbranch')).to be(false)
+      end
+      it "when string is '*'" do
+        expect(subject.matches('*')).to be(false)
+      end
+      it "when string is '**'" do
+        expect(subject.matches('**')).to be(false)
+      end
+    end
+
+  end
+
+  context "when branchspec is origin/*" do
+    let(:subject) { BranchSpec.new('origin/*') }
+
+    context "will match" do
+      it "when string is 'origin/*'" do
+        expect(subject.matches('origin/*')).to be(true)
+      end
+      it "when string is 'origin/master'" do
+        expect(subject.matches('origin/master')).to be(true)
+      end
+      it "when string is 'origin/otherbranch'" do
+        expect(subject.matches('origin/otherbranch')).to be(true)
+      end
+    end
+
+    context "will not match" do
+      it "when string is 'master'" do
+        expect(subject.matches('master')).to be(false)
+      end
+      it "when string is 'refs/remotes/master'" do
+        expect(subject.matches('refs/remotes/master')).to be(false)
+      end
+      it "when string is 'remotes/master'" do
+        expect(subject.matches('remotes/master')).to be(false)
+      end
+      it "when string is 'refs/remotes/origin/master'" do
+        expect(subject.matches('refs/remotes/origin/master')).to be(false)
+      end
+      it "when string is 'remotes/origin/master'" do
+        expect(subject.matches('remotes/origin/master')).to be(false)
+      end
+      it "when string is 'other/master'" do
+        expect(subject.matches('other/master')).to be(false)
+      end
+      it "when string is '*/master'" do
+        expect(subject.matches('*/master')).to be(false)
+      end
+      it "when string is '*/otherbranch'" do
+        expect(subject.matches('*/otherbranch')).to be(false)
+      end
+      it "when string is '*'" do
+        expect(subject.matches('*')).to be(false)
+      end
+      it "when string is '**'" do
+        expect(subject.matches('**')).to be(false)
+      end
+      it "when string is '*/*'" do
+        expect(subject.matches('*/*')).to be(false)
+      end
+      it "when string is '**/*'" do
+        expect(subject.matches('**/*')).to be(false)
+      end
+      it "when string is '*/**'" do
+        expect(subject.matches('*/**')).to be(false)
+      end
+    end
+
+  end
+
+  context "when branchspec is 'origin/master'" do
+    let(:subject) { BranchSpec.new('origin/master') }
+
+    context "will match" do
+      it "when string is 'origin/master'" do
+        expect(subject.matches('origin/master')).to be(true)
+      end
+    end
+
+    context "will not match" do
+      it "when string is 'origin/*'" do
+        expect(subject.matches('origin/*')).to be(false)
+      end
+      it "when string is 'origin/otherbranch'" do
+        expect(subject.matches('origin/otherbranch')).to be(false)
+      end
+      it "when string is 'master'" do
+        expect(subject.matches('master')).to be(false)
+      end
+      it "when string is 'other/master'" do
+        expect(subject.matches('other/master')).to be(false)
+      end
+      it "when string is '*/master'" do
+        expect(subject.matches('*/master')).to be(false)
+      end
+      it "when string is '*/otherbranch'" do
+        expect(subject.matches('*/otherbranch')).to be(false)
+      end
+      it "when string is '*'" do
+        expect(subject.matches('*')).to be(false)
+      end
+      it "when string is '**'" do
+        expect(subject.matches('**')).to be(false)
+      end
+      it "when string is '*/*'" do
+        expect(subject.matches('*/*')).to be(false)
+      end
+      it "when string is '**/*'" do
+        expect(subject.matches('**/*')).to be(false)
+      end
+      it "when string is '*/**'" do
+        expect(subject.matches('*/**')).to be(false)
+      end
+    end
+
+  end
+
+  # This is the default value suggested by git plugin
+  context "when branchspec is '*/master'" do
+    let(:subject) { BranchSpec.new('*/master') }
+
+    context "will match" do
+      it "when string is 'origin/master'" do
+        expect(subject.matches('origin/master')).to be(true)
+      end
+      it "when string is 'other/master'" do
+        expect(subject.matches('other/master')).to be(true)
+      end
+      # This is a bit surprising, as wildcard in string seems not to be processed
+      it "when string is '*/master'" do
+        expect(subject.matches('*/master')).to be(true)
+      end
+    end
+
+    context "will not match" do
+      it "when string is 'origin/*'" do
+        expect(subject.matches('origin/*')).to be(false)
+      end
+      it "when string is 'origin/otherbranch'" do
+        expect(subject.matches('origin/otherbranch')).to be(false)
+      end
+      it "when string is 'master'" do
+        expect(subject.matches('master')).to be(false)
+      end
+      it "when string is '*/otherbranch'" do
+        expect(subject.matches('*/otherbranch')).to be(false)
+      end
+      it "when string is '*'" do
+        expect(subject.matches('*')).to be(false)
+      end
+      it "when string is '**'" do
+        expect(subject.matches('**')).to be(false)
+      end
+      it "when string is '*/*'" do
+        expect(subject.matches('*/*')).to be(false)
+      end
+      it "when string is '**/*'" do
+        expect(subject.matches('**/*')).to be(false)
+      end
+      it "when string is '*/**'" do
+        expect(subject.matches('*/**')).to be(false)
+      end
+    end
+
+  end
+
+  context "when branchspec is 'refs/heads/master'" do
+    let(:subject) { BranchSpec.new('refs/heads/master') }
+
+    it "will not match when string is 'master'" do
+      expect(subject.matches('master')).to be(false)
+    end
+
+    context "will not match" do
+      it "when string is 'origin/master'" do
+        expect(subject.matches('origin/master')).to be(false)
+      end
+      it "when string is '*/master'" do
+        expect(subject.matches('*/master')).to be(false)
+      end
+      it "when string is 'remotes/master'" do
+        expect(subject.matches('remotes/master')).to be(false)
+      end
+      it "when string is 'other/master'" do
+        expect(subject.matches('other/master')).to be(false)
+      end
+      it "when string is 'refs/remotes/master'" do
+        expect(subject.matches('refs/remotes/master')).to be(false)
+      end
+      it "when string is 'refs/remotes/origin/master'" do
+        expect(subject.matches('refs/remotes/origin/master')).to be(false)
+      end
+      it "when string is 'remotes/origin/master'" do
+        expect(subject.matches('remotes/origin/master')).to be(false)
+      end
+      it "when string is 'origin/otherbranch'" do
+        expect(subject.matches('origin/otherbranch')).to be(false)
+      end
+      it "when string is '*/otherbranch'" do
+        expect(subject.matches('*/otherbranch')).to be(false)
+      end
+      it "when string is '*'" do
+        expect(subject.matches('*')).to be(false)
+      end
+      it "when string is '**'" do
+        expect(subject.matches('**')).to be(false)
+      end
+    end
+
+  end
+
+  context "when branchspec is 'refs/remotes/origin/master'" do
+    let(:subject) { BranchSpec.new('refs/remotes/origin/master') }
+
+    context "will not match" do
+      it "when string is 'origin/master'" do
+        expect(subject.matches('origin/master')).to be(false)
+      end
+      it "when string is 'origin/*'" do
+        expect(subject.matches('origin/*')).to be(false)
+      end
+      it "when string is 'origin/otherbranch'" do
+        expect(subject.matches('origin/otherbranch')).to be(false)
+      end
+      it "when string is 'master'" do
+        expect(subject.matches('master')).to be(false)
+      end
+      it "when string is 'other/master'" do
+        expect(subject.matches('other/master')).to be(false)
+      end
+      it "when string is '*/master'" do
+        expect(subject.matches('*/master')).to be(false)
+      end
+      it "when string is '*/otherbranch'" do
+        expect(subject.matches('*/otherbranch')).to be(false)
+      end
+      it "when string is '*'" do
+        expect(subject.matches('*')).to be(false)
+      end
+      it "when string is '**'" do
+        expect(subject.matches('**')).to be(false)
+      end
+      it "when string is '*/*'" do
+        expect(subject.matches('*/*')).to be(false)
+      end
+      it "when string is '**/*'" do
+        expect(subject.matches('**/*')).to be(false)
+      end
+      it "when string is '*/**'" do
+        expect(subject.matches('*/**')).to be(false)
+      end
+    end
+
+  end
+
+  # Final verification for the ant-path style match
+  context "when branchspec is 'refs/*/master'" do
+    let(:subject) { BranchSpec.new('refs/*/master') }
+    context "will match" do
+      it "when string is 'refs/remotes/master'" do
+        expect(subject.matches('refs/remotes/master')).to be(true)
+      end
+      it "when string is 'refs/heads/master'" do
+        expect(subject.matches('refs/heads/master')).to be(true)
+      end
+    end
+  end
+
+end