From 13c6191e892543b640076e0ef0de9fc37e822e59 Mon Sep 17 00:00:00 2001
From: Guo Xiang Tan <tgx_world@hotmail.com>
Date: Wed, 21 Dec 2016 17:00:45 +0800
Subject: [PATCH] FIX: Don't allow invalid email to be saved.
---
.../controllers/preferences/email.js.es6 | 20 ++++++++++++++-
.../discourse/templates/preferences-email.hbs | 3 ++-
app/models/email_change_request.rb | 6 +++++
app/models/user.rb | 2 ++
app/models/username_validator.rb | 2 ++
lib/email_updater.rb | 6 ++---
lib/validators/email_validator.rb | 2 +-
.../validators/email_validator_spec.rb | 12 +++++++++
.../email_change_request_fabricator.rb | 6 +++++
spec/models/email_change_request_spec.rb | 14 +++++++++++
spec/models/user_spec.rb | 25 +++++++++++++++++--
spec/services/user_activator_spec.rb | 9 -------
.../acceptance/preferences-test.js.es6 | 8 +++++-
13 files changed, 97 insertions(+), 18 deletions(-)
create mode 100644 spec/fabricators/email_change_request_fabricator.rb
create mode 100644 spec/models/email_change_request_spec.rb
diff --git a/app/assets/javascripts/discourse/controllers/preferences/email.js.es6 b/app/assets/javascripts/discourse/controllers/preferences/email.js.es6
index 099e4a1e689..e125dd73f31 100644
--- a/app/assets/javascripts/discourse/controllers/preferences/email.js.es6
+++ b/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;
diff --git a/app/assets/javascripts/discourse/templates/preferences-email.hbs b/app/assets/javascripts/discourse/templates/preferences-email.hbs
index 0aecb67abd3..6c1beb2ad80 100644
--- a/app/assets/javascripts/discourse/templates/preferences-email.hbs
+++ b/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}}
diff --git a/app/models/email_change_request.rb b/app/models/email_change_request.rb
index 229b858d470..9778e84be3a 100644
--- a/app/models/email_change_request.rb
+++ b/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)
diff --git a/app/models/user.rb b/app/models/user.rb
index e363f5dd55a..f059c96e336 100644
--- a/app/models/user.rb
+++ b/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?
diff --git a/app/models/username_validator.rb b/app/models/username_validator.rb
index 0dbae4bae6e..eeb4df859de 100644
--- a/app/models/username_validator.rb
+++ b/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
diff --git a/lib/email_updater.rb b/lib/email_updater.rb
index ff6b13c9903..ab4c8246a2a 100644
--- a/lib/email_updater.rb
+++ b/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)
diff --git a/lib/validators/email_validator.rb b/lib/validators/email_validator.rb
index b686379571a..468c0050710 100644
--- a/lib/validators/email_validator.rb
+++ b/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
diff --git a/spec/components/validators/email_validator_spec.rb b/spec/components/validators/email_validator_spec.rb
index 05edfbbf47c..bb7d6cb6ec1 100644
--- a/spec/components/validators/email_validator_spec.rb
+++ b/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
diff --git a/spec/fabricators/email_change_request_fabricator.rb b/spec/fabricators/email_change_request_fabricator.rb
new file mode 100644
index 00000000000..ba5a96f6bee
--- /dev/null
+++ b/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
diff --git a/spec/models/email_change_request_spec.rb b/spec/models/email_change_request_spec.rb
new file mode 100644
index 00000000000..030dc13ec95
--- /dev/null
+++ b/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
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 614cf17697e..e989fcaf469 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -3,8 +3,29 @@ require_dependency 'user'
describe User do
- it { is_expected.to validate_presence_of :username }
- it { is_expected.to validate_presence_of :email }
+ 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
diff --git a/spec/services/user_activator_spec.rb b/spec/services/user_activator_spec.rb
index 3a7974983ee..e584189ffd8 100644
--- a/spec/services/user_activator_spec.rb
+++ b/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
diff --git a/test/javascripts/acceptance/preferences-test.js.es6 b/test/javascripts/acceptance/preferences-test.js.es6
index b6885400ea4..9c597df2e0a 100644
--- a/test/javascripts/acceptance/preferences-test.js.es6
+++ b/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');
});
});