From 283fe48243b95f5253a363844f00ec96d17674f3 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 30 Jan 2024 10:32:42 +0000 Subject: [PATCH] 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. --- .../confirm-new-email/bootstrap.js | 4 - .../confirm-new-email/confirm-new-email.js | 27 -- .../app/controllers/confirm-new-email.js | 46 ++++ .../app/controllers/confirm-old-email.js | 39 +++ .../app/controllers/second-factor-auth.js | 5 +- .../discourse/app/routes/app-route-map.js | 2 + .../discourse/app/routes/confirm-new-email.js | 13 + .../discourse/app/routes/confirm-old-email.js | 13 + .../app/templates/confirm-new-email.hbs | 18 ++ .../app/templates/confirm-old-email.hbs | 31 +++ app/controllers/users_email_controller.rb | 159 +++--------- .../show_confirm_new_email.html.erb | 89 ------- .../show_confirm_old_email.html.erb | 34 --- config/initializers/assets.rb | 3 - config/locales/client.en.yml | 12 + config/locales/server.en.yml | 17 -- config/routes.rb | 4 +- lib/second_factor/actions/confirm_email.rb | 29 +++ spec/requests/users_email_controller_spec.rb | 239 ++---------------- spec/system/email_change_spec.rb | 182 +++++++++++++ 20 files changed, 445 insertions(+), 521 deletions(-) delete mode 100644 app/assets/javascripts/confirm-new-email/bootstrap.js delete mode 100644 app/assets/javascripts/confirm-new-email/confirm-new-email.js create mode 100644 app/assets/javascripts/discourse/app/controllers/confirm-new-email.js create mode 100644 app/assets/javascripts/discourse/app/controllers/confirm-old-email.js create mode 100644 app/assets/javascripts/discourse/app/routes/confirm-new-email.js create mode 100644 app/assets/javascripts/discourse/app/routes/confirm-old-email.js create mode 100644 app/assets/javascripts/discourse/app/templates/confirm-new-email.hbs create mode 100644 app/assets/javascripts/discourse/app/templates/confirm-old-email.hbs delete mode 100644 app/views/users_email/show_confirm_new_email.html.erb delete mode 100644 app/views/users_email/show_confirm_old_email.html.erb create mode 100644 lib/second_factor/actions/confirm_email.rb create mode 100644 spec/system/email_change_spec.rb diff --git a/app/assets/javascripts/confirm-new-email/bootstrap.js b/app/assets/javascripts/confirm-new-email/bootstrap.js deleted file mode 100644 index db93c5ee55f..00000000000 --- a/app/assets/javascripts/confirm-new-email/bootstrap.js +++ /dev/null @@ -1,4 +0,0 @@ -// discourse-skip-module -(function () { - require("confirm-new-email/confirm-new-email"); -})(); diff --git a/app/assets/javascripts/confirm-new-email/confirm-new-email.js b/app/assets/javascripts/confirm-new-email/confirm-new-email.js deleted file mode 100644 index 053c9832767..00000000000 --- a/app/assets/javascripts/confirm-new-email/confirm-new-email.js +++ /dev/null @@ -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 = `
`; - document.querySelector("#security-key-error .alert").innerText = - errorMessage; - } - ); - }; -} diff --git a/app/assets/javascripts/discourse/app/controllers/confirm-new-email.js b/app/assets/javascripts/discourse/app/controllers/confirm-new-email.js new file mode 100644 index 00000000000..7745b5b8108 --- /dev/null +++ b/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"); + } +} diff --git a/app/assets/javascripts/discourse/app/controllers/confirm-old-email.js b/app/assets/javascripts/discourse/app/controllers/confirm-old-email.js new file mode 100644 index 00000000000..5bd6f97143d --- /dev/null +++ b/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"); + } +} diff --git a/app/assets/javascripts/discourse/app/controllers/second-factor-auth.js b/app/assets/javascripts/discourse/app/controllers/second-factor-auth.js index ab52cf2056b..0d69bbfb904 100644 --- a/app/assets/javascripts/discourse/app/controllers/second-factor-auth.js +++ b/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 = diff --git a/app/assets/javascripts/discourse/app/routes/app-route-map.js b/app/assets/javascripts/discourse/app/routes/app-route-map.js index 51b5c29f6b3..0db907f8b38 100644 --- a/app/assets/javascripts/discourse/app/routes/app-route-map.js +++ b/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 }, diff --git a/app/assets/javascripts/discourse/app/routes/confirm-new-email.js b/app/assets/javascripts/discourse/app/routes/confirm-new-email.js new file mode 100644 index 00000000000..5e08a00fc44 --- /dev/null +++ b/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`); + } +} diff --git a/app/assets/javascripts/discourse/app/routes/confirm-old-email.js b/app/assets/javascripts/discourse/app/routes/confirm-old-email.js new file mode 100644 index 00000000000..0d2bd97d69e --- /dev/null +++ b/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`); + } +} diff --git a/app/assets/javascripts/discourse/app/templates/confirm-new-email.hbs b/app/assets/javascripts/discourse/app/templates/confirm-new-email.hbs new file mode 100644 index 00000000000..274c1e415bd --- /dev/null +++ b/app/assets/javascripts/discourse/app/templates/confirm-new-email.hbs @@ -0,0 +1,18 @@ +
+
+

{{i18n "user.change_email.title"}}

+

+ {{#if this.model.old_email}} + {{i18n "user.change_email.authorizing_new.description"}} + {{else}} + {{i18n "user.change_email.authorizing_new.description_add"}} + {{/if}} +

+

{{this.model.new_email}}

+ +
+
\ No newline at end of file diff --git a/app/assets/javascripts/discourse/app/templates/confirm-old-email.hbs b/app/assets/javascripts/discourse/app/templates/confirm-old-email.hbs new file mode 100644 index 00000000000..07f89575adf --- /dev/null +++ b/app/assets/javascripts/discourse/app/templates/confirm-old-email.hbs @@ -0,0 +1,31 @@ +
+
+

{{i18n "user.change_email.authorizing_old.title"}}

+

+ {{#if this.model.old_email}} + {{i18n "user.change_email.authorizing_old.description"}} + {{else}} + {{i18n "user.change_email.authorizing_old.description_add"}} + {{/if}} +

+ {{#if this.model.old_email}} +

+ {{i18n + "user.change_email.authorizing_old.old_email" + email=this.model.old_email + }} +

+ {{/if}} +

+ {{i18n + "user.change_email.authorizing_old.new_email" + email=this.model.new_email + }} +

+ +
+
\ No newline at end of file diff --git a/app/controllers/users_email_controller.rb b/app/controllers/users_email_controller.rb index 3c428f7c6ab..83b10c14698 100644 --- a/app/controllers/users_email_controller.rb +++ b/app/controllers/users_email_controller.rb @@ -3,24 +3,16 @@ class UsersEmailController < ApplicationController requires_login only: %i[index update] - skip_before_action :check_xhr, - only: %i[ - confirm_old_email - show_confirm_old_email - confirm_new_email - show_confirm_new_email - ] + skip_before_action :check_xhr, only: %i[show_confirm_old_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] - @error = I18n.t("change_email.already_done") - end + result = + run_second_factor!(SecondFactor::Actions::ConfirmEmail, target_user: change_request.user) - redirect_url = path("/u/confirm-new-email/#{params[:token]}") - - 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 + if result.no_second_factors_enabled? || result.second_factor_auth_completed? 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] - @error = I18n.t("change_email.already_done") - end - - @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" + render json: { + new_email: change_request.new_email, + old_email: change_request.old_email, + token: params[:token], + } 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 - @error = I18n.t("change_email.already_done") - end - end - - if @error - flash[:error] = @error - redirect_to redirect_url + updater = EmailUpdater.new + if updater.confirm(params[:token]) == :authorizing_new + render json: success_json 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] - @error = I18n.t("change_email.already_done") - end + change_request = load_change_request(:old) - @almost_done = true if params[:done].to_s == "true" - - if !@error - @from_email = @user.email - @to_email = @change_request.new_email - end - - render layout: "no_ember" + render json: { + new_email: change_request.new_email, + old_email: change_request.old_email, + token: params[:token], + } 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 = - token.user&.email_change_requests&.where(old_email_token_id: token.id)&.first + { old_email_token_id: token.id, change_state: EmailChangeRequest.states[:authorizing_old] } elsif type == :new - @change_request = - token.user&.email_change_requests&.where(new_email_token_id: token.id)&.first + { new_email_token_id: token.id, change_state: EmailChangeRequest.states[:authorizing_new] } end - end - @user = token&.user - - @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 + token.user&.email_change_requests&.find_by!(**change_request_params) end end diff --git a/app/views/users_email/show_confirm_new_email.html.erb b/app/views/users_email/show_confirm_new_email.html.erb deleted file mode 100644 index 071dc731305..00000000000 --- a/app/views/users_email/show_confirm_new_email.html.erb +++ /dev/null @@ -1,89 +0,0 @@ -
- <% if @done %> -

- <%= t 'change_email.confirmed' %> -

-

- "><%= t('change_email.please_continue', site_name: SiteSetting.title) %> -

- <% elsif @error %> -
- <%= @error %> -
- <% else %> -

<%= t 'change_email.authorizing_new.title' %>

-

- <% if @change_request&.old_email %> - <%= t 'change_email.authorizing_new.description' %> - <% else %> - <%= t 'change_email.authorizing_new.description_add' %> - <% end %> -

-

- <%= @to_email %> -

- - <%=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' %> -
- - <% if @show_invalid_second_factor_error %> -
<%= @invalid_second_factor_message %>
- <% end %> - - <% if @show_backup_codes %> -
- <%= hidden_field_tag 'second_factor_method', UserSecondFactor.methods[:backup_code] %> -

<%= t('login.second_factor_backup_title') %>

- <%= label_tag(:second_factor_token, t("login.second_factor_backup_description")) %> -
<%= render 'common/second_factor_backup_input' %>
- <%= submit_tag(t("submit"), class: "btn btn-primary") %> -
-
- <%= link_to t("login.second_factor_toggle.totp"), show_backup: "false" %> -
- <% 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' %> -
-

<%= t('login.security_key_authenticate') %>

-

<%= t('login.security_key_description') %>

- <%= button_tag t('login.security_key_authenticate'), id: 'submit-security-key', class: 'btn btn-primary' %> -
-
- <% if @show_second_factor %> - <%= link_to t("login.security_key_alternative"), show_totp: "true" %> - <% end %>
- <% if @backup_codes_enabled %> - <%= link_to t("login.second_factor_toggle.backup_code"), show_backup: "true" %> - <% end %> - <% elsif @show_second_factor %> -
- <%= hidden_field_tag 'second_factor_method', UserSecondFactor.methods[:totp] %> -

<%= t('login.second_factor_title') %>

- <%= label_tag(:second_factor_token, t('login.second_factor_description')) %> -
<%= render 'common/second_factor_text_field' %>
- <%= submit_tag t('submit'), class: "btn btn-primary" %> -
-
- <% 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" %> -
diff --git a/app/views/users_email/show_confirm_old_email.html.erb b/app/views/users_email/show_confirm_old_email.html.erb deleted file mode 100644 index 403103e8b26..00000000000 --- a/app/views/users_email/show_confirm_old_email.html.erb +++ /dev/null @@ -1,34 +0,0 @@ -
- <% if @almost_done %> -

<%= t 'change_email.authorizing_old.almost_done_title' %>

-

- <%= t 'change_email.authorizing_old.almost_done_description' %> -

- <% elsif @error %> -
- <%= @error %> -
- <% else %> -

<%= t 'change_email.authorizing_old.title' %>

-

- <% if @change_request&.old_email %> - <%= t 'change_email.authorizing_old.description' %> -
-
- <%= t 'change_email.authorizing_old.old_email', email: @from_email %> -
- <%= t 'change_email.authorizing_old.new_email', email: @to_email %> - <% else %> - <%= t 'change_email.authorizing_old.description_add' %> -
-
- <%= @to_email %> - <% end %> -

- - <%=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 %> -
diff --git a/config/initializers/assets.rb b/config/initializers/assets.rb index 02e001566c8..eef890db1c3 100644 --- a/config/initializers/assets.rb +++ b/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 ] diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 08e4edde2e4..8012d4f198d 100644 --- a/config/locales/client.en.yml +++ b/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" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index e2a09c53cad..d8afaf477a2 100644 --- a/config/locales/server.en.yml +++ b/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)" diff --git a/config/routes.rb b/config/routes.rb index a9fa7597d8b..51ee33f4428 100644 --- a/config/routes.rb +++ b/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( { diff --git a/lib/second_factor/actions/confirm_email.rb b/lib/second_factor/actions/confirm_email.rb new file mode 100644 index 00000000000..8e7cc9d841f --- /dev/null +++ b/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 diff --git a/spec/requests/users_email_controller_spec.rb b/spec/requests/users_email_controller_spec.rb index c90dc6904ca..076d23f5343 100644 --- a/spec/requests/users_email_controller_spec.rb +++ b/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.body).to include(I18n.t("change_email.already_done")) + expect(response.status).to eq(404) 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.redirect_url).to include("done") + expect(response.status).to eq(200) 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.body).to include(I18n.t("change_email.already_done")) + expect(response.status).to eq(404) 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(body).to include("alert-error") + expect(response.status).to eq(403) 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(body).to include(I18n.t("change_email.authorizing_old.title")) - expect(body).to include(I18n.t("change_email.authorizing_old.description")) + expect(response.parsed_body["old_email"]).to eq(moderator.email) + expect(response.parsed_body["new_email"]).to eq("bubblegum@adventuretime.ooo") - 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.redirect_url).to include("done=true") + expect(response.status).to eq(200) end end end diff --git a/spec/system/email_change_spec.rb b/spec/system/email_change_spec.rb new file mode 100644 index 00000000000..a2683fc5b3d --- /dev/null +++ b/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