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 @@
- {{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}}
{{#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: 'eviltrout@example.com') - activator = EmailActivator.new(user, nil, nil, nil) - msg = activator.activate - - expect(msg).to match(/eviltrout@example.com/) - expect(msg).not_to match(//) - 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'); }); });