From 062db10c527641b42b1d7609892f043e95152f82 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Wed, 3 Jun 2020 10:13:25 +0800 Subject: [PATCH] FIX: `EmailValidator` needs to validate format of email. --- app/models/invite.rb | 2 +- app/models/user_email.rb | 3 +-- config/locales/server.en.yml | 1 + lib/validators/email_validator.rb | 4 ++++ spec/models/invite_spec.rb | 11 ++++++++--- spec/models/user_email_spec.rb | 12 ++++++++---- spec/requests/users_email_controller_spec.rb | 2 +- 7 files changed, 24 insertions(+), 11 deletions(-) diff --git a/app/models/invite.rb b/app/models/invite.rb index 2618b37de9a..5fde9a45abe 100644 --- a/app/models/invite.rb +++ b/app/models/invite.rb @@ -18,7 +18,7 @@ class Invite < ActiveRecord::Base has_many :topic_invites has_many :topics, through: :topic_invites, source: :topic validates_presence_of :invited_by_id - validates :email, email: true, format: { with: EmailValidator.email_regex }, allow_blank: true + validates :email, email: true, allow_blank: true before_create do self.invite_key ||= SecureRandom.hex diff --git a/app/models/user_email.rb b/app/models/user_email.rb index 7ce64cb4a2b..a3e76f22fd7 100644 --- a/app/models/user_email.rb +++ b/app/models/user_email.rb @@ -11,8 +11,7 @@ class UserEmail < ActiveRecord::Base before_validation :strip_downcase_email validates :email, presence: true - validates :email, email: true, format: { with: EmailValidator.email_regex }, - if: :validate_email? + validates :email, email: true, if: :validate_email? validates :primary, uniqueness: { scope: [:user_id] }, if: [:user_id, :primary] validate :user_id_not_changed, if: :primary diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 7b564c402b8..da98bd5cc0f 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -2451,6 +2451,7 @@ en: must_not_contain_two_special_chars_in_seq: "must not contain a sequence of 2 or more special chars (.-_)" must_not_end_with_confusing_suffix: "must not end with a confusing suffix like .json or .png etc." email: + invalid: "is invalid." not_allowed: "is not allowed from that email provider. Please use another email address." blocked: "is not allowed." revoked: "Won't be sending emails to '%{email}' until %{date}." diff --git a/lib/validators/email_validator.rb b/lib/validators/email_validator.rb index 6436161b827..7090f2516b1 100644 --- a/lib/validators/email_validator.rb +++ b/lib/validators/email_validator.rb @@ -3,6 +3,10 @@ class EmailValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) + unless value =~ EmailValidator.email_regex + record.errors.add(attribute, I18n.t(:'user.email.invalid')) + end + unless EmailValidator.allowed?(value) record.errors.add(attribute, I18n.t(:'user.email.not_allowed')) end diff --git a/spec/models/invite_spec.rb b/spec/models/invite_spec.rb index fab7da26904..f66a8f84cd6 100644 --- a/spec/models/invite_spec.rb +++ b/spec/models/invite_spec.rb @@ -29,9 +29,9 @@ describe Invite do fab!(:coding_horror) { Fabricate(:coding_horror) } it "should not allow an invite with unformatted email address" do - expect { - Fabricate(:invite, email: "John Doe ") - }.to raise_error(ActiveRecord::RecordInvalid) + invite = Fabricate.build(:invite, email: "John Doe ") + expect(invite.valid?).to eq(false) + expect(invite.errors.details[:email].first[:error]).to eq(I18n.t("user.email.invalid")) end it "should not allow an invite with blacklisted email" do @@ -44,6 +44,11 @@ describe Invite do expect(invite).to be_valid end + it "should not allow an invalid email address" do + invite = Fabricate.build(:invite, email: 'asjdso') + expect(invite.valid?).to eq(false) + expect(invite.errors.details[:email].first[:error]).to eq(I18n.t("user.email.invalid")) + end end context '#create' do diff --git a/spec/models/user_email_spec.rb b/spec/models/user_email_spec.rb index ee48170269c..ebd08de245b 100644 --- a/spec/models/user_email_spec.rb +++ b/spec/models/user_email_spec.rb @@ -3,32 +3,36 @@ require 'rails_helper' describe UserEmail do + fab!(:user) { Fabricate(:user) } + context "validation" do it "allows only one primary email" do - user = Fabricate(:user) expect { Fabricate(:secondary_email, user: user, primary: true) }.to raise_error(ActiveRecord::RecordInvalid) end it "allows multiple secondary emails" do - user = Fabricate(:user) Fabricate(:secondary_email, user: user, primary: false) Fabricate(:secondary_email, user: user, primary: false) expect(user.user_emails.count).to eq 3 end + + it "does not allow an invalid email" do + user_email = Fabricate.build(:user_email, user: user, email: "asjdaiosd") + expect(user_email.valid?).to eq(false) + expect(user_email.errors.details[:email].first[:error]).to eq(I18n.t("user.email.invalid")) + end end context "indexes" do it "allows only one primary email" do - user = Fabricate(:user) expect { Fabricate.build(:secondary_email, user: user, primary: true).save(validate: false) }.to raise_error(ActiveRecord::RecordNotUnique) end it "allows multiple secondary emails" do - user = Fabricate(:user) Fabricate.build(:secondary_email, user: user, primary: false).save(validate: false) Fabricate.build(:secondary_email, user: user, primary: false).save(validate: false) expect(user.user_emails.count).to eq 3 diff --git a/spec/requests/users_email_controller_spec.rb b/spec/requests/users_email_controller_spec.rb index f7c350199d5..257a431caf4 100644 --- a/spec/requests/users_email_controller_spec.rb +++ b/spec/requests/users_email_controller_spec.rb @@ -282,7 +282,7 @@ describe UsersEmailController do it 'raises an error without an invalid email' do put "/u/#{user.username}/preferences/email.json", params: { email: "sam@not-email.com'" } expect(response.status).to eq(422) - expect(response.body).to include("email is invalid") + expect(response.body).to include("Email is invalid") end it "raises an error if you can't edit the user's email" do