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