Extract params filtering logic

Appends hidden groups if remove_hidden_groups is not true

ee/app/assets/javascripts/approvals/components/app.vue

@@ -38,7 +38,7 @@ export default {
 </script>
 
 <template>
-  <div>
+  <div class="js-approval-rules">
     <gl-loading-icon v-if="!hasLoaded" :size="2" />
     <template v-else>
       <div class="border-bottom">

ee/app/assets/javascripts/approvals/components/approvers_list_item.vue

@@ -2,15 +2,17 @@
 import { GlButton } from '@gitlab/ui';
 import Icon from '~/vue_shared/components/icon.vue';
 import Avatar from '~/vue_shared/components/project_avatar/default.vue';
-import { TYPE_USER, TYPE_GROUP } from '../constants';
+import HiddenGroupsItem from './hidden_groups_item.vue';
+import { TYPE_USER, TYPE_GROUP, TYPE_HIDDEN_GROUPS } from '../constants';
 
-const types = [TYPE_USER, TYPE_GROUP];
+const types = [TYPE_USER, TYPE_GROUP, TYPE_HIDDEN_GROUPS];
 
 export default {
   components: {
     GlButton,
     Icon,
     Avatar,
+    HiddenGroupsItem,
   },
   props: {
     approver: {
@@ -23,6 +25,9 @@ export default {
     isGroup() {
       return this.approver.type === TYPE_GROUP;
     },
+    isHiddenGroups() {
+      return this.approver.type === TYPE_HIDDEN_GROUPS;
+    },
     displayName() {
       return this.isGroup ? this.approver.full_path : this.approver.name;
     },
@@ -34,7 +39,10 @@ export default {
   <transition name="fade">
     <li class="settings-flex-row">
       <div class="px-3 d-flex align-items-center">
-        <avatar :project="approver" :size="24" /><span>{{ displayName }}</span>
+        <hidden-groups-item v-if="isHiddenGroups" />
+        <template v-else>
+          <avatar :project="approver" :size="24" /><span>{{ displayName }}</span>
+        </template>
         <gl-button variant="none" class="ml-auto" @click="$emit('remove', approver)">
           <icon name="remove" :aria-label="__('Remove')" />
         </gl-button>

ee/app/assets/javascripts/approvals/components/hidden_groups_item.vue

+<script>
+import { GlTooltipDirective, GlButton } from '@gitlab/ui';
+import Icon from '~/vue_shared/components/icon.vue';
+
+export default {
+  components: {
+    GlButton,
+    Icon,
+  },
+  directives: {
+    GlTooltip: GlTooltipDirective,
+  },
+};
+</script>
+
+<template>
+  <div class="d-flex align-items-center">
+    <div class="square s24 d-flex-center mr-2 text-tertiary">
+      <icon name="folder-o" :size="16" />
+    </div>
+    <span>{{ __('Private group(s)') }}</span>
+    <gl-button
+      v-gl-tooltip
+      :title="__('One or more groups that you don\'t have access to.')"
+      variant="blank"
+      class="ml-1 text-secondary"
+    >
+      <icon name="question-o" :size="16" />
+    </gl-button>
+  </div>
+</template>

ee/app/assets/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs.vue

@@ -8,6 +8,8 @@ const INPUT_APPROVALS_REQUIRED = 'merge_request[approval_rules_attributes][][app
 const INPUT_USER_IDS = 'merge_request[approval_rules_attributes][][user_ids][]';
 const INPUT_GROUP_IDS = 'merge_request[approval_rules_attributes][][group_ids][]';
 const INPUT_DELETE = 'merge_request[approval_rules_attributes][][_destroy]';
+const INPUT_REMOVE_HIDDEN_GROUPS =
+  'merge_request[approval_rules_attributes][][remove_hidden_groups]';
 const INPUT_FALLBACK_APPROVALS_REQUIRED = 'merge_request[approvals_before_merge]';
 
 export default {
@@ -26,6 +28,7 @@ export default {
   INPUT_USER_IDS,
   INPUT_GROUP_IDS,
   INPUT_DELETE,
+  INPUT_REMOVE_HIDDEN_GROUPS,
   INPUT_FALLBACK_APPROVALS_REQUIRED,
 };
 </script>
@@ -82,6 +85,12 @@ export default {
         :name="$options.INPUT_GROUP_IDS"
         type="hidden"
       />
+      <input
+        v-if="rule.removeHiddenGroups"
+        value="true"
+        :name="$options.INPUT_REMOVE_HIDDEN_GROUPS"
+        type="hidden"
+      />
     </div>
   </div>
 </template>

ee/app/assets/javascripts/approvals/components/rule_form.vue

@@ -5,7 +5,7 @@ import { GlButton } from '@gitlab/ui';
 import { sprintf, __ } from '~/locale';
 import ApproversList from './approvers_list.vue';
 import ApproversSelect from './approvers_select.vue';
-import { TYPE_USER, TYPE_GROUP } from '../constants';
+import { TYPE_USER, TYPE_GROUP, TYPE_HIDDEN_GROUPS } from '../constants';
 
 const DEFAULT_NAME = 'Default';
 
@@ -31,6 +31,7 @@ export default {
       approversToAdd: [],
       showValidation: false,
       isFallback: false,
+      containsHiddenGroups: false,
       ...this.getInitialData(),
     };
   },
@@ -102,6 +103,9 @@ export default {
         this.settings.allowMultiRule && this.isFallback && !this.name && !this.approvers.length
       );
     },
+    removeHiddenGroups() {
+      return this.containsHiddenGroups && !this.approversByType[TYPE_HIDDEN_GROUPS];
+    },
     submissionData() {
       return {
         id: this.initRule && this.initRule.id,
@@ -111,6 +115,7 @@ export default {
         groups: this.groupIds,
         userRecords: this.users,
         groupRecords: this.groups,
+        removeHiddenGroups: this.removeHiddenGroups,
       };
     },
   },
@@ -191,6 +196,8 @@ export default {
         };
       }
 
+      const { containsHiddenGroups = false, removeHiddenGroups = false } = this.initRule;
+
       const users = this.initRule.users.map(x => ({ ...x, type: TYPE_USER }));
       const groups = this.initRule.groups.map(x => ({ ...x, type: TYPE_GROUP }));
 
@@ -198,7 +205,12 @@ export default {
         name: this.initRule.name || '',
         approvalsRequired: this.initRule.approvalsRequired || 0,
         minApprovalsRequired: this.initRule.minApprovalsRequired || 0,
-        approvers: groups.concat(users),
+        containsHiddenGroups,
+        approvers: groups
+          .concat(users)
+          .concat(
+            containsHiddenGroups && !removeHiddenGroups ? [{ type: TYPE_HIDDEN_GROUPS }] : [],
+          ),
       };
     },
   },

ee/app/assets/javascripts/approvals/constants.js

 export const TYPE_USER = 'user';
 export const TYPE_GROUP = 'group';
+export const TYPE_HIDDEN_GROUPS = 'hidden_groups';
 
 export const RULE_TYPE_FALLBACK = 'fallback';
 export const RULE_TYPE_REGULAR = 'regular';

ee/app/assets/javascripts/approvals/mappers.js

@@ -6,6 +6,7 @@ export const mapApprovalRuleRequest = req => ({
   approvals_required: req.approvalsRequired,
   users: req.users,
   groups: req.groups,
+  remove_hidden_groups: req.removeHiddenGroups,
 });
 
 export const mapApprovalFallbackRuleRequest = req => ({
@@ -19,6 +20,7 @@ export const mapApprovalRuleResponse = res => ({
   approvalsRequired: res.approvals_required,
   minApprovalsRequired: res.source_rule ? res.source_rule.approvals_required : 0,
   approvers: res.approvers,
+  containsHiddenGroups: res.contains_hidden_groups,
   users: res.users,
   groups: res.groups,
 });

ee/app/controllers/ee/projects/merge_requests/application_controller.rb

@@ -14,7 +14,7 @@ module EE
 
         def merge_request_params_attributes
           attrs = super.push(
-            { approval_rules_attributes: [:id, :name, { user_ids: [] }, { group_ids: [] }, :approvals_required, :approval_project_rule_id, :_destroy] },
+            approval_rule_attributes,
             :approvals_before_merge,
             :approver_group_ids,
             :approver_ids
@@ -23,6 +23,21 @@ module EE
           attrs
         end
 
+        def approval_rule_attributes
+          {
+            approval_rules_attributes: [
+              :id,
+              :name,
+              { user_ids: [] },
+              { group_ids: [] },
+              :approvals_required,
+              :approval_project_rule_id,
+              :remove_hidden_groups,
+              :_destroy
+            ]
+          }
+        end
+
         # If the number of approvals is not greater than the project default, set to
         # the project default, so that we fall back to the project default. And
         # still allow overriding rules defined at the project level but not allow

ee/app/finders/approval_rules/group_finder.rb

+# frozen_string_literal: true
+
+# For caching group related queries relative to current_user
+module ApprovalRules
+  class GroupFinder
+    attr_reader :rule, :current_user
+
+    def initialize(rule, user)
+      @rule = rule
+      @current_user = user
+    end
+
+    def visible_groups
+      @visible_groups ||= rule.groups.public_or_visible_to_user(current_user)
+    end
+
+    # rubocop: disable CodeReuse/ActiveRecord
+    def hidden_groups
+      @hidden_groups ||= rule.groups.where.not(id: visible_groups.map(&:id))
+    end
+
+    def contains_hidden_groups?
+      hidden_groups.loaded? ? hidden_groups.present? : hidden_groups.exists?
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+  end
+end

ee/app/presenters/approval_rule_presenter.rb

@@ -2,6 +2,16 @@
 
 class ApprovalRulePresenter < Gitlab::View::Presenter::Delegated
   def groups
-    super.public_or_visible_to_user(current_user)
+    group_query_service.visible_groups
+  end
+
+  def contains_hidden_groups?
+    group_query_service.contains_hidden_groups?
+  end
+
+  private
+
+  def group_query_service
+    @group_query_service ||= ApprovalRules::GroupFinder.new(@subject, current_user)
   end
 end

ee/app/services/approval_rules/params_filtering_service.rb

+# frozen_string_literal: true
+
+# @params target [MergeRequest, Project]
+# @params params [Hash] for updating or creating target
+# @params user [User] current user
+#
+# Returns a copy of `params` with rules modified,
+# filtering rule users and groups based on accessibility from user
+module ApprovalRules
+  class ParamsFilteringService
+    include Gitlab::Utils::StrongMemoize
+
+    attr_reader :target, :params, :current_user, :rules, :visible_group_ids, :visible_user_ids
+
+    def initialize(target, user, params)
+      @target = target
+      @current_user = user
+      @params = params.with_indifferent_access
+
+      batch_load_visible_user_and_group_ids
+    end
+
+    def execute
+      return params unless params.key?(:approval_rules_attributes)
+
+      params[:approval_rules_attributes].each do |rule_attributes|
+        handle_rule(rule_attributes)
+      end
+
+      params
+    end
+
+    private
+
+    def handle_rule(rule_attributes)
+      if rule_attributes.key?(:group_ids)
+        provided_group_ids = rule_attributes[:group_ids].map(&:to_i)
+        rule_attributes[:group_ids] = provided_group_ids & visible_group_ids
+
+        append_hidden_groups(rule_attributes)
+      end
+
+      if rule_attributes.key?(:user_ids)
+        provided_user_ids = rule_attributes[:user_ids].map(&:to_i)
+        rule_attributes[:user_ids] = provided_user_ids & visible_user_ids
+      end
+    end
+
+    # Append hidden groups to params to prevent them from being removed,
+    # as hidden group ids are never passed to/back from clients for security reasons.
+    def append_hidden_groups(rule_attributes)
+      keep_hidden_groups = !Gitlab::Utils.to_boolean(rule_attributes.delete(:remove_hidden_groups))
+
+      return unless keep_hidden_groups
+      return unless rule_attributes.key?(:group_ids)
+
+      hidden_group_sourcing_rule = find_hidden_group_sourcing_rule(rule_attributes)
+
+      return unless hidden_group_sourcing_rule
+
+      rule_attributes[:group_ids].concat(
+        ::ApprovalRules::GroupFinder.new(hidden_group_sourcing_rule, current_user).hidden_groups.map(&:id)
+      )
+    end
+
+    # Allow ruby-level filtering with only 2 queries
+    def batch_load_visible_user_and_group_ids
+      return unless params.key?(:approval_rules_attributes)
+
+      # rubocop: disable CodeReuse/ActiveRecord
+      @visible_group_ids = params[:approval_rules_attributes].flat_map { |hash| hash[:group_ids] }
+      if @visible_group_ids.present?
+        @visible_group_ids = ::Group.id_in(@visible_group_ids).public_or_visible_to_user(current_user).pluck(:id)
+      end
+
+      @visible_user_ids = params[:approval_rules_attributes].flat_map { |hash| hash[:user_ids] }
+      if @visible_user_ids.present?
+        @visible_user_ids = project.members_among(::User.id_in(@visible_user_ids)).pluck(:id)
+      end
+      # rubocop: enable CodeReuse/ActiveRecord
+    end
+
+    def project
+      if target.is_a?(Project)
+        target
+      else
+        target.target_project
+      end
+    end
+
+    def updating?
+      strong_memoize(:updating) { @target.persisted? }
+    end
+
+    def find_hidden_group_sourcing_rule(rule_attributes)
+      rule_id = updating? ? rule_attributes[:id] : rule_attributes[:approval_project_rule_id]
+
+      return if rule_id.blank?
+
+      hidden_group_sourcing_rules[rule_id.to_i]
+    end
+
+    def hidden_group_sourcing_rules
+      strong_memoize(:hidden_group_sourcing_rules) do
+        source = updating? ? target : project
+        source.approval_rules.includes(:groups).index_by(&:id) # rubocop: disable CodeReuse/ActiveRecord
+      end
+    end
+  end
+end

ee/app/services/approval_rules/update_service.rb

@@ -2,11 +2,23 @@
 
 module ApprovalRules
   class UpdateService < ::ApprovalRules::BaseService
-    attr_reader :rule
+    attr_reader :rule, :keep_existing_hidden_groups
 
     def initialize(approval_rule, user, params)
       @rule = approval_rule
+      @keep_existing_hidden_groups = !Gitlab::Utils.to_boolean(params.delete(:remove_hidden_groups))
+
       super(@rule.project, user, params)
     end
+
+    def filter_eligible_groups!
+      super
+
+      # Append hidden groups unless removal is explicitly stated,
+      # as hidden group ids are never passed to/back from clients for security reasons.
+      if params[:groups] && keep_existing_hidden_groups
+        params[:groups] += GroupFinder.new(rule, current_user).hidden_groups
+      end
+    end
   end
 end

ee/app/services/ee/merge_requests/base_service.rb

@@ -12,36 +12,10 @@ module EE
           params.delete(:approver_group_ids)
         end
 
-        filter_approval_rule_groups_and_users(merge_request)
+        self.params = ApprovalRules::ParamsFilteringService.new(merge_request, current_user, params).execute
 
         super
       end
-
-      def filter_approval_rule_groups_and_users(merge_request)
-        return unless params.key?(:approval_rules_attributes)
-
-        # For efficiency, we avoid repeated check per rule for eligibility of users and groups
-        # but instead consolidate all ids so eligibility can be checked in one go.
-        group_ids = params[:approval_rules_attributes].flat_map { |hash| hash[:group_ids] }
-        user_ids = params[:approval_rules_attributes].flat_map { |hash| hash[:user_ids] }
-
-        # rubocop: disable CodeReuse/ActiveRecord
-        group_ids = ::Group.id_in(group_ids).public_or_visible_to_user(current_user).pluck(:id) unless group_ids.empty?
-        user_ids = merge_request.project.members_among(::User.id_in(user_ids)).pluck(:id) unless user_ids.empty?
-        # rubocop: enable CodeReuse/ActiveRecord
-
-        params[:approval_rules_attributes].each do |rule_attributes|
-          if rule_attributes.key?(:group_ids)
-            provided_group_ids = rule_attributes[:group_ids].map(&:to_i)
-            rule_attributes[:group_ids] = provided_group_ids & group_ids
-          end
-
-          if rule_attributes.key?(:user_ids)
-            provided_user_ids = rule_attributes[:user_ids].map(&:to_i)
-            rule_attributes[:user_ids] = provided_user_ids & user_ids
-          end
-        end
-      end
     end
   end
 end

ee/changelogs/unreleased/10356-private-group-handling.yml

+---
+title: Fix project approval rule with only private group being considered as approved when override is allowed
+merge_request: 10356
+author:
+type: fixed

ee/lib/api/project_approval_rules.rb

@@ -56,6 +56,7 @@ module API
               optional :approvals_required, type: Integer, desc: 'The number of required approvals for this rule'
               optional :users, as: :user_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The user ids for this rule'
               optional :groups, as: :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule'
+              optional :remove_hidden_groups, type: Boolean, desc: 'Whether hidden groups should be removed'
             end
             put do
               authorize! :admin_project, user_project

ee/lib/ee/api/entities.rb

@@ -280,6 +280,7 @@ module EE
         expose :approvals_required
         expose :users, using: ::API::Entities::UserBasic
         expose :groups, using: ::API::Entities::Group
+        expose :contains_hidden_groups?, as: :contains_hidden_groups
       end
 
       class MergeRequestApprovalRule < ApprovalRule

ee/spec/features/merge_request/user_sets_approval_rules_spec.rb

+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe 'Merge request > User sets approval rules', :js do
+  let(:approver) { create(:user) }
+  let(:author) { create(:user) }
+  let(:project) { create(:project, :public, :repository) }
+
+  before do
+    stub_licensed_features(multiple_approval_rules: true)
+
+    [approver, author].each do |member|
+      project.add_maintainer(member)
+    end
+  end
+
+  context "with project approval rules" do
+    let!(:regular_rule) { create(:approval_project_rule, project: project, users: [approver], name: 'Regular Rule') }
+
+    context "with a private group rule" do
+      let!(:private_group) { create(:group, :private) }
+      let!(:private_rule) { create(:approval_project_rule, project: project, groups: [private_group], name: 'Private Rule') }
+      let!(:rules) { [regular_rule, private_rule] }
+
+      before do
+        private_group.add_developer(approver)
+
+        sign_in(author)
+        visit project_new_merge_request_path(project, merge_request: { target_branch: 'master', source_branch: 'feature' })
+        wait_for_requests
+      end
+
+      it "shows approval rules" do
+        names = page.all('.js-approval-rules table .js-name')
+        rules.each.with_index do |rule, idx|
+          expect(names[idx]).to have_text(rule.name)
+        end
+      end
+
+      it "persists hidden groups that author has no access to when creating MR" do
+        click_on("Submit merge request")
+        wait_for_requests
+
+        click_on("View eligible approvers")
+        wait_for_requests
+
+        tr = page.find(:css, 'tr', text: private_rule.name)
+        expect(tr).to have_selector('.js-approvers a.user-avatar-link')
+      end
+    end
+  end
+end

ee/spec/finders/approval_rules/group_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ApprovalRules::GroupFinder do
+  let(:rule) { create(:approval_project_rule) }
+  let(:user) { create(:user) }
+
+  let(:public_group) { create(:group, name: 'public_group') }
+  let(:private_inaccessible_group) { create(:group, :private, name: 'private_inaccessible_group') }
+  let(:private_accessible_group) { create(:group, :private, name: 'private_accessible_group') }
+
+  subject { described_class.new(rule, user) }
+
+  before do
+    private_accessible_group.add_owner(user)
+  end
+
+  context 'when with inaccessible groups' do
+    before do
+      rule.groups = [public_group, private_inaccessible_group, private_accessible_group]
+    end
+
+    it 'returns groups' do
+      expect(subject.visible_groups).to contain_exactly(public_group, private_accessible_group)
+      expect(subject.hidden_groups).to contain_exactly(private_inaccessible_group)
+      expect(subject.contains_hidden_groups?).to eq(true)
+    end
+  end
+
+  context 'when without inaccessible groups' do
+    before do
+      rule.groups = [public_group, private_accessible_group]
+    end
+
+    it 'returns groups' do
+      expect(subject.visible_groups).to contain_exactly(public_group, private_accessible_group)
+      expect(subject.hidden_groups).to be_empty
+      expect(subject.contains_hidden_groups?).to eq(false)
+    end
+  end
+end

ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rule.json

@@ -4,6 +4,7 @@
     "id": { "type": "integer" },
     "name": { "type": "string" },
     "approvals_required": { "type": "integer" },
+    "contains_hidden_groups": { "type": "boolean" },
     "rule_type": { "type": "tring" },
     "approvers": {
       "type": "array",

ee/spec/javascripts/approvals/components/approvers_list_item_spec.js

 import { shallowMount, createLocalVue } from '@vue/test-utils';
 import { GlButton } from '@gitlab/ui';
 import Avatar from '~/vue_shared/components/project_avatar/default.vue';
-import { TYPE_USER, TYPE_GROUP } from 'ee/approvals/constants';
+import { TYPE_USER, TYPE_GROUP, TYPE_HIDDEN_GROUPS } from 'ee/approvals/constants';
 import ApproversListItem from 'ee/approvals/components/approvers_list_item.vue';
+import HiddenGroupsItem from 'ee/approvals/components/hidden_groups_item.vue';
 
 const localVue = createLocalVue();
 const TEST_USER = {
@@ -69,5 +70,27 @@ describe('Approvals ApproversListItem', () => {
       expect(wrapper.text()).toContain(TEST_GROUP.full_path);
       expect(wrapper.text()).not.toContain(TEST_GROUP.name);
     });
+
+    it('does not render hidden-groups-item', () => {
+      expect(wrapper.find(HiddenGroupsItem).exists()).toBe(false);
+    });
+  });
+
+  describe('when hidden groups', () => {
+    beforeEach(() => {
+      factory({
+        propsData: {
+          approver: { type: TYPE_HIDDEN_GROUPS },
+        },
+      });
+    });
+
+    it('renders hidden-groups-item', () => {
+      expect(wrapper.find(HiddenGroupsItem).exists()).toBe(true);
+    });
+
+    it('does not render avatar', () => {
+      expect(wrapper.find(Avatar).exists()).toBe(false);
+    });
   });
 });

ee/spec/javascripts/approvals/components/hidden_groups_item_spec.js

+import { shallowMount, createLocalVue } from '@vue/test-utils';
+import HiddenGroupsItem from 'ee/approvals/components/hidden_groups_item.vue';
+
+const localVue = createLocalVue();
+
+describe('Approvals HiddenGroupsItem', () => {
+  let wrapper;
+
+  const factory = (options = {}) => {
+    wrapper = shallowMount(localVue.extend(HiddenGroupsItem), {
+      ...options,
+      localVue,
+      sync: false,
+    });
+  };
+
+  afterEach(() => {
+    wrapper.destroy();
+    wrapper = null;
+  });
+
+  it('renders successfully', () => {
+    factory();
+
+    expect(wrapper.exists()).toBe(true);
+  });
+});

ee/spec/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs_spec.js

@@ -16,6 +16,7 @@ const {
   INPUT_USER_IDS,
   INPUT_GROUP_IDS,
   INPUT_DELETE,
+  INPUT_REMOVE_HIDDEN_GROUPS,
   INPUT_FALLBACK_APPROVALS_REQUIRED,
 } = MRRulesHiddenInputs;
 const TEST_USERS = [{ id: 1 }, { id: 10 }];
@@ -184,6 +185,21 @@ describe('EE Approvlas MRRulesHiddenInputs', () => {
           });
         });
       });
+
+      describe('with remove hidden groups', () => {
+        beforeEach(() => {
+          rule.removeHiddenGroups = true;
+        });
+
+        it('renders input to remove hidden groups', () => {
+          factory();
+
+          expect(findHiddenInputs()).toContain({
+            name: INPUT_REMOVE_HIDDEN_GROUPS,
+            value: 'true',
+          });
+        });
+      });
     });
   });
 });

ee/spec/javascripts/approvals/components/rule_form_spec.js

@@ -4,8 +4,9 @@ import { GlButton } from '@gitlab/ui';
 import { createStoreOptions } from 'ee/approvals/stores';
 import projectSettingsModule from 'ee/approvals/stores/modules/project_settings';
 import ApproversSelect from 'ee/approvals/components/approvers_select.vue';
+import ApproversList from 'ee/approvals/components/approvers_list.vue';
 import RuleForm from 'ee/approvals/components/rule_form.vue';
-import { TYPE_USER, TYPE_GROUP } from 'ee/approvals/constants';
+import { TYPE_USER, TYPE_GROUP, TYPE_HIDDEN_GROUPS } from 'ee/approvals/constants';
 
 const TEST_PROJECT_ID = '7';
 const TEST_RULE = {
@@ -25,6 +26,8 @@ const TEST_FALLBACK_RULE = {
 const localVue = createLocalVue();
 localVue.use(Vuex);
 
+const addType = type => x => Object.assign(x, { type });
+
 describe('EE Approvals RuleForm', () => {
   let wrapper;
   let store;
@@ -48,6 +51,7 @@ describe('EE Approvals RuleForm', () => {
   const findApprovalsRequiredValidation = () => findValidation(findApprovalsRequiredInput(), false);
   const findApproversSelect = () => wrapper.find(ApproversSelect);
   const findApproversValidation = () => findValidation(findApproversSelect(), true);
+  const findApproversList = () => wrapper.find(ApproversList);
   const findValidations = () => [
     findNameValidation(),
     findApprovalsRequiredValidation(),
@@ -156,6 +160,7 @@ describe('EE Approvals RuleForm', () => {
           groups,
           userRecords,
           groupRecords,
+          removeHiddenGroups: false,
         };
 
         findNameInput().setValue(expected.name);
@@ -192,6 +197,15 @@ describe('EE Approvals RuleForm', () => {
         });
       });
 
+      it('shows approvers', () => {
+        const list = findApproversList();
+
+        expect(list.props('value')).toEqual([
+          ...TEST_RULE.groups.map(addType(TYPE_GROUP)),
+          ...TEST_RULE.users.map(addType(TYPE_USER)),
+        ]);
+      });
+
       it('on submit, puts rule', () => {
         const userRecords = TEST_RULE.users.map(x => ({ ...x, type: TYPE_USER }));
         const groupRecords = TEST_RULE.groups.map(x => ({ ...x, type: TYPE_GROUP }));
@@ -204,6 +218,7 @@ describe('EE Approvals RuleForm', () => {
           groups,
           userRecords,
           groupRecords,
+          removeHiddenGroups: false,
         };
 
         wrapper.vm.submit();
@@ -298,6 +313,77 @@ describe('EE Approvals RuleForm', () => {
         });
       });
     });
+
+    describe('with hidden groups rule', () => {
+      beforeEach(() => {
+        createComponent({
+          initRule: {
+            ...TEST_RULE,
+            containsHiddenGroups: true,
+          },
+        });
+      });
+
+      it('shows approvers and hidden group', () => {
+        const list = findApproversList();
+
+        expect(list.props('value')).toEqual([
+          ...TEST_RULE.groups.map(addType(TYPE_GROUP)),
+          ...TEST_RULE.users.map(addType(TYPE_USER)),
+          { type: TYPE_HIDDEN_GROUPS },
+        ]);
+      });
+
+      it('on submit, does not remove hidden groups', () => {
+        wrapper.vm.submit();
+
+        expect(actions.putRule).toHaveBeenCalledWith(
+          jasmine.anything(),
+          jasmine.objectContaining({
+            removeHiddenGroups: false,
+          }),
+          undefined,
+        );
+      });
+
+      describe('and hidden groups removed', () => {
+        beforeEach(() => {
+          wrapper.vm.approvers = wrapper.vm.approvers.filter(x => x.type !== TYPE_HIDDEN_GROUPS);
+        });
+
+        it('on submit, removes hidden groups', () => {
+          wrapper.vm.submit();
+
+          expect(actions.putRule).toHaveBeenCalledWith(
+            jasmine.anything(),
+            jasmine.objectContaining({
+              removeHiddenGroups: true,
+            }),
+            undefined,
+          );
+        });
+      });
+    });
+
+    describe('with removed hidden groups rule', () => {
+      beforeEach(() => {
+        createComponent({
+          initRule: {
+            ...TEST_RULE,
+            containsHiddenGroups: true,
+            removeHiddenGroups: true,
+          },
+        });
+      });
+
+      it('does not add hidden groups in approvers', () => {
+        expect(
+          findApproversList()
+            .props('value')
+            .every(x => x.type !== TYPE_HIDDEN_GROUPS),
+        ).toBe(true);
+      });
+    });
   });
 
   describe('when allow only single rule', () => {

ee/spec/presenters/approval_rule_presenter_spec.rb

@@ -3,13 +3,13 @@
 require 'spec_helper'
 
 describe ApprovalRulePresenter do
-  describe '#groups' do
-    set(:user) { create(:user) }
-    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) }
+  set(:user) { create(:user) }
+  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) }
 
+  describe '#groups' do
     shared_examples 'filtering private group' do
       context 'when user has no access to private group' do
         it 'excludes private group' do
@@ -49,4 +49,45 @@ describe ApprovalRulePresenter do
       end
     end
   end
+
+  describe '#contains_hidden_groups?' do
+    shared_examples 'detecting hidden group' do
+      context 'when user has no access to private group' do
+        it 'excludes private group' do
+          expect(subject.contains_hidden_groups?).to eq(true)
+        end
+      end
+
+      context 'when user has access to private group' do
+        it 'includes private group' do
+          private_group.add_owner(user)
+
+          expect(subject.contains_hidden_groups?).to eq(false)
+        end
+      end
+    end
+
+    context 'project rule' do
+      let(:rule) { create(:approval_project_rule, groups: groups) }
+
+      it_behaves_like 'detecting hidden group'
+    end
+
+    context 'wrapped approval rule' do
+      let(:rule) do
+        mr_rule = create(:approval_merge_request_rule, groups: groups)
+        ApprovalWrappedRule.new(mr_rule.merge_request, mr_rule)
+      end
+
+      it_behaves_like 'detecting hidden group'
+    end
+
+    context 'fallback rule' do
+      let(:rule) { ApprovalMergeRequestFallback.new(create(:merge_request)) }
+
+      it 'contains no groups without raising an error' do
+        expect(subject.contains_hidden_groups?).to eq(false)
+      end
+    end
+  end
 end

ee/spec/services/approval_rules/params_filtering_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ApprovalRules::ParamsFilteringService do
+  let(:service) { described_class.new(merge_request, user, params) }
+  let(:project_member) { create(:user) }
+  let(:outsider) { create(:user) }
+  let(:accessible_group) { create(:group, :private) }
+  let(:inaccessible_group) { create(:group, :private) }
+  let(:project) { create(:project, :repository) }
+  let(:user) { create(:user) }
+
+  describe '#execute' do
+    shared_examples_for(:assigning_users_and_groups) do
+      before do
+        project.add_maintainer(user)
+        project.add_reporter(project_member)
+
+        accessible_group.add_developer(user)
+      end
+
+      it 'only assigns eligible users and groups' do
+        params = service.execute
+
+        rule1 = params[:approval_rules_attributes].first
+
+        expect(rule1[:user_ids]).to contain_exactly(project_member.id)
+
+        rule2 = params[:approval_rules_attributes].last
+        expected_group_ids = expected_groups.map(&:id)
+
+        expect(rule2[:user_ids]).to be_empty
+        expect(rule2[:group_ids]).to contain_exactly(*expected_group_ids)
+      end
+    end
+
+    context 'create' do
+      it_behaves_like :assigning_users_and_groups do
+        let(:merge_request) { build(:merge_request, target_project: project, source_project: project) }
+        let(:params) do
+          {
+            title: 'Awesome merge_request',
+            description: 'please fix',
+            source_branch: 'feature',
+            target_branch: 'master',
+            force_remove_source_branch: '1',
+            approval_rules_attributes: [
+              { name: 'foo', user_ids: [project_member.id, outsider.id] },
+              { name: 'bar', user_ids: [outsider.id], group_ids: [accessible_group.id, inaccessible_group.id] }
+            ]
+          }
+        end
+        let(:expected_groups) { [accessible_group] }
+      end
+    end
+
+    context 'update' do
+      let(:merge_request) { create(:merge_request, target_project: project, source_project: project)}
+      let(:existing_private_group) { create(:group, :private) }
+      let!(:rule1) { create(:approval_merge_request_rule, merge_request: merge_request, users: [create(:user)]) }
+      let!(:rule2) { create(:approval_merge_request_rule, merge_request: merge_request, groups: [existing_private_group]) }
+
+      it_behaves_like :assigning_users_and_groups do
+        let(:params) do
+          {
+            approval_rules_attributes: [
+              { id: rule1.id, name: 'foo', user_ids: [project_member.id, outsider.id] },
+              { id: rule2.id, name: 'bar', user_ids: [outsider.id], group_ids: [accessible_group.id, inaccessible_group.id] }
+            ]
+          }
+        end
+        let(:expected_groups) { [accessible_group, existing_private_group] }
+      end
+
+      context 'with remove_hidden_groups being true' do
+        it_behaves_like :assigning_users_and_groups do
+          let(:params) do
+            {
+              approval_rules_attributes: [
+                { id: rule1.id, name: 'foo', user_ids: [project_member.id, outsider.id] },
+                { id: rule2.id, name: 'bar', user_ids: [outsider.id], group_ids: [accessible_group.id, inaccessible_group.id], remove_hidden_groups: true }
+              ]
+            }
+          end
+          let(:expected_groups) { [accessible_group] }
+        end
+      end
+    end
+  end
+end

ee/spec/services/approval_rules/update_service_spec.rb

@@ -54,6 +54,61 @@ describe ApprovalRules::UpdateService do
       end
     end
 
+    context 'when existing groups are inaccessible to user' do
+      let(:private_accessible_group) { create(:group, :private) }
+      let(:private_inaccessible_group) { create(:group, :private) }
+      let(:new_group) { create(:group) }
+
+      before do
+        approval_rule.groups = [private_accessible_group, private_inaccessible_group]
+        private_accessible_group.add_guest user
+      end
+
+      context 'when remove_hidden_groups is false' do
+        it 'preserves inaccessible groups' do
+          result = described_class.new(approval_rule, user, {
+            remove_hidden_groups: false,
+            group_ids: [new_group.id]
+          }).execute
+
+          expect(result[:status]).to eq(:success)
+
+          rule = result[:rule]
+
+          expect(rule.groups).to contain_exactly(private_inaccessible_group, new_group)
+        end
+      end
+
+      context 'when remove_hidden_groups is not specified' do
+        it 'removes inaccessible groups' do
+          result = described_class.new(approval_rule, user, {
+            group_ids: [new_group.id]
+          }).execute
+
+          expect(result[:status]).to eq(:success)
+
+          rule = result[:rule]
+
+          expect(rule.groups).to contain_exactly(private_inaccessible_group, new_group)
+        end
+      end
+
+      context 'when remove_hidden_groups is true' do
+        it 'removes inaccessible groups' do
+          result = described_class.new(approval_rule, user, {
+            remove_hidden_groups: true,
+            group_ids: [new_group.id]
+          }).execute
+
+          expect(result[:status]).to eq(:success)
+
+          rule = result[:rule]
+
+          expect(rule.groups).to contain_exactly(new_group)
+        end
+      end
+    end
+
     context 'when validation fails' do
       it 'returns error message' do
         result = described_class.new(approval_rule, user, {

ee/spec/services/ee/merge_requests/base_service_spec.rb

@@ -3,77 +3,34 @@
 require 'spec_helper'
 
 describe MergeRequests::BaseService do
-  include ProjectForksHelper
-
-  let(:project_member) { create(:user) }
-  let(:outsider) { create(:user) }
-  let(:accessible_group) { create(:group, :private) }
-  let(:inaccessible_group) { create(:group, :private) }
+  subject { MergeRequests::CreateService.new(project, project.owner, params) }
 
   let(:project) { create(:project, :repository) }
-  let(:user) { create(:user) }
+  let(:params_filtering_service) { double(:params_filtering_service) }
+  let(:params) do
+    {
+      title: 'Awesome merge_request',
+      description: 'please fix',
+      source_branch: 'feature',
+      target_branch: 'master'
+    }
+  end
 
   describe '#filter_params' do
     context 'filter users and groups' do
-      shared_examples_for(:assigning_users_and_groups) do
-        before do
-          project.add_maintainer(user)
-          project.add_reporter(project_member)
-
-          accessible_group.add_developer(user)
-
-          allow(service).to receive(:execute_hooks)
-        end
-
-        it 'only assigns eligible users and groups' do
-          merge_request = subject
-
-          rule1 = merge_request.approval_rules.regular.first
-
-          expect(rule1.users).to contain_exactly(*project_member)
-
-          rule2 = merge_request.approval_rules.regular.last
-
-          expect(rule2.users).to be_empty
-          expect(rule2.groups).to contain_exactly(*accessible_group)
-        end
-      end
-
-      context 'create' do
-        it_behaves_like :assigning_users_and_groups do
-          let(:service) { MergeRequests::CreateService.new(project, user, opts) }
-          let(:opts) do
-            {
-              title: 'Awesome merge_request',
-              description: 'please fix',
-              source_branch: 'feature',
-              target_branch: 'master',
-              force_remove_source_branch: '1',
-              approval_rules_attributes: [
-                { name: 'foo', user_ids: [project_member.id, outsider.id] },
-                { name: 'bar', user_ids: [outsider.id], group_ids: [accessible_group.id, inaccessible_group.id] }
-              ]
-            }
-          end
-          subject { service.execute }
-        end
+      before do
+        allow(subject).to receive(:execute_hooks)
       end
 
-      context 'update' do
-        let(:merge_request) { create(:merge_request, target_project: project, source_project: project)}
+      it 'calls ParamsFilteringService' do
+        expect(ApprovalRules::ParamsFilteringService).to receive(:new).with(
+          an_instance_of(MergeRequest),
+          project.owner,
+          params
+        ).and_return(params_filtering_service)
+        expect(params_filtering_service).to receive(:execute).and_return(params)
 
-        it_behaves_like :assigning_users_and_groups do
-          let(:service) { MergeRequests::UpdateService.new(project, user, opts) }
-          let(:opts) do
-            {
-              approval_rules_attributes: [
-                { name: 'foo', user_ids: [project_member.id, outsider.id] },
-                { name: 'bar', user_ids: [outsider.id], group_ids: [accessible_group.id, inaccessible_group.id] }
-              ]
-            }
-          end
-          subject { service.execute(merge_request) }
-        end
+        subject.execute
       end
     end
   end

locale/gitlab.pot

@@ -7326,6 +7326,9 @@ msgid_plural "%d more items"
 msgstr[0] ""
 msgstr[1] ""
 
+msgid "One or more groups that you don't have access to."
+msgstr ""
+
 msgid "One or more of your Bitbucket projects cannot be imported into GitLab directly because they use Subversion or Mercurial for version control, rather than Git."
 msgstr ""
 
@@ -7875,6 +7878,9 @@ msgstr ""
 msgid "Private - The group and its projects can only be viewed by members."
 msgstr ""
 
+msgid "Private group(s)"
+msgstr ""
+
 msgid "Private projects can be created in your personal namespace with:"
 msgstr ""