Merge branch 'add-primary-email-to-emails' into 'master'

Add primary email to emails upon confirmation

See merge request gitlab-org/gitlab!70872

app/models/email.rb

@@ -29,7 +29,7 @@ class Email < ApplicationRecord
   end
 
   def unique_email
-    self.errors.add(:email, 'has already been taken') if User.exists?(email: self.email)
+    self.errors.add(:email, 'has already been taken') if primary_email_of_another_user?
   end
 
   def validate_email_format
@@ -40,4 +40,14 @@ class Email < ApplicationRecord
   def update_invalid_gpg_signatures
     user.update_invalid_gpg_signatures if confirmed?
   end
+
+  def user_primary_email?
+    email.casecmp?(user.email)
+  end
+
+  private
+
+  def primary_email_of_another_user?
+    User.where(email: email).where.not(id: user_id).exists?
+  end
 end

app/models/user.rb

@@ -274,14 +274,21 @@ class User < ApplicationRecord
   after_update :username_changed_hook, if: :saved_change_to_username?
   after_destroy :post_destroy_hook
   after_destroy :remove_key_cache
+  after_create :add_primary_email_to_emails!, if: :confirmed?
   after_commit(on: :update) do
     if previous_changes.key?('email')
-      # Grab previous_email here since previous_changes changes after
-      # #update_emails_with_primary_email and #update_notification_email are called
+      # Add the old primary email to Emails if not added already - this should be removed
+      # after the background migration for MR https://gitlab.com/gitlab-org/gitlab/-/merge_requests/70872/ has completed,
+      # as the primary email is now added to Emails upon confirmation
+      # Issue to remove that: https://gitlab.com/gitlab-org/gitlab/-/issues/344134
       previous_confirmed_at = previous_changes.key?('confirmed_at') ? previous_changes['confirmed_at'][0] : confirmed_at
       previous_email = previous_changes[:email][0]
+      if previous_confirmed_at && !emails.exists?(email: previous_email)
+        # rubocop: disable CodeReuse/ServiceClass
+        Emails::CreateService.new(self, user: self, email: previous_email).execute(confirmed_at: previous_confirmed_at)
+        # rubocop: enable CodeReuse/ServiceClass
+      end
 
-      update_emails_with_primary_email(previous_confirmed_at, previous_email)
       update_invalid_gpg_signatures
     end
   end
@@ -935,6 +942,8 @@ class User < ApplicationRecord
   end
 
   def unique_email
+    return if errors.added?(:email, _('has already been taken'))
+
     if !emails.exists?(email: email) && Email.exists?(email: email)
       errors.add(:email, _('has already been taken'))
     end
@@ -963,24 +972,6 @@ class User < ApplicationRecord
     skip_reconfirmation! if emails.confirmed.where(email: self.email).any?
   end
 
-  # Note: the use of the Emails services will cause `saves` on the user object, running
-  # through the callbacks again and can have side effects, such as the `previous_changes`
-  # hash and `_was` variables getting munged.
-  # By using an `after_commit` instead of `after_update`, we avoid the recursive callback
-  # scenario, though it then requires us to use the `previous_changes` hash
-  # rubocop: disable CodeReuse/ServiceClass
-  def update_emails_with_primary_email(previous_confirmed_at, previous_email)
-    primary_email_record = emails.find_by(email: email)
-    Emails::DestroyService.new(self, user: self).execute(primary_email_record) if primary_email_record
-
-    # the original primary email was confirmed, and we want that to carry over.  We don't
-    # have access to the original confirmation values at this point, so just set confirmed_at
-    Emails::CreateService.new(self, user: self, email: previous_email).execute(confirmed_at: previous_confirmed_at)
-
-    update_columns(confirmed_at: primary_email_record.confirmed_at) if primary_email_record&.confirmed_at
-  end
-  # rubocop: enable CodeReuse/ServiceClass
-
   def update_invalid_gpg_signatures
     gpg_keys.each(&:update_invalid_gpg_signatures)
   end
@@ -1389,7 +1380,7 @@ class User < ApplicationRecord
     all_emails << email unless temp_oauth_email?
     all_emails << private_commit_email if include_private_email
     all_emails.concat(emails.map(&:email))
-    all_emails
+    all_emails.uniq
   end
 
   def verified_emails(include_private_email: true)
@@ -1397,7 +1388,7 @@ class User < ApplicationRecord
     verified_emails << email if primary_email_verified?
     verified_emails << private_commit_email if include_private_email
     verified_emails.concat(emails.confirmed.pluck(:email))
-    verified_emails
+    verified_emails.uniq
   end
 
   def public_verified_emails
@@ -1978,6 +1969,25 @@ class User < ApplicationRecord
     ci_job_token_scope.present?
   end
 
+  # override from Devise::Confirmable
+  #
+  # Add the primary email to user.emails (or confirm it if it was already
+  # present) when the primary email is confirmed.
+  def confirm(*args)
+    saved = super(*args)
+    return false unless saved
+
+    email_to_confirm = self.emails.find_by(email: self.email)
+
+    if email_to_confirm.present?
+      email_to_confirm.confirm(*args)
+    else
+      add_primary_email_to_emails!
+    end
+
+    saved
+  end
+
   protected
 
   # override, from Devise::Validatable
@@ -2018,6 +2028,12 @@ class User < ApplicationRecord
     'en'
   end
 
+  # rubocop: disable CodeReuse/ServiceClass
+  def add_primary_email_to_emails!
+    Emails::CreateService.new(self, user: self, email: self.email).execute(confirmed_at: self.confirmed_at)
+  end
+  # rubocop: enable CodeReuse/ServiceClass
+
   def notification_email_verified
     return if notification_email.blank? || temp_oauth_email?
 

app/services/emails/destroy_service.rb

@@ -3,6 +3,8 @@
 module Emails
   class DestroyService < ::Emails::BaseService
     def execute(email)
+      raise StandardError, 'Cannot delete primary email' if email.user_primary_email?
+
       email.destroy && update_secondary_emails!(email.email)
     end
 

app/views/profiles/emails/index.html.haml

@@ -44,7 +44,7 @@
             %span.badge.badge-muted.badge-pill.gl-badge.badge-info= s_('Profiles|Public email')
           - if @primary_email === current_user.notification_email_or_default
             %span.badge.badge-muted.badge-pill.gl-badge.badge-info= s_('Profiles|Default notification email')
-      - @emails.each do |email|
+      - @emails.reject(&:user_primary_email?).each do |email|
         %li{ data: { qa_selector: 'email_row_content' } }
           = render partial: 'shared/email_with_badge', locals: { email: email.email, verified: email.confirmed? }
           %span.float-right

db/post_migrate/20211004120135_schedule_add_primary_email_to_emails_if_user_confirmed.rb

+# frozen_string_literal: true
+
+class ScheduleAddPrimaryEmailToEmailsIfUserConfirmed < Gitlab::Database::Migration[1.0]
+  INTERVAL = 2.minutes.to_i
+  BATCH_SIZE = 10_000
+  MIGRATION = 'AddPrimaryEmailToEmailsIfUserConfirmed'
+
+  disable_ddl_transaction!
+
+  class User < ActiveRecord::Base
+    include ::EachBatch
+
+    self.table_name = 'users'
+    self.inheritance_column = :_type_disabled
+  end
+
+  def up
+    queue_background_migration_jobs_by_range_at_intervals(
+      User,
+      MIGRATION,
+      INTERVAL,
+      batch_size: BATCH_SIZE,
+      track_jobs: true
+    )
+  end
+
+  def down
+    # intentionally blank
+  end
+end

db/schema_migrations/20211004120135

+9cefd32c003a68752f257973a983f77215b02011b7ca792de06c0e92c2462745
\ No newline at end of file

ee/spec/controllers/admin/users_controller_spec.rb

@@ -102,6 +102,8 @@ RSpec.describe Admin::UsersController do
   end
 
   describe "POST #impersonate" do
+    let_it_be(:user) { create(:user) }
+
     before do
       stub_licensed_features(extended_audit_events: true)
     end

ee/spec/controllers/ee/sessions_controller_spec.rb

@@ -96,7 +96,7 @@ RSpec.describe SessionsController, :geo do
 
       context 'when OTP authentication fails' do
         it_behaves_like 'an auditable failed authentication' do
-          let(:user) { create(:user, :two_factor) }
+          let_it_be(:user) { create(:user, :two_factor) }
           let(:operation) { authenticate_2fa(otp_attempt: 'invalid', otp_user_id: user.id) }
           let(:method) { 'OTP' }
         end
@@ -108,7 +108,7 @@ RSpec.describe SessionsController, :geo do
         end
 
         it_behaves_like 'an auditable failed authentication' do
-          let(:user) { create(:user, :two_factor_via_u2f) }
+          let_it_be(:user) { create(:user, :two_factor_via_u2f) }
           let(:operation) { authenticate_2fa(device_response: 'invalid', otp_user_id: user.id) }
           let(:method) { 'U2F' }
         end
@@ -122,7 +122,7 @@ RSpec.describe SessionsController, :geo do
         end
 
         it_behaves_like 'an auditable failed authentication' do
-          let(:user) { create(:user, :two_factor_via_webauthn) }
+          let_it_be(:user) { create(:user, :two_factor_via_webauthn) }
           let(:operation) { authenticate_2fa(device_response: 'invalid', otp_user_id: user.id) }
           let(:method) { 'WebAuthn' }
         end

ee/spec/controllers/passwords_controller_spec.rb

@@ -9,7 +9,7 @@ RSpec.describe PasswordsController do
       stub_licensed_features(extended_audit_events: true)
     end
 
-    let(:user) { create(:user) }
+    let_it_be(:user) { create(:user) }
 
     subject { post :create, params: { user: { email: user.email } } }
 

ee/spec/lib/audit/changes_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe Audit::Changes do
 
   describe '.audit_changes' do
     let(:current_user) { create(:user, name: 'Mickey Mouse') }
-    let(:user) { create(:user, name: 'Donald Duck') }
+    let!(:user) { create(:user, name: 'Donald Duck') }
     let(:options) { { model: user } }
 
     subject(:audit!) { foo_instance.audit_changes(:name, options) }

ee/spec/lib/ee/audit/project_changes_auditor_spec.rb

@@ -4,7 +4,8 @@ require 'spec_helper'
 
 RSpec.describe EE::Audit::ProjectChangesAuditor do
   describe '.audit_changes' do
-    let(:user) { create(:user) }
+    let_it_be(:user) { create(:user) }
+
     let(:project) do
       create(
         :project,

ee/spec/lib/ee/gitlab/usage_data_non_sql_metrics_spec.rb

@@ -15,7 +15,7 @@ RSpec.describe Gitlab::UsageDataNonSqlMetrics do
         described_class.uncached_data
       end
 
-      expect(recorder.count).to eq(55)
+      expect(recorder.count).to eq(57)
     end
   end
 end

ee/spec/requests/api/audit_events_spec.rb

@@ -52,7 +52,7 @@ RSpec.describe API::AuditEvents do
     end
 
     context 'when authenticated, as an admin' do
-      let(:admin) { create(:admin) }
+      let_it_be(:admin) { create(:admin) }
 
       context 'audit events feature is not available' do
         it_behaves_like '403 response' do

ee/spec/requests/api/repositories_spec.rb

@@ -28,7 +28,7 @@ RSpec.describe API::Repositories do
 
     context 'when unauthenticated', 'and project is public' do
       it_behaves_like 'an auditable and successful request' do
-        let(:project) { create(:project, :public, :repository) }
+        let_it_be(:project) { create(:project, :public, :repository) }
         let(:current_user) { nil }
       end
     end

ee/spec/requests/api/users_spec.rb

@@ -62,6 +62,9 @@ RSpec.describe API::Users do
   end
 
   context 'extended audit events' do
+    let_it_be(:user)  { create(:user) }
+    let_it_be(:admin) { create(:admin) }
+
     before do
       stub_licensed_features(extended_audit_events: true)
     end

ee/spec/requests/smartcard_controller_spec.rb

@@ -164,9 +164,14 @@ RSpec.describe SmartcardController, type: :request do
         end
 
         it 'logs audit event' do
-          audit_event_service = instance_double(AuditEventService)
+          audit_event_service = instance_double(::AuditEventService)
 
-          expect(AuditEventService).to(
+          # Creating a confirmed user also creates an email corresponding to the user primary email
+          expect(::AuditEventService).to(
+            receive(:new)
+              .with(instance_of(User), instance_of(User), action: :create).and_call_original)
+
+          expect(::AuditEventService).to(
             receive(:new)
               .with(instance_of(User), instance_of(User), with: auth_method, ip_address: '127.0.0.1')
               .and_return(audit_event_service))

ee/spec/services/applications/create_service_spec.rb

@@ -5,7 +5,8 @@ require 'spec_helper'
 RSpec.describe ::Applications::CreateService do
   include TestRequestHelpers
 
-  let(:user) { create(:user) }
+  let_it_be(:user) { create(:user) }
+
   let(:group) { create(:group) }
   let(:params) { attributes_for(:application) }
 

ee/spec/services/ee/users/approve_service_spec.rb

@@ -3,12 +3,12 @@
 require 'spec_helper'
 
 RSpec.describe Users::ApproveService do
-  let(:current_user) { create(:admin) }
+  let_it_be(:current_user) { create(:admin) }
 
   subject(:service) { described_class.new(current_user) }
 
   describe '#execute', :enable_admin_mode do
-    let(:user) { create(:user, :blocked_pending_approval) }
+    let!(:user) { create(:user, :blocked_pending_approval) }
 
     subject(:operation) { service.execute(user) }
 

ee/spec/services/ee/users/block_service_spec.rb

@@ -3,12 +3,12 @@
 require 'spec_helper'
 
 RSpec.describe Users::BlockService do
-  let(:current_user) { create(:admin) }
+  let_it_be(:current_user) { create(:admin) }
 
   subject(:service) { described_class.new(current_user) }
 
   describe '#execute' do
-    let(:user) { create(:user) }
+    let!(:user) { create(:user) }
 
     subject(:operation) { service.execute(user) }
 

ee/spec/services/ee/users/create_service_spec.rb

@@ -3,7 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Users::CreateService do
-  let(:current_user) { create(:admin) }
+  let_it_be(:current_user) { create(:admin) }
+
   let(:params) do
     {
       name: 'John Doe',

ee/spec/services/ee/users/destroy_service_spec.rb

@@ -3,12 +3,12 @@
 require 'spec_helper'
 
 RSpec.describe Users::DestroyService do
-  let(:current_user) { create(:admin) }
+  let_it_be(:current_user) { create(:admin) }
 
   subject(:service) { described_class.new(current_user) }
 
   describe '#execute' do
-    let(:user) { create(:user) }
+    let!(:user) { create(:user) }
 
     subject(:operation) { service.execute(user) }
 

ee/spec/services/ee/users/reject_service_spec.rb

@@ -32,7 +32,7 @@ RSpec.describe Users::RejectService do
         end
 
         context 'when user does not have permission to reject another user' do
-          let(:current_user) { create(:user) }
+          let_it_be(:current_user) { create(:user) }
 
           it 'does not log any audit event' do
             expect { reject_user }.not_to change { AuditEvent.count }

ee/spec/services/ee/users/update_service_spec.rb

@@ -97,12 +97,14 @@ RSpec.describe Users::UpdateService do
 
     context 'audit events' do
       context 'licensed' do
+        let_it_be_with_reload(:user) { create(:user) }
+
         before do
           stub_licensed_features(admin_audit_log: true)
         end
 
         context 'updating administrator status' do
-          let_it_be(:admin_user) { create(:admin) }
+          let_it_be_with_reload(:admin_user) { create(:admin) }
 
           it 'logs making a user an administrator' do
             expect do
@@ -113,8 +115,9 @@ RSpec.describe Users::UpdateService do
           end
 
           it 'logs making an administrator a user' do
+            admin = create(:admin)
             expect do
-              update_user_as(admin_user, create(:admin), admin: false)
+              update_user_as(admin_user, admin, admin: false)
             end.to change { AuditEvent.count }.by(1)
 
             expect(AuditEvent.last.present.action).to eq('Changed admin status from true to false')
@@ -170,7 +173,8 @@ RSpec.describe Users::UpdateService do
     end
 
     context 'with an admin user' do
-      let!(:admin_user) { create(:admin) }
+      let_it_be_with_reload(:admin_user) { create(:admin) }
+
       let(:service) { described_class.new(admin_user, ActionController::Parameters.new(params).permit!) }
       let(:params) do
         { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: 'mydummypass' }

ee/spec/services/emails/create_service_spec.rb

@@ -3,7 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Emails::CreateService do
-  let(:user) { create(:user) }
+  let_it_be(:user) { create(:user) }
+
   let(:opts) { { email: 'new@email.com', user: user } }
 
   subject(:service) { described_class.new(user, opts) }

ee/spec/services/keys/create_service_spec.rb

@@ -3,8 +3,9 @@
 require 'spec_helper'
 
 RSpec.describe Keys::CreateService do
-  let(:admin) { create(:admin) }
-  let(:user) { create(:user) }
+  let_it_be(:admin) { create(:admin) }
+  let_it_be(:user) { create(:user) }
+
   let(:params) { attributes_for(:key).merge(user: user) }
 
   subject { described_class.new(admin, params) }

ee/spec/services/projects/create_service_spec.rb

@@ -335,6 +335,7 @@ RSpec.describe Projects::CreateService, '#execute' do
 
   context 'audit events' do
     include_examples 'audit event logging' do
+      let_it_be(:user) { create(:user) }
       let(:operation) { create_project(user, opts) }
       let(:fail_condition!) do
         allow(Gitlab::VisibilityLevel).to receive(:allowed_for?).and_return(false)

ee/spec/services/projects/group_links/create_service_spec.rb

@@ -17,6 +17,8 @@ RSpec.describe Projects::GroupLinks::CreateService, '#execute' do
 
   context 'audit events' do
     include_examples 'audit event logging' do
+      let_it_be(:user) { create :user }
+      let_it_be(:project) { create :project }
       let(:operation) { create_group_link(user, project, group, opts) }
       let(:fail_condition!) do
         create(:project_group_link, project: project, group: group)

ee/spec/services/projects/update_service_spec.rb

@@ -123,6 +123,8 @@ RSpec.describe Projects::UpdateService, '#execute' do
   end
 
   context 'audit events' do
+    let_it_be(:user) { create(:user) }
+
     let(:audit_event_params) do
       {
         author_id: user.id,

lib/gitlab/background_migration/add_primary_email_to_emails_if_user_confirmed.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # Add user primary email to emails table if confirmed
+    class AddPrimaryEmailToEmailsIfUserConfirmed
+      INNER_BATCH_SIZE = 1_000
+
+      # Stubbed class to access the User table
+      class User < ActiveRecord::Base
+        include ::EachBatch
+
+        self.table_name = 'users'
+        self.inheritance_column = :_type_disabled
+
+        scope :confirmed, -> { where.not(confirmed_at: nil) }
+
+        has_many :emails
+      end
+
+      # Stubbed class to access the Emails table
+      class Email < ActiveRecord::Base
+        self.table_name = 'emails'
+        self.inheritance_column = :_type_disabled
+
+        belongs_to :user
+      end
+
+      def perform(start_id, end_id)
+        User.confirmed.where(id: start_id..end_id).select(:id, :email, :confirmed_at).each_batch(of: INNER_BATCH_SIZE) do |users|
+          current_time = Time.now.utc
+
+          attributes = users.map do |user|
+            {
+              user_id: user.id,
+              email: user.email,
+              confirmed_at: user.confirmed_at,
+              created_at: current_time,
+              updated_at: current_time
+            }
+          end
+
+          Email.insert_all(attributes)
+        end
+        mark_job_as_succeeded(start_id, end_id)
+      end
+
+      private
+
+      def mark_job_as_succeeded(*arguments)
+        Gitlab::Database::BackgroundMigrationJob.mark_all_as_succeeded(
+          'AddPrimaryEmailToEmailsIfUserConfirmed',
+          arguments
+        )
+      end
+    end
+  end
+end

lib/gitlab/spamcheck/client.rb

@@ -69,6 +69,8 @@ module Gitlab
         user_pb.emails << build_email(user.email, user.confirmed?)
 
         user.emails.each do |email|
+          next if email.user_primary_email?
+
           user_pb.emails << build_email(email.email, email.confirmed?)
         end
 

spec/controllers/oauth/authorizations_controller_spec.rb

@@ -3,8 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe Oauth::AuthorizationsController do
-  let(:user) { create(:user, confirmed_at: confirmed_at) }
-  let(:confirmed_at) { 1.hour.ago }
+  let(:user) { create(:user) }
   let!(:application) { create(:oauth_application, scopes: 'api read_user', redirect_uri: 'http://example.com') }
   let(:params) do
     {
@@ -40,7 +39,7 @@ RSpec.describe Oauth::AuthorizationsController do
     end
 
     context 'when the user is unconfirmed' do
-      let(:confirmed_at) { nil }
+      let(:user) { create(:user, :unconfirmed) }
 
       it 'returns 200 and renders error view' do
         subject
@@ -73,8 +72,6 @@ RSpec.describe Oauth::AuthorizationsController do
     include_examples "Implicit grant can't be used in confidential application"
 
     context 'when the user is confirmed' do
-      let(:confirmed_at) { 1.hour.ago }
-
       context 'when there is already an access token for the application with a matching scope' do
         before do
           scopes = Doorkeeper::OAuth::Scopes.from_string('api')

spec/features/profiles/emails_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 RSpec.describe 'Profile > Emails' do
   let(:user) { create(:user) }
+  let(:other_user) { create(:user) }
 
   before do
     sign_in(user)
@@ -23,15 +24,25 @@ RSpec.describe 'Profile > Emails' do
       expect(page).to have_content('Resend confirmation email')
     end
 
-    it 'does not add a duplicate email' do
-      fill_in('Email', with: user.email)
+    it 'does not add an email that is the primary email of another user' do
+      fill_in('Email', with: other_user.email)
       click_button('Add email address')
 
-      email = user.emails.find_by(email: user.email)
+      email = user.emails.find_by(email: other_user.email)
       expect(email).to be_nil
       expect(page).to have_content('Email has already been taken')
     end
 
+    it 'adds an email that is the primary email of the same user' do
+      fill_in('Email', with: user.email)
+      click_button('Add email address')
+
+      email = user.emails.find_by(email: user.email)
+      expect(email).to be_present
+      expect(page).to have_content("#{user.email} Verified")
+      expect(page).not_to have_content("#{user.email} Unverified")
+    end
+
     it 'does not add an invalid email' do
       fill_in('Email', with: 'test.@example.com')
       click_button('Add email address')

spec/features/profiles/user_manages_emails_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 RSpec.describe 'User manages emails' do
   let(:user) { create(:user) }
+  let(:other_user) { create(:user) }
 
   before do
     sign_in(user)
@@ -11,7 +12,7 @@ RSpec.describe 'User manages emails' do
     visit(profile_emails_path)
   end
 
-  it "shows user's emails" do
+  it "shows user's emails", :aggregate_failures do
     expect(page).to have_content(user.email)
 
     user.emails.each do |email|
@@ -19,7 +20,7 @@ RSpec.describe 'User manages emails' do
     end
   end
 
-  it 'adds an email' do
+  it 'adds an email', :aggregate_failures do
     fill_in('email_email', with: 'my@email.com')
     click_button('Add')
 
@@ -34,21 +35,21 @@ RSpec.describe 'User manages emails' do
     end
   end
 
-  it 'does not add a duplicate email' do
-    fill_in('email_email', with: user.email)
+  it 'does not add an email that is the primary email of another user', :aggregate_failures do
+    fill_in('email_email', with: other_user.email)
     click_button('Add')
 
-    email = user.emails.find_by(email: user.email)
+    email = user.emails.find_by(email: other_user.email)
 
     expect(email).to be_nil
-    expect(page).to have_content(user.email)
+    expect(page).to have_content('Email has already been taken')
 
     user.emails.each do |email|
       expect(page).to have_content(email.email)
     end
   end
 
-  it 'removes an email' do
+  it 'removes an email', :aggregate_failures do
     fill_in('email_email', with: 'my@email.com')
     click_button('Add')
 

spec/features/signed_commits_spec.rb

@@ -11,6 +11,7 @@ RSpec.describe 'GPG signed commits' do
 
     perform_enqueued_jobs do
       create :gpg_key, key: GpgHelpers::User1.public_key, user: user
+      user.reload # necessary to reload the association with gpg_keys
     end
 
     visit project_commit_path(project, ref)

spec/helpers/users_helper_spec.rb

@@ -383,7 +383,7 @@ RSpec.describe UsersHelper do
     end
 
     context 'when `user.unconfirmed_email` is set' do
-      let(:user) { create(:user, unconfirmed_email: 'foo@bar.com') }
+      let(:user) { create(:user, :unconfirmed, unconfirmed_email: 'foo@bar.com') }
 
       it 'sets `modal_attributes.messageHtml` correctly' do
         expect(Gitlab::Json.parse(confirm_user_data[:modal_attributes])['messageHtml']).to eq('This user has an unconfirmed email address (foo@bar.com). You may force a confirmation.')

spec/lib/gitlab/background_migration/add_primary_email_to_emails_if_user_confirmed_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::BackgroundMigration::AddPrimaryEmailToEmailsIfUserConfirmed do
+  let(:users) { table(:users) }
+  let(:emails) { table(:emails) }
+
+  let!(:unconfirmed_user) { users.create!(name: 'unconfirmed', email: 'unconfirmed@example.com', confirmed_at: nil, projects_limit: 100) }
+  let!(:confirmed_user_1) { users.create!(name: 'confirmed-1', email: 'confirmed-1@example.com', confirmed_at: 1.day.ago, projects_limit: 100) }
+  let!(:confirmed_user_2) { users.create!(name: 'confirmed-2', email: 'confirmed-2@example.com', confirmed_at: 1.day.ago, projects_limit: 100) }
+  let!(:email) { emails.create!(user_id: confirmed_user_1.id, email: 'confirmed-1@example.com', confirmed_at: 1.day.ago) }
+
+  let(:perform) { described_class.new.perform(users.first.id, users.last.id) }
+
+  it 'adds the primary email of confirmed users to Emails, unless already added', :aggregate_failures do
+    expect(emails.where(email: [unconfirmed_user.email, confirmed_user_2.email])).to be_empty
+
+    expect { perform }.not_to raise_error
+
+    expect(emails.where(email: unconfirmed_user.email).count).to eq(0)
+    expect(emails.where(email: confirmed_user_1.email, user_id: confirmed_user_1.id).count).to eq(1)
+    expect(emails.where(email: confirmed_user_2.email, user_id: confirmed_user_2.id).count).to eq(1)
+
+    email_2 = emails.find_by(email: confirmed_user_2.email, user_id: confirmed_user_2.id)
+    expect(email_2.confirmed_at).to eq(confirmed_user_2.reload.confirmed_at)
+  end
+
+  it 'sets timestamps on the created Emails' do
+    perform
+
+    email_2 = emails.find_by(email: confirmed_user_2.email, user_id: confirmed_user_2.id)
+
+    expect(email_2.created_at).not_to be_nil
+    expect(email_2.updated_at).not_to be_nil
+  end
+
+  context 'when a range of IDs is specified' do
+    let!(:confirmed_user_3) { users.create!(name: 'confirmed-3', email: 'confirmed-3@example.com', confirmed_at: 1.hour.ago, projects_limit: 100) }
+    let!(:confirmed_user_4) { users.create!(name: 'confirmed-4', email: 'confirmed-4@example.com', confirmed_at: 1.hour.ago, projects_limit: 100) }
+
+    it 'only acts on the specified range of IDs', :aggregate_failures do
+      expect do
+        described_class.new.perform(confirmed_user_2.id, confirmed_user_3.id)
+      end.to change { Email.count }.by(2)
+      expect(emails.where(email: confirmed_user_4.email).count).to eq(0)
+    end
+  end
+end

spec/lib/gitlab/gpg/invalid_gpg_signature_updater_spec.rb

@@ -140,6 +140,8 @@ RSpec.describe Gitlab::Gpg::InvalidGpgSignatureUpdater do
           key: GpgHelpers::User1.public_key,
           user: user
 
+        user.reload # necessary to reload the association with gpg_keys
+
         expect(invalid_gpg_signature.reload.verification_status).to eq 'unverified_key'
 
         # InvalidGpgSignatureUpdater is called by the after_update hook

spec/lib/gitlab/spamcheck/client_spec.rb

@@ -82,7 +82,7 @@ RSpec.describe Gitlab::Spamcheck::Client do
     end
   end
 
-  describe '#build_user_proto_buf', :aggregate_failures do
+  describe '#build_user_protobuf', :aggregate_failures do
     it 'builds the expected protobuf object' do
       user_pb = described_class.new.send(:build_user_protobuf, user)
       expect(user_pb.username).to eq user.username

spec/lib/gitlab/x509/signature_spec.rb

@@ -12,7 +12,7 @@ RSpec.describe Gitlab::X509::Signature do
   end
 
   shared_examples "a verified signature" do
-    let_it_be(:user) { create(:user, email: X509Helpers::User1.certificate_email) }
+    let!(:user) { create(:user, email: X509Helpers::User1.certificate_email) }
 
     subject(:signature) do
       described_class.new(
@@ -30,10 +30,12 @@ RSpec.describe Gitlab::X509::Signature do
       expect(signature.verification_status).to eq(:verified)
     end
 
-    it "returns an unverified signature if the email matches but isn't confirmed" do
-      user.update!(confirmed_at: nil)
+    context "if the email matches but isn't confirmed" do
+      let!(:user) { create(:user, :unconfirmed, email: X509Helpers::User1.certificate_email) }
 
-      expect(signature.verification_status).to eq(:unverified)
+      it "returns an unverified signature" do
+        expect(signature.verification_status).to eq(:unverified)
+      end
     end
 
     it 'returns an unverified signature if email does not match' do
@@ -297,7 +299,7 @@ RSpec.describe Gitlab::X509::Signature do
     end
 
     context 'verified signature' do
-      let_it_be(:user) { create(:user, email: X509Helpers::User1.certificate_email) }
+      let_it_be(:user) { create(:user, :unconfirmed, email: X509Helpers::User1.certificate_email) }
 
       subject(:signature) do
         described_class.new(
@@ -316,52 +318,56 @@ RSpec.describe Gitlab::X509::Signature do
           allow(OpenSSL::X509::Store).to receive(:new).and_return(store)
         end
 
-        it 'returns a verified signature if email does match' do
-          expect(signature.x509_certificate).to have_attributes(certificate_attributes)
-          expect(signature.x509_certificate.x509_issuer).to have_attributes(issuer_attributes)
-          expect(signature.verified_signature).to be_truthy
-          expect(signature.verification_status).to eq(:verified)
-        end
+        context 'when user email is confirmed' do
+          before_all do
+            user.confirm
+          end
 
-        it "returns an unverified signature if the email matches but isn't confirmed" do
-          user.update!(confirmed_at: nil)
+          it 'returns a verified signature if email does match', :ggregate_failures do
+            expect(signature.x509_certificate).to have_attributes(certificate_attributes)
+            expect(signature.x509_certificate.x509_issuer).to have_attributes(issuer_attributes)
+            expect(signature.verified_signature).to be_truthy
+            expect(signature.verification_status).to eq(:verified)
+          end
 
-          expect(signature.verification_status).to eq(:unverified)
-        end
+          it 'returns an unverified signature if email does not match', :aggregate_failures do
+            signature = described_class.new(
+              X509Helpers::User1.signed_tag_signature,
+              X509Helpers::User1.signed_tag_base_data,
+              "gitlab@example.com",
+              X509Helpers::User1.signed_commit_time
+            )
+
+            expect(signature.x509_certificate).to have_attributes(certificate_attributes)
+            expect(signature.x509_certificate.x509_issuer).to have_attributes(issuer_attributes)
+            expect(signature.verified_signature).to be_truthy
+            expect(signature.verification_status).to eq(:unverified)
+          end
 
-        it 'returns an unverified signature if email does not match' do
-          signature = described_class.new(
-            X509Helpers::User1.signed_tag_signature,
-            X509Helpers::User1.signed_tag_base_data,
-            "gitlab@example.com",
-            X509Helpers::User1.signed_commit_time
-          )
+          it 'returns an unverified signature if email does match and time is wrong', :aggregate_failures do
+            signature = described_class.new(
+              X509Helpers::User1.signed_tag_signature,
+              X509Helpers::User1.signed_tag_base_data,
+              X509Helpers::User1.certificate_email,
+              Time.new(2020, 2, 22)
+            )
+
+            expect(signature.x509_certificate).to have_attributes(certificate_attributes)
+            expect(signature.x509_certificate.x509_issuer).to have_attributes(issuer_attributes)
+            expect(signature.verified_signature).to be_falsey
+            expect(signature.verification_status).to eq(:unverified)
+          end
 
-          expect(signature.x509_certificate).to have_attributes(certificate_attributes)
-          expect(signature.x509_certificate.x509_issuer).to have_attributes(issuer_attributes)
-          expect(signature.verified_signature).to be_truthy
-          expect(signature.verification_status).to eq(:unverified)
-        end
+          it 'returns an unverified signature if certificate is revoked' do
+            expect(signature.verification_status).to eq(:verified)
 
-        it 'returns an unverified signature if email does match and time is wrong' do
-          signature = described_class.new(
-            X509Helpers::User1.signed_tag_signature,
-            X509Helpers::User1.signed_tag_base_data,
-            X509Helpers::User1.certificate_email,
-            Time.new(2020, 2, 22)
-          )
+            signature.x509_certificate.revoked!
 
-          expect(signature.x509_certificate).to have_attributes(certificate_attributes)
-          expect(signature.x509_certificate.x509_issuer).to have_attributes(issuer_attributes)
-          expect(signature.verified_signature).to be_falsey
-          expect(signature.verification_status).to eq(:unverified)
+            expect(signature.verification_status).to eq(:unverified)
+          end
         end
 
-        it 'returns an unverified signature if certificate is revoked' do
-          expect(signature.verification_status).to eq(:verified)
-
-          signature.x509_certificate.revoked!
-
+        it 'returns an unverified signature if the email matches but is not confirmed' do
           expect(signature.verification_status).to eq(:unverified)
         end
       end

spec/migrations/schedule_add_primary_email_to_emails_if_user_confirmed_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require_migration!
+
+RSpec.describe ScheduleAddPrimaryEmailToEmailsIfUserConfirmed, :sidekiq do
+  let(:migration) { described_class.new }
+  let(:users) { table(:users) }
+
+  let!(:user_1) { users.create!(name: 'confirmed-user-1', email: 'confirmed-1@example.com', confirmed_at: 1.day.ago, projects_limit: 100) }
+  let!(:user_2) { users.create!(name: 'confirmed-user-2', email: 'confirmed-2@example.com', confirmed_at: 1.day.ago, projects_limit: 100) }
+  let!(:user_3) { users.create!(name: 'confirmed-user-3', email: 'confirmed-3@example.com', confirmed_at: 1.day.ago, projects_limit: 100) }
+  let!(:user_4) { users.create!(name: 'confirmed-user-4', email: 'confirmed-4@example.com', confirmed_at: 1.day.ago, projects_limit: 100) }
+
+  before do
+    stub_const("#{described_class.name}::BATCH_SIZE", 2)
+    stub_const("#{described_class.name}::INTERVAL", 2.minutes.to_i)
+  end
+
+  it 'schedules addition of primary email to emails in delayed batches' do
+    Sidekiq::Testing.fake! do
+      freeze_time do
+        migration.up
+
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(2.minutes, user_1.id, user_2.id)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(4.minutes, user_3.id, user_4.id)
+        expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+      end
+    end
+  end
+end

spec/models/email_spec.rb

@@ -13,6 +13,15 @@ RSpec.describe Email do
     it_behaves_like 'an object with RFC3696 compliant email-formatted attributes', :email do
       subject { build(:email) }
     end
+
+    context 'when the email conflicts with the primary email of a different user' do
+      let(:user) { create(:user) }
+      let(:email) { build(:email, email: user.email) }
+
+      it 'is invalid' do
+        expect(email).to be_invalid
+      end
+    end
   end
 
   it 'normalize email value' do
@@ -33,7 +42,7 @@ RSpec.describe Email do
   end
 
   describe 'scopes' do
-    let(:user) { create(:user) }
+    let(:user) { create(:user, :unconfirmed) }
 
     it 'scopes confirmed emails' do
       create(:email, :confirmed, user: user)

spec/models/user_spec.rb

@@ -1123,7 +1123,7 @@ RSpec.describe User do
   end
 
   describe 'after commit hook' do
-    describe '#update_emails_with_primary_email' do
+    describe 'when the primary email is updated' do
       before do
         @user = create(:user, email: 'primary@example.com').tap do |user|
           user.skip_reconfirmation!
@@ -1132,13 +1132,7 @@ RSpec.describe User do
         @user.reload
       end
 
-      it 'gets called when email updated' do
-        expect(@user).to receive(:update_emails_with_primary_email)
-
-        @user.update!(email: 'new_primary@example.com')
-      end
-
-      it 'adds old primary to secondary emails when secondary is a new email' do
+      it 'keeps old primary to secondary emails when secondary is a new email' do
         @user.update!(email: 'new_primary@example.com')
         @user.reload
 
@@ -1146,22 +1140,6 @@ RSpec.describe User do
         expect(@user.emails.pluck(:email)).to match_array([@secondary.email, 'primary@example.com'])
       end
 
-      it 'adds old primary to secondary emails if secondary is becoming a primary' do
-        @user.update!(email: @secondary.email)
-        @user.reload
-
-        expect(@user.emails.count).to eq 1
-        expect(@user.emails.first.email).to eq 'primary@example.com'
-      end
-
-      it 'transfers old confirmation values into new secondary' do
-        @user.update!(email: @secondary.email)
-        @user.reload
-
-        expect(@user.emails.count).to eq 1
-        expect(@user.emails.first.confirmed_at).not_to eq nil
-      end
-
       context 'when the first email was unconfirmed and the second email gets confirmed' do
         let(:user) { create(:user, :unconfirmed, email: 'should-be-unconfirmed@test.com') }
 
@@ -1178,11 +1156,8 @@ RSpec.describe User do
           expect(user).to be_confirmed
         end
 
-        it 'keeps the unconfirmed email unconfirmed' do
-          email = user.emails.first
-
-          expect(email.email).to eq('should-be-unconfirmed@test.com')
-          expect(email).not_to be_confirmed
+        it 'does not add unconfirmed email to secondary' do
+          expect(user.emails.map(&:email)).not_to include('should-be-unconfirmed@test.com')
         end
 
         it 'has only one email association' do
@@ -1244,7 +1219,7 @@ RSpec.describe User do
           expect(user.email).to eq(confirmed_email)
         end
 
-        it 'moves the old email' do
+        it 'keeps the old email' do
           email = user.reload.emails.first
 
           expect(email.email).to eq(old_confirmed_email)
@@ -1499,7 +1474,7 @@ RSpec.describe User do
       allow_any_instance_of(ApplicationSetting).to receive(:send_user_confirmation_email).and_return(true)
     end
 
-    let(:user) { create(:user, confirmed_at: nil, unconfirmed_email: 'test@gitlab.com') }
+    let(:user) { create(:user, :unconfirmed, unconfirmed_email: 'test@gitlab.com') }
 
     it 'returns unconfirmed' do
       expect(user.confirmed?).to be_falsey
@@ -1509,6 +1484,22 @@ RSpec.describe User do
       user.confirm
       expect(user.confirmed?).to be_truthy
     end
+
+    it 'adds the confirmed primary email to emails' do
+      expect(user.emails.confirmed.map(&:email)).not_to include(user.email)
+
+      user.confirm
+
+      expect(user.emails.confirmed.map(&:email)).to include(user.email)
+    end
+  end
+
+  context 'if the user is created with confirmed_at set to a time' do
+    let!(:user) { create(:user, email: 'test@gitlab.com', confirmed_at: Time.now.utc) }
+
+    it 'adds the confirmed primary email to emails upon creation' do
+      expect(user.emails.confirmed.map(&:email)).to include(user.email)
+    end
   end
 
   describe '#to_reference' do
@@ -2216,7 +2207,7 @@ RSpec.describe User do
       end
 
       context 'primary email not confirmed' do
-        let(:user) { create(:user, confirmed_at: nil) }
+        let(:user) { create(:user, :unconfirmed) }
         let!(:email) { create(:email, :confirmed, user: user, email: 'foo@example.com') }
 
         it 'finds user respecting the confirmed flag' do
@@ -2231,7 +2222,7 @@ RSpec.describe User do
     end
 
     it 'returns nil when user is not confirmed' do
-      user = create(:user, email: 'foo@example.com', confirmed_at: nil)
+      user = create(:user, :unconfirmed, email: 'foo@example.com')
 
       expect(described_class.find_by_any_email(user.email, confirmed: false)).to eq(user)
       expect(described_class.find_by_any_email(user.email, confirmed: true)).to be_nil
@@ -6011,7 +6002,7 @@ RSpec.describe User do
     subject { user.confirmation_required_on_sign_in? }
 
     context 'when user is confirmed' do
-      let(:user) { build_stubbed(:user) }
+      let(:user) { create(:user) }
 
       it 'is falsey' do
         expect(user.confirmed?).to be_truthy

spec/requests/api/users_spec.rb

@@ -1906,7 +1906,8 @@ RSpec.describe API::Users do
         expect(response).to have_gitlab_http_status(:ok)
         expect(response).to include_pagination_headers
         expect(json_response).to be_an Array
-        expect(json_response.first['email']).to eq(email.email)
+        expect(json_response.first['email']).to eq(user.email)
+        expect(json_response.second['email']).to eq(email.email)
       end
 
       it "returns a 404 for invalid ID" do
@@ -2488,7 +2489,8 @@ RSpec.describe API::Users do
         expect(response).to have_gitlab_http_status(:ok)
         expect(response).to include_pagination_headers
         expect(json_response).to be_an Array
-        expect(json_response.first["email"]).to eq(email.email)
+        expect(json_response.first['email']).to eq(user.email)
+        expect(json_response.second['email']).to eq(email.email)
       end
 
       context "scopes" do

spec/requests/users_controller_spec.rb

@@ -305,7 +305,7 @@ RSpec.describe UsersController do
 
     context 'user with keys' do
       let!(:gpg_key) { create(:gpg_key, user: user) }
-      let!(:another_gpg_key) { create(:another_gpg_key, user: user) }
+      let!(:another_gpg_key) { create(:another_gpg_key, user: user.reload) }
 
       shared_examples_for 'renders all verified GPG keys' do
         it 'renders all verified keys separated with a new line with text/plain content type' do

spec/services/emails/create_service_spec.rb

@@ -3,7 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Emails::CreateService do
-  let(:user) { create(:user) }
+  let_it_be(:user) { create(:user) }
+
   let(:opts) { { email: 'new@email.com', user: user } }
 
   subject(:service) { described_class.new(user, opts) }
@@ -22,7 +23,7 @@ RSpec.describe Emails::CreateService do
     it 'has the right user association' do
       service.execute
 
-      expect(user.emails).to eq(Email.where(opts))
+      expect(user.emails).to include(Email.find_by(opts))
     end
   end
 end

spec/services/emails/destroy_service_spec.rb

@@ -15,5 +15,15 @@ RSpec.describe Emails::DestroyService do
       expect(user.emails).not_to include(email)
       expect(response).to be true
     end
+
+    context 'when it corresponds to the user primary email' do
+      let(:email) { user.emails.find_by!(email: user.email) }
+
+      it 'does not remove the email and raises an exception' do
+        expect { service.execute(email) }.to raise_error(StandardError, 'Cannot delete primary email')
+
+        expect(user.emails).to include(email)
+      end
+    end
   end
 end