fix horribly broken invite code, could lead to inviting the wrong person to a conversation

app/controllers/admin/impersonate_controller.rb

@@ -3,7 +3,7 @@ class Admin::ImpersonateController < Admin::AdminController
   def create
     params.require(:username_or_email)
 
-    user = User.find_by_username_or_email(params[:username_or_email]).first
+    user = User.find_by_username_or_email(params[:username_or_email])
 
     raise Discourse::NotFound if user.blank?
 

app/controllers/topics_controller.rb

@@ -153,7 +153,7 @@ class TopicsController < ApplicationController
     guardian.ensure_can_invite_to!(topic)
 
     if topic.invite(current_user, params[:user])
-      user = User.find_by_username_or_email(params[:user]).first
+      user = User.find_by_username_or_email(params[:user])
       if user
         render_json_dump BasicUserSerializer.new(user, scope: guardian, root: 'user')
       else

app/models/topic.rb

@@ -381,9 +381,9 @@ class Topic < ActiveRecord::Base
   def invite(invited_by, username_or_email)
     if private_message?
       # If the user exists, add them to the topic.
-      user = User.find_by_username_or_email(username_or_email).first
-      if user.present?
-        if topic_allowed_users.create!(user_id: user.id)
+      user = User.find_by_username_or_email(username_or_email)
+      if user && topic_allowed_users.create!(user_id: user.id)
+
         # Notify the user they've been invited
         user.notifications.create(notification_type: Notification.types[:invited_to_private_message],
                                   topic_id: id,
@@ -392,17 +392,15 @@ class Topic < ActiveRecord::Base
                                           display_username: invited_by.username }.to_json)
         return true
       end
-      elsif username_or_email =~ /^.+@.+$/
-        # If the user doesn't exist, but it looks like an email, invite the user by email.
-        return invite_by_email(invited_by, username_or_email)
-      end
-    else
-      # Success is whether the invite was created
-      return invite_by_email(invited_by, username_or_email).present?
     end
 
+    if username_or_email =~ /^.+@.+$/
+      # NOTE callers expect an invite object if an invite was sent via email
+      invite_by_email(invited_by, username_or_email)
+    else
       false
     end
+  end
 
   # Invite a user by email and return the invite. Return the previously existing invite
   # if already exists. Returns nil if the invite can't be created.

app/models/user.rb

@@ -133,7 +133,22 @@ class User < ActiveRecord::Base
   def self.find_by_username_or_email(username_or_email)
     lower_user = username_or_email.downcase
     lower_email = Email.downcase(username_or_email)
-    where("username_lower = :user or lower(username) = :user or email = :email or lower(name) = :user", user: lower_user, email: lower_email)
+
+    users =
+      if username_or_email.include?('@')
+        User.where(email: lower_email)
+      else
+        User.where(username_lower: lower_user)
+      end
+        .to_a
+
+    if users.count > 1
+      raise Discourse::TooManyMatches
+    elsif users.count == 1
+      users[0]
+    else
+      nil
+    end
   end
 
   def enqueue_welcome_message(message_type)

lib/discourse.rb

@@ -2,6 +2,9 @@ require 'cache'
 
 module Discourse
 
+  # Expected less matches than what we got in a find
+  class TooManyMatches < Exception; end
+
   # When they try to do something they should be logged in for
   class NotLoggedIn < Exception; end
 

spec/controllers/admin/impersonate_controller_spec.rb

@@ -52,11 +52,6 @@ describe Admin::ImpersonateController do
           session[:current_user_id].should == user.id
         end
 
-        it "also works with a name" do
-          xhr :post, :create, username_or_email: user.name
-          session[:current_user_id].should == user.id
-        end
-
       end
 
     end

spec/models/user_spec.rb

@@ -847,4 +847,19 @@ describe User do
     end
   end
 
+  describe '#find_by_username_or_email' do
+    it 'works correctly' do
+      bob = Fabricate(:user, username: 'bob', name: 'bobs', email: 'bob@bob.com')
+      bob2 = Fabricate(:user, username: 'bob2', name: 'bobs', email: 'bob2@bob.com')
+
+      expect(User.find_by_username_or_email('bob22@bob.com')).to eq(nil)
+      expect(User.find_by_username_or_email('bobs')).to eq(nil)
+
+      expect(User.find_by_username_or_email('bob2')).to eq(bob2)
+      expect(User.find_by_username_or_email('bob2@BOB.com')).to eq(bob2)
+
+      expect(User.find_by_username_or_email('bob')).to eq(bob)
+      expect(User.find_by_username_or_email('bob@BOB.com')).to eq(bob)
+    end
+  end
 end