From 607fbf52870cf132651c0c0b3c4e1d974df6189f Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Tue, 28 May 2024 11:08:34 +0200 Subject: [PATCH 1/5] feat(user): block unverified_email from being sent --- app/lib/balancer_delivery_method.rb | 15 ++++++ app/models/individual.rb | 2 + app/models/user.rb | 2 + spec/lib/balancer_delivery_method_spec.rb | 62 +++++++++++++++++++++-- 4 files changed, 78 insertions(+), 3 deletions(-) diff --git a/app/lib/balancer_delivery_method.rb b/app/lib/balancer_delivery_method.rb index 2f9774af0..2ed33ae91 100644 --- a/app/lib/balancer_delivery_method.rb +++ b/app/lib/balancer_delivery_method.rb @@ -14,6 +14,7 @@ # # Be sure to restart your server when you modify this file. class BalancerDeliveryMethod + BYPASS_UNVERIFIED_MAIL_PROTECTION = 'BYPASS_UNVERIFIED_MAIL_PROTECTION'.freeze FORCE_DELIVERY_METHOD_HEADER = 'X-deliver-with' # Allows configuring the random number generator used for selecting a delivery method, # mostly for testing purposes. @@ -24,6 +25,8 @@ class BalancerDeliveryMethod end def deliver!(mail) + return if prevent_delivery?(mail) + balanced_delivery_method = delivery_method(mail) ApplicationMailer.wrap_delivery_behavior(mail, balanced_delivery_method) @@ -40,6 +43,18 @@ class BalancerDeliveryMethod private + def prevent_delivery?(mail) + return false if mail[BYPASS_UNVERIFIED_MAIL_PROTECTION].present? + + user = User.find_by(email: mail.to.first) + return user.unverified_email? if user.present? + + individual = Individual.find_by(email: mail.to.first) + return individual.unverified_email? if individual.present? + + true + end + def force_delivery_method?(mail) @delivery_methods.keys.map(&:to_s).include?(mail[FORCE_DELIVERY_METHOD_HEADER]&.value) end diff --git a/app/models/individual.rb b/app/models/individual.rb index 812435242..fd9406391 100644 --- a/app/models/individual.rb +++ b/app/models/individual.rb @@ -29,4 +29,6 @@ class Individual < ApplicationRecord gender: fc_information.gender == 'female' ? GENDER_FEMALE : GENDER_MALE ) end + + def unverified_email? = !email_verified_at? end diff --git a/app/models/user.rb b/app/models/user.rb index d047ab1d1..576f65759 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -267,6 +267,8 @@ class User < ApplicationRecord super && blocked_at.nil? end + def unverified_email? = !email_verified_at? + private def does_not_merge_on_self diff --git a/spec/lib/balancer_delivery_method_spec.rb b/spec/lib/balancer_delivery_method_spec.rb index 7fb2495be..36c691195 100644 --- a/spec/lib/balancer_delivery_method_spec.rb +++ b/spec/lib/balancer_delivery_method_spec.rb @@ -1,7 +1,11 @@ RSpec.describe BalancerDeliveryMethod do class ExampleMailer < ApplicationMailer - def greet(name) - mail(to: "smtp_to", from: "smtp_from", body: "Hello #{name}") + def greet(name, bypass_unverified_mail_protection: true) + mail(to: name, from: "smtp_from", body: "Hello #{name}") + + if bypass_unverified_mail_protection + headers['BYPASS_UNVERIFIED_MAIL_PROTECTION'] = true + end end end @@ -9,7 +13,8 @@ RSpec.describe BalancerDeliveryMethod do before_action :set_x_deliver_with def greet(name) - mail(to: "smtp_to", from: "smtp_from", body: "Hello #{name}") + mail(to: name, from: "smtp_from", body: "Hello #{name}") + headers['BYPASS_UNVERIFIED_MAIL_PROTECTION'] = true end private @@ -145,6 +150,57 @@ RSpec.describe BalancerDeliveryMethod do end end + context 'when the email does not bypass unverified mail protection' do + let(:mail) { ExampleMailer.greet(email, bypass_unverified_mail_protection:) } + let(:bypass_unverified_mail_protection) { false } + + before do + ActionMailer::Base.balancer_settings = { mock_smtp: 10 } + mail.deliver_now + end + + context 'when the email belongs to a user' do + let(:email) { user.email } + let(:user) { create(:user, email: 'u@a.com', email_verified_at:) } + + context 'and the email is not verified' do + let(:email_verified_at) { nil } + + it { expect(mail).not_to have_been_delivered_using(MockSmtp) } + end + + context 'and the email is not verified but a bypass flag is added' do + let(:email_verified_at) { nil } + let(:bypass_unverified_mail_protection) { true } + + it { expect(mail).to have_been_delivered_using(MockSmtp) } + end + + context 'and the email is verified' do + let(:email_verified_at) { Time.current } + + it { expect(mail).to have_been_delivered_using(MockSmtp) } + end + end + + context 'when the email belongs to a individual' do + let(:email) { individual.email } + let(:individual) { create(:individual, email: 'u@a.com', email_verified_at:) } + + context 'and the email is not verified' do + let(:email_verified_at) { nil } + + it { expect(mail).not_to have_been_delivered_using(MockSmtp) } + end + + context 'and the email is verified' do + let(:email_verified_at) { Time.current } + + it { expect(mail).to have_been_delivered_using(MockSmtp) } + end + end + end + # Helpers def have_been_delivered_using(delivery_class) From 8104157da677999daa7b6a12f26a019832898db1 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Wed, 29 May 2024 09:52:54 +0200 Subject: [PATCH 2/5] feat(user): always allow devise mail --- app/mailers/concerns/balanced_delivery_concern.rb | 4 ++++ app/mailers/devise_user_mailer.rb | 2 ++ spec/lib/balancer_delivery_method_spec.rb | 11 +++++++---- spec/mailers/devise_user_mailer_spec.rb | 5 ++++- 4 files changed, 17 insertions(+), 5 deletions(-) diff --git a/app/mailers/concerns/balanced_delivery_concern.rb b/app/mailers/concerns/balanced_delivery_concern.rb index 486aadac4..3bbeb6d7e 100644 --- a/app/mailers/concerns/balanced_delivery_concern.rb +++ b/app/mailers/concerns/balanced_delivery_concern.rb @@ -8,6 +8,10 @@ module BalancedDeliveryConcern self.class.critical_email?(action_name) end + def bypass_unverified_mail_protection! + headers[BalancerDeliveryMethod::BYPASS_UNVERIFIED_MAIL_PROTECTION] = true + end + private def forced_delivery_provider? diff --git a/app/mailers/devise_user_mailer.rb b/app/mailers/devise_user_mailer.rb index d9acb25c3..26cad7c03 100644 --- a/app/mailers/devise_user_mailer.rb +++ b/app/mailers/devise_user_mailer.rb @@ -34,6 +34,8 @@ class DeviseUserMailer < Devise::Mailer @procedure = opts[:procedure_after_confirmation] || nil @prefill_token = opts[:prefill_token] + bypass_unverified_mail_protection! + I18n.with_locale(record.locale) do super end diff --git a/spec/lib/balancer_delivery_method_spec.rb b/spec/lib/balancer_delivery_method_spec.rb index 36c691195..659adb46e 100644 --- a/spec/lib/balancer_delivery_method_spec.rb +++ b/spec/lib/balancer_delivery_method_spec.rb @@ -1,20 +1,23 @@ RSpec.describe BalancerDeliveryMethod do class ExampleMailer < ApplicationMailer + include BalancedDeliveryConcern + def greet(name, bypass_unverified_mail_protection: true) mail(to: name, from: "smtp_from", body: "Hello #{name}") - if bypass_unverified_mail_protection - headers['BYPASS_UNVERIFIED_MAIL_PROTECTION'] = true - end + bypass_unverified_mail_protection! if bypass_unverified_mail_protection end end class ImportantEmail < ApplicationMailer + include BalancedDeliveryConcern + before_action :set_x_deliver_with def greet(name) mail(to: name, from: "smtp_from", body: "Hello #{name}") - headers['BYPASS_UNVERIFIED_MAIL_PROTECTION'] = true + + bypass_unverified_mail_protection! end private diff --git a/spec/mailers/devise_user_mailer_spec.rb b/spec/mailers/devise_user_mailer_spec.rb index ed0e946ff..88fd2c21c 100644 --- a/spec/mailers/devise_user_mailer_spec.rb +++ b/spec/mailers/devise_user_mailer_spec.rb @@ -5,7 +5,10 @@ RSpec.describe DeviseUserMailer, type: :mailer do subject { described_class.confirmation_instructions(user, token, opts = {}) } context 'without SafeMailer configured' do - it { expect(subject[BalancerDeliveryMethod::FORCE_DELIVERY_METHOD_HEADER]&.value).to eq(nil) } + it do + expect(subject[BalancerDeliveryMethod::FORCE_DELIVERY_METHOD_HEADER]&.value).to eq(nil) + expect(subject[BalancerDeliveryMethod::BYPASS_UNVERIFIED_MAIL_PROTECTION]).to be_present + end end context 'with SafeMailer configured' do From 5d259ec47b2dd0ae49578366483e4103291954de Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Wed, 29 May 2024 10:22:44 +0200 Subject: [PATCH 3/5] refactor(user): rename invite! -> invite_instructeur! --- app/controllers/instructeurs/groupe_instructeurs_controller.rb | 2 +- app/controllers/manager/instructeurs_controller.rb | 2 +- app/models/groupe_instructeur.rb | 2 +- app/models/user.rb | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/instructeurs/groupe_instructeurs_controller.rb b/app/controllers/instructeurs/groupe_instructeurs_controller.rb index d7bedc485..1ad00f79a 100644 --- a/app/controllers/instructeurs/groupe_instructeurs_controller.rb +++ b/app/controllers/instructeurs/groupe_instructeurs_controller.rb @@ -65,7 +65,7 @@ module Instructeurs administrateurs: [procedure.administrateurs.first] ) - user.invite! if user.valid? + user.invite_instructeur! if user.valid? user.instructeur end diff --git a/app/controllers/manager/instructeurs_controller.rb b/app/controllers/manager/instructeurs_controller.rb index 2bfe85d9c..1cd9c0b9c 100644 --- a/app/controllers/manager/instructeurs_controller.rb +++ b/app/controllers/manager/instructeurs_controller.rb @@ -2,7 +2,7 @@ module Manager class InstructeursController < Manager::ApplicationController def reinvite instructeur = Instructeur.find(params[:id]) - instructeur.user.invite! + instructeur.user.invite_instructeur! flash[:notice] = "Instructeur réinvité." redirect_to manager_instructeur_path(instructeur) end diff --git a/app/models/groupe_instructeur.rb b/app/models/groupe_instructeur.rb index bd9aad659..f628b136e 100644 --- a/app/models/groupe_instructeur.rb +++ b/app/models/groupe_instructeur.rb @@ -58,7 +58,7 @@ class GroupeInstructeur < ApplicationRecord if not_found_emails.present? instructeurs_to_add += not_found_emails.map do |email| user = User.create_or_promote_to_instructeur(email, SecureRandom.hex, administrateurs: procedure.administrateurs) - user.invite! + user.invite_instructeur! user.instructeur end end diff --git a/app/models/user.rb b/app/models/user.rb index 576f65759..d98229788 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -79,7 +79,7 @@ class User < ApplicationRecord owns?(dossier) || invite?(dossier) end - def invite! + def invite_instructeur! UserMailer.invite_instructeur(self, set_reset_password_token).deliver_later end From 7c514e3585a4e6efa786e2c07609bc815e2e9c0d Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Wed, 29 May 2024 12:24:05 +0200 Subject: [PATCH 4/5] feat(user): always allow invitation mail --- app/mailers/administrateur_mailer.rb | 2 ++ app/mailers/administration_mailer.rb | 4 ++++ app/mailers/user_mailer.rb | 4 ++++ spec/mailers/administrateur_mailer_spec.rb | 5 ++++- spec/mailers/administration_mailer_spec.rb | 12 +++++++++--- spec/mailers/user_mailer_spec.rb | 13 +++++++++++++ 6 files changed, 36 insertions(+), 4 deletions(-) diff --git a/app/mailers/administrateur_mailer.rb b/app/mailers/administrateur_mailer.rb index 0ed0d6598..19df4f960 100644 --- a/app/mailers/administrateur_mailer.rb +++ b/app/mailers/administrateur_mailer.rb @@ -8,6 +8,8 @@ class AdministrateurMailer < ApplicationMailer @expiration_date = @user.reset_password_sent_at + Devise.reset_password_within @subject = "N'oubliez pas d’activer votre compte administrateur" + bypass_unverified_mail_protection! + mail(to: user.email, subject: @subject, reply_to: CONTACT_EMAIL) diff --git a/app/mailers/administration_mailer.rb b/app/mailers/administration_mailer.rb index baa26e321..7880eb8e3 100644 --- a/app/mailers/administration_mailer.rb +++ b/app/mailers/administration_mailer.rb @@ -8,6 +8,8 @@ class AdministrationMailer < ApplicationMailer @author_name = "Équipe de #{APPLICATION_NAME}" subject = "Activez votre compte administrateur" + bypass_unverified_mail_protection! + mail(to: user.email, subject: subject, reply_to: CONTACT_EMAIL) @@ -16,6 +18,8 @@ class AdministrationMailer < ApplicationMailer def refuse_admin(admin_email) subject = "Votre demande de compte a été refusée" + bypass_unverified_mail_protection! + mail(to: admin_email, subject: subject, reply_to: CONTACT_EMAIL) diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb index f45892699..08d45509a 100644 --- a/app/mailers/user_mailer.rb +++ b/app/mailers/user_mailer.rb @@ -41,6 +41,8 @@ class UserMailer < ApplicationMailer configure_defaults_for_user(user) + bypass_unverified_mail_protection! + mail(to: user.email, subject: subject, reply_to: Current.contact_email) @@ -54,6 +56,8 @@ class UserMailer < ApplicationMailer configure_defaults_for_user(user) + bypass_unverified_mail_protection! + mail(to: user.email, subject: subject, reply_to: Current.contact_email) diff --git a/spec/mailers/administrateur_mailer_spec.rb b/spec/mailers/administrateur_mailer_spec.rb index a42edb2b3..baf023934 100644 --- a/spec/mailers/administrateur_mailer_spec.rb +++ b/spec/mailers/administrateur_mailer_spec.rb @@ -23,7 +23,10 @@ RSpec.describe AdministrateurMailer, type: :mailer do subject { described_class.activate_before_expiration(user, token) } context 'without SafeMailer configured' do - it { expect(subject[BalancerDeliveryMethod::FORCE_DELIVERY_METHOD_HEADER]&.value).to eq(nil) } + it do + expect(subject[BalancerDeliveryMethod::FORCE_DELIVERY_METHOD_HEADER]&.value).to eq(nil) + expect(subject['BYPASS_UNVERIFIED_MAIL_PROTECTION']).to be_present + end end context 'with SafeMailer configured' do diff --git a/spec/mailers/administration_mailer_spec.rb b/spec/mailers/administration_mailer_spec.rb index 42a416acf..e4dc16b2a 100644 --- a/spec/mailers/administration_mailer_spec.rb +++ b/spec/mailers/administration_mailer_spec.rb @@ -9,8 +9,11 @@ RSpec.describe AdministrationMailer, type: :mailer do it { expect(subject.subject).not_to be_empty } describe "when the user has not been activated" do - it { expect(subject.body).to include(admin_activate_path(token: token)) } - it { expect(subject.body).not_to include(edit_user_password_url(admin_user, reset_password_token: token)) } + it do + expect(subject.body).to include(admin_activate_path(token: token)) + expect(subject.body).not_to include(edit_user_password_url(admin_user, reset_password_token: token)) + expect(subject['BYPASS_UNVERIFIED_MAIL_PROTECTION']).to be_present + end end describe "when the user is already active" do @@ -25,6 +28,9 @@ RSpec.describe AdministrationMailer, type: :mailer do subject { described_class.refuse_admin(mail) } - it { expect(subject.subject).not_to be_empty } + it do + expect(subject.subject).not_to be_empty + expect(subject['BYPASS_UNVERIFIED_MAIL_PROTECTION']).to be_present + end end end diff --git a/spec/mailers/user_mailer_spec.rb b/spec/mailers/user_mailer_spec.rb index 7b6ef9641..0dc7a9212 100644 --- a/spec/mailers/user_mailer_spec.rb +++ b/spec/mailers/user_mailer_spec.rb @@ -168,4 +168,17 @@ RSpec.describe UserMailer, type: :mailer do end end end + + describe '.invite_instructeur' do + subject { described_class.invite_instructeur(user, "reset_token") } + + it { expect(subject['BYPASS_UNVERIFIED_MAIL_PROTECTION']).to be_present } + end + + describe '.invite_gestionnaire' do + let(:groupe_gestionnaire) { create(:groupe_gestionnaire) } + subject { described_class.invite_gestionnaire(user, "reset_token", groupe_gestionnaire) } + + it { expect(subject['BYPASS_UNVERIFIED_MAIL_PROTECTION']).to be_present } + end end From 819fa2cde2ff61b798619dce9aa12d88492aebf4 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Fri, 31 May 2024 14:33:40 +0200 Subject: [PATCH 5/5] feat(User): always allow reset_password_instructions --- app/mailers/devise_user_mailer.rb | 6 ++++++ spec/mailers/devise_user_mailer_spec.rb | 1 + 2 files changed, 7 insertions(+) diff --git a/app/mailers/devise_user_mailer.rb b/app/mailers/devise_user_mailer.rb index 26cad7c03..5e993daa0 100644 --- a/app/mailers/devise_user_mailer.rb +++ b/app/mailers/devise_user_mailer.rb @@ -41,6 +41,12 @@ class DeviseUserMailer < Devise::Mailer end end + def reset_password_instructions(record, token, opts = {}) + bypass_unverified_mail_protection! + + super + end + def self.critical_email?(action_name) true end diff --git a/spec/mailers/devise_user_mailer_spec.rb b/spec/mailers/devise_user_mailer_spec.rb index 88fd2c21c..2044da2f2 100644 --- a/spec/mailers/devise_user_mailer_spec.rb +++ b/spec/mailers/devise_user_mailer_spec.rb @@ -73,6 +73,7 @@ RSpec.describe DeviseUserMailer, type: :mailer do it "respect preferred domain" do expect(header_value("From", subject.message)).to include(CONTACT_EMAIL) expect(subject.message.to_s).to include("#{ENV.fetch("APP_HOST_LEGACY")}/users/password") + expect(subject[BalancerDeliveryMethod::BYPASS_UNVERIFIED_MAIL_PROTECTION]).to be_present end end