Merge pull request #1849 from basecamp/prevent-crash-in-join-code-race-condition

Fix crash in join code redemption race condition

Author: monorkin

app/controllers/join_codes_controller.rb

@@ -12,7 +12,7 @@ def new
   def create
     identity = Identity.find_or_create_by!(email_address: params.expect(:email_address))
 
-    @join_code.redeem { |account| identity.join(account) } unless identity.member_of?(@join_code.account)
+    @join_code.redeem_if { |account| identity.join(account) }
     user = User.active.find_by!(account: @join_code.account, identity: identity)
 
     if identity == Current.identity && user.setup?

app/models/account/join_code.rb

@@ -7,10 +7,9 @@ class Account::JoinCode < ApplicationRecord
 
   before_create :generate_code, if: -> { code.blank? }
 
-  def redeem
+  def redeem_if(&block)
     transaction do
-      increment!(:usage_count)
-      yield account if block_given?
+      increment!(:usage_count) if block.call(account)
     end
   end
 

app/models/identity/joinable.rb

@@ -5,11 +5,9 @@ def join(account, **attributes)
     attributes[:name] ||= email_address
 
     transaction do
-      account.users.create!(**attributes, identity: self)
+      account.users.find_or_create_by!(identity: self) do |user|
+        user.assign_attributes(attributes)
+      end.previously_new_record?
     end
   end
-
-  def member_of?(account)
-    account.users.exists?(identity: self)
-  end
 end

test/controllers/join_codes_controller_test.rb

@@ -43,7 +43,7 @@ class JoinCodesControllerTest < ActionDispatch::IntegrationTest
     identity = identities(:jz)
     sign_in_as :jz
 
-    assert identity.member_of?(@account), "JZ should be a member of 37s for this test"
+    assert identity.users.exists?(account: @account), "JZ should be a member of 37s for this test"
     assert identity.users.find_by!(account: @account).setup?, "JZ's user should be setup for this test"
 
     assert_no_difference -> { Identity.count } do
@@ -59,7 +59,7 @@ class JoinCodesControllerTest < ActionDispatch::IntegrationTest
     identity = identities(:mike)
     sign_in_as :mike
 
-    assert_not identity.member_of?(@account), "Mike should not be a member of 37s for this test"
+    assert_not identity.users.exists?(account: @account), "Mike should not be a member of 37s for this test"
 
     assert_no_difference -> { Identity.count } do
       assert_difference -> { User.count }, 1 do

test/models/account/join_code_test.rb

@@ -10,11 +10,19 @@ class Account::JoinCodeTest < ActiveSupport::TestCase
     assert_equal 3, parts.count
   end
 
-  test "redeem" do
+  test "redeem_if increments usage_count when block returns true" do
     join_code = account_join_codes(:"37s")
 
-    assert_difference -> { join_code.reload.usage_count }, 1 do
-      join_code.redeem
+    assert_difference -> { join_code.reload.usage_count }, +1 do
+      join_code.redeem_if { true }
+    end
+  end
+
+  test "redeem_if does not increment usage_count when block returns false" do
+    join_code = account_join_codes(:"37s")
+
+    assert_no_difference -> { join_code.reload.usage_count } do
+      join_code.redeem_if { false }
     end
   end
 

test/models/identity/joinable_test.rb

@@ -1,25 +1,37 @@
 require "test_helper"
 
 class Identity::JoinableTest < ActiveSupport::TestCase
-  test "join" do
+  test "join creates a new user and returns true" do
     identity = identities(:david)
 
-    user = identity.join(accounts(:initech))
-    assert_kind_of User, user
-    assert_equal accounts(:initech), user.account
+    assert_difference -> { User.count }, 1 do
+      result = identity.join(accounts(:initech))
+      assert result, "join should return true when creating a new user"
+    end
+
+    user = identity.users.find_by!(account: accounts(:initech))
     assert_equal identity.email_address, user.name
+  end
 
+  test "join with custom attributes" do
     identity = identities(:mike)
 
-    user = identity.join(accounts("37s"), name: "Mike")
-    assert_kind_of User, user
-    assert_equal accounts("37s"), user.account
+    result = identity.join(accounts("37s"), name: "Mike")
+    assert result
+
+    user = identity.users.find_by!(account: accounts("37s"))
     assert_equal "Mike", user.name
   end
 
-  test "member_of?" do
+  test "join returns false if user already exists" do
     identity = identities(:david)
-    assert identity.member_of?(accounts("37s"))
-    assert_not identity.member_of?(accounts(:initech))
+    account = accounts("37s")
+
+    assert identity.users.exists?(account: account), "David should already be a member of 37s"
+
+    assert_no_difference -> { User.count } do
+      result = identity.join(account)
+      assert_not result, "join should return false when user already exists"
+    end
   end
 end