Merge branch 'security-2773-milestones-fix' into 'master'

[master] Check issue milestone availability

See merge request gitlab/gitlabhq!2788

app/models/concerns/issuable.rb

@@ -75,6 +75,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) }
@@ -118,6 +119,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

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
@@ -847,10 +850,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

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

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

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

changelogs/unreleased/51971-milestones-visibility.yml

+---
+title: Check if desired milestone for an issue is available
+merge_request:
+author:
+type: security

spec/controllers/dashboard/milestones_controller_spec.rb

@@ -13,7 +13,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]) }

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)

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,

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') }

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

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

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

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) }
 

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

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)

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
 

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)

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