summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/models/commit.rb25
-rw-r--r--app/models/user.rb49
-rw-r--r--lib/gitlab/private_commit_email.rb4
-rw-r--r--spec/lib/gitlab/private_commit_email_spec.rb24
-rw-r--r--spec/models/commit_spec.rb24
-rw-r--r--spec/models/user_spec.rb37
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