SECURITY: Support for confirm old as well as new email accounts

app/controllers/admin/email_templates_controller.rb

@@ -20,7 +20,8 @@ class Admin::EmailTemplatesController < Admin::AdminController
                      "system_messages.unblocked", "system_messages.user_automatically_blocked",
                      "system_messages.welcome_invite", "system_messages.welcome_user", "test_mailer",
                      "user_notifications.account_created", "user_notifications.admin_login",
-                     "user_notifications.authorize_email", "user_notifications.forgot_password",
+                     "user_notifications.confirm_new_email", "user_notifications.confirm_old_email",
+                     "user_notifications.notify_old_email", "user_notifications.forgot_password",
                      "user_notifications.set_password", "user_notifications.signup",
                      "user_notifications.signup_after_approval",
                      "user_notifications.user_invited_to_private_message_pm",

app/controllers/users_controller.rb

@@ -5,7 +5,7 @@ require_dependency 'rate_limiter'
 class UsersController < ApplicationController
 
   skip_before_filter :authorize_mini_profiler, only: [:avatar]
-  skip_before_filter :check_xhr, only: [:show, :password_reset, :update, :account_created, :activate_account, :perform_account_activation, :authorize_email, :user_preferences_redirect, :avatar, :my_redirect, :toggle_anon, :admin_login]
+  skip_before_filter :check_xhr, only: [:show, :password_reset, :update, :account_created, :activate_account, :perform_account_activation, :user_preferences_redirect, :avatar, :my_redirect, :toggle_anon, :admin_login]
 
   before_filter :ensure_logged_in, only: [:username, :update, :user_preferences_redirect, :upload_user_image, :pick_avatar, :destroy_user_image, :destroy, :check_emails]
   before_filter :respond_to_suspicious_request, only: [:create]
@@ -21,7 +21,6 @@ class UsersController < ApplicationController
                                                             :activate_account,
                                                             :perform_account_activation,
                                                             :send_activation_email,
-                                                            :authorize_email,
                                                             :password_reset,
                                                             :confirm_email_token,
                                                             :admin_login]
@@ -471,16 +470,6 @@ class UsersController < ApplicationController
     end
   end
 
-  def authorize_email
-    expires_now()
-    if @user = EmailToken.confirm(params[:token])
-      log_on_user(@user)
-    else
-      flash[:error] = I18n.t('change_email.error')
-    end
-    render layout: 'no_ember'
-  end
-
   def account_created
     @message = session['user_created_message'] || I18n.t('activation.missing_session')
     expires_now

app/controllers/users_email_controller.rb

@@ -1,9 +1,13 @@
 require_dependency 'rate_limiter'
 require_dependency 'email_validator'
+require_dependency 'email_updater'
 
 class UsersEmailController < ApplicationController
 
-  before_filter :ensure_logged_in
+  before_filter :ensure_logged_in, only: [:index, :update]
+
+  skip_before_filter :check_xhr, only: [:confirm]
+  skip_before_filter :redirect_to_login_if_required, only: [:confirm]
 
   def index
   end
@@ -11,31 +15,32 @@ class UsersEmailController < ApplicationController
   def update
     params.require(:email)
     user = fetch_user_from_params
-    guardian.ensure_can_edit_email!(user)
-    lower_email = Email.downcase(params[:email]).strip
 
     RateLimiter.new(user, "change-email-hr-#{request.remote_ip}", 6, 1.hour).performed!
     RateLimiter.new(user, "change-email-min-#{request.remote_ip}", 3, 1.minute).performed!
 
-    EmailValidator.new(attributes: :email).validate_each(user, :email, lower_email)
-    return render_json_error(user.errors.full_messages) if user.errors[:email].present?
+    updater = EmailUpdater.new(guardian, user)
+    updater.change_to(params[:email])
 
-    # Raise an error if the email is already in use
-    return render_json_error(I18n.t('change_email.error')) if User.find_by_email(lower_email)
-
-    email_token = user.email_tokens.create(email: lower_email)
-    Jobs.enqueue(
-      :user_email,
-      to_address: lower_email,
-      type: :authorize_email,
-      user_id: user.id,
-      email_token: email_token.token
-    )
+    if updater.errors.present?
+      return render_json_error(updater.errors.full_messages)
+    end
 
     render nothing: true
   rescue RateLimiter::LimitExceeded
     render_json_error(I18n.t("rate_limiter.slow_down"))
   end
 
+  def confirm
+    expires_now
+    updater = EmailUpdater.new
+    @update_result = updater.confirm(params[:token])
+
+    # Log in the user if the process is complete (and they're not logged in)
+    log_on_user(updater.user) if @update_result == :complete
+
+    render layout: 'no_ember'
+  end
+
 end
 

app/jobs/regular/user_email.rb

@@ -109,6 +109,10 @@ module Jobs
         email_args[:email_token] = email_token
       end
 
+      if type == 'notify_old_email'
+        email_args[:new_email] = user.email
+      end
+
       message = UserNotifications.send(type, user, email_args)
 
       # Update the to address if we have a custom one

app/mailers/user_notifications.rb

@@ -23,9 +23,23 @@ class UserNotifications < ActionMailer::Base
                 new_user_tips: I18n.t('system_messages.usage_tips.text_body_template', base_url: Discourse.base_url, locale: locale))
   end
 
-  def authorize_email(user, opts={})
+  def notify_old_email(user, opts={})
     build_email(user.email,
-                template: "user_notifications.authorize_email",
+                template: "user_notifications.notify_old_email",
+                locale: user_locale(user),
+                new_email: opts[:new_email])
+  end
+
+  def confirm_old_email(user, opts={})
+    build_email(user.email,
+                template: "user_notifications.confirm_old_email",
+                locale: user_locale(user),
+                email_token: opts[:email_token])
+  end
+
+  def confirm_new_email(user, opts={})
+    build_email(user.email,
+                template: "user_notifications.confirm_new_email",
                 locale: user_locale(user),
                 email_token: opts[:email_token])
   end

app/models/email_change_request.rb

@@ -0,0 +1,9 @@
+class EmailChangeRequest < ActiveRecord::Base
+  belongs_to :old_email_token, class_name: 'EmailToken'
+  belongs_to :new_email_token, class_name: 'EmailToken'
+
+  def self.states
+    @states ||= Enum.new(authorizing_old: 1, authorizing_new: 2, complete: 3)
+  end
+
+end

app/models/email_token.rb

@@ -41,28 +41,41 @@ class EmailToken < ActiveRecord::Base
     return token.present? && token =~ /[a-f0-9]{#{token.length/2}}/i
   end
 
-  def self.confirm(token)
-    return unless valid_token_format?(token)
+  def self.atomic_confirm(token)
+    failure = { success: false }
+    return failure unless valid_token_format?(token)
 
     email_token = confirmable(token)
-    return if email_token.blank?
+    return failure if email_token.blank?
 
     user = email_token.user
+    failure[:user] = user
+    row_count = EmailToken.where(id: email_token.id, expired: false).update_all 'confirmed = true'
+    if row_count == 1
+      return { success: true, user: user, email_token: email_token }
+    end
+
+    return failure
+  end
+
+  def self.confirm(token)
     User.transaction do
-      row_count = EmailToken.where(id: email_token.id, expired: false).update_all 'confirmed = true'
-      if row_count == 1
+      result = atomic_confirm(token)
+      user = result[:user]
+      if result[:success]
         # If we are activating the user, send the welcome message
         user.send_welcome_message = !user.active?
 
         user.active = true
-        user.email = email_token.email
+        user.email = result[:email_token].email
         user.save!
       end
-    end
 
-    # redeem invite, if available
-    return User.find_by(email: Email.downcase(user.email)) if Invite.redeem_from_email(user.email).present?
-    user
+      if user
+        return User.find_by(email: Email.downcase(user.email)) if Invite.redeem_from_email(user.email).present?
+        user
+      end
+    end
   rescue ActiveRecord::RecordInvalid
     # If the user's email is already taken, just return nil (failure)
   end

app/models/user.rb

@@ -36,6 +36,7 @@ class User < ActiveRecord::Base
   has_many :uploads
   has_many :warnings
   has_many :user_archived_messages, dependent: :destroy
+  has_many :email_change_requests, dependent: :destroy
 
 
   has_one :user_option, dependent: :destroy

app/views/users/authorize_email.html.erb

@@ -1,12 +0,0 @@
-<div id="simple-container">
-  <%if flash[:error]%>
-    <div class='alert alert-error'>
-      <%=flash[:error]%>
-    </div>
-  <%else%>
-    <h2><%= t 'change_email.confirmed' %></h2>
-    <br>
-    <a class="btn" href="/"><%= t('change_email.please_continue', site_name: SiteSetting.title) %></a>
-    <%= render partial: 'auto_redirect_home' %>
-  <%end%>
-</div>

app/views/users_email/confirm.html.erb

@@ -0,0 +1,15 @@
+<div id="simple-container">
+  <% if @update_result == :authorizing_new %>
+    <h2><%= t 'change_email.authorizing_old.title' %></h2>
+    <br>
+    <p><%= t 'change_email.authorizing_old.description' %></p>
+  <% elsif @update_result == :complete %>
+    <h2><%= t 'change_email.confirmed' %></h2>
+    <br>
+    <a class="btn" href="/"><%= t('change_email.please_continue', site_name: SiteSetting.title) %></a>
+  <% else %>
+    <div class='alert alert-error'>
+      <%=t 'change_email.error' %>
+    </div>
+  <% end %>
+</div>