Remove callbacks on secondary emails

The 3 User "secondary emails each have default behavior. Previously
this behavior was managed through a complex set of model callbacks that
required workarounds to have the correct behavior. That complexity
fostered multiple security issues. We have refactored the default
behavior to exist only in custom getter methods, so there is no more
need to use callbacks to manipulate the database values. This removes
the callbacks, because they no longer have any effect.

app/models/user.rb

@@ -256,10 +256,7 @@ class User < ApplicationRecord
   validates :website_url, allow_blank: true, url: true, if: :website_url_changed?
 
   before_validation :sanitize_attrs
-  before_validation :reset_secondary_emails, if: :email_changed?
   before_save :default_private_profile_to_false
-  before_save :set_public_email, if: :public_email_changed? # in case validation is skipped
-  before_save :set_commit_email, if: :commit_email_changed? # in case validation is skipped
   before_save :ensure_incoming_email_token
   before_save :ensure_user_rights_and_limits, if: ->(user) { user.new_record? || user.external_changed? }
   before_save :skip_reconfirmation!, if: ->(user) { user.email_changed? && user.read_only_attribute?(:email) }
@@ -2029,30 +2026,6 @@ class User < ApplicationRecord
     errors.add(:commit_email, _("must be an email you have verified")) unless verified_emails.include?(commit_email)
   end
 
-  def set_notification_email
-    if verified_emails.exclude?(notification_email)
-      self.notification_email = nil
-    end
-  end
-
-  def set_public_email
-    if verified_emails.exclude?(public_email)
-      self.public_email = ''
-    end
-  end
-
-  def set_commit_email
-    if verified_emails.exclude?(commit_email)
-      self.commit_email = nil
-    end
-  end
-
-  def reset_secondary_emails
-    set_public_email
-    set_commit_email
-    set_notification_email
-  end
-
   def callouts_by_feature_name
     @callouts_by_feature_name ||= callouts.index_by(&:feature_name)
   end

spec/features/atom/merge_requests_spec.rb

@@ -4,8 +4,8 @@ require 'spec_helper'
 
 RSpec.describe 'Merge Requests Feed' do
   describe 'GET /merge_requests' do
-    let_it_be_with_reload(:user) { create(:user, email: 'private1@example.com', public_email: 'public1@example.com') }
-    let_it_be(:assignee) { create(:user, email: 'private2@example.com', public_email: 'public2@example.com') }
+    let_it_be_with_reload(:user) { create(:user, email: 'private1@example.com') }
+    let_it_be(:assignee) { create(:user, email: 'private2@example.com') }
     let_it_be(:group) { create(:group) }
     let_it_be(:project) { create(:project, :repository) }
     let_it_be(:merge_request) { create(:merge_request, source_project: project, assignees: [assignee]) }

spec/models/user_spec.rb

@@ -681,6 +681,22 @@ RSpec.describe User do
           end
         end
       end
+
+      context 'when secondary email is same as primary' do
+        let(:user) { create(:user, email: 'user@example.com') }
+
+        it 'lets user change primary email without failing validations' do
+          user.commit_email = user.email
+          user.notification_email = user.email
+          user.public_email = user.email
+          user.save!
+
+          user.email = 'newemail@example.com'
+          user.confirm
+
+          expect(user).to be_valid
+        end
+      end
     end
   end