From 799b4027784c5ca0b1f3eebc7b5126e8d2f55b9f Mon Sep 17 00:00:00 2001 From: Sam Date: Wed, 19 Jun 2013 10:31:19 +1000 Subject: [PATCH] fix horribly broken invite code, could lead to inviting the wrong person to a conversation --- .../admin/impersonate_controller.rb | 2 +- app/controllers/topics_controller.rb | 2 +- app/models/topic.rb | 34 +++++++++---------- app/models/user.rb | 17 +++++++++- lib/discourse.rb | 3 ++ .../admin/impersonate_controller_spec.rb | 5 --- spec/models/user_spec.rb | 15 ++++++++ 7 files changed, 52 insertions(+), 26 deletions(-) diff --git a/app/controllers/admin/impersonate_controller.rb b/app/controllers/admin/impersonate_controller.rb index eec39b4aa..8e3129942 100644 --- a/app/controllers/admin/impersonate_controller.rb +++ b/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? diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index c56ae55bb..185ba7e10 100644 --- a/app/controllers/topics_controller.rb +++ b/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 diff --git a/app/models/topic.rb b/app/models/topic.rb index ddf092e15..32fafdb34 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -381,27 +381,25 @@ 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) - # Notify the user they've been invited - user.notifications.create(notification_type: Notification.types[:invited_to_private_message], - topic_id: id, - post_number: 1, - data: { topic_title: title, - 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) + 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, + post_number: 1, + data: { topic_title: title, + display_username: invited_by.username }.to_json) + return true end - else - # Success is whether the invite was created - return invite_by_email(invited_by, username_or_email).present? end - false + 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 diff --git a/app/models/user.rb b/app/models/user.rb index b9f745f66..2be3b0306 100644 --- a/app/models/user.rb +++ b/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) diff --git a/lib/discourse.rb b/lib/discourse.rb index 533d6a465..3b89797b2 100644 --- a/lib/discourse.rb +++ b/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 diff --git a/spec/controllers/admin/impersonate_controller_spec.rb b/spec/controllers/admin/impersonate_controller_spec.rb index 45a6aecb3..601f19dcf 100644 --- a/spec/controllers/admin/impersonate_controller_spec.rb +++ b/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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 244e73c24..d7c4a0cbd 100644 --- a/spec/models/user_spec.rb +++ b/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