From 03026a721e2a05e31cd9be548ec27ff9a2bc5b49 Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Mon, 17 Jun 2024 20:42:59 -0700 Subject: [PATCH] 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 --- .../email_confirmations_controller.rb | 8 +- app/models/user.rb | 3 +- test/factories/user.rb | 5 + .../email_confirmations_controller_test.rb | 122 ++++++++++++++---- test/integration/email_confirmation_test.rb | 21 +-- test/integration/sign_up_test.rb | 8 +- 6 files changed, 129 insertions(+), 38 deletions(-) diff --git a/app/controllers/email_confirmations_controller.rb b/app/controllers/email_confirmations_controller.rb index 223deb8ced0..46b1401eaed 100644 --- a/app/controllers/email_confirmations_controller.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 32ddbd61510..8503e11c7b7 100644 --- a/app/models/user.rb +++ b/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 diff --git a/test/factories/user.rb b/test/factories/user.rb index 7a1ca820648..8e7460601b5 100644 --- a/test/factories/user.rb +++ b/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"] } diff --git a/test/functional/email_confirmations_controller_test.rb b/test/functional/email_confirmations_controller_test.rb index 71a5a378d86..771951970b2 100644 --- a/test/functional/email_confirmations_controller_test.rb +++ b/test/functional/email_confirmations_controller_test.rb @@ -5,7 +5,7 @@ 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 @@ -13,9 +13,27 @@ class EmailConfirmationsControllerTest < ActionController::TestCase 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: "new@example.com") + 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: "new@example.com") + + 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,17 +292,58 @@ 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: "new@example.com") + @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 "new@example.com", @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 @@ -270,7 +351,6 @@ class EmailConfirmationsControllerTest < ActionController::TestCase 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 diff --git a/test/integration/email_confirmation_test.rb b/test/integration/email_confirmation_test.rb index 275ea33e32d..d55eaa3c727 100644 --- a/test/integration/email_confirmation_test.rb +++ b/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 diff --git a/test/integration/sign_up_test.rb b/test/integration/sign_up_test.rb index a8b119bbe3e..5b1128b5499 100644 --- a/test/integration/sign_up_test.rb +++ b/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@person.com" + fill_in "Password", with: PasswordHelpers::SECURE_TEST_PASSWORD + click_button "Sign in" + + assert page.has_content? "Sign out" end teardown do