Merge pull request #18207 from mozilla/FXA-10945

task(recovery-phone): Add support for confirming code on sign in

Author: dschom

libs/accounts/recovery-phone/src/lib/recovery-phone.factories.ts

@@ -8,7 +8,6 @@ import {
   testAccountDatabaseSetup,
 } from '@fxa/shared/db/mysql/account';
 import { Test } from '@nestjs/testing';
-import { RecoveryPhoneFactory } from './recovery-phone.factories';
 
 describe('RecoveryPhoneManager', () => {
   let recoveryPhoneManager: RecoveryPhoneManager;
@@ -57,7 +56,7 @@ describe('RecoveryPhoneManager', () => {
           useValue: db,
         },
         {
-          provide: 'Redis',
+          provide: 'RecoveryPhoneRedis',
           useValue: mockRedis,
         },
       ],

libs/accounts/recovery-phone/src/lib/recovery-phone.manager.in.spec.ts

@@ -82,7 +82,9 @@ export class RecoveryPhoneManager {
    *
    * @param uid
    */
-  async getConfirmedPhoneNumber(uid: string): Promise<{ phoneNumber: string }> {
+  async getConfirmedPhoneNumber(
+    uid: string
+  ): Promise<{ uid: Buffer; phoneNumber: string }> {
     const uidBuffer = Buffer.from(uid, 'hex');
     const result = await getConfirmedPhoneNumber(this.db, uidBuffer);
     if (!result) {
@@ -163,6 +165,19 @@ export class RecoveryPhoneManager {
     return JSON.parse(data);
   }
 
+  /**
+   * Removes a code from redis. Once a code is validated, it's good to proactively remove it from the database
+   * so it cannot be used again.
+   * @param uid The user's unique identifier
+   * @param code The SMS code associated with this user
+   * @returns
+   */
+  async removeCode(uid: string, code: string) {
+    const redisKey = `${this.redisPrefix}:${uid}:${code}`;
+    const count = await this.redisClient.del(redisKey);
+    return count > 0;
+  }
+
   /**
    * Check if a user has recovery codes. Recovery codes are required
    * to set up a recovery phone.

libs/accounts/recovery-phone/src/lib/recovery-phone.manager.ts

@@ -29,6 +29,7 @@ describe('RecoveryPhoneService', () => {
     removePhoneNumber: jest.fn(),
     getConfirmedPhoneNumber: jest.fn(),
     hasRecoveryCodes: jest.fn(),
+    removeCode: jest.fn(),
   };
   const mockOtpManager = { generateCode: jest.fn() };
   const mockRecoveryPhoneConfig = {
@@ -157,28 +158,28 @@ describe('RecoveryPhoneService', () => {
     });
   });
 
-  describe('confirm code', () => {
-    it('can confirm valid sms code', async () => {
-      mockRecoveryPhoneManager.getUnconfirmed.mockReturnValue({});
-
-      const result = await service.confirmCode(uid, code);
-
-      expect(result).toBeTruthy();
-      expect(mockRecoveryPhoneManager.getUnconfirmed).toBeCalledWith(uid, code);
-    });
-
+  describe('confirm setup code', () => {
     it('can confirm valid sms code used for setup', async () => {
       mockRecoveryPhoneManager.getUnconfirmed.mockReturnValue({
         isSetup: true,
       });
       mockRecoveryPhoneManager.registerPhoneNumber.mockReturnValue(true);
 
-      const result = await service.confirmCode(uid, code);
+      const result = await service.confirmSetupCode(uid, code);
 
       expect(result).toBeTruthy();
       expect(mockRecoveryPhoneManager.getUnconfirmed).toBeCalledWith(uid, code);
     });
 
+    it('will not confirm a valid sms code for signin', async () => {
+      mockRecoveryPhoneManager.getUnconfirmed.mockReturnValue({});
+
+      const result = await service.confirmSetupCode(uid, code);
+
+      expect(result).toBeFalsy();
+      expect(mockRecoveryPhoneManager.getUnconfirmed).toBeCalledWith(uid, code);
+    });
+
     it('can confirm valid sms code used for setup', async () => {
       mockRecoveryPhoneManager.getUnconfirmed.mockResolvedValue({
         isSetup: true,
@@ -189,7 +190,7 @@ describe('RecoveryPhoneService', () => {
         phoneNumber: '+15005550000',
       });
 
-      const result = await service.confirmCode(uid, code);
+      const result = await service.confirmSetupCode(uid, code);
 
       expect(result).toEqual(true);
       expect(mockRecoveryPhoneManager.getUnconfirmed).toBeCalledWith(uid, code);
@@ -203,23 +204,52 @@ describe('RecoveryPhoneService', () => {
     it('can indicate invalid sms code', async () => {
       mockRecoveryPhoneManager.getUnconfirmed.mockReturnValue(null);
 
-      const result = await service.confirmCode(uid, code);
+      const result = await service.confirmSetupCode(uid, code);
 
       expect(result).toEqual(false);
       expect(mockRecoveryPhoneManager.getUnconfirmed).toBeCalledWith(uid, code);
     });
 
     it('throws library error while confirming sms code', () => {
       mockRecoveryPhoneManager.getUnconfirmed.mockRejectedValueOnce(mockError);
-      expect(service.confirmCode(uid, code)).rejects.toEqual(mockError);
+      expect(service.confirmSetupCode(uid, code)).rejects.toEqual(mockError);
     });
 
     it('throws library error while registering phone number for sms code', () => {
       mockRecoveryPhoneManager.getUnconfirmed.mockResolvedValue({
         isSetup: true,
       });
       mockRecoveryPhoneManager.registerPhoneNumber.mockRejectedValue(mockError);
-      expect(service.confirmCode(uid, code)).rejects.toEqual(mockError);
+      expect(service.confirmSetupCode(uid, code)).rejects.toEqual(mockError);
+    });
+  });
+
+  describe('confirm signin code', () => {
+    it('can confirm valid sms code', async () => {
+      mockRecoveryPhoneManager.getUnconfirmed.mockReturnValue({});
+
+      const result = await service.confirmSigninCode(uid, code);
+
+      expect(result).toBeTruthy();
+      expect(mockRecoveryPhoneManager.getUnconfirmed).toBeCalledWith(uid, code);
+    });
+
+    it('will not confirm valid sms code used for setup', async () => {
+      mockRecoveryPhoneManager.getUnconfirmed.mockReturnValue({
+        isSetup: true,
+      });
+
+      const result = await service.confirmSigninCode(uid, code);
+
+      expect(result).toBeFalsy();
+    });
+
+    it('will not confirm unknown sms code used', async () => {
+      mockRecoveryPhoneManager.getUnconfirmed.mockReturnValue(null);
+
+      const result = await service.confirmSigninCode(uid, code);
+
+      expect(result).toBeFalsy();
     });
   });
 

libs/accounts/recovery-phone/src/lib/recovery-phone.service.spec.ts

@@ -91,27 +91,52 @@ export class RecoveryPhoneService {
    * @param code A otp code
    * @returns True if successful
    */
-  public async confirmCode(uid: string, code: string) {
+  public async confirmSetupCode(uid: string, code: string) {
     const data = await this.recoveryPhoneManager.getUnconfirmed(uid, code);
 
     // If there is no data, it means there's no record of this code being sent to the uid provided
     if (data == null) {
       return false;
     }
 
+    // The code must be intended for a setup, ie recovery phone create, action.
+    if (data.isSetup !== true) {
+      return false;
+    }
+
     // If this was for a setup operation. Register the phone number to the uid.
+    const lookupData = await this.smsManager.phoneNumberLookup(
+      data.phoneNumber
+    );
+    await this.recoveryPhoneManager.registerPhoneNumber(
+      uid,
+      data.phoneNumber,
+      lookupData
+    );
+
+    // The code was valid. Remove entry. It cannot be used again.
+    await this.recoveryPhoneManager.removeCode(uid, code);
+
+    // There was a record matching, the uid / code. The confirmation was successful.
+    return true;
+  }
+
+  public async confirmSigninCode(uid: string, code: string) {
+    const data = await this.recoveryPhoneManager.getUnconfirmed(uid, code);
+
+    // If there is no data, it means there's no record of this code being sent to the uid provided
+    if (data == null) {
+      return false;
+    }
+
+    // A code intended for setup, cannot be used for sign in.
     if (data.isSetup === true) {
-      const lookupData = await this.smsManager.phoneNumberLookup(
-        data.phoneNumber
-      );
-      await this.recoveryPhoneManager.registerPhoneNumber(
-        uid,
-        data.phoneNumber,
-        lookupData
-      );
+      return false;
     }
 
-    // There was a record matching, the uid / code. The confirmation was successful.
+    // The code was valid. Remove entry. It cannot be used again.
+    await this.recoveryPhoneManager.removeCode(uid, code);
+
     return true;
   }
 

libs/accounts/recovery-phone/src/lib/recovery-phone.service.ts

@@ -3,3 +3,4 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 export * from './lib/account.manager';
 export * from './lib/account.error';
+export { VerificationMethods } from './lib/account.repository';

libs/shared/account/account/src/index.ts

@@ -57,5 +57,13 @@ describe('accountManager', () => {
         accountManager.createAccountStub(email, 1, 'en-US')
       ).rejects.toBeInstanceOf(AccountAlreadyExistsError);
     });
+
+    // TODO: Setup tests for verify session
+    // it.skip('should mark session valid', async () => {
+    //   // TODO: Create an account
+    //   //       Create a session with unverified session
+    //   //       Call verifySession
+    //   //       Validate that session is verified, and unverified session no longer exists.
+    // });
   });
 });

libs/shared/account/account/src/lib/account.manager.in.spec.ts

@@ -7,7 +7,12 @@ import { Inject, Injectable } from '@nestjs/common';
 import type { AccountDatabase } from '@fxa/shared/db/mysql/account';
 import { AccountDbProvider } from '@fxa/shared/db/mysql/account';
 
-import { createAccount, getAccounts } from './account.repository';
+import {
+  createAccount,
+  getAccounts,
+  verifyAccountSession,
+  VerificationMethods,
+} from './account.repository';
 import { normalizeEmail, randomBytesAsync } from './account.util';
 import { uuidTransformer } from '@fxa/shared/db/mysql/core';
 
@@ -49,4 +54,17 @@ export class AccountManager {
     const bufferUids = uids.map((uid) => uuidTransformer.to(uid));
     return getAccounts(this.db, bufferUids);
   }
+
+  async verifySession(
+    uid: string,
+    sessionTokenId: string,
+    verificationMethod: VerificationMethods
+  ) {
+    return verifyAccountSession(
+      this.db,
+      uuidTransformer.to(uid),
+      uuidTransformer.to(sessionTokenId),
+      verificationMethod
+    );
+  }
 }

libs/shared/account/account/src/lib/account.manager.ts

@@ -72,3 +72,83 @@ export function getAccounts(db: AccountDatabase, uids: Buffer[]) {
     .where('uid', 'in', uids)
     .execute();
 }
+
+/** See session_token.js in auth server for master list. */
+export enum VerificationMethods {
+  email = 0,
+  email2fa = 1,
+  totp2fa = 2,
+  recoveryCode = 3,
+  sms2fa = 4,
+}
+
+/**
+ * Marks account session as verified
+ * @param db Database instance
+ * @param uid Users id
+ * @param sessionTokenId User's session id
+ * @param verificationMethod, See VerificationMethods
+ */
+export async function verifyAccountSession(
+  db: AccountDatabase,
+  uid: Buffer,
+  sessionTokenId: Buffer,
+  verificationMethod: VerificationMethods
+): Promise<boolean> {
+  // It appears that Date.now() results in the number 'format' as UNIX_TIMESTAMP(NOW(3)) * 1000 used
+  // by the stored procedure.
+  const now = Date.now();
+
+  // Ported from session-token.ts -> verify
+
+  return await db.transaction().execute(async (trx) => {
+    await trx
+      .updateTable('accounts')
+      .set({
+        profileChangedAt: now,
+      })
+      .where('uid', '=', uid)
+      .executeTakeFirstOrThrow();
+
+    // Equivalent of 'verifyTokensWithMethod_3' sproc
+    await trx
+      .updateTable('sessionTokens')
+      .set({
+        verifiedAt: now,
+        verificationMethod: verificationMethod,
+      })
+      .where('tokenId', '=', sessionTokenId)
+      .executeTakeFirstOrThrow();
+
+    // next locate corresponding unverified session tokens
+    const token = await trx
+      .selectFrom('sessionTokens')
+      .innerJoin(
+        'unverifiedTokens',
+        'unverifiedTokens.tokenId',
+        'sessionTokens.tokenId'
+      )
+      .select(['unverifiedTokens.tokenVerificationId as tokenVerificationId'])
+      .where('sessionTokens.tokenId', '=', sessionTokenId)
+      .executeTakeFirst();
+
+    if (token) {
+      // next mark token as verified. Equivalent to 'verifyToken_3' sproc
+      await trx
+        .updateTable('securityEvents')
+        .set({
+          verified: 1,
+        })
+        .where('uid', '=', uid)
+        .where('tokenVerificationId', '=', token.tokenVerificationId)
+        .executeTakeFirstOrThrow();
+      await trx
+        .deleteFrom('unverifiedTokens')
+        .where('uid', '=', uid)
+        .where('tokenVerificationId', '=', token.tokenVerificationId)
+        .executeTakeFirstOrThrow();
+    }
+
+    return true;
+  });
+}