Merge pull request #1857 from basecamp/friendly-error-message-when-changing-email-with-an-invalid-code

Render a friendly error message when using an invalid email change token

app/controllers/users/email_addresses/confirmations_controller.rb

@@ -8,12 +8,14 @@ class Users::EmailAddresses::ConfirmationsController < ApplicationController
   end
 
   def create
-    user = @user.change_email_address_using_token(token)
+    if @user.change_email_address_using_token(token)
+      terminate_session if Current.session
+      start_new_session_for @user.identity
 
-    terminate_session if Current.session
-    start_new_session_for user.identity
-
-    redirect_to edit_user_url(script_name: user.account.slug, id: user)
+      redirect_to edit_user_url(script_name: @user.account.slug, id: @user)
+    else
+      render :invalid_token, status: :unprocessable_entity
+    end
   end
 
   private

app/models/user/email_address_changeable.rb

@@ -7,14 +7,12 @@ module User::EmailAddressChangeable
   def change_email_address_using_token(token)
     parsed_token = SignedGlobalID.parse(token, for: EMAIL_CHANGE_TOKEN_PURPOSE)
 
-    if parsed_token.nil?
-      raise ArgumentError, "The token is invalid"
-    elsif parsed_token.find != self
-      raise ArgumentError, "The token was generated for a different user"
-    elsif identity.email_address != parsed_token.params.fetch("old_email_address")
-      raise ArgumentError, "The token was generated for a different email address"
+    old_email_address = parsed_token&.params&.fetch("old_email_address")
+    new_email_address = parsed_token&.params&.fetch("new_email_address")
+
+    if parsed_token.nil? || parsed_token.find != self || identity.email_address != old_email_address
+      false
     else
-      new_email_address = parsed_token.params.fetch("new_email_address")
       change_email_address(new_email_address)
     end
   end
@@ -37,8 +35,6 @@ module User::EmailAddressChangeable
       new_identity = Identity.find_or_create_by!(email_address: new_email_address)
       update!(identity: new_identity)
     end
-
-    self
   end
 
   private

app/views/users/email_addresses/confirmations/invalid_token.html.erb

@@ -0,0 +1,16 @@
+<% @page_title = "Link expired" %>
+
+<div class="panel panel--centered center flex flex-column gap-half" style="--panel-size: 50ch;">
+  <h1 class="txt-x-large font-weight-black margin-none">
+    <%= @page_title %>
+  </h1>
+  <p class="margin-none-block-start margin-block-end-half">
+    That email confirmation link is no longer valid—they expire after 30 minues. You’ll have to try again.
+  </p>
+
+  <%= link_to "Change my email address", new_user_email_address_path(user_id: @user, script_name: @user.account.slug), class: "btn btn--link center" %>
+
+  <p class="txt-small txt-subtle">
+    If you get stuck, <%= mail_to "support@fizzy.do", "send us an email" %> and we’ll get you back on track.
+  </p>
+</div>

app/views/users/email_addresses/confirmations/show.html.erb

@@ -1,11 +1,11 @@
 <% @page_title = "Confirm email change" %>
 
-<div class="panel shadow center flex flex-column gap-half" style="--panel-size: 45ch;">
+<div class="panel panel--centered center flex flex-column gap-half" style="--panel-size: 45ch;">
   <header>
     <h1 class="txt-x-large font-weight-black margin-none">
       <%= @page_title %>
     </h1>
-    <p class="margin-none">Just a sec while we confirm your new email address.</p>
+    <p class="margin-none-block-start margin-block-end-half">Just a sec while we confirm your new email address.</p>
   </header>
 
   <%= form_with url: user_email_address_confirmation_path(user_id: @user.id), method: :post, data: { controller: "form auto-submit" } do |form| %>

app/views/users/email_addresses/create.html.erb

@@ -6,10 +6,10 @@
   </div>
 <% end %>
 
-<div class="panel panel--centered flex flex-column gap-half">
+<div class="panel panel--centered flex flex-column gap-half" style="--panel-size: 50ch;">
   <h1 class="txt-x-large font-weight-black margin-none">Check your email</h1>
   <p class="margin-none">We just sent an email to <strong><%= params[:email_address] %></strong></p>
-  <p class="margin-none-block-start">Hit the link in the email to confirm this is the email address you want to use with Fizzy going forward.</p>
+  <p class="margin-none-block-start margin-block-end-half">Hit the link in the email to confirm this is the email address you want to use with Fizzy going forward.</p>
 
   <%= link_to "Done", edit_user_path(@user, script_name: @user.account.slug), class: "btn btn--link center" %>
 </div>

app/views/users/email_addresses/new.html.erb

@@ -6,7 +6,7 @@
   </div>
 <% end %>
 
-<div class="panel panel--centered flex flex-column gap-half">
+<div class="panel panel--centered flex flex-column gap-half" style="--panel-size: 45ch;">
   <h1 class="txt-x-large font-weight-black margin-none">
     <%= @page_title %>
   </h1>
@@ -18,7 +18,7 @@
       </label>
     </div>
 
-    <p class="margin-none">Enter your new email address, then hit the link to confirm it</p>
+    <p class="margin-none-block-start margin-block-end-half">Enter your new email address, then check your email to confirm the change.</p>
 
     <button type="submit" class="btn btn--link center" data-form-target="submit">
       <span>Continue</span>

test/controllers/users/email_addresses/confirmations_controller_test.rb

@@ -3,6 +3,7 @@ require "test_helper"
 class Users::EmailAddresses::ConfirmationsControllerTest < ActionDispatch::IntegrationTest
   setup do
     @user = users(:david)
+    @old_email = @user.identity.email_address
     @new_email = "newemail@example.com"
     @token = @user.send(:generate_email_address_change_token, to: @new_email)
   end
@@ -13,8 +14,6 @@ class Users::EmailAddresses::ConfirmationsControllerTest < ActionDispatch::Integ
   end
 
   test "create" do
-    old_email = @user.identity.email_address
-
     post user_email_address_confirmation_path(user_id: @user.id, email_address_token: @token, script_name: @user.account.slug)
 
     assert_equal @new_email, @user.reload.identity.email_address
@@ -22,8 +21,10 @@ class Users::EmailAddresses::ConfirmationsControllerTest < ActionDispatch::Integ
   end
 
   test "create with invalid token" do
-    assert_raises(ArgumentError) do
-      post user_email_address_confirmation_path(user_id: @user.id, email_address_token: "invalid", script_name: @user.account.slug)
-    end
+    post user_email_address_confirmation_path(user_id: @user.id, email_address_token: "invalid", script_name: @user.account.slug)
+
+    assert_equal @user.identity.email_address, @old_email
+    assert_response :unprocessable_entity
+    assert_match /Link expired/, response.body
   end
 end

test/models/user/email_address_changeable_test.rb

@@ -7,6 +7,7 @@ class User::EmailAddressChangeableTest < ActiveSupport::TestCase
     @identity = identities(:kevin)
     @user = @identity.users.find_by!(account: accounts("37s"))
     @new_email = "newart@example.com"
+    @old_email = @identity.email_address
   end
 
   test "send_email_address_change_confirmation" do
@@ -42,15 +43,15 @@ class User::EmailAddressChangeableTest < ActiveSupport::TestCase
   end
 
   test "change_email_address_using_token with invalid token" do
-    assert_raises(ArgumentError, match: /invalid/) do
-      @user.change_email_address_using_token("invalid_token")
-    end
+    assert_not @user.change_email_address_using_token("invalid_token")
+    assert_equal @old_email, @user.reload.identity.email_address
 
     token = @user.send(:generate_email_address_change_token, to: @new_email)
-    @identity.update!(email_address: "different@example.com")
+    old_email = "#{SecureRandom.hex(16)}@example.com"
+    @identity.update!(email_address: old_email)
+    @user.reload
 
-    assert_raises(ArgumentError, match: /different email address/) do
-      @user.change_email_address_using_token(token)
-    end
+    assert_not @user.change_email_address_using_token(token)
+    assert_equal old_email, @user.reload.identity.email_address
   end
 end