FIX: Prevent critical emails bypassing disable, and improve email test logic

- The test_email job is removed, because it was always being run synchronously (not in sidekiq)
- 34b29f62 added a bypass for critical emails, to match the spec. This removes the bypass, and removes the spec.
- This adapts the specs for 72ffabf6, so that they check for emails being sent
- This reimplements c2797921, allowing test emails to be sent even when emails are disabled

app/controllers/admin/email_controller.rb

@@ -10,14 +10,10 @@ class Admin::EmailController < Admin::AdminController
   def test
     params.require(:email_address)
     begin
-      Jobs::TestEmail.new.execute(to_address: params[:email_address])
-      if SiteSetting.disable_emails == "yes"
-        render json: { sent_test_email_message: I18n.t("admin.email.sent_test_disabled") }
-      elsif SiteSetting.disable_emails == "non-staff" && !User.find_by_email(params[:email_address])&.staff?
-        render json: { sent_test_email_message: I18n.t("admin.email.sent_test_disabled_for_non_staff") }
-      else
-        render json: { sent_test_email_message: I18n.t("admin.email.sent_test") }
-      end
+      message = TestMailer.send_test(params[:email_address])
+      Email::Sender.new(message, :test_message).send
+
+      render json: { sent_test_email_message: I18n.t("admin.email.sent_test") }
     rescue => e
       render json: { errors: [e.message] }, status: 422
     end

app/jobs/regular/test_email.rb

@@ -1,20 +0,0 @@
-require_dependency 'email/sender'
-
-module Jobs
-
-  # Asynchronously send an email
-  class TestEmail < Jobs::Base
-
-    sidekiq_options queue: 'critical'
-
-    def execute(args)
-
-      raise Discourse::InvalidParameters.new(:to_address) unless args[:to_address].present?
-
-      message = TestMailer.send_test(args[:to_address])
-      Email::Sender.new(message, :test_message).send
-    end
-
-  end
-
-end

app/jobs/regular/user_email.rb

@@ -54,9 +54,7 @@ module Jobs
       )
 
       if message
-        Email::Sender.new(message, type, user).send(
-          is_critical: self.class == Jobs::CriticalUserEmail
-        )
+        Email::Sender.new(message, type, user).send
 
         if (b = user.user_stat.bounce_score) > SiteSetting.bounce_score_erode_on_send
           # erode bounce score each time we send an email

config/locales/server.en.yml

@@ -2174,8 +2174,6 @@ en:
   admin:
     email:
       sent_test: "sent!"
-      sent_test_disabled: "cannot send because emails are disabled"
-      sent_test_disabled_for_non_staff: "cannot send because emails are disabled for non-staff"
 
   user:
     deactivated: "Was deactivated due to too many bounced emails to '%{email}'."

lib/email/sender.rb

@@ -11,6 +11,7 @@ require 'uri'
 require 'net/smtp'
 
 SMTP_CLIENT_ERRORS = [Net::SMTPFatalError, Net::SMTPSyntaxError]
+BYPASS_DISABLE_TYPES = ["admin_login", "test_message"]
 
 module Email
   class Sender
@@ -21,11 +22,10 @@ module Email
       @user = user
     end
 
-    def send(is_critical: false)
-      if SiteSetting.disable_emails == "yes" &&
-         @email_type.to_s != "admin_login" &&
-         !is_critical
+    def send
+      bypass_disable = BYPASS_DISABLE_TYPES.include?(@email_type.to_s)
 
+      if SiteSetting.disable_emails == "yes" && !bypass_disable
         return
       end
 
@@ -35,7 +35,7 @@ module Email
       return skip(SkippedEmailLog.reason_types[:sender_message_blank])    if @message.blank?
       return skip(SkippedEmailLog.reason_types[:sender_message_to_blank]) if @message.to.blank?
 
-      if SiteSetting.disable_emails == "non-staff"
+      if SiteSetting.disable_emails == "non-staff" && !bypass_disable
         return unless User.find_by_email(to_address)&.staff?
       end
 

spec/components/email/sender_spec.rb

@@ -12,15 +12,24 @@ describe Email::Sender do
       before { SiteSetting.disable_emails = "yes" }
 
       it "doesn't deliver mail when mails are disabled" do
-        Mail::Message.any_instance.expects(:deliver_now).never
-        message = Mail::Message.new(to: moderator.email , body: "hello")
-        expect(Email::Sender.new(message, :hello).send).to eq(nil)
+        message = UserNotifications.email_login(moderator)
+        Email::Sender.new(message, :email_login).send
+
+        expect(ActionMailer::Base.deliveries).to eq([])
       end
 
       it "delivers mail when mails are disabled but the email_type is admin_login" do
-        Mail::Message.any_instance.expects(:deliver_now).once
-        message = Mail::Message.new(to: moderator.email , body: "hello")
+        message = UserNotifications.admin_login(moderator)
         Email::Sender.new(message, :admin_login).send
+
+        expect(ActionMailer::Base.deliveries.first.to).to eq([moderator.email])
+      end
+
+      it "delivers mail when mails are disabled but the email_type is test_message" do
+        message = TestMailer.send_test(moderator.email)
+        Email::Sender.new(message, :test_message).send
+
+        expect(ActionMailer::Base.deliveries.first.to).to eq([moderator.email])
       end
     end
 

spec/jobs/test_email_spec.rb

@@ -1,25 +0,0 @@
-require 'rails_helper'
-require_dependency 'jobs/base'
-
-describe Jobs::TestEmail do
-
-  context '.execute' do
-    it 'raises an error when the address is missing' do
-      expect { Jobs::TestEmail.new.execute({}) }.to raise_error(Discourse::InvalidParameters)
-    end
-
-    context 'with an address' do
-
-      let (:mailer) { Mail::Message.new(to: 'eviltrout@test.domain') }
-
-      it 'delegates to the test mailer' do
-        Email::Sender.any_instance.expects(:send)
-        TestMailer.expects(:send_test).with('eviltrout@test.domain').returns(mailer)
-        Jobs::TestEmail.new.execute(to_address: 'eviltrout@test.domain')
-      end
-
-    end
-
-  end
-
-end

spec/jobs/user_email_spec.rb

@@ -80,15 +80,6 @@ describe Jobs::UserEmail do
 
       expect(ActionMailer::Base.deliveries).to eq([])
     end
-
-    it "sends when critical" do
-      SiteSetting.disable_emails = 'yes'
-      Jobs::CriticalUserEmail.new.execute(type: :confirm_new_email, user_id: user.id)
-
-      expect(ActionMailer::Base.deliveries.first.to).to contain_exactly(
-        user.email
-      )
-    end
   end
 
   context "recently seen" do

spec/requests/admin/email_controller_spec.rb

@@ -137,32 +137,40 @@ describe Admin::EmailController do
       let(:eviltrout) { Fabricate(:evil_trout) }
       let(:admin) { Fabricate(:admin) }
 
-      it 'does not sends mail to anyone when setting is "yes"' do
+      it 'bypasses disable when setting is "yes"' do
         SiteSetting.disable_emails = 'yes'
-
         post "/admin/email/test.json", params: { email_address: admin.email }
 
-        incoming = JSON.parse(response.body)
-        expect(incoming['sent_test_email_message']).to eq(I18n.t("admin.email.sent_test_disabled"))
-      end
+        expect(ActionMailer::Base.deliveries.first.to).to contain_exactly(
+          admin.email
+        )
 
-      it 'sends mail to staff only when setting is "non-staff"' do
-        SiteSetting.disable_emails = 'non-staff'
-
-        post "/admin/email/test.json", params: { email_address: admin.email }
         incoming = JSON.parse(response.body)
         expect(incoming['sent_test_email_message']).to eq(I18n.t("admin.email.sent_test"))
-
-        post "/admin/email/test.json", params: { email_address: eviltrout.email }
-        incoming = JSON.parse(response.body)
-        expect(incoming['sent_test_email_message']).to eq(I18n.t("admin.email.sent_test_disabled_for_non_staff"))
       end
 
-      it 'sends mail to everyone when setting is "no"' do
+      it 'bypasses disable when setting is "non-staff"' do
+        SiteSetting.disable_emails = 'non-staff'
+
+        post "/admin/email/test.json", params: { email_address: eviltrout.email }
+
+        expect(ActionMailer::Base.deliveries.first.to).to contain_exactly(
+          eviltrout.email
+        )
+
+        incoming = JSON.parse(response.body)
+        expect(incoming['sent_test_email_message']).to eq(I18n.t("admin.email.sent_test"))
+      end
+
+      it 'works when setting is "no"' do
         SiteSetting.disable_emails = 'no'
 
         post "/admin/email/test.json", params: { email_address: eviltrout.email }
 
+        expect(ActionMailer::Base.deliveries.first.to).to contain_exactly(
+          eviltrout.email
+        )
+
         incoming = JSON.parse(response.body)
         expect(incoming['sent_test_email_message']).to eq(I18n.t("admin.email.sent_test"))
       end