Merge pull request #14401 from mozilla/FXA-5772-pushbox-delete-user

feat(pushbox): delete all db records on account delete

packages/fxa-auth-server/lib/pushbox/db.ts

@@ -65,6 +65,10 @@ export class PushboxDB {
       .delete()
       .where({ user_id: x.uid, device_id: x.deviceId });
   }
+
+  async deleteAccount(uid: string) {
+    await Record.query().delete().where({ user_id: uid });
+  }
 }
 
 export default PushboxDB;

packages/fxa-auth-server/lib/pushbox/index.ts

@@ -308,7 +308,24 @@ export const pushboxApi = (
     },
 
     async deleteAccount(uid: string) {
-      // TODO to be implemented in FXA-5772
+      if (shouldUseDb()) {
+        const startTime = performance.now();
+        try {
+          await pushboxDb!.deleteAccount(uid);
+          statsd.timing(
+            'pushbox.db.delete.account.success',
+            performance.now() - startTime
+          );
+          statsd.increment('pushbox.db.delete.account');
+        } catch (err) {
+          statsd.timing(
+            'pushbox.db.delete.account.failure',
+            performance.now() - startTime
+          );
+          log.error('pushbox.db.delete.account', { error: err });
+          throw error.unexpectedError();
+        }
+      }
     },
   };
 };

packages/fxa-auth-server/lib/routes/account.ts

@@ -14,6 +14,7 @@ import TopEmailDomains from 'fxa-shared/email/topEmailDomains';
 import { tryResolveIpv4, tryResolveMx } from 'fxa-shared/email/validateEmail';
 import ScopeSet from 'fxa-shared/oauth/scopes';
 import { WebSubscription } from 'fxa-shared/subscriptions/types';
+import * as Sentry from '@sentry/node';
 import isA from 'joi';
 import Stripe from 'stripe';
 import { Container } from 'typedi';
@@ -75,7 +76,8 @@ export class AccountHandler {
     private verificationReminders: any,
     private subscriptionAccountReminders: any,
     private oauth: any,
-    private stripeHelper: StripeHelper
+    private stripeHelper: StripeHelper,
+    private pushbox: any
   ) {
     this.otpUtils = require('./utils/otp')(log, config, db);
     this.skipConfirmationForEmailAddresses = config.signinConfirmation
@@ -1554,6 +1556,16 @@ export class AccountHandler {
 
     await this.oauth.removeUser(uid);
 
+    // No need to await and block the other notifications.  The pushbox records
+    // will be deleted once they expire even if they were not successfully
+    // deleted here.
+    this.pushbox.deleteAccount(uid).catch((err: Error) => {
+      Sentry.withScope((scope) => {
+        scope.setContext('pushboxDeleteAccount', { uid });
+        Sentry.captureException(err);
+      });
+    });
+
     try {
       await this.push.notifyAccountDestroyed(uid, devices);
     } catch (err) {
@@ -1631,7 +1643,8 @@ export const accountRoutes = (
   verificationReminders: any,
   subscriptionAccountReminders: any,
   oauth: any,
-  stripeHelper: StripeHelper
+  stripeHelper: StripeHelper,
+  pushbox: any
 ) => {
   const accountHandler = new AccountHandler(
     log,
@@ -1646,7 +1659,8 @@ export const accountRoutes = (
     verificationReminders,
     subscriptionAccountReminders,
     oauth,
-    stripeHelper
+    stripeHelper,
+    pushbox
   );
   const routes = [
     {

packages/fxa-auth-server/lib/routes/index.js

@@ -70,7 +70,8 @@ module.exports = function (
     verificationReminders,
     subscriptionAccountReminders,
     oauthRawDB,
-    stripeHelper
+    stripeHelper,
+    pushbox
   );
   const oauth = require('./oauth')(log, config, db, mailer, devicesImpl);
   const devicesSessions = require('./devices-and-sessions')(

packages/fxa-auth-server/test/local/pushbox.js

@@ -264,6 +264,49 @@ describe('pushbox', () => {
         }
       );
     });
+
+    it('deletes all records for an account', () => {
+      stubDbModule.deleteAccount.resolves();
+      const log = mockLog();
+      const pushbox = pushboxApi(log, mockConfig, mockStatsD, stubConstructor);
+      return pushbox.deleteAccount(mockUid).then(
+        (res) => {
+          assert.isUndefined(res);
+          assert.strictEqual(
+            mockStatsD.timing.args[0][0],
+            'pushbox.db.delete.account.success'
+          );
+          sinon.assert.calledOnceWithExactly(
+            mockStatsD.increment,
+            'pushbox.db.delete.account'
+          );
+        },
+        (err) => {
+          assert.ok(false, err);
+        }
+      );
+    });
+
+    it('throws error when delete account fails', () => {
+      stubDbModule.deleteAccount.rejects(
+        new Error('someone deleted the pushboxv1 table')
+      );
+      const log = mockLog();
+      const pushbox = pushboxApi(log, mockConfig, mockStatsD, stubConstructor);
+      return pushbox.deleteAccount(mockUid).then(
+        () => assert.ok(false, 'should not happen'),
+        (err) => {
+          assert.ok(err);
+          assert.equal(err.errno, error.ERRNO.UNEXPECTED_ERROR);
+          sinon.assert.calledOnce(log.error);
+          assert.equal(log.error.args[0][0], 'pushbox.db.delete.account');
+          assert.equal(
+            log.error.args[0][1]['error']['message'],
+            'someone deleted the pushboxv1 table'
+          );
+        }
+      );
+    });
   });
 
   describe('using Pushbox service', () => {

packages/fxa-auth-server/test/local/routes/account.js

@@ -96,6 +96,7 @@ const makeRoutes = function (options = {}, requireMocks) {
       push,
       verificationReminders
     );
+  const pushbox = options.pushbox || { deleteAccount: sinon.fake.resolves() };
   return accountRoutes(
     log,
     db,
@@ -112,7 +113,8 @@ const makeRoutes = function (options = {}, requireMocks) {
       removeUser: () => {},
       removePublicAndCanGrantTokens: () => {},
     },
-    options.stripeHelper
+    options.stripeHelper,
+    pushbox
   );
 };
 
@@ -1987,7 +1989,7 @@ describe('/account/login', () => {
   const defaultEmailAccountRecord = mockDB.accountRecord;
 
   beforeEach(() => {
-    Container.set(CapabilityService, sinon.fake);
+    Container.set(CapabilityService, sinon.fake.resolves());
   });
 
   afterEach(() => {
@@ -3678,7 +3680,8 @@ describe('/account/destroy', () => {
     mockPush,
     mockStripeHelper,
     mockPaypalHelper,
-    mockAuthModels;
+    mockAuthModels,
+    mockPushbox;
 
   beforeEach(async () => {
     mockDB = {
@@ -3711,6 +3714,7 @@ describe('/account/destroy', () => {
     mockAuthModels.deleteAllPayPalBAs = sinon.spy(async (uid) => {
       return;
     });
+    mockPushbox = { deleteAccount: sinon.fake.resolves() };
   });
 
   function buildRoute(subscriptionsEnabled = true) {
@@ -3734,6 +3738,7 @@ describe('/account/destroy', () => {
         log: mockLog,
         push: mockPush,
         stripeHelper: mockStripeHelper,
+        pushbox: mockPushbox,
       },
       { 'fxa-shared/db/models/auth': mockAuthModels }
     );
@@ -3779,9 +3784,19 @@ describe('/account/destroy', () => {
         mockAuthModels.deleteAllPayPalBAs,
         uid
       );
+      sinon.assert.calledOnceWithExactly(mockPushbox.deleteAccount, uid);
     });
   });
 
+  it('does not fail if pushbox fails to delete', async () => {
+    mockPushbox = { deleteAccount: sinon.fake.rejects() };
+    try {
+      await runTest(buildRoute(), mockRequest);
+    } catch (err) {
+      assert.fail('no exception should have been thrown');
+    }
+  });
+
   it('should fail if stripeHelper update customer fails', async () => {
     mockStripeHelper.removeCustomer(async (uid, email) => {
       throw new Error('wibble');

packages/fxa-auth-server/test/remote/pushbox/db.ts

@@ -117,4 +117,10 @@ describe('pushbox db', () => {
       await pushboxDb.deleteDevice({ uid: r.uid, deviceId: r.deviceId });
     });
   });
+
+  describe('deleteAccount', () => {
+    it('deletes without error', async () => {
+      await pushboxDb.deleteAccount(r.uid);
+    });
+  });
 });