Merge branch 'security-585-revert-the-revert' into 'master'

Users can be searched by exact email match even if email is private

See merge request gitlab-org/security/gitlab!2182

app/finders/users_finder.rb

@@ -78,7 +78,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)

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?
 

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:
 

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:
 

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
 

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

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' ) }