fix: Mail client issue with port 465 on premise (#5150)

* fix: skip workaround for port 465

* chore: add test for checking if retrieve or resend is skipped

* Changes from lint:fix

* chore: add changeset

* fix: use callback

* chore: make test work again

* fix: lint

* fix: e2e due to proxy option

* fix: lint

* fix: mailClientOptions and proxyConfiguration

* fix: remove unused mock implementation

* refactor: add Options type back

* refactor: buildSocksProxyUrl

* fix: lint

* fix: lint

---------

Co-authored-by: cloud-sdk-js <[email protected]>
Co-authored-by: Deeksha Sinha <[email protected]>

Author: 3 people

.changeset/lucky-badgers-swim.md

@@ -0,0 +1,5 @@
+---
+'@sap-cloud-sdk/mail-client': patch
+---
+
+[Fixed Issue] Fix mail client issue for port 465 with on-premise setup.

packages/mail-client/package.json

@@ -40,7 +40,6 @@
   "dependencies": {
     "@sap-cloud-sdk/connectivity": "^3.22.2",
     "@sap-cloud-sdk/util": "^3.22.2",
-    "async-retry": "^1.3.3",
     "nodemailer": "6.9.16",
     "socks": "2.8.3"
   },

packages/mail-client/src/mail-client.spec.ts

@@ -10,6 +10,7 @@ import {
 } from '../../../test-resources/test/test-util';
 import {
   buildSocksProxy,
+  buildSocksProxyUrl,
   isMailSentInSequential,
   sendMail
 } from './mail-client';
@@ -29,7 +30,8 @@ describe('mail client', () => {
   const mockTransport = {
     sendMail: jest.fn(),
     close: jest.fn(),
-    verify: jest.fn()
+    verify: jest.fn(),
+    set: jest.fn()
   };
 
   it('should work with destination from service - proxy-type Internet', async () => {
@@ -239,98 +241,26 @@ describe('mail client', () => {
         'proxy-authorization': 'jwt'
       }
     };
+
     const mailOptions: MailConfig = {
       from: '[email protected]',
       to: '[email protected]'
     };
 
     it('should create transport/socket, send mails and close the transport/socket', async () => {
-      const { connection, createConnectionSpy } = mockSocketConnection();
       const spyCreateTransport = jest
         .spyOn(nodemailer, 'createTransport')
         .mockReturnValue(mockTransport as any);
-      const spySendMail = jest
-        .spyOn(mockTransport, 'sendMail')
-        .mockImplementation(() => {
-          connection.socket.on('data', () => {});
-        });
-
+      const spySendMail = jest.spyOn(mockTransport, 'sendMail');
       const spyCloseTransport = jest.spyOn(mockTransport, 'close');
-      const spyEndSocket = jest.spyOn(connection.socket, 'end');
-      const spyDestroySocket = jest.spyOn(connection.socket, 'destroy');
 
       await expect(
         sendMail(destination, mailOptions, { sdkOptions: { parallel: false } })
       ).resolves.not.toThrow();
-      expect(createConnectionSpy).toHaveBeenCalledTimes(1);
       expect(spyCreateTransport).toHaveBeenCalledTimes(1);
       expect(spySendMail).toHaveBeenCalledTimes(1);
       expect(spySendMail).toHaveBeenCalledWith(mailOptions);
       expect(spyCloseTransport).toHaveBeenCalledTimes(1);
-      expect(spyEndSocket).toHaveBeenCalledTimes(1);
-      expect(spyDestroySocket).toHaveBeenCalledTimes(1);
-    });
-
-    it('should resend greeting', async () => {
-      const { connection } = mockSocketConnection();
-      jest
-        .spyOn(nodemailer, 'createTransport')
-        .mockReturnValue(mockTransport as any);
-
-      const req = sendMail(destination, mailOptions, {
-        sdkOptions: { parallel: false }
-      });
-
-      // The socket emits data for the first time before nodemailer listens to it.
-      // We re-emit the data until a listener listened for it.
-      // In this test we listen for the data event to check that we in fact re-emit the message.
-      const emitsTwice = new Promise(resolve => {
-        let dataEmitCount = 0;
-        const collectedData: string[] = [];
-        connection.socket.on('data', data => {
-          dataEmitCount++;
-          collectedData.push(data.toString());
-          if (dataEmitCount === 2) {
-            resolve(collectedData);
-          }
-        });
-      });
-
-      await expect(emitsTwice).resolves.toEqual([
-        '220 smtp.gmail.com ESMTP',
-        '220 smtp.gmail.com ESMTP'
-      ]);
-      await expect(req).resolves.not.toThrow();
-    });
-
-    it('should fail if nodemailer never listens to greeting', async () => {
-      mockSocketConnection();
-
-      jest
-        .spyOn(nodemailer, 'createTransport')
-        .mockReturnValue(mockTransport as any);
-
-      const req = sendMail(destination, mailOptions, {
-        sdkOptions: { parallel: false }
-      });
-
-      await expect(req).rejects.toThrowErrorMatchingInlineSnapshot(
-        '"Failed to re-emit greeting message. No data listener found."'
-      );
-    }, 15000);
-
-    it('should throw if greeting (really) was not received', async () => {
-      const { connection } = mockSocketConnection(true);
-
-      jest.spyOn(mockTransport, 'sendMail').mockImplementation(() => {
-        connection.socket.on('data', () => {});
-      });
-
-      await expect(() =>
-        sendMail(destination, mailOptions, {
-          sdkOptions: { parallel: false }
-        })
-      ).rejects.toThrowErrorMatchingInlineSnapshot('"Something went wrong"');
     });
   });
 });
@@ -378,6 +308,19 @@ describe('buildSocksProxy', () => {
     const proxy = buildSocksProxy(dest);
     expect(isValidSocksProxy(proxy)).toBe(true);
   });
+
+  it('build valid socks proxy url', () => {
+    const dest: MailDestination = {
+      proxyConfiguration: {
+        host: 'www.proxy.com',
+        port: 12345,
+        protocol: 'socks',
+        'proxy-authorization': 'jwt'
+      }
+    };
+    const proxyUrl = buildSocksProxyUrl(dest);
+    expect(proxyUrl).toBe('socks5://www.proxy.com:12345');
+  });
 });
 
 // copied from socks lib

packages/mail-client/src/mail-client.ts

@@ -3,16 +3,15 @@ import { resolveDestination } from '@sap-cloud-sdk/connectivity/internal';
 import { createLogger } from '@sap-cloud-sdk/util';
 import nodemailer from 'nodemailer';
 import { SocksClient } from 'socks';
-import retry from 'async-retry';
 import {
   customAuthRequestHandler,
   customAuthResponseHandler
 } from './socket-proxy';
+// eslint-disable-next-line import/no-internal-modules
+import type { Options } from 'nodemailer/lib/smtp-pool';
 import type { Socket } from 'node:net';
 import type { SentMessageInfo, Transporter } from 'nodemailer';
 import type { SocksClientOptions, SocksProxy } from 'socks';
-// eslint-disable-next-line import/no-internal-modules
-import type { Options } from 'nodemailer/lib/smtp-pool';
 import type {
   MailClientOptions,
   MailConfig,
@@ -130,6 +129,18 @@ export function buildSocksProxy(mailDestination: MailDestination): SocksProxy {
   };
 }
 
+/**
+ * @internal
+ */
+export function buildSocksProxyUrl(mailDestination: MailDestination): string {
+  if (!mailDestination.proxyConfiguration) {
+    throw Error(
+      'The proxy configuration is undefined, which is mandatory for creating a socket connection.'
+    );
+  }
+  return `socks5://${mailDestination.proxyConfiguration?.host}:${mailDestination.proxyConfiguration?.port}`;
+}
+
 async function createSocket(mailDestination: MailDestination): Promise<Socket> {
   const connectionOptions: SocksClientOptions = {
     proxy: buildSocksProxy(mailDestination),
@@ -140,54 +151,12 @@ async function createSocket(mailDestination: MailDestination): Promise<Socket> {
     }
   };
   const { socket } = await SocksClient.createConnection(connectionOptions);
-
   return socket;
 }
 
-function retrieveGreeting(socket: Socket): Promise<Buffer> {
-  logger.debug('Waiting for SMTP greeting message...');
-  return new Promise((resolve, reject) => {
-    const onData = data => {
-      logger.debug(`Data received from mail socket: ${data?.toString()}`);
-      if (data?.toString().startsWith('220')) {
-        logger.debug('Removing mail socket listeners...');
-        socket.removeListener('data', onData);
-        socket.removeListener('error', onError);
-        resolve(data);
-      }
-    };
-
-    const onError = err => {
-      reject(new Error(err));
-    };
-
-    socket.on('data', onData);
-    socket.on('error', onError);
-  });
-}
-
-async function resendGreetingUntilReceived(
-  greeting: Buffer,
-  socket: Socket
-): Promise<void> {
-  return retry(
-    () => {
-      // resend the greeting message until a listener is attached
-      // note: this is dangerous because there could be another listener that is not the mailer
-      if (!socket.emit('data', greeting)) {
-        throw new Error(
-          'Failed to re-emit greeting message. No data listener found.'
-        );
-      }
-    },
-    { maxRetryTime: 5000 }
-  );
-}
-
 function createTransport(
   mailDestination: MailDestination,
-  mailClientOptions?: MailClientOptions,
-  socket?: Socket
+  mailClientOptions?: MailClientOptions
 ): Transporter<SentMessageInfo> {
   const baseOptions: Options = {
     pool: true,
@@ -199,8 +168,11 @@ function createTransport(
     port: mailDestination.port
   };
 
-  if (mailDestination.proxyType === 'OnPremise' && socket) {
-    baseOptions.connection = socket;
+  if (mailDestination.proxyType === 'OnPremise') {
+    mailClientOptions = {
+      ...(mailClientOptions || {}),
+      proxy: buildSocksProxyUrl(mailDestination)
+    };
   }
 
   return nodemailer.createTransport({
@@ -274,14 +246,11 @@ async function sendMailWithNodemailer<T extends MailConfig>(
   mailClientOptions?: MailClientOptions
 ): Promise<MailResponse[]> {
   let socket: Socket | undefined;
-  let resendGreeting: Promise<void> | undefined;
-  if (mailDestination.proxyType === 'OnPremise') {
+  const transport = createTransport(mailDestination, mailClientOptions);
+  transport.set('proxy_handler_socks5', async (_, __, callback) => {
     socket = await createSocket(mailDestination);
-    // Workaround for incorrect order of events in nodemailer https://github.com/nodemailer/nodemailer/issues/1684
-    const greeting = await retrieveGreeting(socket);
-    resendGreeting = resendGreetingUntilReceived(greeting, socket);
-  }
-  const transport = createTransport(mailDestination, mailClientOptions, socket);
+    callback(null, { connection: socket });
+  });
 
   const mailConfigsFromDestination =
     buildMailConfigsFromDestination(mailDestination);
@@ -298,10 +267,6 @@ async function sendMailWithNodemailer<T extends MailConfig>(
         mailConfigs
       );
 
-  if (resendGreeting) {
-    await resendGreeting;
-  }
-
   teardown(transport, socket);
   return response;
 }