From 41fd6d4d38aaef723e501ff3ab38ae63e31d4efb Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Thu, 3 Feb 2022 11:28:54 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@14-7-stable-ee --- app/finders/users_finder.rb | 2 +- app/models/user.rb | 19 +++- doc/api/members.md | 8 +- doc/api/users.md | 2 +- .../features/groups/members/manage_members_spec.rb | 4 +- spec/finders/users_finder_spec.rb | 12 +++ spec/models/user_spec.rb | 111 ++++++++++++++++++--- 7 files changed, 133 insertions(+), 25 deletions(-) diff --git a/app/finders/users_finder.rb b/app/finders/users_finder.rb index 8054ecbd502..2b4ce615090 100644 --- a/app/finders/users_finder.rb +++ b/app/finders/users_finder.rb @@ -74,7 +74,7 @@ class UsersFinder def by_search(users) return users unless params[:search].present? - users.search(params[:search]) + users.search(params[:search], with_private_emails: current_user&.admin?) end def by_blocked(users) diff --git a/app/models/user.rb b/app/models/user.rb index a587723053f..1d452fc2e50 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -648,6 +648,7 @@ class User < ApplicationRecord # This method uses ILIKE on PostgreSQL. # # query - The search query as a String + # with_private_emails - include private emails in search # # Returns an ActiveRecord::Relation. def search(query, **options) @@ -660,14 +661,16 @@ class User < ApplicationRecord CASE WHEN users.name = :query THEN 0 WHEN users.username = :query THEN 1 - WHEN users.email = :query THEN 2 + WHEN users.public_email = :query THEN 2 ELSE 3 END SQL sanitized_order_sql = Arel.sql(sanitize_sql_array([order, query: query])) - search_with_secondary_emails(query).reorder(sanitized_order_sql, :name) + scope = options[:with_private_emails] ? search_with_secondary_emails(query) : search_with_public_emails(query) + + scope.reorder(sanitized_order_sql, :name) end # Limits the result set to users _not_ in the given query/list of IDs. @@ -682,6 +685,18 @@ class User < ApplicationRecord reorder(:name) end + def search_with_public_emails(query) + return none if query.blank? + + query = query.downcase + + where( + fuzzy_arel_match(:name, query, use_minimum_char_limit: user_search_minimum_char_limit) + .or(fuzzy_arel_match(:username, query, use_minimum_char_limit: user_search_minimum_char_limit)) + .or(arel_table[:public_email].eq(query)) + ) + end + def search_without_secondary_emails(query) return none if query.blank? diff --git a/doc/api/members.md b/doc/api/members.md index ee6d2cfa2d8..95565d40897 100644 --- a/doc/api/members.md +++ b/doc/api/members.md @@ -267,11 +267,11 @@ respectively. GET /groups/:id/billable_members ``` -| Attribute | Type | Required | Description | -| --------- | ---- | -------- | ----------- | +| Attribute | Type | Required | Description | +| --------- | ---- | -------- |--------------------------------------------------------------------------------------------------------------| | `id` | integer/string | yes | The ID or [URL-encoded path of the group](index.md#namespaced-path-encoding) owned by the authenticated user | -| `search` | string | no | A query string to search for group members by name, username, or email. | -| `sort` | string | no | A query string containing parameters that specify the sort attribute and order. See supported values below.| +| `search` | string | no | A query string to search for group members by name, username, or public email. | +| `sort` | string | no | A query string containing parameters that specify the sort attribute and order. See supported values below. | The supported values for the `sort` attribute are: diff --git a/doc/api/users.md b/doc/api/users.md index 28e1512ea12..9b79a249220 100644 --- a/doc/api/users.md +++ b/doc/api/users.md @@ -39,7 +39,7 @@ GET /users ] ``` -You can also search for users by name, username, primary email, or secondary email, by using `?search=`. For example. `/users?search=John`. +You can also search for users by name, username or public email by using `?search=`. For example. `/users?search=John`. In addition, you can lookup users by username: diff --git a/spec/features/groups/members/manage_members_spec.rb b/spec/features/groups/members/manage_members_spec.rb index 0ce50107e54..e5dad5ee4be 100644 --- a/spec/features/groups/members/manage_members_spec.rb +++ b/spec/features/groups/members/manage_members_spec.rb @@ -103,7 +103,7 @@ RSpec.describe 'Groups > Members > Manage members' do find('[data-testid="members-token-select-input"]').set('undisclosed_email@gitlab.com') wait_for_requests - expect(page).to have_content("Jane 'invisible' Doe") + expect(page).to have_content('Invite "undisclosed_email@gitlab.com" by email') end context 'when Invite Members modal is disabled' do @@ -129,7 +129,7 @@ RSpec.describe 'Groups > Members > Manage members' do select_input.send_keys('undisclosed_email@gitlab.com') wait_for_requests - expect(page).to have_content("Jane 'invisible' Doe") + expect(page).to have_content('Invite "undisclosed_email@gitlab.com" by email') end end diff --git a/spec/finders/users_finder_spec.rb b/spec/finders/users_finder_spec.rb index b0f8b803141..fab48cf3178 100644 --- a/spec/finders/users_finder_spec.rb +++ b/spec/finders/users_finder_spec.rb @@ -39,6 +39,12 @@ RSpec.describe UsersFinder do expect(users).to contain_exactly(blocked_user) end + it 'does not filter by private emails search' do + users = described_class.new(user, search: normal_user.email).execute + + expect(users).to be_empty + end + it 'filters by blocked users' do users = described_class.new(user, blocked: true).execute @@ -135,6 +141,12 @@ RSpec.describe UsersFinder do expect(users).to contain_exactly(normal_user) end + + it 'filters by private emails search' do + users = described_class.new(admin, search: normal_user.email).execute + + expect(users).to contain_exactly(normal_user) + end end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index ac2474ac393..c2535fd3698 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -2582,6 +2582,12 @@ RSpec.describe User do describe '.search' do let_it_be(:user) { create(:user, name: 'user', username: 'usern', email: 'email@example.com') } + let_it_be(:public_email) do + create(:email, :confirmed, user: user, email: 'publicemail@example.com').tap do |email| + user.update!(public_email: email.email) + end + end + let_it_be(:user2) { create(:user, name: 'user name', username: 'username', email: 'someemail@example.com') } let_it_be(:user3) { create(:user, name: 'us', username: 'se', email: 'foo@example.com') } let_it_be(:email) { create(:email, user: user, email: 'alias@example.com') } @@ -2609,30 +2615,31 @@ RSpec.describe User do end describe 'email matching' do - it 'returns users with a matching Email' do - expect(described_class.search(user.email)).to eq([user]) + it 'returns users with a matching public email' do + expect(described_class.search(user.public_email)).to match_array([user]) end - it 'does not return users with a partially matching Email' do - expect(described_class.search(user.email[1...-1])).to be_empty + it 'does not return users with a partially matching public email' do + expect(described_class.search(user.public_email[1...-1])).to be_empty end - it 'returns users with a matching Email regardless of the casing' do - expect(described_class.search(user2.email.upcase)).to eq([user2]) + it 'returns users with a matching public email regardless of the casing' do + expect(described_class.search(user.public_email.upcase)).to match_array([user]) end - end - describe 'secondary email matching' do - it 'returns users with a matching secondary email' do - expect(described_class.search(email.email)).to include(email.user) + it 'does not return users with a matching private email' do + expect(described_class.search(user.email)).to be_empty + expect(described_class.search(email.email)).to be_empty end - it 'does not return users with a matching part of secondary email' do - expect(described_class.search(email.email[1...-1])).to be_empty - end + context 'with private emails search' do + it 'returns users with matching private email' do + expect(described_class.search(user.email, with_private_emails: true)).to match_array([user]) + end - it 'returns users with a matching secondary email regardless of the casing' do - expect(described_class.search(email.email.upcase)).to include(email.user) + it 'returns users with matching private secondary email' do + expect(described_class.search(email.email, with_private_emails: true)).to match_array([user]) + end end end @@ -2733,6 +2740,80 @@ RSpec.describe User do end end + describe '.search_with_public_emails' do + let_it_be(:user) { create(:user, name: 'John Doe', username: 'john.doe', email: 'someone.1@example.com' ) } + let_it_be(:another_user) { create(:user, name: 'Albert Smith', username: 'albert.smith', email: 'another.2@example.com' ) } + let_it_be(:public_email) do + create(:email, :confirmed, user: another_user, email: 'alias@example.com').tap do |email| + another_user.update!(public_email: email.email) + end + end + + let_it_be(:secondary_email) do + create(:email, :confirmed, user: another_user, email: 'secondary@example.com') + end + + it 'returns users with a matching name' do + expect(described_class.search_with_public_emails(user.name)).to match_array([user]) + end + + it 'returns users with a partially matching name' do + expect(described_class.search_with_public_emails(user.name[0..2])).to match_array([user]) + end + + it 'returns users with a matching name regardless of the casing' do + expect(described_class.search_with_public_emails(user.name.upcase)).to match_array([user]) + end + + it 'returns users with a matching public email' do + expect(described_class.search_with_public_emails(another_user.public_email)).to match_array([another_user]) + end + + it 'does not return users with a partially matching email' do + expect(described_class.search_with_public_emails(another_user.public_email[1...-1])).to be_empty + end + + it 'returns users with a matching email regardless of the casing' do + expect(described_class.search_with_public_emails(another_user.public_email.upcase)).to match_array([another_user]) + end + + it 'returns users with a matching username' do + expect(described_class.search_with_public_emails(user.username)).to match_array([user]) + end + + it 'returns users with a partially matching username' do + expect(described_class.search_with_public_emails(user.username[0..2])).to match_array([user]) + end + + it 'returns users with a matching username regardless of the casing' do + expect(described_class.search_with_public_emails(user.username.upcase)).to match_array([user]) + end + + it 'does not return users with a matching whole private email' do + expect(described_class.search_with_public_emails(user.email)).not_to include(user) + end + + it 'does not return users with a matching whole private email' do + expect(described_class.search_with_public_emails(secondary_email.email)).to be_empty + end + + it 'does not return users with a matching part of secondary email' do + expect(described_class.search_with_public_emails(secondary_email.email[1...-1])).to be_empty + end + + it 'does not return users with a matching part of private email' do + expect(described_class.search_with_public_emails(user.email[1...-1])).to be_empty + end + + it 'returns no matches for an empty string' do + expect(described_class.search_with_public_emails('')).to be_empty + end + + it 'returns no matches for nil' do + expect(described_class.search_with_public_emails(nil)).to be_empty + end + end + describe '.search_with_secondary_emails' do let_it_be(:user) { create(:user, name: 'John Doe', username: 'john.doe', email: 'someone.1@example.com' ) } let_it_be(:another_user) { create(:user, name: 'Albert Smith', username: 'albert.smith', email: 'another.2@example.com' ) } -- cgit v1.2.1