From 5fe2ad236fcf956cf0612c3b9c678026ad576a03 Mon Sep 17 00:00:00 2001 From: KishenKumarrrrr Date: Thu, 8 Aug 2024 14:05:43 +0800 Subject: [PATCH] refactor: modify default from address --- backend/src/core/config.ts | 4 +-- backend/src/core/utils/from-address.ts | 8 ++++- .../src/email/middlewares/email.middleware.ts | 15 +++++++--- .../tests/email-campaign.routes.test.ts | 24 +++++++-------- .../tests/email-transactional.routes.test.ts | 30 +++++++++---------- backend/src/test-utils/test-env.ts | 2 +- e2e/config.ts | 8 ++--- .../dashboard/create/email/EmailTemplate.tsx | 2 +- frontend/src/locales/en/messages.po | 2 +- worker/src/core/config.ts | 2 +- 10 files changed, 54 insertions(+), 43 deletions(-) diff --git a/backend/src/core/config.ts b/backend/src/core/config.ts index ae8e40c62..2ebfc77a1 100644 --- a/backend/src/core/config.ts +++ b/backend/src/core/config.ts @@ -810,9 +810,7 @@ const config: Config = convict({ }) // If mailFrom was not set in an env var, set it using the app_name -const defaultMailFrom = `${config.get( - 'APP_NAME' -)} ` +const defaultMailFrom = `${config.get('APP_NAME')} ` config.set('mailFrom', config.get('mailFrom') || defaultMailFrom) // Override some defaults diff --git a/backend/src/core/utils/from-address.ts b/backend/src/core/utils/from-address.ts index 2e17615e3..c2dd40c00 100644 --- a/backend/src/core/utils/from-address.ts +++ b/backend/src/core/utils/from-address.ts @@ -15,7 +15,13 @@ export const isDefaultFromAddress = (from: string): boolean => { const { fromAddress: defaultFromAddress } = parseFromAddress( config.get('mailFrom') ) - return fromAddress === defaultFromAddress + // As part of a PSD directive, we have changed the defaultFromAddress to info@mail.postman.gov.sg. + // To prevent any breaking changes, we must now support both the new and old default address + const allowedDefaultAddresses = [ + defaultFromAddress, + 'donotreply@mail.postman.gov.sg', + ] + return allowedDefaultAddresses.includes(fromAddress) } export const fromAddressValidator = Joi.string() diff --git a/backend/src/email/middlewares/email.middleware.ts b/backend/src/email/middlewares/email.middleware.ts index 9b300bd4a..3cb00fb85 100644 --- a/backend/src/email/middlewares/email.middleware.ts +++ b/backend/src/email/middlewares/email.middleware.ts @@ -30,7 +30,7 @@ export interface EmailMiddleware { } export const INVALID_FROM_ADDRESS_ERROR_MESSAGE = - "Invalid 'from' email address, which must be either the default donotreply@mail.postman.gov.sg or the user's email (which requires setup with Postman team). Contact us to learn more." + "Invalid 'from' email address, which must be either the default info@mail.postman.gov.sg or the user's email (which requires setup with Postman team). Contact us to learn more." export const UNVERIFIED_FROM_ADDRESS_ERROR_MESSAGE = 'From Address has not been verified. Contact us to learn more.' @@ -223,10 +223,17 @@ export const InitEmailMiddleware = ( const { fromName: defaultFromName, fromAddress: defaultFromAddress } = parseFromAddress(config.get('mailFrom')) + // As part of a PSD directive, we have changed the defaultFromAddress to info@mail.postman.gov.sg. + // To prevent any breaking changes, we must now support both the new and old default address + const allowedDefaultAddresses = [ + defaultFromAddress, + 'donotreply@mail.postman.gov.sg', + ] + if ( - // user enters an email that is neither their own nor donotreply@mail.postman.gov.sg + // user enters an email that is neither their own nor info@mail.postman.gov.sg fromAddress !== userEmail && - fromAddress !== defaultFromAddress + allowedDefaultAddresses.includes(fromAddress) ) { logger.error({ message: INVALID_FROM_ADDRESS_ERROR_MESSAGE, @@ -304,7 +311,7 @@ export const InitEmailMiddleware = ( /** * Verifies the user's email address to see if it can be used as custom 'from' address - * - if it is the default donotreply@mail.postman.gov.sg, return immediately + * - if it is the default info@mail.postman.gov.sg, return immediately * - else, make network calls to AWS SES and the user's domain to verify DNS settings are set up properly. */ const verifyFromAddress = async ( diff --git a/backend/src/email/routes/tests/email-campaign.routes.test.ts b/backend/src/email/routes/tests/email-campaign.routes.test.ts index 0fdafb70b..b8f76981b 100644 --- a/backend/src/email/routes/tests/email-campaign.routes.test.ts +++ b/backend/src/email/routes/tests/email-campaign.routes.test.ts @@ -95,7 +95,7 @@ describe('PUT /campaign/{campaignId}/email/template', () => { expect.objectContaining({ message: `Template for campaign ${campaignId} updated`, template: expect.objectContaining({ - from: 'Postman ', + from: 'Postman ', reply_to: 'user@agency.gov.sg', }), }) @@ -106,7 +106,7 @@ describe('PUT /campaign/{campaignId}/email/template', () => { const res = await request(app) .put(`/campaign/${campaignId}/email/template`) .send({ - from: 'Agency.gov.sg ', + from: 'Agency.gov.sg ', subject: 'test', body: 'test', reply_to: 'user@agency.gov.sg', @@ -116,7 +116,7 @@ describe('PUT /campaign/{campaignId}/email/template', () => { expect.objectContaining({ message: `Template for campaign ${campaignId} updated`, template: expect.objectContaining({ - from: 'Agency.gov.sg via Postman ', + from: 'Agency.gov.sg via Postman ', reply_to: 'user@agency.gov.sg', }), }) @@ -127,7 +127,7 @@ describe('PUT /campaign/{campaignId}/email/template', () => { const res = await request(app) .put(`/campaign/${campaignId}/email/template`) .send({ - from: 'Postman ', + from: 'Postman ', subject: 'test', body: 'test', reply_to: 'user@agency.gov.sg', @@ -137,7 +137,7 @@ describe('PUT /campaign/{campaignId}/email/template', () => { expect.objectContaining({ message: `Template for campaign ${campaignId} updated`, template: expect.objectContaining({ - from: 'Postman ', + from: 'Postman ', reply_to: 'user@agency.gov.sg', }), }) @@ -194,7 +194,7 @@ describe('PUT /campaign/{campaignId}/email/template', () => { const res = await request(app) .put(`/campaign/${campaignId}/email/template`) .send({ - from: 'Custom Name ', + from: 'Custom Name ', subject: 'test', body: 'test', reply_to: 'user@agency.gov.sg', @@ -205,7 +205,7 @@ describe('PUT /campaign/{campaignId}/email/template', () => { expect.objectContaining({ message: `Template for campaign ${campaignId} updated`, template: expect.objectContaining({ - from: `Custom Name ${mailVia} `, + from: `Custom Name ${mailVia} `, reply_to: 'user@agency.gov.sg', }), }) @@ -266,7 +266,7 @@ describe('PUT /campaign/{campaignId}/email/template', () => { const res = await request(app) .put(`/campaign/${campaignId}/email/template`) .send({ - from: `Custom Name ${mailVia} `, + from: `Custom Name ${mailVia} `, subject: 'test', body: 'test', reply_to: 'user@agency.gov.sg', @@ -277,7 +277,7 @@ describe('PUT /campaign/{campaignId}/email/template', () => { expect.objectContaining({ message: `Template for campaign ${campaignId} updated`, template: expect.objectContaining({ - from: `Custom Name ${mailVia} `, + from: `Custom Name ${mailVia} `, reply_to: 'user@agency.gov.sg', }), }) @@ -347,7 +347,7 @@ describe('PUT /campaign/{campaignId}/email/template', () => { expect.objectContaining({ message: `Template for campaign ${protectedCampaignId} updated`, template: expect.objectContaining({ - from: 'Postman ', + from: 'Postman ', reply_to: 'user@agency.gov.sg', }), }) @@ -391,7 +391,7 @@ describe('PUT /campaign/{campaignId}/email/template', () => { message: 'Please re-upload your recipient list as template has changed.', template: expect.objectContaining({ - from: 'Postman ', + from: 'Postman ', reply_to: 'user@agency.gov.sg', }), }) @@ -418,7 +418,7 @@ describe('PUT /campaign/{campaignId}/email/template', () => { template: { subject: 'test', body: 'test {{name}}', - from: 'Postman ', + from: 'Postman ', reply_to: 'user@agency.gov.sg', params: ['name'], }, diff --git a/backend/src/email/routes/tests/email-transactional.routes.test.ts b/backend/src/email/routes/tests/email-transactional.routes.test.ts index 107b33dfc..41c448152 100644 --- a/backend/src/email/routes/tests/email-transactional.routes.test.ts +++ b/backend/src/email/routes/tests/email-transactional.routes.test.ts @@ -74,7 +74,7 @@ describe(`${emailTransactionalRoute}/send`, () => { recipient: 'recipient@agency.gov.sg', subject: 'subject', body: '

body

', - from: 'Postman ', + from: 'Postman ', reply_to: 'user@agency.gov.sg', } const generateRandomSmallFile = () => { @@ -150,7 +150,7 @@ describe(`${emailTransactionalRoute}/send`, () => { .spyOn(EmailService, 'sendEmail') .mockResolvedValue(true) - const from = 'Hello ' + const from = 'Hello ' const res = await request(app) .post(endpoint) .set('Authorization', `Bearer ${apiKey}`) @@ -172,14 +172,14 @@ describe(`${emailTransactionalRoute}/send`, () => { expect(transactionalEmail).not.toBeNull() expect(transactionalEmail).toMatchObject({ recipient: validApiCall.recipient, - from: 'Hello ', + from: 'Hello ', status: TransactionalEmailMessageStatus.Accepted, errorCode: null, }) expect(transactionalEmail?.params).toMatchObject({ subject: validApiCall.subject, body: validApiCall.body, - from: 'Hello ', + from: 'Hello ', reply_to: user.email, }) }) @@ -479,7 +479,7 @@ describe(`${emailTransactionalRoute}/send`, () => { .field('recipient', validApiCallAttachment.recipient) .field('subject', validApiCallAttachment.subject) .field('body', validApiCallAttachment.body) - .field('from', 'Postman ') + .field('from', 'Postman ') .field('reply_to', validApiCallAttachment.reply_to) .attach('attachments', validAttachment, validAttachmentName) expect(res.status).toBe(403) @@ -971,9 +971,9 @@ describe(`GET ${emailTransactionalRoute}`, () => { const endpoint = emailTransactionalRoute const acceptedMessage = { recipient: 'recipient@gmail.com', - from: 'Postman ', + from: 'Postman ', params: { - from: 'Postman ', + from: 'Postman ', subject: 'Test', body: 'Test Body', }, @@ -981,9 +981,9 @@ describe(`GET ${emailTransactionalRoute}`, () => { } const sentMessage = { recipient: 'recipient@agency.gov.sg', - from: 'Postman ', + from: 'Postman ', params: { - from: 'Postman ', + from: 'Postman ', subject: 'Test', body: 'Test Body', }, @@ -991,9 +991,9 @@ describe(`GET ${emailTransactionalRoute}`, () => { } const deliveredMessage = { recipient: 'recipient3@agency.gov.sg', - from: 'Postman ', + from: 'Postman ', params: { - from: 'Postman ', + from: 'Postman ', subject: 'Test', body: 'Test Body', }, @@ -1330,9 +1330,9 @@ describe(`GET ${emailTransactionalRoute}/:emailId`, () => { const message = await EmailMessageTransactional.create({ userId: user.id, recipient: 'recipient@agency.gov.sg', - from: 'Postman ', + from: 'Postman ', params: { - from: 'Postman ', + from: 'Postman ', subject: 'Test', body: 'Test Body', }, @@ -1368,9 +1368,9 @@ describe(`GET ${emailTransactionalRoute}/:emailId`, () => { const message = await EmailMessageTransactional.create({ userId: user.id, recipient: 'recipient@agency.gov.sg', - from: 'Postman ', + from: 'Postman ', params: { - from: 'Postman ', + from: 'Postman ', subject: 'Test', body: 'Test Body', }, diff --git a/backend/src/test-utils/test-env.ts b/backend/src/test-utils/test-env.ts index ff606050d..352866565 100644 --- a/backend/src/test-utils/test-env.ts +++ b/backend/src/test-utils/test-env.ts @@ -10,7 +10,7 @@ process.env.SENDGRID_PUBLIC_KEY = process.env.SESSION_SECRET = 'SESSIONSECRET' process.env.DB_URI = 'postgres://postgres:postgres@localhost:5432/postmangovsg_test' -process.env.BACKEND_SES_FROM = 'Postman ' +process.env.BACKEND_SES_FROM = 'Postman ' process.env.API_KEY_SALT_V1 = bcrypt.genSaltSync(1) process.env.TRANSACTIONAL_EMAIL_RATE = '1' process.env.TWILIO_CREDENTIAL_CACHE_MAX_AGE = '0' diff --git a/e2e/config.ts b/e2e/config.ts index cc52f4ed7..e218c12ea 100644 --- a/e2e/config.ts +++ b/e2e/config.ts @@ -1,11 +1,11 @@ export const API_URL = - process.env.API_URL || 'https://api-staging.postman.gov.sg'; + process.env.API_URL || "https://api-staging.postman.gov.sg"; export const API_KEY = process.env.API_KEY as string; -export const MAILBOX = process.env.MAIL_BOX || 'internal-testing@open.gov.sg'; +export const MAILBOX = process.env.MAIL_BOX || "internal-testing@open.gov.sg"; export const DASHBOARD_URL = - process.env.DASHBOARD_URL || 'https://staging.postman.gov.sg'; -export const POSTMAN_FROM = 'donotreply@mail.postman.gov.sg'; + process.env.DASHBOARD_URL || "https://staging.postman.gov.sg"; +export const POSTMAN_FROM = "info@mail.postman.gov.sg"; export const SMS_NUMBER = process.env.SMS_NUMBER as string; export const TWILIO_ACC_SID = process.env.TWILIO_ACC_SID as string; diff --git a/frontend/src/components/dashboard/create/email/EmailTemplate.tsx b/frontend/src/components/dashboard/create/email/EmailTemplate.tsx index 511aecfc0..d04d04c06 100644 --- a/frontend/src/components/dashboard/create/email/EmailTemplate.tsx +++ b/frontend/src/components/dashboard/create/email/EmailTemplate.tsx @@ -197,7 +197,7 @@ const EmailTemplate = ({ parseFromAddress(selectedFrom) // Use custom from name if it has already been set. For e.g., - // for "Custom "", we should + // for "Custom "", we should // use "Custom" instead of the default "Postman.gov.sg". setFromName( selectedFromAddress === initialFromAddress diff --git a/frontend/src/locales/en/messages.po b/frontend/src/locales/en/messages.po index 17e8f089e..61f6e5d01 100644 --- a/frontend/src/locales/en/messages.po +++ b/frontend/src/locales/en/messages.po @@ -120,7 +120,7 @@ msgstr "Want to send your campaigns through WhatsApp?" #: components/dashboard/settings/custom-from-address/CustomFromAddress.tsx:112 #: config.ts:87 msgid "defaultMailFrom" -msgstr "donotreply@mail.postman.gov.sg" +msgstr "info@mail.postman.gov.sg" #: components/dashboard/demo/demo-info-banner/DemoInfoBanner.tsx:8 msgid "demoMessage" diff --git a/worker/src/core/config.ts b/worker/src/core/config.ts index 8c582f8af..be0eff23d 100644 --- a/worker/src/core/config.ts +++ b/worker/src/core/config.ts @@ -397,7 +397,7 @@ const config: Config = convict({ }) // If mailFrom was not set in an env var, set it using the app_name -const defaultMailFrom = 'Postman.gov.sg ' +const defaultMailFrom = 'Postman.gov.sg ' config.set('mailFrom', config.get('mailFrom') || defaultMailFrom) // Only development is a non-production environment