From ad96120af4cb974fe88792c5190a4ea829d392e0 Mon Sep 17 00:00:00 2001 From: Michele Esposito <34438276+mikesposito@users.noreply.github.com> Date: Wed, 29 Nov 2023 15:45:14 +0100 Subject: [PATCH] Use encryptor `isVaultUpdated` (#310) * chore: update browser-passworder * refactor: remove `updateVault` from `GenericEncryptor` --- package.json | 2 +- src/KeyringController.test.ts | 19 ++++--------------- src/KeyringController.ts | 5 ++--- src/test/encryptor.mock.ts | 4 ++++ src/types.ts | 14 +++++++++----- yarn.lock | 4 ++-- 6 files changed, 22 insertions(+), 26 deletions(-) diff --git a/package.json b/package.json index 832b7dca..fa1b2ec9 100644 --- a/package.json +++ b/package.json @@ -44,7 +44,7 @@ }, "dependencies": { "@ethereumjs/tx": "^4.2.0", - "@metamask/browser-passworder": "^4.2.0", + "@metamask/browser-passworder": "^4.3.0", "@metamask/eth-hd-keyring": "^7.0.1", "@metamask/eth-sig-util": "^7.0.0", "@metamask/eth-simple-keyring": "^6.0.1", diff --git a/src/KeyringController.test.ts b/src/KeyringController.test.ts index 667a5f30..fca3a39c 100644 --- a/src/KeyringController.test.ts +++ b/src/KeyringController.test.ts @@ -863,13 +863,11 @@ describe('KeyringController', () => { }); deleteEncryptionKeyAndSalt(keyringController); const initialVault = keyringController.store.getState().vault; - const updatedVaultMock = - '{"vault": "updated_vault_detail", "salt": "salt"}'; const mockEncryptionResult = { data: '0x1234', iv: 'an iv', }; - sinon.stub(mockEncryptor, 'updateVault').resolves(updatedVaultMock); + sinon.stub(mockEncryptor, 'isVaultUpdated').returns(false); sinon .stub(mockEncryptor, 'encryptWithKey') .resolves(mockEncryptionResult); @@ -898,21 +896,12 @@ describe('KeyringController', () => { }, }); const initialVault = keyringController.store.getState().vault; - const updatedVaultMock = - '{"vault": "updated_vault_detail", "salt": "salt"}'; - const mockEncryptionResult = { - data: '0x1234', - iv: 'an iv', - }; - sinon.stub(mockEncryptor, 'updateVault').resolves(updatedVaultMock); - sinon - .stub(mockEncryptor, 'encryptWithKey') - .resolves(mockEncryptionResult); + sinon.stub(mockEncryptor, 'isVaultUpdated').returns(false); await keyringController.unlockKeyrings(PASSWORD); const updatedVault = keyringController.store.getState().vault; - expect(initialVault).not.toBe(updatedVault); + expect(initialVault).toBe(updatedVault); }); }); @@ -929,7 +918,7 @@ describe('KeyringController', () => { const initialVault = keyringController.store.getState().vault; const updatedVaultMock = '{"vault": "updated_vault_detail", "salt": "salt"}'; - sinon.stub(mockEncryptor, 'updateVault').resolves(updatedVaultMock); + sinon.stub(mockEncryptor, 'isVaultUpdated').returns(false); sinon.stub(mockEncryptor, 'encrypt').resolves(updatedVaultMock); await keyringController.unlockKeyrings(PASSWORD); diff --git a/src/KeyringController.ts b/src/KeyringController.ts index 412a67a6..4978feec 100644 --- a/src/KeyringController.ts +++ b/src/KeyringController.ts @@ -931,9 +931,8 @@ class KeyringController extends EventEmitter { if ( this.password && (!this.#cacheEncryptionKey || !encryptionKey) && - this.#encryptor.updateVault && - (await this.#encryptor.updateVault(encryptedVault, this.password)) !== - encryptedVault + this.#encryptor.isVaultUpdated && + !this.#encryptor.isVaultUpdated(encryptedVault) ) { // Re-encrypt the vault with safer method if one is available await this.persistAllKeyrings(); diff --git a/src/test/encryptor.mock.ts b/src/test/encryptor.mock.ts index 0d3a4155..2c97d5ff 100644 --- a/src/test/encryptor.mock.ts +++ b/src/test/encryptor.mock.ts @@ -81,6 +81,10 @@ export class MockEncryptor implements ExportableKeyEncryptor { return _vault; } + isVaultUpdated(_vault: string) { + return true; + } + generateSalt() { return MOCK_ENCRYPTION_SALT; } diff --git a/src/types.ts b/src/types.ts index 67b2385f..4d8feda1 100644 --- a/src/types.ts +++ b/src/types.ts @@ -2,6 +2,7 @@ import type { DetailedDecryptResult, DetailedEncryptionResult, EncryptionResult, + KeyDerivationOptions, } from '@metamask/browser-passworder'; import type { Json, Keyring } from '@metamask/utils'; @@ -63,14 +64,17 @@ export type GenericEncryptor = { */ decrypt: (password: string, encryptedString: string) => Promise; /** - * Optional vault migration helper. Updates the provided vault, re-encrypting - * data with a safer algorithm if one is available. + * Optional vault migration helper. Checks if the provided vault is up to date + * with the desired encryption algorithm. * - * @param vault - The encrypted string to update. - * @param password - The password to decrypt the vault with. + * @param vault - The encrypted string to check. + * @param targetDerivationParams - The desired target derivation params. * @returns The updated encrypted string. */ - updateVault?: (vault: string, password: string) => Promise; + isVaultUpdated?: ( + vault: string, + targetDerivationParams?: KeyDerivationOptions, + ) => boolean; }; /** diff --git a/yarn.lock b/yarn.lock index 3f67aa4a..4f5484bc 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1127,7 +1127,7 @@ __metadata: languageName: node linkType: hard -"@metamask/browser-passworder@npm:^4.2.0": +"@metamask/browser-passworder@npm:^4.3.0": version: 4.3.0 resolution: "@metamask/browser-passworder@npm:4.3.0" dependencies: @@ -1208,7 +1208,7 @@ __metadata: "@lavamoat/allow-scripts": ^2.3.1 "@lavamoat/preinstall-always-fail": ^1.0.0 "@metamask/auto-changelog": ^3.0.0 - "@metamask/browser-passworder": ^4.2.0 + "@metamask/browser-passworder": ^4.3.0 "@metamask/eslint-config": ^12.2.0 "@metamask/eslint-config-jest": ^12.1.0 "@metamask/eslint-config-nodejs": ^12.1.0