From 29f0b5f2fe3751a7cca84fc17e473702bf0c37ee Mon Sep 17 00:00:00 2001
From: Vasilii Iakliushin <viakliushin@gitlab.com>
Date: Tue, 4 Oct 2022 17:40:47 +0200
Subject: [PATCH] Approvers group does not persist after adding it to the
 approval rules

Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/376289

**Problem**

`public_or_visible_to_user` does not correctly resolves inherited
permissions for subgroups.

For the structure,

```
Group A -> Group B
User has direct permissions to Group A
```

`public_or_visible_to_user` will return `false` for Group B.

**Solution**

Use `accessible_to_user` method to correctly apply the permission check.
---
 .../approval_rules_eligible_filter.yml        |  8 ++++
 .../concerns/approval_rules/updater.rb        |  7 ++-
 .../approval_rules/create_service_spec.rb     | 46 +++++++++++++++++++
 3 files changed, 60 insertions(+), 1 deletion(-)
 create mode 100644 config/feature_flags/development/approval_rules_eligible_filter.yml

diff --git a/config/feature_flags/development/approval_rules_eligible_filter.yml b/config/feature_flags/development/approval_rules_eligible_filter.yml
new file mode 100644
index 00000000000000..e8d925d08a708c
--- /dev/null
+++ b/config/feature_flags/development/approval_rules_eligible_filter.yml
@@ -0,0 +1,8 @@
+---
+name: approval_rules_eligible_filter
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/100192
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/376331
+milestone: '15.5'
+type: development
+group: group::source code
+default_enabled: false
diff --git a/ee/app/services/concerns/approval_rules/updater.rb b/ee/app/services/concerns/approval_rules/updater.rb
index edb0bd3f280c86..60fb6eb729c9dd 100644
--- a/ee/app/services/concerns/approval_rules/updater.rb
+++ b/ee/app/services/concerns/approval_rules/updater.rb
@@ -42,7 +42,12 @@ def filter_eligible_users!
     def filter_eligible_groups!
       return unless params.key?(:group_ids)
 
-      params[:groups] = Group.id_in(params.delete(:group_ids)).public_or_visible_to_user(current_user)
+      params[:groups] =
+        if Feature.enabled?(:approval_rules_eligible_filter, project)
+          Group.id_in(params.delete(:group_ids)).accessible_to_user(current_user)
+        else
+          Group.id_in(params.delete(:group_ids)).public_or_visible_to_user(current_user)
+        end
     end
 
     def filter_eligible_protected_branches!
diff --git a/ee/spec/services/approval_rules/create_service_spec.rb b/ee/spec/services/approval_rules/create_service_spec.rb
index 8680bb757125ef..ccd01c235bec19 100644
--- a/ee/spec/services/approval_rules/create_service_spec.rb
+++ b/ee/spec/services/approval_rules/create_service_spec.rb
@@ -64,6 +64,52 @@
       end
     end
 
+    context 'when group is a subgroup with indirect permissions' do
+      let(:subgroup) { create(:group, :private, parent: new_groups.first) }
+
+      before do
+        new_groups.first.add_guest(user)
+      end
+
+      it 'creates and includes eligible groups and subgroups' do
+        result = described_class.new(target, user, {
+          name: 'security',
+          approvals_required: 1,
+          group_ids: new_groups.map(&:id) + [subgroup.id]
+        }).execute
+
+        expect(result[:status]).to eq(:success)
+
+        rule = result[:rule]
+
+        expect(rule.name).to eq('security')
+        expect(rule.approvals_required).to eq(1)
+        expect(rule.groups).to contain_exactly(new_groups.first, subgroup)
+      end
+
+      context 'when approval_rules_eligible_filter FF is disabled' do
+        before do
+          stub_feature_flags(approval_rules_eligible_filter: false)
+        end
+
+        it 'does not include subgroups with indirect permissions' do
+          result = described_class.new(target, user, {
+            name: 'security',
+            approvals_required: 1,
+            group_ids: new_groups.map(&:id) + [subgroup.id]
+          }).execute
+
+          expect(result[:status]).to eq(:success)
+
+          rule = result[:rule]
+
+          expect(rule.name).to eq('security')
+          expect(rule.approvals_required).to eq(1)
+          expect(rule.groups).to contain_exactly(new_groups.first)
+        end
+      end
+    end
+
     context 'when validation fails' do
       it 'returns error message' do
         result = described_class.new(target, user, {
-- 
GitLab