Add comprehensive email validation for blocked users

app/assets/javascripts/discourse/controllers/create_account_controller.js

@@ -14,6 +14,7 @@ Discourse.CreateAccountController = Discourse.Controller.extend(Discourse.ModalF
   accountPasswordConfirm: 0,
   accountChallenge: 0,
   formSubmitted: false,
+  rejectedEmails: Em.A([]),
 
   submitDisabled: function() {
     if (this.get('formSubmitted')) return true;
@@ -64,6 +65,14 @@ Discourse.CreateAccountController = Discourse.Controller.extend(Discourse.ModalF
     }
 
     email = this.get("accountEmail");
+
+    if (this.get('rejectedEmails').contains(email)) {
+      return Discourse.InputValidation.create({
+        failed: true,
+        reason: I18n.t('user.email.invalid')
+      });
+    }
+
     if ((this.get('authOptions.email') === email) && this.get('authOptions.email_valid')) {
       return Discourse.InputValidation.create({
         ok: true,
@@ -84,7 +93,7 @@ Discourse.CreateAccountController = Discourse.Controller.extend(Discourse.ModalF
       failed: true,
       reason: I18n.t('user.email.invalid')
     });
-  }.property('accountEmail'),
+  }.property('accountEmail', 'rejectedEmails.@each'),
 
   usernameMatch: function() {
     if (this.usernameNeedsToBeValidatedWithEmail()) {
@@ -262,6 +271,9 @@ Discourse.CreateAccountController = Discourse.Controller.extend(Discourse.ModalF
         createAccountController.set('complete', true);
       } else {
         createAccountController.flash(result.message || I18n.t('create_account.failed'), 'error');
+        if (result.errors && result.errors.email && result.values) {
+          createAccountController.get('rejectedEmails').pushObject(result.values.email);
+        }
         createAccountController.set('formSubmitted', false);
       }
       if (result.active) {

app/controllers/users_controller.rb

@@ -194,7 +194,9 @@ def create
     else
       render json: {
         success: false,
-        message: I18n.t("login.errors", errors: user.errors.full_messages.join("\n"))
+        message: I18n.t("login.errors", errors: user.errors.full_messages.join("\n")),
+        errors: user.errors.to_hash,
+        values: user.attributes.slice("name", "username", "email")
       }
     end
   rescue ActiveRecord::StatementInvalid

app/models/blocked_email.rb

@@ -0,0 +1,25 @@
+class BlockedEmail < ActiveRecord::Base
+
+  before_validation :set_defaults
+
+  validates :email, presence: true, uniqueness: true
+
+  def self.actions
+    @actions ||= Enum.new(:block, :do_nothing)
+  end
+
+  def self.should_block?(email)
+    record = BlockedEmail.where(email: email).first
+    if record
+      record.match_count += 1
+      record.last_match_at = Time.zone.now
+      record.save
+    end
+    record && record.action_type == actions[:block]
+  end
+
+  def set_defaults
+    self.action_type ||= BlockedEmail.actions[:block]
+  end
+
+end

app/models/user.rb

@@ -42,10 +42,9 @@ class User < ActiveRecord::Base
   has_one :user_search_data
 
   validates_presence_of :username
-  validates_presence_of :email
-  validates_uniqueness_of :email
   validate :username_validator
-  validate :email_validator, if: :email_changed?
+  validates :email, presence: true, uniqueness: true
+  validates :email, email: true, if: :email_changed?
   validate :password_validator
 
   before_save :cook
@@ -565,24 +564,6 @@ def username_validator
     end
   end
 
-  def email_validator
-    if (setting = SiteSetting.email_domains_whitelist).present?
-      unless email_in_restriction_setting?(setting)
-        errors.add(:email, I18n.t(:'user.email.not_allowed'))
-      end
-    elsif (setting = SiteSetting.email_domains_blacklist).present?
-      if email_in_restriction_setting?(setting)
-        errors.add(:email, I18n.t(:'user.email.not_allowed'))
-      end
-    end
-  end
-
-  def email_in_restriction_setting?(setting)
-    domains = setting.gsub('.', '\.')
-    regexp = Regexp.new("@(#{domains})", true)
-    self.email =~ regexp
-  end
-
   def password_validator
     if (@raw_password && @raw_password.length < 6) || (@password_required && !@raw_password)
       errors.add(:password, "must be 6 letters or longer")

config/locales/server.en.yml

@@ -731,6 +731,7 @@ en:
       must_begin_with_alphanumeric: "must begin with a letter or number"
     email:
       not_allowed: "is not allowed from that email provider. Please use another email address."
+      blocked: "is not allowed."
 
   invite_mailer:
     subject_template: "[%{site_name}] %{invitee_name} invited you to join a discussion on %{site_name}"

db/migrate/20130724201552_create_blocked_emails.rb

@@ -0,0 +1,12 @@
+class CreateBlockedEmails < ActiveRecord::Migration
+  def change
+    create_table :blocked_emails do |t|
+      t.string :email, null: false
+      t.integer :action_type, null: false
+      t.integer :match_count, null: false, default: 0
+      t.datetime :last_match_at
+      t.timestamps
+    end
+    add_index :blocked_emails, :email, unique: true
+  end
+end

lib/validators/email_validator.rb

@@ -0,0 +1,24 @@
+class EmailValidator < ActiveModel::EachValidator
+
+  def validate_each(record, attribute, value)
+    if (setting = SiteSetting.email_domains_whitelist).present?
+      unless email_in_restriction_setting?(setting, value)
+        record.errors.add(attribute, I18n.t(:'user.email.not_allowed'))
+      end
+    elsif (setting = SiteSetting.email_domains_blacklist).present?
+      if email_in_restriction_setting?(setting, value)
+        record.errors.add(attribute, I18n.t(:'user.email.not_allowed'))
+      end
+    end
+    if record.errors[attribute].blank? and BlockedEmail.should_block?(value)
+      record.errors.add(attribute, I18n.t(:'user.email.blocked'))
+    end
+  end
+
+  def email_in_restriction_setting?(setting, value)
+    domains = setting.gsub('.', '\.')
+    regexp = Regexp.new("@(#{domains})", true)
+    value =~ regexp
+  end
+
+end

spec/components/validators/email_validator_spec.rb

@@ -0,0 +1,23 @@
+require 'spec_helper'
+
+describe EmailValidator do
+
+  let(:record) { Fabricate.build(:user, email: "[email protected]") }
+  let(:validator) { described_class.new({attributes: :email}) }
+  subject(:validate) { validator.validate_each(record,:email,record.email) }
+
+  context "blocked email" do
+    it "doesn't add an error when email doesn't match a blocked email" do
+      BlockedEmail.stubs(:should_block?).with(record.email).returns(false)
+      validate
+      record.errors[:email].should_not be_present
+    end
+
+    it "adds an error when email matches a blocked email" do
+      BlockedEmail.stubs(:should_block?).with(record.email).returns(true)
+      validate
+      record.errors[:email].should be_present
+    end
+  end
+
+end

spec/fabricators/blocked_email_fabricator.rb

@@ -0,0 +1,4 @@
+Fabricator(:blocked_email) do
+  email { sequence(:email) { |n| "bad#{n}@spammers.org" } }
+  action_type BlockedEmail.actions[:block]
+end

spec/models/blocked_email_spec.rb

@@ -0,0 +1,48 @@
+require 'spec_helper'
+
+describe BlockedEmail do
+
+  let(:email) { '[email protected]' }
+
+  describe "new record" do
+    it "sets a default action_type" do
+      BlockedEmail.create(email: email).action_type.should == BlockedEmail.actions[:block]
+    end
+
+    it "last_match_at is null" do
+      # If we manually load the table with some emails, we can see whether those emails
+      # have ever been blocked by looking at last_match_at.
+      BlockedEmail.create(email: email).last_match_at.should be_nil
+    end
+  end
+
+  describe "#should_block?" do
+    subject { BlockedEmail.should_block?(email) }
+
+    it "returns false if a record with the email doesn't exist" do
+      subject.should be_false
+    end
+
+    shared_examples "when a BlockedEmail record matches" do
+      it "updates statistics" do
+        Timecop.freeze(Time.zone.now) do
+          expect { subject }.to change { blocked_email.reload.match_count }.by(1)
+          blocked_email.last_match_at.should be_within_one_second_of(Time.zone.now)
+        end
+      end
+    end
+
+    context "action_type is :block" do
+      let!(:blocked_email) { Fabricate(:blocked_email, email: email, action_type: BlockedEmail.actions[:block]) }
+      it { should be_true }
+      include_examples "when a BlockedEmail record matches"
+    end
+
+    context "action_type is :do_nothing" do
+      let!(:blocked_email) { Fabricate(:blocked_email, email: email, action_type: BlockedEmail.actions[:do_nothing]) }
+      it { should be_false }
+      include_examples "when a BlockedEmail record matches"
+    end
+  end
+
+end