Add MX record validation to e-mail subscriptions, refactor validator (#38502)

app/models/email_subscription.rb

@@ -20,6 +20,7 @@ class EmailSubscription < ApplicationRecord
   normalizes :email, with: ->(str) { str.squish.downcase }
 
   validates :email, presence: true, email_address: true, uniqueness: { scope: :account_id }
+  validates :email, email_mx: true, if: -> { email_changed? && !Rails.env.local? }
 
   scope :confirmed, -> { where.not(confirmed_at: nil) }
   scope :unconfirmed, -> { where(confirmed_at: nil) }

app/models/user.rb

@@ -93,9 +93,9 @@ class User < ApplicationRecord
   validates :invite_request, presence: true, on: :create, if: :invite_text_required?
 
   validates :email, presence: true, email_address: true
+  validates :email, email_mx: { attempt_ip: :sign_up_ip }, if: :validate_email_dns?
 
   validates_with UserEmailValidator, if: -> { ENV['EMAIL_DOMAIN_LISTS_APPLY_AFTER_CONFIRMATION'] == 'true' || !confirmed? }
-  validates_with EmailMxValidator, if: :validate_email_dns?
   validates :agreement, acceptance: { allow_nil: false, accept: [true, 'true', '1'] }, on: :create
 
   # Honeypot/anti-spam fields

app/validators/email_mx_validator.rb

@@ -2,21 +2,21 @@
 
 require 'resolv'
 
-class EmailMxValidator < ActiveModel::Validator
-  def validate(user)
-    return if user.email.blank?
+class EmailMxValidator < ActiveModel::EachValidator
+  def validate_each(record, attribute, value)
+    return if value.blank?
 
-    domain = get_domain(user.email)
+    domain = get_domain(value)
 
     if domain.blank? || domain.include?('..')
-      user.errors.add(:email, :invalid)
+      record.errors.add(attribute, :invalid)
     elsif !on_allowlist?(domain)
       resolved_ips, resolved_domains = resolve_mx(domain)
 
       if resolved_ips.empty?
-        user.errors.add(:email, :unreachable)
-      elsif email_domain_blocked?(resolved_domains, user.sign_up_ip)
-        user.errors.add(:email, :blocked)
+        record.errors.add(attribute, :unreachable)
+      elsif email_domain_blocked?([domain, *resolved_domains], options[:attempt_ip].is_a?(Symbol) ? record.public_send(options[:attempt_ip]) : nil)
+        record.errors.add(attribute, :blocked)
       end
     end
   end

spec/validators/email_mx_validator_spec.rb

@@ -3,6 +3,21 @@
 require 'rails_helper'
 
 RSpec.describe EmailMxValidator do
+  subject { record_class.new }
+
+  context 'with no options' do
+    let(:record_class) do
+      Class.new do
+        include ActiveModel::Validations
+
+        def self.name = 'Record'
+
+        attr_accessor :email
+
+        validates :email, email_mx: true
+      end
+    end
+
     let(:user) { Fabricate.build :user, email: }
     let(:email) { 'foo@example.com' }
     let(:resolv_dns_double) { instance_double(Resolv::DNS) }
@@ -18,20 +33,14 @@ RSpec.describe EmailMxValidator do
       context 'when there are not DNS records' do
         before { configure_resolver('example.com') }
 
-      it 'does not add errors to record' do
-        subject.validate(user)
-        expect(user.errors).to be_empty
-      end
+        it { is_expected.to allow_value(email).for(:email) }
       end
     end
 
     context 'when there are DNS records for the domain' do
       before { configure_resolver('example.com', a: resolv_double_a('192.0.2.42')) }
 
-    it 'does not add errors to record' do
-      subject.validate(user)
-      expect(user.errors).to be_empty
-    end
+      it { is_expected.to allow_value(email).for(:email) }
     end
 
     context 'when the TagManager fails to normalize the domain' do
@@ -42,19 +51,13 @@ RSpec.describe EmailMxValidator do
 
       let(:tag_manage_double) { instance_double(TagManager) }
 
-    it 'adds errors to record' do
-      subject.validate(user)
-      expect(user.errors).to be_present
-    end
+      it { is_expected.to_not allow_value(email).for(:email) }
     end
 
     context 'when the email portion is blank' do
       let(:email) { 'foo@' }
 
-    it 'adds errors to record' do
-      subject.validate(user)
-      expect(user.errors).to be_present
-    end
+      it { is_expected.to_not allow_value(email).for(:email) }
     end
 
     context 'when the email domain contains empty labels' do
@@ -62,19 +65,13 @@ RSpec.describe EmailMxValidator do
 
       before { configure_resolver('example..com', a: resolv_double_a('192.0.2.42')) }
 
-    it 'adds errors to record' do
-      subject.validate(user)
-      expect(user.errors).to be_present
-    end
+      it { is_expected.to_not allow_value(email).for(:email) }
     end
 
     context 'when there are no DNS records for the email domain' do
       before { configure_resolver('example.com') }
 
-    it 'adds errors to record' do
-      subject.validate(user)
-      expect(user.errors).to be_present
-    end
+      it { is_expected.to_not allow_value(email).for(:email) }
     end
 
     context 'when MX record does not lead to an IP' do
@@ -83,10 +80,7 @@ RSpec.describe EmailMxValidator do
         configure_resolver('mail.example.com')
       end
 
-    it 'adds errors to record' do
-      subject.validate(user)
-      expect(user.errors).to be_present
-    end
+      it { is_expected.to_not allow_value(email).for(:email) }
     end
 
     context 'when the MX record has an email domain block' do
@@ -103,9 +97,7 @@ RSpec.describe EmailMxValidator do
         )
       end
 
-    it 'adds errors to record' do
-      subject.validate(user)
-      expect(user.errors).to be_present
+      it { is_expected.to_not allow_value(email).for(:email) }
     end
   end