Merge pull request #2086 from basecamp/harden-magic-links

Pin sign in attempts to the current session

app/controllers/concerns/authentication.rb

@@ -6,6 +6,7 @@ module Authentication
     before_action :require_authentication
     after_action :ensure_development_magic_link_not_leaked
     helper_method :authenticated?
+    helper_method :email_address_pending_authentication
 
     etag { Current.identity.id if authenticated? }
 
@@ -107,8 +108,22 @@ module Authentication
       end
     end
 
+    def email_address_pending_authentication_matches?(email_address)
+      if ActiveSupport::SecurityUtils.secure_compare(email_address, email_address_pending_authentication || "")
+        session.delete(:email_address_pending_authentication)
+        true
+      else
+        false
+      end
+    end
+
+    def email_address_pending_authentication
+      session[:email_address_pending_authentication]
+    end
+
     def redirect_to_session_magic_link(magic_link, return_to: nil)
       serve_development_magic_link(magic_link)
+      session[:email_address_pending_authentication] = magic_link.identity.email_address if magic_link
       session[:return_to_after_authenticating] = return_to if return_to
       redirect_to main_app.session_magic_link_url(script_name: nil)
     end

app/controllers/sessions/magic_links_controller.rb

@@ -2,6 +2,7 @@ class Sessions::MagicLinksController < ApplicationController
   disallow_account_scope
   require_unauthenticated_access
   rate_limit to: 10, within: 15.minutes, only: :create, with: -> { redirect_to session_magic_link_path, alert: "Wait 15 minutes, then try again" }
+  before_action :ensure_that_email_address_pending_authentication_exists
 
   layout "public"
 
@@ -10,14 +11,28 @@ class Sessions::MagicLinksController < ApplicationController
 
   def create
     if magic_link = MagicLink.consume(code)
-      start_new_session_for magic_link.identity
-      redirect_to after_sign_in_url(magic_link)
+      authenticate_with magic_link
     else
       redirect_to session_magic_link_path, flash: { shake: true }
     end
   end
 
   private
+    def ensure_that_email_address_pending_authentication_exists
+      unless email_address_pending_authentication.present?
+        redirect_to new_session_path, alert: "Enter your email address to sign in."
+      end
+    end
+
+    def authenticate_with(magic_link)
+      if email_address_pending_authentication_matches?(magic_link.identity.email_address)
+        start_new_session_for magic_link.identity
+        redirect_to after_sign_in_url(magic_link)
+      else
+        redirect_to new_session_path, alert: "Authentication failed. Please try again."
+      end
+    end
+
     def code
       params.expect(:code)
     end

app/controllers/sessions_controller.rb

@@ -14,8 +14,8 @@ class SessionsController < ApplicationController
     else
       signup = Signup.new(email_address: email_address)
       if signup.valid?(:identity_creation)
-        identity = signup.create_identity if Account.accepting_signups?
-        redirect_to_session_magic_link identity
+        magic_link = signup.create_identity if Account.accepting_signups?
+        redirect_to_session_magic_link magic_link
       else
         head :unprocessable_entity
       end

app/views/sessions/magic_links/show.html.erb

@@ -15,7 +15,7 @@
         data: { magic_link_target: "input", action: "keydown.enter->magic-link#submitOnEnter paste->magic-link#submitOnPaste" } %>
   <% end %>
 
-  <p class="txt-small">The code you receive will work for <%= distance_of_time_in_words(MagicLink::EXPIRATION_TIME) %>.</p>
+  <p class="txt-small">The code sent to <strong><%= email_address_pending_authentication %></strong> will work for <%= distance_of_time_in_words(MagicLink::EXPIRATION_TIME) %>.</p>
 </div>
 
 <% if Rails.env.development? && flash[:magic_link_code].present? %>

test/controllers/sessions/magic_links_controller_test.rb

@@ -5,6 +5,14 @@ class Sessions::MagicLinksControllerTest < ActionDispatch::IntegrationTest
     untenanted do
       get session_magic_link_url
 
+      assert_response :redirect, "Without an email address pending authentication, should redirect"
+      assert_redirected_to new_session_path
+    end
+
+    untenanted do
+      post session_path, params: { email_address: "test@example.com" }
+      get session_magic_link_url
+
       assert_response :success
     end
   end
@@ -14,6 +22,7 @@ class Sessions::MagicLinksControllerTest < ActionDispatch::IntegrationTest
     magic_link = MagicLink.create!(identity: identity)
 
     untenanted do
+      post session_path, params: { email_address: identity.email_address }
       post session_magic_link_url, params: { code: magic_link.code }
 
       assert_response :redirect
@@ -28,6 +37,7 @@ class Sessions::MagicLinksControllerTest < ActionDispatch::IntegrationTest
     magic_link = MagicLink.create!(identity: identity, purpose: :sign_up)
 
     untenanted do
+      post session_path, params: { email_address: identity.email_address }
       post session_magic_link_url, params: { code: magic_link.code }
 
       assert_response :redirect
@@ -37,6 +47,20 @@ class Sessions::MagicLinksControllerTest < ActionDispatch::IntegrationTest
     end
   end
 
+  test "create with cross-user code" do
+    identity = identities(:kevin)
+    other_identity = identities(:jason)
+    magic_link = MagicLink.create!(identity: other_identity)
+
+    untenanted do
+      post session_path, params: { email_address: identity.email_address }
+      post session_magic_link_url, params: { code: magic_link.code }
+
+      assert_redirected_to new_session_path
+      assert_not cookies[:session_token].present?
+    end
+  end
+
   test "create with invalid code" do
     identity = identities(:kevin)
     magic_link = MagicLink.create!(identity: identity)

test/test_helpers/session_test_helper.rb

@@ -18,6 +18,7 @@ module SessionTestHelper
     magic_link = identity.magic_links.order(id: :desc).first
 
     untenanted do
+      post session_path, params: { email_address: identity.email_address }
       post session_magic_link_url, params: { code: magic_link.code }
     end