Merge branch '350400-be-add-ability-to-filter-by-provisioned-users' into 'master'

Resolve "[BE] Add ability to filter by provisioned users"

See merge request gitlab-org/gitlab!81804

app/finders/group_members_finder.rb

@@ -60,6 +60,8 @@ class GroupMembersFinder < UnionFinder
       members = members.filter_by_2fa(params[:two_factor])
     end
 
+    members = apply_additional_filters(members)
+
     by_created_at(members)
   end
 
@@ -84,6 +86,11 @@ class GroupMembersFinder < UnionFinder
       raise ArgumentError, "#{(include_relations - RELATIONS).first} #{INVALID_RELATION_TYPE_ERROR_MSG}"
     end
   end
+
+  def apply_additional_filters(members)
+    # overridden in EE to include additional filtering conditions.
+    members
+  end
 end
 
 GroupMembersFinder.prepend_mod_with('GroupMembersFinder')

app/models/application_record.rb

@@ -102,6 +102,10 @@ class ApplicationRecord < ActiveRecord::Base
     where('EXISTS (?)', query.select(1))
   end
 
+  def self.where_not_exists(query)
+    where('NOT EXISTS (?)', query.select(1))
+  end
+
   def self.declarative_enum(enum_mod)
     enum(enum_mod.key => enum_mod.values)
   end

ee/app/controllers/ee/groups/group_members_controller.rb

@@ -82,6 +82,11 @@ module EE
 
         group.all_group_members
       end
+
+      override :filter_params
+      def filter_params
+        super.merge(params.permit(:enterprise))
+      end
     end
   end
 end

ee/app/finders/ee/group_members_finder.rb

@@ -27,4 +27,26 @@ module EE::GroupMembersFinder
 
     super
   end
+
+  override :apply_additional_filters
+  def apply_additional_filters(filtered_members)
+    members = super
+
+    filter_by_enterprise_users(members)
+  end
+
+  private
+
+  def filter_by_enterprise_users(members)
+    filter_by_enterprise_param = ::Gitlab::Utils.to_boolean(params[:enterprise])
+
+    return members if filter_by_enterprise_param.nil? # we require this param to be either `true` or `false`
+    return members unless can_filter_by_enterprise?
+
+    members.filter_by_enterprise_users(filter_by_enterprise_param)
+  end
+
+  def can_filter_by_enterprise?
+    can_manage_members && group.root_ancestor.saml_enabled?
+  end
 end

ee/app/helpers/ee/groups/group_members_helper.rb

@@ -19,7 +19,8 @@ module EE::Groups::GroupMembersHelper
   def group_members_app_data(group, members:, invited:, access_requests:)
     super.merge!({
        can_export_members: can?(current_user, :export_group_memberships, group),
-       export_csv_path: export_csv_group_group_members_path(group)
+       export_csv_path: export_csv_group_group_members_path(group),
+       can_filter_by_enterprise: can?(current_user, :admin_group_member, group) && group.root_ancestor.saml_enabled?
      })
   end
 end

ee/app/models/ee/group_member.rb

@@ -31,6 +31,20 @@ module EE
       def member_of_group?(group, user)
         exists?(group: group, user: user)
       end
+
+      def filter_by_enterprise_users(value)
+        subquery =
+          ::UserDetail.where(
+            ::UserDetail.arel_table[:provisioned_by_group_id].eq(arel_table[:source_id]).and(
+              ::UserDetail.arel_table[:user_id].eq(arel_table[:user_id]))
+          )
+
+        if value
+          where_exists(subquery)
+        else
+          where_not_exists(subquery)
+        end
+      end
     end
 
     def provisioned_by_this_group?

ee/spec/finders/ee/group_members_finder_spec.rb

@@ -21,8 +21,9 @@ RSpec.describe GroupMembersFinder do
   end
 
   describe '#execute' do
+    context 'minimal access' do
       let_it_be(:group_minimal_access_membership) do
-      create(:group_member, :minimal_access, source: group, user: create(:user))
+        create(:group_member, :minimal_access, source: group)
       end
 
       context 'when group does not allow minimal access members' do
@@ -50,4 +51,103 @@ RSpec.describe GroupMembersFinder do
         end
       end
     end
+
+    context 'filter by enterprise users' do
+      let_it_be(:saml_provider) { create(:saml_provider, group: group) }
+      let_it_be(:enterprise_member_1_of_root_group) { group.add_developer(create(:user, provisioned_by_group_id: group.id)) }
+      let_it_be(:enterprise_member_2_of_root_group) { group.add_developer(create(:user, provisioned_by_group_id: group.id)) }
+
+      let(:all_members) do
+        [
+          group_owner_membership,
+          group_member_membership,
+          dedicated_member_account_membership,
+          enterprise_member_1_of_root_group,
+          enterprise_member_2_of_root_group
+        ]
+      end
+
+      context 'the group has SAML enabled' do
+        context 'when requested by owner' do
+          let(:current_user) { group_owner_membership.user }
+
+          context 'direct members of the group' do
+            it 'returns Enterprise members when the filter is `true`' do
+              result = described_class.new(group, current_user, params: { enterprise: 'true' }).execute
+
+              expect(result.to_a).to match_array([enterprise_member_1_of_root_group, enterprise_member_2_of_root_group])
+            end
+
+            it 'returns members that are not Enterprise members when the filter is `false`' do
+              result = described_class.new(group, current_user, params: { enterprise: 'false' }).execute
+
+              expect(result.to_a).to match_array([group_owner_membership, group_member_membership, dedicated_member_account_membership])
+            end
+
+            it 'returns all members when the filter is not specified' do
+              result = described_class.new(group, current_user, params: {}).execute
+
+              expect(result.to_a).to match_array(all_members)
+            end
+
+            it 'returns all members when the filter is not either of `true` or `false`' do
+              result = described_class.new(group, current_user, params: { enterprise: 'not-valid' }).execute
+
+              expect(result.to_a).to match_array(all_members)
+            end
+          end
+
+          context 'inherited members of the group' do
+            let_it_be(:subgroup) { create(:group, parent: group) }
+            let_it_be(:subgroup_member_membership) { subgroup.add_developer(create(:user)) }
+
+            it 'returns all members including inherited members, that are Enterprise members, when the filter is `true`' do
+              result = described_class.new(subgroup, current_user, params: { enterprise: 'true' }).execute
+
+              expect(result.to_a).to match_array([enterprise_member_1_of_root_group, enterprise_member_2_of_root_group])
+            end
+
+            it 'returns all members including inherited members, that are not Enterprise members, when the filter is `false`' do
+              result = described_class.new(subgroup, current_user, params: { enterprise: 'false' }).execute
+
+              expect(result.to_a).to match_array(
+                [
+                  group_owner_membership,
+                  group_member_membership,
+                  dedicated_member_account_membership,
+                  subgroup_member_membership
+                ]
+              )
+            end
+          end
+        end
+
+        context 'when requested by non-owner' do
+          let(:current_user) { group_member_membership.user }
+
+          it 'returns all members, as non-owners do not have the ability to filter by Enterprise users' do
+            result = described_class.new(group, current_user, params: { enterprise: 'true' }).execute
+
+            expect(result.to_a).to match_array(all_members)
+          end
+        end
+      end
+
+      context 'the group does not have SAML enabled' do
+        before do
+          group.saml_provider.destroy!
+        end
+
+        context 'when requested by owner' do
+          let(:current_user) { group_owner_membership.user }
+
+          it 'returns all members, because `Enterprise` filter can only be applied on groups that have SAML enabled' do
+            result = described_class.new(group, current_user, params: { enterprise: 'true' }).execute
+
+            expect(result.to_a).to match_array(all_members)
+          end
+        end
+      end
+    end
+  end
 end

ee/spec/helpers/ee/groups/group_members_helper_spec.rb

@@ -50,5 +50,10 @@ RSpec.describe Groups::GroupMembersHelper do
     it 'adds `export_csv_path`' do
       expect(subject[:export_csv_path]).not_to be_nil
     end
+
+    it 'adds `can_filter_by_enterprise`' do
+      allow(group.root_ancestor).to receive(:saml_enabled?).and_return(true)
+      expect(subject[:can_filter_by_enterprise]).to eq(true)
+    end
   end
 end

ee/spec/models/group_member_spec.rb

@@ -105,6 +105,25 @@ RSpec.describe GroupMember do
     end
   end
 
+  describe '.filter_by_enterprise_users' do
+    let_it_be(:group) { create(:group) }
+    let_it_be(:provisioned_member_1_of_group) { group.add_developer(create(:user, provisioned_by_group_id: group.id)) }
+    let_it_be(:provisioned_member_2_of_group) { group.add_developer(create(:user, provisioned_by_group_id: group.id)) }
+    let_it_be(:normal_group_member) { group.add_developer(create(:user)) }
+
+    it 'returns members that are provisioned by a group when the filter is `true`' do
+      result = described_class.filter_by_enterprise_users(true)
+
+      expect(result.to_a).to match_array([provisioned_member_1_of_group, provisioned_member_2_of_group])
+    end
+
+    it 'returns members that are not provisioned by a group when the filter is `false`' do
+      result = described_class.filter_by_enterprise_users(false)
+
+      expect(result.to_a).to match_array([normal_group_member])
+    end
+  end
+
   context 'refreshing project_authorizations' do
     let_it_be_with_refind(:group) { create(:group) }
     let_it_be_with_refind(:user) { create(:user) }

spec/models/application_record_spec.rb

@@ -104,6 +104,18 @@ RSpec.describe ApplicationRecord do
     end
   end
 
+  describe '.where_not_exists' do
+    it 'produces a WHERE NOT EXISTS query' do
+      create(:user, :two_factor_via_u2f)
+      user_2 = create(:user)
+
+      expect(
+        User.where_not_exists(
+          U2fRegistration.where(U2fRegistration.arel_table[:user_id].eq(User.arel_table[:id])))
+      ).to match_array([user_2])
+    end
+  end
+
   describe '.transaction', :delete do
     it 'opens a new transaction' do
       expect(described_class.connection.transaction_open?).to be false