Merge branch 'sh-issue-28493' into 'master'

Do not attribute unverified commit e-mails to GitLab users

Closes #28493

See merge request gitlab-org/gitlab!21214

app/models/commit.rb

@@ -246,7 +246,7 @@ class Commit
 
   def lazy_author
     BatchLoader.for(author_email.downcase).batch do |emails, loader|
-      users = User.by_any_email(emails).includes(:emails)
+      users = User.by_any_email(emails, confirmed: true).includes(:emails)
 
       emails.each do |email|
         user = users.find { |u| u.any_email?(email) }
@@ -263,8 +263,8 @@ class Commit
   end
   request_cache(:author) { author_email.downcase }
 
-  def committer
-    @committer ||= User.find_by_any_email(committer_email)
+  def committer(confirmed: true)
+    @committer ||= User.find_by_any_email(committer_email, confirmed: confirmed)
   end
 
   def parents

changelogs/unreleased/sh-issue-28493.yml

+---
+title: Do not attribute unverified commit e-mails to GitLab users
+merge_request: 21214
+author:
+type: fixed

ee/lib/ee/gitlab/checks/push_rules/commit_check.rb

@@ -82,9 +82,13 @@ module EE
 
           def committer_check(commit)
             unless push_rule.committer_allowed?(commit.committer_email, user_access.user)
-              committer_is_current_user = commit.committer == user_access.user
+              # We can assume only one user holds an unconfirmed e-mail address. Since we want
+              # to give feedback whether this is an unconfirmed address, we look for any user that
+              # matches by disabling the confirmation requirement.
+              committer = commit.committer(confirmed: false)
+              committer_is_current_user = committer == user_access.user
 
-              if committer_is_current_user && !commit.committer.verified_email?(commit.committer_email)
+              if committer_is_current_user && !committer.verified_email?(commit.committer_email)
                 ERROR_MESSAGES[:committer_not_verified] % { committer_email: commit.committer_email }
               else
                 ERROR_MESSAGES[:committer_not_allowed] % { committer_email: commit.committer_email }

spec/features/participants_autocomplete_spec.rb

@@ -61,7 +61,7 @@ describe 'Member autocomplete', :js do
 
     before do
       allow(User).to receive(:find_by_any_email)
-        .with(noteable.author_email.downcase).and_return(author)
+        .with(noteable.author_email.downcase, confirmed: true).and_return(author)
 
       visit project_commit_path(project, noteable)
     end

spec/features/projects/commits/user_browses_commits_spec.rb

@@ -76,16 +76,23 @@ describe 'User browses commits' do
   end
 
   context 'secondary email' do
+    let(:user) { create(:user) }
+
     it 'finds a commit by a secondary email' do
-      user =
-        create(:user) do |user|
-          create(:email, { user: user, email: 'dmitriy.zaporozhets@gmail.com' })
-        end
+      create(:email, :confirmed, user: user, email: 'dmitriy.zaporozhets@gmail.com')
 
       visit(project_commit_path(project, sample_commit.parent_id))
 
       check_author_link(sample_commit.author_email, user)
     end
+
+    it 'links to an unverified e-mail address instead of the user' do
+      create(:email, user: user, email: 'dmitriy.zaporozhets@gmail.com')
+
+      visit(project_commit_path(project, sample_commit.parent_id))
+
+      check_author_email(sample_commit.author_email)
+    end
   end
 
   context 'when the blob does not exist' do
@@ -263,3 +270,9 @@ def check_author_link(email, author)
   expect(author_link['href']).to eq(user_path(author))
   expect(find('.commit-author-name').text).to eq(author.name)
 end
+
+def check_author_email(email)
+  author_link = find('.commit-author-link')
+
+  expect(author_link['href']).to eq("mailto:#{email}")
+end

spec/models/commit_spec.rb

@@ -80,6 +80,17 @@ describe Commit do
       expect(commit.author).to eq(user)
     end
 
+    context 'with a user with an unconfirmed e-mail' do
+      before do
+        user = create(:user)
+        create(:email, user: user, email: commit.author_email)
+      end
+
+      it 'returns no user' do
+        expect(commit.author).to be_nil
+      end
+    end
+
     context 'using eager loading' do
       let!(:alice) { create(:user, email: 'alice@example.com') }
       let!(:bob) { create(:user, email: 'hunter2@example.com') }
@@ -115,7 +126,7 @@ describe Commit do
       let!(:commits) { [alice_commit, bob_commit, eve_commit, jeff_commit] }
 
       before do
-        create(:email, user: bob, email: 'bob@example.com')
+        create(:email, :confirmed, user: bob, email: 'bob@example.com')
       end
 
       it 'executes only two SQL queries' do
@@ -179,6 +190,32 @@ describe Commit do
     end
   end
 
+  describe '#committer' do
+    context 'with a confirmed e-mail' do
+      it 'returns the user' do
+        user = create(:user, email: commit.committer_email)
+
+        expect(commit.committer).to eq(user)
+      end
+    end
+
+    context 'with an unconfirmed e-mail' do
+      let(:user) { create(:user) }
+
+      before do
+        create(:email, user: user, email: commit.committer_email)
+      end
+
+      it 'returns no user' do
+        expect(commit.committer).to be_nil
+      end
+
+      it 'returns the user' do
+        expect(commit.committer(confirmed: false)).to eq(user)
+      end
+    end
+  end
+
   describe '#to_reference' do
     let(:project) { create(:project, :repository, path: 'sample-project') }