opengovsg / postmangovsg

Templated message sending for public officers
https://postman.gov.sg
60 stars 16 forks source link

fix: email bcc bug #2222

Closed KishenKumarrrrr closed 1 year ago

KishenKumarrrrr commented 1 year ago

Problem

Email messages might have the wrong delivery status reflected due to a bug on Postman. This bug came about because upon receiving a delivery notification, we did not check whether or not the notification was related to our main recipient of the email before updating the DB table. This resulted in a race condition where delivery notifications to the main recipient and the BCCed individuals would result in the wrong status being displayed in the postman UI and report.

An example scenario is given below: 1) A record is created in the email_messages table with messageID = 1 1) Email to the main recipient hard bounced. AWS SES replies with a Bounce status. The status=Hard Bounce is reflected in the DB for messageID = 1 2) AWS SES replies with a Delivery status when the BCCed individual receives the email. The DB value is updated to status=SUCCESS for messageID = 1

messageID = 1 now has the wrong status being reflected.

Solution

I've added a function isNotificationAndEventForMainRecipient to check whether a particular delivery notification pertains to the main recipient of the email. If it does not, then we ignore the delivery notification from AWS SES.

This allows us to filter notification for the following statuses:

Notifications with status Open and Sent do not specify which user triggered the action. As such, we cannot use the above function to filter the notifications. In order to solve this problem, we only allow overwriting of statuses if the errorCode column is NULL. This means we will always show the user the most pessimistic status for a particular email. This is a better tradeoff than reflecting that a user received the email when they in fact did not.

jia1 commented 1 year ago

Thank you for the helpful PR description also!