Merge branch 'jm-searchable-users' into 'master'

Only admins can search blocked, banned and unconfirmed users

See merge request gitlab-org/gitlab!84080

app/finders/users_finder.rb

@@ -55,7 +55,8 @@ class UsersFinder
   private
 
   def base_scope
-    User.all.order_id_desc
+    scope = current_user&.admin? ? User.all : User.without_forbidden_states
+    scope.order_id_desc
   end
 
   def by_username(users)

app/models/user.rb

@@ -46,6 +46,8 @@ class User < ApplicationRecord
     :public_email
   ].freeze
 
+  FORBIDDEN_SEARCH_STATES = %w(blocked banned ldap_blocked).freeze
+
   add_authentication_token_field :incoming_email_token, token_generator: -> { SecureRandom.hex.to_i(16).to_s(36) }
   add_authentication_token_field :feed_token
   add_authentication_token_field :static_object_token, encrypted: :optional
@@ -469,6 +471,7 @@ class User < ApplicationRecord
   scope :with_no_activity, -> { with_state(:active).human_or_service_user.where(last_activity_on: nil) }
   scope :by_provider_and_extern_uid, ->(provider, extern_uid) { joins(:identities).merge(Identity.with_extern_uid(provider, extern_uid)) }
   scope :by_ids_or_usernames, -> (ids, usernames) { where(username: usernames).or(where(id: ids)) }
+  scope :without_forbidden_states, -> { confirmed.where.not(state: FORBIDDEN_SEARCH_STATES) }
 
   strip_attributes! :name
 

db/post_migrate/20220404183350_add_forbidden_state_index_to_users.rb

+# frozen_string_literal: true
+
+class AddForbiddenStateIndexToUsers < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  INDEX_NAME = 'users_forbidden_state_idx'
+
+  def up
+    add_concurrent_index :users, :id,
+      name: INDEX_NAME,
+      where: "confirmed_at IS NOT NULL AND (state <> ALL (ARRAY['blocked', 'banned', 'ldap_blocked']))"
+  end
+
+  def down
+    remove_concurrent_index_by_name :users, INDEX_NAME
+  end
+end

db/schema_migrations/20220404183350

+fcf7a6569afb7fdb95834179df5632ad14165d27476eb020e9db07e504f75f32
\ No newline at end of file

db/structure.sql

@@ -29680,6 +29680,8 @@ CREATE UNIQUE INDEX unique_merge_request_metrics_by_merge_request_id ON merge_re
 
 CREATE INDEX user_follow_users_followee_id_idx ON user_follow_users USING btree (followee_id);
 
+CREATE INDEX users_forbidden_state_idx ON users USING btree (id) WHERE ((confirmed_at IS NOT NULL) AND ((state)::text <> ALL (ARRAY['blocked'::text, 'banned'::text, 'ldap_blocked'::text])));
+
 CREATE UNIQUE INDEX vulnerability_feedback_unique_idx ON vulnerability_feedback USING btree (project_id, category, feedback_type, project_fingerprint);
 
 CREATE UNIQUE INDEX vulnerability_occurrence_pipelines_on_unique_keys ON vulnerability_occurrence_pipelines USING btree (occurrence_id, pipeline_id);

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, external_user, ldap_user, internal_user, admin_user)
+          expect(users).to contain_exactly(normal_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, external_user, internal_user, admin_user)
+          expect(users).to contain_exactly(normal_user, omniauth_user, external_user, internal_user, admin_user)
         end
       end
 
@@ -36,7 +36,7 @@ RSpec.describe UsersFinder do
         it 'returns all users by default' do
           users = described_class.new(normal_user).execute
 
-          expect(users).to contain_exactly(normal_user, blocked_user, omniauth_user, external_user, internal_user, admin_user, saml_user, non_saml_user)
+          expect(users).to contain_exactly(normal_user, omniauth_user, external_user, internal_user, admin_user, saml_user, non_saml_user)
         end
 
         it 'returns only saml users from the provided saml_provider_id' do
@@ -46,5 +46,46 @@ RSpec.describe UsersFinder do
         end
       end
     end
+
+    context 'with an admin user' do
+      context 'with LDAP users' do
+        let_it_be(:ldap_user) { create(:omniauth_user, provider: 'ldap') }
+
+        it 'returns ldap users by default' do
+          users = described_class.new(admin_user).execute
+
+          expect(users).to contain_exactly(normal_user, blocked_user, unconfirmed_user, banned_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(admin_user, skip_ldap: true).execute
+
+          expect(users).to contain_exactly(normal_user, blocked_user, unconfirmed_user, banned_user, omniauth_user, external_user, internal_user, admin_user)
+        end
+      end
+
+      context 'with SAML users' do
+        let_it_be(:group) { create(:group) }
+        let_it_be(:saml_provider) { create(:saml_provider, group: group, enabled: true, enforced_sso: true) }
+        let_it_be(:saml_user) { create(:user) }
+        let_it_be(:non_saml_user) { create(:user) }
+
+        before do
+          create(:identity, provider: 'group_saml1', saml_provider_id: saml_provider.id, user: saml_user)
+        end
+
+        it 'returns all users by default' do
+          users = described_class.new(admin_user).execute
+
+          expect(users).to contain_exactly(normal_user, blocked_user, unconfirmed_user, banned_user, omniauth_user, external_user, internal_user, admin_user, saml_user, non_saml_user)
+        end
+
+        it 'returns only saml users from the provided saml_provider_id' do
+          users = described_class.new(admin_user, saml_provider_id: saml_provider.id).execute
+
+          expect(users).to contain_exactly(saml_user)
+        end
+      end
+    end
   end
 end

spec/finders/users_finder_spec.rb

@@ -11,10 +11,10 @@ RSpec.describe UsersFinder do
     context 'with a normal user' do
       let_it_be(:user) { create(:user) }
 
-      it 'returns all users' do
+      it 'returns searchable users' do
         users = described_class.new(user).execute
 
-        expect(users).to contain_exactly(user, normal_user, blocked_user, external_user, omniauth_user, internal_user, admin_user, project_bot)
+        expect(users).to contain_exactly(user, normal_user, external_user, omniauth_user, internal_user, admin_user, project_bot)
       end
 
       it 'filters by username' do
@@ -36,9 +36,9 @@ RSpec.describe UsersFinder do
       end
 
       it 'filters by search' do
-        users = described_class.new(user, search: 'orando').execute
+        users = described_class.new(user, search: 'ohndo').execute
 
-        expect(users).to contain_exactly(blocked_user)
+        expect(users).to contain_exactly(normal_user)
       end
 
       it 'does not filter by private emails search' do
@@ -47,18 +47,6 @@ RSpec.describe UsersFinder do
         expect(users).to be_empty
       end
 
-      it 'filters by blocked users' do
-        users = described_class.new(user, blocked: true).execute
-
-        expect(users).to contain_exactly(blocked_user)
-      end
-
-      it 'filters by active users' do
-        users = described_class.new(user, active: true).execute
-
-        expect(users).to contain_exactly(user, normal_user, external_user, omniauth_user, admin_user, project_bot)
-      end
-
       it 'filters by external users' do
         users = described_class.new(user, external: true).execute
 
@@ -68,7 +56,7 @@ RSpec.describe UsersFinder do
       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, project_bot)
+        expect(users).to contain_exactly(user, normal_user, omniauth_user, internal_user, admin_user, project_bot)
       end
 
       it 'filters by created_at' do
@@ -85,13 +73,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, external_user, blocked_user, omniauth_user, admin_user, project_bot)
-      end
-
-      it 'filters by without project bots' do
-        users = described_class.new(user, without_project_bots: true).execute
-
-        expect(users).to contain_exactly(user, normal_user, external_user, blocked_user, omniauth_user, internal_user, admin_user)
+        expect(users).to contain_exactly(user, normal_user, external_user, omniauth_user, admin_user, project_bot)
       end
 
       it 'does not filter by custom attributes' do
@@ -100,18 +82,18 @@ RSpec.describe UsersFinder do
           custom_attributes: { foo: 'bar' }
         ).execute
 
-        expect(users).to contain_exactly(user, normal_user, blocked_user, external_user, omniauth_user, internal_user, admin_user, project_bot)
+        expect(users).to contain_exactly(user, normal_user, external_user, omniauth_user, internal_user, admin_user, project_bot)
       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, external_user, omniauth_user, internal_user, project_bot, user])
+        expect(users).to eq([normal_user, admin_user, external_user, omniauth_user, internal_user, project_bot, 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, external_user, admin_user, blocked_user, omniauth_user, internal_user, project_bot)
+        expect(users).to contain_exactly(user, normal_user, external_user, admin_user, omniauth_user, internal_user, project_bot)
       end
     end
 
@@ -127,7 +109,19 @@ RSpec.describe UsersFinder do
       it 'returns all users' do
         users = described_class.new(admin).execute
 
-        expect(users).to contain_exactly(admin, normal_user, blocked_user, external_user, omniauth_user, internal_user, admin_user, project_bot)
+        expect(users).to contain_exactly(admin, normal_user, blocked_user, unconfirmed_user, banned_user, external_user, omniauth_user, internal_user, admin_user, project_bot)
+      end
+
+      it 'filters by blocked users' do
+        users = described_class.new(admin, blocked: true).execute
+
+        expect(users).to contain_exactly(blocked_user)
+      end
+
+      it 'filters by active users' do
+        users = described_class.new(admin, active: true).execute
+
+        expect(users).to contain_exactly(admin, normal_user, unconfirmed_user, external_user, omniauth_user, admin_user, project_bot)
       end
 
       it 'returns only admins' do

spec/models/user_spec.rb

@@ -6642,6 +6642,23 @@ RSpec.describe User do
     end
   end
 
+  describe '.without_forbidden_states' do
+    let_it_be(:normal_user) { create(:user, username: 'johndoe') }
+    let_it_be(:admin_user) { create(:user, :admin, username: 'iamadmin') }
+    let_it_be(:blocked_user) { create(:user, :blocked, username: 'notsorandom') }
+    let_it_be(:banned_user) { create(:user, :banned, username: 'iambanned') }
+    let_it_be(:external_user) { create(:user, :external) }
+    let_it_be(:unconfirmed_user) { create(:user, confirmed_at: nil) }
+    let_it_be(:omniauth_user) { create(:omniauth_user, provider: 'twitter', extern_uid: '123456') }
+    let_it_be(:internal_user) { User.alert_bot.tap { |u| u.confirm } }
+
+    it 'does not return blocked, banned or unconfirmed users' do
+      expect(described_class.without_forbidden_states).to match_array([
+        normal_user, admin_user, external_user, omniauth_user, internal_user
+      ])
+    end
+  end
+
   describe 'user_project' do
     it 'returns users project matched by username and public visibility' do
       user = create(:user)

spec/requests/api/users_spec.rb

@@ -338,12 +338,14 @@ RSpec.describe API::Users do
         expect(response).to match_response_schema('public_api/v4/user/basics')
         expect(json_response.first.keys).not_to include 'is_admin'
       end
+    end
 
+    context "when admin" do
       context 'exclude_internal param' do
         let_it_be(:internal_user) { User.alert_bot }
 
         it 'returns all users when it is not set' do
-          get api("/users?exclude_internal=false", user)
+          get api("/users?exclude_internal=false", admin)
 
           expect(response).to match_response_schema('public_api/v4/user/basics')
           expect(response).to include_pagination_headers

spec/support/shared_contexts/finders/users_finder_shared_contexts.rb

@@ -3,8 +3,10 @@
 RSpec.shared_context 'UsersFinder#execute filter by project context' do
   let_it_be(:normal_user) { create(:user, username: 'johndoe') }
   let_it_be(:admin_user) { create(:user, :admin, username: 'iamadmin') }
+  let_it_be(:banned_user) { create(:user, :banned, username: 'iambanned') }
   let_it_be(:blocked_user) { create(:user, :blocked, username: 'notsorandom') }
   let_it_be(:external_user) { create(:user, :external) }
+  let_it_be(:unconfirmed_user) { create(:user, confirmed_at: nil) }
   let_it_be(:omniauth_user) { create(:omniauth_user, provider: 'twitter', extern_uid: '123456') }
-  let_it_be(:internal_user) { User.alert_bot }
+  let_it_be(:internal_user) { User.alert_bot.tap { |u| u.confirm } }
 end