Merge pull request #18326 from mozilla/fix-broken-destroy-tests

Fix broken destroy tests

Author: vbudhram

packages/functional-tests/lib/testAccountTracker.ts

@@ -206,6 +206,18 @@ export class TestAccountTracker {
         account.email,
         account.password
       );
+
+      await this.target.authClient.sessionResendVerifyCode(
+        credentials.sessionToken
+      );
+      const code = await this.target.emailClient.getVerifyLoginCode(
+        account.email
+      );
+      await this.target.authClient.sessionVerifyCode(
+        credentials.sessionToken,
+        code
+      );
+
       await this.target.authClient.accountDestroy(
         account.email,
         account.password,

packages/functional-tests/tests/oauth/oauthPermissionsSignup.spec.ts

@@ -7,7 +7,8 @@ import { expect, test } from '../../lib/fixtures/standard';
 test.describe('severity-1 #smoke', () => {
   test.describe('oauth permissions for trusted reliers - sign up', () => {
     test('signup without `prompt=consent`', async ({
-      pages: { page, signup, relier },
+      target,
+      pages: { page, signup, relier, confirmSignupCode },
       testAccountTracker,
     }) => {
       const { email, password } = testAccountTracker.generateAccountDetails();
@@ -19,12 +20,16 @@ test.describe('severity-1 #smoke', () => {
 
       //no permissions asked for, straight to confirm
       await expect(page).toHaveURL(/confirm_signup_code/);
+
+      // Provide the code so the account can be cleaned up.
+      const code = await target.emailClient.getVerifyShortCode(email);
+      await confirmSignupCode.fillOutCodeForm(code);
     });
 
     test('signup with `prompt=consent`', async ({
       target,
       page,
-      pages: { configPage, signup, relier },
+      pages: { configPage, signup, relier, confirmSignupCode },
       testAccountTracker,
     }) => {
       const config = await configPage.getConfig();
@@ -48,6 +53,10 @@ test.describe('severity-1 #smoke', () => {
 
       //Verify sign up code header
       await expect(page).toHaveURL(/confirm_signup_code/);
+
+      // Provide the code so the account can be cleaned up.
+      const code = await target.emailClient.getVerifyShortCode(email);
+      await confirmSignupCode.fillOutCodeForm(code);
     });
   });
 });

packages/functional-tests/tests/oauth/signin.spec.ts

@@ -138,8 +138,9 @@ test.describe('severity-1 #smoke', () => {
     });
 
     test('oauth endpoint chooses the right auth flows', async ({
-      pages: { page, signin, signup, relier },
+      pages: { page, signin, signup, relier, confirmSignupCode },
       testAccountTracker,
+      target,
     }) => {
       // Create unverified account
       const { email, password } = testAccountTracker.generateAccountDetails();
@@ -155,6 +156,11 @@ test.describe('severity-1 #smoke', () => {
       await relier.goto();
       await relier.clickChooseFlow();
       await expect(signin.cachedSigninHeading).toBeVisible();
+
+      await signin.signInButton.click();
+      await expect(page).toHaveURL(/confirm_signup_code/);
+      const code = await target.emailClient.getVerifyShortCode(email);
+      await confirmSignupCode.fillOutCodeForm(code);
     });
   });
 });

packages/functional-tests/tests/react-conversion/oauthSignin.spec.ts

@@ -182,7 +182,6 @@ test.describe('severity-1 #smoke', () => {
 
       // Verify email and ensure user is redirected to relier
       await expect(page).toHaveURL(/confirm_signup_code/);
-      await expect(page).toHaveURL(/confirm_signup_code/);
       const code = await target.emailClient.getVerifyShortCode(email);
       await confirmSignupCode.fillOutCodeForm(code);
 
@@ -227,6 +226,9 @@ test.describe('severity-1 #smoke', () => {
 
       await expect(signin.cachedSigninHeading).toBeVisible();
       await expect(page.getByText(email)).toBeVisible();
+
+      await signin.signInButton.click();
+      expect(await relier.isLoggedIn()).toBe(true);
     });
   });
 });

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

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

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

@@ -1857,6 +1857,29 @@ 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 (
+      this.config.accountDestroy.requireVerifiedSession &&
+      !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/lib/routes/session.js

@@ -421,6 +421,7 @@ module.exports = function (
       },
       handler: async function (request) {
         log.begin('Session.resend_code', request);
+        console.log('!!! resend code');
         const sessionToken = request.auth.credentials;
 
         request.emitMetricsEvent('session.resend_code');
@@ -471,12 +472,14 @@ module.exports = function (
         if (account.primaryEmail.isVerified) {
           // Unverified emails mean that the user is attempting to resend the code from signup page,
           // therefore they get sent a different email template with the code.
+          console.log('!!! sendVerifyLoginCodeEmail', account.email);
           await mailer.sendVerifyLoginCodeEmail(
             account.emails,
             account,
             options
           );
         } else {
+          console.log('!!! sendVerifyShortCodeEmail');
           await mailer.sendVerifyShortCodeEmail([], account, options);
         }
 

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/local/routes/account.js

@@ -3822,6 +3822,7 @@ describe('/account/keys', () => {
 
 describe('/account/destroy', () => {
   const email = '[email protected]';
+  const tokenVerified = true;
   const uid = uuid.v4({}, Buffer.alloc(16)).toString('hex');
 
   let mockDB, mockLog, mockRequest, mockPush, mockPushbox;
@@ -3832,6 +3833,7 @@ describe('/account/destroy', () => {
     };
     mockLog = mocks.mockLog();
     mockRequest = mocks.mockRequest({
+      credentials: { uid, email, tokenVerified },
       log: mockLog,
       payload: {
         email: email,
@@ -3856,6 +3858,9 @@ describe('/account/destroy', () => {
             enabled: true,
           },
         },
+        accountDestroy: {
+          requireVerifiedAccount: false,
+        },
         domain: 'wibble',
       },
       db: mockDB,
@@ -3912,6 +3917,7 @@ describe('/account/destroy', () => {
   it('should delete the passwordless account', () => {
     mockDB = { ...mocks.mockDB({ email, uid, verifierSetAt: 0 }) };
     mockRequest = mocks.mockRequest({
+      credentials: { uid, email, tokenVerified },
       log: mockLog,
       payload: {
         email: email,

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

@@ -161,10 +161,12 @@ const {
         .then(() => {
           return server.mailbox.waitForEmail(email);
         })
-        .then((emailData) => {
+        .then(async (emailData) => {
           assert.include(emailData.text, 'Confirm account', 'en-US');
           // TODO: reinstate after translations catch up
           //assert.notInclude(emailData.text, 'Ativar agora', 'not pt-BR');
+          const code = emailData.headers['x-verify-code'];
+          await client.verifyEmail(code, {});
           return client.destroyAccount();
         })
         .then(() => {
@@ -179,10 +181,12 @@ const {
         .then(() => {
           return server.mailbox.waitForEmail(email);
         })
-        .then((emailData) => {
+        .then(async (emailData) => {
           assert.notInclude(emailData.text, 'Confirm email', 'not en-US');
           // TODO: reinstate after translations catch up
           //assert.include(emailData.text, 'Ativar agora', 'is pt-BR');
+          const code = emailData.headers['x-verify-code'];
+          await client.verifyEmail(code, {});
           return client.destroyAccount();
         });
     });
@@ -994,8 +998,6 @@ const {
       assert.equal(kB2, originalKb);
     });
 
-
-
     async function login(email, password, version = '') {
       return await Client.login(config.publicUrl, email, password, {
         ...testOptions,