Merge pull request #4618 from tgxworld/fix_invalid_emails

FIX: Don't allow invalid email to be saved.

app/assets/javascripts/discourse/controllers/preferences/email.js.es6

@@ -1,4 +1,7 @@
 import { propertyEqual } from 'discourse/lib/computed';
+import InputValidation from 'discourse/models/input-validation';
+import { emailValid } from 'discourse/lib/utilities';
+import computed from 'ember-addons/ember-computed-decorators';
 
 export default Ember.Controller.extend({
   taken: false,
@@ -8,7 +11,7 @@ export default Ember.Controller.extend({
   newEmail: null,
 
   newEmailEmpty: Em.computed.empty('newEmail'),
-  saveDisabled: Em.computed.or('saving', 'newEmailEmpty', 'taken', 'unchanged'),
+  saveDisabled: Em.computed.or('saving', 'newEmailEmpty', 'taken', 'unchanged', 'invalidEmail'),
   unchanged: propertyEqual('newEmailLower', 'currentUser.email'),
 
   newEmailLower: function() {
@@ -20,6 +23,21 @@ export default Ember.Controller.extend({
     return I18n.t("user.change");
   }.property('saving'),
 
+  @computed('newEmail')
+  invalidEmail(newEmail) {
+    return !emailValid(newEmail);
+  },
+
+  @computed('invalidEmail')
+  emailValidation(invalidEmail) {
+    if (invalidEmail) {
+      return InputValidation.create({
+        failed: true,
+        reason: I18n.t('user.email.invalid')
+      });
+    }
+  },
+
   actions: {
     changeEmail: function() {
       var self = this;

app/assets/javascripts/discourse/templates/preferences-email.hbs

@@ -25,7 +25,8 @@
       <div class="control-group">
         <label class="control-label">{{i18n 'user.email.title'}}</label>
         <div class="controls">
-          {{text-field value=newEmail id="change_email" classNames="input-xxlarge" autofocus="autofocus"}}
+          {{text-field value=newEmail id="change-email" classNames="input-xxlarge" autofocus="autofocus"}}
+          {{input-tip validation=emailValidation}}
         </div>
         <div class='instructions'>
           {{#if taken}}

app/models/email_change_request.rb

@@ -1,6 +1,12 @@
+require_dependency 'email_validator'
+
 class EmailChangeRequest < ActiveRecord::Base
   belongs_to :old_email_token, class_name: 'EmailToken'
   belongs_to :new_email_token, class_name: 'EmailToken'
+  belongs_to :user
+
+  validates :old_email, presence: true
+  validates :new_email, presence: true, format: { with: EmailValidator.email_regex }
 
   def self.states
     @states ||= Enum.new(authorizing_old: 1, authorizing_new: 2, complete: 3)

app/models/user.rb

@@ -1,5 +1,6 @@
 require_dependency 'email'
 require_dependency 'email_token'
+require_dependency 'email_validator'
 require_dependency 'trust_level'
 require_dependency 'pbkdf2'
 require_dependency 'discourse'
@@ -76,6 +77,7 @@ class User < ActiveRecord::Base
   validates_presence_of :username
   validate :username_validator, if: :username_changed?
   validates :email, presence: true, uniqueness: true
+  validates :email, format: { with: EmailValidator.email_regex }, if: :email_changed?
   validates :email, email: true, if: :should_validate_email?
   validate :password_validator
   validates :name, user_full_name: true, if: :name_changed?

app/models/username_validator.rb

@@ -1,3 +1,5 @@
+require_dependency 'user'
+
 class UsernameValidator
   # Public: Perform the validation of a field in a given object
   # it adds the errors (if any) to the object that we're giving as parameter

lib/email_updater.rb

@@ -40,14 +40,14 @@ class EmailUpdater
 
       if authorize_both?
         args[:change_state] = EmailChangeRequest.states[:authorizing_old]
-        email_token = @user.email_tokens.create(email: args[:old_email])
+        email_token = @user.email_tokens.create!(email: args[:old_email])
         args[:old_email_token] = email_token
       else
         args[:change_state] = EmailChangeRequest.states[:authorizing_new]
-        email_token = @user.email_tokens.create(email: args[:new_email])
+        email_token = @user.email_tokens.create!(email: args[:new_email])
         args[:new_email_token] = email_token
       end
-      @user.email_change_requests.create(args)
+      @user.email_change_requests.create!(args)
 
       if args[:change_state] == EmailChangeRequest.states[:authorizing_new]
         send_email(:confirm_new_email, email_token)

lib/validators/email_validator.rb

@@ -26,7 +26,7 @@ class EmailValidator < ActiveModel::EachValidator
   end
 
   def self.email_regex
-    /^[a-zA-Z0-9!#\$%&'*+\/=?\^_`{|}~\-]+(?:\.[a-zA-Z0-9!#\$%&'\*+\/=?\^_`{|}~\-]+)*@(?:[a-zA-Z0-9](?:[a-zA-Z0-9\-]*[a-zA-Z0-9])?\.)+[a-zA-Z0-9](?:[a-zA-Z0-9\-]*[a-zA-Z0-9])?$/
+    /\A[a-zA-Z0-9!#\$%&'*+\/=?\^_`{|}~\-]+(?:\.[a-zA-Z0-9!#\$%&'\*+\/=?\^_`{|}~\-]+)*@(?:[a-zA-Z0-9](?:[a-zA-Z0-9\-]*[a-zA-Z0-9])?\.)+[a-zA-Z0-9](?:[a-zA-Z0-9\-]*[a-zA-Z0-9])?$\z/
   end
 
 end

spec/components/validators/email_validator_spec.rb

@@ -32,4 +32,16 @@ describe EmailValidator do
     end
   end
 
+  context '.email_regex' do
+    it 'should match valid emails' do
+      expect(!!('test@discourse.org' =~ EmailValidator.email_regex)).to eq(true)
+    end
+
+    it 'should not match invalid emails' do
+      ['testdiscourse.org', 'test@discourse.org; a@discourse.org', 'random'].each do |email|
+        expect(!!(email =~ EmailValidator.email_regex)).to eq(false)
+      end
+    end
+  end
+
 end

spec/fabricators/email_change_request_fabricator.rb

@@ -0,0 +1,6 @@
+Fabricator(:email_change_request) do
+  user
+  old_email { sequence(:old_email) { |i| "bruce#{i}@wayne.com" } }
+  new_email { sequence(:new_email) { |i| "super#{i}@man.com" } }
+  change_state EmailChangeRequest.states[:authorizing_old]
+end

spec/models/email_change_request_spec.rb

@@ -0,0 +1,14 @@
+require 'rails_helper'
+
+RSpec.describe EmailChangeRequest do
+  context 'validations' do
+    describe '#new_email' do
+      describe 'when email is invalid' do
+        it 'should not be valid' do
+          email_change_request = Fabricate.build(:email_change_request, new_email: 'testdiscourse.org')
+          expect(email_change_request).to_not be_valid
+        end
+      end
+    end
+  end
+end

spec/models/user_spec.rb

@@ -3,9 +3,30 @@ require_dependency 'user'
 
 describe User do
 
+  context 'validations' do
     it { is_expected.to validate_presence_of :username }
+
+    describe 'emails' do
+      let(:user) { Fabricate.build(:user) }
+
       it { is_expected.to validate_presence_of :email }
 
+      describe 'when record has a valid email' do
+        it "should be valid" do
+          user.email = 'test@gmail.com'
+          expect(user).to be_valid
+        end
+      end
+
+      describe 'when record has an invalid email' do
+        it 'should not be valid' do
+          user.email = 'test@gmailcom'
+          expect(user).to_not be_valid
+        end
+      end
+    end
+  end
+
   describe '#count_by_signup_date' do
     before(:each) do
       User.destroy_all

spec/services/user_activator_spec.rb

@@ -28,14 +28,5 @@ describe UserActivator do
       expect(user.email_tokens.last.created_at).to be_within_one_second_of(Time.zone.now)
     end
 
-    it "escapes the email in the message" do
-      user = Fabricate(:user, email: '<strike>eviltrout@example.com</strike>')
-      activator = EmailActivator.new(user, nil, nil, nil)
-      msg = activator.activate
-
-      expect(msg).to match(/eviltrout@example.com/)
-      expect(msg).not_to match(/<strike>/)
-    end
-
   end
 end

test/javascripts/acceptance/preferences-test.js.es6

@@ -36,6 +36,12 @@ test("about me", () => {
 test("email", () => {
   visit("/users/eviltrout/preferences/email");
   andThen(() => {
-    ok(exists("#change_email"), "it has the input element");
+    ok(exists("#change-email"), "it has the input element");
+  });
+
+  fillIn("#change-email", 'invalidemail');
+
+  andThen(() => {
+    equal(find('.tip.bad').text().trim(), I18n.t('user.email.invalid'), 'it should display invalid email tip');
   });
 });