Merge branch 'ms-adds-custom-email-incorrect-target-email-error' into 'master'

Adds custom email incorrect_forwarding_target error

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/138371



Merged-by: Frédéric Caplette <fcaplette@gitlab.com>
Approved-by: Rudy Crespo <rcrespo@gitlab.com>
Approved-by: Frédéric Caplette <fcaplette@gitlab.com>
Reviewed-by: Nicolas Dular <ndular@gitlab.com>
Reviewed-by: Marcin Sedlak-Jakubowski <msedlakjakubowski@gitlab.com>
Reviewed-by: Frédéric Caplette <fcaplette@gitlab.com>
Reviewed-by: Marc Saleiko <msaleiko@gitlab.com>
Reviewed-by: Rudy Crespo <rcrespo@gitlab.com>
Co-authored-by: Marc Saleiko <msaleiko@gitlab.com>

app/assets/javascripts/projects/settings_service_desk/components/custom_email.vue

@@ -25,6 +25,10 @@ export default {
   I18N_STATE_VERIFICATION_FINISHED_TOGGLE_HELP,
   I18N_RESET_BUTTON_LABEL,
   props: {
+    incomingEmail: {
+      type: String,
+      required: true,
+    },
     customEmail: {
       type: String,
       required: true,
@@ -128,7 +132,13 @@ export default {
       <p class="gl-mb-0">
         <strong>{{ errorLabel }}</strong>
       </p>
-      <p>{{ errorDescription }}</p>
+      <p>
+        <gl-sprintf :message="errorDescription">
+          <template #incomingEmail>
+            <code>{{ incomingEmail }}</code>
+          </template>
+        </gl-sprintf>
+      </p>
     </template>
     <p>{{ resetNote }}</p>

app/assets/javascripts/projects/settings_service_desk/components/custom_email_wrapper.vue

@@ -216,6 +216,7 @@ export default {
           <custom-email
             v-if="customEmail"
+            :incoming-email="incomingEmail"
             :custom-email="customEmail"
             :smtp-address="smtpAddress"
             :verification-state="verificationState"

app/assets/javascripts/projects/settings_service_desk/custom_email_constants.js

@@ -132,6 +132,12 @@ export const I18N_ERROR_READ_TIMEOUT_LABEL = s__('ServiceDesk|Read timeout');
 export const I18N_ERROR_READ_TIMEOUT_DESC = s__(
   'ServiceDesk|The SMTP server did not respond in time.',
 );
+export const I18N_ERROR_INCORRECT_FORWARDING_TARGET_LABEL = s__(
+  'ServiceDesk|Incorrect forwarding target',
+);
+export const I18N_ERROR_INCORRECT_FORWARDING_TARGET_DESC = s__(
+  'ServiceDesk|Forward all emails to the custom email address to %{incomingEmail}.',
+);
 export const I18N_VERIFICATION_ERRORS = {
   smtp_host_issue: {
@@ -158,4 +164,8 @@ export const I18N_VERIFICATION_ERRORS = {
     label: I18N_ERROR_READ_TIMEOUT_LABEL,
     description: I18N_ERROR_READ_TIMEOUT_DESC,
   },
+  incorrect_forwarding_target: {
+    label: I18N_ERROR_INCORRECT_FORWARDING_TARGET_LABEL,
+    description: I18N_ERROR_INCORRECT_FORWARDING_TARGET_DESC,
+  },
 };

app/mailers/previews/notify_preview.rb

@@ -272,6 +272,10 @@ def service_desk_verification_result_email_for_verified_state
     end
   end
+  def service_desk_verification_result_email_for_incorrect_forwarding_target_error
+    service_desk_verification_result_email_for_error_state(error: :incorrect_forwarding_target)
+  end
+
   def service_desk_verification_result_email_for_read_timeout_error
     service_desk_verification_result_email_for_error_state(error: :read_timeout)
   end

app/models/service_desk/custom_email_verification.rb

@@ -11,7 +11,8 @@ class CustomEmailVerification < ApplicationRecord
       mail_not_received_within_timeframe: 2,
       invalid_credentials: 3,
       smtp_host_issue: 4,
-      read_timeout: 5
+      read_timeout: 5,
+      incorrect_forwarding_target: 6
     }
     attr_encrypted :token,

app/services/service_desk/custom_email_verifications/update_service.rb

@@ -44,6 +44,7 @@ def already_failed_and_no_mail?
       def verify
         return :mail_not_received_within_timeframe if mail_not_received_within_timeframe?
+        return :incorrect_forwarding_target if forwarded_to_service_desk_alias_address?
         return :incorrect_from if incorrect_from?
         return :incorrect_token if incorrect_token?
@@ -55,6 +56,16 @@ def mail_not_received_within_timeframe?
         mail.blank? || !verification.in_timeframe?
       end
+      def forwarded_to_service_desk_alias_address?
+        return false unless Gitlab::Email::ServiceDeskEmail.enabled?
+
+        # Users must use the Service Desk address created from `incoming_email`
+        # so all reply by email features work as expected.
+        # Using the Service Desk alias address generated from `service_desk_email`
+        # doesn't allow to ingest email replies, so we'd always add a new issue.
+        addresses_from_headers.include?(project.service_desk_alias_address)
+      end
+
       def incorrect_from?
         # Does the email forwarder preserve the FROM header?
         mail.from.first != settings.custom_email
@@ -70,6 +81,12 @@ def incorrect_token?
         scan_result.first.first != verification.token
       end
+      def addresses_from_headers
+        # Common headers for forwarding target addresses are
+        # `To` and `Delivered-To`. We may expand that list if necessary.
+        (Array(mail.to) + Array(mail['Delivered-To']).map(&:value)).uniq
+      end
+
       def error_parameter_missing
         error_response(s_('ServiceDesk|Service Desk setting or verification object missing'))
       end

app/views/notify/service_desk_verification_result_email.html.haml

@@ -11,6 +11,7 @@
 - verify_email_address = @service_desk_setting.custom_email_address_for_verification
 - code_open = '<code>'.html_safe
 - code_end = '</code>'.html_safe
+- service_desk_incoming_address = @service_desk_setting.project.service_desk_incoming_address
 %tr
   %td.text-content
@@ -59,5 +60,10 @@
           %b
             = s_('Notify|Read timeout:')
           = s_('Notify|The SMTP server did not respond in time.')
+      - if @verification.incorrect_forwarding_target?
+        %p
+          %b
+            = s_('Notify|Incorrect forwarding target:')
+          = html_escape(s_('Notify|Forward all emails to the custom email address to %{code_open}%{service_desk_incoming_address}%{code_end}.')) % { code_open: code_open, service_desk_incoming_address: service_desk_incoming_address, code_end: code_end }
       %p
         = html_escape(s_('Notify|To restart the verification process, go to your %{settings_link_start}project\'s Service Desk settings page%{settings_link_end}.')) % { settings_link_start: settings_link_start, settings_link_end: settings_link_end }

app/views/notify/service_desk_verification_result_email.text.erb

 <% project_name = @service_desk_setting.project.human_name %>
 <% email_address = @service_desk_setting.custom_email %>
 <% verify_email_address = @service_desk_setting.custom_email_address_for_verification %>
+<% service_desk_incoming_address = @service_desk_setting.project.service_desk_incoming_address %>
 <% if @verification.finished? %>
   <%= s_("Notify|Email successfully verified") %>
@@ -35,6 +36,9 @@
   <% elsif @verification.read_timeout? %>
     <%= s_('Notify|Read timeout:') %>
     <%= s_('Notify|The SMTP server did not respond in time.') %>
+  <% elsif @verification.incorrect_forwarding_target? %>
+    <%= s_('Notify|Incorrect forwarding target:') %>
+    <%= s_('Notify|Forward all emails to the custom email address to %{code_open}%{service_desk_incoming_address}%{code_end}.') % { code_open: '', service_desk_incoming_address: service_desk_incoming_address, code_end: '' } %>
   <% end %>
   <%= s_('Notify|To restart the verification process, go to your %{settings_link_start}project\'s Service Desk settings page%{settings_link_end}.') % { settings_link_start: '', settings_link_end: '' } %>

doc/user/project/service_desk/configure.md

@@ -264,6 +264,26 @@ To troubleshoot this:
      1. Select one of the supported authentication methods in the custom email setup form.
+##### Incorrect forwarding target
+
+You might get an error that states that an incorrect forwarding target was used.
+
+This occurs when the verification email was forwarded to a different email address than the
+project-specific Service Desk address that's displayed in the custom email configuration form.
+
+You must use the Service Desk address generated from `incoming_email`. Forwarding to the additional
+Service Desk alias address generated from `service_desk_email` is not supported because it doesn't support
+all reply by email functionalities.
+
+To troubleshoot this:
+
+1. Find the correct email address to forward emails to. Either:
+   - Note the address from the verification result email that all project owners and the user that
+      triggered the verification process receive.
+   - Copy the address from the **Service Desk email address to forward emails to** input in the
+      custom email setup form.
+1. Forward all emails to the custom email address to the correct target email address.
+
 ### Enable or disable the custom email address
 After the custom email address has been verified, administrators can enable or disable sending Service Desk emails via the custom email address.

locale/gitlab.pot

@@ -32602,6 +32602,9 @@ msgstr ""
 msgid "Notify|Fingerprint: %{fingerprint}"
 msgstr ""
+msgid "Notify|Forward all emails to the custom email address to %{code_open}%{service_desk_incoming_address}%{code_end}."
+msgstr ""
+
 msgid "Notify|Here are the results for your CSV import for %{project_link}."
 msgstr ""
@@ -32626,6 +32629,9 @@ msgstr ""
 msgid "Notify|Incorrect %{code_open}From%{code_end} header:"
 msgstr ""
+msgid "Notify|Incorrect forwarding target:"
+msgstr ""
+
 msgid "Notify|Incorrect verification token:"
 msgstr ""
@@ -44718,9 +44724,15 @@ msgstr ""
 msgid "ServiceDesk|For help setting up the Service Desk for your instance, please contact an administrator."
 msgstr ""
+msgid "ServiceDesk|Forward all emails to the custom email address to %{incomingEmail}."
+msgstr ""
+
 msgid "ServiceDesk|Incorrect From header"
 msgstr ""
+msgid "ServiceDesk|Incorrect forwarding target"
+msgstr ""
+
 msgid "ServiceDesk|Incorrect verification token"
 msgstr ""

spec/frontend/projects/settings_service_desk/components/custom_email_spec.js

@@ -3,7 +3,6 @@ import { mount } from '@vue/test-utils';
 import { nextTick } from 'vue';
 import CustomEmail from '~/projects/settings_service_desk/components/custom_email.vue';
 import {
-  I18N_VERIFICATION_ERRORS,
   I18N_STATE_VERIFICATION_STARTED,
   I18N_STATE_VERIFICATION_FAILED,
   I18N_STATE_VERIFICATION_FAILED_RESET_PARAGRAPH,
@@ -15,6 +14,7 @@ describe('CustomEmail', () => {
   let wrapper;
   const defaultProps = {
+    incomingEmail: 'incoming+test-1-issue-@example.com',
     customEmail: 'user@example.com',
     smtpAddress: 'smtp.example.com',
     verificationState: 'started',
@@ -70,19 +70,21 @@ describe('CustomEmail', () => {
   });
   describe('verification error', () => {
-    it.each([
-      'smtp_host_issue',
-      'invalid_credentials',
-      'mail_not_received_within_timeframe',
-      'incorrect_from',
-      'incorrect_token',
-      'read_timeout',
-    ])('displays %s label and description', (error) => {
+    it.each`
+      error                                   | label                                                 | description
+      ${'smtp_host_issue'}                    | ${'SMTP host issue'}                                  | ${'A connection to the specified host could not be made or an SSL issue occurred.'}
+      ${'invalid_credentials'}                | ${'Invalid credentials'}                              | ${'The given credentials (username and password) were rejected by the SMTP server, or you need to explicitly set an authentication method.'}
+      ${'mail_not_received_within_timeframe'} | ${'Verification email not received within timeframe'} | ${"The verification email wasn't received in time. There is a 30 minutes timeframe for verification emails to appear in your instance's Service Desk. Make sure that you have set up email forwarding correctly."}
+      ${'incorrect_from'}                     | ${'Incorrect From header'}                            | ${'Check your forwarding settings and make sure the original email sender remains in the From header.'}
+      ${'incorrect_token'}                    | ${'Incorrect verification token'}                     | ${"The received email didn't contain the verification token that was sent to your email address."}
+      ${'read_timeout'}                       | ${'Read timeout'}                                     | ${'The SMTP server did not respond in time.'}
+      ${'incorrect_forwarding_target'}        | ${'Incorrect forwarding target'}                      | ${`Forward all emails to the custom email address to ${defaultProps.incomingEmail}`}
+    `('displays $error label and description', ({ error, label, description }) => {
       createWrapper({ verificationError: error });
       const text = wrapper.text();
-      expect(text).toContain(I18N_VERIFICATION_ERRORS[error].label);
-      expect(text).toContain(I18N_VERIFICATION_ERRORS[error].description);
+      expect(text).toContain(label);
+      expect(text).toContain(description);
     });
   });

spec/frontend/projects/settings_service_desk/components/custom_email_wrapper_spec.js

@@ -38,6 +38,12 @@ describe('CustomEmailWrapper', () => {
     customEmailEndpoint: '/flightjs/Flight/-/service_desk/custom_email',
   };
+  const defaultCustomEmailProps = {
+    incomingEmail: defaultProps.incomingEmail,
+    customEmail: 'user@example.com',
+    smtpAddress: 'smtp.example.com',
+  };
+
   const showToast = jest.fn();
   const createWrapper = (props = {}) => {
@@ -117,8 +123,7 @@ describe('CustomEmailWrapper', () => {
         expect(showToast).toHaveBeenCalledWith(I18N_TOAST_SAVED);
         expect(findCustomEmail().props()).toEqual({
-          customEmail: 'user@example.com',
-          smtpAddress: 'smtp.example.com',
+          ...defaultCustomEmailProps,
           verificationState: 'started',
           verificationError: null,
           isEnabled: false,
@@ -140,8 +145,7 @@ describe('CustomEmailWrapper', () => {
     it('displays CustomEmail component', () => {
       expect(findCustomEmail().props()).toEqual({
-        customEmail: 'user@example.com',
-        smtpAddress: 'smtp.example.com',
+        ...defaultCustomEmailProps,
         verificationState: 'started',
         verificationError: null,
         isEnabled: false,
@@ -193,8 +197,7 @@ describe('CustomEmailWrapper', () => {
     it('fetches data from endpoint and displays CustomEmail component', () => {
       expect(findCustomEmail().props()).toEqual({
-        customEmail: 'user@example.com',
-        smtpAddress: 'smtp.example.com',
+        ...defaultCustomEmailProps,
         verificationState: 'failed',
         verificationError: 'smtp_host_issue',
         isEnabled: false,
@@ -225,8 +228,7 @@ describe('CustomEmailWrapper', () => {
     it('fetches data from endpoint and displays CustomEmail component', () => {
       expect(findCustomEmail().props()).toEqual({
-        customEmail: 'user@example.com',
-        smtpAddress: 'smtp.example.com',
+        ...defaultCustomEmailProps,
         verificationState: 'finished',
         verificationError: null,
         isEnabled: false,
@@ -257,8 +259,7 @@ describe('CustomEmailWrapper', () => {
       expect(showToast).toHaveBeenCalledWith(I18N_TOAST_ENABLED);
       expect(findCustomEmail().props()).toEqual({
-        customEmail: 'user@example.com',
-        smtpAddress: 'smtp.example.com',
+        ...defaultCustomEmailProps,
         verificationState: 'finished',
         verificationError: null,
         isEnabled: true,
@@ -279,8 +280,7 @@ describe('CustomEmailWrapper', () => {
     it('fetches data from endpoint and displays CustomEmail component', () => {
       expect(findCustomEmail().props()).toEqual({
-        customEmail: 'user@example.com',
-        smtpAddress: 'smtp.example.com',
+        ...defaultCustomEmailProps,
         verificationState: 'finished',
         verificationError: null,
         isEnabled: true,
@@ -301,8 +301,7 @@ describe('CustomEmailWrapper', () => {
       expect(showToast).toHaveBeenCalledWith(I18N_TOAST_DISABLED);
       expect(findCustomEmail().props()).toEqual({
-        customEmail: 'user@example.com',
-        smtpAddress: 'smtp.example.com',
+        ...defaultCustomEmailProps,
         verificationState: 'finished',
         verificationError: null,
         isEnabled: false,

spec/lib/gitlab/email/handler/service_desk_handler_spec.rb

@@ -462,7 +462,7 @@ def receive_reply
           end
         end
-        shared_examples 'a handler that does not verify the custom email' do |error_identifier|
+        shared_examples 'a handler that does not verify the custom email' do
           it 'does not verify the custom email address' do
             # project has no owner, so only notify verification triggerer
             expect(Notify).to receive(:service_desk_verification_result_email).once
@@ -477,20 +477,32 @@ def receive_reply
           end
         end
-        shared_examples 'a handler that verifies Service Desk custom email verification emails' do
+        context 'when using incoming_email address' do
+          before do
+            stub_incoming_email_setting(enabled: true, address: 'support+%{key}@example.com')
+          end
+
           it_behaves_like 'an early exiting handler'
           context 'with valid service desk settings' do
             let_it_be(:user) { create(:user) }
-            let!(:settings) { create(:service_desk_setting, project: project, custom_email: 'custom-support-email@example.com') }
-            let!(:verification) { create(:service_desk_custom_email_verification, project: project, token: 'ZROT4ZZXA-Y6', triggerer: user) }
+            let_it_be(:credentials) { create(:service_desk_custom_email_credential, project: project) }
+            let_it_be_with_reload(:settings) do
+              create(:service_desk_setting, project: project, custom_email: 'custom-support-email@example.com')
+            end
+
+            let_it_be_with_reload(:verification) do
+              create(:service_desk_custom_email_verification, project: project, token: 'ZROT4ZZXA-Y6', triggerer: user)
+            end
             let(:message_delivery) { instance_double(ActionMailer::MessageDelivery) }
-            before do
+            before_all do
               project.add_maintainer(user)
+            end
+            before do
               allow(message_delivery).to receive(:deliver_later)
               allow(Notify).to receive(:service_desk_verification_result_email).and_return(message_delivery)
             end
@@ -521,7 +533,9 @@ def receive_reply
                 verification.update!(token: 'XXXXXXXXXXXX')
               end
-              it_behaves_like 'a handler that does not verify the custom email', 'incorrect_token'
+              it_behaves_like 'a handler that does not verify the custom email' do
+                let(:error_identifier) { 'incorrect_token' }
+              end
             end
             context 'and verification email ingested too late' do
@@ -529,7 +543,9 @@ def receive_reply
                 verification.update!(triggered_at: ServiceDesk::CustomEmailVerification::TIMEFRAME.ago)
               end
-              it_behaves_like 'a handler that does not verify the custom email', 'mail_not_received_within_timeframe'
+              it_behaves_like 'a handler that does not verify the custom email' do
+                let(:error_identifier) { 'mail_not_received_within_timeframe' }
+              end
             end
             context 'and from header differs from custom email address' do
@@ -537,19 +553,13 @@ def receive_reply
                 settings.update!(custom_email: 'different-from@example.com')
               end
-              it_behaves_like 'a handler that does not verify the custom email', 'incorrect_from'
+              it_behaves_like 'a handler that does not verify the custom email' do
+                let(:error_identifier) { 'incorrect_from' }
+              end
             end
           end
         end
-        context 'when using incoming_email address' do
-          before do
-            stub_incoming_email_setting(enabled: true, address: 'support+%{key}@example.com')
-          end
-
-          it_behaves_like 'a handler that verifies Service Desk custom email verification emails'
-        end
-
         context 'when using service_desk_email address' do
           let(:receiver) { Gitlab::Email::ServiceDeskReceiver.new(email_raw) }
@@ -557,7 +567,35 @@ def receive_reply
             stub_service_desk_email_setting(enabled: true, address: 'support+%{key}@example.com')
           end
-          it_behaves_like 'a handler that verifies Service Desk custom email verification emails'
+          it_behaves_like 'an early exiting handler'
+
+          context 'with valid service desk settings' do
+            let_it_be(:user) { create(:user) }
+            let_it_be(:credentials) { create(:service_desk_custom_email_credential, project: project) }
+
+            let_it_be_with_reload(:settings) do
+              create(:service_desk_setting, project: project, custom_email: 'custom-support-email@example.com')
+            end
+
+            let_it_be_with_reload(:verification) do
+              create(:service_desk_custom_email_verification, project: project, token: 'ZROT4ZZXA-Y6', triggerer: user)
+            end
+
+            let(:message_delivery) { instance_double(ActionMailer::MessageDelivery) }
+
+            before_all do
+              project.add_maintainer(user)
+            end
+
+            before do
+              allow(message_delivery).to receive(:deliver_later)
+              allow(Notify).to receive(:service_desk_verification_result_email).and_return(message_delivery)
+            end
+
+            it_behaves_like 'a handler that does not verify the custom email' do
+              let(:error_identifier) { 'incorrect_forwarding_target' }
+            end
+          end
         end
       end
     end

spec/mailers/emails/service_desk_spec.rb

@@ -625,5 +625,10 @@
       let(:error_identifier) { 'read_timeout' }
       let(:expected_text) { 'Read timeout' }
     end
+
+    it_behaves_like 'a custom email verification process result email with error' do
+      let(:error_identifier) { 'incorrect_forwarding_target' }
+      let(:expected_text) { 'Incorrect forwarding target' }
+    end
   end
 end

spec/services/service_desk/custom_email_verifications/update_service_spec.rb

@@ -27,6 +27,9 @@
     before do
       allow(message_delivery).to receive(:deliver_later)
       allow(Notify).to receive(:service_desk_verification_result_email).and_return(message_delivery)
+
+      stub_incoming_email_setting(enabled: true, address: 'support+%{key}@example.com')
+      stub_service_desk_email_setting(enabled: true, address: 'contact+%{key}@example.com')
     end
     shared_examples 'a failing verification process' do |expected_error_identifier|
@@ -118,7 +121,34 @@
           verification.update!(token: 'ZROT4ZZXA-Y6') # token from email fixture
         end
-        let(:email_raw) { email_fixture('emails/service_desk_custom_email_address_verification.eml') }
+        let(:service_desk_address) { project.service_desk_incoming_address }
+        let(:verification_address) { 'custom-support-email+verify@example.com' }
+        let(:verification_token) { 'ZROT4ZZXA-Y6' }
+        let(:shared_email_raw) do
+          <<~EMAIL
+          From: Flight Support <custom-support-email@example.com>
+          Subject: Verify custom email address custom-support-email@example.com for Flight
+          Auto-Submitted: no
+
+
+          This email is auto-generated. It verifies the ownership of the entered Service Desk custom email address and
+          correct functionality of email forwarding.
+
+          Verification token: #{verification_token}
+          --
+
+          You're receiving this email because of your account on 127.0.0.1.
+          EMAIL
+        end
+
+        let(:email_raw) do
+          <<~EMAIL
+          Delivered-To: #{service_desk_address}
+          To: #{verification_address}
+          #{shared_email_raw}
+          EMAIL
+        end
+
         let(:mail_object) { Mail::Message.new(email_raw) }
         it 'verifies and sends result emails' do
@@ -160,6 +190,38 @@
           it_behaves_like 'a failing verification process', 'mail_not_received_within_timeframe'
         end
+        context 'and service desk address from service_desk_email was used as forwarding target' do
+          let(:service_desk_address) { project.service_desk_alias_address }
+
+          it_behaves_like 'a failing verification process', 'incorrect_forwarding_target'
+
+          context 'when multiple Delivered-To headers are present' do
+            let(:email_raw) do
+              <<~EMAIL
+              Delivered-To: other@example.com
+              Delivered-To: #{service_desk_address}
+              To: #{verification_address}
+              #{shared_email_raw}
+              EMAIL
+            end
+
+            it_behaves_like 'a failing verification process', 'incorrect_forwarding_target'
+          end
+
+          context 'when multiple To headers are present' do
+            # Microsoft Exchange forwards emails this way when forwarding
+            # to an external email address using a transport rule
+            let(:email_raw) do
+              <<~EMAIL
+              To: #{service_desk_address}, #{verification_address}
+              #{shared_email_raw}
+              EMAIL
+            end
+
+            it_behaves_like 'a failing verification process', 'incorrect_forwarding_target'
+          end
+        end
+
         context 'when already verified' do
           let(:expected_error_message) { error_already_finished }