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/app/models/signup.rb b/app/models/signup.rb index 15ac5873c..110c253ef 100644 --- a/app/models/signup.rb +++ b/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 diff --git a/test/controllers/join_codes_controller_test.rb b/test/controllers/join_codes_controller_test.rb index 0a4bf5793..9558b605a 100644 --- a/test/controllers/join_codes_controller_test.rb +++ b/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 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/models/signup_test.rb b/test/models/signup_test.rb index 30910bf81..589bcd49a 100644 --- a/test/models/signup_test.rb +++ b/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 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