Merge branch 'security-id-fix-mr-visibility' into 'master'

Display the correct number of MRs a user has access to

Closes #2790

See merge request gitlab/gitlabhq!2880

app/models/concerns/milestoneish.rb

@@ -46,13 +46,6 @@ module Milestoneish
     end
   end
 
-  def merge_requests_visible_to_user(user)
-    memoize_per_user(user, :merge_requests_visible_to_user) do
-      MergeRequestsFinder.new(user, {})
-        .execute.where(milestone_id: milestoneish_id)
-    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))
@@ -73,6 +66,13 @@ module Milestoneish
     merge_requests_visible_to_user(user).sort_by_attribute('label_priority')
   end
 
+  def merge_requests_visible_to_user(user)
+    memoize_per_user(user, :merge_requests_visible_to_user) do
+      MergeRequestsFinder.new(user, issues_finder_params)
+        .execute.where(milestone_id: milestoneish_id)
+    end
+  end
+
   def upcoming?
     start_date && start_date.future?
   end

app/models/project_feature.rb

@@ -76,7 +76,7 @@ class ProjectFeature < ActiveRecord::Base
     # This feature might not be behind a feature flag at all, so default to true
     return false unless ::Feature.enabled?(feature, user, default_enabled: true)
 
-    get_permission(user, access_level(feature))
+    get_permission(user, feature)
   end
 
   def access_level(feature)
@@ -134,12 +134,12 @@ class ProjectFeature < ActiveRecord::Base
     (FEATURES - %i(pages)).each {|f| validator.call("#{f}_access_level")}
   end
 
-  def get_permission(user, level)
-    case level
+  def get_permission(user, feature)
+    case access_level(feature)
     when DISABLED
       false
     when PRIVATE
-      user && (project.team.member?(user) || user.full_private_access?)
+      team_access?(user, feature)
     when ENABLED
       true
     when PUBLIC
@@ -148,4 +148,11 @@ class ProjectFeature < ActiveRecord::Base
       true
     end
   end
+
+  def team_access?(user, feature)
+    return unless user
+    return true if user.full_private_access?
+
+    project.team.member?(user, ProjectFeature.required_minimum_access_level(feature))
+  end
 end

app/policies/project_policy.rb

@@ -465,7 +465,7 @@ class ProjectPolicy < BasePolicy
     when ProjectFeature::DISABLED
       false
     when ProjectFeature::PRIVATE
-      guest? || admin?
+      admin? || team_access_level >= ProjectFeature.required_minimum_access_level(feature)
     else
       true
     end

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

@@ -32,7 +32,7 @@
       = milestone_progress_bar(milestone)
       = link_to pluralize(milestone.total_issues_count(current_user), 'Issue'), issues_path
       ·
-      = link_to pluralize(milestone.merge_requests.size, 'Merge Request'), merge_requests_path
+      = link_to pluralize(milestone.merge_requests_visible_to_user(current_user).size, 'Merge Request'), merge_requests_path
       .float-lg-right.light #{milestone.percent_complete(current_user)}% complete
     .col-sm-2
       .milestone-actions.d-flex.justify-content-sm-start.justify-content-md-end

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

@@ -12,7 +12,7 @@
       %li.nav-item
         = link_to '#tab-merge-requests', class: 'nav-link', 'data-toggle' => 'tab', 'data-endpoint': milestone_merge_request_tab_path(milestone) do
           Merge Requests
-          %span.badge.badge-pill= milestone.merge_requests.size
+          %span.badge.badge-pill= milestone.merge_requests_visible_to_user(current_user).size
     - else
       %li.nav-item
         = link_to '#tab-merge-requests', class: 'nav-link active', 'data-toggle' => 'tab', 'data-endpoint': milestone_merge_request_tab_path(milestone) do

changelogs/unreleased/security-id-fix-mr-visibility.yml

+---
+title: Display the correct number of MRs a user has access to
+merge_request:
+author:
+type: security

spec/finders/merge_requests_finder_spec.rb

@@ -13,6 +13,7 @@ describe MergeRequestsFinder do
     end
   end
 
+  context "multiple projects with merge requests" do
     let(:user)  { create :user }
     let(:user2) { create :user }
 
@@ -55,7 +56,7 @@ describe MergeRequestsFinder do
       project6.add_developer(user)
     end
 
-  describe "#execute" do
+    describe '#execute' do
       it 'filters by scope' do
         params = { scope: 'authored', state: 'opened' }
         merge_requests = described_class.new(user, params).execute
@@ -278,6 +279,49 @@ describe MergeRequestsFinder do
           expect(merge_requests).to contain_exactly(old_merge_request, new_merge_request)
         end
       end
+    end
+
+    describe '#row_count', :request_store do
+      it 'returns the number of rows for the default state' do
+        finder = described_class.new(user)
+
+        expect(finder.row_count).to eq(7)
+      end
+
+      it 'returns the number of rows for a given state' do
+        finder = described_class.new(user, state: 'closed')
+
+        expect(finder.row_count).to eq(1)
+      end
+    end
+  end
+
+  context 'when projects require different access levels for merge requests' do
+    let(:user) { create(:user) }
+
+    let(:public_project) { create(:project, :public) }
+    let(:internal) { create(:project, :internal) }
+    let(:private_project) { create(:project, :private) }
+    let(:public_with_private_repo) { create(:project, :public, :repository, :repository_private) }
+    let(:internal_with_private_repo) { create(:project, :internal, :repository, :repository_private) }
+
+    let(:merge_requests) { described_class.new(user, {}).execute }
+
+    let!(:mr_public) { create(:merge_request, source_project: public_project) }
+    let!(:mr_private) { create(:merge_request, source_project: private_project) }
+    let!(:mr_internal) { create(:merge_request, source_project: internal) }
+    let!(:mr_private_repo_access) { create(:merge_request, source_project: public_with_private_repo) }
+    let!(:mr_internal_private_repo_access) { create(:merge_request, source_project: internal_with_private_repo) }
+
+    context 'with admin user' do
+      let(:user) { create(:user, :admin) }
+
+      it 'returns all merge requests' do
+        expect(merge_requests).to eq(
+          [mr_internal_private_repo_access, mr_private_repo_access, mr_internal, mr_private, mr_public]
+        )
+      end
+    end
 
     context 'when project restricts merge requests' do
       let(:non_member) { create(:user) }
@@ -293,19 +337,85 @@ describe MergeRequestsFinder do
         expect(merge_requests).to be_empty
       end
     end
+
+    context 'with external user' do
+      let(:user) { create(:user, :external) }
+
+      it 'returns only public merge requests' do
+        expect(merge_requests).to eq([mr_public])
+      end
     end
 
-  describe '#row_count', :request_store do
-    it 'returns the number of rows for the default state' do
-      finder = described_class.new(user)
+    context 'with authenticated user' do
+      it 'returns public and internal merge requests' do
+        expect(merge_requests).to eq([mr_internal, mr_public])
+      end
 
-      expect(finder.row_count).to eq(7)
+      context 'being added to the private project' do
+        context 'as a guest' do
+          before do
+            private_project.add_guest(user)
           end
 
-    it 'returns the number of rows for a given state' do
-      finder = described_class.new(user, state: 'closed')
+          it 'does not return merge requests from the private project' do
+            expect(merge_requests).to eq([mr_internal, mr_public])
+          end
+        end
 
-      expect(finder.row_count).to eq(1)
+        context 'as a developer' do
+          before do
+            private_project.add_developer(user)
+          end
+
+          it 'returns merge requests from the private project' do
+            expect(merge_requests).to eq([mr_internal, mr_private, mr_public])
+          end
+        end
+      end
+
+      context 'being added to the public project with private repo access' do
+        context 'as a guest' do
+          before do
+            public_with_private_repo.add_guest(user)
+          end
+
+          it 'returns merge requests from the project' do
+            expect(merge_requests).to eq([mr_internal, mr_public])
+          end
+        end
+
+        context 'as a reporter' do
+          before do
+            public_with_private_repo.add_reporter(user)
+          end
+
+          it 'returns merge requests from the project' do
+            expect(merge_requests).to eq([mr_private_repo_access, mr_internal, mr_public])
+          end
+        end
+      end
+
+      context 'being added to the internal project with private repo access' do
+        context 'as a guest' do
+          before do
+            internal_with_private_repo.add_guest(user)
+          end
+
+          it 'returns merge requests from the project' do
+            expect(merge_requests).to eq([mr_internal, mr_public])
+          end
+        end
+
+        context 'as a reporter' do
+          before do
+            internal_with_private_repo.add_reporter(user)
+          end
+
+          it 'returns merge requests from the project' do
+            expect(merge_requests).to eq([mr_internal_private_repo_access, mr_internal, mr_public])
+          end
+        end
+      end
     end
   end
 end