diff options
19 files changed, 187 insertions, 73 deletions
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 0a77fbeba08..8918dc8f417 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -74,6 +74,7 @@ module Issuable validates :author, presence: true validates :title, presence: true, length: { maximum: 255 } + validate :milestone_is_valid scope :authored, ->(user) { where(author_id: user) } scope :recent, -> { reorder(id: :desc) } @@ -117,6 +118,16 @@ module Issuable def has_multiple_assignees? assignees.count > 1 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 class_methods do diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 2035bffd829..a551e3e387d 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -189,6 +189,9 @@ class MergeRequest < ActiveRecord::Base after_save :keep_around_commit + alias_attribute :project, :target_project + alias_attribute :project_id, :target_project_id + def self.reference_prefix '!' end @@ -837,10 +840,6 @@ class MergeRequest < ActiveRecord::Base target_project != source_project end - def project - target_project - end - # If the merge request closes any issues, save this information in the # `MergeRequestsClosingIssues` model. This is a performance optimization. # Calculating this information for a number of merge requests requires diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index ef991eaf234..1e1f2fbd08e 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -387,4 +387,10 @@ class IssuableBaseService < BaseService def parent project 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 diff --git a/app/services/issues/build_service.rb b/app/services/issues/build_service.rb index 52b45f1b2ce..77724e78972 100644 --- a/app/services/issues/build_service.rb +++ b/app/services/issues/build_service.rb @@ -6,7 +6,9 @@ module Issues def execute 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 def issue_params_with_info_from_discussions diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index 48419da98ad..109c964e577 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -19,6 +19,7 @@ module MergeRequests merge_request.target_project = find_target_project merge_request.target_branch = find_target_branch merge_request.can_be_created = projects_and_branches_valid? + ensure_milestone_available(merge_request) # compare branches only if branches are valid, otherwise # compare_branches may raise an error diff --git a/changelogs/unreleased/51971-milestones-visibility.yml b/changelogs/unreleased/51971-milestones-visibility.yml new file mode 100644 index 00000000000..818f0071e6c --- /dev/null +++ b/changelogs/unreleased/51971-milestones-visibility.yml @@ -0,0 +1,5 @@ +--- +title: Check if desired milestone for an issue is available +merge_request: +author: +type: security diff --git a/spec/controllers/dashboard/milestones_controller_spec.rb b/spec/controllers/dashboard/milestones_controller_spec.rb index 8b176e07bc8..ddf33ebad16 100644 --- a/spec/controllers/dashboard/milestones_controller_spec.rb +++ b/spec/controllers/dashboard/milestones_controller_spec.rb @@ -15,7 +15,7 @@ describe Dashboard::MilestonesController do ) end 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!(:group_label) { create(:group_label, group: group, title: 'Group Issue Label', issues: [group_issue]) } diff --git a/spec/features/issues_spec.rb b/spec/features/issues_spec.rb index 406e80e91aa..9bc340ed4bb 100644 --- a/spec/features/issues_spec.rb +++ b/spec/features/issues_spec.rb @@ -233,8 +233,8 @@ describe 'Issues' do created_at: Time.now - (index * 60)) end end - let(:newer_due_milestone) { create(:milestone, due_date: '2013-12-11') } - let(:later_due_milestone) { create(:milestone, due_date: '2013-12-12') } + let(:newer_due_milestone) { create(:milestone, project: project, due_date: '2013-12-11') } + let(:later_due_milestone) { create(:milestone, project: project, due_date: '2013-12-12') } it 'sorts by newest' do visit project_issues_path(project, sort: sort_value_created_date) diff --git a/spec/features/merge_requests/user_lists_merge_requests_spec.rb b/spec/features/merge_requests/user_lists_merge_requests_spec.rb index ef7ae490b0f..c691011b9ca 100644 --- a/spec/features/merge_requests/user_lists_merge_requests_spec.rb +++ b/spec/features/merge_requests/user_lists_merge_requests_spec.rb @@ -13,7 +13,7 @@ describe 'Merge requests > User lists merge requests' do source_project: project, source_branch: 'fix', 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, updated_at: 1.minute.ago) create(:merge_request, @@ -21,7 +21,7 @@ describe 'Merge requests > User lists merge requests' do source_project: project, source_branch: 'markdown', 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, updated_at: 2.minutes.ago) create(:merge_request, diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index 41159348e04..72c6161424b 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -32,17 +32,56 @@ describe Issuable do end describe "Validation" do - subject { build(:issue) } + context 'general validations' do + subject { build(:issue) } - before do - allow(InternalId).to receive(:generate_next).and_return(nil) + before do + 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 - 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) } + describe 'milestone' do + let(:project) { create(:project) } + let(:milestone_id) { create(:milestone, project: project).id } + let(:params) do + { + 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 describe "Scope" do @@ -66,6 +105,48 @@ describe Issuable do 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 let!(:searchable_issue) { create(:issue, title: "Searchable awesome issue") } let!(:searchable_issue2) { create(:issue, title: 'Aw') } diff --git a/spec/models/issue/metrics_spec.rb b/spec/models/issue/metrics_spec.rb index 1bf0ecb98ad..b7291eebe64 100644 --- a/spec/models/issue/metrics_spec.rb +++ b/spec/models/issue/metrics_spec.rb @@ -9,7 +9,7 @@ describe Issue::Metrics do context "milestones" do it "records the first time an issue is associated with a milestone" do time = Time.now - Timecop.freeze(time) { subject.update(milestone: create(:milestone)) } + Timecop.freeze(time) { subject.update(milestone: create(:milestone, project: project)) } metrics = subject.metrics expect(metrics).to be_present @@ -18,9 +18,9 @@ describe Issue::Metrics do it "does not record the second time an issue is associated with a milestone" do 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 + 6.hours) { subject.update(milestone: create(:milestone)) } + Timecop.freeze(time + 6.hours) { subject.update(milestone: create(:milestone, project: project)) } metrics = subject.metrics expect(metrics).to be_present diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index af7e3d3a6c9..77b7042424c 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -182,7 +182,7 @@ describe Milestone do describe '#total_items_count' do before do create :closed_issue, milestone: milestone, project: project - create :merge_request, milestone: milestone + create :merge_request, milestone: milestone, source_project: project end it 'returns total count of issues and merge requests assigned to milestone' do @@ -192,10 +192,10 @@ describe Milestone do describe '#can_be_closed?' do before do - milestone = create :milestone - create :closed_issue, milestone: milestone + milestone = create :milestone, project: project + create :closed_issue, milestone: milestone, project: project - create :issue + create :issue, project: project end it 'returns true if milestone active and all nested issues closed' do diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 04908378a24..46cd3ec88e1 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -49,7 +49,7 @@ describe API::Issues do create(:label, title: 'label', color: '#FFAABB', project: project) end 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 create(:milestone, title: '2.0.0', project: project) end diff --git a/spec/services/issuable/common_system_notes_service_spec.rb b/spec/services/issuable/common_system_notes_service_spec.rb index fa5d5ebac5c..0edc9016c96 100644 --- a/spec/services/issuable/common_system_notes_service_spec.rb +++ b/spec/services/issuable/common_system_notes_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Issuable::CommonSystemNotesService do let(:user) { create(:user) } let(:project) { create(:project) } - let(:issuable) { create(:issue) } + let(:issuable) { create(:issue, project: project) } context 'on issuable update' do it_behaves_like 'system note creation', { title: 'New title' }, 'changed title' @@ -70,7 +70,7 @@ describe Issuable::CommonSystemNotesService do end 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) } diff --git a/spec/services/issues/build_service_spec.rb b/spec/services/issues/build_service_spec.rb index 248e7d5a389..86e58fe06b9 100644 --- a/spec/services/issues/build_service_spec.rb +++ b/spec/services/issues/build_service_spec.rb @@ -8,29 +8,29 @@ describe Issues::BuildService do project.add_developer(user) end + def build_issue(issue_params = {}) + described_class.new(project, user, issue_params).execute + end + context 'for a single discussion' do describe '#execute' do 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(: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 - issue = service.execute + subject { build_issue(merge_request_to_resolve_discussions_of: merge_request.iid, discussion_to_resolve: discussion.id) } - 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 it 'adds the note content to the description' do - issue = service.execute - - expect(issue.description).to include('Almost done') + expect(subject.description).to include('Almost done') end end end context 'for discussions in a merge request' do 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 it 'has an item for each discussion' do @@ -66,28 +66,30 @@ describe Issues::BuildService do end describe '#execute' do - it 'has the merge request reference in the title' do - expect(issue.title).to include(merge_request.title) - end + let(:base_params) { { merge_request_to_resolve_discussions_of: merge_request.iid } } - it 'has the reference of the merge request in the description' do - expect(issue.description).to include(merge_request.to_reference) + context 'without additional params' do + 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 - it 'does not assign title when a title was given' do - issue = described_class.new(project, user, - merge_request_to_resolve_discussions_of: merge_request, - title: 'What an issue').execute + it 'uses provided title if title param given' do + issue = build_issue(base_params.merge(title: 'What an issue')) expect(issue.title).to eq('What an issue') end - it 'does not assign description when a description was given' do - issue = described_class.new(project, user, - merge_request_to_resolve_discussions_of: merge_request, - description: 'Fix at your earliest conveignance').execute + it 'uses provided description if description param given' do + issue = build_issue(base_params.merge(description: 'Fix at your earliest convenience')) - expect(issue.description).to eq('Fix at your earliest conveignance') + expect(issue.description).to eq('Fix at your earliest convenience') end describe 'with multiple discussions' do @@ -96,20 +98,20 @@ describe Issues::BuildService do it 'mentions all the authors in the description' do 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 it 'has a link for each unresolved discussion in the description' do notes = merge_request.resolvable_discussions.map(&:first_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 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) - expect(issue.description).to include('(+2 comments)') + expect(build_issue(base_params).description).to include('(+2 comments)') end end end @@ -120,7 +122,7 @@ describe Issues::BuildService do describe '#execute' 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}") end @@ -128,20 +130,18 @@ describe Issues::BuildService do end describe '#execute' do - let(:milestone) { create(:milestone, project: project) } - it 'builds a new issues with given params' do - issue = described_class.new( - project, - 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') + milestone = create(:milestone, project: project) + issue = build_issue(milestone_id: milestone.id) + expect(issue.milestone).to eq(milestone) 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 diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 931e47d3a77..f1684209729 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -356,7 +356,7 @@ describe Issues::UpdateService, :mailer do it_behaves_like 'system notes for milestones' it 'sends notifications for subscribers of changed milestone' do - issue.milestone = create(:milestone) + issue.milestone = create(:milestone, project: project) issue.save @@ -380,7 +380,7 @@ describe Issues::UpdateService, :mailer do end it 'marks todos as done' do - update_issue(milestone: create(:milestone)) + update_issue(milestone: create(:milestone, project: project)) expect(todo.reload.done?).to eq true end @@ -389,7 +389,7 @@ describe Issues::UpdateService, :mailer do it 'sends notifications for subscribers of changed milestone' do perform_enqueued_jobs do - update_issue(milestone: create(:milestone)) + update_issue(milestone: create(:milestone, project: project)) end should_email(subscriber) diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index 536d0d345a4..057e8137a4e 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -229,6 +229,15 @@ describe MergeRequests::BuildService do end end end + + context 'when a milestone is from another project' do + let(:milestone) { create(:milestone, project: create(:project)) } + let(:milestone_id) { milestone.id } + + it 'sets milestone to nil' do + expect(merge_request.milestone).to be_nil + end + end end end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 20580bf14b9..8e367db031c 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -328,7 +328,7 @@ describe MergeRequests::UpdateService, :mailer do it_behaves_like 'system notes for milestones' it 'sends notifications for subscribers of changed milestone' do - merge_request.milestone = create(:milestone) + merge_request.milestone = create(:milestone, project: project) merge_request.save @@ -352,7 +352,7 @@ describe MergeRequests::UpdateService, :mailer do end it 'marks pending todos as done' do - update_merge_request({ milestone: create(:milestone) }) + update_merge_request({ milestone: create(:milestone, project: project) }) expect(pending_todo.reload).to be_done end @@ -361,7 +361,7 @@ describe MergeRequests::UpdateService, :mailer do it 'sends notifications for subscribers of changed milestone' do perform_enqueued_jobs do - update_merge_request(milestone: create(:milestone)) + update_merge_request(milestone: create(:milestone, project: project)) end should_email(subscriber) diff --git a/spec/support/shared_examples/issuable_shared_examples.rb b/spec/support/shared_examples/issuable_shared_examples.rb index c3d40c5b231..d97b21f71cd 100644 --- a/spec/support/shared_examples/issuable_shared_examples.rb +++ b/spec/support/shared_examples/issuable_shared_examples.rb @@ -31,7 +31,7 @@ shared_examples 'system notes for milestones' do context 'project milestones' do it 'creates a system note' do expect do - update_issuable(milestone: create(:milestone)) + update_issuable(milestone: create(:milestone, project: project)) end.to change { Note.system.count }.by(1) end end |