Commit af1ed0e2 authored by Douwe Maan's avatar Douwe Maan

Merge branch '54046-fix-by-any-email-for-private-commit-emails' into 'master'

Match users better by their private commit email

Closes #54046

See merge request gitlab-org/gitlab-ce!23080
parents d5f08596 59c4fb4e
...@@ -230,24 +230,13 @@ class Commit ...@@ -230,24 +230,13 @@ class Commit
def lazy_author def lazy_author
BatchLoader.for(author_email.downcase).batch do |emails, loader| BatchLoader.for(author_email.downcase).batch do |emails, loader|
# A Hash that maps user Emails to the corresponding User objects. The users = User.by_any_email(emails).includes(:emails)
# Emails at this point are the _primary_ Emails of the Users.
users_for_emails = User emails.each do |email|
.by_any_email(emails) user = users.find { |u| u.any_email?(email) }
.each_with_object({}) { |user, hash| hash[user.email] = user }
loader.call(email, user)
users_for_ids = users_for_emails end
.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) }
end end
end end
......
...@@ -349,20 +349,28 @@ class User < ActiveRecord::Base ...@@ -349,20 +349,28 @@ class User < ActiveRecord::Base
def find_by_any_email(email, confirmed: false) def find_by_any_email(email, confirmed: false)
return unless email return unless email
downcased = email.downcase by_any_email(email, confirmed: confirmed).take
find_by_private_commit_email(downcased) || by_any_email(downcased, confirmed: confirmed).take
end end
# Returns a relation containing all the users for the given Email address # Returns a relation containing all the users for the given email addresses
def by_any_email(email, confirmed: false) #
users = where(email: email) # @param emails [String, Array<String>] email addresses to check
users = users.confirmed if confirmed # @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 }) from_emails = joins(:emails).where(emails: { email: emails })
emails = emails.confirmed if confirmed 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 end
def find_by_private_commit_email(email) def find_by_private_commit_email(email)
...@@ -1031,6 +1039,7 @@ class User < ActiveRecord::Base ...@@ -1031,6 +1039,7 @@ class User < ActiveRecord::Base
def all_emails def all_emails
all_emails = [] all_emails = []
all_emails << email unless temp_oauth_email? all_emails << email unless temp_oauth_email?
all_emails << private_commit_email
all_emails.concat(emails.map(&:email)) all_emails.concat(emails.map(&:email))
all_emails all_emails
end end
...@@ -1043,16 +1052,24 @@ class User < ActiveRecord::Base ...@@ -1043,16 +1052,24 @@ class User < ActiveRecord::Base
verified_emails verified_emails
end end
def verified_email?(check_email) def any_email?(check_email)
downcased = check_email.downcase downcased = check_email.downcase
if email == downcased # handle the outdated private commit email case
primary_email_verified? return true if persisted? &&
else id == Gitlab::PrivateCommitEmail.user_id_for_email(downcased)
user_id = Gitlab::PrivateCommitEmail.user_id_for_email(downcased)
user_id == id || emails.confirmed.where(email: downcased).exists? all_emails.include?(check_email.downcase)
end end
def verified_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)
verified_emails.include?(check_email.downcase)
end end
def hook_attrs def hook_attrs
......
...@@ -18,6 +18,10 @@ module Gitlab ...@@ -18,6 +18,10 @@ module Gitlab
match[:id].to_i match[:id].to_i
end end
def user_ids_for_emails(emails)
emails.map { |email| user_id_for_email(email) }.compact.uniq
end
def for_user(user) def for_user(user)
hostname = Gitlab::CurrentSettings.current_application_settings.commit_email_hostname hostname = Gitlab::CurrentSettings.current_application_settings.commit_email_hostname
......
...@@ -4,6 +4,9 @@ require 'spec_helper' ...@@ -4,6 +4,9 @@ require 'spec_helper'
describe Gitlab::PrivateCommitEmail do describe Gitlab::PrivateCommitEmail do
let(:hostname) { Gitlab::CurrentSettings.current_application_settings.commit_email_hostname } 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 context '.regex' do
subject { described_class.regex } subject { described_class.regex }
...@@ -16,18 +19,25 @@ describe Gitlab::PrivateCommitEmail do ...@@ -16,18 +19,25 @@ describe Gitlab::PrivateCommitEmail do
end end
context '.user_id_for_email' do context '.user_id_for_email' do
let(:id) { 1 }
it 'parses user id from email' do it 'parses user id from email' do
email = "#{id}-foo@#{hostname}" expect(described_class.user_id_for_email(valid_email)).to eq(id)
expect(described_class.user_id_for_email(email)).to eq(id)
end end
it 'returns nil on invalid commit email' do 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
end end
......
...@@ -72,6 +72,7 @@ describe Commit do ...@@ -72,6 +72,7 @@ describe Commit do
context 'using eager loading' do context 'using eager loading' do
let!(:alice) { create(:user, email: 'alice@example.com') } let!(:alice) { create(:user, email: 'alice@example.com') }
let!(:bob) { create(:user, email: 'hunter2@example.com') } let!(:bob) { create(:user, email: 'hunter2@example.com') }
let!(:jeff) { create(:user) }
let(:alice_commit) do let(:alice_commit) do
described_class.new(RepoHelpers.sample_commit, project).tap do |c| described_class.new(RepoHelpers.sample_commit, project).tap do |c|
...@@ -93,7 +94,14 @@ describe Commit do ...@@ -93,7 +94,14 @@ describe Commit do
end end
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 before do
create(:email, user: bob, email: 'bob@example.com') create(:email, user: bob, email: 'bob@example.com')
...@@ -125,6 +133,20 @@ describe Commit do ...@@ -125,6 +133,20 @@ describe Commit do
expect(bob_commit.author).to eq(bob) expect(bob_commit.author).to eq(bob)
end 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 it 'sets the author to Nil if an author could not be found for a Commit' do
commits.each(&:lazy_author) commits.each(&:lazy_author)
......
...@@ -1174,6 +1174,22 @@ describe User do ...@@ -1174,6 +1174,22 @@ describe User do
expect(described_class.by_any_email(user.email, confirmed: true)).to eq([user]) expect(described_class.by_any_email(user.email, confirmed: true)).to eq([user])
end 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 end
describe '.search' do describe '.search' do
...@@ -1501,7 +1517,12 @@ describe User do ...@@ -1501,7 +1517,12 @@ describe User do
email_unconfirmed = create :email, user: user email_unconfirmed = create :email, user: user
user.reload 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
end end
...@@ -1512,7 +1533,11 @@ describe User do ...@@ -1512,7 +1533,11 @@ describe User do
email_confirmed = create :email, user: user, confirmed_at: Time.now email_confirmed = create :email, user: user, confirmed_at: Time.now
create :email, user: user 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
end end
...@@ -1532,6 +1557,14 @@ describe User do ...@@ -1532,6 +1557,14 @@ describe User do
expect(user.verified_email?(user.private_commit_email)).to be_truthy expect(user.verified_email?(user.private_commit_email)).to be_truthy
end 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 it 'returns false when the email is not verified/confirmed' do
email_unconfirmed = create :email, user: user email_unconfirmed = create :email, user: user
user.reload user.reload
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment