Merge branch 'fix/include-external-users-in-user-search' into 'master'

Include external users in user search

See merge request gitlab-org/gitlab!53584

app/finders/users_finder.rb

@@ -14,6 +14,7 @@
 #     active: boolean
 #     blocked: boolean
 #     external: boolean
+#     non_external: boolean
 #     without_projects: boolean
 #     sort: string
 #     id: integer
@@ -40,6 +41,7 @@ class UsersFinder
     users = by_active(users)
     users = by_external_identity(users)
     users = by_external(users)
+    users = by_non_external(users)
     users = by_2fa(users)
     users = by_created_at(users)
     users = by_without_projects(users)
@@ -97,13 +99,18 @@ class UsersFinder
 
   # rubocop: disable CodeReuse/ActiveRecord
   def by_external(users)
-    return users = users.where.not(external: true) unless current_user&.admin?
     return users unless params[:external]
 
     users.external
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
+  def by_non_external(users)
+    return users unless params[:non_external]
+
+    users.non_external
+  end
+
   def by_2fa(users)
     case params[:two_factor]
     when 'enabled'

app/models/user.rb

@@ -360,6 +360,7 @@ class User < ApplicationRecord
   scope :blocked, -> { with_states(:blocked, :ldap_blocked) }
   scope :blocked_pending_approval, -> { with_states(:blocked_pending_approval) }
   scope :external, -> { where(external: true) }
+  scope :non_external, -> { where(external: false) }
   scope :confirmed, -> { where.not(confirmed_at: nil) }
   scope :active, -> { with_state(:active).non_internal }
   scope :active_without_ghosts, -> { with_state(:active).without_ghosts }

changelogs/unreleased/fix-include-external-users-in-user-search.yml

+---
+title: 'API: include external users in user search for non-admins'
+merge_request: 53584
+author: Jonas Wälter @wwwjon
+type: changed

doc/api/users.md

@@ -53,6 +53,9 @@ For example:
 GET /users?username=jack_smith
 ```
 
+NOTE:
+Username search is case insensitive.
+
 In addition, you can filter users based on the states `blocked` and `active`.
 It does not support `active=false` or `blocked=false`. The list of billable users
 is the total number of users minus the blocked users.
@@ -65,6 +68,13 @@ GET /users?active=true
 GET /users?blocked=true
 ```
 
+In addition, you can search for external users only with `external=true`.
+It does not support `external=false`.
+
+```plaintext
+GET /users?external=true
+```
+
 GitLab supports bot users such as the [alert bot](../operations/incident_management/integrations.md)
 or the [support bot](../user/project/service_desk.md#support-bot-user).
 You can exclude the following types of [internal users](../development/internal_users.md#internal-users)
@@ -80,8 +90,11 @@ However, this action does not exclude [project bot users](../user/project/settin
 GET /users?exclude_internal=true
 ```
 
-NOTE:
-Username search is case insensitive.
+In addition, to exclude external users from the users' list, you can use the parameter `exclude_external=true`.
+
+```plaintext
+GET /users?exclude_external=true
+```
 
 ### For admins
 
@@ -223,10 +236,6 @@ For example:
 GET /users?extern_uid=1234567&provider=github
 ```
 
-Instance administrators can search for users who are external with: `/users?external=true`
-
-You cannot search for external users if you are not an instance administrator. 
-
 You can search users by creation date time range with:
 
 ```plaintext

ee/spec/finders/users_finder_spec.rb

@@ -13,13 +13,13 @@ RSpec.describe UsersFinder do
         it 'returns ldap users by default' do
           users = described_class.new(normal_user).execute
 
-          expect(users).to contain_exactly(normal_user, blocked_user, omniauth_user, ldap_user, internal_user, admin_user)
+          expect(users).to contain_exactly(normal_user, blocked_user, omniauth_user, external_user, ldap_user, internal_user, admin_user)
         end
 
         it 'returns only non-ldap users with skip_ldap: true' do
           users = described_class.new(normal_user, skip_ldap: true).execute
 
-          expect(users).to contain_exactly(normal_user, blocked_user, omniauth_user, internal_user, admin_user)
+          expect(users).to contain_exactly(normal_user, blocked_user, omniauth_user, external_user, internal_user, admin_user)
         end
       end
     end

lib/api/users.rb

@@ -82,6 +82,7 @@ module API
         optional :search, type: String, desc: 'Search for a username'
         optional :active, type: Boolean, default: false, desc: 'Filters only active users'
         optional :external, type: Boolean, default: false, desc: 'Filters only external users'
+        optional :exclude_external, as: :non_external, type: Boolean, default: false, desc: 'Filters only non external users'
         optional :blocked, type: Boolean, default: false, desc: 'Filters only blocked users'
         optional :created_after, type: DateTime, desc: 'Return users created after the specified time'
         optional :created_before, type: DateTime, desc: 'Return users created before the specified time'
@@ -97,7 +98,7 @@ module API
       end
       # rubocop: disable CodeReuse/ActiveRecord
       get feature_category: :users do
-        authenticated_as_admin! if params[:external].present? || (params[:extern_uid].present? && params[:provider].present?)
+        authenticated_as_admin! if params[:extern_uid].present? && params[:provider].present?
 
         unless current_user&.admin?
           params.except!(:created_after, :created_before, :order_by, :sort, :two_factor, :without_projects)

spec/finders/users_finder_spec.rb

@@ -12,7 +12,7 @@ RSpec.describe UsersFinder do
       it 'returns all users' do
         users = described_class.new(user).execute
 
-        expect(users).to contain_exactly(user, normal_user, blocked_user, omniauth_user, internal_user, admin_user)
+        expect(users).to contain_exactly(user, normal_user, blocked_user, external_user, omniauth_user, internal_user, admin_user)
       end
 
       it 'filters by username' do
@@ -48,12 +48,18 @@ RSpec.describe UsersFinder do
       it 'filters by active users' do
         users = described_class.new(user, active: true).execute
 
-        expect(users).to contain_exactly(user, normal_user, omniauth_user, admin_user)
+        expect(users).to contain_exactly(user, normal_user, external_user, omniauth_user, admin_user)
       end
 
-      it 'returns no external users' do
+      it 'filters by external users' do
         users = described_class.new(user, external: true).execute
 
+        expect(users).to contain_exactly(external_user)
+      end
+
+      it 'filters by non external users' do
+        users = described_class.new(user, non_external: true).execute
+
         expect(users).to contain_exactly(user, normal_user, blocked_user, omniauth_user, internal_user, admin_user)
       end
 
@@ -71,7 +77,7 @@ RSpec.describe UsersFinder do
       it 'filters by non internal users' do
         users = described_class.new(user, non_internal: true).execute
 
-        expect(users).to contain_exactly(user, normal_user, blocked_user, omniauth_user, admin_user)
+        expect(users).to contain_exactly(user, normal_user, external_user, blocked_user, omniauth_user, admin_user)
       end
 
       it 'does not filter by custom attributes' do
@@ -80,18 +86,18 @@ RSpec.describe UsersFinder do
           custom_attributes: { foo: 'bar' }
         ).execute
 
-        expect(users).to contain_exactly(user, normal_user, blocked_user, omniauth_user, internal_user, admin_user)
+        expect(users).to contain_exactly(user, normal_user, blocked_user, external_user, omniauth_user, internal_user, admin_user)
       end
 
       it 'orders returned results' do
         users = described_class.new(user, sort: 'id_asc').execute
 
-        expect(users).to eq([normal_user, admin_user, blocked_user, omniauth_user, internal_user, user])
+        expect(users).to eq([normal_user, admin_user, blocked_user, external_user, omniauth_user, internal_user, user])
       end
 
       it 'does not filter by admins' do
         users = described_class.new(user, admins: true).execute
-        expect(users).to contain_exactly(user, normal_user, admin_user, blocked_user, omniauth_user, internal_user)
+        expect(users).to contain_exactly(user, normal_user, external_user, admin_user, blocked_user, omniauth_user, internal_user)
       end
     end
 

spec/requests/api/users_spec.rb

@@ -320,6 +320,18 @@ RSpec.describe API::Users do
         expect(json_response).to all(include('state' => /(blocked|ldap_blocked)/))
       end
 
+      it "returns an array of external users" do
+        create(:user)
+        external_user = create(:user, external: true)
+
+        get api("/users?external=true", user)
+
+        expect(response).to match_response_schema('public_api/v4/user/basics')
+        expect(response).to include_pagination_headers
+        expect(json_response.size).to eq(1)
+        expect(json_response[0]['id']).to eq(external_user.id)
+      end
+
       it "returns one user" do
         get api("/users?username=#{omniauth_user.username}", user)