Merge branch 'issue_828' into 'master'

Prevent wrong markdown on issue ids when project has Jira service activated

fixes gitlab-org/gitlab-ee#828

See merge request !6728

CHANGELOG.md

@@ -13,6 +13,7 @@ Please view this file on the master branch, on stable branches it's out of date.
   - Update runner version only when updating contacted_at
   - Add link from system note to compare with previous version
   - Use gitlab-shell v3.6.6
+  - Ignore references to internal issues when using external issues tracker
   - Ability to resolve merge request conflicts with editor !6374
   - Add `/projects/visible` API endpoint (Ben Boeckel)
   - Fix centering of custom header logos (Ashley Dumaine)

app/models/external_issue.rb

@@ -29,11 +29,6 @@ class ExternalIssue
     @project
   end
 
-  # Pattern used to extract `JIRA-123` issue references from text
-  def self.reference_pattern
-    @reference_pattern ||= %r{(?<issue>\b([A-Z][A-Z0-9_]+-)\d+)}
-  end
-
   def to_reference(_from_project = nil)
     id
   end

app/models/project.rb

@@ -668,6 +668,10 @@ class Project < ActiveRecord::Base
     end
   end
 
+  def issue_reference_pattern
+    issues_tracker.reference_pattern
+  end
+
   def default_issues_tracker?
     !external_issue_tracker
   end

app/models/project_services/issue_tracker_service.rb

@@ -3,6 +3,12 @@ class IssueTrackerService < Service
 
   default_value_for :category, 'issue_tracker'
 
+  # Pattern used to extract links from comments
+  # Override this method on services that uses different patterns
+  def reference_pattern
+    @reference_pattern ||= %r{(\b[A-Z][A-Z0-9_]+-|#{Issue.reference_prefix})(?<issue>\d+)}
+  end
+
   def default?
     default
   end

app/models/project_services/jira_service.rb

@@ -13,6 +13,11 @@ class JiraService < IssueTrackerService
 
   before_update :reset_password
 
+  # {PROJECT-KEY}-{NUMBER} Examples: JIRA-1, PROJECT-1
+  def reference_pattern
+    @reference_pattern ||= %r{(?<issue>\b([A-Z][A-Z0-9_]+-)\d+)}
+  end
+
   def reset_password
     # don't reset the password if a new one is provided
     if api_url_changed? && !password_touched?

lib/banzai/filter/abstract_reference_filter.rb

@@ -208,8 +208,12 @@ module Banzai
         @references_per_project ||= begin
           refs = Hash.new { |hash, key| hash[key] = Set.new }
 
-          regex = Regexp.union(object_class.reference_pattern,
-                               object_class.link_reference_pattern)
+          regex =
+            if uses_reference_pattern?
+              Regexp.union(object_class.reference_pattern, object_class.link_reference_pattern)
+            else
+              object_class.link_reference_pattern
+            end
 
           nodes.each do |node|
             node.to_html.scan(regex) do
@@ -295,6 +299,14 @@ module Banzai
           value
         end
       end
+
+      # There might be special cases like filters
+      # that should ignore reference pattern
+      # eg: IssueReferenceFilter when using a external issues tracker
+      # In those cases this method should be overridden on the filter subclass
+      def uses_reference_pattern?
+        true
+      end
     end
   end
 end

lib/banzai/filter/external_issue_reference_filter.rb

@@ -8,7 +8,7 @@ module Banzai
 
       # Public: Find `JIRA-123` issue references in text
       #
-      #   ExternalIssueReferenceFilter.references_in(text) do |match, issue|
+      #   ExternalIssueReferenceFilter.references_in(text, pattern) do |match, issue|
       #     "<a href=...>##{issue}</a>"
       #   end
       #
@@ -17,8 +17,8 @@ module Banzai
       # Yields the String match and the String issue reference.
       #
       # Returns a String replaced with the return of the block.
-      def self.references_in(text)
-        text.gsub(ExternalIssue.reference_pattern) do |match|
+      def self.references_in(text, pattern)
+        text.gsub(pattern) do |match|
           yield match, $~[:issue]
         end
       end
@@ -27,7 +27,7 @@ module Banzai
         # Early return if the project isn't using an external tracker
         return doc if project.nil? || default_issues_tracker?
 
-        ref_pattern = ExternalIssue.reference_pattern
+        ref_pattern = issue_reference_pattern
         ref_start_pattern = /\A#{ref_pattern}\z/
 
         each_node do |node|
@@ -60,7 +60,7 @@ module Banzai
       def issue_link_filter(text, link_text: nil)
         project = context[:project]
 
-        self.class.references_in(text) do |match, id|
+        self.class.references_in(text, issue_reference_pattern) do |match, id|
           ExternalIssue.new(id, project)
 
           url = url_for_issue(id, project, only_path: context[:only_path])
@@ -82,18 +82,21 @@ module Banzai
       end
 
       def default_issues_tracker?
-        if RequestStore.active?
-          default_issues_tracker_cache[project.id] ||=
-            project.default_issues_tracker?
-        else
-          project.default_issues_tracker?
+        external_issues_cached(:default_issues_tracker?)
       end
+
+      def issue_reference_pattern
+        external_issues_cached(:issue_reference_pattern)
       end
 
       private
 
-      def default_issues_tracker_cache
-        RequestStore[:banzai_default_issues_tracker_cache] ||= {}
+      def external_issues_cached(attribute)
+        return project.public_send(attribute) unless RequestStore.active?
+
+        cached_attributes = RequestStore[:banzai_external_issues_tracker_attributes] ||= Hash.new { |h, k| h[k] = {} }
+        cached_attributes[project.id][attribute] = project.public_send(attribute) if cached_attributes[project.id][attribute].nil?
+        cached_attributes[project.id][attribute]
       end
     end
   end

lib/banzai/filter/issue_reference_filter.rb

@@ -4,6 +4,10 @@ module Banzai
     # issues that do not exist are ignored.
     #
     # This filter supports cross-project references.
+    #
+    # When external issues tracker like Jira is activated we should not
+    # use issue reference pattern, but we should still be able
+    # to reference issues from other GitLab projects.
     class IssueReferenceFilter < AbstractReferenceFilter
       self.reference_type = :issue
 
@@ -11,6 +15,10 @@ module Banzai
         Issue
       end
 
+      def uses_reference_pattern?
+        context[:project].default_issues_tracker?
+      end
+
       def find_object(project, iid)
         issues_per_project[project][iid]
       end

spec/lib/banzai/filter/external_issue_reference_filter_spec.rb

@@ -7,12 +7,7 @@ describe Banzai::Filter::ExternalIssueReferenceFilter, lib: true do
     IssuesHelper
   end
 
-  let(:project) { create(:jira_project) }
-
-  context 'JIRA issue references' do
-    let(:issue)     { ExternalIssue.new('JIRA-123', project) }
-    let(:reference) { issue.to_reference }
-
+  shared_examples_for "external issue tracker" do
     it 'requires project context' do
       expect { described_class.call('') }.to raise_error(ArgumentError, /:project/)
     end
@@ -20,6 +15,7 @@ describe Banzai::Filter::ExternalIssueReferenceFilter, lib: true do
     %w(pre code a style).each do |elem|
       it "ignores valid references contained inside '#{elem}' element" do
         exp = act = "<#{elem}>Issue #{reference}</#{elem}>"
+
         expect(filter(act).to_html).to eq exp
       end
     end
@@ -33,25 +29,30 @@ describe Banzai::Filter::ExternalIssueReferenceFilter, lib: true do
 
     it 'links to a valid reference' do
       doc = filter("Issue #{reference}")
+      issue_id = doc.css('a').first.attr("data-external-issue")
+
       expect(doc.css('a').first.attr('href'))
-        .to eq helper.url_for_issue(reference, project)
+        .to eq helper.url_for_issue(issue_id, project)
     end
 
     it 'links to the external tracker' do
       doc = filter("Issue #{reference}")
+
       link = doc.css('a').first.attr('href')
+      issue_id = doc.css('a').first.attr("data-external-issue")
 
-      expect(link).to eq "http://jira.example/browse/#{reference}"
+      expect(link).to eq(helper.url_for_issue(issue_id, project))
     end
 
     it 'links with adjacent text' do
       doc = filter("Issue (#{reference}.)")
+
       expect(doc.to_html).to match(/\(<a.+>#{reference}<\/a>\.\)/)
     end
 
     it 'includes a title attribute' do
       doc = filter("Issue #{reference}")
-      expect(doc.css('a').first.attr('title')).to eq "Issue in JIRA tracker"
+      expect(doc.css('a').first.attr('title')).to include("Issue in #{project.issues_tracker.title}")
     end
 
     it 'escapes the title attribute' do
@@ -69,9 +70,60 @@ describe Banzai::Filter::ExternalIssueReferenceFilter, lib: true do
 
     it 'supports an :only_path context' do
       doc = filter("Issue #{reference}", only_path: true)
+
       link = doc.css('a').first.attr('href')
+      issue_id = doc.css('a').first["data-external-issue"]
 
-      expect(link).to eq helper.url_for_issue("#{reference}", project, only_path: true)
+      expect(link).to eq helper.url_for_issue(issue_id, project, only_path: true)
+    end
+
+    context 'with RequestStore enabled' do
+      let(:reference_filter) { HTML::Pipeline.new([described_class]) }
+
+      before { allow(RequestStore).to receive(:active?).and_return(true) }
+
+      it 'queries the collection on the first call' do
+        expect_any_instance_of(Project).to receive(:default_issues_tracker?).once.and_call_original
+        expect_any_instance_of(Project).to receive(:issue_reference_pattern).once.and_call_original
+
+        not_cached = reference_filter.call("look for #{reference}", { project: project })
+
+        expect_any_instance_of(Project).not_to receive(:default_issues_tracker?)
+        expect_any_instance_of(Project).not_to receive(:issue_reference_pattern)
+
+        cached = reference_filter.call("look for #{reference}", { project: project })
+
+        # Links must be the same
+        expect(cached[:output].css('a').first[:href]).to eq(not_cached[:output].css('a').first[:href])
+      end
+    end
+  end
+
+  context "redmine project" do
+    let(:project) { create(:redmine_project) }
+    let(:issue) { ExternalIssue.new("#123", project) }
+    let(:reference) { issue.to_reference }
+
+    it_behaves_like "external issue tracker"
+  end
+
+  context "jira project" do
+    let(:project) { create(:jira_project) }
+    let(:reference) { issue.to_reference }
+
+    context "with right markdown" do
+      let(:issue) { ExternalIssue.new("JIRA-123", project) }
+
+      it_behaves_like "external issue tracker"
+    end
+
+    context "with wrong markdown" do
+      let(:issue) { ExternalIssue.new("#123", project) }
+
+      it "ignores reference" do
+        exp = act = "Issue #{reference}"
+        expect(filter(act).to_html).to eq exp
+      end
     end
   end
 end

spec/lib/banzai/filter/issue_reference_filter_spec.rb

@@ -25,9 +25,7 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do
     let(:reference) { issue.to_reference }
 
     it 'ignores valid references when using non-default tracker' do
-      expect_any_instance_of(described_class).to receive(:find_object).
-        with(project, issue.iid).
-        and_return(nil)
+      allow(project).to receive(:default_issues_tracker?).and_return(false)
 
       exp = act = "Issue #{reference}"
       expect(reference_filter(act).to_html).to eq exp
@@ -199,19 +197,6 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do
     end
   end
 
-  context 'referencing external issues' do
-    let(:project) { create(:redmine_project) }
-
-    it 'renders internal issue IDs as external issue links' do
-      doc = reference_filter('#1')
-      link = doc.css('a').first
-
-      expect(link.attr('data-reference-type')).to eq('external_issue')
-      expect(link.attr('title')).to eq('Issue in Redmine')
-      expect(link.attr('data-external-issue')).to eq('1')
-    end
-  end
-
   describe '#issues_per_Project' do
     context 'using an internal issue tracker' do
       it 'returns a Hash containing the issues per project' do

spec/models/external_issue_spec.rb

@@ -10,21 +10,6 @@ describe ExternalIssue, models: true do
     it { is_expected.to include_module(Referable) }
   end
 
-  describe '.reference_pattern' do
-    it 'allows underscores in the project name' do
-      expect(ExternalIssue.reference_pattern.match('EXT_EXT-1234')[0]).to eq 'EXT_EXT-1234'
-    end
-
-    it 'allows numbers in the project name' do
-      expect(ExternalIssue.reference_pattern.match('EXT3_EXT-1234')[0]).to eq 'EXT3_EXT-1234'
-    end
-
-    it 'requires the project name to begin with A-Z' do
-      expect(ExternalIssue.reference_pattern.match('3EXT_EXT-1234')).to eq nil
-      expect(ExternalIssue.reference_pattern.match('EXT_EXT-1234')[0]).to eq 'EXT_EXT-1234'
-    end
-  end
-
   describe '#to_reference' do
     it 'returns a String reference to the object' do
       expect(issue.to_reference).to eq issue.id

spec/models/project_services/jira_service_spec.rb

@@ -30,6 +30,15 @@ describe JiraService, models: true do
     end
   end
 
+  describe '#reference_pattern' do
+    it_behaves_like 'allows project key on reference pattern'
+
+    it 'does not allow # on the code' do
+      expect(subject.reference_pattern.match('#123')).to be_nil
+      expect(subject.reference_pattern.match('1#23#12')).to be_nil
+    end
+  end
+
   describe "Execute" do
     let(:user)    { create(:user) }
     let(:project) { create(:project) }

spec/models/project_services/redmine_service_spec.rb

@@ -26,4 +26,12 @@ describe RedmineService, models: true do
       it { is_expected.not_to validate_presence_of(:new_issue_url) }
     end
   end
+
+  describe '#reference_pattern' do
+    it_behaves_like 'allows project key on reference pattern'
+
+    it 'does allow # on the reference' do
+      expect(subject.reference_pattern.match('#123')[:issue]).to eq('123')
+    end
+  end
 end

spec/services/git_push_service_spec.rb

@@ -415,7 +415,7 @@ describe GitPushService, services: true do
       it "doesn't close issues when external issue tracker is in use" do
         allow_any_instance_of(Project).to receive(:default_issues_tracker?).
           and_return(false)
-        external_issue_tracker = double(title: 'My Tracker', issue_path: issue.iid)
+        external_issue_tracker = double(title: 'My Tracker', issue_path: issue.iid, reference_pattern: project.issue_reference_pattern)
         allow_any_instance_of(Project).to receive(:external_issue_tracker).and_return(external_issue_tracker)
 
         # The push still shouldn't create cross-reference notes.
@@ -485,31 +485,47 @@ describe GitPushService, services: true do
 
       context "closing an issue" do
         let(:message)         { "this is some work.\n\ncloses JIRA-1" }
+        let(:transition_body) { { transition: { id: '2' } }.to_json }
+        let(:comment_body)    { { body: "Issue solved with [#{closing_commit.id}|http://localhost/#{project.path_with_namespace}/commit/#{closing_commit.id}]." }.to_json }
 
+        context "using right markdown" do
           it "initiates one api call to jira server to close the issue" do
-          transition_body = {
-            transition: {
-              id: '2'
-            }
-          }.to_json
-
             execute_service(project, commit_author, @oldrev, @newrev, @ref )
+
             expect(WebMock).to have_requested(:post, jira_api_transition_url).with(
               body: transition_body
             ).once
           end
 
           it "initiates one api call to jira server to comment on the issue" do
-          comment_body = {
-            body: "Issue solved with [#{closing_commit.id}|http://localhost/#{project.path_with_namespace}/commit/#{closing_commit.id}]."
-          }.to_json
-
             execute_service(project, commit_author, @oldrev, @newrev, @ref )
+
             expect(WebMock).to have_requested(:post, jira_api_comment_url).with(
               body: comment_body
             ).once
           end
         end
+
+        context "using wrong markdown" do
+          let(:message) { "this is some work.\n\ncloses #1" }
+
+          it "does not initiates one api call to jira server to close the issue" do
+            execute_service(project, commit_author, @oldrev, @newrev, @ref )
+
+            expect(WebMock).not_to have_requested(:post, jira_api_transition_url).with(
+              body: transition_body
+            )
+          end
+
+          it "does not initiates one api call to jira server to comment on the issue" do
+            execute_service(project, commit_author, @oldrev, @newrev, @ref )
+
+            expect(WebMock).not_to have_requested(:post, jira_api_comment_url).with(
+              body: comment_body
+            ).once
+          end
+        end
+      end
     end
   end
 

spec/services/merge_requests/merge_service_spec.rb

@@ -74,6 +74,18 @@ describe MergeRequests::MergeService, services: true do
 
           service.execute(merge_request)
         end
+
+        context "wrong issue markdown" do
+          it 'does not close issues on JIRA issue tracker' do
+            jira_issue = ExternalIssue.new('#123', project)
+            commit = double('commit', safe_message: "Fixes #{jira_issue.to_reference}")
+            allow(merge_request).to receive(:commits).and_return([commit])
+
+            expect_any_instance_of(JiraService).not_to receive(:close_issue)
+
+            service.execute(merge_request)
+          end
+        end
       end
     end
 

spec/support/issue_tracker_service_shared_example.rb

@@ -5,3 +5,18 @@ RSpec.shared_examples 'issue tracker service URL attribute' do |url_attr|
   it { is_expected.not_to allow_value('ftp://example.com').for(url_attr) }
   it { is_expected.not_to allow_value('herp-and-derp').for(url_attr) }
 end
+
+RSpec.shared_examples 'allows project key on reference pattern' do |url_attr|
+  it 'allows underscores in the project name' do
+    expect(subject.reference_pattern.match('EXT_EXT-1234')[0]).to eq 'EXT_EXT-1234'
+  end
+
+  it 'allows numbers in the project name' do
+    expect(subject.reference_pattern.match('EXT3_EXT-1234')[0]).to eq 'EXT3_EXT-1234'
+  end
+
+  it 'requires the project name to begin with A-Z' do
+    expect(subject.reference_pattern.match('3EXT_EXT-1234')).to eq nil
+    expect(subject.reference_pattern.match('EXT_EXT-1234')[0]).to eq 'EXT_EXT-1234'
+  end
+end