Merge pull request #1009 from mozilla/fix-ses-callbacks-985

fix #985: update SES callback code for multiple emails

controllers/ses.js

@@ -91,7 +91,7 @@ async function handleComplaintMessage(message) {
 
 async function removeSubscribersFromDB(recipients) {
   for (const recipient of recipients) {
-    await DB.removeSubscriberByEmail(recipient.emailAddress);
+    await DB.removeEmail(recipient.emailAddress);
   }
 }
 

db/DB.js

@@ -75,6 +75,21 @@ const DB = {
     return subscriber;
   },
 
+  async getEmailAddressRecordByEmail(email) {
+    const emailAddresses = await knex("email_addresses").where({
+      "email": email, verified: true,
+    });
+    if (!emailAddresses) {
+      return null;
+    }
+    if (emailAddresses.length > 1) {
+      // TODO: handle multiple emails in separate(?) subscriber accounts?
+      log.warn("getEmailAddressRecordByEmail", {msg: "found the same email multiple times"});
+    }
+    return emailAddresses[0];
+  },
+
+
   async addSubscriberUnverifiedEmailHash(user, email) {
     const res = await knex("email_addresses").insert({
       subscriber_id: user.id,
@@ -280,18 +295,35 @@ const DB = {
     await knex("subscribers").where({"id": subscriber.id}).del();
   },
 
-  async removeSubscriberByEmail(email) {
-    const sha1 = getSha1(email);
-    return await this._getSha1EntryAndDo(sha1, async aEntry => {
-      await knex("subscribers")
-        .where("id", "=", aEntry.id)
+  // This is used by SES callbacks to remove email addresses when recipients
+  // perma-bounce or mark our emails as spam
+  // Removes from either subscribers or email_addresses as necessary
+  async removeEmail(email) {
+    const subscriber = await this.getSubscriberByEmail(email);
+    if (!subscriber) {
+      const emailAddress = await this.getEmailAddressRecordByEmail(email);
+      if (!emailAddress) {
+        log.warn("removed-subscriber-not-found");
+        return;
+      }
+      await knex("email_addresses")
+        .where({
+          "email": email,
+          "verified": true,
+        })
         .del();
-      log.info("removed-subscriber", { id: aEntry.id });
-      return aEntry;
-    }, async () => {
-      log.warn("removed-subscriber-not-found");
       return;
-    });
+    }
+    // This can fail if a subscriber has more email_addresses and marks
+    // a primary email as spam, but we should let it fail so we can see it
+    // in the logs
+    await knex("subscribers")
+      .where({
+        "primary_verification_token": subscriber.primary_verification_token,
+        "primary_sha1": subscriber.primary_sha1,
+      })
+      .del();
+    return;
   },
 
   async removeSubscriberByToken(token, emailSha1) {

db/seeds/test_subscribers.js

@@ -43,6 +43,7 @@ exports.TEST_SUBSCRIBERS = {
 
 exports.TEST_EMAIL_ADDRESSES = {
   firefox_account: {
+    id: 11111,
     subscriber_id: 12345,
     sha1: getSha1("firefoxaccount-secondary@test.com"),
     email: "firefoxaccount-secondary@test.com",

tests/controllers/ses.test.js

@@ -23,7 +23,7 @@ const createRequestBody = function(notificationType) {
 };
 
 
-test("ses notification with Permanent bounce unsubscribes recipient", async () => {
+test("ses notification with Permanent bounce unsubscribes recipient subscriber", async () => {
   // TODO: restore tests for ["General", "NoEmail", "Suppressed"] sub types
   const testEmail = "bounce@simulator.amazonses.com";
   const testHashes = [getSha1(testEmail)];
@@ -47,7 +47,7 @@ test("ses notification with Permanent bounce unsubscribes recipient", async () =
 });
 
 
-test("ses notification with Complaint unsubscribes recipient", async () => {
+test("ses notification with Complaint unsubscribes recipient subscriber", async () => {
   const testEmail = "complaint@simulator.amazonses.com";
 
   await DB.addSubscriber(testEmail);
@@ -69,6 +69,28 @@ test("ses notification with Complaint unsubscribes recipient", async () => {
 });
 
 
+test("ses notification with Complaint unsubscribes recipient from email_addresses", async () => {
+  const testPrimaryEmail = "secondary-email-complainer@mailinator.com";
+  const testSignupLanguage = "en";
+  const testUser = await DB.addSubscriber(testPrimaryEmail, testSignupLanguage);
+  const testEmail = "complaint@simulator.amazonses.com";
+
+  await DB.addSubscriberUnverifiedEmailHash(testUser, testEmail);
+
+  const req = httpMocks.createRequest({
+    method: "POST",
+    url: "/ses/notification",
+    body: createRequestBody("complaint"),
+  });
+  const resp = httpMocks.createResponse();
+
+  await ses.notification(req, resp);
+  expect(resp.statusCode).toEqual(200);
+  const noMoreEmailAddressRecord = await DB.getEmailAddressRecordByEmail(testEmail);
+  expect(noMoreEmailAddressRecord).toBeUndefined();
+});
+
+
 test("ses notification with invalid signature responds with error and doesn't change subscribers", async () => {
   const testEmail = "complaint@simulator.amazonses.com";
 
@@ -91,7 +113,7 @@ test("ses notification with invalid signature responds with error and doesn't ch
 });
 
 
-test("sns notification for FxA account delete deletes monitor subscriber record", async () => {
+test("sns notification for FxA account deletes monitor subscriber record", async () => {
   const testEmail = "fxa-deleter@mailinator.com";
   const testSignupLanguage = "en";
   const testFxaAccessToken = "abcdef123456789";

tests/db.test.js

@@ -2,7 +2,7 @@
 
 const HIBP = require("../hibp");
 const DB = require("../db/DB");
-const { TEST_EMAIL_ADDRESSES } = require("../db/seeds/test_subscribers");
+const { TEST_SUBSCRIBERS, TEST_EMAIL_ADDRESSES } = require("../db/seeds/test_subscribers");
 const getSha1 = require("../sha1-utils");
 
 require("./resetDB");
@@ -152,14 +152,40 @@ test("setAllEmailsToPrimary updates column and returns subscriber", async() => {
 });
 
 
-test("removeSubscriber accepts email and removes the email address", async () => {
+test("removeSubscriber accepts subscriber and removes everything from subscribers and email_addresses tables", async () => {
+  const startingSubscriber = await DB.getSubscriberByEmail(TEST_SUBSCRIBERS.firefox_account.primary_email);
+  expect(startingSubscriber.id).toEqual(TEST_SUBSCRIBERS.firefox_account.id);
+  const startingEmailAddressRecord = await DB.getEmailById(TEST_EMAIL_ADDRESSES.firefox_account.id);
+  expect(startingEmailAddressRecord.id).toEqual(TEST_EMAIL_ADDRESSES.firefox_account.id);
+
+  await DB.removeSubscriber(startingSubscriber);
+
+  const noMoreSubscribers = await DB.getSubscriberByEmail(startingSubscriber.primary_email);
+  expect(noMoreSubscribers).toBeUndefined();
+  const noMoreEmailAddress = await DB.getEmailById(startingEmailAddressRecord.id);
+  expect(noMoreEmailAddress).toBeUndefined();
+});
+
+
+test("removeEmail accepts email and removes from subscribers table", async () => {
   const testEmail = "removingFirefoxAccount@test.com";
 
   const verifiedSubscriber = await DB.addSubscriber(testEmail);
-  let subscribers = await DB.getSubscribersByHashes([getSha1(testEmail)]);
+  const subscribers = await DB.getSubscribersByHashes([getSha1(testEmail)]);
   expect(subscribers.length).toEqual(1);
 
-  await DB.removeSubscriberByEmail(verifiedSubscriber.primary_email);
-  subscribers = await DB.getSubscribersByHashes([getSha1(testEmail)]);
-  expect(subscribers.length).toEqual(0);
+  await DB.removeEmail(verifiedSubscriber.primary_email);
+  const noMoreSubscribers = await DB.getSubscribersByHashes([getSha1(testEmail)]);
+  expect(noMoreSubscribers.length).toEqual(0);
+});
+
+
+test("removeEmail accepts email and removes from email_addresses table", async () => {
+  const testEmailAddress = TEST_EMAIL_ADDRESSES.all_emails_to_primary;
+  const emailAddress = await DB.getEmailById(testEmailAddress.id);
+  expect(emailAddress.email).toEqual(testEmailAddress.email);
+
+  await DB.removeEmail(emailAddress.email);
+  const noMoreEmailAddress = await DB.getEmailById(testEmailAddress.id);
+  expect(noMoreEmailAddress).toBeUndefined();
 });