Validate email before creating identity during sign-up and sign-in

Avoid Sentry exceptions when attackers try to stuff invalid emails. The
browser performs form field validation that should normally prevent this
from occurring, so we just return 422 without validation error messages.

Also:

- extract redirect_to_session_magic_link helper
- some controller refactoring and cleanup

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

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

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

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

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

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
 

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