diff --git a/app/controllers/auth_services_controller.rb b/app/controllers/auth_services_controller.rb index 3e1097dcf..ab2676592 100644 --- a/app/controllers/auth_services_controller.rb +++ b/app/controllers/auth_services_controller.rb @@ -44,13 +44,26 @@ def create uid: omnihash[:uid] ) - member.save! + created = false + begin + member.save! + created = true + rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotUnique + # Concurrent OAuth callback for the same account: another request just + # created this member/auth_service (both email and (uid, provider) are + # unique), so our insert/validation collided with it. Reuse the winner + # instead of erroring. find_by! is the guard: if no winner exists this + # isn't a race and we surface the real error. + member_service = AuthService.find_by!(provider: omnihash[:provider], + uid: omnihash[:uid]) + member = member_service.member + end # Set, not Toggle: toggling would flip can_log_in off for a member who # links a second auth service. Skip the conditional profile validations # here on purpose — a brand-new member completes name/about_you on the # next (details) page, and running them now aborts the signup callback. - member.update_column(:can_log_in, true) # rubocop:disable Rails/SkipsModelValidations + member.update_column(:can_log_in, true) if created # rubocop:disable Rails/SkipsModelValidations session[:member_id] = member.id session[:service_id] = member_service.id diff --git a/spec/requests/auth_services_callback_spec.rb b/spec/requests/auth_services_callback_spec.rb new file mode 100644 index 000000000..ae04ea59f --- /dev/null +++ b/spec/requests/auth_services_callback_spec.rb @@ -0,0 +1,26 @@ +require 'rails_helper' + +RSpec.describe 'AuthServices callback' do + it 'reuses the winner when a concurrent callback just created the same auth service' do + winner = Fabricate(:member, email: 'winner@example.com') + winner_service = Fabricate(:auth_service, member: winner, + provider: 'github', + uid: 'race-uid-123') + + mock_auth_hash(provider: 'github', uid: 'race-uid-123', + email: 'loser@example.com') + + # Simulate the losing callback: it read the DB *before* the winner committed, + # so its initial find_by saw nothing; the insert then collides and the rescue + # re-finds the winner via find_by!. + allow(AuthService).to receive_messages(find_by: nil, find_by!: winner_service) + + expect { post '/auth/github/callback' }.not_to raise_error + + expect(response).to redirect_to(edit_member_details_path) + expect(session[:member_id]).to eq(winner.id) + expect(session[:service_id]).to eq(winner_service.id) + # the losing callback must not flip the winner's can_log_in flag back + expect(winner.reload.can_log_in).to be(false) + end +end