Merge pull request #9978 from mfo/US/enforce-real-email-validation

feat(email): stricter validation

app/components/simple_format_component.rb

@@ -22,7 +22,7 @@ class SimpleFormatComponent < ApplicationComponent
   }
 
   SIMPLE_URL_REGEX = %r{https?://[^\s<>]+}
-  EMAIL_IN_TEXT_REGEX = Regexp.new(Devise.email_regexp.source.gsub(/\\A|\\z/, '\b'))
+  EMAIL_IN_TEXT_REGEX = Regexp.new(StrictEmailValidator::REGEXP.source.gsub(/\\A|\\z/, '\b'))
 
   def initialize(text, allow_a: true, allow_autolink: true, class_names_map: {})
     @allow_a = allow_a

app/controllers/users/sessions_controller.rb

@@ -29,7 +29,7 @@ class Users::SessionsController < Devise::SessionsController
   end
 
   def link_sent
-    if Devise.email_regexp.match?(params[:email])
+    if StrictEmailValidator::REGEXP.match?(params[:email])
       @email = params[:email]
     else
       redirect_to root_path

app/models/avis.rb

@@ -21,7 +21,7 @@ class Avis < ApplicationRecord
     content_type: AUTHORIZED_CONTENT_TYPES,
     size: { less_than: FILE_MAX_SIZE }
 
-  validates :email, format: { with: Devise.email_regexp, message: "n'est pas valide" }, allow_nil: true
+  validates :email, strict_email: true, allow_nil: true
   validates :question_answer, inclusion: { in: [true, false] }, on: :update, if: -> { question_label.present? }
   validates :piece_justificative_file, size: { less_than: FILE_MAX_SIZE }
   validates :introduction_file, size: { less_than: FILE_MAX_SIZE }

app/models/champs/email_champ.rb

@@ -1,2 +1,5 @@
 class Champs::EmailChamp < Champs::TextChamp
+  include EmailSanitizableConcern
+  before_validation -> { sanitize_email(:value) }
+  validates :value, format: { with: StrictEmailValidator::REGEXP }, if: :validate_champ_value?
 end

app/models/concerns/user_find_by_concern.rb

@@ -11,7 +11,7 @@ module UserFindByConcern
     end
 
     def self.find_all_by_identifier_with_emails(ids: [], emails: [])
-      valid_emails, invalid_emails = emails.partition { Devise.email_regexp.match?(_1) }
+      valid_emails, invalid_emails = emails.partition { StrictEmailValidator::REGEXP.match?(_1) }
 
       [
         where(id: ids).or(where(users: { email: valid_emails })).distinct(:id),

app/models/contact_information.rb

@@ -1,13 +1,16 @@
 class ContactInformation < ApplicationRecord
+  include EmailSanitizableConcern
+
   belongs_to :groupe_instructeur
 
   validates :nom, presence: { message: 'doit être renseigné' }, allow_nil: false
   validates :nom, uniqueness: { scope: :groupe_instructeur, message: 'existe déjà' }
-  validates :email, format: { with: Devise.email_regexp, message: "n'est pas valide" }, presence: { message: 'doit être renseigné' }, allow_nil: false
+  validates :email, strict_email: true, presence: { message: 'doit être renseigné' }, allow_nil: false
   validates :telephone, phone: { possible: true, allow_blank: false }
   validates :horaires, presence: { message: 'doivent être renseignés' }, allow_nil: false
   validates :adresse, presence: { message: 'doit être renseignée' }, allow_nil: false
   validates :groupe_instructeur, presence: { message: 'doit être renseigné' }, allow_nil: false
+  before_validation -> { sanitize_email(:email) }
 
   def pretty_nom
     nom

app/models/dossier_transfer.rb

@@ -4,7 +4,7 @@ class DossierTransfer < ApplicationRecord
 
   EXPIRATION_LIMIT = 2.weeks
 
-  validates :email, format: { with: Devise.email_regexp }
+  validates :email, strict_email: true, presence: true
   before_validation -> { sanitize_email(:email) }
 
   scope :pending, -> { where('created_at > ?', (Time.zone.now - EXPIRATION_LIMIT)) }

app/models/invite.rb

@@ -11,8 +11,7 @@ class Invite < ApplicationRecord
 
   validates :email, presence: true
   validates :email, uniqueness: { scope: :dossier_id }
-
-  validates :email, format: { with: Devise.email_regexp, message: "n'est pas valide" }, allow_nil: true
+  validates :email, strict_email: true, allow_nil: true
 
   scope :with_dossiers, -> { joins(:dossier).merge(Dossier.visible_by_user) }
 

app/models/user.rb

@@ -33,10 +33,14 @@ class User < ApplicationRecord
   accepts_nested_attributes_for :france_connect_information
 
   default_scope { eager_load(:instructeur, :administrateur, :expert) }
-  before_validation -> { sanitize_email(:email) }
 
+  before_validation -> { sanitize_email(:email) }
   validate :does_not_merge_on_self, if: :requested_merge_into_id_changed?
 
+  before_validation :remove_devise_email_format_validator
+  # plug our custom validation a la devise (same options) https://github.com/heartcombo/devise/blob/main/lib/devise/models/validatable.rb#L30
+  validates :email, strict_email: true, allow_blank: true, if: :devise_will_save_change_to_email?
+
   def validate_password_complexity?
     administrateur?
   end
@@ -268,4 +272,16 @@ class User < ApplicationRecord
   def link_invites!
     Invite.where(email: email).update_all(user_id: id)
   end
+
+  # we just want to remove the devise format validator
+  #   https://github.com/heartcombo/devise/blob/main/lib/devise/models/validatable.rb#L30
+  def remove_devise_email_format_validator
+    _validators[:email]&.reject! { _1.is_a?(ActiveModel::Validations::FormatValidator) }
+    _validate_callbacks.each do |callback|
+      next if !callback.filter.is_a?(ActiveModel::Validations::FormatValidator)
+      next if !callback.filter.attributes.include? :email
+
+      callback.filter.attributes.delete(:email)
+    end
+  end
 end

app/validators/strict_email_validator.rb

@@ -0,0 +1,32 @@
+class StrictEmailValidator < ActiveModel::EachValidator
+  # default devise email is : /\A[^@\s]+@[^@\s]+\z/
+  #   saying that it's quite permissive
+  #   but we want more, we want to ensure it's a domain with extension
+  #   so we append \.[A-Za-z]{2,}
+  REGEXP = /\A[^@\s]+@[^@\s\.]+\.[^@\s]{2,}\z/
+  DATE_SINCE_STRICT_EMAIL_VALIDATION = Date.parse(ENV.fetch('STRICT_EMAIL_VALIDATION_STARTS_ON')) rescue 0
+
+  def validate_each(record, attribute, value)
+    if value.present? && !regexp_for(record).match?(value)
+      record.errors.add(attribute, :invalid_email_format)
+    end
+  end
+
+  def regexp_for(record)
+    if StrictEmailValidator.eligible_to_new_validation?(record)
+      REGEXP
+    else
+      Devise.email_regexp
+    end
+  end
+
+  def self.eligible_to_new_validation?(record)
+    return false if !strict_validation_enabled?
+    return false if (record.created_at || Time.zone.now) < DATE_SINCE_STRICT_EMAIL_VALIDATION
+    true
+  end
+
+  def self.strict_validation_enabled?
+    ENV.key?('STRICT_EMAIL_VALIDATION_STARTS_ON')
+  end
+end

config/locales/en.yml

@@ -605,6 +605,7 @@ en:
         not_a_phone: 'Invalid phone number'
         not_a_rna: 'Invalid RNA number'
         url: 'is not a valid link'
+        invalid_email_format: "is invalid. Please fill in a valid email ex: john.doe@exemple.fr"
       models:
         attestation_template:
           attributes:

config/locales/fr.yml

@@ -608,6 +608,7 @@ fr:
         not_a_phone: 'Numéro de téléphone invalide'
         not_a_rna: 'Numéro RNA invalide'
         url: 'n’est pas un lien valide'
+        invalid_email_format: "est invalide. Saisir une adresse électronique valide, exemple : john.doe@exemple.fr"
       models:
         attestation_template:
           attributes:

spec/controllers/administrateurs/groupe_instructeurs_controller_spec.rb

@@ -346,8 +346,9 @@ describe Administrateurs::GroupeInstructeursController, type: :controller do
     end
 
     context 'when the admin wants to assign an instructor who is already assigned on this procedure' do
-      let(:emails) { ['instructeur_1@ministere_a.gouv.fr'].to_json }
-      it { expect(subject.request.flash[:alert]).to be_present }
+      let(:instructeur) { create(:instructeur) }
+      before { procedure_non_routee.groupe_instructeurs.first.add_instructeurs(emails: [instructeur.user.email]) }
+      let(:emails) { [instructeur.email].to_json }
       it { expect(subject).to redirect_to admin_procedure_groupe_instructeurs_path(procedure_non_routee) }
     end
 

spec/controllers/experts/avis_controller_spec.rb

@@ -414,7 +414,7 @@ describe Experts::AvisController, type: :controller do
 
         it do
           expect(response).to render_template :instruction
-          expect(flash.alert).to eq(["toto.fr : Le champ « Email » n'est pas valide"])
+          expect(flash.alert).to eq(["toto.fr : Le champ « Email » est invalide. Saisir une adresse électronique valide, exemple : john.doe@exemple.fr"])
           expect(Avis.last).to eq(previous_avis)
           expect(dossier.last_avis_updated_at).to eq(nil)
         end
@@ -445,7 +445,7 @@ describe Experts::AvisController, type: :controller do
 
         it do
           expect(response).to render_template :instruction
-          expect(flash.alert).to eq(["toto.fr : Le champ « Email » n'est pas valide"])
+          expect(flash.alert).to eq(["toto.fr : Le champ « Email » est invalide. Saisir une adresse électronique valide, exemple : john.doe@exemple.fr"])
           expect(flash.notice).to eq("Une demande d’avis a été envoyée à titi@titimail.com")
           expect(Avis.count).to eq(old_avis_count + 1)
         end

spec/controllers/instructeurs/dossiers_controller_spec.rb

@@ -798,7 +798,7 @@ describe Instructeurs::DossiersController, type: :controller do
         before { subject }
 
         it { expect(response).to render_template :avis }
-        it { expect(flash.alert).to eq(["emaila.com : Le champ « Email » n'est pas valide"]) }
+        it { expect(flash.alert).to eq(["emaila.com : Le champ « Email » est invalide. Saisir une adresse électronique valide, exemple : john.doe@exemple.fr"]) }
         it { expect { subject }.not_to change(Avis, :count) }
         it { expect(dossier.last_avis_updated_at).to eq(nil) }
       end
@@ -820,7 +820,7 @@ describe Instructeurs::DossiersController, type: :controller do
         before { subject }
 
         it { expect(response).to render_template :avis }
-        it { expect(flash.alert).to eq(["toto.fr : Le champ « Email » n'est pas valide"]) }
+        it { expect(flash.alert).to eq(["toto.fr : Le champ « Email » est invalide. Saisir une adresse électronique valide, exemple : john.doe@exemple.fr"]) }
         it { expect(flash.notice).to eq("Une demande d’avis a été envoyée à titi@titimail.com") }
         it { expect(Avis.count).to eq(old_avis_count + 1) }
         it { expect(saved_avis.expert.email).to eq("titi@titimail.com") }

spec/controllers/manager/dossiers_controller_spec.rb

@@ -54,7 +54,7 @@ describe Manager::DossiersController, type: :controller do
 
       it do
         expect { subject }.not_to have_enqueued_mail
-        expect(flash[:alert]).to eq("L’adresse email est invalide")
+        expect(flash[:alert]).to eq("L’adresse email est invalide. Saisir une adresse électronique valide, exemple : john.doe@exemple.fr")
       end
     end
   end

spec/controllers/users/profil_controller_spec.rb

@@ -134,7 +134,7 @@ describe Users::ProfilController, type: :controller do
 
       it "should not transfer to an empty email" do
         expect { subject }.not_to change { DossierTransfer.count }
-        expect(flash.alert).to eq(["L’adresse email est invalide"])
+        expect(flash.alert).to eq(["L’adresse email doit être rempli"])
       end
     end
   end

spec/controllers/users/transfers_controller_spec.rb

@@ -84,18 +84,22 @@ describe Users::TransfersController, type: :controller do
 
     shared_examples 'email error' do
       it { expect { subject }.not_to change { DossierTransfer.count } }
-      it { expect(flash.alert).to match([/invalide/]) }
+      it { expect(flash.alert).to include(expected_error) }
       it { is_expected.to redirect_to transferer_dossier_path(dossier.id) }
     end
 
     context "when email is empty" do
       let(:email) { "" }
-      it_behaves_like 'email error'
+      it_behaves_like 'email error' do
+        let(:expected_error) { 'L’adresse email doit être rempli' }
+      end
     end
 
     context "when email is invalid" do
       let(:email) { "not-an-email" }
-      it_behaves_like 'email error'
+      it_behaves_like 'email error' do
+        let(:expected_error) { "L’adresse email est invalide. Saisir une adresse électronique valide, exemple : john.doe@exemple.fr" }
+      end
     end
   end
 end

spec/models/avis_spec.rb

@@ -39,7 +39,7 @@ RSpec.describe Avis, type: :model do
       before do
         avis.reload
       end
-
+      it { expect(avis.valid?).to be_truthy }
       it { expect(avis.email).to be_nil }
       it { expect(avis.experts_procedure).to eq(experts_procedure) }
     end
@@ -75,6 +75,22 @@ RSpec.describe Avis, type: :model do
     end
   end
 
+  describe 'email validation' do
+    let(:now_invalid_email) { "toto@tps" }
+    context 'new avis' do
+      before { allow(StrictEmailValidator).to receive(:eligible_to_new_validation?).and_return(true) }
+
+      it { expect(build(:avis, email: now_invalid_email).valid?).to be_falsey }
+      it { expect(build(:avis, email: nil).valid?).to be_truthy }
+    end
+    context 'old avis' do
+      before { allow(StrictEmailValidator).to receive(:eligible_to_new_validation?).and_return(false) }
+
+      it { expect(build(:avis, email: now_invalid_email).valid?).to be_truthy }
+      it { expect(build(:avis, email: nil).valid?).to be_truthy }
+    end
+  end
+
   describe ".revoke_by!" do
     let(:claimant) { create(:instructeur) }
 

spec/models/champs/email_champ_spec.rb

@@ -0,0 +1,45 @@
+describe Champs::EmailChamp do
+  describe 'validation' do
+    let(:now) { Time.zone.now }
+    let(:before) { now + 1.day }
+    let(:after) { now + 1.day }
+    let(:champ) { build(:champ_email, value: value) }
+
+    subject { champ.valid?(:validate_champ_value) }
+
+    context 'when value is username' do
+      let(:value) { 'username' }
+      # what we allowed but it was a mistake
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when value does not contain extension' do
+      let(:value) { 'username@mailserver' }
+      # what we allowed but it was a mistake
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when value include an alias' do
+      let(:value) { 'username+alias@mailserver.fr' }
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when value includes accents' do
+      let(:value) { 'tech@démarches.gouv.fr' }
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when value is the classic standard user@domain.ext' do
+      let(:value) { 'username@mailserver.domain' }
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when value contains white spaces plus a standard email' do
+      let(:value) { "\r\n\t username@mailserver.domain\r\n\t " }
+      it { is_expected.to be_truthy }
+      it 'normalize value' do
+        expect { subject }.to change { champ.value }.from(value).to('username@mailserver.domain')
+      end
+    end
+  end
+end

spec/models/invite_spec.rb

@@ -31,7 +31,7 @@ describe Invite do
 
         it do
           expect(invite.save).to be false
-          expect(invite.errors.full_messages).to eq(["Le champ « Email » n'est pas valide"])
+          expect(invite.errors.full_messages).to eq(["Le champ « Email » est invalide. Saisir une adresse électronique valide, exemple : john.doe@exemple.fr"])
         end
 
         context 'when an email is empty' do

spec/models/user_spec.rb

@@ -493,4 +493,86 @@ describe User, type: :model do
       end
     end
   end
+
+  describe 'discard default devise validation when needed' do
+    let(:now) { Time.zone.now }
+    let(:before) { now - 1.day }
+    let(:after) { now + 1.day }
+    subject { user.valid? }
+
+    shared_examples_for "validation of users.email was flacky" do
+      context 'when value is username' do
+        let(:email) { 'username' }
+        it { is_expected.to be_falsey }
+      end
+
+      context 'when value does not contain extension' do
+        let(:email) { 'username@mailserver' }
+        # what we allowed but was a mistake
+        it { is_expected.to be_truthy }
+      end
+
+      context 'when value include an alias' do
+        let(:email) { 'username+alias@mailserver.fr' }
+        it { is_expected.to be_truthy }
+      end
+
+      context 'when value includes accents' do
+        let(:email) { 'tech@démarches.gouv.fr' }
+        it { is_expected.to be_falsey }
+      end
+
+      context 'when value is the classic standard user@domain.ext' do
+        let(:email) { 'username@mailserver.domain' }
+        it { is_expected.to be_truthy }
+      end
+    end
+
+    context 'when env var is not present' do
+      let(:user) { build(:user, email: email) }
+      before { allow(StrictEmailValidator).to receive(:strict_validation_enabled?).and_return(false).at_least(1) }
+      it_behaves_like "validation of users.email was flacky"
+    end
+
+    context "record.created_at < ENV['STRICT_EMAIL_VALIDATION_STARTS_ON']" do
+      let(:user) { build(:user, email: email, created_at: before) }
+      before do
+        allow(StrictEmailValidator).to receive(:strict_validation_enabled?).and_return(true).at_least(1)
+        stub_const("StrictEmailValidator::DATE_SINCE_STRICT_EMAIL_VALIDATION", now)
+      end
+      it_behaves_like "validation of users.email was flacky"
+    end
+
+    context "record.created_at > ENV['STRICT_EMAIL_VALIDATION_STARTS_ON']" do
+      let(:user) { build(:user, email: email, created_at: after) }
+      before do
+        allow(StrictEmailValidator).to receive(:strict_validation_enabled?).and_return(true).at_least(1)
+        stub_const("StrictEmailValidator::DATE_SINCE_STRICT_EMAIL_VALIDATION", now)
+      end
+      context 'when value is username' do
+        let(:email) { 'username' }
+        it { is_expected.to be_falsey }
+      end
+
+      context 'when value does not contain extension' do
+        let(:email) { 'username@mailserver' }
+        it { is_expected.to be_falsey }
+      end
+
+      context 'when value include an alias' do
+        let(:email) { 'username+alias@mailserver.fr' }
+        it { is_expected.to be_truthy }
+      end
+
+      context 'when value includes accents' do
+        let(:email) { 'tech@démarches.gouv.fr' }
+        it { is_expected.to be_truthy }
+      end
+
+      context 'when value is the classic standard user@domain.ext' do
+        let(:email) { 'username@mailserver.domain' }
+        it { is_expected.to be_truthy }
+      end
+    end
+  end
 end