Fix issue #6847 (#6905)

* Fix issue #6847

app/controllers/users_controller.rb

@@ -25,49 +25,50 @@ def update
       # change email notifications
       if u[:email_preferences]
         @user.update_user_preferences(u[:email_preferences])
-        flash[:notice] = I18n.t 'users.update.email_notifications_changed'
+        flash[:notice] = I18n.t "users.update.email_notifications_changed"
       # change password
       elsif params[:change_password]
         if @user.update_with_password(u)
           password_changed = true
-          flash[:notice] = I18n.t 'users.update.password_changed'
+          flash[:notice] = I18n.t "users.update.password_changed"
         else
-          flash[:error] = I18n.t 'users.update.password_not_changed'
+          flash[:error] = I18n.t "users.update.password_not_changed"
         end
       elsif u[:show_community_spotlight_in_stream] || u[:getting_started]
         if @user.update_attributes(u)
-          flash[:notice] = I18n.t 'users.update.settings_updated'
+          flash[:notice] = I18n.t "users.update.settings_updated"
         else
-          flash[:notice] = I18n.t 'users.update.settings_not_updated'
+          flash[:notice] = I18n.t "users.update.settings_not_updated"
         end
       elsif u[:strip_exif]
         if @user.update_attributes(u)
-          flash[:notice] = I18n.t 'users.update.settings_updated'
+          flash[:notice] = I18n.t "users.update.settings_updated"
         else
-          flash[:notice] = I18n.t 'users.update.settings_not_updated'
+          flash[:notice] = I18n.t "users.update.settings_not_updated"
         end
       elsif u[:language]
         if @user.update_attributes(u)
           I18n.locale = @user.language
-          flash[:notice] = I18n.t 'users.update.language_changed'
+          flash[:notice] = I18n.t "users.update.language_changed"
         else
-          flash[:error] = I18n.t 'users.update.language_not_changed'
+          flash[:error] = I18n.t "users.update.language_not_changed"
         end
       elsif u[:email]
         @user.unconfirmed_email = u[:email]
         if @user.save
           @user.send_confirm_email
           if @user.unconfirmed_email
-            flash[:notice] = I18n.t 'users.update.unconfirmed_email_changed'
+            flash[:notice] = I18n.t "users.update.unconfirmed_email_changed"
           end
         else
-          flash[:error] = I18n.t 'users.update.unconfirmed_email_not_changed'
+          @user.reload # match user object with the database
+          flash[:error] = I18n.t "users.update.unconfirmed_email_not_changed"
         end
       elsif u[:auto_follow_back]
         if  @user.update_attributes(u)
-          flash[:notice] = I18n.t 'users.update.follow_settings_changed'
+          flash[:notice] = I18n.t "users.update.follow_settings_changed"
         else
-          flash[:error] = I18n.t 'users.update.follow_settings_not_changed'
+          flash[:error] = I18n.t "users.update.follow_settings_not_changed"
         end
       elsif u[:color_theme]
         if @user.update_attributes(u)
@@ -98,9 +99,9 @@ def destroy
       redirect_to(new_user_session_path(format: request[:format]), notice: I18n.t("users.destroy.success"))
     else
       if params[:user].present? && params[:user][:current_password].present?
-        flash[:error] = t 'users.destroy.wrong_password'
+        flash[:error] = t "users.destroy.wrong_password"
       else
-        flash[:error] = t 'users.destroy.no_password'
+        flash[:error] = t "users.destroy.no_password"
       end
       redirect_to :back
     end
@@ -111,16 +112,16 @@ def public
       respond_to do |format|
         format.atom do
           @posts = Post.where(author_id: @user.person_id, public: true)
-                    .order('created_at DESC')
-                    .limit(25)
-                    .map {|post| post.is_a?(Reshare) ? post.absolute_root : post }
-                    .compact
+                       .order("created_at DESC")
+                       .limit(25)
+                       .map {|post| post.is_a?(Reshare) ? post.absolute_root : post }
+                       .compact
         end
 
         format.any { redirect_to person_path(@user.person) }
       end
     else
-      redirect_to stream_path, :error => I18n.t('users.public.does_not_exist', :username => params[:username])
+      redirect_to stream_path, error: I18n.t("users.public.does_not_exist", username: params[:username])
     end
   end
 
@@ -141,7 +142,7 @@ def getting_started_completed
 
   def export_profile
     current_user.queue_export
-    flash[:notice] = I18n.t('users.edit.export_in_progress')
+    flash[:notice] = I18n.t("users.edit.export_in_progress")
     redirect_to edit_user_path
   end
 
@@ -151,7 +152,7 @@ def download_profile
 
   def export_photos
     current_user.queue_export_photos
-    flash[:notice] = I18n.t('users.edit.export_photos_in_progress')
+    flash[:notice] = I18n.t("users.edit.export_photos_in_progress")
     redirect_to edit_user_path
   end
 
@@ -171,9 +172,9 @@ def user_photo
 
   def confirm_email
     if current_user.confirm_email(params[:token])
-      flash[:notice] = I18n.t('users.confirm_email.email_confirmed', :email => current_user.email)
+      flash[:notice] = I18n.t("users.confirm_email.email_confirmed", email: current_user.email)
     elsif current_user.unconfirmed_email.present?
-      flash[:error] = I18n.t('users.confirm_email.email_not_confirmed')
+      flash[:error] = I18n.t("users.confirm_email.email_not_confirmed")
     end
     redirect_to edit_user_path
   end

app/models/user.rb

@@ -32,6 +32,8 @@ class User < ActiveRecord::Base
   validates :color_theme, inclusion: {in: AVAILABLE_COLOR_THEME_CODES}, allow_blank: true
   validates_format_of :unconfirmed_email, :with  => Devise.email_regexp, :allow_blank => true
 
+  validate :unconfirmed_email_quasiuniqueness
+
   validates_presence_of :person, :unless => proc {|user| user.invitation_token.present?}
   validates_associated :person
   validate :no_person_with_same_username
@@ -83,6 +85,8 @@ class User < ActiveRecord::Base
 
   before_save :guard_unconfirmed_email
 
+  after_save :remove_invalid_unconfirmed_emails
+
   def self.all_sharing_with_person(person)
     User.joins(:contacts).where(:contacts => {:person_id => person.id})
   end
@@ -484,6 +488,13 @@ def mine?(target)
   end
 
 
+  # Ensure that the unconfirmed email isn't already someone's email
+  def unconfirmed_email_quasiuniqueness
+    if User.exists?(["id != ? AND email = ?", id, unconfirmed_email])
+      errors.add(:unconfirmed_email, I18n.t("errors.messages.taken"))
+    end
+  end
+
   def guard_unconfirmed_email
     self.unconfirmed_email = nil if unconfirmed_email.blank? || unconfirmed_email == email
 
@@ -492,11 +503,16 @@ def guard_unconfirmed_email
     end
   end
 
+  # Whenever email is set, clear all unconfirmed emails which match
+  def remove_invalid_unconfirmed_emails
+    User.where(unconfirmed_email: email).update_all(unconfirmed_email: nil) if email_changed?
+  end
+
   # Generate public/private keys for User and associated Person
   def generate_keys
-    key_size = (Rails.env == 'test' ? 512 : 4096)
+    key_size = (Rails.env == "test" ? 512 : 4096)
 
-    self.serialized_private_key = OpenSSL::PKey::RSA::generate(key_size).to_s if self.serialized_private_key.blank?
+    self.serialized_private_key = OpenSSL::PKey::RSA.generate(key_size).to_s if serialized_private_key.blank?
 
     if self.person && self.person.serialized_public_key.blank?
       self.person.serialized_public_key = OpenSSL::PKey::RSA.new(self.serialized_private_key).public_key.to_s

spec/models/user_spec.rb

@@ -305,6 +305,13 @@
         alice.email = "somebody@anywhere"
         expect(alice).not_to be_valid
       end
+
+      it "resets a matching unconfirmed_email on save" do
+        eve.update_attribute :unconfirmed_email, "[email protected]"
+        alice.update_attribute :email, "[email protected]"
+        eve.reload
+        expect(eve.unconfirmed_email).to eql(nil)
+      end
     end
 
     describe "of unconfirmed_email" do
@@ -316,11 +323,16 @@
       end
 
       it "does NOT require a unique unconfirmed_email address" do
-        eve.update_attribute :unconfirmed_email, "new@email.com"
-        alice.unconfirmed_email = "new@email.com"
+        eve.update_attribute :unconfirmed_email, "new@example.com"
+        alice.unconfirmed_email = "new@example.com"
         expect(alice).to be_valid
       end
 
+      it "requires an unconfirmed_email address which is not another user's email address" do
+        alice.unconfirmed_email = eve.email
+        expect(alice).not_to be_valid
+      end
+
       it "requires a valid unconfirmed_email address" do
         alice.unconfirmed_email = "somebody@anywhere"
         expect(alice).not_to be_valid