diff --git a/packages/seedless-onboarding-controller/CHANGELOG.md b/packages/seedless-onboarding-controller/CHANGELOG.md index 9074903a3ee..2dcff999ef6 100644 --- a/packages/seedless-onboarding-controller/CHANGELOG.md +++ b/packages/seedless-onboarding-controller/CHANGELOG.md @@ -12,6 +12,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Bump `@metamask/utils` from `^11.12.0` to `^12.0.0` ([#10192](https://github.com/MetaMask/core/pull/10192)) - Bump `@metamask/keyring-controller` from `^28.0.0` to `^28.1.0` ([#10418](https://github.com/MetaMask/core/pull/10418)) +### Fixed + +- Prevent initial TOPRF seed phrase backup retries from skipping the remote metadata write after a partial failure ([#10419](https://github.com/MetaMask/core/pull/10419)) +- Reject duplicate TOPRF key and seed phrase backup attempts when a vault already exists ([#10419](https://github.com/MetaMask/core/pull/10419)) + ## [11.0.0] ### Changed diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts index 3ff1b8651a8..63c3aad5c7d 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -1402,6 +1402,105 @@ describe('SeedlessOnboardingController', () => { ); }); + it('should retry the remote backup after vault creation fails', async () => { + await withController( + { + state: getMockInitialControllerState({ + withMockAuthenticatedUser: true, + }), + }, + async ({ controller, toprfClient, encryptor, baseMessenger }) => { + const { createLocalKeySpy } = mockcreateLocalKey( + toprfClient, + MOCK_PASSWORD, + ); + const retryToprfEncryptor = createMockToprfEncryptor(); + createLocalKeySpy.mockResolvedValueOnce({ + encKey: retryToprfEncryptor.deriveEncKey('retry-password'), + pwEncKey: retryToprfEncryptor.derivePwEncKey('retry-password'), + authKeyPair: + retryToprfEncryptor.deriveAuthKeyPair('retry-password'), + oprfKey: BigInt(1), + seed: stringToBytes('retry-password'), + }); + + const persistLocalKeySpy = jest + .spyOn(toprfClient, 'persistLocalKey') + .mockResolvedValue(); + + // The first attempt writes the remote metadata, then fails while + // creating the vault. The retry must therefore write the metadata + // again using the newly generated TOPRF key. + const firstMockSecretDataAdd = handleMockSecretDataAdd(); + const retryMockSecretDataAdd = handleMockSecretDataAdd(); + jest + .spyOn(encryptor, 'encryptWithDetail') + .mockRejectedValueOnce(new Error('Failed to create vault')); + + await expect( + baseMessenger.call( + 'SeedlessOnboardingController:createToprfKeyAndBackupSeedPhrase', + MOCK_PASSWORD, + MOCK_SEED_PHRASE, + MOCK_KEYRING_ID, + ), + ).rejects.toThrow('Failed to create vault'); + + expect(firstMockSecretDataAdd.isDone()).toBe(true); + expect(controller.state.socialBackupsMetadata).toStrictEqual([]); + + await baseMessenger.call( + 'SeedlessOnboardingController:createToprfKeyAndBackupSeedPhrase', + MOCK_PASSWORD, + MOCK_SEED_PHRASE, + MOCK_KEYRING_ID, + ); + + expect(retryMockSecretDataAdd.isDone()).toBe(true); + expect(createLocalKeySpy).toHaveBeenCalledTimes(2); + expect(persistLocalKeySpy).toHaveBeenCalledTimes(2); + expect(controller.state.socialBackupsMetadata).toStrictEqual([ + { + type: SecretType.Mnemonic, + keyringId: MOCK_KEYRING_ID, + hash: keccak256AndHexify(MOCK_SEED_PHRASE), + }, + ]); + }, + ); + }); + + it('should throw if a vault already exists', async () => { + await withController( + { + state: getMockInitialControllerState({ + withMockAuthenticatedUser: true, + }), + }, + async ({ controller, toprfClient, baseMessenger }) => { + await mockCreateToprfKeyAndBackupSeedPhrase( + toprfClient, + controller, + baseMessenger, + MOCK_PASSWORD, + MOCK_SEED_PHRASE, + MOCK_KEYRING_ID, + ); + + await expect( + baseMessenger.call( + 'SeedlessOnboardingController:createToprfKeyAndBackupSeedPhrase', + MOCK_PASSWORD, + MOCK_SEED_PHRASE, + MOCK_KEYRING_ID, + ), + ).rejects.toThrow( + SeedlessOnboardingControllerErrorMessage.VaultAlreadyExists, + ); + }, + ); + }); + it('should store accessToken in the vault during backup creation', async () => { await withController( { @@ -1470,6 +1569,7 @@ describe('SeedlessOnboardingController', () => { .mockResolvedValueOnce(); // encrypt and store the secret data const mockSecretDataAdd = handleMockSecretDataAdd(); + const retryMockSecretDataAdd = handleMockSecretDataAdd(); const authenticateSpy = jest .spyOn(toprfClient, 'authenticate') @@ -1486,6 +1586,7 @@ describe('SeedlessOnboardingController', () => { ); expect(mockSecretDataAdd.isDone()).toBe(true); + expect(retryMockSecretDataAdd.isDone()).toBe(true); // should call persistLocalKey twice and authenticate once expect(persistLocalKeySpy).toHaveBeenCalledTimes(2); // should call persistLocalKey twice for the first fail attempt due to invalid auth token error and the second attempt succeeds @@ -1982,6 +2083,16 @@ describe('SeedlessOnboardingController', () => { }, ]); + // do not add a duplicate backup + await baseMessenger.call( + 'SeedlessOnboardingController:addNewSecretData', + NEW_KEY_RING_1.seedPhrase, + EncAccountDataType.ImportedSrp, + { + keyringId: NEW_KEY_RING_1.id, + }, + ); + // add another seed phrase backup const mockSecretDataAdd2 = handleMockSecretDataAdd(); await baseMessenger.call( @@ -3939,7 +4050,6 @@ describe('SeedlessOnboardingController', () => { describe('changePassword', () => { const MOCK_PASSWORD = 'mock-password'; const NEW_MOCK_PASSWORD = 'new-mock-password'; - const MOCK_VAULT = JSON.stringify({ foo: 'bar' }); it('should be able to update new password', async () => { await withController( @@ -4116,7 +4226,6 @@ describe('SeedlessOnboardingController', () => { await withController( { state: getMockInitialControllerState({ - vault: MOCK_VAULT, authPubKey: MOCK_AUTH_PUB_KEY_OUTDATED, withMockAuthenticatedUser: true, }), @@ -5039,19 +5148,9 @@ describe('SeedlessOnboardingController', () => { state: getMockInitialControllerState({ withMockAuthenticatedUser: true, withMockAuthPubKey: true, - vault: 'mock-vault', }), }, async ({ controller, toprfClient, baseMessenger }) => { - await expect( - baseMessenger.call( - 'SeedlessOnboardingController:storeKeyringEncryptionKey', - '', - ), - ).rejects.toThrow( - SeedlessOnboardingControllerErrorMessage.WrongPasswordType, - ); - // Setup and store keyring encryption key. await mockCreateToprfKeyAndBackupSeedPhrase( toprfClient, @@ -6831,7 +6930,7 @@ describe('SeedlessOnboardingController', () => { ); expect(mockRefreshJWTToken).toHaveBeenCalled(); - expect(addSecretDataItemSpy).toHaveBeenCalledTimes(1); + expect(addSecretDataItemSpy).toHaveBeenCalledTimes(2); expect(authenticateSpy).toHaveBeenCalled(); // should call persistLocalKey twice, once for the first call and another from the refresh token expect(persistLocalKeySpy).toHaveBeenCalledTimes(2); diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts index ba24723ff4e..18947189270 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts @@ -646,6 +646,12 @@ export class SeedlessOnboardingController< // assert that the user is authenticated before creating the TOPRF key and backing up the seed phrase this.#assertIsAuthenticatedUser(this.state); + if (this.state.vault) { + throw new Error( + SeedlessOnboardingControllerErrorMessage.VaultAlreadyExists, + ); + } + // locally evaluate the encryption key from the password const { encKey, pwEncKey, authKeyPair, oprfKey } = await this.toprfClient.createLocalKey({ @@ -653,7 +659,7 @@ export class SeedlessOnboardingController< }); const performKeyCreationAndBackup = async (): Promise => { // encrypt and store the secret data - await this.#encryptAndStoreSecretData({ + const secretMetadata = await this.#encryptAndStoreSecretData({ data: seedPhrase, dataType: EncAccountDataType.PrimarySrp, encKey, @@ -661,6 +667,8 @@ export class SeedlessOnboardingController< options: { keyringId, }, + // Persist local backup state only after the full operation succeeds. + skipStatePersist: true, }); // store/persist the encryption key shares @@ -678,6 +686,13 @@ export class SeedlessOnboardingController< // Mark migration as complete since this new backup was created with the new data format (dataType) this.#setMigrationVersion(SeedlessOnboardingMigrationVersion.V1); + + // Persist local backup state after all operations succeed. + this.#filterDupesAndUpdateSocialBackupsMetadata({ + keyringId, + data: seedPhrase, + type: secretMetadata.type, + }); }; await this.#executeWithTokenRefresh( @@ -1744,8 +1759,10 @@ export class SeedlessOnboardingController< * @param params.authKeyPair - The authentication key pair to store. * @param params.options - Optional options object. * @param params.options.keyringId - The keyring id of the backup keyring (required for SRP types). + * @param params.skipStatePersist - Whether to persist the backup state after + * the remote metadata is stored. When true, the backup metadata is not persisted to the controller state. * - * @returns A promise that resolves to the success of the operation. + * @returns A promise that resolves to the backup metadata. */ async #encryptAndStoreSecretData(params: { data: Uint8Array; @@ -1755,8 +1772,16 @@ export class SeedlessOnboardingController< options?: { keyringId?: string; }; - }): Promise { - const { options, data, encKey, authKeyPair, dataType } = params; + skipStatePersist?: boolean; + }): Promise { + const { + options, + data, + encKey, + authKeyPair, + dataType, + skipStatePersist = false, + } = params; const secretMetadata = new SecretMetadata(data, { dataType }); const { type } = secretMetadata; @@ -1764,7 +1789,7 @@ export class SeedlessOnboardingController< // before encrypting and create backup, we will check the state if the secret data is already backed up const backupState = this.getSecretDataBackupState(data, type); if (backupState) { - return; + return secretMetadata; } const secretData = secretMetadata.toBytes(); @@ -1777,7 +1802,9 @@ export class SeedlessOnboardingController< } try { - await this.#withPersistedSecretMetadataBackupsState(async () => { + const persistMetadataFn = async (): Promise< + Omit & { data: Uint8Array } + > => { await this.toprfClient.addSecretDataItem({ encKey, secretData, @@ -1789,7 +1816,16 @@ export class SeedlessOnboardingController< data, type, }; - }); + }; + + // Skip local persistence if `skipStatePersist` is true. + if (skipStatePersist) { + await persistMetadataFn(); + } else { + await this.#withPersistedSecretMetadataBackupsState(persistMetadataFn); + } + + return secretMetadata; } catch (error) { if (this.#isAuthTokenError(error)) { throw error; diff --git a/packages/seedless-onboarding-controller/src/constants.ts b/packages/seedless-onboarding-controller/src/constants.ts index e89287ef7cd..5c0b6d34308 100644 --- a/packages/seedless-onboarding-controller/src/constants.ts +++ b/packages/seedless-onboarding-controller/src/constants.ts @@ -44,6 +44,7 @@ export enum SeedlessOnboardingControllerErrorMessage { InvalidVaultData = `${controllerName} - Invalid vault data`, VaultDataError = `${controllerName} - The decrypted vault has an unexpected shape.`, VaultError = `${controllerName} - Cannot unlock without a previous vault.`, + VaultAlreadyExists = `${controllerName} - Vault already exists.`, InvalidSecretMetadata = `${controllerName} - Invalid secret metadata`, MissingKeyringId = `${controllerName} - Keyring ID is required to store SRP backups.`, FailedToEncryptAndStoreSecretData = `${controllerName} - Failed to encrypt and store secret data`,