allow a verified secondary email to be use as the primary without

a reconfirmation

5 files changed, 42 insertions, 7 deletions

diff --git a/app/models/user.rb b/app/models/user.rb
index 88dc5565a3a..c75107472d6 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -161,15 +161,16 @@ class User < ActiveRecord::Base
   before_validation :sanitize_attrs
   before_validation :set_notification_email, if: :email_changed?
   before_validation :set_public_email, if: :public_email_changed?
-
-  after_update :update_emails_with_primary_email, if: :email_changed?
   before_save :ensure_authentication_token, :ensure_incoming_email_token
   before_save :ensure_user_rights_and_limits, if: :external_changed?
   before_save :skip_reconfirmation!, if: ->(user) { user.email_changed? && user.read_only_attribute?(:email) }
+  before_save :check_for_verified_email, if: ->(user) { user.email_changed? && !user.new_record?}
   after_save :ensure_namespace_correct
+  after_destroy :post_destroy_hook
+  after_commit :update_emails_with_primary_email, on: :update, if: -> { previous_changes.key?('email') }
   after_commit :update_invalid_gpg_signatures, on: :update, if: -> { previous_changes.key?('email') }
+
   after_initialize :set_projects_limit
-  after_destroy :post_destroy_hook
 
   # User's Layout preference
   enum layout: [:fixed, :fluid]
@@ -222,6 +223,11 @@ class User < ActiveRecord::Base
     end
   end
 
+  # see if the new email is already a verified secondary email 
+  def check_for_verified_email
+    skip_reconfirmation! if emails.find_by(email: self.email).try(:confirmed?)
+  end
+
   mount_uploader :avatar, AvatarUploader
   has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
@@ -523,14 +529,18 @@ class User < ActiveRecord::Base
     errors.add(:public_email, "is not an email you own") unless all_emails.include?(public_email)
   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 getting cleared.  
   def update_emails_with_primary_email
     primary_email_record = emails.find_by(email: email)
     if primary_email_record
+      previous_email = previous_changes[:email][0]
       Emails::DestroyService.new(self).execute(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, email: email_was).execute(confirmed_at: confirmed_at_was)
+      Emails::CreateService.new(self, email: previous_email).execute(confirmed_at: confirmed_at)
     end
   end
 
diff --git a/spec/controllers/profiles_controller_spec.rb b/spec/controllers/profiles_controller_spec.rb
index b52b63e05a4..3fb631a6fac 100644
--- a/spec/controllers/profiles_controller_spec.rb
+++ b/spec/controllers/profiles_controller_spec.rb
@@ -15,6 +15,20 @@ describe ProfilesController do
       expect(user.unconfirmed_email).to eq('john@gmail.com')
     end
 
+    it "allows an email update without confirmation if existing verified email" do
+      user = create(:user)
+      email = create(:email, :confirmed, user: user, email: 'john@gmail.com')
+      sign_in(user)
+
+      put :update,
+          user: { email: "john@gmail.com", name: "John" }
+
+      user.reload
+
+      expect(response.status).to eq(302)
+      expect(user.unconfirmed_email).to eq nil
+    end
+
     it "ignores an email update from a user with an external email address" do
       stub_omniauth_setting(sync_profile_from_provider: ['ldap'])
       stub_omniauth_setting(sync_profile_attributes: true)
diff --git a/spec/features/profiles/emails_spec.rb b/spec/features/profiles/emails_spec.rb
index 3085e1ee7bc..11cc8aae6f3 100644
--- a/spec/features/profiles/emails_spec.rb
+++ b/spec/features/profiles/emails_spec.rb
@@ -4,7 +4,7 @@ feature 'Profile > Emails' do
   let(:user) { create(:user) }
 
   before do
-    login_as(user)
+    sign_in(user)
   end
 
   describe 'User adds an email' do
diff --git a/spec/models/email_spec.rb b/spec/models/email_spec.rb
index 8fca9db37ca..8ae5ccd89ed 100644
--- a/spec/models/email_spec.rb
+++ b/spec/models/email_spec.rb
@@ -24,8 +24,6 @@ describe Email do
       email = user.emails.create(email: 'new@email.com')
       expect(user).to receive(:update_invalid_gpg_signatures)
       email.confirm
-      # email.save
     end
   end
-
 end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index a230f72449a..c6df8028072 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -359,6 +359,19 @@ describe User do
         expect(external_user.projects_limit).to be 0
       end
     end
+    
+    describe '#check_for_verified_email' do
+      let(:user)      { create(:user) }
+      let(:secondary) { create(:email, :confirmed, email: 'secondary@example.com', user: user, ) }
+      
+      it 'allows a verfied secondary email to be used as the primary without needing reconfirmation' do
+        user.update_attributes!(email: secondary.email)
+        user.reload
+        expect(user.email).to eq secondary.email
+        expect(user.unconfirmed_email).to eq nil
+        expect(user.confirmed?).to be_truthy
+      end
+    end
   end
 
   describe 'after update hook' do