diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index 89eda5887..4cdbbdb69 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -97,6 +97,12 @@ module Authentication end end + def redirect_to_session_magic_link(magic_link, return_to: nil) + serve_development_magic_link(magic_link) + session[:return_to_after_authenticating] = return_to if return_to + redirect_to session_magic_link_url(script_name: nil) + end + def serve_development_magic_link(magic_link) if Rails.env.development? flash[:magic_link_code] = magic_link&.code diff --git a/app/controllers/join_codes_controller.rb b/app/controllers/join_codes_controller.rb index eb328df18..99298880c 100644 --- a/app/controllers/join_codes_controller.rb +++ b/app/controllers/join_codes_controller.rb @@ -20,8 +20,11 @@ class JoinCodesController < ApplicationController elsif identity == Current.identity redirect_to new_users_verification_url(script_name: @join_code.account.slug) else - logout_and_send_new_magic_link(identity) - redirect_to session_magic_link_url(script_name: nil) + terminate_session if Current.identity + + redirect_to_session_magic_link \ + identity.send_magic_link, + return_to: new_users_verification_url(script_name: @join_code.account.slug) end end @@ -37,13 +40,4 @@ class JoinCodesController < ApplicationController render :inactive, status: :gone end end - - def logout_and_send_new_magic_link(identity) - terminate_session if Current.identity - - magic_link = identity.send_magic_link - serve_development_magic_link(magic_link) - - session[:return_to_after_authenticating] = new_users_verification_url(script_name: @join_code.account.slug) - end end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 5335f58f3..fb9fe0617 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -9,17 +9,16 @@ class SessionsController < ApplicationController end def create - identity = Identity.find_by_email_address(email_address) - - magic_link = if identity - identity.send_magic_link + if identity = Identity.find_by_email_address(email_address) + redirect_to_session_magic_link identity.send_magic_link else - Signup.new(email_address: email_address).create_identity + signup = Signup.new(email_address: email_address) + if signup.valid?(:identity_creation) + redirect_to_session_magic_link signup.create_identity + else + head :unprocessable_entity + end end - - serve_development_magic_link(magic_link) - - redirect_to session_magic_link_path end def destroy diff --git a/app/controllers/signups_controller.rb b/app/controllers/signups_controller.rb index 217221378..776421201 100644 --- a/app/controllers/signups_controller.rb +++ b/app/controllers/signups_controller.rb @@ -11,9 +11,12 @@ class SignupsController < ApplicationController end def create - magic_link = Signup.new(signup_params).create_identity - serve_development_magic_link(magic_link) - redirect_to session_magic_link_path + signup = Signup.new(signup_params) + if signup.valid?(:identity_creation) + redirect_to_session_magic_link signup.create_identity + else + head :unprocessable_entity + end end private diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb index f2318bf46..7ebe4d518 100644 --- a/test/controllers/sessions_controller_test.rb +++ b/test/controllers/sessions_controller_test.rb @@ -36,15 +36,29 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest end end - private - test "destroy" do - sign_in_as :kevin - + test "create with invalid email address" do + # Avoid Sentry exceptions when attackers try to stuff invalid emails. The browser performs form + # field validation that should normally prevent this from occurring, so I'm not worried about + # returning proper validation errors. + without_action_dispatch_exception_handling do untenanted do - delete session_path + assert_no_difference -> { Identity.count } do + post session_path, params: { email_address: "not-a-valid-email" } + end - assert_redirected_to new_session_path - assert_not cookies[:session_token].present? + assert_response :unprocessable_entity end end + end + + test "destroy" do + sign_in_as :kevin + + untenanted do + delete session_path + + assert_redirected_to new_session_path + assert_not cookies[:session_token].present? + end + end end diff --git a/test/controllers/signups_controller_test.rb b/test/controllers/signups_controller_test.rb index 83fea8882..17314ee90 100644 --- a/test/controllers/signups_controller_test.rb +++ b/test/controllers/signups_controller_test.rb @@ -34,15 +34,17 @@ class SignupsControllerTest < ActionDispatch::IntegrationTest end end - test "create with email address containing blanks" do - untenanted do - assert_no_difference -> { Identity.count } do - assert_no_difference -> { MagicLink.count } do - post signup_path, params: { signup: { email_address: "sam smith@example.com" } } + test "create with invalid email address" do + without_action_dispatch_exception_handling do + untenanted do + assert_no_difference -> { Identity.count } do + assert_no_difference -> { MagicLink.count } do + post signup_path, params: { signup: { email_address: "not-a-valid-email" } } + end end - end - assert_response :unprocessable_entity + assert_response :unprocessable_entity + end end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 03cda99db..4cec836fd 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -61,6 +61,15 @@ class ActionDispatch::IntegrationTest setup do integration_session.default_url_options[:script_name] = "/#{ActiveRecord::FixtureSet.identify("37signals")}" end + + private + def without_action_dispatch_exception_handling + original = Rails.application.config.action_dispatch.show_exceptions + Rails.application.config.action_dispatch.show_exceptions = :none + yield + ensure + Rails.application.config.action_dispatch.show_exceptions = original + end end class ActionDispatch::SystemTestCase