Commit a5ee0810 authored by Yorick Peterse's avatar Yorick Peterse

Merge branch '11-8-security-2773-milestones-fix' into '11-8-stable'

Check issue milestone availability

See merge request gitlab/gitlabhq!2904
parents cefed976 98df4053
...@@ -74,6 +74,7 @@ def award_emojis_loaded? ...@@ -74,6 +74,7 @@ def award_emojis_loaded?
validates :author, presence: true validates :author, presence: true
validates :title, presence: true, length: { maximum: 255 } validates :title, presence: true, length: { maximum: 255 }
validate :milestone_is_valid
scope :authored, ->(user) { where(author_id: user) } scope :authored, ->(user) { where(author_id: user) }
scope :recent, -> { reorder(id: :desc) } scope :recent, -> { reorder(id: :desc) }
...@@ -117,6 +118,16 @@ def allows_multiple_assignees? ...@@ -117,6 +118,16 @@ def allows_multiple_assignees?
def has_multiple_assignees? def has_multiple_assignees?
assignees.count > 1 assignees.count > 1
end end
def milestone_available?
project_id == milestone&.project_id || project.ancestors_upto.compact.include?(milestone&.group)
end
private
def milestone_is_valid
errors.add(:milestone_id, message: "is invalid") if milestone_id.present? && !milestone_available?
end
end end
class_methods do class_methods do
......
...@@ -189,6 +189,9 @@ def check_state?(merge_status) ...@@ -189,6 +189,9 @@ def check_state?(merge_status)
after_save :keep_around_commit after_save :keep_around_commit
alias_attribute :project, :target_project
alias_attribute :project_id, :target_project_id
def self.reference_prefix def self.reference_prefix
'!' '!'
end end
...@@ -837,10 +840,6 @@ def for_fork? ...@@ -837,10 +840,6 @@ def for_fork?
target_project != source_project target_project != source_project
end end
def project
target_project
end
# If the merge request closes any issues, save this information in the # If the merge request closes any issues, save this information in the
# `MergeRequestsClosingIssues` model. This is a performance optimization. # `MergeRequestsClosingIssues` model. This is a performance optimization.
# Calculating this information for a number of merge requests requires # Calculating this information for a number of merge requests requires
......
...@@ -387,4 +387,10 @@ def update_project_counter_caches?(issuable) ...@@ -387,4 +387,10 @@ def update_project_counter_caches?(issuable)
def parent def parent
project project
end end
# we need to check this because milestone from milestone_id param is displayed on "new" page
# where private project milestone could leak without this check
def ensure_milestone_available(issuable)
issuable.milestone_id = nil unless issuable.milestone_available?
end
end end
...@@ -6,7 +6,9 @@ class BuildService < Issues::BaseService ...@@ -6,7 +6,9 @@ class BuildService < Issues::BaseService
def execute def execute
filter_resolve_discussion_params filter_resolve_discussion_params
@issue = project.issues.new(issue_params) @issue = project.issues.new(issue_params).tap do |issue|
ensure_milestone_available(issue)
end
end end
def issue_params_with_info_from_discussions def issue_params_with_info_from_discussions
......
...@@ -19,6 +19,7 @@ def execute ...@@ -19,6 +19,7 @@ def execute
merge_request.target_project = find_target_project merge_request.target_project = find_target_project
merge_request.target_branch = find_target_branch merge_request.target_branch = find_target_branch
merge_request.can_be_created = projects_and_branches_valid? merge_request.can_be_created = projects_and_branches_valid?
ensure_milestone_available(merge_request)
# compare branches only if branches are valid, otherwise # compare branches only if branches are valid, otherwise
# compare_branches may raise an error # compare_branches may raise an error
......
---
title: Check if desired milestone for an issue is available
merge_request:
author:
type: security
...@@ -13,7 +13,7 @@ ...@@ -13,7 +13,7 @@
) )
end end
let(:issue) { create(:issue, project: project, milestone: project_milestone) } let(:issue) { create(:issue, project: project, milestone: project_milestone) }
let(:group_issue) { create(:issue, milestone: group_milestone) } let(:group_issue) { create(:issue, milestone: group_milestone, project: create(:project, group: group)) }
let!(:label) { create(:label, project: project, title: 'Issue Label', issues: [issue]) } let!(:label) { create(:label, project: project, title: 'Issue Label', issues: [issue]) }
let!(:group_label) { create(:group_label, group: group, title: 'Group Issue Label', issues: [group_issue]) } let!(:group_label) { create(:group_label, group: group, title: 'Group Issue Label', issues: [group_issue]) }
......
...@@ -233,8 +233,8 @@ ...@@ -233,8 +233,8 @@
created_at: Time.now - (index * 60)) created_at: Time.now - (index * 60))
end end
end end
let(:newer_due_milestone) { create(:milestone, due_date: '2013-12-11') } let(:newer_due_milestone) { create(:milestone, project: project, due_date: '2013-12-11') }
let(:later_due_milestone) { create(:milestone, due_date: '2013-12-12') } let(:later_due_milestone) { create(:milestone, project: project, due_date: '2013-12-12') }
it 'sorts by newest' do it 'sorts by newest' do
visit project_issues_path(project, sort: sort_value_created_date) visit project_issues_path(project, sort: sort_value_created_date)
......
...@@ -13,7 +13,7 @@ ...@@ -13,7 +13,7 @@
source_project: project, source_project: project,
source_branch: 'fix', source_branch: 'fix',
assignee: user, assignee: user,
milestone: create(:milestone, due_date: '2013-12-11'), milestone: create(:milestone, project: project, due_date: '2013-12-11'),
created_at: 1.minute.ago, created_at: 1.minute.ago,
updated_at: 1.minute.ago) updated_at: 1.minute.ago)
create(:merge_request, create(:merge_request,
...@@ -21,7 +21,7 @@ ...@@ -21,7 +21,7 @@
source_project: project, source_project: project,
source_branch: 'markdown', source_branch: 'markdown',
assignee: user, assignee: user,
milestone: create(:milestone, due_date: '2013-12-12'), milestone: create(:milestone, project: project, due_date: '2013-12-12'),
created_at: 2.minutes.ago, created_at: 2.minutes.ago,
updated_at: 2.minutes.ago) updated_at: 2.minutes.ago)
create(:merge_request, create(:merge_request,
......
...@@ -32,17 +32,56 @@ ...@@ -32,17 +32,56 @@
end end
describe "Validation" do describe "Validation" do
subject { build(:issue) } context 'general validations' do
subject { build(:issue) }
before do before do
allow(InternalId).to receive(:generate_next).and_return(nil) allow(InternalId).to receive(:generate_next).and_return(nil)
end
it { is_expected.to validate_presence_of(:project) }
it { is_expected.to validate_presence_of(:iid) }
it { is_expected.to validate_presence_of(:author) }
it { is_expected.to validate_presence_of(:title) }
it { is_expected.to validate_length_of(:title).is_at_most(255) }
end end
it { is_expected.to validate_presence_of(:project) } describe 'milestone' do
it { is_expected.to validate_presence_of(:iid) } let(:project) { create(:project) }
it { is_expected.to validate_presence_of(:author) } let(:milestone_id) { create(:milestone, project: project).id }
it { is_expected.to validate_presence_of(:title) } let(:params) do
it { is_expected.to validate_length_of(:title).is_at_most(255) } {
title: 'something',
project: project,
author: build(:user),
milestone_id: milestone_id
}
end
subject { issuable_class.new(params) }
context 'with correct params' do
it { is_expected.to be_valid }
end
context 'with empty string milestone' do
let(:milestone_id) { '' }
it { is_expected.to be_valid }
end
context 'with nil milestone id' do
let(:milestone_id) { nil }
it { is_expected.to be_valid }
end
context 'with a milestone id from another project' do
let(:milestone_id) { create(:milestone).id }
it { is_expected.to be_invalid }
end
end
end end
describe "Scope" do describe "Scope" do
...@@ -66,6 +105,48 @@ ...@@ -66,6 +105,48 @@
end end
end end
describe '#milestone_available?' do
let(:group) { create(:group) }
let(:project) { create(:project, group: group) }
let(:issue) { create(:issue, project: project) }
def build_issuable(milestone_id)
issuable_class.new(project: project, milestone_id: milestone_id)
end
it 'returns true with a milestone from the issue project' do
milestone = create(:milestone, project: project)
expect(build_issuable(milestone.id).milestone_available?).to be_truthy
end
it 'returns true with a milestone from the issue project group' do
milestone = create(:milestone, group: group)
expect(build_issuable(milestone.id).milestone_available?).to be_truthy
end
it 'returns true with a milestone from the the parent of the issue project group', :nested_groups do
parent = create(:group)
group.update(parent: parent)
milestone = create(:milestone, group: parent)
expect(build_issuable(milestone.id).milestone_available?).to be_truthy
end
it 'returns false with a milestone from another project' do
milestone = create(:milestone)
expect(build_issuable(milestone.id).milestone_available?).to be_falsey
end
it 'returns false with a milestone from another group' do
milestone = create(:milestone, group: create(:group))
expect(build_issuable(milestone.id).milestone_available?).to be_falsey
end
end
describe ".search" do describe ".search" do
let!(:searchable_issue) { create(:issue, title: "Searchable awesome issue") } let!(:searchable_issue) { create(:issue, title: "Searchable awesome issue") }
let!(:searchable_issue2) { create(:issue, title: 'Aw') } let!(:searchable_issue2) { create(:issue, title: 'Aw') }
......
...@@ -9,7 +9,7 @@ ...@@ -9,7 +9,7 @@
context "milestones" do context "milestones" do
it "records the first time an issue is associated with a milestone" do it "records the first time an issue is associated with a milestone" do
time = Time.now time = Time.now
Timecop.freeze(time) { subject.update(milestone: create(:milestone)) } Timecop.freeze(time) { subject.update(milestone: create(:milestone, project: project)) }
metrics = subject.metrics metrics = subject.metrics
expect(metrics).to be_present expect(metrics).to be_present
...@@ -18,9 +18,9 @@ ...@@ -18,9 +18,9 @@
it "does not record the second time an issue is associated with a milestone" do it "does not record the second time an issue is associated with a milestone" do
time = Time.now time = Time.now
Timecop.freeze(time) { subject.update(milestone: create(:milestone)) } Timecop.freeze(time) { subject.update(milestone: create(:milestone, project: project)) }
Timecop.freeze(time + 2.hours) { subject.update(milestone: nil) } Timecop.freeze(time + 2.hours) { subject.update(milestone: nil) }
Timecop.freeze(time + 6.hours) { subject.update(milestone: create(:milestone)) } Timecop.freeze(time + 6.hours) { subject.update(milestone: create(:milestone, project: project)) }
metrics = subject.metrics metrics = subject.metrics
expect(metrics).to be_present expect(metrics).to be_present
......
...@@ -182,7 +182,7 @@ ...@@ -182,7 +182,7 @@
describe '#total_items_count' do describe '#total_items_count' do
before do before do
create :closed_issue, milestone: milestone, project: project create :closed_issue, milestone: milestone, project: project
create :merge_request, milestone: milestone create :merge_request, milestone: milestone, source_project: project
end end
it 'returns total count of issues and merge requests assigned to milestone' do it 'returns total count of issues and merge requests assigned to milestone' do
...@@ -192,10 +192,10 @@ ...@@ -192,10 +192,10 @@
describe '#can_be_closed?' do describe '#can_be_closed?' do
before do before do
milestone = create :milestone milestone = create :milestone, project: project
create :closed_issue, milestone: milestone create :closed_issue, milestone: milestone, project: project
create :issue create :issue, project: project
end end
it 'returns true if milestone active and all nested issues closed' do it 'returns true if milestone active and all nested issues closed' do
......
...@@ -49,7 +49,7 @@ ...@@ -49,7 +49,7 @@
create(:label, title: 'label', color: '#FFAABB', project: project) create(:label, title: 'label', color: '#FFAABB', project: project)
end end
let!(:label_link) { create(:label_link, label: label, target: issue) } let!(:label_link) { create(:label_link, label: label, target: issue) }
set(:milestone) { create(:milestone, title: '1.0.0', project: project) } let(:milestone) { create(:milestone, title: '1.0.0', project: project) }
set(:empty_milestone) do set(:empty_milestone) do
create(:milestone, title: '2.0.0', project: project) create(:milestone, title: '2.0.0', project: project)
end end
......
...@@ -3,7 +3,7 @@ ...@@ -3,7 +3,7 @@
describe Issuable::CommonSystemNotesService do describe Issuable::CommonSystemNotesService do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project) { create(:project) } let(:project) { create(:project) }
let(:issuable) { create(:issue) } let(:issuable) { create(:issue, project: project) }
context 'on issuable update' do context 'on issuable update' do
it_behaves_like 'system note creation', { title: 'New title' }, 'changed title' it_behaves_like 'system note creation', { title: 'New title' }, 'changed title'
...@@ -70,7 +70,7 @@ ...@@ -70,7 +70,7 @@
end end
context 'on issuable create' do context 'on issuable create' do
let(:issuable) { build(:issue) } let(:issuable) { build(:issue, project: project) }
subject { described_class.new(project, user).execute(issuable, old_labels: [], is_update: false) } subject { described_class.new(project, user).execute(issuable, old_labels: [], is_update: false) }
......
...@@ -8,29 +8,29 @@ ...@@ -8,29 +8,29 @@
project.add_developer(user) project.add_developer(user)
end end
def build_issue(issue_params = {})
described_class.new(project, user, issue_params).execute
end
context 'for a single discussion' do context 'for a single discussion' do
describe '#execute' do describe '#execute' do
let(:merge_request) { create(:merge_request, title: "Hello world", source_project: project) } let(:merge_request) { create(:merge_request, title: "Hello world", source_project: project) }
let(:discussion) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, note: "Almost done").to_discussion } let(:discussion) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, note: "Almost done").to_discussion }
let(:service) { described_class.new(project, user, merge_request_to_resolve_discussions_of: merge_request.iid, discussion_to_resolve: discussion.id) }
it 'references the noteable title in the issue title' do subject { build_issue(merge_request_to_resolve_discussions_of: merge_request.iid, discussion_to_resolve: discussion.id) }
issue = service.execute
expect(issue.title).to include('Hello world') it 'references the noteable title in the issue title' do
expect(subject.title).to include('Hello world')
end end
it 'adds the note content to the description' do it 'adds the note content to the description' do
issue = service.execute expect(subject.description).to include('Almost done')
expect(issue.description).to include('Almost done')
end end
end end
end end
context 'for discussions in a merge request' do context 'for discussions in a merge request' do
let(:merge_request) { create(:merge_request_with_diff_notes, source_project: project) } let(:merge_request) { create(:merge_request_with_diff_notes, source_project: project) }
let(:issue) { described_class.new(project, user, merge_request_to_resolve_discussions_of: merge_request.iid).execute }
describe '#items_for_discussions' do describe '#items_for_discussions' do
it 'has an item for each discussion' do it 'has an item for each discussion' do
...@@ -66,28 +66,30 @@ ...@@ -66,28 +66,30 @@
end end
describe '#execute' do describe '#execute' do
it 'has the merge request reference in the title' do let(:base_params) { { merge_request_to_resolve_discussions_of: merge_request.iid } }
expect(issue.title).to include(merge_request.title)
end
it 'has the reference of the merge request in the description' do context 'without additional params' do
expect(issue.description).to include(merge_request.to_reference) subject { build_issue(base_params) }
it 'has the merge request reference in the title' do
expect(subject.title).to include(merge_request.title)
end
it 'has the reference of the merge request in the description' do
expect(subject.description).to include(merge_request.to_reference)
end
end end
it 'does not assign title when a title was given' do it 'uses provided title if title param given' do
issue = described_class.new(project, user, issue = build_issue(base_params.merge(title: 'What an issue'))
merge_request_to_resolve_discussions_of: merge_request,
title: 'What an issue').execute
expect(issue.title).to eq('What an issue') expect(issue.title).to eq('What an issue')
end end
it 'does not assign description when a description was given' do it 'uses provided description if description param given' do
issue = described_class.new(project, user, issue = build_issue(base_params.merge(description: 'Fix at your earliest convenience'))
merge_request_to_resolve_discussions_of: merge_request,
description: 'Fix at your earliest conveignance').execute
expect(issue.description).to eq('Fix at your earliest conveignance') expect(issue.description).to eq('Fix at your earliest convenience')
end end
describe 'with multiple discussions' do describe 'with multiple discussions' do
...@@ -96,20 +98,20 @@ ...@@ -96,20 +98,20 @@
it 'mentions all the authors in the description' do it 'mentions all the authors in the description' do
authors = merge_request.resolvable_discussions.map(&:author) authors = merge_request.resolvable_discussions.map(&:author)
expect(issue.description).to include(*authors.map(&:to_reference)) expect(build_issue(base_params).description).to include(*authors.map(&:to_reference))
end end
it 'has a link for each unresolved discussion in the description' do it 'has a link for each unresolved discussion in the description' do
notes = merge_request.resolvable_discussions.map(&:first_note) notes = merge_request.resolvable_discussions.map(&:first_note)
links = notes.map { |note| Gitlab::UrlBuilder.build(note) } links = notes.map { |note| Gitlab::UrlBuilder.build(note) }
expect(issue.description).to include(*links) expect(build_issue(base_params).description).to include(*links)
end end
it 'mentions additional notes' do it 'mentions additional notes' do
create_list(:diff_note_on_merge_request, 2, noteable: merge_request, project: merge_request.target_project, in_reply_to: diff_note) create_list(:diff_note_on_merge_request, 2, noteable: merge_request, project: merge_request.target_project, in_reply_to: diff_note)
expect(issue.description).to include('(+2 comments)') expect(build_issue(base_params).description).to include('(+2 comments)')
end end
end end
end end
...@@ -120,7 +122,7 @@ ...@@ -120,7 +122,7 @@
describe '#execute' do describe '#execute' do
it 'mentions the merge request in the description' do it 'mentions the merge request in the description' do
issue = described_class.new(project, user, merge_request_to_resolve_discussions_of: merge_request.iid).execute issue = build_issue(merge_request_to_resolve_discussions_of: merge_request.iid)
expect(issue.description).to include("Review the conversation in #{merge_request.to_reference}") expect(issue.description).to include("Review the conversation in #{merge_request.to_reference}")
end end
...@@ -128,20 +130,18 @@ ...@@ -128,20 +130,18 @@
end end
describe '#execute' do describe '#execute' do
let(:milestone) { create(:milestone, project: project) }
it 'builds a new issues with given params' do it 'builds a new issues with given params' do
issue = described_class.new( milestone = create(:milestone, project: project)
project, issue = build_issue(milestone_id: milestone.id)
user,
title: 'Issue #1',
description: 'Issue description',
milestone_id: milestone.id
).execute
expect(issue.title).to eq('Issue #1')
expect(issue.description).to eq('Issue description')
expect(issue.milestone).to eq(milestone) expect(issue.milestone).to eq(milestone)
end end
it 'sets milestone to nil if it is not available for the project' do
milestone = create(:milestone, project: create(:project))
issue = build_issue(milestone_id: milestone.id)
expect(issue.milestone).to be_nil
end
end end
end end
...@@ -356,7 +356,7 @@ def update_issue(opts) ...@@ -356,7 +356,7 @@ def update_issue(opts)
it_behaves_like 'system notes for milestones' it_behaves_like 'system notes for milestones'
it 'sends notifications for subscribers of changed milestone' do it 'sends notifications for subscribers of changed milestone' do
issue.milestone = create(:milestone) issue.milestone = create(:milestone, project: project)
issue.save issue.save
...@@ -380,7 +380,7 @@ def update_issue(opts) ...@@ -380,7 +380,7 @@ def update_issue(opts)
end end