diff options
| -rw-r--r-- | app/models/commit.rb | 25 | ||||
| -rw-r--r-- | app/models/user.rb | 49 | ||||
| -rw-r--r-- | lib/gitlab/private_commit_email.rb | 4 | ||||
| -rw-r--r-- | spec/lib/gitlab/private_commit_email_spec.rb | 24 | ||||
| -rw-r--r-- | spec/models/commit_spec.rb | 24 | ||||
| -rw-r--r-- | spec/models/user_spec.rb | 37 |
6 files changed, 119 insertions, 44 deletions
diff --git a/app/models/commit.rb b/app/models/commit.rb index 9dd0cbacd9e..546fcc54a15 100644 --- a/app/models/commit.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index a400058e87e..01eba7e0426 100644 --- a/app/models/user.rb +++ b/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 + # 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 - emails = joins(:emails).where(emails: { email: email }) - emails = emails.confirmed if confirmed + from_emails = joins(:emails).where(emails: { email: emails }) + from_emails = from_emails.confirmed if confirmed - from_union([users, emails]) + 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) + # handle the outdated private commit email case + return true if persisted? && + id == Gitlab::PrivateCommitEmail.user_id_for_email(downcased) - user_id == id || emails.confirmed.where(email: downcased).exists? - end + verified_emails.include?(check_email.downcase) end def hook_attrs diff --git a/lib/gitlab/private_commit_email.rb b/lib/gitlab/private_commit_email.rb index bade2248ccd..536fc9dae3a 100644 --- a/lib/gitlab/private_commit_email.rb +++ b/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 diff --git a/spec/lib/gitlab/private_commit_email_spec.rb b/spec/lib/gitlab/private_commit_email_spec.rb index bc86cd3842a..10bf624bbdd 100644 --- a/spec/lib/gitlab/private_commit_email_spec.rb +++ b/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(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 - expect(described_class.user_id_for_email(email)).to be_nil + 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 diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index ed41ff7a0fa..2a0039a0635 100644 --- a/spec/models/commit_spec.rb +++ b/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) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 0ac5bd666ae..733c1c49f08 100644 --- a/spec/models/user_spec.rb +++ b/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 |