From ea9b6b5b5cdeeb2cb108fe52b6e0b4509b32ec15 Mon Sep 17 00:00:00 2001 From: Zhongpin Wang Date: Tue, 12 Nov 2024 16:20:52 +0100 Subject: [PATCH] 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 Co-authored-by: Deeksha Sinha <88374536+deekshas8@users.noreply.github.com> --- .changeset/lucky-badgers-swim.md | 5 ++ packages/mail-client/package.json | 1 - packages/mail-client/src/mail-client.spec.ts | 93 ++++---------------- packages/mail-client/src/mail-client.ts | 83 +++++------------ 4 files changed, 47 insertions(+), 135 deletions(-) create mode 100644 .changeset/lucky-badgers-swim.md diff --git a/.changeset/lucky-badgers-swim.md b/.changeset/lucky-badgers-swim.md new file mode 100644 index 0000000000..b893f22849 --- /dev/null +++ b/.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. diff --git a/packages/mail-client/package.json b/packages/mail-client/package.json index 262171d8f3..1e5fc804ae 100644 --- a/packages/mail-client/package.json +++ b/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" }, diff --git a/packages/mail-client/src/mail-client.spec.ts b/packages/mail-client/src/mail-client.spec.ts index c5289d006e..6d271a518d 100644 --- a/packages/mail-client/src/mail-client.spec.ts +++ b/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: 'from1@example.com', to: 'to1@example.com' }; 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 diff --git a/packages/mail-client/src/mail-client.ts b/packages/mail-client/src/mail-client.ts index 39b8ce6d55..57f88d2a60 100644 --- a/packages/mail-client/src/mail-client.ts +++ b/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 { const connectionOptions: SocksClientOptions = { proxy: buildSocksProxy(mailDestination), @@ -140,54 +151,12 @@ async function createSocket(mailDestination: MailDestination): Promise { } }; const { socket } = await SocksClient.createConnection(connectionOptions); - return socket; } -function retrieveGreeting(socket: Socket): Promise { - 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 { - 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 { 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( mailClientOptions?: MailClientOptions ): Promise { let socket: Socket | undefined; - let resendGreeting: Promise | 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( mailConfigs ); - if (resendGreeting) { - await resendGreeting; - } - teardown(transport, socket); return response; }