From 487ae991ba9070a9337def153f577d90bd64359d Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Thu, 14 Mar 2019 11:39:53 +0800
Subject: [PATCH 1/7] Extract params filtering logic

Appends hidden groups if remove_hidden_groups is not true
---
 .../params_filtering_service.rb               | 109 ++++++++++++++++++
 .../ee/merge_requests/base_service.rb         |  28 +----
 .../params_filtering_service_spec.rb          |  91 +++++++++++++++
 .../ee/merge_requests/base_service_spec.rb    |  83 ++++---------
 4 files changed, 221 insertions(+), 90 deletions(-)
 create mode 100644 ee/app/services/approval_rules/params_filtering_service.rb
 create mode 100644 ee/spec/services/approval_rules/params_filtering_service_spec.rb

diff --git a/ee/app/services/approval_rules/params_filtering_service.rb b/ee/app/services/approval_rules/params_filtering_service.rb
new file mode 100644
index 00000000000000..12b0ec0aa0ae8a
--- /dev/null
+++ b/ee/app/services/approval_rules/params_filtering_service.rb
@@ -0,0 +1,109 @@
+# frozen_string_literal: true
+
+# @params target [MergeRequest, Project]
+# @params params [Hash] for updating or creating target
+# @params user [User] current user
+#
+# To modify `params` in-place per rule
+# filtering 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
+
+      batch_load_visible_user_and_group_ids
+    end
+
+    def execute
+      return unless params.key?(:approval_rules_attributes)
+
+      params[:approval_rules_attributes].each do |rule_attributes|
+        handle_rule(rule_attributes)
+      end
+    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?
+
+      rule_id = rule_id.to_i
+      hidden_group_sourcing_rules.find { |rule| rule.id == rule_id }
+    end
+
+    def hidden_group_sourcing_rules
+      strong_memoize(:hidden_group_sourcing_rules) do
+        source = updating? ? target : project
+        source.approval_rules.includes(:groups) # rubocop: disable CodeReuse/ActiveRecord
+      end
+    end
+  end
+end
diff --git a/ee/app/services/ee/merge_requests/base_service.rb b/ee/app/services/ee/merge_requests/base_service.rb
index e7715a1eab96a5..b0b2907823cdb0 100644
--- a/ee/app/services/ee/merge_requests/base_service.rb
+++ b/ee/app/services/ee/merge_requests/base_service.rb
@@ -12,36 +12,10 @@ def filter_params(merge_request)
           params.delete(:approver_group_ids)
         end
 
-        filter_approval_rule_groups_and_users(merge_request)
+        ApprovalRules::ParamsFilteringService.new(merge_request, params, current_user).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
diff --git a/ee/spec/services/approval_rules/params_filtering_service_spec.rb b/ee/spec/services/approval_rules/params_filtering_service_spec.rb
new file mode 100644
index 00000000000000..5e81e830b0150a
--- /dev/null
+++ b/ee/spec/services/approval_rules/params_filtering_service_spec.rb
@@ -0,0 +1,91 @@
+# 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
+        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
diff --git a/ee/spec/services/ee/merge_requests/base_service_spec.rb b/ee/spec/services/ee/merge_requests/base_service_spec.rb
index 2dd5a7372a08a8..cf19bc0775d015 100644
--- a/ee/spec/services/ee/merge_requests/base_service_spec.rb
+++ b/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),
+          params,
+          project.owner
+        ).and_return(params_filtering_service)
+        expect(params_filtering_service).to receive(:execute)
 
-        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
-- 
GitLab


From a531d1df5909e39cb9550b5a961aec4cded8e2b5 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Tue, 26 Mar 2019 14:41:26 +0800
Subject: [PATCH 2/7] Add finder for accessing hidden and visible groups

Caches query results to avoid extra queries when rendering API.
---
 ee/app/finders/approval_rules/group_finder.rb | 27 ++++++++++++
 .../approval_rules/group_finder_spec.rb       | 42 +++++++++++++++++++
 2 files changed, 69 insertions(+)
 create mode 100644 ee/app/finders/approval_rules/group_finder.rb
 create mode 100644 ee/spec/finders/approval_rules/group_finder_spec.rb

diff --git a/ee/app/finders/approval_rules/group_finder.rb b/ee/app/finders/approval_rules/group_finder.rb
new file mode 100644
index 00000000000000..a1dfab6b097471
--- /dev/null
+++ b/ee/app/finders/approval_rules/group_finder.rb
@@ -0,0 +1,27 @@
+# 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
diff --git a/ee/spec/finders/approval_rules/group_finder_spec.rb b/ee/spec/finders/approval_rules/group_finder_spec.rb
new file mode 100644
index 00000000000000..7a136867163b9e
--- /dev/null
+++ b/ee/spec/finders/approval_rules/group_finder_spec.rb
@@ -0,0 +1,42 @@
+# 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
-- 
GitLab


From 82dce2471c23b4f33d8d5c498ed87078a6057f99 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Tue, 26 Mar 2019 14:43:32 +0800
Subject: [PATCH 3/7] BE: expose contains_hidden_groups? and accepts
 remove_hidden_groups

Hidden groups can be preserved by appending them at backend,
when remove_hidden_groups is not true.
---
 .../merge_requests/application_controller.rb  | 17 +++++-
 ee/app/presenters/approval_rule_presenter.rb  | 12 +++-
 .../services/approval_rules/update_service.rb | 14 ++++-
 .../ee/merge_requests/base_service.rb         |  2 +-
 ee/lib/api/project_approval_rules.rb          |  1 +
 ee/lib/ee/api/entities.rb                     |  1 +
 .../public_api/v4/project_approval_rule.json  |  1 +
 .../approval_rule_presenter_spec.rb           | 53 ++++++++++++++++--
 .../approval_rules/update_service_spec.rb     | 55 +++++++++++++++++++
 .../ee/merge_requests/base_service_spec.rb    |  4 +-
 10 files changed, 148 insertions(+), 12 deletions(-)

diff --git a/ee/app/controllers/ee/projects/merge_requests/application_controller.rb b/ee/app/controllers/ee/projects/merge_requests/application_controller.rb
index 347478ba526552..d148c9f0099fde 100644
--- a/ee/app/controllers/ee/projects/merge_requests/application_controller.rb
+++ b/ee/app/controllers/ee/projects/merge_requests/application_controller.rb
@@ -14,7 +14,7 @@ def merge_request_params
 
         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 @@ def merge_request_params_attributes
           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
diff --git a/ee/app/presenters/approval_rule_presenter.rb b/ee/app/presenters/approval_rule_presenter.rb
index b7ca2bdf2118ba..fda2b3e299d652 100644
--- a/ee/app/presenters/approval_rule_presenter.rb
+++ b/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
diff --git a/ee/app/services/approval_rules/update_service.rb b/ee/app/services/approval_rules/update_service.rb
index b72236f8a55ef8..8df9c39687fbe2 100644
--- a/ee/app/services/approval_rules/update_service.rb
+++ b/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
diff --git a/ee/app/services/ee/merge_requests/base_service.rb b/ee/app/services/ee/merge_requests/base_service.rb
index b0b2907823cdb0..56d2cb68bfe08a 100644
--- a/ee/app/services/ee/merge_requests/base_service.rb
+++ b/ee/app/services/ee/merge_requests/base_service.rb
@@ -12,7 +12,7 @@ def filter_params(merge_request)
           params.delete(:approver_group_ids)
         end
 
-        ApprovalRules::ParamsFilteringService.new(merge_request, params, current_user).execute
+        ApprovalRules::ParamsFilteringService.new(merge_request, current_user, params).execute
 
         super
       end
diff --git a/ee/lib/api/project_approval_rules.rb b/ee/lib/api/project_approval_rules.rb
index 29d297a8777c20..a3ceade8546303 100644
--- a/ee/lib/api/project_approval_rules.rb
+++ b/ee/lib/api/project_approval_rules.rb
@@ -56,6 +56,7 @@ class ProjectApprovalRules < ::Grape::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
diff --git a/ee/lib/ee/api/entities.rb b/ee/lib/ee/api/entities.rb
index 2ba4df06c2e354..c275204c2303f7 100644
--- a/ee/lib/ee/api/entities.rb
+++ b/ee/lib/ee/api/entities.rb
@@ -280,6 +280,7 @@ def initialize(object, options = {})
         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
diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rule.json b/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rule.json
index c8510433db16fb..226c692a2f3ee0 100644
--- a/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rule.json
+++ b/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",
diff --git a/ee/spec/presenters/approval_rule_presenter_spec.rb b/ee/spec/presenters/approval_rule_presenter_spec.rb
index 87fdb3220e94ea..fd5d3525b5be1c 100644
--- a/ee/spec/presenters/approval_rule_presenter_spec.rb
+++ b/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 @@
       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
diff --git a/ee/spec/services/approval_rules/update_service_spec.rb b/ee/spec/services/approval_rules/update_service_spec.rb
index 6cacd2997edda7..f00b4d2e0025cf 100644
--- a/ee/spec/services/approval_rules/update_service_spec.rb
+++ b/ee/spec/services/approval_rules/update_service_spec.rb
@@ -54,6 +54,61 @@
       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, {
diff --git a/ee/spec/services/ee/merge_requests/base_service_spec.rb b/ee/spec/services/ee/merge_requests/base_service_spec.rb
index cf19bc0775d015..dcdcf9486bb832 100644
--- a/ee/spec/services/ee/merge_requests/base_service_spec.rb
+++ b/ee/spec/services/ee/merge_requests/base_service_spec.rb
@@ -25,8 +25,8 @@
       it 'calls ParamsFilteringService' do
         expect(ApprovalRules::ParamsFilteringService).to receive(:new).with(
           an_instance_of(MergeRequest),
-          params,
-          project.owner
+          project.owner,
+          params
         ).and_return(params_filtering_service)
         expect(params_filtering_service).to receive(:execute)
 
-- 
GitLab


From e1024f858a0d87f7d52f2e2caaf0ee3895b5683f Mon Sep 17 00:00:00 2001
From: Paul Slaughter <pslaughter@gitlab.com>
Date: Wed, 20 Mar 2019 01:27:54 -0500
Subject: [PATCH 4/7] FE for private groups in approval rules

**Why was this needed?**
The BE sends a flag `contains_hidden_groups` if an
approval rule contains groups that are hidden to
the current user. The FE needs to show a hidden groups
placeholder and allow removing the hidden groups without
exposing the groups themselves.
---
 .../components/approvers_list_item.vue        | 14 ++-
 .../components/hidden_groups_item.vue         | 31 +++++++
 .../mr_edit/mr_rules_hidden_inputs.vue        |  9 ++
 .../approvals/components/rule_form.vue        | 16 +++-
 .../assets/javascripts/approvals/constants.js |  1 +
 .../assets/javascripts/approvals/mappers.js   |  2 +
 .../10356-private-group-handling.yml          |  5 ++
 .../components/approvers_list_item_spec.js    | 25 +++++-
 .../components/hidden_groups_item_spec.js     | 27 ++++++
 .../mr_edit/mr_rules_hidden_inputs_spec.js    | 16 ++++
 .../approvals/components/rule_form_spec.js    | 88 ++++++++++++++++++-
 locale/gitlab.pot                             |  6 ++
 12 files changed, 233 insertions(+), 7 deletions(-)
 create mode 100644 ee/app/assets/javascripts/approvals/components/hidden_groups_item.vue
 create mode 100644 ee/changelogs/unreleased/10356-private-group-handling.yml
 create mode 100644 ee/spec/javascripts/approvals/components/hidden_groups_item_spec.js

diff --git a/ee/app/assets/javascripts/approvals/components/approvers_list_item.vue b/ee/app/assets/javascripts/approvals/components/approvers_list_item.vue
index 1ad19127ab0e90..c50cf175eb7536 100644
--- a/ee/app/assets/javascripts/approvals/components/approvers_list_item.vue
+++ b/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>
diff --git a/ee/app/assets/javascripts/approvals/components/hidden_groups_item.vue b/ee/app/assets/javascripts/approvals/components/hidden_groups_item.vue
new file mode 100644
index 00000000000000..2a55a5b1004aef
--- /dev/null
+++ b/ee/app/assets/javascripts/approvals/components/hidden_groups_item.vue
@@ -0,0 +1,31 @@
+<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>
diff --git a/ee/app/assets/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs.vue b/ee/app/assets/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs.vue
index 0e7a9a61f11103..5e69193dcbc40a 100644
--- a/ee/app/assets/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs.vue
+++ b/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>
diff --git a/ee/app/assets/javascripts/approvals/components/rule_form.vue b/ee/app/assets/javascripts/approvals/components/rule_form.vue
index 9ff9566b9f20e5..660a12ffd4d13a 100644
--- a/ee/app/assets/javascripts/approvals/components/rule_form.vue
+++ b/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 }] : [],
+          ),
       };
     },
   },
diff --git a/ee/app/assets/javascripts/approvals/constants.js b/ee/app/assets/javascripts/approvals/constants.js
index bd8532e23d6c54..d5f7c5eb016c86 100644
--- a/ee/app/assets/javascripts/approvals/constants.js
+++ b/ee/app/assets/javascripts/approvals/constants.js
@@ -1,5 +1,6 @@
 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';
diff --git a/ee/app/assets/javascripts/approvals/mappers.js b/ee/app/assets/javascripts/approvals/mappers.js
index 27b9c6ed04352a..3978fdb62481fc 100644
--- a/ee/app/assets/javascripts/approvals/mappers.js
+++ b/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,
 });
diff --git a/ee/changelogs/unreleased/10356-private-group-handling.yml b/ee/changelogs/unreleased/10356-private-group-handling.yml
new file mode 100644
index 00000000000000..f6fdf67b4991fb
--- /dev/null
+++ b/ee/changelogs/unreleased/10356-private-group-handling.yml
@@ -0,0 +1,5 @@
+---
+title: Fix project approval rule with only private group being considered as approved when override is allowed
+merge_request: 10356
+author:
+type: fixed
diff --git a/ee/spec/javascripts/approvals/components/approvers_list_item_spec.js b/ee/spec/javascripts/approvals/components/approvers_list_item_spec.js
index 33bf799c912c09..7d0e4ca966a530 100644
--- a/ee/spec/javascripts/approvals/components/approvers_list_item_spec.js
+++ b/ee/spec/javascripts/approvals/components/approvers_list_item_spec.js
@@ -1,8 +1,9 @@
 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);
+    });
   });
 });
diff --git a/ee/spec/javascripts/approvals/components/hidden_groups_item_spec.js b/ee/spec/javascripts/approvals/components/hidden_groups_item_spec.js
new file mode 100644
index 00000000000000..41b17d02217eea
--- /dev/null
+++ b/ee/spec/javascripts/approvals/components/hidden_groups_item_spec.js
@@ -0,0 +1,27 @@
+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);
+  });
+});
diff --git a/ee/spec/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs_spec.js b/ee/spec/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs_spec.js
index 75fd2e8815c0a7..2b5e1d1c1127f3 100644
--- a/ee/spec/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs_spec.js
+++ b/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',
+          });
+        });
+      });
     });
   });
 });
diff --git a/ee/spec/javascripts/approvals/components/rule_form_spec.js b/ee/spec/javascripts/approvals/components/rule_form_spec.js
index 67ee1fecb7f813..e5cce74d45e2e8 100644
--- a/ee/spec/javascripts/approvals/components/rule_form_spec.js
+++ b/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', () => {
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index a994ce13c9c4fc..1093c2d04cfad8 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -7236,6 +7236,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 ""
 
@@ -7770,6 +7773,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 ""
 
-- 
GitLab


From d1c4a85778cacdd48fe16c44e6c25b92574bf66b Mon Sep 17 00:00:00 2001
From: Paul Slaughter <pslaughter@gitlab.com>
Date: Tue, 26 Mar 2019 23:31:01 -0500
Subject: [PATCH 5/7] Add feature spec for private group approval rules

Rule should display user under private group
---
 .../javascripts/approvals/components/app.vue  |  2 +-
 .../user_sets_approval_rules_spec.rb          | 53 +++++++++++++++++++
 2 files changed, 54 insertions(+), 1 deletion(-)
 create mode 100644 ee/spec/features/merge_request/user_sets_approval_rules_spec.rb

diff --git a/ee/app/assets/javascripts/approvals/components/app.vue b/ee/app/assets/javascripts/approvals/components/app.vue
index d76487e3cd41e6..0b934d9cbfbebc 100644
--- a/ee/app/assets/javascripts/approvals/components/app.vue
+++ b/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">
diff --git a/ee/spec/features/merge_request/user_sets_approval_rules_spec.rb b/ee/spec/features/merge_request/user_sets_approval_rules_spec.rb
new file mode 100644
index 00000000000000..9169a5ae41622e
--- /dev/null
+++ b/ee/spec/features/merge_request/user_sets_approval_rules_spec.rb
@@ -0,0 +1,53 @@
+# 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
-- 
GitLab


From f7174094f3e865166e33cfe663185771c93ec70b Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Thu, 28 Mar 2019 10:39:06 +0800
Subject: [PATCH 6/7] Update params filter

---
 .../approval_rules/params_filtering_service.rb    | 15 ++++++++-------
 .../params_filtering_service_spec.rb              |  2 +-
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/ee/app/services/approval_rules/params_filtering_service.rb b/ee/app/services/approval_rules/params_filtering_service.rb
index 12b0ec0aa0ae8a..7bf79874eb3718 100644
--- a/ee/app/services/approval_rules/params_filtering_service.rb
+++ b/ee/app/services/approval_rules/params_filtering_service.rb
@@ -4,8 +4,8 @@
 # @params params [Hash] for updating or creating target
 # @params user [User] current user
 #
-# To modify `params` in-place per rule
-# filtering users and groups based on accessibility from 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
@@ -15,17 +15,19 @@ class ParamsFilteringService
     def initialize(target, user, params)
       @target = target
       @current_user = user
-      @params = params
+      @params = params.with_indifferent_access
 
       batch_load_visible_user_and_group_ids
     end
 
     def execute
-      return unless params.key?(:approval_rules_attributes)
+      return params unless params.key?(:approval_rules_attributes)
 
       params[:approval_rules_attributes].each do |rule_attributes|
         handle_rule(rule_attributes)
       end
+
+      params
     end
 
     private
@@ -95,14 +97,13 @@ def find_hidden_group_sourcing_rule(rule_attributes)
 
       return if rule_id.blank?
 
-      rule_id = rule_id.to_i
-      hidden_group_sourcing_rules.find { |rule| rule.id == rule_id }
+      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) # rubocop: disable CodeReuse/ActiveRecord
+        source.approval_rules.includes(:groups).index_by(&:id) # rubocop: disable CodeReuse/ActiveRecord
       end
     end
   end
diff --git a/ee/spec/services/approval_rules/params_filtering_service_spec.rb b/ee/spec/services/approval_rules/params_filtering_service_spec.rb
index 5e81e830b0150a..10d33d6d6a0446 100644
--- a/ee/spec/services/approval_rules/params_filtering_service_spec.rb
+++ b/ee/spec/services/approval_rules/params_filtering_service_spec.rb
@@ -21,7 +21,7 @@
       end
 
       it 'only assigns eligible users and groups' do
-        service.execute
+        params = service.execute
 
         rule1 = params[:approval_rules_attributes].first
 
-- 
GitLab


From 4b45bf28d278625e369edbfb4a0ccaf47f2c5eea Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Thu, 28 Mar 2019 10:39:31 +0800
Subject: [PATCH 7/7] Update MR base service

---
 ee/app/services/ee/merge_requests/base_service.rb       | 2 +-
 ee/spec/services/ee/merge_requests/base_service_spec.rb | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ee/app/services/ee/merge_requests/base_service.rb b/ee/app/services/ee/merge_requests/base_service.rb
index 56d2cb68bfe08a..3e9b7a7abea1c7 100644
--- a/ee/app/services/ee/merge_requests/base_service.rb
+++ b/ee/app/services/ee/merge_requests/base_service.rb
@@ -12,7 +12,7 @@ def filter_params(merge_request)
           params.delete(:approver_group_ids)
         end
 
-        ApprovalRules::ParamsFilteringService.new(merge_request, current_user, params).execute
+        self.params = ApprovalRules::ParamsFilteringService.new(merge_request, current_user, params).execute
 
         super
       end
diff --git a/ee/spec/services/ee/merge_requests/base_service_spec.rb b/ee/spec/services/ee/merge_requests/base_service_spec.rb
index dcdcf9486bb832..35731bd383b7aa 100644
--- a/ee/spec/services/ee/merge_requests/base_service_spec.rb
+++ b/ee/spec/services/ee/merge_requests/base_service_spec.rb
@@ -28,7 +28,7 @@
           project.owner,
           params
         ).and_return(params_filtering_service)
-        expect(params_filtering_service).to receive(:execute)
+        expect(params_filtering_service).to receive(:execute).and_return(params)
 
         subject.execute
       end
-- 
GitLab