DEV: Update confirm-email flows to use central 2fa and ember rendering (#25404)

These routes were previously rendered using Rails, and had a fairly fragile 2fa implementation in vanilla-js. This commit refactors the routes to be handled in the Ember app, removes the custom vanilla-js bundles, and leans on our centralized 2fa implementation. It also introduces a set of system specs for the behavior.

app/assets/javascripts/confirm-new-email/bootstrap.js

@@ -1,4 +0,0 @@
-// discourse-skip-module
-(function () {
-  require("confirm-new-email/confirm-new-email");
-})();

app/assets/javascripts/confirm-new-email/confirm-new-email.js

@@ -1,27 +0,0 @@
-import { getWebauthnCredential } from "discourse/lib/webauthn";
-
-const security = document.getElementById("submit-security-key");
-if (security) {
-  security.onclick = function (e) {
-    e.preventDefault();
-    getWebauthnCredential(
-      document.getElementById("security-key-challenge").value,
-      document
-        .getElementById("security-key-allowed-credential-ids")
-        .value.split(","),
-      (credentialData) => {
-        document.getElementById("security-key-credential").value =
-          JSON.stringify(credentialData);
-
-        e.target.closest("form").submit();
-      },
-      (errorMessage) => {
-        document.getElementById(
-          "security-key-error"
-        ).innerHTML = `<div class="alert alert-error"></div>`;
-        document.querySelector("#security-key-error .alert").innerText =
-          errorMessage;
-      }
-    );
-  };
-}

app/assets/javascripts/discourse/app/controllers/confirm-new-email.js

@@ -0,0 +1,46 @@
+import { tracked } from "@glimmer/tracking";
+import Controller from "@ember/controller";
+import { action } from "@ember/object";
+import { inject as service } from "@ember/service";
+import { ajax } from "discourse/lib/ajax";
+import { popupAjaxError } from "discourse/lib/ajax-error";
+import I18n from "discourse-i18n";
+
+export default class ConfirmNewEmailController extends Controller {
+  @service dialog;
+  @service router;
+
+  @tracked loading;
+
+  @action
+  async confirm() {
+    this.loading = true;
+    try {
+      await ajax(`/u/confirm-new-email/${this.model.token}.json`, {
+        type: "PUT",
+      });
+    } catch (error) {
+      const nonce = error.jqXHR?.responseJSON?.second_factor_challenge_nonce;
+      if (nonce) {
+        this.router.transitionTo("second-factor-auth", {
+          queryParams: { nonce },
+        });
+      } else {
+        popupAjaxError(error);
+      }
+      return;
+    } finally {
+      this.loading = false;
+    }
+
+    await new Promise((resolve) =>
+      this.dialog.dialog({
+        message: I18n.t("user.change_email.confirm_success"),
+        type: "alert",
+        didConfirm: resolve,
+      })
+    );
+
+    this.router.transitionTo("/my/preferences/account");
+  }
+}

app/assets/javascripts/discourse/app/controllers/confirm-old-email.js

@@ -0,0 +1,39 @@
+import { tracked } from "@glimmer/tracking";
+import Controller from "@ember/controller";
+import { action } from "@ember/object";
+import { inject as service } from "@ember/service";
+import { ajax } from "discourse/lib/ajax";
+import { popupAjaxError } from "discourse/lib/ajax-error";
+import I18n from "discourse-i18n";
+
+export default class ConfirmOldEmailController extends Controller {
+  @service dialog;
+  @service router;
+
+  @tracked loading;
+
+  @action
+  async confirm() {
+    this.loading = true;
+    try {
+      await ajax(`/u/confirm-old-email/${this.model.token}.json`, {
+        type: "PUT",
+      });
+    } catch (error) {
+      popupAjaxError(error);
+      return;
+    } finally {
+      this.loading = false;
+    }
+
+    await new Promise((resolve) =>
+      this.dialog.dialog({
+        message: I18n.t("user.change_email.authorizing_old.confirm_success"),
+        type: "alert",
+        didConfirm: resolve,
+      })
+    );
+
+    this.router.transitionTo("/my/preferences/account");
+  }
+}

app/assets/javascripts/discourse/app/controllers/second-factor-auth.js

@@ -192,7 +192,10 @@ export default Controller.extend({
         );
         ajax(response.callback_path, {
           type: response.callback_method,
-          data: { second_factor_nonce: this.nonce },
+          data: {
+            second_factor_nonce: this.nonce,
+            ...response.callback_params,
+          },
         })
           .then((callbackResponse) => {
             const redirectUrl =

app/assets/javascripts/discourse/app/routes/app-route-map.js

@@ -105,6 +105,8 @@ export default function () {
     this.route("resent");
     this.route("edit-email");
   });
+  this.route("confirm-new-email", { path: "/u/confirm-new-email/:token" });
+  this.route("confirm-old-email", { path: "/u/confirm-old-email/:token" });
   this.route(
     "user",
     { path: "/u/:username", resetNamespace: true },

app/assets/javascripts/discourse/app/routes/confirm-new-email.js

@@ -0,0 +1,13 @@
+import { ajax } from "discourse/lib/ajax";
+import DiscourseRoute from "discourse/routes/discourse";
+import I18n from "discourse-i18n";
+
+export default class ConfirmNewEmailRoute extends DiscourseRoute {
+  titleToken() {
+    return I18n.t("user.change_email.title");
+  }
+
+  model(params) {
+    return ajax(`/u/confirm-new-email/${params.token}.json`);
+  }
+}

app/assets/javascripts/discourse/app/routes/confirm-old-email.js

@@ -0,0 +1,13 @@
+import { ajax } from "discourse/lib/ajax";
+import DiscourseRoute from "discourse/routes/discourse";
+import I18n from "discourse-i18n";
+
+export default class ConfirmOldEmailRoute extends DiscourseRoute {
+  titleToken() {
+    return I18n.t("user.change_email.title");
+  }
+
+  model(params) {
+    return ajax(`/u/confirm-old-email/${params.token}.json`);
+  }
+}

app/assets/javascripts/discourse/app/templates/confirm-new-email.hbs

@@ -0,0 +1,18 @@
+<div id="simple-container">
+  <div class="confirm-new-email">
+    <h2>{{i18n "user.change_email.title"}}</h2>
+    <p>
+      {{#if this.model.old_email}}
+        {{i18n "user.change_email.authorizing_new.description"}}
+      {{else}}
+        {{i18n "user.change_email.authorizing_new.description_add"}}
+      {{/if}}
+    </p>
+    <p>{{this.model.new_email}}</p>
+    <DButton
+      @translatedLabel={{i18n "user.change_email.confirm"}}
+      class="btn-primary"
+      @action={{this.confirm}}
+    />
+  </div>
+</div>

app/assets/javascripts/discourse/app/templates/confirm-old-email.hbs

@@ -0,0 +1,31 @@
+<div id="simple-container">
+  <div class="confirm-old-email">
+    <h2>{{i18n "user.change_email.authorizing_old.title"}}</h2>
+    <p>
+      {{#if this.model.old_email}}
+        {{i18n "user.change_email.authorizing_old.description"}}
+      {{else}}
+        {{i18n "user.change_email.authorizing_old.description_add"}}
+      {{/if}}
+    </p>
+    {{#if this.model.old_email}}
+      <p>
+        {{i18n
+          "user.change_email.authorizing_old.old_email"
+          email=this.model.old_email
+        }}
+      </p>
+    {{/if}}
+    <p>
+      {{i18n
+        "user.change_email.authorizing_old.new_email"
+        email=this.model.new_email
+      }}
+    </p>
+    <DButton
+      @translatedLabel={{i18n "user.change_email.confirm"}}
+      class="btn-primary"
+      @action={{this.confirm}}
+    />
+  </div>
+</div>

app/controllers/users_email_controller.rb

@@ -3,24 +3,16 @@
 class UsersEmailController < ApplicationController
   requires_login only: %i[index update]
 
-  skip_before_action :check_xhr,
+  skip_before_action :check_xhr, only: %i[show_confirm_old_email show_confirm_new_email]
-                     only: %i[
-                       confirm_old_email
-                       show_confirm_old_email
-                       confirm_new_email
-                       show_confirm_new_email
-                     ]
 
   skip_before_action :redirect_to_login_if_required,
                      only: %i[
-                       confirm_old_email
                        show_confirm_old_email
-                       confirm_new_email
                        show_confirm_new_email
+                       confirm_old_email
+                       confirm_new_email
                      ]
 
-  before_action :require_login, only: %i[confirm_old_email show_confirm_old_email]
-
   def index
   end
 
@@ -61,125 +53,55 @@ class UsersEmailController < ApplicationController
   end
 
   def confirm_new_email
-    load_change_request(:new)
+    change_request = load_change_request(:new)
 
-    if @change_request&.change_state != EmailChangeRequest.states[:authorizing_new]
+    result =
-      @error = I18n.t("change_email.already_done")
+      run_second_factor!(SecondFactor::Actions::ConfirmEmail, target_user: change_request.user)
-    end
 
-    redirect_url = path("/u/confirm-new-email/#{params[:token]}")
+    if result.no_second_factors_enabled? || result.second_factor_auth_completed?
-
-    rate_limit_second_factor!(@user)
-
-    if !@error
-      # this is needed because the form posts this field as JSON and it can be a
-      # hash when authenticating security key.
-      if params[:second_factor_method].to_i == UserSecondFactor.methods[:security_key]
-        begin
-          params[:second_factor_token] = JSON.parse(params[:second_factor_token])
-        rescue JSON::ParserError
-          raise Discourse::InvalidParameters
-        end
-      end
-
-      second_factor_authentication_result = @user.authenticate_second_factor(params, secure_session)
-      if !second_factor_authentication_result.ok
-        flash[:invalid_second_factor] = true
-        flash[:invalid_second_factor_message] = second_factor_authentication_result.error
-        redirect_to redirect_url
-        return
-      end
-    end
-
-    if !@error
       updater = EmailUpdater.new
       if updater.confirm(params[:token]) == :complete
         updater.user.user_stat.reset_bounce_score!
+        render json: success_json
       else
-        @error = I18n.t("change_email.already_done")
+        render json: { error: I18n.t("change_email.already_done") }, status: 400
       end
     end
-
-    if @error
-      flash[:error] = @error
-      redirect_to redirect_url
-    else
-      redirect_to "#{redirect_url}?done=true"
-    end
   end
 
   def show_confirm_new_email
-    load_change_request(:new)
+    return render "default/empty" if request.format.html?
 
-    @done = true if params[:done].to_s == "true"
+    change_request = load_change_request(:new)
 
-    if @change_request&.change_state != EmailChangeRequest.states[:authorizing_new]
+    render json: {
-      @error = I18n.t("change_email.already_done")
+             new_email: change_request.new_email,
-    end
+             old_email: change_request.old_email,
-
+             token: params[:token],
-    @show_invalid_second_factor_error = flash[:invalid_second_factor]
+           }
-    @invalid_second_factor_message = flash[:invalid_second_factor_message]
-
-    if !@error
-      @backup_codes_enabled = @user.backup_codes_enabled?
-      if params[:show_backup].to_s == "true" && @backup_codes_enabled
-        @show_backup_codes = true
-      else
-        @show_second_factor = true if @user.totp_enabled?
-        if @user.security_keys_enabled?
-          DiscourseWebauthn.stage_challenge(@user, secure_session)
-          @show_security_key = params[:show_totp].to_s == "true" ? false : true
-          @security_key_challenge = DiscourseWebauthn.challenge(@user, secure_session)
-          @security_key_allowed_credential_ids =
-            DiscourseWebauthn.allowed_credentials(@user, secure_session)[:allowed_credential_ids]
-        end
-      end
-
-      @to_email = @change_request.new_email
-    end
-
-    render layout: "no_ember"
   end
 
   def confirm_old_email
     load_change_request(:old)
 
-    if @change_request&.change_state != EmailChangeRequest.states[:authorizing_old]
-      @error = I18n.t("change_email.already_done")
-    end
-
-    redirect_url = path("/u/confirm-old-email/#{params[:token]}")
-
-    if !@error
     updater = EmailUpdater.new
-      if updater.confirm(params[:token]) != :authorizing_new
+    if updater.confirm(params[:token]) == :authorizing_new
-        @error = I18n.t("change_email.already_done")
+      render json: success_json
-      end
-    end
-
-    if @error
-      flash[:error] = @error
-      redirect_to redirect_url
     else
-      redirect_to "#{redirect_url}?done=true"
+      render json: { error: I18n.t("change_email.already_done") }, status: 400
     end
   end
 
   def show_confirm_old_email
-    load_change_request(:old)
+    return render "default/empty" if request.format.html?
 
-    if @change_request&.change_state != EmailChangeRequest.states[:authorizing_old]
+    change_request = load_change_request(:old)
-      @error = I18n.t("change_email.already_done")
-    end
 
-    @almost_done = true if params[:done].to_s == "true"
+    render json: {
-
+             new_email: change_request.new_email,
-    if !@error
+             old_email: change_request.old_email,
-      @from_email = @user.email
+             token: params[:token],
-      @to_email = @change_request.new_email
+           }
-    end
-
-    render layout: "no_ember"
   end
 
   private
@@ -189,26 +111,19 @@ class UsersEmailController < ApplicationController
 
     token = EmailToken.confirmable(params[:token], scope: EmailToken.scopes[:email_update])
 
-    if token
+    raise Discourse::NotFound if !token || !token.user
+
+    if current_user && token.user.id != current_user.id
+      raise Discourse::InvalidAccess.new "You are logged in, but this email change link belongs to another user account. Please log out and try again."
+    end
+
+    change_request_params =
       if type == :old
-        @change_request =
+        { old_email_token_id: token.id, change_state: EmailChangeRequest.states[:authorizing_old] }
-          token.user&.email_change_requests&.where(old_email_token_id: token.id)&.first
       elsif type == :new
-        @change_request =
+        { new_email_token_id: token.id, change_state: EmailChangeRequest.states[:authorizing_new] }
-          token.user&.email_change_requests&.where(new_email_token_id: token.id)&.first
-      end
       end
 
-    @user = token&.user
+    token.user&.email_change_requests&.find_by!(**change_request_params)
-
-    @error = I18n.t("change_email.already_done") if (!@user || !@change_request)
-
-    if current_user && current_user.id != @user&.id
-      @error = I18n.t "change_email.wrong_account_error"
-    end
-  end
-
-  def require_login
-    redirect_to_login if !current_user
   end
 end

app/views/users_email/show_confirm_new_email.html.erb

@@ -1,89 +0,0 @@
-<div id="simple-container">
-  <% if @done %>
-    <h2>
-      <%= t 'change_email.confirmed' %>
-    </h2>
-    <p>
-    <a class="btn btn-primary" href="<%= path "/" %>"><%= t('change_email.please_continue', site_name: SiteSetting.title) %></a>
-    </p>
-  <% elsif @error %>
-    <div class='alert alert-error'>
-      <%= @error %>
-    </div>
-  <% else %>
-    <h2><%= t 'change_email.authorizing_new.title' %></h2>
-    <p>
-      <% if @change_request&.old_email %>
-        <%= t 'change_email.authorizing_new.description' %>
-      <% else %>
-        <%= t 'change_email.authorizing_new.description_add' %>
-      <% end %>
-    </p>
-    <p>
-      <%= @to_email %>
-    </p>
-
-    <%=form_tag(u_confirm_new_email_path, method: :put) do %>
-      <%= hidden_field_tag 'token', params[:token] %>
-      <%= hidden_field_tag 'second_factor_token', nil, id: 'security-key-credential' %>
-      <div id="security-key-error"></div>
-
-      <% if @show_invalid_second_factor_error %>
-        <div class='alert alert-error'><%= @invalid_second_factor_message %></div>
-      <% end %>
-
-      <% if @show_backup_codes %>
-        <div id="backup-second-factor-form" style="">
-          <%= hidden_field_tag 'second_factor_method', UserSecondFactor.methods[:backup_code] %>
-          <h3><%= t('login.second_factor_backup_title') %></h3>
-            <%= label_tag(:second_factor_token, t("login.second_factor_backup_description")) %>
-            <div><%= render 'common/second_factor_backup_input' %></div>
-            <%= submit_tag(t("submit"), class: "btn btn-primary") %>
-        </div>
-        <br/>
-        <%= link_to t("login.second_factor_toggle.totp"), show_backup: "false" %>
-        <br/>
-      <% elsif @show_security_key %>
-        <%= hidden_field_tag 'security_key_challenge', @security_key_challenge, id: 'security-key-challenge' %>
-        <%= hidden_field_tag 'second_factor_method', UserSecondFactor.methods[:security_key] %>
-        <%= hidden_field_tag 'security_key_allowed_credential_ids', @security_key_allowed_credential_ids.join(","), id: 'security-key-allowed-credential-ids' %>
-        <div id="security-key-form">
-          <h3><%= t('login.security_key_authenticate') %></h3>
-          <p><%= t('login.security_key_description') %></p>
-          <%= button_tag t('login.security_key_authenticate'), id: 'submit-security-key', class: 'btn btn-primary' %>
-        </div>
-        <br/>
-        <% if @show_second_factor %>
-          <%= link_to t("login.security_key_alternative"), show_totp: "true" %>
-        <% end %><br/>
-        <% if @backup_codes_enabled %>
-          <%= link_to t("login.second_factor_toggle.backup_code"), show_backup: "true" %>
-        <% end %>
-      <% elsif @show_second_factor %>
-        <div id="primary-second-factor-form">
-        <%= hidden_field_tag 'second_factor_method', UserSecondFactor.methods[:totp] %>
-          <h3><%= t('login.second_factor_title') %></h3>
-          <%= label_tag(:second_factor_token, t('login.second_factor_description')) %>
-          <div><%= render 'common/second_factor_text_field' %></div>
-          <%= submit_tag t('submit'), class: "btn btn-primary" %>
-        </div>
-        <br/>
-        <% if @backup_codes_enabled %>
-          <%= link_to t("login.second_factor_toggle.backup_code"), show_backup: "true" %>
-        <% end %>
-      <% else %>
-        <%= submit_tag t('change_email.confirm'), class: "btn btn-primary" %>
-      <% end %>
-    <%end%>
-  <% end%>
-
-  <%= preload_script "vendor" %>
-  <%= preload_script "locales/#{I18n.locale}" %>
-  <%- if ExtraLocalesController.client_overrides_exist? %>
-    <%= preload_script_url ExtraLocalesController.url('overrides') %>
-  <%- end %>
-  <% # TODO: move all this logic into the ember app %>
-  <%= preload_script "discourse/app/lib/webauthn" %>
-  <%= preload_script "confirm-new-email/confirm-new-email" %>
-  <%= preload_script "confirm-new-email/bootstrap" %>
-</div>

app/views/users_email/show_confirm_old_email.html.erb

@@ -1,34 +0,0 @@
-<div id="simple-container">
-  <% if @almost_done %>
-    <h2><%= t 'change_email.authorizing_old.almost_done_title' %></h2>
-    <p>
-      <%= t 'change_email.authorizing_old.almost_done_description' %>
-    </p>
-  <% elsif @error %>
-    <div class='alert alert-error'>
-      <%= @error %>
-    </div>
-  <% else %>
-    <h2><%= t 'change_email.authorizing_old.title' %></h2>
-    <p>
-      <% if @change_request&.old_email %>
-        <%= t 'change_email.authorizing_old.description' %>
-        <br>
-        <br>
-        <%= t 'change_email.authorizing_old.old_email', email: @from_email %>
-        <br>
-        <%= t 'change_email.authorizing_old.new_email', email: @to_email %>
-      <% else %>
-        <%= t 'change_email.authorizing_old.description_add' %>
-        <br>
-        <br>
-        <%= @to_email %>
-      <% end %>
-    </p>
-
-    <%=form_tag(u_confirm_old_email_path, method: :put) do %>
-      <%= hidden_field_tag 'token', params[:token] %>
-      <%= submit_tag t('change_email.confirm'), class: "btn btn-primary" %>
-    <% end %>
-  <% end %>
-</div>

config/initializers/assets.rb

@@ -27,9 +27,6 @@ Rails.application.config.assets.precompile += %w[
   break_string.js
   service-worker.js
   locales/i18n.js
-  discourse/app/lib/webauthn.js
-  confirm-new-email/confirm-new-email.js
-  confirm-new-email/bootstrap.js
   scripts/discourse-test-listen-boot
 ]
 

config/locales/client.en.yml

@@ -1559,6 +1559,18 @@ en:
         success_via_admin: "We've sent an email to that address. The user will need to follow the confirmation instructions in the email."
         success_staff: "We've sent an email to your current address. Please follow the confirmation instructions."
         back_to_preferences: "Back to preferences"
+        confirm_success: "Your email has been updated."
+        confirm: "Confirm"
+        authorizing_new:
+          description: "Please confirm you would like your email address changed to:"
+          description_add: "Please confirm you would like to add an alternate email address:"
+        authorizing_old:
+          title: "Verify old email address"
+          description: "Please verify your old email address to continue changing your email:"
+          description_add: "Please verify your existing email address to continue adding an alternate address:"
+          old_email: "Old email: %{email}"
+          new_email: "New email: %{email}"
+          confirm_success: "We have sent an email to your new email address to confirm the change!"
 
       change_avatar:
         title: "Change your profile picture"

config/locales/server.en.yml

@@ -919,9 +919,6 @@ en:
       unknown: "unknown operating system"
 
   change_email:
-    wrong_account_error: "You are logged in the wrong account, please log out and try again."
-    confirmed: "Your email has been updated."
-    please_continue: "Continue to %{site_name}"
     error: "There was an error changing your email address. Perhaps the address is already in use?"
     doesnt_exist: "That email address is not associated with your account."
     error_staged: "There was an error changing your email address. The address is already in use by a staged user."
@@ -929,20 +926,6 @@ en:
     confirm: "Confirm"
     max_secondary_emails_error: "You have reached the maximum allowed secondary emails limit."
 
-    authorizing_new:
-      title: "Confirm your new email"
-      description: "Please confirm you would like your email address changed to:"
-      description_add: "Please confirm you would like to add an alternate email address:"
-
-    authorizing_old:
-      title: "Change your email address"
-      description: "Please confirm your email address change"
-      description_add: "Please confirm you would like to add an alternate email address:"
-      old_email: "Old email: %{email}"
-      new_email: "New email: %{email}"
-      almost_done_title: "Confirming new email address"
-      almost_done_description: "We have sent an email to your new email address to confirm the change!"
-
   associated_accounts:
     revoke_failed: "Failed to revoke your account with %{provider_name}."
     connected: "(connected)"

config/routes.rb

@@ -538,10 +538,10 @@ Discourse::Application.routes.draw do
       )
 
       get "#{root_path}/confirm-old-email/:token" => "users_email#show_confirm_old_email"
-      put "#{root_path}/confirm-old-email" => "users_email#confirm_old_email"
+      put "#{root_path}/confirm-old-email/:token" => "users_email#confirm_old_email"
 
       get "#{root_path}/confirm-new-email/:token" => "users_email#show_confirm_new_email"
-      put "#{root_path}/confirm-new-email" => "users_email#confirm_new_email"
+      put "#{root_path}/confirm-new-email/:token" => "users_email#confirm_new_email"
 
       get(
         {

lib/second_factor/actions/confirm_email.rb

@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+module SecondFactor::Actions
+  class ConfirmEmail < Base
+    def no_second_factors_enabled!(params)
+      # handled in controller
+    end
+
+    def second_factor_auth_required!(params)
+      {
+        callback_params: {
+          token: params[:token],
+        },
+        redirect_url:
+          (
+            if @current_user
+              "#{Discourse.base_path}/my/preferences/account"
+            else
+              "#{Discourse.base_path}/login"
+            end
+          ),
+      }
+    end
+
+    def second_factor_auth_completed!(callback_params)
+      # handled in controller
+    end
+  end
+end

spec/requests/users_email_controller_spec.rb

@@ -24,10 +24,9 @@ RSpec.describe UsersEmailController do
     it "errors out for invalid tokens" do
       sign_in(user)
 
-      get "/u/confirm-new-email/invalidtoken"
+      get "/u/confirm-new-email/invalidtoken.json"
 
-      expect(response.status).to eq(200)
+      expect(response.status).to eq(404)
-      expect(response.body).to include(I18n.t("change_email.already_done"))
     end
 
     it "does not change email if accounts mismatch for a signed in user" do
@@ -38,7 +37,8 @@ RSpec.describe UsersEmailController do
 
       sign_in(moderator)
 
-      put "/u/confirm-new-email", params: { token: "#{email_token.token}" }
+      put "/u/confirm-new-email/#{email_token.token}.json"
+      expect(response.status).to eq(404)
       expect(user.reload.email).to eq(old_email)
     end
 
@@ -50,212 +50,17 @@ RSpec.describe UsersEmailController do
         updater.change_to("bubblegum@adventuretime.ooo")
       end
 
-      it "includes security_key_allowed_credential_ids in a hidden field" do
-        key1 = Fabricate(:user_security_key_with_random_credential, user: user)
-        key2 = Fabricate(:user_security_key_with_random_credential, user: user)
-
-        get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}"
-
-        doc = Nokogiri.HTML5(response.body)
-        credential_ids = doc.css("#security-key-allowed-credential-ids").first["value"].split(",")
-        expect(credential_ids).to contain_exactly(key1.credential_id, key2.credential_id)
-      end
-
       it "confirms with a correct token" do
         user.user_stat.update_columns(bounce_score: 42, reset_bounce_score_after: 1.week.from_now)
 
-        put "/u/confirm-new-email", params: { token: "#{updater.change_req.new_email_token.token}" }
+        put "/u/confirm-new-email/#{updater.change_req.new_email_token.token}.json"
 
-        expect(response.status).to eq(302)
+        expect(response.status).to eq(200)
-        expect(response.redirect_url).to include("done")
         user.reload
         expect(user.user_stat.bounce_score).to eq(0)
         expect(user.user_stat.reset_bounce_score_after).to eq(nil)
         expect(user.email).to eq("bubblegum@adventuretime.ooo")
       end
-
-      context "when second factor is required" do
-        fab!(:second_factor) { Fabricate(:user_second_factor_totp, user: user) }
-        fab!(:backup_code) { Fabricate(:user_second_factor_backup, user: user) }
-
-        it "requires a second factor token" do
-          get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}"
-
-          expect(response.status).to eq(200)
-          expect(response.body).to include(I18n.t("login.second_factor_title"))
-          expect(response.body).not_to include(I18n.t("login.invalid_second_factor_code"))
-        end
-
-        it "requires a backup token" do
-          get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}?show_backup=true"
-
-          expect(response.status).to eq(200)
-          expect(response.body).to include(I18n.t("login.second_factor_backup_title"))
-        end
-
-        it "adds an error on a second factor attempt" do
-          put "/u/confirm-new-email",
-              params: {
-                token: updater.change_req.new_email_token.token,
-                second_factor_token: "000000",
-                second_factor_method: UserSecondFactor.methods[:totp],
-              }
-
-          expect(response.status).to eq(302)
-          expect(flash[:invalid_second_factor]).to eq(true)
-        end
-
-        it "confirms with a correct second token" do
-          put "/u/confirm-new-email",
-              params: {
-                second_factor_token: ROTP::TOTP.new(second_factor.data).now,
-                second_factor_method: UserSecondFactor.methods[:totp],
-                token: updater.change_req.new_email_token.token,
-              }
-
-          expect(response.status).to eq(302)
-          expect(user.reload.email).to eq("bubblegum@adventuretime.ooo")
-        end
-
-        context "with rate limiting" do
-          before { RateLimiter.enable }
-
-          use_redis_snapshotting
-
-          it "rate limits by IP" do
-            freeze_time
-
-            6.times do
-              put "/u/confirm-new-email",
-                  params: {
-                    token: "blah",
-                    second_factor_token: "000000",
-                    second_factor_method: UserSecondFactor.methods[:totp],
-                  }
-
-              expect(response.status).to eq(302)
-            end
-
-            put "/u/confirm-new-email",
-                params: {
-                  token: "blah",
-                  second_factor_token: "000000",
-                  second_factor_method: UserSecondFactor.methods[:totp],
-                }
-
-            expect(response.status).to eq(429)
-          end
-
-          it "rate limits by username" do
-            freeze_time
-
-            6.times do |x|
-              user.email_change_requests.last.update(
-                change_state: EmailChangeRequest.states[:complete],
-              )
-              put "/u/confirm-new-email",
-                  params: {
-                    token: updater.change_req.new_email_token.token,
-                    second_factor_token: "000000",
-                    second_factor_method: UserSecondFactor.methods[:totp],
-                  },
-                  env: {
-                    REMOTE_ADDR: "1.2.3.#{x}",
-                  }
-
-              expect(response.status).to eq(302)
-            end
-
-            user.email_change_requests.last.update(
-              change_state: EmailChangeRequest.states[:authorizing_new],
-            )
-            put "/u/confirm-new-email",
-                params: {
-                  token: updater.change_req.new_email_token.token,
-                  second_factor_token: "000000",
-                  second_factor_method: UserSecondFactor.methods[:totp],
-                },
-                env: {
-                  REMOTE_ADDR: "1.2.3.4",
-                }
-
-            expect(response.status).to eq(429)
-          end
-        end
-      end
-
-      context "when security key is required" do
-        fab!(:user_security_key) do
-          Fabricate(
-            :user_security_key,
-            user: user,
-            credential_id: valid_security_key_data[:credential_id],
-            public_key: valid_security_key_data[:public_key],
-          )
-        end
-
-        before { simulate_localhost_webauthn_challenge }
-
-        it "requires a security key" do
-          get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}"
-
-          expect(response.status).to eq(200)
-          expect(response.body).to include(I18n.t("login.security_key_authenticate"))
-          expect(response.body).to include(I18n.t("login.security_key_description"))
-        end
-
-        context "if the user has a TOTP enabled and wants to use that instead" do
-          before { Fabricate(:user_second_factor_totp, user: user) }
-
-          it "allows entering the totp code instead" do
-            get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}?show_totp=true"
-
-            expect(response.status).to eq(200)
-            expect(response.body).to include(I18n.t("login.second_factor_title"))
-            expect(response.body).not_to include(I18n.t("login.security_key_authenticate"))
-          end
-        end
-
-        it "adds an error on a security key attempt" do
-          get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}"
-          put "/u/confirm-new-email",
-              params: {
-                token: updater.change_req.new_email_token.token,
-                second_factor_token: "{}",
-                second_factor_method: UserSecondFactor.methods[:security_key],
-              }
-
-          expect(response.status).to eq(302)
-          expect(flash[:invalid_second_factor]).to eq(true)
-        end
-
-        it "confirms with a correct security key token" do
-          get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}"
-          put "/u/confirm-new-email",
-              params: {
-                token: updater.change_req.new_email_token.token,
-                second_factor_token: valid_security_key_auth_post_data.to_json,
-                second_factor_method: UserSecondFactor.methods[:security_key],
-              }
-
-          expect(response.status).to eq(302)
-          expect(user.reload.email).to eq("bubblegum@adventuretime.ooo")
-        end
-
-        context "if the security key data JSON is garbled" do
-          it "raises an invalid parameters error" do
-            get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}"
-            put "/u/confirm-new-email",
-                params: {
-                  token: updater.change_req.new_email_token.token,
-                  second_factor_token: "{someweird: 8notjson}",
-                  second_factor_method: UserSecondFactor.methods[:security_key],
-                }
-
-            expect(response.status).to eq(400)
-          end
-        end
-      end
     end
 
     it "destroys email tokens associated with the old email after the new email is confirmed" do
@@ -268,7 +73,8 @@ RSpec.describe UsersEmailController do
       updater.change_to("bubblegum@adventuretime.ooo")
 
       sign_in(user)
-      put "/u/confirm-new-email", params: { token: "#{updater.change_req.new_email_token.token}" }
+      put "/u/confirm-new-email/#{updater.change_req.new_email_token.token}.json"
+      expect(response.status).to eq(200)
 
       new_password = SecureRandom.hex
       put "/u/password-reset/#{email_token.token}.json", params: { password: new_password }
@@ -281,20 +87,12 @@ RSpec.describe UsersEmailController do
   end
 
   describe "#confirm-old-email" do
-    it "redirects to login for signed out accounts" do
-      get "/u/confirm-old-email/invalidtoken"
-
-      expect(response.status).to eq(302)
-      expect(response.redirect_url).to eq("http://test.localhost/login")
-    end
-
     it "errors out for invalid tokens" do
       sign_in(user)
 
-      get "/u/confirm-old-email/invalidtoken"
+      get "/u/confirm-old-email/invalidtoken.json"
 
-      expect(response.status).to eq(200)
+      expect(response.status).to eq(404)
-      expect(response.body).to include(I18n.t("change_email.already_done"))
     end
 
     it "bans change when accounts do not match" do
@@ -302,10 +100,9 @@ RSpec.describe UsersEmailController do
       updater = EmailUpdater.new(guardian: moderator.guardian, user: moderator)
       email_change_request = updater.change_to("bubblegum@adventuretime.ooo")
 
-      get "/u/confirm-old-email/#{email_change_request.old_email_token.token}"
+      get "/u/confirm-old-email/#{email_change_request.old_email_token.token}.json"
 
-      expect(response.status).to eq(200)
+      expect(response.status).to eq(403)
-      expect(body).to include("alert-error")
     end
 
     context "with valid old token" do
@@ -314,17 +111,15 @@ RSpec.describe UsersEmailController do
         updater = EmailUpdater.new(guardian: moderator.guardian, user: moderator)
         email_change_request = updater.change_to("bubblegum@adventuretime.ooo")
 
-        get "/u/confirm-old-email/#{email_change_request.old_email_token.token}"
+        get "/u/confirm-old-email/#{email_change_request.old_email_token.token}.json"
 
         expect(response.status).to eq(200)
-        body = CGI.unescapeHTML(response.body)
+        expect(response.parsed_body["old_email"]).to eq(moderator.email)
-        expect(body).to include(I18n.t("change_email.authorizing_old.title"))
+        expect(response.parsed_body["new_email"]).to eq("bubblegum@adventuretime.ooo")
-        expect(body).to include(I18n.t("change_email.authorizing_old.description"))
 
-        put "/u/confirm-old-email", params: { token: email_change_request.old_email_token.token }
+        put "/u/confirm-old-email/#{email_change_request.old_email_token.token}.json"
 
-        expect(response.status).to eq(302)
+        expect(response.status).to eq(200)
-        expect(response.redirect_url).to include("done=true")
       end
     end
   end

spec/system/email_change_spec.rb

@@ -0,0 +1,182 @@
+# frozen_string_literal: true
+
+describe "Changing email", type: :system do
+  fab!(:password) { "mysupersecurepassword" }
+  fab!(:user) { Fabricate(:user, active: true, password: password) }
+  let(:new_email) { "newemail@example.com" }
+  let(:user_preferences_security_page) { PageObjects::Pages::UserPreferencesSecurity.new }
+
+  before { Jobs.run_immediately! }
+
+  def generate_confirm_link
+    visit "/my/preferences/account"
+
+    email_dropdown = PageObjects::Components::SelectKit.new(".email-dropdown")
+    expect(email_dropdown.visible?).to eq(true)
+    email_dropdown.select_row_by_value("updateEmail")
+
+    find("#change-email").fill_in with: "newemail@example.com"
+
+    find(".save-button button").click
+
+    wait_for(timeout: 5) { ActionMailer::Base.deliveries.count === 1 }
+
+    if user.admin?
+      get_link_from_email(:old)
+    else
+      get_link_from_email(:new)
+    end
+  end
+
+  def get_link_from_email(type)
+    mail = ActionMailer::Base.deliveries.last
+    expect(mail.to).to contain_exactly(type == :new ? new_email : user.email)
+
+    mail.body.to_s[%r{/u/confirm-#{type}-email/\S+}, 0]
+  end
+
+  it "allows regular user to change their email" do
+    sign_in user
+
+    visit generate_confirm_link
+
+    find(".confirm-new-email .btn-primary").click
+
+    expect(page).to have_css(".dialog-body", text: I18n.t("js.user.change_email.confirm_success"))
+    find(".dialog-footer .btn-primary").click
+
+    expect(user.reload.email).to eq(new_email)
+  end
+
+  it "works when user has totp 2fa" do
+    second_factor = Fabricate(:user_second_factor_totp, user: user)
+    sign_in user
+
+    visit generate_confirm_link
+
+    find(".confirm-new-email .btn-primary").click
+
+    find(".second-factor-token-input").fill_in with: second_factor.totp_object.now
+
+    find("button[type=submit]").click
+
+    try_until_success { expect(current_url).to match("/u/#{user.username}/preferences/account") }
+
+    expect(user.reload.email).to eq(new_email)
+  end
+
+  it "works when user has webauthn 2fa" do
+    sign_in user
+
+    DiscourseWebauthn.stubs(:origin).returns(current_host + ":" + Capybara.server_port.to_s)
+    options =
+      ::Selenium::WebDriver::VirtualAuthenticatorOptions.new(
+        user_verification: true,
+        user_verified: true,
+        resident_key: true,
+      )
+    authenticator = page.driver.browser.add_virtual_authenticator(options)
+
+    user_preferences_security_page.visit(user)
+    user_preferences_security_page.visit_second_factor(password)
+
+    find(".security-key .new-security-key").click
+    expect(user_preferences_security_page).to have_css("input#security-key-name")
+
+    find(".d-modal__body input#security-key-name").fill_in(with: "First Key")
+    find(".add-security-key").click
+
+    expect(user_preferences_security_page).to have_css(".security-key .second-factor-item")
+
+    visit generate_confirm_link
+
+    find(".confirm-new-email .btn-primary").click
+
+    find("#security-key-authenticate-button").click
+
+    try_until_success { expect(current_url).to match("/u/#{user.username}/preferences/account") }
+
+    expect(user.reload.email).to eq(new_email)
+  end
+
+  it "does not require login to verify" do
+    second_factor = Fabricate(:user_second_factor_totp, user: user)
+    sign_in user
+
+    confirm_link = generate_confirm_link
+
+    Capybara.reset_sessions! # log out
+
+    visit confirm_link
+
+    find(".confirm-new-email .btn-primary").click
+
+    find(".second-factor-token-input").fill_in with: second_factor.totp_object.now
+
+    find("button[type=submit]").click
+
+    try_until_success { expect(current_url).to match("/latest") }
+
+    expect(user.reload.email).to eq(new_email)
+  end
+
+  it "makes admins verify old email" do
+    user.update!(admin: true)
+    sign_in user
+
+    confirm_old_link = generate_confirm_link
+
+    # Confirm old email
+    visit confirm_old_link
+    find(".confirm-old-email .btn-primary").click
+    expect(page).to have_css(
+      ".dialog-body",
+      text: I18n.t("js.user.change_email.authorizing_old.confirm_success"),
+    )
+    find(".dialog-footer .btn-primary").click
+
+    # Confirm new email
+    wait_for(timeout: 5) { ActionMailer::Base.deliveries.count === 2 }
+    confirm_new_link = get_link_from_email(:new)
+
+    visit confirm_new_link
+
+    find(".confirm-new-email .btn-primary").click
+
+    expect(page).to have_css(".dialog-body", text: I18n.t("js.user.change_email.confirm_success"))
+    find(".dialog-footer .btn-primary").click
+
+    expect(user.reload.email).to eq(new_email)
+  end
+
+  it "allows admin to verify old email while logged out" do
+    user.update!(admin: true)
+    sign_in user
+
+    confirm_old_link = generate_confirm_link
+
+    Capybara.reset_sessions! # log out
+
+    # Confirm old email
+    visit confirm_old_link
+    find(".confirm-old-email .btn-primary").click
+    expect(page).to have_css(
+      ".dialog-body",
+      text: I18n.t("js.user.change_email.authorizing_old.confirm_success"),
+    )
+    find(".dialog-footer .btn-primary").click
+
+    # Confirm new email
+    wait_for(timeout: 5) { ActionMailer::Base.deliveries.count === 2 }
+    confirm_new_link = get_link_from_email(:new)
+
+    visit confirm_new_link
+
+    find(".confirm-new-email .btn-primary").click
+
+    expect(page).to have_css(".dialog-body", text: I18n.t("js.user.change_email.confirm_success"))
+    find(".dialog-footer .btn-primary").click
+
+    expect(user.reload.email).to eq(new_email)
+  end
+end