Hide approvers if a rule has any hidden groups

An approval rule that contains a private group can disclose information
about the membership of that group via the list of approvers for that
rule, which is constructed from all members of all groups, plus each
individual user included in the rule.

To avoid this information disclosure, hide all approvers in a rule
where even one of the groups is hidden to the viewer. This removes more
information than is strictly necessary, but is a simple fix for a hard
problem - right now, we don't track which approvers come from which
group, so it's difficult to be more precise, and this is something of
an edge case anyway.

ee/app/presenters/approval_rule_presenter.rb

 # frozen_string_literal: true
 
 class ApprovalRulePresenter < Gitlab::View::Presenter::Delegated
+  include Gitlab::Utils::StrongMemoize
+
+  # Hide all approvers if any of them might come from a hidden group. This
+  # represents an abundance of caution, but we can't tell which approvers come
+  # from a hidden group and which don't, from here, so this is the simplest
+  # thing we can do
+  def approvers
+    return [] if contains_hidden_groups?
+
+    super
+  end
+
   def groups
     group_query_service.visible_groups
   end
 
   def contains_hidden_groups?
+    strong_memoize(:contains_hidden_groups) do
       group_query_service.contains_hidden_groups?
     end
+  end
 
   private
 

ee/changelogs/unreleased/security-11214-hide-approvers.yml

+---
+title: Hide approvers if a rule has any hidden groups
+merge_request:
+author:
+type: security

ee/spec/features/merge_request/user_sets_approval_rules_spec.rb

@@ -60,7 +60,10 @@ describe 'Merge request > User sets approval rules', :js do
         wait_for_requests
 
         tr = page.find(:css, 'tr', text: private_rule.name)
-        expect(tr).to have_selector('.js-approvers a.user-avatar-link')
+        td = tr.find(:css, '.js-approvers')
+
+        # The approver granted by the private group is not visible
+        expect(td).to have_text('None')
       end
     end
   end

ee/spec/presenters/approval_rule_presenter_spec.rb

@@ -7,7 +7,28 @@ describe ApprovalRulePresenter do
   set(:public_group) { create(:group) }
   set(:private_group) { create(:group, :private) }
   let(:groups) { [public_group, private_group] }
-  subject { described_class.new(rule, current_user: user) }
+
+  subject(:presenter) { described_class.new(rule, current_user: user) }
+
+  describe '#approvers' do
+    set(:private_member) { create(:group_member, group: private_group) }
+    set(:public_member) { create(:group_member, group: public_group) }
+    set(:rule) { create(:approval_merge_request_rule, groups: [public_group, private_group]) }
+
+    subject { presenter.approvers }
+
+    context 'user cannot see one of the groups' do
+      it { is_expected.to be_empty }
+    end
+
+    context 'user can see all groups' do
+      before do
+        private_group.add_guest(user)
+      end
+
+      it { is_expected.to contain_exactly(user, private_member.user, public_member.user) }
+    end
+  end
 
   describe '#groups' do
     shared_examples 'filtering private group' do