Merge branch 'security-2774-milestones-detail' into 'master'

Display only information visible to current user on Milestone detail

See merge request gitlab/gitlab-ee!776

app/models/concerns/milestoneish.rb

@@ -46,6 +46,18 @@ module Milestoneish
     end
   end
 
+  def issue_participants_visible_by_user(user)
+    User.joins(:issue_assignees)
+      .where('issue_assignees.issue_id' => issues_visible_to_user(user).select(:id))
+      .distinct
+  end
+
+  def issue_labels_visible_by_user(user)
+    Label.joins(:label_links)
+      .where('label_links.target_id' => issues_visible_to_user(user).select(:id), 'label_links.target_type' => 'Issue')
+      .distinct
+  end
+
   def sorted_issues(user)
     issues_visible_to_user(user).preload_associations.sort_by_attribute('label_priority')
   end

app/views/shared/milestones/_tabs.html.haml

@@ -21,11 +21,11 @@
     %li.nav-item
       = link_to '#tab-participants', class: 'nav-link', 'data-toggle' => 'tab', 'data-endpoint': milestone_participants_tab_path(milestone) do
         Participants
-        %span.badge.badge-pill= milestone.participants.count
+        %span.badge.badge-pill= milestone.issue_participants_visible_by_user(current_user).count
     %li.nav-item
       = link_to '#tab-labels', class: 'nav-link', 'data-toggle' => 'tab', 'data-endpoint': milestone_labels_tab_path(milestone) do
         Labels
-        %span.badge.badge-pill= milestone.labels.count
+        %span.badge.badge-pill= milestone.issue_labels_visible_by_user(current_user).count
 
 - issues = milestone.sorted_issues(current_user)
 - show_project_name = local_assigns.fetch(:show_project_name, false)

changelogs/unreleased/security-2774-milestones-detail.yml

+---
+title: Display only information visible to current user on the Milestone page
+merge_request:
+author:
+type: security

ee/app/helpers/ee/milestones_helper.rb

@@ -3,7 +3,7 @@
 module EE
   module MilestonesHelper
     def burndown_chart(milestone)
-      Burndown.new(milestone) if milestone.supports_burndown_charts?
+      Burndown.new(milestone, current_user) if milestone.supports_burndown_charts?
     end
 
     def can_generate_chart?(milestone, burndown)

ee/app/models/burndown.rb

@@ -17,12 +17,13 @@ class Burndown
     end
   end
 
-  attr_reader :start_date, :due_date, :end_date, :accurate, :legacy_data, :milestone
+  attr_reader :start_date, :due_date, :end_date, :accurate, :legacy_data, :milestone, :current_user
   alias_method :accurate?, :accurate
   alias_method :empty?, :legacy_data
 
-  def initialize(milestone)
+  def initialize(milestone, current_user)
     @milestone = milestone
+    @current_user = current_user
     @start_date = @milestone.start_date
     @due_date = @milestone.due_date
     @end_date = @milestone.due_date
@@ -89,10 +90,10 @@ class Burndown
     strong_memoize(:opened_issues_grouped_by_date) do
       issues =
         @milestone
-          .issues
-          .where('created_at <= ?', end_date)
+          .issues_visible_to_user(current_user)
+          .where('issues.created_at <= ?', end_date)
           .reorder(nil)
-          .order(:created_at).to_a
+          .order('issues.created_at').to_a
 
       issues.group_by do |issue|
         issue.created_at.to_date
@@ -124,9 +125,8 @@ class Burndown
         # `issues.closed_at` can't be used once it's nullified if the issue is
         # reopened.
         internal_clause =
-          ::Issue
+          @milestone.issues_visible_to_user(current_user)
             .joins("LEFT OUTER JOIN events e ON issues.id = e.target_id AND e.target_type = 'Issue' AND e.action = #{Event::CLOSED}")
-            .where(milestone: @milestone)
             .where("state = 'closed' OR (state = 'opened' AND e.action = #{Event::CLOSED})") # rubocop:disable GitlabSecurity/SqlInjection
 
         rel =

ee/spec/models/burndown_spec.rb

@@ -2,6 +2,7 @@ require 'spec_helper'
 
 describe Burndown do
   set(:user) { create(:user) }
+  set(:non_member) { create(:user) }
   let(:start_date) { "2017-03-01" }
   let(:due_date) { "2017-03-03" }
 
@@ -16,13 +17,13 @@ describe Burndown do
       end
     end
 
-    subject { described_class.new(milestone).to_json }
+    subject { described_class.new(milestone, user).to_json }
 
     it "generates an array with date, issue count and weight" do
       expect(subject).to eq([
-        ["2017-03-01", 3, 6],
-        ["2017-03-02", 4, 8],
-        ["2017-03-03", 2, 4]
+        ["2017-03-01", 4, 8],
+        ["2017-03-02", 5, 10],
+        ["2017-03-03", 3, 6]
       ].to_json)
     end
 
@@ -45,7 +46,7 @@ describe Burndown do
     end
 
     it "sets attribute accurate to true" do
-      burndown = described_class.new(milestone)
+      burndown = described_class.new(milestone, user)
 
       expect(burndown).to be_accurate
     end
@@ -57,14 +58,14 @@ describe Burndown do
 
       it "considers closed_at as milestone start date" do
         expect(subject).to eq([
-          ["2017-03-01", 3, 6],
-          ["2017-03-02", 3, 6],
-          ["2017-03-03", 3, 6]
+          ["2017-03-01", 4, 8],
+          ["2017-03-02", 4, 8],
+          ["2017-03-03", 4, 8]
         ].to_json)
       end
 
       it "sets attribute empty to true" do
-        burndown = described_class.new(milestone)
+        burndown = described_class.new(milestone, user)
 
         expect(burndown).to be_empty
       end
@@ -76,7 +77,7 @@ describe Burndown do
       end
 
       it "sets attribute accurate to false" do
-        burndown = described_class.new(milestone)
+        burndown = described_class.new(milestone, user)
 
         expect(burndown).not_to be_accurate
       end
@@ -92,12 +93,26 @@ describe Burndown do
         create(:issue, milestone: milestone, project: project, created_at: creation_date, weight: 3)
 
         expect(subject).to eq([
-          ['2017-03-01', 3, 6],
-          ['2017-03-02', 6, 13],
-          ['2017-03-03', 4, 9]
+          ['2017-03-01', 4, 8],
+          ['2017-03-02', 7, 15],
+          ['2017-03-03', 5, 11]
         ].to_json)
       end
     end
+
+    context 'when issues belong to a public project' do
+      it 'does not include confidential issues for users who are not project members' do
+        project.update(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+
+        expected_result = [
+            ["2017-03-01", 3, 6],
+            ["2017-03-02", 4, 8],
+            ["2017-03-03", 2, 4]
+        ].to_json
+
+        expect(described_class.new(milestone, non_member).to_json).to eq(expected_result)
+      end
+    end
   end
 
   describe 'project milestone burndown' do
@@ -126,6 +141,10 @@ describe Burndown do
     let(:nested_group_milestone) { create(:milestone, group: nested_group, start_date: start_date, due_date: due_date) }
 
     context 'when nested group milestone', :nested_groups do
+      before do
+        group.add_developer(user)
+      end
+
       it_behaves_like 'burndown for milestone' do
         let(:milestone) { nested_group_milestone }
         let(:project) { nested_group_project }
@@ -194,6 +213,9 @@ describe Burndown do
         issue_closed_twice = reopened_issues.last
         close_issue(issue_closed_twice)
         reopen_issue(issue_closed_twice)
+
+        # create one confidential issue
+        create(:issue, :confidential, issue_params) if Date.today == milestone.start_date
       end
     end
   end

spec/models/concerns/milestoneish_spec.rb

@@ -9,8 +9,10 @@ describe Milestone, 'Milestoneish' do
   let(:admin) { create(:admin) }
   let(:project) { create(:project, :public) }
   let(:milestone) { create(:milestone, project: project) }
-  let!(:issue) { create(:issue, project: project, milestone: milestone) }
-  let!(:security_issue_1) { create(:issue, :confidential, project: project, author: author, milestone: milestone) }
+  let(:label1) { create(:label, project: project) }
+  let(:label2) { create(:label, project: project) }
+  let!(:issue) { create(:issue, project: project, milestone: milestone, assignees: [member], labels: [label1]) }
+  let!(:security_issue_1) { create(:issue, :confidential, project: project, author: author, milestone: milestone, labels: [label2]) }
   let!(:security_issue_2) { create(:issue, :confidential, project: project, assignees: [assignee], milestone: milestone) }
   let!(:closed_issue_1) { create(:issue, :closed, project: project, milestone: milestone) }
   let!(:closed_issue_2) { create(:issue, :closed, project: project, milestone: milestone) }
@@ -42,6 +44,95 @@ describe Milestone, 'Milestoneish' do
     end
   end
 
+  context 'attributes visibility' do
+    using RSpec::Parameterized::TableSyntax
+
+    let(:users) do
+      {
+        anonymous: nil,
+        non_member: non_member,
+        guest: guest,
+        member: member,
+        assignee: assignee
+      }
+    end
+
+    let(:project_visibility_levels) do
+      {
+        public: Gitlab::VisibilityLevel::PUBLIC,
+        internal: Gitlab::VisibilityLevel::INTERNAL,
+        private: Gitlab::VisibilityLevel::PRIVATE
+      }
+    end
+
+    describe '#issue_participants_visible_by_user' do
+      where(:visibility, :user_role, :result) do
+        :public   | nil         | [:member]
+        :public   | :non_member | [:member]
+        :public   | :guest      | [:member]
+        :public   | :member     | [:member, :assignee]
+        :internal | nil         | []
+        :internal | :non_member | [:member]
+        :internal | :guest      | [:member]
+        :internal | :member     | [:member, :assignee]
+        :private  | nil         | []
+        :private  | :non_member | []
+        :private  | :guest      | [:member]
+        :private  | :member     | [:member, :assignee]
+      end
+
+      with_them do
+        before do
+          project.update(visibility_level: project_visibility_levels[visibility])
+        end
+
+        it 'returns the proper participants' do
+          user = users[user_role]
+          participants = result.map { |role| users[role] }
+
+          expect(milestone.issue_participants_visible_by_user(user)).to match_array(participants)
+        end
+      end
+    end
+
+    describe '#issue_labels_visible_by_user' do
+      let(:labels) do
+        {
+          label1: label1,
+          label2: label2
+        }
+      end
+
+      where(:visibility, :user_role, :result) do
+        :public   | nil         | [:label1]
+        :public   | :non_member | [:label1]
+        :public   | :guest      | [:label1]
+        :public   | :member     | [:label1, :label2]
+        :internal | nil         | []
+        :internal | :non_member | [:label1]
+        :internal | :guest      | [:label1]
+        :internal | :member     | [:label1, :label2]
+        :private  | nil         | []
+        :private  | :non_member | []
+        :private  | :guest      | [:label1]
+        :private  | :member     | [:label1, :label2]
+      end
+
+      with_them do
+        before do
+          project.update(visibility_level: project_visibility_levels[visibility])
+        end
+
+        it 'returns the proper participants' do
+          user = users[user_role]
+          expected_labels = result.map { |label| labels[label] }
+
+          expect(milestone.issue_labels_visible_by_user(user)).to match_array(expected_labels)
+        end
+      end
+    end
+  end
+
   describe '#sorted_merge_requests' do
     it 'sorts merge requests by label priority' do
       merge_request_1 = create(:labeled_merge_request, labels: [label_2], source_project: project, source_branch: 'branch_1', milestone: milestone)