Merge pull request #1996 from basecamp/flavorjones/email-validation-in-signup-signin

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

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

app/models/signup.rb

@@ -6,9 +6,8 @@ class Signup
   attr_accessor :full_name, :email_address, :identity
   attr_reader :account, :user
 
-  with_options on: :completion do
-    validates_presence_of :full_name, :identity
-  end
+  validates :email_address, format: { with: URI::MailTo::EMAIL_REGEXP }, on: :identity_creation
+  validates :full_name, :identity, presence: true, on: :completion
 
   def initialize(...)
     super

test/controllers/join_codes_controller_test.rb

@@ -69,4 +69,17 @@ class JoinCodesControllerTest < ActionDispatch::IntegrationTest
 
     assert_redirected_to new_users_verification_url(script_name: @account.slug)
   end
+
+  test "create for different identity terminates existing session" do
+    sign_in_as :kevin
+
+    assert_difference -> { Identity.count }, 1 do
+      assert_difference -> { User.count }, 1 do
+        post join_path(code: @join_code.code, script_name: @account.slug), params: { email_address: "new_user@example.com" }
+      end
+    end
+
+    assert_redirected_to session_magic_link_url(script_name: nil)
+    assert_not_predicate cookies[:session_token], :present?
+  end
 end

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/models/signup_test.rb

@@ -1,15 +1,26 @@
 require "test_helper"
 
 class SignupTest < ActiveSupport::TestCase
+  test "validates email format for identity creation" do
+    signup = Signup.new(email_address: "not-an-email")
+    assert_not signup.valid?(:identity_creation)
+    assert signup.errors[:email_address].any?
+
+    signup = Signup.new(email_address: "valid@example.com")
+    assert signup.valid?(:identity_creation)
+  end
+
   test "#create_identity" do
     signup = Signup.new(email_address: "brian@example.com")
 
+    magic_link = nil
     assert_difference -> { Identity.count }, 1 do
       assert_difference -> { MagicLink.count }, 1 do
-        assert signup.create_identity
+        magic_link = signup.create_identity
       end
     end
 
+    assert_kind_of MagicLink, magic_link
     assert_empty signup.errors
     assert signup.identity
     assert signup.identity.persisted?
@@ -18,10 +29,12 @@ class SignupTest < ActiveSupport::TestCase
 
     assert_no_difference -> { Identity.count } do
       assert_difference -> { MagicLink.count }, 1 do
-        assert signup_existing.create_identity, "Should send magic link for existing identity"
+        magic_link = signup_existing.create_identity
       end
     end
 
+    assert_kind_of MagicLink, magic_link
+
     signup_invalid = Signup.new(email_address: "")
     assert_raises do
       signup_invalid.create_identity
@@ -48,4 +61,15 @@ class SignupTest < ActiveSupport::TestCase
       assert_not_empty signup_invalid.errors[:full_name]
     end
   end
+
+  test "#complete with invalid data" do
+    Current.without_account do
+      signup = Signup.new
+      assert_not signup.complete
+      assert signup.errors[:full_name].any?
+      assert signup.errors[:identity].any?
+      assert_nil signup.account
+      assert_nil signup.user
+    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