From 8217c1ac948c9022805f61d3fdd43ff2d527dd21 Mon Sep 17 00:00:00 2001
From: Madelein van Niekerk <mvanniekerk@gitlab.com>
Date: Wed, 12 Mar 2025 20:21:11 +0200
Subject: [PATCH] Add group and project scope for basic users autocomplete
Changelog: changed
---
app/finders/users_finder.rb | 44 +++++
app/models/member.rb | 2 +
..._to_authorized_namespaces_basic_search.yml | 9 +
...horized_namespaces_basic_search_by_ids.yml | 9 +
lib/gitlab/search_results.rb | 27 ++-
spec/finders/users_finder_spec.rb | 71 ++++++++
spec/helpers/search_helper_spec.rb | 24 +++
spec/lib/gitlab/search_results_spec.rb | 158 +++++++++++++++++-
8 files changed, 338 insertions(+), 6 deletions(-)
create mode 100644 config/feature_flags/gitlab_com_derisk/users_search_scoped_to_authorized_namespaces_basic_search.yml
create mode 100644 config/feature_flags/gitlab_com_derisk/users_search_scoped_to_authorized_namespaces_basic_search_by_ids.yml
diff --git a/app/finders/users_finder.rb b/app/finders/users_finder.rb
index d0ed3515bfd66..d2b4b18f9207f 100644
--- a/app/finders/users_finder.rb
+++ b/app/finders/users_finder.rb
@@ -55,6 +55,8 @@ def execute
users = by_custom_attributes(users)
users = by_non_internal(users)
users = by_without_project_bots(users)
+ users = by_membership(users)
+ users = by_member_source_ids(users)
order(users)
end
@@ -180,6 +182,48 @@ def by_without_project_bots(users)
users.without_project_bot
end
+ def by_membership(users)
+ return users unless params[:by_membership]
+
+ group_members = Member
+ .non_request
+ .with_source(current_user.authorized_groups.self_and_ancestors)
+ .select(:user_id)
+ .to_sql
+
+ project_members = Member
+ .non_request
+ .with_source(current_user.authorized_projects)
+ .select(:user_id)
+ .to_sql
+
+ query = "users.id IN (#{group_members} UNION #{project_members})"
+ users.where(query) # rubocop: disable CodeReuse/ActiveRecord -- finder
+ end
+
+ def by_member_source_ids(users)
+ group_member_source_ids = params[:group_member_source_ids]
+ project_member_source_ids = params[:project_member_source_ids]
+
+ return users unless group_member_source_ids || project_member_source_ids
+
+ member_queries = []
+
+ if group_member_source_ids.present?
+ member_queries << Member.with_source_id(group_member_source_ids).with_source_type('Namespace')
+ end
+
+ if project_member_source_ids.present?
+ member_queries << Member.with_source_id(project_member_source_ids).with_source_type('Project')
+ end
+
+ return users if member_queries.empty?
+
+ member_query = member_queries.reduce(:or).non_request
+
+ users.id_in(member_query.select(:user_id))
+ end
+
def order(users)
return users unless params[:sort]
diff --git a/app/models/member.rb b/app/models/member.rb
index deffb3768a7ad..614315657e03a 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -209,6 +209,8 @@ class Member < ApplicationRecord
end
scope :with_source_id, ->(source_id) { where(source_id: source_id) }
+ scope :with_source, ->(source) { where(source: source) }
+ scope :with_source_type, ->(source_type) { where(source_type: source_type) }
scope :including_source, -> { includes(:source) }
scope :including_user, -> { includes(:user) }
diff --git a/config/feature_flags/gitlab_com_derisk/users_search_scoped_to_authorized_namespaces_basic_search.yml b/config/feature_flags/gitlab_com_derisk/users_search_scoped_to_authorized_namespaces_basic_search.yml
new file mode 100644
index 0000000000000..b7c9a64f67675
--- /dev/null
+++ b/config/feature_flags/gitlab_com_derisk/users_search_scoped_to_authorized_namespaces_basic_search.yml
@@ -0,0 +1,9 @@
+---
+name: users_search_scoped_to_authorized_namespaces_basic_search
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/442091
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/182557
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/520710
+milestone: '17.10'
+group: group::global search
+type: gitlab_com_derisk
+default_enabled: false
diff --git a/config/feature_flags/gitlab_com_derisk/users_search_scoped_to_authorized_namespaces_basic_search_by_ids.yml b/config/feature_flags/gitlab_com_derisk/users_search_scoped_to_authorized_namespaces_basic_search_by_ids.yml
new file mode 100644
index 0000000000000..fd4fbcbaa7c40
--- /dev/null
+++ b/config/feature_flags/gitlab_com_derisk/users_search_scoped_to_authorized_namespaces_basic_search_by_ids.yml
@@ -0,0 +1,9 @@
+---
+name: users_search_scoped_to_authorized_namespaces_basic_search_by_ids
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/442091
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/182557
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/524297
+milestone: '17.10'
+group: group::global search
+type: gitlab_com_derisk
+default_enabled: false
diff --git a/lib/gitlab/search_results.rb b/lib/gitlab/search_results.rb
index b94184cfe8c6d..c795c12b157eb 100644
--- a/lib/gitlab/search_results.rb
+++ b/lib/gitlab/search_results.rb
@@ -109,7 +109,18 @@ def count_limit
def users
return User.none unless Ability.allowed?(current_user, :read_users_list)
- UsersFinder.new(current_user, { search: query, use_minimum_char_limit: false }).execute
+ params = { search: query, use_minimum_char_limit: false }
+
+ if current_user && filters[:autocomplete]
+ if Feature.enabled?(:users_search_scoped_to_authorized_namespaces_basic_search, current_user)
+ params[:by_membership] = true
+ elsif Feature.enabled?(:users_search_scoped_to_authorized_namespaces_basic_search_by_ids, current_user)
+ params[:group_member_source_ids] = current_user_authorized_group_ids
+ params[:project_member_source_ids] = current_user_authorized_project_ids
+ end
+ end
+
+ UsersFinder.new(current_user, params).execute
end
# highlighting is only performed by Elasticsearch backed results
@@ -266,6 +277,20 @@ def issuable_params
end
end
+ def current_user_authorized_group_ids
+ GroupsFinder
+ .new(current_user, { all_available: false })
+ .execute
+ .pluck("#{Group.table_name}.#{Group.primary_key}") # rubocop: disable CodeReuse/ActiveRecord -- need to find ids
+ end
+
+ def current_user_authorized_project_ids
+ ProjectsFinder
+ .new(current_user: current_user, params: { non_public: true })
+ .execute
+ .pluck_primary_key
+ end
+
# rubocop: disable CodeReuse/ActiveRecord
def limited_count(relation)
relation.reorder(nil).limit(count_limit).size
diff --git a/spec/finders/users_finder_spec.rb b/spec/finders/users_finder_spec.rb
index a766923a69ac3..770d7d6aeaed7 100644
--- a/spec/finders/users_finder_spec.rb
+++ b/spec/finders/users_finder_spec.rb
@@ -149,6 +149,77 @@
users = described_class.new(user, admins: true).execute
expect(users).to contain_exactly(user, normal_user, external_user, admin_user, unconfirmed_user, omniauth_user, internal_user, project_bot, service_account_user)
end
+
+ context 'when filtering by_membership' do
+ let_it_be(:group_user) { create(:user) }
+ let_it_be(:project_user) { create(:user) }
+ let_it_be(:group) { create(:group, developers: [user]) }
+ let_it_be(:project) { create(:project, developers: [user]) }
+
+ subject(:users) { described_class.new(user, by_membership: true).execute }
+
+ it 'includes the user and project owner' do
+ expect(users).to contain_exactly(user, project.owner)
+ end
+
+ it 'includes users who are members of the user groups' do
+ group.add_developer(group_user)
+
+ expect(users).to contain_exactly(user, project.owner, group_user)
+ end
+
+ it 'includes users who are members of the user projects' do
+ project.add_developer(project_user)
+
+ expect(users).to contain_exactly(user, project.owner, project_user)
+ end
+ end
+
+ context 'when filtering by_member_source_ids' do
+ let_it_be(:group_user) { create(:user) }
+ let_it_be(:project_user) { create(:user) }
+ let_it_be(:group) { create(:group) }
+ let_it_be(:project) { create(:project) }
+
+ it 'filters by group membership' do
+ group.add_developer(group_user)
+
+ users = described_class.new(user, group_member_source_ids: [group.id]).execute
+
+ expect(users).to contain_exactly(group_user)
+ end
+
+ it 'filters by project membership' do
+ project.add_developer(project_user)
+
+ users = described_class.new(user, project_member_source_ids: [project.id]).execute
+
+ expect(users).to contain_exactly(project_user, project.owner)
+ end
+
+ it 'filters by group and project membership' do
+ group.add_developer(group_user)
+ project.add_developer(project_user)
+
+ users = described_class
+ .new(user, group_member_source_ids: [group.id], project_member_source_ids: [project.id])
+ .execute
+
+ expect(users).to contain_exactly(group_user, project_user, project.owner)
+ end
+
+ it 'does not include members not part of the filtered group' do
+ users = described_class.new(user, group_member_source_ids: [group.id]).execute
+
+ expect(users).not_to include(group_user)
+ end
+
+ it 'does not include members not part of the filtered project' do
+ users = described_class.new(user, project_member_source_ids: [project.id]).execute
+
+ expect(users).not_to include(project_user)
+ end
+ end
end
shared_examples 'executes users finder as admin' do
diff --git a/spec/helpers/search_helper_spec.rb b/spec/helpers/search_helper_spec.rb
index 36544a75648d5..782d49f496823 100644
--- a/spec/helpers/search_helper_spec.rb
+++ b/spec/helpers/search_helper_spec.rb
@@ -63,8 +63,10 @@ def simple_sanitize(str)
shared_examples 'for users' do
let_it_be(:another_user) { create(:user, name: 'Jane Doe') }
let(:term) { 'jane' }
+ let_it_be(:project) { create(:project, developers: user) }
it 'returns users matching the term' do
+ project.add_developer(another_user)
result = search_autocomplete_opts(term)
expect(result.size).to eq(1)
expect(result.first[:id]).to eq(another_user.id)
@@ -97,21 +99,29 @@ def simple_sanitize(str)
it 'includes users with matching public emails' do
public_email_user
+ project.add_developer(public_email_user)
+
expect(ids).to include(public_email_user.id)
end
it 'includes users in forbidden states' do
banned_user
+ project.add_developer(banned_user)
+
expect(ids).to include(banned_user.id)
end
it 'includes users without matching public emails but with matching private emails' do
private_email_user
+ project.add_developer(private_email_user)
+
expect(ids).to include(private_email_user.id)
end
it 'includes users matching on secondary email' do
secondary_email
+ project.add_developer(user_with_other_email)
+
expect(ids).to include(secondary_email.user_id)
end
end
@@ -123,21 +133,29 @@ def simple_sanitize(str)
it 'includes users with matching public emails' do
public_email_user
+ project.add_developer(public_email_user)
+
expect(ids).to include(public_email_user.id)
end
it 'does not include users in forbidden states' do
banned_user
+ project.add_developer(banned_user)
+
expect(ids).not_to include(banned_user.id)
end
it 'does not include users without matching public emails but with matching private emails' do
private_email_user
+ project.add_developer(private_email_user)
+
expect(ids).not_to include(private_email_user.id)
end
it 'does not include users matching on secondary email' do
secondary_email
+ project.add_developer(secondary_email)
+
expect(ids).not_to include(secondary_email.user_id)
end
end
@@ -146,6 +164,12 @@ def simple_sanitize(str)
context 'with limiting' do
let_it_be(:users) { create_list(:user, 6, name: 'Jane Doe') }
+ before do
+ users.each do |user|
+ project.add_developer(user)
+ end
+ end
+
it 'only returns the first 5 users' do
result = search_autocomplete_opts(term)
expect(result.size).to eq(5)
diff --git a/spec/lib/gitlab/search_results_spec.rb b/spec/lib/gitlab/search_results_spec.rb
index b15af98e1d9b5..42971c087fc7f 100644
--- a/spec/lib/gitlab/search_results_spec.rb
+++ b/spec/lib/gitlab/search_results_spec.rb
@@ -7,7 +7,7 @@
include SearchHelpers
using RSpec::Parameterized::TableSyntax
- let_it_be(:user) { create(:user) }
+ let_it_be(:user) { create(:user, username: 'foobar') }
let_it_be(:project) { create(:project, name: 'foo') }
let_it_be(:issue) { create(:issue, project: project, title: 'foo') }
let_it_be(:milestone) { create(:milestone, project: project, title: 'foo') }
@@ -313,18 +313,166 @@
end
describe '#users' do
+ subject(:user_search_result) { results.objects('users') }
+
+ let_it_be(:another_user) { create(:user, username: 'barfoo') }
+ let_it_be(:group) { create(:group) }
+
it 'does not call the UsersFinder when the current_user is not allowed to read users list' do
allow(Ability).to receive(:allowed?).and_return(false)
- expect(UsersFinder).not_to receive(:new).with(user, { search: 'foo', use_minimum_char_limit: false }).and_call_original
+ expect(UsersFinder).not_to receive(:new)
- results.objects('users')
+ user_search_result
end
it 'calls the UsersFinder' do
- expect(UsersFinder).to receive(:new).with(user, { search: 'foo', use_minimum_char_limit: false }).and_call_original
+ expected_params = {
+ search: 'foo',
+ use_minimum_char_limit: false
+ }
+
+ expect(UsersFinder).to receive(:new).with(user, expected_params).and_call_original
+
+ user_search_result
+ end
+
+ context 'when the autocomplete filter is added' do
+ let(:filters) { { autocomplete: true } }
+
+ shared_examples 'returns users' do
+ it 'returns the current_user since they match the query' do
+ expect(user_search_result).to match_array(user)
+ end
+
+ context 'when another user belongs to a project the current_user belongs to' do
+ before do
+ project.add_developer(another_user)
+ end
+
+ it 'includes the other user' do
+ expect(user_search_result).to match_array([user, another_user])
+ end
+ end
+
+ context 'when another user belongs to a group' do
+ before do
+ group.add_developer(another_user)
+ end
+
+ it 'does not include the other user' do
+ expect(user_search_result).not_to include(another_user)
+ end
+
+ context 'when the current_user also belongs to that group' do
+ before do
+ group.add_developer(user)
+ end
+
+ it 'includes the other user' do
+ expect(user_search_result).to match_array([user, another_user])
+ end
+ end
+
+ context 'when the current_user belongs to a parent of the group' do
+ let_it_be(:parent_group) { create(:group) }
+ let_it_be(:group) { create(:group, parent: parent_group) }
+
+ before do
+ parent_group.add_developer(user)
+ end
+
+ it 'includes the other user' do
+ expect(user_search_result).to match_array([user, another_user])
+ end
+ end
+
+ context 'when the current_user belongs to a group that is shared by the group' do
+ let_it_be_with_reload(:shared_with_group) { create(:group) }
+ let_it_be_with_reload(:group_group_link) do
+ create(
+ :group_group_link,
+ group_access: ::Gitlab::Access::GUEST,
+ shared_group: group,
+ shared_with_group: shared_with_group
+ )
+ end
+
+ before do
+ shared_with_group.add_developer(user)
+ end
+
+ it 'includes the other user' do
+ expect(user_search_result).to match_array([user, another_user])
+ end
+ end
+
+ context 'when the current_user belongs to a child of the group' do
+ let_it_be(:child_group) { create(:group, parent: group) }
+
+ before do
+ child_group.add_developer(user)
+ end
+
+ it 'includes the other user' do
+ expect(user_search_result).to match_array([user, another_user])
+ end
+ end
+ end
+
+ context 'when another user is a guest of a private group' do
+ let_it_be(:private_group) { create(:group, :private) }
+
+ before do
+ private_group.add_guest(another_user)
+ end
- results.objects('users')
+ it 'does not include the other user' do
+ expect(user_search_result).to match_array(user)
+ end
+
+ context 'when the current_user is a guest of the private group' do
+ before do
+ private_group.add_guest(user)
+ end
+
+ it 'includes the other user' do
+ expect(user_search_result).to match_array([user, another_user])
+ end
+ end
+
+ context 'when the current_user is a guest of the public parent of the private group' do
+ let_it_be(:public_parent_group) { create(:group, :public) }
+ let_it_be(:private_group) { create(:group, :private, parent: public_parent_group) }
+
+ before do
+ public_parent_group.add_guest(user)
+ end
+
+ it 'includes the other user' do
+ expect(user_search_result).to match_array([user, another_user])
+ end
+ end
+ end
+ end
+
+ context 'when users_search_scoped_to_authorized_namespaces_basic_search is enabled' do
+ before do
+ stub_feature_flags(users_search_scoped_to_authorized_namespaces_basic_search: true)
+ stub_feature_flags(users_search_scoped_to_authorized_namespaces_basic_search_by_ids: false)
+ end
+
+ include_examples 'returns users'
+ end
+
+ context 'when users_search_scoped_to_authorized_namespaces_basic_search_by_ids is enabled' do
+ before do
+ stub_feature_flags(users_search_scoped_to_authorized_namespaces_basic_search_by_ids: true)
+ stub_feature_flags(users_search_scoped_to_authorized_namespaces_basic_search: false)
+ end
+
+ include_examples 'returns users'
+ end
end
end
end
--
GitLab