Display the correct number of MRs a user has access to

app/models/concerns/milestoneish.rb

@@ -46,13 +46,6 @@ def issues_visible_to_user(user)
     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 sorted_issues(user)
     issues_visible_to_user(user).preload_associations.sort_by_attribute('label_priority')
   end
@@ -61,6 +54,13 @@ def sorted_merge_requests(user)
     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 @@ def feature_available?(feature, user)
     # 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 @@ def allowed_access_levels
     (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 @@ def get_permission(user, level)
       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

@@ -464,7 +464,7 @@ def feature_available?(feature)
     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,269 +13,313 @@ def create_project_without_n_plus_1(*args)
     end
   end
 
-  let(:user)  { create :user }
-  let(:user2) { create :user }
-
-  let(:group) { create(:group) }
-  let(:subgroup) { create(:group, parent: group) }
-  let(:project1) { create_project_without_n_plus_1(group: group) }
-  let(:project2) do
-    Gitlab::GitalyClient.allow_n_plus_1_calls do
-      fork_project(project1, user)
+  context "multiple projects with merge requests" do
+    let(:user)  { create :user }
+    let(:user2) { create :user }
+
+    let(:group) { create(:group) }
+    let(:subgroup) { create(:group, parent: group) }
+    let(:project1) { create_project_without_n_plus_1(group: group) }
+    let(:project2) do
+      Gitlab::GitalyClient.allow_n_plus_1_calls do
+        fork_project(project1, user)
+      end
     end
-  end
-  let(:project3) do
-    Gitlab::GitalyClient.allow_n_plus_1_calls do
-      p = fork_project(project1, user)
-      p.update!(archived: true)
-      p
+    let(:project3) do
+      Gitlab::GitalyClient.allow_n_plus_1_calls do
+        p = fork_project(project1, user)
+        p.update!(archived: true)
+        p
+      end
     end
-  end
-  let(:project4) { create_project_without_n_plus_1(:repository, group: subgroup) }
-  let(:project5) { create_project_without_n_plus_1(group: subgroup) }
-  let(:project6) { create_project_without_n_plus_1(group: subgroup) }
-
-  let!(:merge_request1) { create(:merge_request, :simple, author: user, source_project: project2, target_project: project1) }
-  let!(:merge_request2) { create(:merge_request, :conflict, author: user, source_project: project2, target_project: project1, state: 'closed') }
-  let!(:merge_request3) { create(:merge_request, :simple, author: user, source_project: project2, target_project: project2, state: 'locked', title: 'thing WIP thing') }
-  let!(:merge_request4) { create(:merge_request, :simple, author: user, source_project: project3, target_project: project3, title: 'WIP thing') }
-  let!(:merge_request5) { create(:merge_request, :simple, author: user, source_project: project4, target_project: project4, title: '[WIP]') }
-  let!(:merge_request6) { create(:merge_request, :simple, author: user, source_project: project5, target_project: project5, title: 'WIP: thing') }
-  let!(:merge_request7) { create(:merge_request, :simple, author: user, source_project: project6, target_project: project6, title: 'wip thing') }
-  let!(:merge_request8) { create(:merge_request, :simple, author: user, source_project: project1, target_project: project1, title: '[wip] thing') }
-  let!(:merge_request9) { create(:merge_request, :simple, author: user, source_project: project1, target_project: project2, title: 'wip: thing') }
-
-  before do
-    project1.add_maintainer(user)
-    project2.add_developer(user)
-    project3.add_developer(user)
-    project2.add_developer(user2)
-    project4.add_developer(user)
-    project5.add_developer(user)
-    project6.add_developer(user)
-  end
-
-  describe "#execute" do
-    it 'filters by scope' do
-      params = { scope: 'authored', state: 'opened' }
-      merge_requests = described_class.new(user, params).execute
-      expect(merge_requests.size).to eq(7)
+    let(:project4) { create_project_without_n_plus_1(:repository, group: subgroup) }
+    let(:project5) { create_project_without_n_plus_1(group: subgroup) }
+    let(:project6) { create_project_without_n_plus_1(group: subgroup) }
+
+    let!(:merge_request1) { create(:merge_request, :simple, author: user, source_project: project2, target_project: project1) }
+    let!(:merge_request2) { create(:merge_request, :conflict, author: user, source_project: project2, target_project: project1, state: 'closed') }
+    let!(:merge_request3) { create(:merge_request, :simple, author: user, source_project: project2, target_project: project2, state: 'locked', title: 'thing WIP thing') }
+    let!(:merge_request4) { create(:merge_request, :simple, author: user, source_project: project3, target_project: project3, title: 'WIP thing') }
+    let!(:merge_request5) { create(:merge_request, :simple, author: user, source_project: project4, target_project: project4, title: '[WIP]') }
+    let!(:merge_request6) { create(:merge_request, :simple, author: user, source_project: project5, target_project: project5, title: 'WIP: thing') }
+    let!(:merge_request7) { create(:merge_request, :simple, author: user, source_project: project6, target_project: project6, title: 'wip thing') }
+    let!(:merge_request8) { create(:merge_request, :simple, author: user, source_project: project1, target_project: project1, title: '[wip] thing') }
+    let!(:merge_request9) { create(:merge_request, :simple, author: user, source_project: project1, target_project: project2, title: 'wip: thing') }
+
+    before do
+      project1.add_maintainer(user)
+      project2.add_developer(user)
+      project3.add_developer(user)
+      project2.add_developer(user2)
+      project4.add_developer(user)
+      project5.add_developer(user)
+      project6.add_developer(user)
     end
 
-    it 'filters by project' do
-      params = { project_id: project1.id, scope: 'authored', state: 'opened' }
-      merge_requests = described_class.new(user, params).execute
-      expect(merge_requests.size).to eq(2)
-    end
+    describe '#execute' do
+      it 'filters by scope' do
+        params = { scope: 'authored', state: 'opened' }
+        merge_requests = described_class.new(user, params).execute
+        expect(merge_requests.size).to eq(7)
+      end
 
-    it 'filters by commit sha' do
-      merge_requests = described_class.new(
-        user,
-        commit_sha: merge_request5.merge_request_diff.last_commit_sha
-      ).execute
+      it 'filters by project' do
+        params = { project_id: project1.id, scope: 'authored', state: 'opened' }
+        merge_requests = described_class.new(user, params).execute
+        expect(merge_requests.size).to eq(2)
+      end
 
-      expect(merge_requests).to contain_exactly(merge_request5)
-    end
+      it 'filters by commit sha' do
+        merge_requests = described_class.new(
+          user,
+          commit_sha: merge_request5.merge_request_diff.last_commit_sha
+        ).execute
+
+        expect(merge_requests).to contain_exactly(merge_request5)
+      end
+
+      context 'filtering by group' do
+        it 'includes all merge requests when user has access' do
+          params = { group_id: group.id }
+
+          merge_requests = described_class.new(user, params).execute
 
-    context 'filtering by group' do
-      it 'includes all merge requests when user has access' do
-        params = { group_id: group.id }
+          expect(merge_requests.size).to eq(3)
+        end
+
+        it 'excludes merge requests from projects the user does not have access to' do
+          private_project = create_project_without_n_plus_1(:private, group: group)
+          private_mr = create(:merge_request, :simple, author: user, source_project: private_project, target_project: private_project)
+          params = { group_id: group.id }
+
+          private_project.add_guest(user)
+          merge_requests = described_class.new(user, params).execute
+
+          expect(merge_requests.size).to eq(3)
+          expect(merge_requests).not_to include(private_mr)
+        end
+
+        it 'filters by group including subgroups', :nested_groups do
+          params = { group_id: group.id, include_subgroups: true }
+
+          merge_requests = described_class.new(user, params).execute
+
+          expect(merge_requests.size).to eq(6)
+        end
+      end
+
+      it 'filters by non_archived' do
+        params = { non_archived: true }
+        merge_requests = described_class.new(user, params).execute
+        expect(merge_requests.size).to eq(8)
+      end
+
+      it 'filters by iid' do
+        params = { project_id: project1.id, iids: merge_request1.iid }
 
         merge_requests = described_class.new(user, params).execute
 
-        expect(merge_requests.size).to eq(3)
+        expect(merge_requests).to contain_exactly(merge_request1)
       end
 
-      it 'excludes merge requests from projects the user does not have access to' do
-        private_project = create_project_without_n_plus_1(:private, group: group)
-        private_mr = create(:merge_request, :simple, author: user, source_project: private_project, target_project: private_project)
-        params = { group_id: group.id }
+      it 'filters by source branch' do
+        params = { source_branch: merge_request2.source_branch }
 
-        private_project.add_guest(user)
         merge_requests = described_class.new(user, params).execute
 
-        expect(merge_requests.size).to eq(3)
-        expect(merge_requests).not_to include(private_mr)
+        expect(merge_requests).to contain_exactly(merge_request2)
       end
 
-      it 'filters by group including subgroups', :nested_groups do
-        params = { group_id: group.id, include_subgroups: true }
+      it 'filters by target branch' do
+        params = { target_branch: merge_request2.target_branch }
 
         merge_requests = described_class.new(user, params).execute
 
-        expect(merge_requests.size).to eq(6)
+        expect(merge_requests).to contain_exactly(merge_request2)
       end
-    end
 
-    it 'filters by non_archived' do
-      params = { non_archived: true }
-      merge_requests = described_class.new(user, params).execute
-      expect(merge_requests.size).to eq(8)
-    end
+      it 'filters by state' do
+        params = { state: 'locked' }
 
-    it 'filters by iid' do
-      params = { project_id: project1.id, iids: merge_request1.iid }
+        merge_requests = described_class.new(user, params).execute
 
-      merge_requests = described_class.new(user, params).execute
+        expect(merge_requests).to contain_exactly(merge_request3)
+      end
 
-      expect(merge_requests).to contain_exactly(merge_request1)
-    end
+      it 'filters by wip' do
+        params = { wip: 'yes' }
 
-    it 'filters by source branch' do
-      params = { source_branch: merge_request2.source_branch }
+        merge_requests = described_class.new(user, params).execute
 
-      merge_requests = described_class.new(user, params).execute
+        expect(merge_requests).to contain_exactly(merge_request4, merge_request5, merge_request6, merge_request7, merge_request8, merge_request9)
+      end
 
-      expect(merge_requests).to contain_exactly(merge_request2)
-    end
+      it 'filters by not wip' do
+        params = { wip: 'no' }
 
-    it 'filters by target branch' do
-      params = { target_branch: merge_request2.target_branch }
+        merge_requests = described_class.new(user, params).execute
 
-      merge_requests = described_class.new(user, params).execute
+        expect(merge_requests).to contain_exactly(merge_request1, merge_request2, merge_request3)
+      end
 
-      expect(merge_requests).to contain_exactly(merge_request2)
-    end
+      it 'returns all items if no valid wip param exists' do
+        params = { wip: '' }
 
-    it 'filters by state' do
-      params = { state: 'locked' }
+        merge_requests = described_class.new(user, params).execute
 
-      merge_requests = described_class.new(user, params).execute
+        expect(merge_requests).to contain_exactly(merge_request1, merge_request2, merge_request3, merge_request4, merge_request5, merge_request6, merge_request7, merge_request8, merge_request9)
+      end
 
-      expect(merge_requests).to contain_exactly(merge_request3)
-    end
+      it 'adds wip to scalar params' do
+        scalar_params = described_class.scalar_params
 
-    it 'filters by wip' do
-      params = { wip: 'yes' }
+        expect(scalar_params).to include(:wip, :assignee_id)
+      end
 
-      merge_requests = described_class.new(user, params).execute
+      context 'filtering by group milestone' do
+        let!(:group) { create(:group, :public) }
+        let(:group_milestone) { create(:milestone, group: group) }
+        let!(:group_member) { create(:group_member, group: group, user: user) }
+        let(:params) { { milestone_title: group_milestone.title } }
 
-      expect(merge_requests).to contain_exactly(merge_request4, merge_request5, merge_request6, merge_request7, merge_request8, merge_request9)
-    end
+        before do
+          project2.update(namespace: group)
+          merge_request2.update(milestone: group_milestone)
+          merge_request3.update(milestone: group_milestone)
+        end
 
-    it 'filters by not wip' do
-      params = { wip: 'no' }
+        it 'returns issues assigned to that group milestone' do
+          merge_requests = described_class.new(user, params).execute
 
-      merge_requests = described_class.new(user, params).execute
+          expect(merge_requests).to contain_exactly(merge_request2, merge_request3)
+        end
+      end
 
-      expect(merge_requests).to contain_exactly(merge_request1, merge_request2, merge_request3)
-    end
+      context 'filtering by created_at/updated_at' do
+        let(:new_project) { create(:project, forked_from_project: project1) }
 
-    it 'returns all items if no valid wip param exists' do
-      params = { wip: '' }
+        let!(:new_merge_request) do
+          create(:merge_request,
+                :simple,
+                author: user,
+                created_at: 1.week.from_now,
+                updated_at: 1.week.from_now,
+                source_project: new_project,
+                target_project: new_project)
+        end
 
-      merge_requests = described_class.new(user, params).execute
+        let!(:old_merge_request) do
+          create(:merge_request,
+                :simple,
+                author: user,
+                source_branch: 'feature_1',
+                created_at: 1.week.ago,
+                updated_at: 1.week.ago,
+                source_project: new_project,
+                target_project: new_project)
+        end
 
-      expect(merge_requests).to contain_exactly(merge_request1, merge_request2, merge_request3, merge_request4, merge_request5, merge_request6, merge_request7, merge_request8, merge_request9)
-    end
+        before do
+          new_project.add_maintainer(user)
+        end
 
-    it 'adds wip to scalar params' do
-      scalar_params = described_class.scalar_params
+        it 'filters by created_after' do
+          params = { project_id: new_project.id, created_after: new_merge_request.created_at }
 
-      expect(scalar_params).to include(:wip, :assignee_id)
-    end
+          merge_requests = described_class.new(user, params).execute
 
-    context 'filtering by group milestone' do
-      let!(:group) { create(:group, :public) }
-      let(:group_milestone) { create(:milestone, group: group) }
-      let!(:group_member) { create(:group_member, group: group, user: user) }
-      let(:params) { { milestone_title: group_milestone.title } }
+          expect(merge_requests).to contain_exactly(new_merge_request)
+        end
 
-      before do
-        project2.update(namespace: group)
-        merge_request2.update(milestone: group_milestone)
-        merge_request3.update(milestone: group_milestone)
-      end
+        it 'filters by created_before' do
+          params = { project_id: new_project.id, created_before: old_merge_request.created_at }
 
-      it 'returns issues assigned to that group milestone' do
-        merge_requests = described_class.new(user, params).execute
+          merge_requests = described_class.new(user, params).execute
 
-        expect(merge_requests).to contain_exactly(merge_request2, merge_request3)
-      end
-    end
+          expect(merge_requests).to contain_exactly(old_merge_request)
+        end
 
-    context 'filtering by created_at/updated_at' do
-      let(:new_project) { create(:project, forked_from_project: project1) }
-
-      let!(:new_merge_request) do
-        create(:merge_request,
-               :simple,
-               author: user,
-               created_at: 1.week.from_now,
-               updated_at: 1.week.from_now,
-               source_project: new_project,
-               target_project: new_project)
-      end
+        it 'filters by created_after and created_before' do
+          params = {
+            project_id: new_project.id,
+            created_after: old_merge_request.created_at,
+            created_before: new_merge_request.created_at
+          }
 
-      let!(:old_merge_request) do
-        create(:merge_request,
-               :simple,
-               author: user,
-               source_branch: 'feature_1',
-               created_at: 1.week.ago,
-               updated_at: 1.week.ago,
-               source_project: new_project,
-               target_project: new_project)
-      end
+          merge_requests = described_class.new(user, params).execute
 
-      before do
-        new_project.add_maintainer(user)
-      end
+          expect(merge_requests).to contain_exactly(old_merge_request, new_merge_request)
+        end
 
-      it 'filters by created_after' do
-        params = { project_id: new_project.id, created_after: new_merge_request.created_at }
+        it 'filters by updated_after' do
+          params = { project_id: new_project.id, updated_after: new_merge_request.updated_at }
 
-        merge_requests = described_class.new(user, params).execute
+          merge_requests = described_class.new(user, params).execute
 
-        expect(merge_requests).to contain_exactly(new_merge_request)
-      end
+          expect(merge_requests).to contain_exactly(new_merge_request)
+        end
 
-      it 'filters by created_before' do
-        params = { project_id: new_project.id, created_before: old_merge_request.created_at }
+        it 'filters by updated_before' do
+          params = { project_id: new_project.id, updated_before: old_merge_request.updated_at }
 
-        merge_requests = described_class.new(user, params).execute
+          merge_requests = described_class.new(user, params).execute
 
-        expect(merge_requests).to contain_exactly(old_merge_request)
-      end
+          expect(merge_requests).to contain_exactly(old_merge_request)
+        end
 
-      it 'filters by created_after and created_before' do
-        params = {
-          project_id: new_project.id,
-          created_after: old_merge_request.created_at,
-          created_before: new_merge_request.created_at
-        }
+        it 'filters by updated_after and updated_before' do
+          params = {
+            project_id: new_project.id,
+            updated_after: old_merge_request.updated_at,
+            updated_before: new_merge_request.updated_at
+          }
 
-        merge_requests = described_class.new(user, params).execute
+          merge_requests = described_class.new(user, params).execute
 
-        expect(merge_requests).to contain_exactly(old_merge_request, new_merge_request)
+          expect(merge_requests).to contain_exactly(old_merge_request, new_merge_request)
+        end
       end
+    end
 
-      it 'filters by updated_after' do
-        params = { project_id: new_project.id, updated_after: new_merge_request.updated_at }
+    describe '#row_count', :request_store do
+      it 'returns the number of rows for the default state' do
+        finder = described_class.new(user)
 
-        merge_requests = described_class.new(user, params).execute
+        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(merge_requests).to contain_exactly(new_merge_request)
+        expect(finder.row_count).to eq(1)
       end
+    end
+  end
 
-      it 'filters by updated_before' do
-        params = { project_id: new_project.id, updated_before: old_merge_request.updated_at }
+  context 'when projects require different access levels for merge requests' do
+    let(:user) { create(:user) }
 
-        merge_requests = described_class.new(user, params).execute
+    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) }
 
-        expect(merge_requests).to contain_exactly(old_merge_request)
-      end
+    let(:merge_requests) { described_class.new(user, {}).execute }
 
-      it 'filters by updated_after and updated_before' do
-        params = {
-          project_id: new_project.id,
-          updated_after: old_merge_request.updated_at,
-          updated_before: new_merge_request.updated_at
-        }
+    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) }
 
-        merge_requests = described_class.new(user, params).execute
+    context 'with admin user' do
+      let(:user) { create(:user, :admin) }
 
-        expect(merge_requests).to contain_exactly(old_merge_request, new_merge_request)
+      it 'returns all merge requests' do
+        expect(merge_requests).to eq(
+          [mr_internal_private_repo_acce