From 6ca53f946aac08cd4974d1d139a714eeec10a74c Mon Sep 17 00:00:00 2001
From: Priit Tark <priit@gitlab.eu>
Date: Thu, 6 Aug 2015 13:21:15 +0300
Subject: [PATCH] Validate contact and invoice emails when they change #2745

---
 Gemfile                     |  2 ++
 Gemfile.lock                |  3 +++
 app/models/contact.rb       |  1 +
 app/models/invoice.rb       |  2 +-
 app/models/registrar.rb     |  4 +++-
 spec/epp/contact_spec.rb    | 19 +++++++++++++++++++
 spec/models/contact_spec.rb | 14 ++++++++++++++
 7 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/Gemfile b/Gemfile
index b2fd0f844..4866b0396 100644
--- a/Gemfile
+++ b/Gemfile
@@ -18,6 +18,8 @@ gem 'figaro', '~> 1.1.1'
 # model related
 gem 'pg',                        '~> 0.18.0'
 gem 'ransack',                   '~> 1.5.1' # for searching
+gem 'validates_email_format_of', '~> 1.6.3' # validates email against RFC 2822 and RFC 3696
+
 # with polymorphic fix
 gem 'paper_trail',
   github: 'airblade/paper_trail',
diff --git a/Gemfile.lock b/Gemfile.lock
index ee1c0ad36..57e700c12 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -521,6 +521,8 @@ GEM
       parser (~> 2.2.2)
       procto (~> 0.0.2)
     uuidtools (2.1.5)
+    validates_email_format_of (1.6.3)
+      i18n
     virtus (1.0.5)
       axiom-types (~> 0.1)
       coercible (~> 1.0)
@@ -622,4 +624,5 @@ DEPENDENCIES
   uglifier (~> 2.7.1)
   unicorn
   uuidtools (~> 2.1.4)
+  validates_email_format_of (~> 1.6.3)
   whenever (~> 0.9.4)
diff --git a/app/models/contact.rb b/app/models/contact.rb
index c69797403..fc29c56eb 100644
--- a/app/models/contact.rb
+++ b/app/models/contact.rb
@@ -19,6 +19,7 @@ class Contact < ActiveRecord::Base
   # Phone nr validation is very minimam in order to support legacy requirements
   validates :phone, format: /\+[0-9]{1,3}\.[0-9]{1,14}?/
   validates :email, format: /@/
+  validates :email, email_format: { message: :invalid }, if: proc { |c| c.email_changed? }
   validates :ident,
     format: { with: /\d{4}-\d{2}-\d{2}/, message: :invalid_birthday_format },
     if: proc { |c| c.ident_type == 'birthday' }
diff --git a/app/models/invoice.rb b/app/models/invoice.rb
index 83145553b..a658b69df 100644
--- a/app/models/invoice.rb
+++ b/app/models/invoice.rb
@@ -12,7 +12,7 @@ class Invoice < ActiveRecord::Base
   }
 
   attr_accessor :billing_email
-  validates :billing_email, format: { with: /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\z/i }, allow_blank: true
+  validates :billing_email, email_format: { message: :invalid }, allow_blank: true
 
   validates :invoice_type, :due_date, :currency, :seller_name,
             :seller_iban, :buyer_name, :invoice_items, :vat_prc, presence: true
diff --git a/app/models/registrar.rb b/app/models/registrar.rb
index d5ba8bd9a..0110e5a16 100644
--- a/app/models/registrar.rb
+++ b/app/models/registrar.rb
@@ -45,7 +45,9 @@ class Registrar < ActiveRecord::Base
     end
   end
 
-  validates :email, :billing_email, format: /@/, allow_blank: true
+  validates :email, :billing_email, 
+    email_format: { message: :invalid }, 
+    allow_blank: true, if: proc { |c| c.email_changed? }
 
   WHOIS_TRIGGERS = %w(name email phone street city state zip)
 
diff --git a/spec/epp/contact_spec.rb b/spec/epp/contact_spec.rb
index b7b6bc976..754c5a4db 100644
--- a/spec/epp/contact_spec.rb
+++ b/spec/epp/contact_spec.rb
@@ -133,6 +133,13 @@ describe 'EPP Contact', epp: true do
         cr_date.text.in_time_zone.utc.should be_within(5).of(Time.zone.now)
       end
 
+      it 'should return email issue' do
+        response = create_request(email: { value: 'not@valid' })
+
+        response[:msg].should == 'Email is invalid [email]'
+        response[:result_code].should == '2005'
+      end
+
       it 'should add registrar prefix for code when missing' do
         response = create_request({ id: { value: 'abc12345' } })
         response[:msg].should == 'Command completed successfully'
@@ -397,6 +404,18 @@ describe 'EPP Contact', epp: true do
         response[:results][1][:result_code].should == '2005'
       end
 
+      it 'should return email issue' do
+        response = update_request({
+          id: { value: 'FIRST0:SH8013' },
+          chg: {
+            email: { value: 'legacy@wrong' }
+          }
+        })
+
+        response[:msg].should == 'Email is invalid [email]'
+        response[:result_code].should == '2005'
+      end
+
       it 'should not update code with custom string' do
         response = update_request(
           {
diff --git a/spec/models/contact_spec.rb b/spec/models/contact_spec.rb
index 6cc3ee124..b55626d26 100644
--- a/spec/models/contact_spec.rb
+++ b/spec/models/contact_spec.rb
@@ -109,6 +109,12 @@ describe Contact do
     it 'should have no related domain descriptions' do
       @contact.related_domain_descriptions.should == {}
     end
+
+    it 'should fully validate email syntax for new records' do
+      @contact.email = 'not@correct'
+      @contact.valid?
+      @contact.errors[:email].should == ['Email is invalid']
+    end
   end
 
   context 'with valid attributes' do
@@ -247,6 +253,14 @@ describe Contact do
         contact = @domain.contacts.first
         contact.related_domain_descriptions.should == { "#{@domain.name}" => [:admin] }
       end
+
+      it 'should fully validate email syntax for old records' do
+        old = @contact.email
+        @contact.email = 'legacy@support-not-correct'
+        @contact.valid?
+        @contact.errors[:email].should == ['Email is invalid']
+        @contact.email = old
+      end
     end
 
     context 'as birthday' do