Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(email): Fix SendGrid fallback integration #1026

Merged
merged 17 commits into from Apr 19, 2021

Conversation

zwliew
Copy link
Contributor

@zwliew zwliew commented Mar 3, 2021

Problem

We currently use SendGrid as our email fallback. This causes the
following functions to fail:

  1. Verifying custom from addresses (e.g. adding a new address, storing
    a campaign template with a custom address)
  2. Sending emails with custom from addresses will fail
  3. Receiving delivery report callbacks since the code currently parses the callbacks incorrectly

Closes #1021

Solution

Since custom from addresses are not verified with SendGrid, we shouldn't use
them at all.

In the future, if we do intend to authenticate custom from addresses with SendGrid,
we will also have to rewire verifyFromAddress() to use SendGrid APIs.

Features:

  1. Send all emails using the default address during the fallback.
  2. Disable verification of custom from addresses during the fallback.
  3. Disable storing of templates using custom from addresses.
  4. Documented instructions to activate the email fallback on the incident response doc
  5. Fix parsing of SendGrid webhook events

Before & After Screenshots

Click to show images

AFTER:
Attempting to verify a custom from address:
Screenshot 2021-03-10 at 9 07 24 AM

Attempting to save an email template using a custom from address:
Screenshot 2021-03-10 at 9 24 54 AM

Creating a new email campaign:
Screenshot 2021-03-10 at 9 03 46 AM

Tests

Test 1: all campaign emails should be sent using the default from address during fallback

  • Store a campaign using a custom from address
  • Set EMAIL_FALLBACK_ACTIVATE to true for the worker
  • Send the campaign
  • The campaign emails should be sent using the default from address

Test 2: verification of custom from addresses should be disabled during fallback

  • Set EMAIL_FALLBACK_ACTIVATE to true for the backend
  • Try to verify a custom from address
  • The UI should display an error message like in the first screenshot above

Test 3: storing of templates with custom from addresses should be disabled during fallback

  • Set EMAIL_FALLBACK_ACTIVATE to true for the backend
  • Try to store a campaign using a custom from address
  • The UI should display an error message like in the second screenshot above

Test 4: SES callbacks should be received and parsed correctly

Test 5: SendGrid callbacks should be received and parsed correctly

  • Test sending via SendGrid
    1. Set EMAIL_FALLBACK_ACTIVATE=true on Secrets Manager
    2. Prefix the Secrets Manager variables listed below with BAK_
    3. Add SendGrid-variants of the variables previously prefixed with BAK_
    4. Set the SendGrid event webhook HTTP POST URL to the correct callback server endpoint, with the correct username and password
    5. Deploy the branch on Travis with STAGING_BRANCH=sendgrid-email-fallback and DEPLOY_WORKER=true
    6. Send an email campaign (using a valid email and bouncetest@tribulant.com)
    7. Ensure that the status and error_code columns of theemail_messages are updated correctly

Deploy Notes

New environment variables:

  • EMAIL_FALLBACK_ACTIVATE: Switch to true to use SendGrid as fallback for all emails. Ensure that the SMTP settings are properly configured as well. (worker and backend)

Todo

  • Update the incident response doc to explain about the SendGrid fallback
BACKEND_SES_HOST
BACKEND_SES_USER
BACKEND_SES_PASS
WORKER_SES_HOST
WORKER_SES_USER
WORKER_SES_PASS

@zwliew zwliew changed the title fix: Disable the use of custom from addresses while on SendGrid email fallback fix: Disable custom from addresses while on SendGrid email fallback Mar 3, 2021
@zwliew zwliew marked this pull request as ready for review March 5, 2021 05:23
@lamkeewei lamkeewei self-requested a review March 8, 2021 02:31
@zwliew zwliew changed the title fix: Disable custom from addresses while on SendGrid email fallback fix(email): Disable custom from addresses during fallback Mar 10, 2021
@miazima
Copy link
Contributor

miazima commented Mar 17, 2021

We want to log email bounce sub types in email callbacks, here is the relevant pr #1062. Sendgrid provides bounce reasons which can be stored as error sub types too. I have not included this in the pr as we are unable to test the Sendgrid flow right now.

Here is the previous code written before I reverted the Sendgrid changes.
https://github.com/opengovsg/postmangovsg/blob/c5afd8cc36dbee6e1439f901fbaad5916d21d02f/backend/src/email/utils/callback/parsers/sendgrid.ts

@zwliew
Copy link
Contributor Author

zwliew commented Mar 17, 2021

We want to log email bounce sub types in email callbacks, here is the relevant pr #1062. Sendgrid provides bounce reasons which can be stored as error sub types too. I have not included this in the pr as we are unable to test the Sendgrid flow right now.

Here is the previous code written before I reverted the Sendgrid changes.
https://github.com/opengovsg/postmangovsg/blob/c5afd8cc36dbee6e1439f901fbaad5916d21d02f/backend/src/email/utils/callback/parsers/sendgrid.ts

Oh I see, thanks! Since we are unable to test the Sendgrid flow in time, perhaps it should be in a separate PR?

@lamkeewei
Copy link
Contributor

Update - pending SendGrid account setup before continue on review

We currently use SendGrid as our email fallback. This causes the
following functions to fail:

1. Verifying custom from addresses (e.g. adding a new address, storing
   a campaign template with a custom address)
2. Sending emails with custom from addresses may not fail, but will not
   be DKIM-authenticated.

Since custom from addresses are not DKIM-authenticated with SendGrid,
we shouldn't use them at all with SendGrid.

Hence,
1. Send all emails using the default address during the fallback.
2. Disallow verification of custom from addresses during the fallback.
3. Disallow storing of templates using custom from addresses.
Oops... this shouldn't be enabled by default.
SendGrid requires the payload to be parsed in text, byte for byte.
Since we parse the payload as JSON using `bodyParser.json()`, we
unintentionally strip away the whitespace characters and mess up the
encoding.

Fix the payload and parse it correctly before verifying the callback.
This is very brittle code and may break in the future without notice.

For example, the integration test that SendGrid provides fails the
verification, but actual email delivery events pass.

Revert it and keep it in the history for reference while investigating a
more reliable fix.

This reverts commit 68f9bd699000b4dc33d9227ec60bc069a9ac7f65.
backend/Dockerfile Outdated Show resolved Hide resolved
// This is to avoid stripping whitespace characters and messing with unicode encoding.
// This route affects SES callbacks as well, so we'll need to parse the text as JSON
// in the parseEvent() handle before parsing the SES event.
app.use('/v1/callback/email', bodyParser.text({ type: 'application/json' }))
Copy link
Contributor Author

@zwliew zwliew Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SendGrid events contain carriage return and newline characters (\r\n) and some even have unicode-encoded characters. The JSON body-parser that we currently use strips the newline characters and decodes the unicode characters, which causes the ECDSA verification to fail.

I had a less intrusive fix here: 201d0f4
But, I think it's not great for reasons stated in the revert: 5473c72

I ended up with this solution since it isn't super intrusive, but it might affect SES callbacks. I doubt it would in practice though.

One other solution which doesn't affect SES callbacks is to split the SES and SendGrid callback endpoints, and only apply the text body-parser for SendGrid, but that would be sort of a breaking change. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine. I've checked and only one body-parser middleware will run each time.

@zwliew zwliew changed the title fix(email): Disable custom from addresses during fallback fix(email): Fix SendGrid fallback integration Apr 15, 2021
* develop:
  fix(docker/awscli): upgrade to python3 and install python3-dev (#1140)
  feat(frontend): set up test fixtures and deployment processes (#1086)
…g fallback"

A side effect of this is that the email section in the settings tab will
be reverted to the request form (like when they don't have a custom from
registered).

This might cause users to be alarmed that their custom from is now
missing.

Let's just rely on the error message from the subsequent PUT request.

This reverts commit 40228df.
Copy link
Contributor

@lamkeewei lamkeewei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! Tested the following and works as expected:

  1. Activation of email fallback should prevent verifying new custom from address
  2. Activation of email fallback should prevent usage of custom from address
  3. SendGrid events are parsed and handled correctly
  4. SES events are parsed and handled correctly
  5. Sendgrid is able to deliver to SGMail email addresses

@lamkeewei lamkeewei merged commit 9a1e4f7 into develop Apr 19, 2021
@lamkeewei lamkeewei deleted the sendgrid-email-fallback branch April 19, 2021 09:55
lamkeewei added a commit that referenced this pull request May 5, 2021
* develop:
  refactor: use shared function to initialize models (#1172)
  chore: setup scaffolding for backend tests (#940)
  1.23.0
  fix(frontend): fix frontend test flakiness (#1162)
  fix: update successful delivery status only if error does not exist (#1150)
  chore: upgrade dependencies (#1153)
  feat: add unit tests for error states in critical workflows (#1118)
  feat: support whitelisting domains through `agencies` table (#1141)
  feat: add tests for happy paths in critical workflows (#1110)
  fix: prevent campaign names from causing dashboard rows to overflow (#1147)
  fix(email): Fix SendGrid fallback integration (#1026)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test Sendgrid
3 participants