From b24c1a1ad9fd9ebfe210864d54a018e042870bf7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Mon, 15 Apr 2013 02:20:33 +0200 Subject: [PATCH] better consistency around email case sensitivity --- .../views/modal/forgot_password_view.js | 2 +- app/controllers/session_controller.rb | 10 ++--- app/controllers/users_controller.rb | 7 ++-- app/models/invite.rb | 5 +-- app/models/topic.rb | 4 +- app/models/user.rb | 4 +- lib/email.rb | 7 +++- spec/components/email_spec.rb | 41 ++++++++++++++----- spec/controllers/session_controller_spec.rb | 8 ++-- spec/models/invite_spec.rb | 9 ++-- 10 files changed, 62 insertions(+), 35 deletions(-) diff --git a/app/assets/javascripts/discourse/views/modal/forgot_password_view.js b/app/assets/javascripts/discourse/views/modal/forgot_password_view.js index dfb59c4a6..976afacdb 100644 --- a/app/assets/javascripts/discourse/views/modal/forgot_password_view.js +++ b/app/assets/javascripts/discourse/views/modal/forgot_password_view.js @@ -18,7 +18,7 @@ Discourse.ForgotPasswordView = Discourse.ModalBodyView.extend({ submit: function() { Discourse.ajax(Discourse.getURL("/session/forgot_password"), { - data: { username: this.get('accountEmailOrUsername') }, + data: { login: this.get('accountEmailOrUsername') }, type: 'POST' }); diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index be6defc7c..833168da6 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -3,13 +3,13 @@ class SessionController < ApplicationController def create requires_parameter(:login, :password) - login = params[:login].downcase + login = params[:login] login = login[1..-1] if login[0] == "@" if login =~ /@/ - @user = User.where(email: login).first + @user = User.where(email: Email.downcase(login)).first else - @user = User.where(username_lower: login).first + @user = User.where(username_lower: login.downcase).first end if @user.present? @@ -37,9 +37,9 @@ class SessionController < ApplicationController end def forgot_password - requires_parameter(:username) + requires_parameter(:login) - user = User.where('username_lower = :username or email = :username', username: params[:username].downcase).first + user = User.where('username_lower = :username or email = :email', username: params[:login].downcase, email: Email.downcase(params[:login])).first if user.present? email_token = user.email_tokens.create(email: user.email) Jobs.enqueue(:user_email, type: :forgot_password, user_id: user.id, email_token: email_token.token) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 479839a7e..fa95e5339 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -265,16 +265,17 @@ class UsersController < ApplicationController requires_parameter(:email) user = fetch_user_from_params guardian.ensure_can_edit!(user) + lower_email = Email.downcase(params[:email]) # Raise an error if the email is already in use - if User.where("lower(email) = ?", params[:email].downcase).exists? + if User.where("email = ?", lower_email).exists? raise Discourse::InvalidParameters.new(:email) end - email_token = user.email_tokens.create(email: params[:email]) + email_token = user.email_tokens.create(email: lower_email) Jobs.enqueue( :user_email, - to_address: params[:email], + to_address: lower_email, type: :authorize_email, user_id: user.id, email_token: email_token.token diff --git a/app/models/invite.rb b/app/models/invite.rb index da940ae68..8427afccd 100644 --- a/app/models/invite.rb +++ b/app/models/invite.rb @@ -11,13 +11,12 @@ class Invite < ActiveRecord::Base acts_as_paranoid - before_create do self.invite_key ||= SecureRandom.hex end before_save do - self.email = email.downcase + self.email = Email.downcase(email) end validate :user_doesnt_already_exist @@ -26,7 +25,7 @@ class Invite < ActiveRecord::Base def user_doesnt_already_exist @email_already_exists = false return if email.blank? - if User.where("lower(email) = ?", email.downcase).exists? + if User.where("email = ?", Email.downcase(email)).exists? @email_already_exists = true errors.add(:email) end diff --git a/app/models/topic.rb b/app/models/topic.rb index 41a895331..fe926af36 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -398,11 +398,9 @@ class Topic < ActiveRecord::Base # 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. def invite_by_email(invited_by, email) - lower_email = email.downcase - + lower_email = Email.downcase(email) invite = Invite.with_deleted.where('invited_by_id = ? and email = ?', invited_by.id, lower_email).first - if invite.blank? invite = Invite.create(invited_by: invited_by, email: lower_email) unless invite.valid? diff --git a/app/models/user.rb b/app/models/user.rb index 600b6f280..fa4467897 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -193,7 +193,9 @@ class User < ActiveRecord::Base end def self.find_by_username_or_email(username_or_email) - where("username_lower = :user or lower(username) = :user or lower(email) = :user or lower(name) = :user", user: username_or_email.downcase) + 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) end # tricky, we need our bus to be subscribed from the right spot diff --git a/lib/email.rb b/lib/email.rb index 8d1c956e9..1f19cb33d 100644 --- a/lib/email.rb +++ b/lib/email.rb @@ -1,8 +1,8 @@ require 'mail' + module Email def self.is_valid?(email) - parser = Mail::RFC2822Parser.new parser.root = :addr_spec result = parser.parse(email) @@ -12,4 +12,9 @@ module Email result && result.respond_to?(:domain) && result.domain.dot_atom_text.elements.size > 1 end + def self.downcase(email) + return email unless Email.is_valid?(email) + email.gsub(/^(.+@{1})([^@]+)$/) { $1 + $2.downcase } + end + end diff --git a/spec/components/email_spec.rb b/spec/components/email_spec.rb index a9c6887e9..ae93e6b72 100644 --- a/spec/components/email_spec.rb +++ b/spec/components/email_spec.rb @@ -3,20 +3,39 @@ require 'email' describe Email do + describe "is_valid?" do + + it 'treats a good email as valid' do + Email.is_valid?('sam@sam.com').should be_true + end + + it 'treats a bad email as invalid' do + Email.is_valid?('sam@sam').should be_false + end + + it 'allows museum tld' do + Email.is_valid?('sam@nic.museum').should be_true + end + + it 'does not think a word is an email' do + Email.is_valid?('sam').should be_false + end - it 'should treat a good email as valid' do - Email.is_valid?('sam@sam.com').should be_true end - it 'should treat a bad email as invalid' do - Email.is_valid?('sam@sam').should be_false + describe "downcase" do + + it 'downcases only the host part' do + Email.downcase('SAM@GMAIL.COM').should == 'SAM@gmail.com' + Email.downcase('sam@GMAIL.COM').should_not == 'SAM@gmail.com' + end + + it 'leaves invalid emails untouched' do + Email.downcase('SAM@GMAILCOM').should == 'SAM@GMAILCOM' + Email.downcase('samGMAIL.COM').should == 'samGMAIL.COM' + Email.downcase('sam@GM@AIL.COM').should == 'sam@GM@AIL.COM' + end + end - it 'should allow museum tld' do - Email.is_valid?('sam@nic.museum').should be_true - end - - it 'should not think a word is an email' do - Email.is_valid?('sam').should be_false - end end diff --git a/spec/controllers/session_controller_spec.rb b/spec/controllers/session_controller_spec.rb index 44d54e38d..285f520e2 100644 --- a/spec/controllers/session_controller_spec.rb +++ b/spec/controllers/session_controller_spec.rb @@ -119,12 +119,12 @@ describe SessionController do context 'for a non existant username' do it "doesn't generate a new token for a made up username" do - lambda { xhr :post, :forgot_password, username: 'made_up'}.should_not change(EmailToken, :count) + lambda { xhr :post, :forgot_password, login: 'made_up'}.should_not change(EmailToken, :count) end it "doesn't enqueue an email" do Jobs.expects(:enqueue).with(:user_mail, anything).never - xhr :post, :forgot_password, username: 'made_up' + xhr :post, :forgot_password, login: 'made_up' end end @@ -132,12 +132,12 @@ describe SessionController do let(:user) { Fabricate(:user) } it "generates a new token for a made up username" do - lambda { xhr :post, :forgot_password, username: user.username}.should change(EmailToken, :count) + lambda { xhr :post, :forgot_password, login: user.username}.should change(EmailToken, :count) end it "enqueues an email" do Jobs.expects(:enqueue).with(:user_email, has_entries(type: :forgot_password, user_id: user.id)) - xhr :post, :forgot_password, username: user.username + xhr :post, :forgot_password, login: user.username end end diff --git a/spec/models/invite_spec.rb b/spec/models/invite_spec.rb index a3b0ddc30..7e8540fa1 100644 --- a/spec/models/invite_spec.rb +++ b/spec/models/invite_spec.rb @@ -36,7 +36,6 @@ describe Invite do end end - context 'to a topic' do let!(:topic) { Fabricate(:topic) } let(:inviter) { topic.user } @@ -97,8 +96,12 @@ describe Invite do topic.invite_by_email(inviter, 'iceking@adventuretime.ooo').should == @invite end - it 'matches case insensitively' do - topic.invite_by_email(inviter, 'ICEKING@adventuretime.ooo').should == @invite + it 'matches case insensitively for the domain part' do + topic.invite_by_email(inviter, 'iceking@ADVENTURETIME.ooo').should == @invite + end + + it 'matches case sensitively for the local part' do + topic.invite_by_email(inviter, 'ICEKING@adventuretime.ooo').should_not == @invite end end