Merge branch '255981_correctly_check_subgroup_permissions' into 'master'

Fix approval visible groups detection

See merge request gitlab-org/gitlab!91598

config/feature_flags/development/subgroups_approval_rules.yml

+---
+name: subgroups_approval_rules
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91598
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/366741
+milestone: '15.2'
+type: development
+group: group::source code
+default_enabled: false

ee/app/finders/approval_rules/group_finder.rb

@@ -3,6 +3,8 @@
 # For caching group related queries relative to current_user
 module ApprovalRules
   class GroupFinder
+    include Gitlab::Utils::StrongMemoize
+
     attr_reader :rule, :current_user
 
     def initialize(rule, user)
@@ -11,7 +13,14 @@ def initialize(rule, user)
     end
 
     def visible_groups
-      @visible_groups ||= groups.public_or_visible_to_user(current_user)
+      if Feature.enabled?(:subgroups_approval_rules, rule.project)
+        strong_memoize(:visible_groups) do
+          Preloaders::GroupPolicyPreloader.new(groups, current_user).execute
+          groups.select { |group| current_user.can?(:read_group, group) }
+        end
+      else
+        @visible_groups ||= groups.public_or_visible_to_user(current_user)
+      end
     end
 
     # rubocop: disable CodeReuse/ActiveRecord
@@ -27,9 +36,9 @@ def contains_hidden_groups?
     private
 
     def groups
-      return Group.none if rule.any_approver?
-
-      rule.groups
+      strong_memoize(:groups) do
+        rule.any_approver? ? Group.none : rule.groups
+      end
     end
   end
 end

ee/spec/finders/approval_rules/group_finder_spec.rb

@@ -9,6 +9,9 @@
   let_it_be(:public_group) { create(:group, name: 'public_group') }
   let_it_be(:private_inaccessible_group) { create(:group, :private, name: 'private_inaccessible_group') }
   let_it_be(:private_accessible_group) { create(:group, :private, name: 'private_accessible_group') }
+  let_it_be(:private_accessible_subgroup) do
+    create(:group, :private, parent: private_accessible_group, name: 'private_accessible_subgroup')
+  end
 
   subject { described_class.new(rule, user) }
 
@@ -18,23 +21,67 @@
 
   context 'when with inaccessible groups' do
     before do
-      rule.groups = [public_group, private_inaccessible_group, private_accessible_group]
+      rule.groups = [public_group, private_inaccessible_group, private_accessible_group, private_accessible_subgroup]
     end
 
     it 'returns groups' do
-      expect(subject.visible_groups).to contain_exactly(public_group, private_accessible_group)
+      expect(subject.visible_groups).to contain_exactly(
+        public_group, private_accessible_group, private_accessible_subgroup
+      )
       expect(subject.hidden_groups).to contain_exactly(private_inaccessible_group)
       expect(subject.contains_hidden_groups?).to eq(true)
     end
+
+    context 'when user is an admin', :enable_admin_mode do
+      subject { described_class.new(rule, create(:admin)) }
+
+      it 'returns groups' do
+        expect(subject.visible_groups).to contain_exactly(
+          public_group, private_accessible_group, private_accessible_subgroup, private_inaccessible_group
+        )
+        expect(subject.hidden_groups).to be_empty
+        expect(subject.contains_hidden_groups?).to eq(false)
+      end
+    end
+
+    context 'avoid N+1 query', :request_store do
+      it 'avoids N+1 database queries' do
+        rule.reload
+
+        count = ActiveRecord::QueryRecorder.new { subject.visible_groups }.count
+
+        # Clear cached association and request cache
+        rule.reload
+        RequestStore.clear!
+
+        rule.groups << create(:group, :private, parent: private_accessible_group, name: 'private_accessible_subgroup2')
+
+        expect { described_class.new(rule, user).visible_groups }.not_to exceed_query_limit(count)
+      end
+    end
+
+    context 'when FF is disabled' do
+      before do
+        stub_feature_flags(subgroups_approval_rules: false)
+      end
+
+      it 'does not return subgroups' do
+        expect(subject.visible_groups).to contain_exactly(public_group, private_accessible_group)
+        expect(subject.hidden_groups).to contain_exactly(private_inaccessible_group, private_accessible_subgroup)
+        expect(subject.contains_hidden_groups?).to eq(true)
+      end
+    end
   end
 
   context 'when without inaccessible groups' do
     before do
-      rule.groups = [public_group, private_accessible_group]
+      rule.groups = [public_group, private_accessible_group, private_accessible_subgroup]
     end
 
     it 'returns groups' do
-      expect(subject.visible_groups).to contain_exactly(public_group, private_accessible_group)
+      expect(subject.visible_groups).to contain_exactly(
+        public_group, private_accessible_group, private_accessible_subgroup
+      )
       expect(subject.hidden_groups).to be_empty
       expect(subject.contains_hidden_groups?).to eq(false)
     end