Merge branch 'qmnguyen0711/1488-send-rejection-email-if-incoming-emails-are-too-big' into 'master'

Send a rejection email if incoming emails are too large

See merge request gitlab-org/gitlab!77638

app/workers/email_receiver_worker.rb

@@ -90,37 +90,6 @@ class EmailReceiverWorker # rubocop:disable Scalability/IdempotentWorker
   def handle_failure(error)
     return unless raw.present?
 
-    can_retry = false
-    reason =
-      case error
-      when Gitlab::Email::UnknownIncomingEmail
-        s_("EmailError|We couldn't figure out what the email is for. Please create your issue or comment through the web interface.")
-      when Gitlab::Email::SentNotificationNotFoundError
-        s_("EmailError|We couldn't figure out what the email is in reply to. Please create your comment through the web interface.")
-      when Gitlab::Email::ProjectNotFound
-        s_("EmailError|We couldn't find the project. Please check if there's any typo.")
-      when Gitlab::Email::EmptyEmailError
-        can_retry = true
-        s_("EmailError|It appears that the email is blank. Make sure your reply is at the top of the email, we can't process inline replies.")
-      when Gitlab::Email::UserNotFoundError
-        s_("EmailError|We couldn't figure out what user corresponds to the email. Please create your comment through the web interface.")
-      when Gitlab::Email::UserBlockedError
-        s_("EmailError|Your account has been blocked. If you believe this is in error, contact a staff member.")
-      when Gitlab::Email::UserNotAuthorizedError
-        s_("EmailError|You are not allowed to perform this action. If you believe this is in error, contact a staff member.")
-      when Gitlab::Email::NoteableNotFoundError
-        s_("EmailError|The thread you are replying to no longer exists, perhaps it was deleted? If you believe this is in error, contact a staff member.")
-      when Gitlab::Email::InvalidAttachment
-        error.message
-      when Gitlab::Email::InvalidRecordError
-        can_retry = true
-        error.message
-      end
-
-    if reason
-      receiver.mail.body = nil
-
-      EmailRejectionMailer.rejection(reason, receiver.mail.encoded, can_retry).deliver_later
-    end
+    Gitlab::Email::FailureHandler.handle(receiver, error)
   end
 end

lib/api/internal/mail_room.rb

@@ -30,11 +30,15 @@ module API
           end
           post "/*mailbox_type" do
             worker = Gitlab::MailRoom.worker_for(params[:mailbox_type])
+            raw = request.body.read
             begin
-              worker.perform_async(request.body.read)
-            rescue Gitlab::SidekiqMiddleware::SizeLimiter::ExceedLimitError => e
+              worker.perform_async(raw)
+            rescue Gitlab::SidekiqMiddleware::SizeLimiter::ExceedLimitError
+              receiver = Gitlab::Email::Receiver.new(raw)
+              reason = Gitlab::Email::FailureHandler.handle(receiver, Gitlab::Email::EmailTooLarge.new)
+
               status 400
-              break { success: false, message: e.message }
+              break { success: false, message: reason }
             end
 
             status 200

lib/gitlab/email.rb

@@ -18,5 +18,6 @@ module Gitlab
     InvalidMergeRequestError = Class.new(InvalidRecordError)
     UnknownIncomingEmail = Class.new(ProcessingError)
     InvalidAttachment = Class.new(ProcessingError)
+    EmailTooLarge = Class.new(ProcessingError)
   end
 end

lib/gitlab/email/failure_handler.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Email
+    module FailureHandler
+      def self.handle(receiver, error)
+        can_retry = false
+        reason =
+          case error
+          when Gitlab::Email::UnknownIncomingEmail
+            s_("EmailError|We couldn't figure out what the email is for. Please create your issue or comment through the web interface.")
+          when Gitlab::Email::SentNotificationNotFoundError
+            s_("EmailError|We couldn't figure out what the email is in reply to. Please create your comment through the web interface.")
+          when Gitlab::Email::ProjectNotFound
+            s_("EmailError|We couldn't find the project. Please check if there's any typo.")
+          when Gitlab::Email::EmptyEmailError
+            can_retry = true
+            s_("EmailError|It appears that the email is blank. Make sure your reply is at the top of the email, we can't process inline replies.")
+          when Gitlab::Email::UserNotFoundError
+            s_("EmailError|We couldn't figure out what user corresponds to the email. Please create your comment through the web interface.")
+          when Gitlab::Email::UserBlockedError
+            s_("EmailError|Your account has been blocked. If you believe this is in error, contact a staff member.")
+          when Gitlab::Email::UserNotAuthorizedError
+            s_("EmailError|You are not allowed to perform this action. If you believe this is in error, contact a staff member.")
+          when Gitlab::Email::NoteableNotFoundError
+            s_("EmailError|The thread you are replying to no longer exists, perhaps it was deleted? If you believe this is in error, contact a staff member.")
+          when Gitlab::Email::InvalidAttachment
+            error.message
+          when Gitlab::Email::InvalidRecordError
+            can_retry = true
+            error.message
+          when Gitlab::Email::EmailTooLarge
+            s_("EmailError|We couldn't process your email because it is too large. Please create your issue or comment through the web interface.")
+          end
+
+        if reason
+          receiver.mail.body = nil
+
+          EmailRejectionMailer.rejection(reason, receiver.mail.encoded, can_retry).deliver_later
+        end
+
+        reason
+      end
+    end
+  end
+end

locale/gitlab.pot

@@ -13066,6 +13066,9 @@ msgstr ""
 msgid "EmailError|We couldn't find the project. Please check if there's any typo."
 msgstr ""
 
+msgid "EmailError|We couldn't process your email because it is too large. Please create your issue or comment through the web interface."
+msgstr ""
+
 msgid "EmailError|You are not allowed to perform this action. If you believe this is in error, contact a staff member."
 msgstr ""
 

spec/lib/gitlab/email/failure_handler_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Email::FailureHandler do
+  let(:raw_message) { fixture_file('emails/valid_reply.eml') }
+  let(:receiver) { Gitlab::Email::Receiver.new(raw_message) }
+
+  context 'email processing errors' do
+    where(:error, :message, :can_retry) do
+      [
+        [Gitlab::Email::UnknownIncomingEmail, "We couldn't figure out what the email is for", false],
+        [Gitlab::Email::SentNotificationNotFoundError, "We couldn't figure out what the email is in reply to", false],
+        [Gitlab::Email::ProjectNotFound, "We couldn't find the project", false],
+        [Gitlab::Email::EmptyEmailError, "It appears that the email is blank", true],
+        [Gitlab::Email::UserNotFoundError, "We couldn't figure out what user corresponds to the email", false],
+        [Gitlab::Email::UserBlockedError, "Your account has been blocked", false],
+        [Gitlab::Email::UserNotAuthorizedError, "You are not allowed to perform this action", false],
+        [Gitlab::Email::NoteableNotFoundError, "The thread you are replying to no longer exists", false],
+        [Gitlab::Email::InvalidAttachment, "Could not deal with that", false],
+        [Gitlab::Email::InvalidRecordError, "The note could not be created for the following reasons", true],
+        [Gitlab::Email::EmailTooLarge, "it is too large", false]
+      ]
+    end
+
+    with_them do
+      it "sends out a rejection email for #{params[:error]}" do
+        perform_enqueued_jobs do
+          described_class.handle(receiver, error.new(message))
+        end
+
+        email = ActionMailer::Base.deliveries.last
+        expect(email).not_to be_nil
+        expect(email.to).to match_array(["jake@adventuretime.ooo"])
+        expect(email.subject).to include("Rejected")
+        expect(email.body.parts.last.to_s).to include(message)
+      end
+
+      it 'strips out the body before passing to EmailRejectionMailer' do
+        mail = Mail.new(raw_message)
+        mail.body = nil
+
+        expect(EmailRejectionMailer).to receive(:rejection).with(match(message), mail.encoded, can_retry).and_call_original
+
+        described_class.handle(receiver, error.new(message))
+      end
+    end
+  end
+
+  context 'non-processing errors' do
+    where(:error) do
+      [
+        [Gitlab::Email::AutoGeneratedEmailError.new("")],
+        [ActiveRecord::StatementTimeout.new("StatementTimeout")],
+        [RateLimitedService::RateLimitedError.new(key: :issues_create, rate_limiter: nil)]
+      ]
+    end
+
+    with_them do
+      it "does not send a rejection email for #{params[:error]}" do
+        perform_enqueued_jobs do
+          described_class.handle(receiver, error)
+        end
+
+        expect(ActionMailer::Base.deliveries).to be_empty
+      end
+    end
+  end
+end

spec/requests/api/internal/mail_room_spec.rb

@@ -100,17 +100,26 @@ RSpec.describe API::Internal::MailRoom do
         )
       end
 
-      it 'responds with 400 bad request' do
-        Sidekiq::Testing.fake! do
-          expect do
-            post api("/internal/mail_room/incoming_email"), headers: auth_headers, params: email_content
-          end.not_to change { EmailReceiverWorker.jobs.size }
+      it 'responds with 400 bad request and replies with a failure message' do
+        perform_enqueued_jobs do
+          Sidekiq::Testing.fake! do
+            expect do
+              post api("/internal/mail_room/incoming_email"), headers: auth_headers, params: email_content
+            end.not_to change { EmailReceiverWorker.jobs.size }
+          end
         end
 
         expect(response).to have_gitlab_http_status(:bad_request)
         expect(Gitlab::Json.parse(response.body)).to match a_hash_including(
-          { "success" => false, "message" => "EmailReceiverWorker job exceeds payload size limit" }
+          "success" => false,
+          "message" => "We couldn't process your email because it is too large. Please create your issue or comment through the web interface."
         )
+
+        email = ActionMailer::Base.deliveries.last
+        expect(email).not_to be_nil
+        expect(email.to).to match_array(["jake@adventuretime.ooo"])
+        expect(email.subject).to include("Rejected")
+        expect(email.body.parts.last.to_s).to include("We couldn't process your email")
       end
     end
 

spec/workers/email_receiver_worker_spec.rb

@@ -21,87 +21,45 @@ RSpec.describe EmailReceiverWorker, :mailer do
     context "when an error occurs" do
       before do
         allow_any_instance_of(Gitlab::Email::Receiver).to receive(:execute).and_raise(error)
-        expect(Sidekiq.logger).to receive(:error).with(hash_including('exception.class' => error.class.name)).and_call_original
       end
 
-      context 'when the error is Gitlab::Email::EmptyEmailError' do
+      context 'when error is a processing error' do
         let(:error) { Gitlab::Email::EmptyEmailError.new }
 
-        it 'sends out a rejection email' do
-          perform_enqueued_jobs do
-            described_class.new.perform(raw_message)
+        it 'triggers email failure handler' do
+          expect(Gitlab::Email::FailureHandler).to receive(:handle) do |receiver, received_error|
+            expect(receiver).to be_a(Gitlab::Email::Receiver)
+            expect(receiver.mail.encoded).to eql(Mail::Message.new(raw_message).encoded)
+            expect(received_error).to be(error)
           end
 
-          email = ActionMailer::Base.deliveries.last
-          expect(email).not_to be_nil
-          expect(email.to).to eq(["jake@adventuretime.ooo"])
-          expect(email.subject).to include("Rejected")
-        end
-
-        it 'strips out the body before passing to EmailRejectionMailer' do
-          mail = Mail.new(raw_message)
-          mail.body = nil
-
-          expect(EmailRejectionMailer).to receive(:rejection).with(anything, mail.encoded, anything).and_call_original
-
           described_class.new.perform(raw_message)
         end
-      end
-
-      context 'when the error is Gitlab::Email::AutoGeneratedEmailError' do
-        let(:error) { Gitlab::Email::AutoGeneratedEmailError.new }
-
-        it 'does not send out any rejection email' do
-          perform_enqueued_jobs do
-            described_class.new.perform(raw_message)
-          end
-
-          should_not_email_anyone
-        end
-      end
 
-      context 'when the error is Gitlab::Email::InvalidAttachment' do
-        let(:error) { Gitlab::Email::InvalidAttachment.new("Could not deal with that") }
+        it 'logs the error' do
+          expect(Sidekiq.logger).to receive(:error).with(hash_including('exception.class' => error.class.name)).and_call_original
 
-        it 'reports the error to the sender' do
-          perform_enqueued_jobs do
-            described_class.new.perform(raw_message)
-          end
-
-          email = ActionMailer::Base.deliveries.last
-          expect(email).not_to be_nil
-          expect(email.to).to eq(["jake@adventuretime.ooo"])
-          expect(email.body.parts.last.to_s).to include("Could not deal with that")
+          described_class.new.perform(raw_message)
         end
       end
 
-      context 'when the error is ActiveRecord::StatementTimeout' do
+      context 'when error is not a processing error' do
         let(:error) { ActiveRecord::StatementTimeout.new("Statement timeout") }
 
-        it 'does not report the error to the sender' do
-          expect(Gitlab::ErrorTracking).to receive(:track_exception).with(error).and_call_original
-
-          perform_enqueued_jobs do
-            described_class.new.perform(raw_message)
+        it 'triggers email failure handler' do
+          expect(Gitlab::Email::FailureHandler).to receive(:handle) do |receiver, received_error|
+            expect(receiver).to be_a(Gitlab::Email::Receiver)
+            expect(receiver.mail.encoded).to eql(Mail::Message.new(raw_message).encoded)
+            expect(received_error).to be(error)
           end
 
-          email = ActionMailer::Base.deliveries.last
-          expect(email).to be_nil
+          described_class.new.perform(raw_message)
         end
-      end
-
-      context 'when the error is RateLimitedService::RateLimitedError' do
-        let(:error) { RateLimitedService::RateLimitedError.new(key: :issues_create, rate_limiter: Gitlab::ApplicationRateLimiter) }
 
-        it 'does not report the error to the sender' do
+        it 'reports the error' do
           expect(Gitlab::ErrorTracking).to receive(:track_exception).with(error).and_call_original
 
-          perform_enqueued_jobs do
-            described_class.new.perform(raw_message)
-          end
-
-          email = ActionMailer::Base.deliveries.last
-          expect(email).to be_nil
+          described_class.new.perform(raw_message)
         end
       end
     end