Skip to content

Use Mandrill's HTTPS API to deliver emails#1506

Merged
jmhooper merged 1 commit intomasterfrom
jmhooper-mandrill-mailer
Jul 18, 2017
Merged

Use Mandrill's HTTPS API to deliver emails#1506
jmhooper merged 1 commit intomasterfrom
jmhooper-mandrill-mailer

Conversation

@jmhooper
Copy link
Contributor

Why: For our LOA3 ATO we need to configure our firewall to only allow HTTPS traffic. This will prevent us from sending emails over 587 with SMTPS.

This commit uses mandril_dm which provides an ActionMailer delivery method built on top of the mandril-api gem.

With the current setup, the gem is only installed and configured in production, causing the production environments to diverge a bit from development, which is worrisome.

This setup also requires a Mandrill API token, which I don't have at the moment, so I've yet to actually test this out to make sure it works. I'm marking it WIP until I get to that. Also, regarding the API tokens, deploying this will require adding the Mandrill API token to the env for the production apps.

ice_nine (0.11.2)
iniparse (1.4.2)
jmespath (1.3.1)
json (2.1.0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I came in here and manually changed the Gemfile.lock since the Mandrill API gem has json >= 1.7.7 < 2.0 as a dependency.

There is a PR to fix to bump the json version number, but it looks like it has been open since December 2016 with lots of +1s and little movement.

@@ -0,0 +1,5 @@
if Rails.env.production?
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we just guarded this with Figaro.env.mandrill_api_token.present?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💡

logins_per_ip_limit: '2'
logins_per_ip_period: '60'
max_mail_events: '2'
mandrill_api_token: '123abc'
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you familiar with our process for adding new configs like this? We'll need a corresponding devops PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, we're pairing on this later today.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is an excellent reference for a PR requiring a corresponding application.yml config change:

@jmhooper
Copy link
Contributor Author

Tested this out this morning. It works!

@jmhooper
Copy link
Contributor Author

The PR to add the token in the devops repo is here: 18f/identity-devops#392

@jmhooper
Copy link
Contributor Author

Looks like this melted on CI. I'll look into that.

@jmhooper
Copy link
Contributor Author

Fixed CI and pulled in the latest from master earlier today. This should be ready for review, but we'll want to hold off on merging until the corresponding devops PR is handled

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM!

@jmhooper
Copy link
Contributor Author

I got the 👍 from @timothy-spencer that all the devops changes have been deployed and this is ready to go.

**Why**: For our LOA3 ATO we need to configure our firewall to only
allow HTTPS traffic. This will prevent us from sending emails over 587
with SMTPS.

Set MandrilDM API key when key is present

**Why**: Whether the Mandrill API key is present is a better indicator
of whether we want to use Mandrill than which environment we are in

Remove smtp settings from application.yml.example

**Why**: We no longer need the SMTP settings in the production now that
we are using the Mandrill API to deliver emails.
@jmhooper jmhooper force-pushed the jmhooper-mandrill-mailer branch from 7c1ac9a to 2117969 Compare July 18, 2017 16:51
@jmhooper jmhooper merged commit 37d9d46 into master Jul 18, 2017
@brodygov brodygov deleted the jmhooper-mandrill-mailer branch July 22, 2017 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants