Match users better by their private commit email

Private commit emails were introduced in !22560, but some parts of
GitLab were not updated to take account of them. This commit adds
support in places that were missed.

app/models/commit.rb

@@ -230,24 +230,13 @@ class Commit
   def lazy_author
     BatchLoader.for(author_email.downcase).batch do |emails, loader|
-      # A Hash that maps user Emails to the corresponding User objects. The
-      # Emails at this point are the _primary_ Emails of the Users.
-      users_for_emails = User
-        .by_any_email(emails)
-        .each_with_object({}) { |user, hash| hash[user.email] = user }
-
-      users_for_ids = users_for_emails
-        .values
-        .each_with_object({}) { |user, hash| hash[user.id] = user }
-
-      # Some commits may have used an alternative Email address. In this case we
-      # need to query the "emails" table to map those addresses to User objects.
-      Email
-        .where(email: emails - users_for_emails.keys)
-        .pluck(:email, :user_id)
-        .each { |(email, id)| users_for_emails[email] = users_for_ids[id] }
-
-      users_for_emails.each { |email, user| loader.call(email, user) }
+      users = User.by_any_email(emails).includes(:emails)
+
+      emails.each do |email|
+        user = users.find { |u| u.any_email?(email) }
+
+        loader.call(email, user)
+      end
     end
   end

app/models/user.rb

@@ -349,20 +349,28 @@ class User < ActiveRecord::Base
     def find_by_any_email(email, confirmed: false)
       return unless email
-      downcased = email.downcase
-
-      find_by_private_commit_email(downcased) || by_any_email(downcased, confirmed: confirmed).take
+      by_any_email(email, confirmed: confirmed).take
     end
-    # Returns a relation containing all the users for the given Email address
-    def by_any_email(email, confirmed: false)
-      users = where(email: email)
-      users = users.confirmed if confirmed
-      emails = joins(:emails).where(emails: { email: email })
-      emails = emails.confirmed if confirmed
-      from_union([users, emails])
+    # Returns a relation containing all the users for the given email addresses
+    #
+    # @param emails [String, Array<String>] email addresses to check
+    # @param confirmed [Boolean] Only return users where the email is confirmed
+    def by_any_email(emails, confirmed: false)
+      emails = Array(emails).map(&:downcase)
+
+      from_users = where(email: emails)
+      from_users = from_users.confirmed if confirmed
+      from_emails = joins(:emails).where(emails: { email: emails })
+      from_emails = from_emails.confirmed if confirmed
+      items = [from_users, from_emails]
+
+      user_ids = Gitlab::PrivateCommitEmail.user_ids_for_emails(emails)
+      items << where(id: user_ids) if user_ids.present?
+
+      from_union(items)
     end
     def find_by_private_commit_email(email)
@@ -1031,6 +1039,7 @@ class User < ActiveRecord::Base
   def all_emails
     all_emails = []
     all_emails << email unless temp_oauth_email?
+    all_emails << private_commit_email
     all_emails.concat(emails.map(&:email))
     all_emails
   end
@@ -1043,16 +1052,24 @@ class User < ActiveRecord::Base
     verified_emails
   end
+  def any_email?(check_email)
+    downcased = check_email.downcase
+
+    # handle the outdated private commit email case
+    return true if persisted? &&
+        id == Gitlab::PrivateCommitEmail.user_id_for_email(downcased)
+
+    all_emails.include?(check_email.downcase)
+  end
+
   def verified_email?(check_email)
     downcased = check_email.downcase
-    if email == downcased
-      primary_email_verified?
-    else
-      user_id = Gitlab::PrivateCommitEmail.user_id_for_email(downcased)
-      user_id == id || emails.confirmed.where(email: downcased).exists?
-    end
+    # handle the outdated private commit email case
+    return true if persisted? &&
+        id == Gitlab::PrivateCommitEmail.user_id_for_email(downcased)
+    verified_emails.include?(check_email.downcase)
   end
   def hook_attrs

lib/gitlab/private_commit_email.rb

@@ -18,6 +18,10 @@ module Gitlab
         match[:id].to_i
       end
+      def user_ids_for_emails(emails)
+        emails.map { |email| user_id_for_email(email) }.compact.uniq
+      end
+
       def for_user(user)
         hostname = Gitlab::CurrentSettings.current_application_settings.commit_email_hostname

spec/lib/gitlab/private_commit_email_spec.rb

@@ -4,6 +4,9 @@ require 'spec_helper'
 describe Gitlab::PrivateCommitEmail do
   let(:hostname) { Gitlab::CurrentSettings.current_application_settings.commit_email_hostname }
+  let(:id) { 1 }
+  let(:valid_email) { "#{id}-foo@#{hostname}" }
+  let(:invalid_email) { "#{id}-foo@users.noreply.bar.com" }
   context '.regex' do
     subject { described_class.regex }
@@ -16,18 +19,25 @@ describe Gitlab::PrivateCommitEmail do
   end
   context '.user_id_for_email' do
-    let(:id) { 1 }
-
     it 'parses user id from email' do
-      email = "#{id}-foo@#{hostname}"
-
-      expect(described_class.user_id_for_email(email)).to eq(id)
+      expect(described_class.user_id_for_email(valid_email)).to eq(id)
     end
     it 'returns nil on invalid commit email' do
-      email = "#{id}-foo@users.noreply.bar.com"
-      expect(described_class.user_id_for_email(email)).to be_nil
+      expect(described_class.user_id_for_email(invalid_email)).to be_nil
+    end
+  end
+
+  context '.user_ids_for_email' do
+    it 'returns deduplicated user IDs for each valid email' do
+      result = described_class.user_ids_for_emails([valid_email, valid_email, invalid_email])
+
+      expect(result).to eq([id])
+    end
+    it 'returns an empty array with no valid emails' do
+      result = described_class.user_ids_for_emails([invalid_email])
+      expect(result).to eq([])
     end
   end

spec/models/commit_spec.rb

@@ -72,6 +72,7 @@ describe Commit do
     context 'using eager loading' do
       let!(:alice) { create(:user, email: 'alice@example.com') }
       let!(:bob) { create(:user, email: 'hunter2@example.com') }
+      let!(:jeff) { create(:user) }
       let(:alice_commit) do
         described_class.new(RepoHelpers.sample_commit, project).tap do |c|
@@ -93,7 +94,14 @@ describe Commit do
         end
       end
-      let!(:commits) { [alice_commit, bob_commit, eve_commit] }
+      let(:jeff_commit) do
+        # The commit for Jeff uses his private commit email
+        described_class.new(RepoHelpers.sample_commit, project).tap do |c|
+          c.author_email = jeff.private_commit_email
+        end
+      end
+
+      let!(:commits) { [alice_commit, bob_commit, eve_commit, jeff_commit] }
       before do
         create(:email, user: bob, email: 'bob@example.com')
@@ -125,6 +133,20 @@ describe Commit do
         expect(bob_commit.author).to eq(bob)
       end
+      it "preloads the authors for Commits using a User's private commit Email" do
+        commits.each(&:lazy_author)
+
+        expect(jeff_commit.author).to eq(jeff)
+      end
+
+      it "preloads the authors for Commits using a User's outdated private commit Email" do
+        jeff.update!(username: 'new-username')
+
+        commits.each(&:lazy_author)
+
+        expect(jeff_commit.author).to eq(jeff)
+      end
+
       it 'sets the author to Nil if an author could not be found for a Commit' do
         commits.each(&:lazy_author)

spec/models/user_spec.rb

@@ -1174,6 +1174,22 @@ describe User do
       expect(described_class.by_any_email(user.email, confirmed: true)).to eq([user])
     end
+
+    it 'finds user through a private commit email' do
+      user = create(:user)
+      private_email = user.private_commit_email
+
+      expect(described_class.by_any_email(private_email)).to eq([user])
+      expect(described_class.by_any_email(private_email, confirmed: true)).to eq([user])
+    end
+
+    it 'finds user through a private commit email in an array' do
+      user = create(:user)
+      private_email = user.private_commit_email
+
+      expect(described_class.by_any_email([private_email])).to eq([user])
+      expect(described_class.by_any_email([private_email], confirmed: true)).to eq([user])
+    end
   end
   describe '.search' do
@@ -1501,7 +1517,12 @@ describe User do
       email_unconfirmed = create :email, user: user
       user.reload
-      expect(user.all_emails).to match_array([user.email, email_unconfirmed.email, email_confirmed.email])
+      expect(user.all_emails).to contain_exactly(
+        user.email,
+        user.private_commit_email,
+        email_unconfirmed.email,
+        email_confirmed.email
+      )
     end
   end
@@ -1512,7 +1533,11 @@ describe User do
       email_confirmed = create :email, user: user, confirmed_at: Time.now
       create :email, user: user
-      expect(user.verified_emails).to match_array([user.email, user.private_commit_email, email_confirmed.email])
+      expect(user.verified_emails).to contain_exactly(
+        user.email,
+        user.private_commit_email,
+        email_confirmed.email
+      )
     end
   end
@@ -1532,6 +1557,14 @@ describe User do
       expect(user.verified_email?(user.private_commit_email)).to be_truthy
     end
+    it 'returns true for an outdated private commit email' do
+      old_email = user.private_commit_email
+
+      user.update!(username: 'changed-username')
+
+      expect(user.verified_email?(old_email)).to be_truthy
+    end
+
     it 'returns false when the email is not verified/confirmed' do
       email_unconfirmed = create :email, user: user
       user.reload