Merge branch 'security-11-1-6881-project-group-approvers-leaks-private-group-info-ce' into 'security-11-1'

[11.1] Project group approvers leaks private group info

See merge request gitlab/gitlabhq!2490

20 files changed, 159 insertions, 44 deletions

diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb
index 81129456ad8..37b5627c240 100644
--- a/app/controllers/projects/merge_requests/creations_controller.rb
+++ b/app/controllers/projects/merge_requests/creations_controller.rb
@@ -104,6 +104,10 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
     @commits = prepare_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
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index a7c5f858c42..d6ad0104ef0 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -313,6 +313,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     @source_project = @merge_request.source_project
     @target_project = @merge_request.target_project
     @target_branches = @merge_request.target_project.repository.branch_names
+
+    # 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
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index f2abe27f60e..e5303fd323e 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -13,6 +13,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]
@@ -431,4 +432,8 @@ class ProjectsController < Projects::ApplicationController
   def export_project_object_storage?
     @project.export_project_object_exists?
   end
+
+  def present_project
+    @project = @project.present(current_user: current_user)
+  end
 end
diff --git a/app/finders/joined_groups_finder.rb b/app/finders/joined_groups_finder.rb
index 47174980258..95eb808a185 100644
--- a/app/finders/joined_groups_finder.rb
+++ b/app/finders/joined_groups_finder.rb
@@ -1,4 +1,4 @@
-class JoinedGroupsFinder < UnionFinder
+class JoinedGroupsFinder
   def initialize(user)
     @user = user
   end
@@ -6,19 +6,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
diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb
index 70509e9066d..2f4a6dcdd0d 100644
--- a/app/mailers/emails/merge_requests.rb
+++ b/app/mailers/emails/merge_requests.rb
@@ -1,13 +1,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
 
@@ -71,11 +72,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
 
diff --git a/app/models/group.rb b/app/models/group.rb
index 9c171de7fc3..e2ae3bafb44 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -78,8 +78,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
@@ -91,6 +100,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
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index b4090fd8baf..bd57c234a45 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -4,6 +4,7 @@ class MergeRequest < ActiveRecord::Base
   include Issuable
   include Noteable
   include Referable
+  include Presentable
   include IgnorableColumn
   include TimeTrackable
   include ManualInverseAssociation
diff --git a/app/models/user.rb b/app/models/user.rb
index 27a5d0278b7..c0cb0049383 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -573,10 +573,12 @@ class User < ActiveRecord::Base
 
   # Returns the groups a user has access to, either through a membership or a project authorization
   def authorized_groups
-    union = Gitlab::SQL::Union
-      .new([groups.select(:id), authorized_projects.select(:namespace_id)])
+    Group.unscoped do
+      union = Gitlab::SQL::Union
+        .new([groups.select(:id), authorized_projects.select(:namespace_id)])
 
-    Group.where("namespaces.id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection
+      Group.where("namespaces.id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection
+    end
   end
 
   # Returns the groups a user is a member of, either directly or through a parent group
diff --git a/app/views/notify/new_merge_request_email.html.haml b/app/views/notify/new_merge_request_email.html.haml
index dd6a84e503d..5acd45b74a7 100644
--- a/app/views/notify/new_merge_request_email.html.haml
+++ b/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
diff --git a/app/views/notify/new_merge_request_email.text.erb b/app/views/notify/new_merge_request_email.text.erb
index d5b8f8d764f..754f4bca1cd 100644
--- a/app/views/notify/new_merge_request_email.text.erb
+++ b/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 %>
diff --git a/app/views/projects/merge_requests/_form.html.haml b/app/views/projects/merge_requests/_form.html.haml
index 5a59f956cb5..13b967beba1 100644
--- a/app/views/projects/merge_requests/_form.html.haml
+++ b/app/views/projects/merge_requests/_form.html.haml
@@ -1,4 +1,4 @@
 = 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
diff --git a/app/views/projects/merge_requests/creations/_new_submit.html.haml b/app/views/projects/merge_requests/creations/_new_submit.html.haml
index f7a5d85500f..6f226aac26b 100644
--- a/app/views/projects/merge_requests/creations/_new_submit.html.haml
+++ b/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
diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml
index b49e47a7266..17ce1c2b08b 100644
--- a/app/views/shared/issuable/_form.html.haml
+++ b/app/views/shared/issuable/_form.html.haml
@@ -1,6 +1,7 @@
 - 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
 
diff --git a/rubocop/cop/group_public_or_visible_to_user.rb b/rubocop/cop/group_public_or_visible_to_user.rb
new file mode 100644
index 00000000000..beda0b7f8ba
--- /dev/null
+++ b/rubocop/cop/group_public_or_visible_to_user.rb
@@ -0,0 +1,22 @@
+# 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
diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb
index aa7ae601f75..405b607b796 100644
--- a/rubocop/rubocop.rb
+++ b/rubocop/rubocop.rb
@@ -26,3 +26,4 @@ require_relative 'cop/project_path_helper'
 require_relative 'cop/rspec/env_assignment'
 require_relative 'cop/rspec/factories_in_migration_specs'
 require_relative 'cop/sidekiq_options_queue'
+require_relative 'cop/group_public_or_visible_to_user'
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 9fe1186a8c9..e708f22fc49 100644
--- a/spec/models/group_spec.rb
+++ b/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::MASTER)
-      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
 
diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb
index da23fdd7dca..8b11cd6f369 100644
--- a/spec/requests/api/groups_spec.rb
+++ b/spec/requests/api/groups_spec.rb
@@ -155,7 +155,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
@@ -164,7 +164,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
@@ -173,7 +173,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
@@ -182,7 +182,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
@@ -191,7 +191,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
@@ -211,6 +211,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
diff --git a/spec/rubocop/cop/group_public_or_visible_to_user_spec.rb b/spec/rubocop/cop/group_public_or_visible_to_user_spec.rb
new file mode 100644
index 00000000000..7b5235a3da7
--- /dev/null
+++ b/spec/rubocop/cop/group_public_or_visible_to_user_spec.rb
@@ -0,0 +1,28 @@
+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
diff --git a/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb b/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb
index 9ab105c3238..1822875f0fd 100644
--- a/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb
+++ b/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb
@@ -10,6 +10,7 @@ describe 'projects/merge_requests/creations/_new_submit.html.haml' do
     assign(:merge_request, merge_request)
     assign(:commits, merge_request.commits)
     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('#')
diff --git a/spec/views/projects/merge_requests/edit.html.haml_spec.rb b/spec/views/projects/merge_requests/edit.html.haml_spec.rb
index 9b74a7e1946..c13eab30054 100644
--- a/spec/views/projects/merge_requests/edit.html.haml_spec.rb
+++ b/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)