Email copy of message to user #98

randylubin · 2017-02-09T06:11:20Z

This branch enables users to send a copy of their message to themselves.

It adds a checkbox on the Compose page and uses the AWS SES API to send the message.

The AWS keys need to be added to Config. The EFF email address needs to be added to eff-aws-ses.js

I would strongly advise a code review as my knowledge of the backend is rusty and I did some cargo-cult coding based on how we implemented the ‘subscribe to EFF mailing list’ feature (we should check to make sure that feature still works).

I haven't added any tests, though I'm not sure I needed tests for the sections I worked on. I would also like a quick check to see if the error handling is correct.

Thanks!

I naively copied over everything from the EFF email list subscription and did some manual find / replace. Still a ways to go

created the models, paths, etc I think all that remains is implementing the actual email API (though there are probably bugs to work out)

wioux · 2017-02-09T18:05:05Z

server/routes/api/email-copy.js

+  var request = apiHelpers.getModelData(req.body, models.EmailCopyRequest);
+
+  var params = {
+    'contact_params': {


Let's unquote these keys, for consistency.

wioux · 2017-02-09T18:27:06Z

server/routes/api/email-copy.js

+
+  effAWSSES.emailUserCopyOfMessage(params, req.app.locals.CONFIG, function(err) {
+    if (err) {
+      res.status(400).json(resHelpers.makeError(err));


This copies a pattern from another area in the app, but I think it's problematic. We should bail out of the function here by returning. res.json sends the response, so the subsequent call below will cause problems in the err case as it tries to send a response when headers/body have already been sent here.

Ok - I just cut out the response – anything else here?

We should send back the 400/json response and also return from the function. So like

if (err) { return res.status(400).json(resHelpers.makeError(err)); }

I opened another PR #99 to address this in the rest of the app.

wioux · 2017-02-09T18:54:57Z

server/services/third-party-apis/eff-aws-ses.js

+
+var emailUserCopyOfMessage = function(params, config, cb) {
+
+  awsConfig = config.get('CREDENTIALS.AWS');


Can we change this to var awsConfig = ... to clarify the scope?

wioux · 2017-02-09T18:56:34Z

server/services/third-party-apis/eff-aws-ses.js

+  var to = [params.contact_params.email];
+
+  // this must relate to a verified SES account
+  var from = '[email protected]'; //TODO change to EFF email verified with AWS SES


You could add a key to the config for the from address.

Oh, you mention in your PR body the email address is supposed to be pulled from eff-aws-ses.js?

It's currently set to be hard coded in eff-aws-ses.js somewhat like the Civi CRM has an endpoint hard coded. I don't have any preference one way or another.

If we do want to put it in the Config, I'll defer to you all as the best place to put it. Doesn't neatly fit under Credentials or API

Sorry, I misunderstood here (this is eff-aws-ses.js). But yeah, let's put it in the config so we don't have to modify a tracked file. I agree it doesn't exactly fit but we could do API -> AWS_SES_EMAIL.

wioux · 2017-02-09T18:59:16Z

server/services/third-party-apis/eff-aws-ses.js

+  }
+
+  , function(err, data) {
+      if(err) throw err


Missing semi-colon.

wioux · 2017-02-09T19:07:48Z

Thanks @randylubin! I've added notes on some things I think should be addressed before we merge (email config and error handling).

randylubin · 2017-02-09T19:20:39Z

Thanks @wioux – I'll try to get to these tonight or this weekend

mfb · 2017-02-09T19:36:43Z

Does this do bounce handling? for https://act.eff.org we have an endpoint to receive bounce notifications from AWS and add to list of bad email addresses - see https://github.com/EFForg/action-center-platform/blob/master/app/controllers/bounce_controller.rb

I'm not sure of the details for how we handle bouncing addresses - I would guess they need to be stored for some period of time N rather than permanently, after which we could attempt delivery again.

randylubin · 2017-02-10T02:08:55Z

@mfb Right now there isn't any endpoint for bounce handling; I think we'd need to use AWS SNS service.

These aren't critical emails and they're only sent once, whereas the Action Center might send multiple emails. It might be fine to ignore bounces in this case.

Another option would be use the shared list with the Action Center bounce list. Depending on how things are configured, that list might start getting the bounces from D.io with the current setup.

mfb · 2017-02-10T02:13:42Z

The problem is that AWS will shut us off if we send emails to a bouncing address at too high a rate, to protect sending reputation. This happens when someone is pentesting or otherwise attacking a site. That's why Mailgun is easier to setup than SES - they automatically ignore delivery attempts to bouncing addresses.

randylubin · 2017-02-10T02:27:30Z

Ah, makes sense. The best solution might be to have just one EFF SES service that handles requests from all other EFF projects.

It seems we have a few options, let me know which makes sense.

I'm happy to recode it for Mailgun if EFF has an endpoint
I'm can have the server check a shared bounce list if you have an endpoint for me to hit
I can look into setting up SNS + an expiring email list in Redis. We already have an IP throttling set up that I can reference. This is a bigger project and I may need some help.

mfb · 2017-02-10T02:37:36Z

The redis bounce list sounds fine (as long as it has good uptime, because we'd need it to last longer than the IP throttling). AWS doesn't disclose what is the maximum bounce rate: http://docs.aws.amazon.com/ses/latest/DeveloperGuide/e-faq-bn.html A shared bounce list sounds neat but overly complex because we have to figure out more APIs for apps to talk to each other.

We haven't starting lining up another email delivery provider but it's doable.

randylubin · 2017-02-10T17:46:08Z

A possible quick solution: how about a captcha? That should mitigate the risks of pentesting / attack, right?

sinak · 2017-02-13T04:37:58Z

I think we should stick with the Redis. A captcha would work, but some users already need to fill in one or more captchas, so adding in another seems like a bit much.

@randylubin If the redis piece is a heavy lift on your end @l12s or @wioux might be able to help.

randylubin · 2017-02-15T02:22:39Z

I've started exploring the work around email bounces and it's a lot for me to bite off. I haven't done any work with Redis or Amazon SNS endpoints, though I'm sure I can learn. It might make sense to pair with someone in EFF who has access to the AWS console so we can get the notifications settings right.

I'm heading out on vacation for a week and can try to dive into this when I return (around Feb 25th).

Otherwise, this feature is ready to launch. We could also just launch now and disable the feature if our bounce rate is too high but that might be too high risk.

sinak · 2017-04-13T22:45:43Z

@wioux Would be awesome to get this feature live, as it's the single thing we get the most requests for. Any chance you have bandwidth to rake over getting the email bounces issue sorted?

randylubin added 8 commits February 2, 2017 20:20

added html / css for checkbox

10ba4ca

initial work on email copy API and logic

b5bcbb0

I naively copied over everything from the EFF email list subscription and did some manual find / replace. Still a ways to go

implemented backend for sending email copy

aff114d

created the models, paths, etc I think all that remains is implementing the actual email API (though there are probably bugs to work out)

working end to end

0ab93f3

Merge branch 'master' into email-message-to-user

59421bd

cleaned up code and console logs

4f2199d

remove public keys

7b09bbe

delete unused model

8ff5806

wioux reviewed Feb 9, 2017

View reviewed changes

update based on PR feedback

401ecd2

store SES email in credentials

14a045b

updated error handling to match PR 99

0dde964

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Email copy of message to user #98

Email copy of message to user #98

randylubin commented Feb 9, 2017

wioux Feb 9, 2017

randylubin Feb 10, 2017

wioux Feb 9, 2017

randylubin Feb 10, 2017

wioux Feb 11, 2017

wioux Feb 9, 2017

randylubin Feb 10, 2017

wioux Feb 9, 2017 •

edited

Loading

wioux Feb 9, 2017

randylubin Feb 10, 2017

randylubin Feb 10, 2017

wioux Feb 11, 2017

randylubin Feb 15, 2017

wioux Feb 9, 2017

randylubin Feb 10, 2017

wioux commented Feb 9, 2017

randylubin commented Feb 9, 2017

mfb commented Feb 9, 2017

randylubin commented Feb 10, 2017

mfb commented Feb 10, 2017

randylubin commented Feb 10, 2017

mfb commented Feb 10, 2017

randylubin commented Feb 10, 2017

sinak commented Feb 13, 2017

randylubin commented Feb 15, 2017

sinak commented Apr 13, 2017


		var emailUserCopyOfMessage = function(params, config, cb) {

		awsConfig = config.get('CREDENTIALS.AWS');

Email copy of message to user #98

Are you sure you want to change the base?

Email copy of message to user #98

Conversation

randylubin commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wioux Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wioux commented Feb 9, 2017

randylubin commented Feb 9, 2017

mfb commented Feb 9, 2017

randylubin commented Feb 10, 2017

mfb commented Feb 10, 2017

randylubin commented Feb 10, 2017

mfb commented Feb 10, 2017

randylubin commented Feb 10, 2017

sinak commented Feb 13, 2017

randylubin commented Feb 15, 2017

sinak commented Apr 13, 2017

wioux Feb 9, 2017 •

edited

Loading