Merge branch 'security-6881-project-group-approvers-leaks-private-group-info' into 'master'

[master] Project group approvers leaks private group info

See merge request gitlab/gitlab-ee!657

app/controllers/projects/merge_requests/creations_controller.rb

@@ -108,6 +108,10 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
     @commits = set_commits_for_rendering(@merge_request.commits)
     @commit = @merge_request.diff_head_commit
 
+    # FIXME: We have to assign a presenter to another instance variable
+    # due to class_name checks being made with issuable classes
+    @mr_presenter = @merge_request.present(current_user: current_user)
+
     @labels = LabelsFinder.new(current_user, project_id: @project.id).execute
 
     set_pipeline_variables

app/controllers/projects/merge_requests_controller.rb

@@ -343,6 +343,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     @target_project = @merge_request.target_project
     @target_branches = @merge_request.target_project.repository.branch_names
     @noteable = @merge_request
+
+    # FIXME: We have to assign a presenter to another instance variable
+    # due to class_name checks being made with issuable classes
+    @mr_presenter = @merge_request.present(current_user: current_user)
   end
 
   def finder_type

app/controllers/projects_controller.rb

@@ -18,6 +18,7 @@ class ProjectsController < Projects::ApplicationController
   before_action :tree, only: [:show], if: [:repo_exists?, :project_view_files?]
   before_action :lfs_blob_ids, only: [:show], if: [:repo_exists?, :project_view_files?]
   before_action :project_export_enabled, only: [:export, :download_export, :remove_export, :generate_new_export]
+  before_action :present_project, only: [:edit]
 
   # Authorize
   before_action :authorize_admin_project!, only: [:edit, :update, :housekeeping, :download_export, :export, :remove_export, :generate_new_export]
@@ -442,4 +443,8 @@ class ProjectsController < Projects::ApplicationController
   def whitelist_query_limiting
     Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42440')
   end
+
+  def present_project
+    @project = @project.present(current_user: current_user)
+  end
 end

app/finders/joined_groups_finder.rb

 # frozen_string_literal: true
 
-class JoinedGroupsFinder < UnionFinder
+class JoinedGroupsFinder
   def initialize(user)
     @user = user
   end
@@ -8,19 +8,8 @@ class JoinedGroupsFinder < UnionFinder
   # Finds the groups of the source user, optionally limited to those visible to
   # the current user.
   def execute(current_user = nil)
-    segments = all_groups(current_user)
-
-    find_union(segments, Group).order_id_desc
-  end
-
-  private
-
-  def all_groups(current_user)
-    groups = []
-
-    groups << @user.authorized_groups.visible_to_user(current_user) if current_user
-    groups << @user.authorized_groups.public_to_user(current_user)
-
-    groups
+    @user.authorized_groups
+      .public_or_visible_to_user(current_user)
+      .order_id_desc
   end
 end

app/mailers/emails/merge_requests.rb

@@ -3,13 +3,14 @@
 module Emails
   module MergeRequests
     def new_merge_request_email(recipient_id, merge_request_id, reason = nil)
-      setup_merge_request_mail(merge_request_id, recipient_id)
+      setup_merge_request_mail(merge_request_id, recipient_id, present: true)
 
       mail_new_thread(@merge_request, merge_request_thread_options(@merge_request.author_id, recipient_id, reason))
     end
 
     def new_mention_in_merge_request_email(recipient_id, merge_request_id, updated_by_user_id, reason = nil)
-      setup_merge_request_mail(merge_request_id, recipient_id)
+      setup_merge_request_mail(merge_request_id, recipient_id, present: true)
+
       mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason))
     end
 
@@ -67,7 +68,7 @@ module Emails
     end
 
     def add_merge_request_approver_email(recipient_id, merge_request_id, updated_by_user_id, reason = nil)
-      setup_merge_request_mail(merge_request_id, recipient_id)
+      setup_merge_request_mail(merge_request_id, recipient_id, present: true)
 
       @updated_by = User.find(updated_by_user_id)
       mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason))
@@ -96,11 +97,16 @@ module Emails
 
     private
 
-    def setup_merge_request_mail(merge_request_id, recipient_id)
+    def setup_merge_request_mail(merge_request_id, recipient_id, present: false)
       @merge_request = MergeRequest.find(merge_request_id)
       @project = @merge_request.project
       @target_url = project_merge_request_url(@project, @merge_request)
 
+      if present
+        recipient = User.find(recipient_id)
+        @mr_presenter = @merge_request.present(current_user: recipient)
+      end
+
       @sent_notification = SentNotification.record(@merge_request, recipient_id, reply_key)
     end
 

app/models/group.rb

@@ -84,8 +84,17 @@ class Group < Namespace
       User.reference_pattern
     end
 
-    def visible_to_user(user)
-      where(id: user.authorized_groups.select(:id).reorder(nil))
+    # WARNING: This method should never be used on its own
+    # please do make sure the number of rows you are filtering is small
+    # enough for this query
+    def public_or_visible_to_user(user)
+      return public_to_user unless user
+
+      public_for_user = public_to_user_arel(user)
+      visible_for_user = visible_to_user_arel(user)
+      public_or_visible = public_for_user.or(visible_for_user)
+
+      where(public_or_visible)
     end
 
     def select_for_project_authorization
@@ -97,6 +106,23 @@ class Group < Namespace
         super
       end
     end
+
+    private
+
+    def public_to_user_arel(user)
+      self.arel_table[:visibility_level]
+        .in(Gitlab::VisibilityLevel.levels_for_user(user))
+    end
+
+    def visible_to_user_arel(user)
+      groups_table = self.arel_table
+      authorized_groups = user.authorized_groups.as('authorized')
+
+      groups_table.project(1)
+        .from(authorized_groups)
+        .where(authorized_groups[:id].eq(groups_table[:id]))
+        .exists
+    end
   end
 
   # Overrides notification_settings has_many association

app/models/merge_request.rb

@@ -6,6 +6,7 @@ class MergeRequest < ActiveRecord::Base
   include Issuable
   include Noteable
   include Referable
+  include Presentable
   include Elastic::MergeRequestsSearch
   include IgnorableColumn
   include TimeTrackable

app/models/user.rb

@@ -692,10 +692,12 @@ class User < ActiveRecord::Base
 
   # Returns the groups a user has access to, either through a membership or a project authorization
   def authorized_groups
-    Group.from_union([
-      groups,
-      authorized_projects.joins(:namespace).select('namespaces.*')
-    ])
+    Group.unscoped do
+      Group.from_union([
+        groups,
+        authorized_projects.joins(:namespace).select('namespaces.*')
+      ])
+    end
   end
 
   # Returns the groups a user is a member of, either directly or through a parent group

app/views/notify/new_merge_request_email.html.haml

@@ -9,7 +9,7 @@
   %p
     Assignee: #{@merge_request.assignee_name}
 
-= render_if_exists 'notify/merge_request_approvers', merge_request: @merge_request
+= render_if_exists 'notify/merge_request_approvers', presenter: @mr_presenter
 
 - if @merge_request.description
   %div

app/views/notify/new_merge_request_email.text.erb

@@ -5,6 +5,6 @@ New Merge Request <%= @merge_request.to_reference %>
 <%= merge_path_description(@merge_request, 'to') %>
 Author:    <%= @merge_request.author_name %>
 Assignee:  <%= @merge_request.assignee_name %>
-<%= render_if_exists 'notify/merge_request_approvers', merge_request: @merge_request %>
+<%= render_if_exists 'notify/merge_request_approvers', presenter: @mr_presenter %>
 
 <%= @merge_request.description %>

app/views/projects/merge_requests/_form.html.haml

 = form_for [@project.namespace.becomes(Namespace), @project, @merge_request],
            html: { class: 'merge-request-form common-note-form js-requires-input js-quick-submit' },
            data: { markdown_version: @merge_request.cached_markdown_version } do |f|
-  = render 'shared/issuable/form', f: f, issuable: @merge_request
+  = render 'shared/issuable/form', f: f, issuable: @merge_request, presenter: @mr_presenter

app/views/projects/merge_requests/creations/_new_submit.html.haml

@@ -11,7 +11,7 @@
     = link_to 'Change branches', mr_change_branches_path(@merge_request)
 %hr
 = form_for [@project.namespace.becomes(Namespace), @project, @merge_request], html: { class: 'merge-request-form common-note-form js-requires-input js-quick-submit' } do |f|
-  = render 'shared/issuable/form', f: f, issuable: @merge_request, commits: @commits
+  = render 'shared/issuable/form', f: f, issuable: @merge_request, commits: @commits, presenter: @mr_presenter
   = f.hidden_field :source_project_id
   = f.hidden_field :source_branch
   = f.hidden_field :target_project_id

app/views/shared/issuable/_form.html.haml

 - form = local_assigns.fetch(:f)
 - commits = local_assigns[:commits]
 - project = @target_project || @project
+- presenter = local_assigns.fetch(:presenter, nil)
 
 = form_errors(issuable)
 
@@ -29,7 +30,7 @@
 
 = render 'shared/issuable/form/metadata', issuable: issuable, form: form
 
-= render_if_exists 'shared/issuable/approvals', issuable: issuable, form: form
+= render_if_exists 'shared/issuable/approvals', issuable: issuable, presenter: presenter, form: form
 
 = render 'shared/issuable/form/branch_chooser', issuable: issuable, form: form
 

ee/app/models/approver_group.rb

@@ -5,4 +5,10 @@ class ApproverGroup < ActiveRecord::Base
   validates :group, presence: true
 
   delegate :users, to: :group
+
+  def self.filtered_approver_groups(approver_groups, user)
+    public_or_visible_groups = Group.public_or_visible_to_user(user) # rubocop:disable Cop/GroupPublicOrVisibleToUser
+
+    approver_groups.joins(:group).merge(public_or_visible_groups)
+  end
 end

ee/app/models/concerns/approvable.rb

 module Approvable
-  include Gitlab::Utils::StrongMemoize
-
-  def requires_approve?
-    # keep until UI changes implemented
-    true
-  end
+  # A method related to approvers that is user facing
+  # should be moved to VisibleApprovable because
+  # of the fact that we use filtered versions of certain methods
+  # such as approver_groups and target_project in presenters
+  include ::VisibleApprovable
 
   def approval_needed?
     approvals_required&.nonzero?
@@ -36,66 +35,17 @@ module Approvable
     super
   end
 
-  # Users in the list of approvers who have not already approved this MR.
-  #
-  def approvers_left
-    strong_memoize(:approvers_left) do
-      User.where(id: all_approvers_including_groups.map(&:id)).where.not(id: approved_by_users.select(:id))
-    end
-  end
-
-  # The list of approvers from either this MR (if they've been set on the MR) or the
-  # target project. Excludes the author if 'self-approval' isn't explicitly
-  # enabled on project settings.
+  # Even though this method is used in VisibleApprovable
+  # we do not want to encounter a scenario where there are
+  # no user approvers but there is one merge request group
+  # approver that is not visible to the current_user,
+  # which would make this method return false
+  # when it should still report as overwritten.
+  # To prevent this from happening, approvers_overwritten?
+  # makes use of the unfiltered version of approver_groups so that
+  # it always takes into account every approver
+  # available
   #
-  # Before a merge request has been created, author will be nil, so pass the current user
-  # on the MR create page.
-  #
-  def overall_approvers
-    approvers_relation = approvers_overwritten? ? approvers : target_project.approvers
-
-    if author && !authors_can_approve?
-      approvers_relation = approvers_relation.where.not(user_id: author.id)
-    end
-
-    approvers_relation.includes(:user)
-  end
-
-  def overall_approver_groups
-    approvers_overwritten? ? approver_groups : target_project.approver_groups
-  end
-
-  def all_approvers_including_groups
-    strong_memoize(:all_approvers_including_groups) do
-      approvers = []
-
-      # Approvers from direct assignment
-      approvers << approvers_from_users
-
-      approvers << approvers_from_groups
-
-      approvers.flatten
-    end
-  end
-
-  def approvers_from_users
-    overall_approvers.map(&:user)
-  end
-
-  def approvers_from_groups
-    group_approvers = []
-
-    overall_approver_groups.each do |approver_group|
-      group_approvers << approver_group.users
-    end
-
-    group_approvers.flatten!
-
-    group_approvers.delete(author) unless authors_can_approve?
-
-    group_approvers
-  end
-
   def approvers_overwritten?
     approvers.to_a.any? || approver_groups.to_a.any?
   end
@@ -145,12 +95,4 @@ module Approvable
       approver_groups.find_or_initialize_by(group_id: group_id, target_id: id)
     end
   end
-
-  def reset_approval_cache!
-    approvals(true)
-    approved_by_users(true)
-
-    clear_memoization(:approvers_left)
-    clear_memoization(:all_approvers_including_groups)
-  end
 end

ee/app/models/concerns/visible_approvable.rb

+# This module may only be used by presenters or modules
+# that include the Approvable concern
+module VisibleApprovable
+  include ::Gitlab::Utils::StrongMemoize
+
+  def requires_approve?
+    # keep until UI changes implemented
+    true
+  end
+
+  # Users in the list of approvers who have not already approved this MR.
+  #
+  def approvers_left
+    strong_memoize(:approvers_left) do
+      User.where(id: all_approvers_including_groups.map(&:id)).where.not(id: approved_by_users.select(:id))
+    end
+  end
+
+  # The list of approvers from either this MR (if they've been set on the MR) or the
+  # target project. Excludes the author if 'self-approval' isn't explicitly
+  # enabled on project settings.
+  #
+  # Before a merge request has been created, author will be nil, so pass the current user
+  # on the MR create page.
+  #
+  def overall_approvers
+    approvers_relation = approvers_overwritten? ? approvers : target_project.approvers
+
+    if author && !authors_can_approve?
+      approvers_relation = approvers_relation.where.not(user_id: author.id)
+    end
+
+    approvers_relation.includes(:user)
+  end
+
+  def overall_approver_groups
+    approvers_overwritten? ? approver_groups : target_project.approver_groups
+  end
+
+  def all_approvers_including_groups
+    strong_memoize(:all_approvers_including_groups) do
+      approvers = []
+
+      # Approvers from direct assignment
+      approvers << approvers_from_users
+
+      approvers << approvers_from_groups
+
+      approvers.flatten
+    end
+  end
+
+  def approvers_from_users
+    overall_approvers.map(&:user)
+  end
+
+  def approvers_from_groups
+    group_approvers = overall_approver_groups.flat_map(&:users)
+    group_approvers.delete(author) unless authors_can_approve?
+    group_approvers
+  end
+
+  def reset_approval_cache!
+    approvals(true)
+    approved_by_users(true)
+
+    clear_memoization(:approvers_left)
+    clear_memoization(:all_approvers_including_groups)
+  end
+end

ee/app/presenters/ee/merge_request_presenter.rb

 module EE
   module MergeRequestPresenter
+    include ::VisibleApprovable
+
     def approvals_path
       if requires_approve?
         approvals_project_merge_request_path(project, merge_request)
       end
     end
+
+    def target_project
+      merge_request.target_project.present(current_user: current_user)
+    end
+
+    def approver_groups
+      ::ApproverGroup.filtered_approver_groups(merge_request.approver_groups, current_user)
+    end
   end
 end

ee/app/presenters/ee/project_presenter.rb

@@ -20,6 +20,10 @@ module EE
       anchors
     end
 
+    def approver_groups
+      ::ApproverGroup.filtered_approver_groups(project.approver_groups, current_user)
+    end
+
     private
 
     def security_dashboard_data

ee/app/views/notify/_merge_request_approvers.html.haml

-- merge_request = local_assigns.fetch(:merge_request)
-- return unless merge_request.approvers.any?
+- presenter = local_assigns.fetch(:presenter)
+- return unless presenter.approvers.any?
 
 %p
-  Approvers: #{render_items_list(merge_request.approvers_left.map(&:name))}
+  Approvers: #{render_items_list(presenter.approvers_left.map(&:name))}

ee/app/views/notify/_merge_request_approvers.text.erb

-<%- merge_request = local_assigns.fetch(:merge_request) -%>
-<%- return unless merge_request.approvers.any? -%>
-Approvers: <%= render_items_list(merge_request.approvers_left.map(&:name)) %>
+<%- presenter = local_assigns.fetch(:presenter) -%>
+<%- return unless presenter.approvers.any? -%>
+Approvers: <%= render_items_list(presenter.approvers_left.map(&:name)) %>

ee/app/views/notify/add_merge_request_approver_email.html.haml

@@ -8,9 +8,7 @@
   %p
     Assignee: #{@merge_request.author_name} → #{@merge_request.assignee_name}
 
-- if @merge_request.approvers.any?
-  %p
-    Approvers: #{render_items_list(@merge_request.approvers_left.map(&:name))}
+= render_if_exists 'notify/merge_request_approvers', presenter: @mr_presenter
 
 - if @merge_request.description
   = markdown(@merge_request.description, pipeline: :email, author: @merge_request.author)

ee/app/views/notify/add_merge_request_approver_email.text.erb

@@ -5,6 +5,4 @@
 <%= merge_path_description(@merge_request, 'to') %>
 Author:    <%= @merge_request.author_name %>
 Assignee:  <%= @merge_request.assignee_name %>
-<% if @merge_request.approvers.any? %>
-Approvers: <%= render_items_list(@merge_request.approvers_left.map(&:name)) %>
-<% end %>
+<%= render_if_exists 'notify/merge_request_approvers', presenter: @mr_presenter %>

ee/app/views/shared/issuable/_approvals.html.haml

 - issuable = local_assigns.fetch(:issuable)
+- presenter = local_assigns.fetch(:presenter)
 - form = local_assigns.fetch(:form)
 
 - return unless issuable.is_a?(MergeRequest)
-- return unless issuable.requires_approve?
+- return unless presenter.requires_approve?
 
 - author = issuable.author || current_user
-- authors_can_approve = issuable.merge_requests_author_approval?
 - can_update_approvers = can?(current_user, :update_approvers, issuable)
 
 .form-group.row
@@ -13,7 +13,7 @@
     Approvers
   .col-sm-10
     - if can_update_approvers
-      - skip_users = [*issuable.all_approvers_including_groups, (author unless authors_can_approve)].compact
+      - skip_users = [*presenter.all_approvers_including_groups, (author unless presenter.authors_can_approve?)].compact
 
       = users_select_tag("merge_request[approver_ids]",
         multiple: true,
@@ -26,7 +26,7 @@
         This merge request must be approved by these users.
         You can override the project settings by setting your own list of approvers.
 
-      - skip_groups = issuable.overall_approver_groups.pluck(:group_id) # rubocop: disable CodeReuse/ActiveRecord
+      - skip_groups = presenter.overall_approver_groups.pluck(:group_id) # rubocop: disable CodeReuse/ActiveRecord
       = groups_select_tag('merge_request[approver_group_ids]', multiple: true, data: { skip_groups: skip_groups, all_available: true, project: issuable.target_project }, class: 'input-large')
       .form-text.text-muted
         This merge request must be approved by members of these groups.
@@ -36,12 +36,12 @@
       .card-header
         Approvers
       %ul.content-list.approver-list
-        - if issuable.all_approvers_including_groups.empty?
+        - if presenter.all_approvers_including_groups.empty?
           %li.no-approvers There are no approvers
         - else
-          - unsaved_approvers = !issuable.approvers_overwritten?
+          - unsaved_approvers = !presenter.approvers_overwritten?
           - item_classes = unsaved_approvers ? ['unsaved-approvers'] : []
-          - issuable.overall_approvers.each do |approver|
+          - presenter.overall_approvers.each do |approver|
             %li{ id: dom_id(approver.user), class: item_classes + ['approver'] }
               = link_to approver.user.name, approver.user
               - if can_update_approvers
@@ -54,7 +54,7 @@
                     = link_to project_merge_request_approver_path(@project, issuable, approver), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, method: :delete, class: "btn-sm btn btn-remove", title: 'Remove approver' do
                       = icon("sign-out")
                       Remove
-          - issuable.overall_approver_groups.each do |approver_group|
+          - presenter.overall_approver_groups.each do |approver_group|
             %li{ id: dom_id(approver_group.group), class: item_classes + ['approver-group'] }
               Group:
               = link_to approver_group.group.name, approver_group.group

ee/changelogs/unreleased/security-6881-project-group-approvers-leaks-private-group-info.yml

+---
+title: Project groups approvers no longer leak private groups info
+merge_request:
+author:
+type: security

ee/lib/api/merge_request_approvals.rb

@@ -42,7 +42,7 @@ module API
         get 'approvals' do
           merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
-          present merge_request, with: Entities::MergeRequestApprovals, current_user: current_user
+          present merge_request.present(current_user: current_user), with: Entities::MergeRequestApprovals, current_user: current_user
         end
 
         desc 'Change approval-related configuration' do
@@ -60,7 +60,7 @@ module API
           merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, approvals_before_merge: params[:approvals_required]).execute(merge_request)
 
           if merge_request.valid?
-            present merge_request, with: Entities::MergeRequestApprovals, current_user: current_user
+            present merge_request.present(current_user: current_user), with: Entities::MergeRequestApprovals, current_user: current_user
           else
             handle_merge_request_errors! merge_request.errors
           end
@@ -80,7 +80,7 @@ module API
           merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, declared(params, include_parent_namespaces: false).merge(remove_old_approvers: true)).execute(merge_request)
 
           if merge_request.valid?
-            present merge_request, with: Entities::MergeRequestApprovals, current_user: current_user
+            present merge_request.present(current_user: current_user), with: Entities::MergeRequestApprovals, current_user: current_user
           else
             handle_merge_request_errors! merge_request.errors
           end
@@ -111,7 +111,7 @@ module API
             .new(user_project, current_user)
             .execute(merge_request)
 
-          present merge_request, with: Entities::MergeRequestApprovals, current_user: current_user
+          present merge_request.present(current_user: current_user), with: Entities::MergeRequestApprovals, current_user: current_user
         end
 
         desc 'Remove an approval from a merge request' do
@@ -126,7 +126,7 @@ module API
             .new(user_project, current_user)
             .execute(merge_request)
 
-          present merge_request, with: Entities::MergeRequestApprovals, current_user: current_user
+          present merge_request.present(current_user: current_user), with: Entities::MergeRequestApprovals, current_user: current_user
         end
       end
     end

ee/lib/api/project_approvals.rb

@@ -15,7 +15,7 @@ module API
           success ::API::Entities::ApprovalSettings
         end
         get '/' do
-          present user_project, with: ::API::Entities::ApprovalSettings
+          present user_project.present(current_user: current_user), with: ::API::Entities::ApprovalSettings
         end
 
         desc 'Change approval-related configuration' do
@@ -34,7 +34,7 @@ module API
           result = ::Projects::UpdateService.new(user_project, current_user, project_params).execute
 
           if result[:status] == :success
-            present user_project, with: ::API::Entities::ApprovalSettings
+            present user_project.present(current_user: current_user), with: ::API::Entities::ApprovalSettings
           else
             render_validation_error!(user_project)
           end
@@ -53,7 +53,7 @@ module API
         result = ::Projects::UpdateService.new(user_project, current_user, declared(params, include_parent_namespaces: false).merge(remove_old_approvers: true)).execute
 
         if result[:status] == :success
-          present user_project, with: ::API::Entities::ApprovalSettings
+          present user_project.present(current_user: current_user), with: ::API::Entities::ApprovalSettings
         else
           render_validation_error!(user_project)
         end

ee/spec/features/projects/settings/service_desk_setting_spec.rb

@@ -2,13 +2,15 @@ require 'spec_helper'
 
 describe 'Service Desk Setting', :js do
   let(:project) { create(:project_empty_repo, :private, service_desk_enabled: false) }
+  let(:presenter) { project.present(current_user: user) }
   let(:user) { create(:user) }
 
   before do
     project.add_maintainer(user)
     sign_in(user)
 
-    allow(::EE::Gitlab::ServiceDesk).to receive(:enabled?).with(project: project).and_return(true)
+    allow_any_instance_of(Project).to receive(:present).with(current_user: user).and_return(presenter)
+    allow(::EE::Gitlab::ServiceDesk).to receive(:enabled?).with(project: presenter).and_return(true)
     allow(::Gitlab::IncomingEmail).to receive(:enabled?) { true }
     allow(::Gitlab::IncomingEmail).to receive(:supports_wildcard?) { true }
 

ee/spec/models/approvable_spec.rb

 require 'spec_helper'
 
 describe Approvable do
-  let(:merge_request) { create(:merge_request, :with_approver) }
+  let(:merge_request) { create(:merge_request) }
 
-  describe '#approvers_left' do
-    it 'only queries once' do
-      merge_request
+  describe '#approvers_overwritten?' do
+    subject { merge_request.approvers_overwritten? }
 
-      expect(User).to receive(:where).and_call_original.once
-
-      3.times { merge_request.approvers_left }
+    it 'returns false when merge request has no approvers' do
+      is_expected.to be false
     end
-  end
 
-  describe '#reset_approval_cache!' do
-    it 'clears the cache of approvers left' do
-      user_can_approve = merge_request.approvers_left.first
-      merge_request.approvals.create!(user: user_can_approve)
+    it 'returns true when merge request has user approver' do
+      create(:approver, target: merge_request)
+
+      is_expected.to be true
+    end
 
-      merge_request.reset_approval_cache!
+    it 'returns true when merge request has group approver' do
+      group = create(:group_with_members)
+      create(:approver_group, target: merge_request, group: group)
 
-      expect(merge_request.approvers_left).to be_empty
+      is_expected.to be true
     end
   end
 end

ee/spec/models/approver_group_spec.rb

@@ -4,4 +4,24 @@ describe ApproverGroup do
   subject { create(:approver_group) }
 
   it { is_expected.to be_valid }
+
+  describe '.filtered_approver_groups' do
+    let!(:project) { create(:project) }
+    let!(:user) { project.creator }
+    let!(:private_group) { create(:group, :private) }
+    let!(:public_approver_group) { create(:approver_group, target: project) }
+    let!(:private_approver_group) { create(:approver_group, target: project, group: private_group) }
+
+    subject { described_class.filtered_approver_groups(project.approver_groups, user) }
+
+    it { is_expected.to match_array([public_approver_group]) }
+
+    context 'when user has access to private group' do
+      before do
+        private_group.add_user(user, Gitlab::Access::DEVELOPER)
+      end
+
+      it { is_expected.to match_array([public_approver_group, private_approver_group]) }
+    end
+  end
 end

ee/spec/models/visible_approvable_spec.rb

+require 'spec_helper'
+
+describe VisibleApprovable do
+  let(:resource) { create(:merge_request, source_project: project) }
+  let!(:project) { create(:project, :repository) }
+  let!(:user) { project.creator }
+
+  describe '#requires_approve' do
+    subject { resource.requires_approve? }
+
+    it { is_expected.to be true }
+  end
+
+  describe '#approvers_left' do
+    let!(:private_group) { create(:group_with_members, :private) }
+    let!(:public_group) { create(:group_with_members) }
+    let!(:public_approver_group) { create(:approver_group, target: resource, group: public_group) }
+    let!(:private_approver_group) { create(:approver_group, target: resource, group: private_group) }
+    let!(:approver) { create(:approver, target: resource) }
+
+    subject { resource.approvers_left }
+
+    it 'only queries once' do
+      expect(User).to receive(:where).and_call_original.once
+
+      3.times { subject }
+    end
+
+    it 'returns all approvers left' do
+      resource.approvals.create!(user: approver.user)
+
+      is_expected.to match_array(public_approver_group.users + private_approver_group.users)
+    end
+  end
+
+  describe '#overall_approvers' do
+    let!(:project_approver) { create(:approver, target: project) }
+
+    subject { resource.overall_approvers }
+
+    it 'returns a list of all the project approvers' do
+      is_expected.to match_array(project_approver)
+    end
+
+    context 'when author is approver' do
+      let!(:author_approver) { create(:approver, target: project, user: resource.author) }
+
+      it 'excludes author if authors cannot approve' do
+        is_expected.not_to include(author_approver)
+      end
+
+      it 'includes author if authors are able to approve' do
+        project.update(merge_requests_author_approval: true)
+
+        is_expected.to include(author_approver)
+      end
+    end
+
+    context 'when approvers are overwritten' do
+      let!(:approver) { create(:approver, target: resource) }
+
+      it 'returns the list of all the merge request user approvers' do
+        is_expected.to match_array(approver)
+      end
+    end
+  end
+
+  describe '#overall_approver_groups' do
+    before do
+      group = create(:group_with_members)
+      create(:approver_group, target: project, group: group)
+    end
+
+    subject { resource.overall_approver_groups }
+
+    it 'returns all the project approver groups' do
+      is_expected.to match_array(project.approver_groups)
+    end
+
+    context 'when group approvers are overwritten' do
+      it 'returns all the merge request approver groups' do
+        group = create(:group_with_members)
+        create(:approver_group, target: resource, group: group)
+
+        is_expected.to match_array(resource.approver_groups)
+      end
+    end
+  end
+
+  describe '#all_approvers_including_groups' do
+    let!(:group) { create(:group_with_members) }
+    let!(:approver_group) { create(:approver_group, target: resource, group: group) }
+    let!(:approver) { create(:approver, target: resource) }
+
+    subject { resource.all_approvers_including_groups }
+
+    it 'only queries once' do
+      expect(resource).to receive(:approvers_from_users).and_call_original.once
+
+      3.times { subject }
+    end
+
+    it 'returns all approvers (groups and users)' do
+      is_expected.to match_array(approver_group.users + [approver.user])
+    end
+  end
+
+  describe '#authors_can_approve?' do
+    subject { resource.authors_can_approve? }
+
+    it 'returns false when merge_requests_author_approval flag is off' do
+      is_expected.to be false
+    end
+
+    it 'returns true when merge_requests_author_approval flag is turned on' do
+      project.update(merge_requests_author_approval: true)
+
+      is_expected.to be true
+    end
+  end
+
+  describe '#reset_approval_cache!' do
+    before do
+      create(:approver, target: resource)
+    end
+
+    subject { resource.reset_approval_cache! }
+
+    it 'clears the cache of approvers left' do
+      user_can_approve = resource.approvers_left.first
+      resource.approvals.create!(user: user_can_approve)
+
+      subject
+
+      expect(resource.approvers_left).to be_empty
+    end
+
+    it 'clears the all_approvers_including_groups cache' do
+      resource.all_approvers_including_groups.first.destroy!
+
+      subject
+
+      expect(resource.all_approvers_including_groups).to be_empty
+    end
+  end
+end

ee/spec/presenters/merge_request_presenter_spec.rb

+require 'spec_helper'
+
+describe MergeRequestPresenter do
+  let(:resource) { create :merge_request, source_project: project }
+  let!(:project) { create(:project, :repository) }
+  let!(:user) { project.creator }
+
+  describe '#approvals_path' do
+    subject { described_class.new(resource, current_user: user).approvals_path }
+
+    it 'returns path' do
+      is_expected.to eq("/#{resource.project.full_path}/merge_requests/#{resource.iid}/approvals")
+    end
+  end
+
+  describe '#approvers_left' do
+    let!(:private_group) { create(:group_with_members, :private) }
+    let!(:public_group) { create(:group_with_members) }
+    let!(:public_approver_group) { create(:approver_group, target: resource, group: public_group) }
+    let!(:private_approver_group) { create(:approver_group, target: resource, group: private_group) }
+    let!(:approver) { create(:approver, target: resource) }
+
+    before do
+      resource.approvals.create!(user: approver.user)
+    end
+
+    subject { described_class.new(resource, current_user: user).approvers_left }
+
+    it { is_expected.to match_array(public_approver_group.users) }
+
+    context 'when user has access to private group' do
+      before do
+        private_group.add_user(user, Gitlab::Access::DEVELOPER)
+      end
+
+      it do
+        approvers = public_approver_group.users + private_approver_group.users - [user]
+
+        is_expected.to match_array(approvers)
+      end
+    end
+  end
+
+  describe '#overall_approver_groups' do
+    let!(:private_group) { create(:group_with_members, :private) }
+    let!(:public_group) { create(:group_with_members) }
+    let!(:public_approver_group) { create(:approver_group, target: resource, group: public_group) }
+    let!(:private_approver_group) { create(:approver_group, target: resource, group: private_group) }
+
+    subject { described_class.new(resource, current_user: user).overall_approver_groups }
+
+    it { is_expected.to match_array([public_approver_group]) }
+
+    context 'when user has access to private group' do
+      before do
+        private_group.add_user(user, Gitlab::Access::DEVELOPER)
+      end
+
+      it { is_expected.to match_array([public_approver_group, private_approver_group]) }
+    end
+  end
+
+  describe '#all_approvers_including_groups' do
+    let!(:private_group) { create(:group_with_members, :private) }
+    let!(:public_group) { create(:group_with_members) }
+    let!(:public_approver_group) { create(:approver_group, target: resource, group: public_group) }
+    let!(:private_approver_group) { create(:approver_group, target: resource, group: private_group) }
+    let!(:approver) { create(:approver, target: resource) }
+
+    subject { described_class.new(resource, current_user: user).all_approvers_including_groups }
+
+    it { is_expected.to match_array(public_approver_group.users + [approver.user]) }
+
+    context 'when user has access to private group' do
+      before do
+        private_group.add_user(user, Gitlab::Access::DEVELOPER)
+      end
+
+      it do
+        approvers = [public_approver_group.users, private_approver_group.users, approver.user].flatten - [user]
+
+        is_expected.to match_array(approvers)
+      end
+    end
+  end
+end

ee/spec/requests/api/merge_request_approvals_spec.rb

@@ -33,6 +33,29 @@ describe API::MergeRequestApprovals do
       expect(json_response['approver_groups'][0]['group']['name']).to eq(group.name)
     end
 
+    context 'when private group approver' do
+      before do
+        private_group = create(:group, :private)
+        merge_request.approver_groups.create(group: private_group)
+      end
+
+      it 'only shows group approvers visible to the user' do
+        get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", user)
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response['approver_groups']).to be_empty
+      end
+
+      context 'when admin' do
+        it 'shows all approver groups' do
+          get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", admin)
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(json_response['approver_groups'].size).to eq(1)
+        end
+      end
+    end
+
     context 'when approvers are set to zero' do
       before do
         project.update!(approvals_before_merge: 0)
@@ -100,17 +123,29 @@ describe API::MergeRequestApprovals do
           expect(response).to have_gitlab_http_status(422)
         end
       end
+
+      it 'only shows approver groups that are visible to current user' do
+        private_group = create(:group, :private)
+        merge_request.approver_groups.create(group: private_group)
+
+        post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", current_user), approvals_required: 5
+
+        expect(response).to have_gitlab_http_status(201)
+        expect(json_response['approver_groups'].size).to eq(approver_groups_count)
+      end
     end
 
     context 'as a project admin' do
       it_behaves_like 'user allowed to override approvals required' do
         let(:current_user) { user }
+        let(:approver_groups_count) { 0 }
       end
     end
 
     context 'as a global admin' do
       it_behaves_like 'user allowed to override approvals required' do
         let(:current_user) { admin }
+        let(:approver_groups_count) { 1 }
       end
     end
 
@@ -196,17 +231,30 @@ describe API::MergeRequestApprovals do
           end
         end
       end
+
+      it 'only shows approver groups that are visible to current user' do
+        private_group = create(:group, :private)
+        merge_request.approver_groups.create(group: private_group)
+
+        put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvers", current_user),
+          approver_ids: [approver.id], approver_group_ids: [private_group.id, approver_group.id]
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response['approver_groups'].size).to eq(approver_groups_count)
+      end
     end
 
     context 'as a project admin' do
       it_behaves_like 'user allowed to change approvers' do
         let(:current_user) { user }
+        let(:approver_groups_count) { 1 }
       end
     end
 
     context 'as a global admin' do
       it_behaves_like 'user allowed to change approvers' do
         let(:current_user) { admin }
+        let(:approver_groups_count) { 2 }
       end
     end
 
@@ -288,6 +336,16 @@ describe API::MergeRequestApprovals do
           expect(merge_request.reload.approvals_left).to eq(2)
         end
       end
+
+      it 'only shows group approvers visible to the user' do
+        private_group = create(:group, :private)
+        merge_request.approver_groups.create(group: private_group)
+
+        approve
+
+        expect(response).to have_gitlab_http_status(201)
+        expect(json_response['approver_groups']).to be_empty
+      end
     end
   end
 
@@ -302,11 +360,11 @@ describe API::MergeRequestApprovals do
         project.add_developer(create(:user))
         merge_request.approvals.create(user: approver)
         merge_request.approvals.create(user: unapprover)
-
-        post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/unapprove", unapprover)
       end
 
       it 'unapproves the merge request' do
+        post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/unapprove", unapprover)
+
         expect(response).to have_gitlab_http_status(201)
         expect(json_response['approvals_left']).to eq(1)
         usernames = json_response['approved_by'].map { |u| u['user']['username'] }
@@ -315,6 +373,16 @@ describe API::MergeRequestApprovals do
         expect(json_response['user_has_approved']).to be false
         expect(json_response['user_can_approve']).to be true
       end
+
+      it 'only shows group approvers visible to the user' do
+        private_group = create(:group, :private)
+        merge_request.approver_groups.create(group: private_group)
+
+        post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/unapprove", unapprover)
+
+        expect(response).to have_gitlab_http_status(201)
+        expect(json_response['approver_groups']).to be_empty
+      end
     end
   end
 end

ee/spec/requests/api/project_approvals_spec.rb

@@ -27,6 +27,16 @@ describe API::ProjectApprovals do
         expect(response).to match_response_schema('public_api/v4/project_approvers', dir: 'ee')
       end
     end
+
+    it 'only shows approver groups that are visible to the user' do
+      private_group = create(:group, :private)
+      project.approver_groups.create(group: private_group)
+
+      get api(url, user)
+
+      expect(response).to match_response_schema('public_api/v4/project_approvers', dir: 'ee')
+      expect(json_response["approver_groups"]).to be_empty
+    end
   end
 
   describe 'POST /projects/:id/approvers' do
@@ -68,18 +78,30 @@ describe API::ProjectApprovals do
 
           expect(JSON.parse(response.body).symbolize_keys).to include(settings)
         end
+
+        it 'only shows approver groups that are visible to the current user' do
+          private_group = create(:group, :private)
+          project.approver_groups.create(group: private_group)
+
+          post api(url, current_user), approvals_before_merge: 3
+
+          expect(response).to match_response_schema('public_api/v4/project_approvers', dir: 'ee')
+          expect(json_response["approver_groups"].size).to eq(visible_approver_groups_count)
+        end
       end
     end
 
     context 'as a project admin' do
       it_behaves_like 'a user with access' do
         let(:current_user) { user }
+        let(:visible_approver_groups_count) { 0 }
       end
     end
 
     context 'as a global admin' do
       it_behaves_like 'a user with access' do
         let(:current_user) { admin }
+        let(:visible_approver_groups_count) { 1 }
       end
     end
 
@@ -94,6 +116,7 @@ describe API::ProjectApprovals do
 
   describe 'PUT /projects/:id/approvers' do
     let(:url) { "/projects/#{project.id}/approvers" }
+
     shared_examples_for 'a user with access' do
       it 'removes all approvers if no params are given' do
         project.approvers.create(user: approver)
@@ -136,17 +159,31 @@ describe API::ProjectApprovals do
         expect(json_response['approvers'][0]['user']['username']).to eq(approver.username)
         expect(json_response['approver_groups'][0]['group']['name']).to eq(group.name)
       end
+
+      it 'only shows approver groups that are visible to the current user' do
+        private_group = create(:group, :private)
+        project.approvers.create(user: approver)
+
+        expect do
+          put api(url, current_user), approver_ids: [approver.id], approver_group_ids: [private_group.id]
+        end.to change { project.approver_groups.count }.from(0).to(1)
+
+        expect(response).to match_response_schema('public_api/v4/project_approvers', dir: 'ee')
+        expect(json_response["approver_groups"].size).to eq(visible_approver_groups_count)
+      end
     end
 
     context 'as a project admin' do
       it_behaves_like 'a user with access' do
         let(:current_user) { user }
+        let(:visible_approver_groups_count) { 0 }
       end
     end
 
     context 'as a global admin' do
       it_behaves_like 'a user with access' do
         let(:current_user) { admin }
+        let(:visible_approver_groups_count) { 1 }
       end
     end
 

ee/spec/views/shared/issuable/_approvals.html.haml_spec.rb

@@ -4,6 +4,7 @@ describe 'shared/issuable/_approvals.html.haml' do
   let(:user) { create(:user) }
   let(:project) { build(:project) }
   let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+  let(:presenter) { merge_request.present(current_user: user) }
   let(:form) { double('form') }
 
   before do
@@ -18,14 +19,14 @@ describe 'shared/issuable/_approvals.html.haml' do
 
   context 'has no approvers' do
     it 'shows empty approvers list' do
-      render 'shared/issuable/approvals', form: form, issuable: merge_request
+      render 'shared/issuable/approvals', form: form, issuable: merge_request, presenter: presenter
 
       expect(rendered).to have_text('There are no approvers')
     end
 
     context 'can override approvers' do
       before do
-        render 'shared/issuable/approvals', form: form, issuable: merge_request
+        render 'shared/issuable/approvals', form: form, issuable: merge_request, presenter: presenter
       end
 
       it 'shows suggested approvers' do
@@ -44,7 +45,7 @@ describe 'shared/issuable/_approvals.html.haml' do
     context 'can not override approvers' do
       before do
         allow(view).to receive(:can?).with(user, :update_approvers, merge_request).and_return(false)
-        render 'shared/issuable/approvals', form: form, issuable: merge_request
+        render 'shared/issuable/approvals', form: form, issuable: merge_request, presenter: presenter
       end
 
       it 'hides suggested approvers' do
@@ -69,10 +70,11 @@ describe 'shared/issuable/_approvals.html.haml' do
     before do
       assign(:approver, approver)
       assign(:approver_group, approver_group)
+      assign(:presenter, merge_request.present(current_user: user))
     end
 
     it 'shows approver in table' do
-      render 'shared/issuable/approvals', form: form, issuable: merge_request, project: project
+      render 'shared/issuable/approvals', form: form, issuable: merge_request, project: project, presenter: presenter
 
       expect(rendered).to have_text(approver[:name])
       expect(rendered).to have_text(approver_group[:name])
@@ -80,7 +82,7 @@ describe 'shared/issuable/_approvals.html.haml' do
 
     context 'can override approvers' do
       it 'shows remove button for approver' do
-        render 'shared/issuable/approvals', form: form, issuable: merge_request
+        render 'shared/issuable/approvals', form: form, issuable: merge_request, presenter: presenter
 
         expect(rendered).to have_css('.btn-remove')
       end
@@ -90,7 +92,7 @@ describe 'shared/issuable/_approvals.html.haml' do
       it 'hides remove button' do
         allow(view).to receive(:can?).with(user, :update_approvers, merge_request).and_return(false)
 
-        render 'shared/issuable/approvals', form: form, issuable: merge_request
+        render 'shared/issuable/approvals', form: form, issuable: merge_request, presenter: presenter
 
         expect(rendered).not_to have_css('.btn-remove')
       end

rubocop/cop/group_public_or_visible_to_user.rb

+# frozen_string_literal: true
+#
+module RuboCop
+  module Cop
+    # Cop that blacklists the usage of Group.public_or_visible_to_user
+    class GroupPublicOrVisibleToUser < RuboCop::Cop::Cop
+      MSG = '`Group.public_or_visible_to_user` should be used with extreme care. ' \
+        'Please ensure that you are not using it on its own and that the amount ' \
+        'of rows being filtered is reasonable.'
+
+      def_node_matcher :public_or_visible_to_user?, <<~PATTERN
+        (send (const nil? :Group) :public_or_visible_to_user ...)
+      PATTERN
+
+      def on_send(node)
+        return unless public_or_visible_to_user?(node)
+
+        add_offense(node, location: :expression)
+      end
+    end
+  end
+end

rubocop/rubocop.rb

@@ -38,3 +38,4 @@ require_relative 'cop/code_reuse/service_class'
 require_relative 'cop/code_reuse/presenter'
 require_relative 'cop/code_reuse/serializer'
 require_relative 'cop/code_reuse/active_record'
+require_relative 'cop/group_public_or_visible_to_user'

spec/models/group_spec.rb

@@ -169,22 +169,42 @@ describe Group do
     end
   end
 
-  describe '.visible_to_user' do
-    let!(:group) { create(:group) }
-    let!(:user)  { create(:user) }
+  describe '.public_or_visible_to_user' do
+    let!(:private_group)  { create(:group, :private)  }
+    let!(:internal_group) { create(:group, :internal) }
 
-    subject { described_class.visible_to_user(user) }
+    subject { described_class.public_or_visible_to_user(user) }
 
-    describe 'when the user has access to a group' do
-      before do
-        group.add_user(user, Gitlab::Access::MAINTAINER)
-      end
+    context 'when user is nil' do
+      let!(:user) { nil }
 
-      it { is_expected.to eq([group]) }
+      it { is_expected.to match_array([group]) }
     end
 
-    describe 'when the user does not have access to any groups' do
-      it { is_expected.to eq([]) }
+    context 'when user' do
+      let!(:user) { create(:user) }
+
+      context 'when user does not have access to any private group' do
+        it { is_expected.to match_array([internal_group, group]) }
+      end
+
+      context 'when user is a member of private group' do
+        before do
+          private_group.add_user(user, Gitlab::Access::DEVELOPER)
+        end
+
+        it { is_expected.to match_array([private_group, internal_group, group]) }
+      end
+
+      context 'when user is a member of private subgroup', :postgresql do
+        let!(:private_subgroup) { create(:group, :private, parent: private_group) }
+
+        before do
+          private_subgroup.add_user(user, Gitlab::Access::DEVELOPER)
+        end
+
+        it { is_expected.to match_array([private_subgroup, internal_group, group]) }
+      end
     end
   end
 

spec/presenters/merge_request_presenter_spec.rb

@@ -434,33 +434,6 @@ describe MergeRequestPresenter do
     end
   end
 
-  describe '#approvals_path' do
-    before do
-      allow(resource).to receive(:requires_approve?) { requires_approve }
-    end
-
-    subject do
-      described_class.new(resource, current_user: user).approvals_path
-    end
-
-    context 'when approvals required' do
-      let(:requires_approve) { true }
-
-      it 'returns path' do
-        is_expected
-            .to eq("/#{resource.project.full_path}/merge_requests/#{resource.iid}/approvals")
-      end
-    end
-
-    context 'when approvals not required' do
-      let(:requires_approve) { false }
-
-      it 'returns nil' do
-        is_expected.to be_nil
-      end
-    end
-  end
-
   describe '#rebase_path' do
     before do
       allow(resource).to receive(:rebase_in_progress?) { rebase_in_progress }

spec/requests/api/groups_spec.rb

@@ -169,7 +169,7 @@ describe API::Groups do
         expect(response).to have_gitlab_http_status(200)
         expect(response).to include_pagination_headers
         expect(json_response).to be_an Array
-        expect(response_groups).to eq(Group.visible_to_user(user1).order(:name).pluck(:name))
+        expect(response_groups).to eq(groups_visible_to_user(user1).order(:name).pluck(:name))
       end
 
       it "sorts in descending order when passed" do
@@ -178,7 +178,7 @@ describe API::Groups do
         expect(response).to have_gitlab_http_status(200)
         expect(response).to include_pagination_headers
         expect(json_response).to be_an Array
-        expect(response_groups).to eq(Group.visible_to_user(user1).order(name: :desc).pluck(:name))
+        expect(response_groups).to eq(groups_visible_to_user(user1).order(name: :desc).pluck(:name))
       end
 
       it "sorts by path in order_by param" do
@@ -187,7 +187,7 @@ describe API::Groups do
         expect(response).to have_gitlab_http_status(200)
         expect(response).to include_pagination_headers
         expect(json_response).to be_an Array
-        expect(response_groups).to eq(Group.visible_to_user(user1).order(:path).pluck(:name))
+        expect(response_groups).to eq(groups_visible_to_user(user1).order(:path).pluck(:name))
       end
 
       it "sorts by id in the order_by param" do
@@ -196,7 +196,7 @@ describe API::Groups do
         expect(response).to have_gitlab_http_status(200)
         expect(response).to include_pagination_headers
         expect(json_response).to be_an Array
-        expect(response_groups).to eq(Group.visible_to_user(user1).order(:id).pluck(:name))
+        expect(response_groups).to eq(groups_visible_to_user(user1).order(:id).pluck(:name))
       end
 
       it "sorts also by descending id with pagination fix" do
@@ -205,7 +205,7 @@ describe API::Groups do
         expect(response).to have_gitlab_http_status(200)
         expect(response).to include_pagination_headers
         expect(json_response).to be_an Array
-        expect(response_groups).to eq(Group.visible_to_user(user1).order(id: :desc).pluck(:name))
+        expect(response_groups).to eq(groups_visible_to_user(user1).order(id: :desc).pluck(:name))
       end
 
       it "sorts identical keys by id for good pagination" do
@@ -225,6 +225,10 @@ describe API::Groups do
         expect(json_response).to be_an Array
         expect(response_groups_ids).to eq(Group.select { |group| group['name'] == 'same-name' }.map { |group| group['id'] }.sort)
       end
+
+      def groups_visible_to_user(user)
+        Group.where(id: user.authorized_groups.select(:id).reorder(nil))
+      end
     end
 
     context 'when using owned in the request' do

spec/rubocop/cop/group_public_or_visible_to_user_spec.rb

+require 'spec_helper'
+require 'rubocop'
+require 'rubocop/rspec/support'
+require_relative '../../../rubocop/cop/group_public_or_visible_to_user'
+
+describe RuboCop::Cop::GroupPublicOrVisibleToUser do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  it 'flags the use of Group.public_or_visible_to_user with a constant receiver' do
+    inspect_source('Group.public_or_visible_to_user')
+
+    expect(cop.offenses.size).to eq(1)
+  end
+
+  it 'does not flat the use of public_or_visible_to_user with a constant that is not Group' do
+    inspect_source('Project.public_or_visible_to_user')
+
+    expect(cop.offenses.size).to eq(0)
+  end
+
+  it 'does not flag the use of Group.public_or_visible_to_user with a send receiver' do
+    inspect_source('foo.public_or_visible_to_user')
+
+    expect(cop.offenses.size).to eq(0)
+  end
+end

spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb

@@ -12,6 +12,7 @@ describe 'projects/merge_requests/creations/_new_submit.html.haml' do
     assign(:hidden_commit_count, 0)
     assign(:total_commit_count, merge_request.commits.count)
     assign(:project, merge_request.target_project)
+    assign(:mr_presenter, merge_request.present(current_user: merge_request.author))
 
     allow(view).to receive(:can?).and_return(true)
     allow(view).to receive(:url_for).and_return('#')

spec/views/projects/merge_requests/edit.html.haml_spec.rb

@@ -24,6 +24,7 @@ describe 'projects/merge_requests/edit.html.haml' do
   before do
     assign(:project, project)
     assign(:merge_request, closed_merge_request)
+    assign(:mr_presenter, closed_merge_request.present(current_user: user))
 
     allow(view).to receive(:can?).and_return(true)
     allow(view).to receive(:current_user)