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

This commit is contained in:
Guo Xiang Tan 2016-12-21 17:00:45 +08:00
parent 5d7f3223f0
commit 13c6191e89
13 changed files with 97 additions and 18 deletions

View File

@ -1,4 +1,7 @@
import { propertyEqual } from 'discourse/lib/computed'; 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({ export default Ember.Controller.extend({
taken: false, taken: false,
@ -8,7 +11,7 @@ export default Ember.Controller.extend({
newEmail: null, newEmail: null,
newEmailEmpty: Em.computed.empty('newEmail'), 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'), unchanged: propertyEqual('newEmailLower', 'currentUser.email'),
newEmailLower: function() { newEmailLower: function() {
@ -20,6 +23,21 @@ export default Ember.Controller.extend({
return I18n.t("user.change"); return I18n.t("user.change");
}.property('saving'), }.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: { actions: {
changeEmail: function() { changeEmail: function() {
var self = this; var self = this;

View File

@ -25,7 +25,8 @@
<div class="control-group"> <div class="control-group">
<label class="control-label">{{i18n 'user.email.title'}}</label> <label class="control-label">{{i18n 'user.email.title'}}</label>
<div class="controls"> <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>
<div class='instructions'> <div class='instructions'>
{{#if taken}} {{#if taken}}

View File

@ -1,6 +1,12 @@
require_dependency 'email_validator'
class EmailChangeRequest < ActiveRecord::Base class EmailChangeRequest < ActiveRecord::Base
belongs_to :old_email_token, class_name: 'EmailToken' belongs_to :old_email_token, class_name: 'EmailToken'
belongs_to :new_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 def self.states
@states ||= Enum.new(authorizing_old: 1, authorizing_new: 2, complete: 3) @states ||= Enum.new(authorizing_old: 1, authorizing_new: 2, complete: 3)

View File

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

View File

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

View File

@ -40,14 +40,14 @@ class EmailUpdater
if authorize_both? if authorize_both?
args[:change_state] = EmailChangeRequest.states[:authorizing_old] 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 args[:old_email_token] = email_token
else else
args[:change_state] = EmailChangeRequest.states[:authorizing_new] 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 args[:new_email_token] = email_token
end end
@user.email_change_requests.create(args) @user.email_change_requests.create!(args)
if args[:change_state] == EmailChangeRequest.states[:authorizing_new] if args[:change_state] == EmailChangeRequest.states[:authorizing_new]
send_email(:confirm_new_email, email_token) send_email(:confirm_new_email, email_token)

View File

@ -26,7 +26,7 @@ class EmailValidator < ActiveModel::EachValidator
end end
def self.email_regex 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
end end

View File

@ -32,4 +32,16 @@ describe EmailValidator do
end end
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 end

View File

@ -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

View File

@ -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

View File

@ -3,9 +3,30 @@ require_dependency 'user'
describe User do describe User do
context 'validations' do
it { is_expected.to validate_presence_of :username } it { is_expected.to validate_presence_of :username }
describe 'emails' do
let(:user) { Fabricate.build(:user) }
it { is_expected.to validate_presence_of :email } 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 describe '#count_by_signup_date' do
before(:each) do before(:each) do
User.destroy_all User.destroy_all

View File

@ -28,14 +28,5 @@ describe UserActivator do
expect(user.email_tokens.last.created_at).to be_within_one_second_of(Time.zone.now) expect(user.email_tokens.last.created_at).to be_within_one_second_of(Time.zone.now)
end 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
end end

View File

@ -36,6 +36,12 @@ test("about me", () => {
test("email", () => { test("email", () => {
visit("/users/eviltrout/preferences/email"); visit("/users/eviltrout/preferences/email");
andThen(() => { 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');
}); });
}); });