task(auth): Add more tests around account destroy

Author: dschom

packages/fxa-auth-server/config/index.ts

@@ -1977,6 +1977,14 @@ const convictConf = convict({
       env: 'OTP_SIGNUP_DIGIT',
     },
   },
+  accountDestroy: {
+    requireVerifiedAccount: {
+      doc: 'Whether or not the account must be verified in order to destroy it.',
+      default: true,
+      format: Boolean,
+      env: 'ACCOUNT_DESTROY__REQUIRE_VERIFIED_ACCOUNT',
+    },
+  },
   passwordForgotOtp: {
     digits: {
       doc: 'Number of digits in token',

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

@@ -1857,6 +1857,26 @@ export class AccountHandler {
       throw error.unverifiedSession();
     }
 
+    // We can also check that the email was verified. This proves the account is active and
+    // related to a valid email. Accounts that aren't activated get deleted automatically
+    // by cron jobs anyways...
+    if (
+      this.config.accountDestroy.requireVerifiedAccount &&
+      !accountRecord.emailVerified
+    ) {
+      throw error.unverifiedAccount();
+    }
+
+    // Regardless of whether or not an account has TOTP, we must make sure the user actually
+    // owns the account. This means the sessionToken must be verified.
+    //
+    // The UI will request an OTP code verification before destroying the account in the event
+    // the session is currently unverified. The following check will ensure that this OTP code
+    // was actually provided by the user.
+    if (!sessionToken.tokenVerified) {
+      throw error.unverifiedSession();
+    }
+
     // In other scenarios, fall back to the default behavior and let the user
     // delete the account. If they have a password set, we verify it here. Users
     // that don't have a password set will be able to delete their account without

packages/fxa-auth-server/test/client/api.js

@@ -219,7 +219,6 @@ module.exports = (config) => {
     if (!options) {
       options = { keys: true };
     }
-
     return this.doRequest(
       'POST',
       `${this.baseURL}/account/login${getQueryString(options)}`,

packages/fxa-auth-server/test/remote/account_destroy_tests.js

@@ -7,6 +7,8 @@
 const { assert } = require('chai');
 const TestServer = require('../test_server');
 const Client = require('../client')();
+const otplib = require('otplib');
+const crypto = require('crypto');
 
 const config = require('../../config').default.getProperties();
 
@@ -15,108 +17,264 @@ const config = require('../../config').default.getProperties();
   describe(`#integration${testOptions.version} - remote account destroy`, function () {
     this.timeout(60000);
     let server;
+    let tempConfigValue;
 
     before(async function () {
+      // Important, this config impacts test logic. By default this is enabled
+      // in development/test with a very large max time. In the real world the max time
+      // is much lower, and confirmation is not skipped very often.The test cases in this
+      // suite are looking at edge cases on unconfirmed accounts and sessions. Therefore,
+      // this config will always be disabled here.
+      tempConfigValue = config.signinConfirmation.skipForNewAccounts.enabled;
+      config.signinConfirmation.skipForNewAccounts.enabled = false;
       server = await TestServer.start(config);
     });
 
     after(async function () {
+      config.signinConfirmation.skipForNewAccounts.enabled = tempConfigValue;
       await TestServer.stop(server);
     });
 
-    it('account destroy', () => {
+    // Note that this test case most closely aligns with what most users experience.
+    // In this case we have a user with a verified account, but unverified session
+    // and no totp. When this occurs our UI will send an OTP email to user and prompt
+    // them to enter the code prior to deleting the account.
+    it('can delete account by providing short code', async function () {
       const email = server.uniqueEmail();
-      const password = 'allyourbasearebelongtous';
-      let client = null;
-      return Client.createAndVerify(
+      const password = 'ok';
+      await Client.create(config.publicUrl, email, password, {
+        ...testOptions,
+        verificationMethod: 'email-2fa',
+        keys: true,
+      });
+      const client = await Client.login(config.publicUrl, email, password, {
+        ...testOptions,
+        verificationMethod: 'email-2fa',
+        keys: true,
+      });
+
+      // Send a short code, this will validate the account and the session.
+      // In the UI this happens when a user clicks on delete account, and
+      // OTP message box pops up.
+      await client.resendVerifyShortCodeEmail();
+      const emailData = await server.mailbox.waitForEmail(email);
+      let code;
+      for (let i = 0; i < emailData.length; i++) {
+        if (emailData[i].headers['x-verify-short-code']) {
+          code = emailData[i].headers['x-verify-short-code'];
+        }
+      }
+      assert.isDefined(code);
+      await client.verifyShortCodeEmail(code);
+
+      // Should not throw
+      await client.destroyAccount();
+    });
+
+    it('can delete account by providing verify code', async function () {
+      const email = server.uniqueEmail();
+      const password = 'ok';
+      await Client.create(config.publicUrl, email, password, {
+        ...testOptions,
+        verificationMethod: 'email-2fa',
+        keys: true,
+      });
+      const client = await Client.login(config.publicUrl, email, password, {
+        ...testOptions,
+        verificationMethod: 'email-2fa',
+        keys: true,
+      });
+
+      const emailData = await server.mailbox.waitForEmail(email);
+      const code = emailData[emailData.length - 1].headers['x-verify-code'];
+      await client.verifyEmail(code);
+
+      // Should not throw
+      await client.destroyAccount();
+    });
+
+    it('cannot delete account with invalid authPW', async function () {
+      const email = server.uniqueEmail();
+      const password = 'ok';
+      const c = await Client.createAndVerify(
         config.publicUrl,
         email,
         password,
         server.mailbox,
         testOptions
-      )
-        .then((x) => {
-          client = x;
-          return client.sessionStatus();
-        })
-        .then((status) => {
-          return client.destroyAccount();
-        })
-        .then(() => {
-          return client.keys();
-        })
-        .then(
-          (keys) => {
-            assert(false, 'account not destroyed');
-          },
-          (err) => {
-            assert.equal(err.message, 'Unknown account', 'account destroyed');
-          }
+      );
+
+      c.authPW = Buffer.from(
+        '0000000000000000000000000000000000000000000000000000000000000000',
+        'hex'
+      );
+      c.authPWVersion2 = Buffer.from(
+        '0000000000000000000000000000000000000000000000000000000000000000',
+        'hex'
+      );
+
+      try {
+        await c.destroyAccount();
+        assert.fail(
+          'should not be able to destroy account with invalid password'
         );
+      } catch (err) {
+        assert.equal(err.errno, 103);
+      }
     });
 
-    it('invalid authPW on account destroy', () => {
+    it('cannot delete account without verifying TOTP', async function () {
       const email = server.uniqueEmail();
       const password = 'ok';
-      return Client.createAndVerify(
+
+      await Client.createAndVerifyAndTOTP(
         config.publicUrl,
         email,
         password,
         server.mailbox,
+        {
+          ...testOptions,
+          keys: true,
+        }
+      );
+
+      // Create a new unverified session
+      const client = await Client.login(
+        config.publicUrl,
+        email,
+        password,
+        testOptions
+      );
+      const res = await await client.emailStatus();
+      assert.equal(res.sessionVerified, false, 'session not verified');
+
+      try {
+        await client.destroyAccount();
+        assert.fail(
+          'Should not be able to destroy account without verifying totp'
+        );
+      } catch (err) {
+        assert.equal(err.errno, 138, 'unverified session');
+      }
+    });
+
+    it('cannot delete account with TOTP by supplying email otp code', async function () {
+      const email = server.uniqueEmail();
+      const password = 'ok';
+      await Client.create(config.publicUrl, email, password, {
+        ...testOptions,
+        verificationMethod: 'email-2fa',
+        keys: true,
+      });
+      let client = await Client.login(config.publicUrl, email, password, {
+        ...testOptions,
+        verificationMethod: 'email-2fa',
+        keys: true,
+      });
+
+      // Send a short code, this will validate the account and the session.
+      // In the UI this happens when a user clicks on delete account, and
+      // OTP message box pops up.
+      await client.resendVerifyShortCodeEmail();
+      const emailData = await server.mailbox.waitForEmail(email);
+      let code;
+      for (let i = 0; i < emailData.length; i++) {
+        if (emailData[i].headers['x-verify-short-code']) {
+          code = emailData[i].headers['x-verify-short-code'];
+        }
+      }
+      assert.isDefined(code);
+      await client.verifyShortCodeEmail(code);
+
+      // Add totp to account.
+      client.totpAuthenticator = new otplib.authenticator.Authenticator();
+      const totpTokenResult = await client.createTotpToken();
+      assert.isDefined(totpTokenResult);
+      60;
+      client.totpAuthenticator.options = {
+        secret: totpTokenResult.secret,
+        crypto: crypto,
+      };
+      const totpCode = client.totpAuthenticator.generate();
+      await client.verifyTotpCode(totpCode);
+
+      // Log in again. This creates a new unverified session
+      client = await Client.login(
+        config.publicUrl,
+        email,
+        password,
         testOptions
-      )
-        .then((c) => {
-          c.authPW = Buffer.from(
-            '0000000000000000000000000000000000000000000000000000000000000000',
-            'hex'
-          );
-          c.authPWVersion2 = Buffer.from(
-            '0000000000000000000000000000000000000000000000000000000000000000',
-            'hex'
-          );
-          return c.destroyAccount();
-        })
-        .then(
-          (r) => {
-            assert(false);
-          },
-          (err) => {
-            assert.equal(err.errno, 103);
-          }
+      );
+      const res = await client.emailStatus();
+      assert.equal(res.sessionVerified, false, 'session not verified');
+
+      // Try verifying the the session with a short code. This should
+      // not be enough to bypass 2FA. This sort of mimics a valid email code
+      // being stolen...
+      await client.verifyShortCodeEmail(code);
+      assert.equal(
+        (await client.emailStatus()).sessionVerified,
+        true,
+        'session should be verified'
+      );
+
+      // Destroying the account should not work. Despite the session being 'verified',
+      // totp has not been provided.
+      try {
+        await client.destroyAccount();
+        assert.fail(
+          'Should not be able to destroy account without verifying totp'
         );
+      } catch (error) {
+        assert.equal(error.errno, 138, 'unverified session');
+      }
     });
 
-    it('should fail to delete account with TOTP with unverified session', () => {
+    it('cannot delete without verifying session', async function () {
       const email = server.uniqueEmail();
       const password = 'ok';
-      let client;
-      return Client.createAndVerifyAndTOTP(
+      let client = await Client.createAndVerify(
         config.publicUrl,
         email,
         password,
         server.mailbox,
-        {
-          ...testOptions,
-          keys: true,
-        }
-      )
-        .then(() => {
-          // Create a new unverified session
-          return Client.login(config.publicUrl, email, password, testOptions);
-        })
-        .then((response) => {
-          client = response;
-          return client.emailStatus();
-        })
-        .then((res) =>
-          assert.equal(res.sessionVerified, false, 'session not verified')
-        )
-        .then(() => client.destroyAccount())
-        .then(assert.fail, (err) => {
-          assert.equal(err.errno, 138, 'unverified session');
-        });
-    });
+        testOptions
+      );
+
+      // Login again requiring email-2fa for session verification. The account is now verified but the session is not.
+      client = await Client.login(config.publicUrl, email, password, {
+        ...testOptions,
+        verificationMethod: 'email-2fa',
+      });
 
+      try {
+        await client.destroyAccount();
+        assert.fail('Should not be able allowed to destroy account.');
+      } catch (err) {
+        assert.equal(err.message, 'Unconfirmed session');
+      }
+    });
 
+    it('cannot delete without verifying account', async function () {
+      const email = server.uniqueEmail();
+      const password = 'ok';
+      await Client.create(config.publicUrl, email, password, {
+        ...testOptions,
+        verificationMethod: 'email-2fa',
+        keys: true,
+      });
+      const client = await Client.login(
+        config.publicUrl,
+        email,
+        password,
+        testOptions
+      );
+      try {
+        await client.destroyAccount();
+        assert.fail('Should not be able allowed to destroy account.');
+      } catch (err) {
+        assert.equal(err.message, 'Unconfirmed account');
+      }
+    });
   });
 });