Merge branch '215919-email-validation' into 'master'

Change validation rules for profile email addresses

Closes #215919

See merge request gitlab-org/gitlab!30633

Gemfile

@@ -495,3 +495,6 @@ gem 'mail', '= 2.7.1'
 
 # File encryption
 gem 'lockbox', '~> 0.3.3'
+
+# Email validation
+gem 'valid_email', '~> 0.1'

Gemfile.lock

@@ -842,7 +842,7 @@ GEM
       rake (>= 0.8.7)
       thor (>= 0.20.3, < 2.0)
     rainbow (3.0.0)
-    raindrops (0.19.0)
+    raindrops (0.19.1)
     rake (12.3.3)
     rb-fsevent (0.10.2)
     rb-inotify (0.9.10)
@@ -1109,6 +1109,9 @@ GEM
       equalizer (~> 0.0.9)
       parser (>= 2.6.5)
       procto (~> 0.0.2)
+    valid_email (0.1.3)
+      activemodel
+      mail (>= 2.6.1)
     validate_email (0.1.6)
       activemodel (>= 3.0)
       mail (>= 2.2.5)
@@ -1402,6 +1405,7 @@ DEPENDENCIES
   unicorn (~> 5.5)
   unicorn-worker-killer (~> 0.4.4)
   unleash (~> 0.1.5)
+  valid_email (~> 0.1)
   validates_hostname (~> 1.0.6)
   version_sorter (~> 2.2.4)
   vmstat (~> 2.3.0)

app/models/email.rb

@@ -6,7 +6,8 @@ class Email < ApplicationRecord
 
   belongs_to :user, optional: false
 
-  validates :email, presence: true, uniqueness: true, devise_email: true
+  validates :email, presence: true, uniqueness: true
+  validate :validate_email_format
   validate :unique_email, if: ->(email) { email.email_changed? }
 
   scope :confirmed, -> { where.not(confirmed_at: nil) }
@@ -30,6 +31,10 @@ class Email < ApplicationRecord
     user.accept_pending_invitations!
   end
 
+  def validate_email_format
+    self.errors.add(:email, I18n.t(:invalid, scope: 'valid_email.validations.email')) unless ValidateEmail.valid?(self.email)
+  end
+
   # once email is confirmed, update the gpg signatures
   def update_invalid_gpg_signatures
     user.update_invalid_gpg_signatures if confirmed?

changelogs/unreleased/215919-email-validation.yml

+---
+title: Change validation rules for profile email addresses
+merge_request: 30633
+author:
+type: fixed

spec/controllers/profiles/emails_controller_spec.rb

@@ -10,12 +10,20 @@ describe Profiles::EmailsController do
   end
 
   describe '#create' do
-    let(:email_params) { { email: "add_email@example.com" } }
+    context 'when email address is valid' do
+      let(:email_params) { { email: "add_email@example.com" } }
 
-    it 'sends an email confirmation' do
-      expect { post(:create, params: { email: email_params }) }.to change { ActionMailer::Base.deliveries.size }
-      expect(ActionMailer::Base.deliveries.last.to).to eq [email_params[:email]]
-      expect(ActionMailer::Base.deliveries.last.subject).to match "Confirmation instructions"
+      it 'sends an email confirmation' do
+        expect { post(:create, params: { email: email_params }) }.to change { ActionMailer::Base.deliveries.size }
+      end
+    end
+
+    context 'when email address is invalid' do
+      let(:email_params) { { email: "test.@example.com" } }
+
+      it 'does not send an email confirmation' do
+        expect { post(:create, params: { email: email_params }) }.not_to change { ActionMailer::Base.deliveries.size }
+      end
     end
   end
 

spec/features/profiles/emails_spec.rb

@@ -31,6 +31,15 @@ describe 'Profile > Emails' do
       expect(email).to be_nil
       expect(page).to have_content('Email has already been taken')
     end
+
+    it 'does not add an invalid email' do
+      fill_in('Email', with: 'test.@example.com')
+      click_button('Add email address')
+
+      email = user.emails.find_by(email: email)
+      expect(email).to be_nil
+      expect(page).to have_content('Email is invalid')
+    end
   end
 
   it 'User removes email' do

spec/models/email_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 describe Email do
   describe 'validations' do
-    it_behaves_like 'an object with email-formated attributes', :email do
+    it_behaves_like 'an object with RFC3696 compliant email-formated attributes', :email do
       subject { build(:email) }
     end
   end

spec/models/user_spec.rb

@@ -296,7 +296,7 @@ describe User, :do_not_mock_admin_mode do
       subject { build(:user) }
     end
 
-    it_behaves_like 'an object with email-formated attributes', :public_email, :notification_email do
+    it_behaves_like 'an object with RFC3696 compliant email-formated attributes', :public_email, :notification_email do
       subject { build(:user).tap { |user| user.emails << build(:email, email: email_value) } }
     end
 
@@ -916,7 +916,6 @@ describe User, :do_not_mock_admin_mode do
             user.tap { |u| u.update!(email: new_email) }.reload
           end.to change(user, :unconfirmed_email).to(new_email)
         end
-
         it 'does not change :notification_email' do
           expect do
             user.tap { |u| u.update!(email: new_email) }.reload

spec/support/shared_examples/models/email_format_shared_examples.rb

@@ -44,3 +44,44 @@ RSpec.shared_examples 'an object with email-formated attributes' do |*attributes
     end
   end
 end
+
+RSpec.shared_examples 'an object with RFC3696 compliant email-formated attributes' do |*attributes|
+  attributes.each do |attribute|
+    describe "specifically its :#{attribute} attribute" do
+      %w[
+        info@example.com
+        info+test@example.com
+        o'reilly@example.com
+      ].each do |valid_email|
+        context "with a value of '#{valid_email}'" do
+          let(:email_value) { valid_email }
+
+          it 'is valid' do
+            subject.send("#{attribute}=", valid_email)
+
+            expect(subject).to be_valid
+          end
+        end
+      end
+
+      %w[
+        foobar
+        test@test@example.com
+        test.test.@example.com
+        .test.test@example.com
+        mailto:test@example.com
+        lol!'+=?><#$%^&*()@gmail.com
+      ].each do |invalid_email|
+        context "with a value of '#{invalid_email}'" do
+          let(:email_value) { invalid_email }
+
+          it 'is invalid' do
+            subject.send("#{attribute}=", invalid_email)
+
+            expect(subject).to be_invalid
+          end
+        end
+      end
+    end
+  end
+end