Commit 8e4301d9 authored by Felipe Artur's avatar Felipe Artur
Browse files

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

parent 4d18073b5832
......@@ -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)
......
......@@ -29,11 +29,6 @@ def project
@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
......
......@@ -664,6 +664,10 @@ def issues_tracker
end
end
def issue_reference_pattern
issues_tracker.reference_pattern
end
def default_issues_tracker?
!external_issue_tracker
end
......
......@@ -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
......
......@@ -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?
......
......@@ -208,8 +208,12 @@ def references_per_project
@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 @@ def get_or_set_cache(cache, key)
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
......@@ -8,7 +8,7 @@ class ExternalIssueReferenceFilter < ReferenceFilter
# 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 @@ class ExternalIssueReferenceFilter < ReferenceFilter
# 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 @@ def call
# 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 @@ def call
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 @@ def url_for_issue(*args)
end
def default_issues_tracker?
if RequestStore.active?
default_issues_tracker_cache[project.id] ||=
project.default_issues_tracker?
else
project.default_issues_tracker?
end
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
......
......@@ -4,6 +4,10 @@ module Filter
# 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 @@ def self.object_class
Issue
end
def uses_reference_pattern?
context[:project].default_issues_tracker?
end
def find_object(project, iid)
issues_per_project[project][iid]
end
......
......@@ -7,12 +7,7 @@ def helper
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 @@ def helper
%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 @@ def helper
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 @@ def helper
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(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 })
expect(link).to eq helper.url_for_issue("#{reference}", project, only_path: true)
# 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
......@@ -25,9 +25,7 @@ def helper
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 @@ def helper
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
......
......@@ -10,21 +10,6 @@
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
......
......@@ -30,6 +30,15 @@
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) }
......
......@@ -26,4 +26,12 @@
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
......@@ -415,7 +415,7 @@
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.
......@@ -484,30 +484,46 @@
end
context "closing an issue" do
let(:message) { "this is some work.\n\ncloses JIRA-1" }
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
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
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
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
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
context "using wrong markdown" do
let(:message) { "this is some work.\n\ncloses #1" }
execute_service(project, commit_author, @oldrev, @newrev, @ref )
expect(WebMock).to have_requested(:post, jira_api_comment_url).with(
body: comment_body
).once
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
......
......@@ -74,6 +74,18 @@
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
......
......@@ -5,3 +5,18 @@
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
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