No auto sign in after email confirmation (#4810)

* Don't sign in user on email confirmation so that all users use the main path to sign in.
* Remove unused user_id in tests leftover from previous change

app/controllers/email_confirmations_controller.rb

@@ -8,7 +8,7 @@ class EmailConfirmationsController < ApplicationController
   before_action :redirect_to_new_mfa, if: :mfa_required_not_yet_enabled?, only: :unconfirmed
   before_action :redirect_to_settings_strong_mfa_required, if: :mfa_required_weak_level_enabled?, only: :unconfirmed
   before_action :validate_confirmation_token, only: %i[update otp_update webauthn_update]
-  before_action :require_mfa, only: %i[update]
+  before_action :require_mfa, only: :update
   before_action :validate_otp, only: :otp_update
   before_action :validate_webauthn, only: :webauthn_update
   after_action :delete_mfa_expiry_session, only: %i[otp_update webauthn_update]
@@ -63,11 +63,11 @@ def validate_confirmation_token
 
   def confirm_email
     if @user.confirm_email!
-      sign_in @user
-      redirect_to root_path, notice: t("email_confirmations.update.confirmed_email")
+      flash[:notice] = t("email_confirmations.update.confirmed_email")
     else
-      redirect_to root_path, alert: @user.errors.full_messages.to_sentence
+      flash[:alert] = @user.errors.full_messages.to_sentence
     end
+    redirect_to signed_in? ? dashboard_path : sign_in_path
   end
 
   def email_params

app/models/user.rb

@@ -280,8 +280,7 @@ def ld_context
   private
 
   def update_email
-    self.attributes = { email: unconfirmed_email, unconfirmed_email: nil, mail_fails: 0 }
-    save
+    update(email: unconfirmed_email, unconfirmed_email: nil, mail_fails: 0)
   end
 
   def unconfirmed_email_uniqueness

test/factories/user.rb

@@ -13,6 +13,11 @@
     end
     mfa_hashed_recovery_codes { mfa_recovery_codes.map { |code| BCrypt::Password.create(code) } }
 
+    trait :unconfirmed do
+      email_confirmed { false }
+      unconfirmed_email { "#{SecureRandom.hex(8)}#{email}" }
+    end
+
     trait :mfa_enabled do
       totp_seed { "123abc" }
       mfa_level { User.mfa_levels["ui_and_api"] }

test/functional/email_confirmations_controller_test.rb

@@ -5,17 +5,35 @@ class EmailConfirmationsControllerTest < ActionController::TestCase
   include ActiveJob::TestHelper
 
   context "on GET to update" do
-    setup { @user = create(:user) }
+    setup { @user = create(:user, :unconfirmed) }
 
     context "user exists and token has not expired" do
       setup do
         get :update, params: { token: @user.confirmation_token }
       end
 
       should "should confirm user account" do
-        assert @user.email_confirmed
+        assert @user.reload.email_confirmed
+      end
+      should "not sign in user" do
+        refute cookies[:remember_token]
+      end
+    end
+
+    context "successful confirmation while signed in" do
+      setup do
+        @user.confirm_email! # must be confirmed to sign in
+        sign_in_as(@user)
+        @user.update!(unconfirmed_email: "[email protected]")
+        get :update, params: { token: @user.confirmation_token }
+      end
+
+      should redirect_to("the dashboard") { dashboard_url }
+
+      should "should confirm user account" do
+        assert @user.reload.email_confirmed
       end
-      should "sign in user" do
+      should "keep the user signed in" do
         assert cookies[:remember_token]
       end
     end
@@ -65,7 +83,7 @@ class EmailConfirmationsControllerTest < ActionController::TestCase
         get :update, params: { token: @user.confirmation_token }
       end
 
-      should redirect_to("the homepage") { root_url }
+      should redirect_to("the sign in page") { sign_in_url }
 
       should "confirm email for first user" do
         assert_equal @email, @user.reload.email
@@ -76,7 +94,7 @@ class EmailConfirmationsControllerTest < ActionController::TestCase
           get :update, params: { token: @second_user.confirmation_token }
         end
 
-        should "show error to second user on confirmation request and not " do
+        should "show error to second user on confirmation request" do
           assert_equal "Email address has already been taken", flash[:alert]
         end
 
@@ -162,29 +180,52 @@ class EmailConfirmationsControllerTest < ActionController::TestCase
   context "on POST to otp_update" do
     context "user has mfa enabled" do
       setup do
-        @user = create(:user)
+        @user = create(:user, :unconfirmed)
         @user.enable_totp!(ROTP::Base32.random_base32, :ui_only)
       end
 
       context "when OTP is correct" do
         setup do
-          get :update, params: { token: @user.confirmation_token, user_id: @user.id }
+          get :update, params: { token: @user.confirmation_token }
           post :otp_update, params: { token: @user.confirmation_token, otp: ROTP::TOTP.new(@user.totp_seed).now }
         end
 
-        should redirect_to("the homepage") { root_url }
+        should set_flash[:notice]
+        should redirect_to("the sign in page") { sign_in_url }
 
         should "should confirm user account" do
-          assert @user.email_confirmed
+          assert @user.reload.email_confirmed
         end
+
         should "clear mfa_expires_at" do
           assert_nil @controller.session[:mfa_expires_at]
         end
       end
 
+      context "user is already signed in and OTP is correct" do
+        setup do
+          @user.confirm_email!
+          sign_in_as(@user)
+          @user.update!(unconfirmed_email: "[email protected]")
+
+          assert @user.confirmation_token
+          get :update, params: { token: @user.confirmation_token }
+          post :otp_update, params: { token: @user.confirmation_token, otp: ROTP::TOTP.new(@user.totp_seed).now }
+        end
+
+        should redirect_to("the dashboard") { dashboard_url }
+
+        should "should confirm user account" do
+          assert @user.reload.email_confirmed
+        end
+        should "keep the user signed in" do
+          assert cookies[:remember_token]
+        end
+      end
+
       context "when OTP is incorrect" do
         setup do
-          get :update, params: { token: @user.confirmation_token, user_id: @user.id }
+          get :update, params: { token: @user.confirmation_token }
           post :otp_update, params: { token: @user.confirmation_token, otp: "incorrect" }
         end
 
@@ -197,7 +238,7 @@ class EmailConfirmationsControllerTest < ActionController::TestCase
 
       context "when the OTP session is expired" do
         setup do
-          get :update, params: { token: @user.confirmation_token, user_id: @user.id }
+          get :update, params: { token: @user.confirmation_token }
           travel 16.minutes do
             post :otp_update, params: { token: @user.confirmation_token, otp: ROTP::TOTP.new(@user.totp_seed).now }
           end
@@ -223,9 +264,9 @@ class EmailConfirmationsControllerTest < ActionController::TestCase
 
   context "on POST to webauthn_update" do
     setup do
-      @user = create(:user)
+      @user = create(:user, :unconfirmed)
       @webauthn_credential = create(:webauthn_credential, user: @user)
-      get :update, params: { token: @user.confirmation_token, user_id: @user.id }
+      get :update, params: { token: @user.confirmation_token }
       @origin = WebAuthn.configuration.origin
       @rp_id = URI.parse(@origin).host
       @client = WebAuthn::FakeClient.new(@origin, encoding: false)
@@ -241,7 +282,6 @@ class EmailConfirmationsControllerTest < ActionController::TestCase
         post(
           :webauthn_update,
           params: {
-            user_id: @user.id,
             token: @user.confirmation_token,
             credentials:
             WebauthnHelpers.get_result(
@@ -252,25 +292,65 @@ class EmailConfirmationsControllerTest < ActionController::TestCase
         )
       end
 
-      should "redirect to root" do
-        assert_redirected_to root_url
+      should redirect_to("the sign in page") { sign_in_url }
+
+      should "change the user's email" do
+        assert @user.reload.email_confirmed
       end
 
+      should "clear mfa_expires_at" do
+        assert_nil @controller.session[:mfa_expires_at]
+      end
+
+      should "set flash notice" do
+        assert_equal "Your email address has been verified.", flash[:notice]
+      end
+    end
+
+    context "while signed in with successful webauthn" do
+      setup do
+        @user.confirm_email!
+        sign_in_as(@user)
+        @user.update!(unconfirmed_email: "[email protected]")
+        @challenge = session[:webauthn_authentication]["challenge"]
+        WebauthnHelpers.create_credential(
+          webauthn_credential: @webauthn_credential,
+          client: @client
+        )
+        post(
+          :webauthn_update,
+          params: {
+            token: @user.confirmation_token,
+            credentials:
+            WebauthnHelpers.get_result(
+              client: @client,
+              challenge: @challenge
+            )
+          }
+        )
+      end
+
+      should redirect_to("the dashboard") { dashboard_url }
+
       should "change the user's email" do
         assert @user.reload.email_confirmed
+        assert_equal "[email protected]", @user.email
       end
 
       should "clear mfa_expires_at" do
         assert_nil @controller.session[:mfa_expires_at]
       end
+
+      should "set flash notice" do
+        assert_equal "Your email address has been verified.", flash[:notice]
+      end
     end
 
     context "when not providing credentials" do
       setup do
         post(
           :webauthn_update,
           params: {
-            user_id: @user.id,
             token: @user.confirmation_token
           }
         )
@@ -293,7 +373,6 @@ class EmailConfirmationsControllerTest < ActionController::TestCase
         post(
           :webauthn_update,
           params: {
-            user_id: @user.id,
             token: @user.confirmation_token,
             credentials:
             WebauthnHelpers.get_result(
@@ -325,7 +404,6 @@ class EmailConfirmationsControllerTest < ActionController::TestCase
           post(
             :webauthn_update,
             params: {
-              user_id: @user.id,
               token: @user.confirmation_token,
               credentials:
               WebauthnHelpers.get_result(
@@ -493,7 +571,7 @@ class EmailConfirmationsControllerTest < ActionController::TestCase
             end
 
             should "should confirm user account" do
-              assert @user.email_confirmed
+              assert @user.reload.email_confirmed
             end
           end
 
@@ -546,7 +624,7 @@ class EmailConfirmationsControllerTest < ActionController::TestCase
             end
 
             should "should confirm user account" do
-              assert @user.email_confirmed
+              assert @user.reload.email_confirmed
             end
           end
 
@@ -599,7 +677,7 @@ class EmailConfirmationsControllerTest < ActionController::TestCase
             end
 
             should "should confirm user account" do
-              assert @user.email_confirmed
+              assert @user.reload.email_confirmed
             end
           end
 

test/integration/email_confirmation_test.rb

@@ -30,16 +30,18 @@ def request_confirmation_mail(email)
     assert_not_nil link
     visit link
 
-    assert page.has_content? "Sign out"
+    assert page.has_content? "Sign in"
     assert page.has_selector? "#flash_notice", text: "Your email address has been verified"
   end
 
-  test "re-using confirmation link does not sign in user" do
+  test "re-using confirmation link, asks user to double check the link" do
     request_confirmation_mail @user.email
 
     link = last_email_link
     visit link
-    click_link "Sign out"
+
+    assert page.has_content? "Sign in"
+    assert page.has_selector? "#flash_notice", text: "Your email address has been verified"
 
     visit link
 
@@ -75,7 +77,8 @@ def request_confirmation_mail(email)
     fill_in "otp", with: ROTP::TOTP.new(@user.totp_seed).now
     click_button "Authenticate"
 
-    assert page.has_content? "Sign out"
+    assert page.has_content? "Sign in"
+    assert page.has_selector? "#flash_notice", text: "Your email address has been verified"
   end
 
   test "requesting confirmation mail with webauthn enabled" do
@@ -93,9 +96,10 @@ def request_confirmation_mail(email)
 
     click_on "Authenticate with security device"
 
-    find(:css, ".header__popup-link").click
+    assert page.has_content? "Sign in"
+    skip("There's a glitch where the webauthn javascript(?) triggers the next page to render twice, clearing flash.")
 
-    assert page.has_content?("SIGN OUT")
+    assert page.has_selector? "#flash_notice", text: "Your email address has been verified"
   end
 
   test "requesting confirmation mail with webauthn enabled using recovery codes" do
@@ -114,9 +118,8 @@ def request_confirmation_mail(email)
     fill_in "otp", with: @mfa_recovery_codes.first
     click_button "Authenticate"
 
-    find(:css, ".header__popup-link").click
-
-    assert page.has_content?("SIGN OUT")
+    assert page.has_content? "Sign in"
+    assert page.has_selector? "#flash_notice", text: "Your email address has been verified"
   end
 
   test "requesting confirmation mail with mfa enabled, but mfa session is expired" do

test/integration/sign_up_test.rb

@@ -94,8 +94,14 @@ class SignUpTest < SystemTest
     assert_not_nil link
     visit link
 
-    assert page.has_content? "Sign out"
+    assert page.has_content? "Sign in"
     assert page.has_selector? "#flash_notice", text: "Your email address has been verified"
+
+    fill_in "Email or Username", with: "[email protected]"
+    fill_in "Password", with: PasswordHelpers::SECURE_TEST_PASSWORD
+    click_button "Sign in"
+
+    assert page.has_content? "Sign out"
   end
 
   teardown do