From 5bb5ea231cf018f314096423a1dee44cbbbe7a49 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 25 Jan 2024 17:00:51 +0100 Subject: [PATCH 1/3] test: add test cases --- .../src/KeyringController.test.ts | 261 ++++++++++++++++++ 1 file changed, 261 insertions(+) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 9b9be3fa31..db2a224e30 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -78,6 +78,38 @@ describe('KeyringController', () => { sinon.restore(); }); + describe('constructor', () => { + it('should use the default encryptor if none is provided', async () => { + expect( + () => + new KeyringController({ + messenger: buildKeyringControllerMessenger(), + cacheEncryptionKey: true, + updateIdentities: jest.fn(), + setAccountLabel: jest.fn(), + syncIdentities: jest.fn(), + setSelectedAddress: jest.fn(), + }), + ).not.toThrow(); + }); + + it('should throw error if cacheEncryptionKey is true and encryptor does not support key export', () => { + expect( + () => + // @ts-expect-error testing an invalid encryptor + new KeyringController({ + messenger: buildKeyringControllerMessenger(), + cacheEncryptionKey: true, + encryptor: { encrypt: jest.fn(), decrypt: jest.fn() }, + updateIdentities: jest.fn(), + setAccountLabel: jest.fn(), + syncIdentities: jest.fn(), + setSelectedAddress: jest.fn(), + }), + ).toThrow(KeyringControllerError.UnsupportedEncryptionKeyExport); + }); + }); + describe('addNewAccount', () => { describe('when accountCount is not provided', () => { it('should add new account', async () => { @@ -459,6 +491,24 @@ describe('KeyringController', () => { expect(controller.state.vault).toBeDefined(); }); }); + + it('should throw error if password is of wrong type', async () => { + const controller = new KeyringController({ + messenger: buildKeyringControllerMessenger(), + encryptor: new MockEncryptor(), + updateIdentities: jest.fn(), + setAccountLabel: jest.fn(), + syncIdentities: jest.fn(), + setSelectedAddress: jest.fn(), + }); + + await expect( + controller.createNewVaultAndKeychain( + // @ts-expect-error invalid password + 123, + ), + ).rejects.toThrow(KeyringControllerError.WrongPasswordType); + }); }); describe('when there is an existing vault', () => { @@ -784,6 +834,25 @@ describe('KeyringController', () => { ); }); }); + + it('should throw an error if there are no keyrings', async () => { + const controller = new KeyringController({ + messenger: buildKeyringControllerMessenger(), + encryptor: new MockEncryptor(), + updateIdentities: jest.fn(), + setAccountLabel: jest.fn(), + syncIdentities: jest.fn(), + setSelectedAddress: jest.fn(), + }); + + await expect( + controller.getKeyringForAccount( + '0x51253087e6f8358b5f10c0a94315d69db3357859', + ), + ).rejects.toThrow( + 'KeyringController - No keyring found. Error info: There are no keyrings', + ); + }); }); }); @@ -826,6 +895,16 @@ describe('KeyringController', () => { expect(controller.state.keyrings[0].accounts[1]).toBe(addedAccount); }); }); + + it('should throw error when locked', async () => { + await withController(async ({ controller }) => { + await controller.setLocked(); + + await expect(controller.persistAllKeyrings()).rejects.toThrow( + KeyringControllerError.MissingCredentials, + ); + }); + }); }); describe('importAccountWithStrategy', () => { @@ -936,6 +1015,23 @@ describe('KeyringController', () => { expect(preferences.setSelectedAddress.called).toBe(false); }); }); + + it('should throw error when importing a duplicate account', async () => { + await withController(async ({ controller }) => { + const somePassword = 'holachao123'; + await controller.importAccountWithStrategy( + AccountImportStrategy.json, + [input, somePassword], + ); + + await expect( + controller.importAccountWithStrategy(AccountImportStrategy.json, [ + input, + somePassword, + ]), + ).rejects.toThrow(KeyringControllerError.DuplicatedAccount); + }); + }); }); describe('when wrong data is provided', () => { @@ -1824,6 +1920,72 @@ describe('KeyringController', () => { ); }); + it('should unlock also with unsupported keyrings', async () => { + await withController( + { cacheEncryptionKey }, + async ({ controller, encryptor }) => { + await controller.setLocked(); + jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + { + type: 'UnsupportedKeyring', + data: '0x1234', + }, + ]); + + await controller.submitPassword(password); + + expect(controller.state.isUnlocked).toBe(true); + }, + ); + }); + + it('should throw error if vault unlocked has an unexpected shape', async () => { + await withController( + { cacheEncryptionKey }, + async ({ controller, encryptor }) => { + await controller.setLocked(); + jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + { + foo: 'bar', + }, + ]); + + await expect(controller.submitPassword(password)).rejects.toThrow( + KeyringControllerError.VaultDataError, + ); + }, + ); + }); + + it('should throw error if vault is missing', async () => { + const controller = new KeyringController({ + messenger: buildKeyringControllerMessenger(), + encryptor: new MockEncryptor(), + updateIdentities: jest.fn(), + setAccountLabel: jest.fn(), + syncIdentities: jest.fn(), + setSelectedAddress: jest.fn(), + }); + + await expect(controller.submitPassword(password)).rejects.toThrow( + KeyringControllerError.VaultError, + ); + }); + + !cacheEncryptionKey && + it('should throw error if password is of wrong type', async () => { + await withController( + { cacheEncryptionKey }, + async ({ controller }) => { + await expect( + // @ts-expect-error we are testing the case of a user using + // the wrong password type + controller.submitPassword(12341234), + ).rejects.toThrow(KeyringControllerError.WrongPasswordType); + }, + ); + }); + cacheEncryptionKey && it('should set encryptionKey and encryptionSalt in state', async () => { withController({ cacheEncryptionKey }, async ({ controller }) => { @@ -1849,6 +2011,75 @@ describe('KeyringController', () => { }, ); }); + + it('should unlock also with unsupported keyrings', async () => { + await withController( + { cacheEncryptionKey: true }, + async ({ controller, initialState, encryptor }) => { + await controller.setLocked(); + jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + { + type: 'UnsupportedKeyring', + data: '0x1234', + }, + ]); + + await controller.submitEncryptionKey( + MOCK_ENCRYPTION_KEY, + initialState.encryptionSalt as string, + ); + + expect(controller.state.isUnlocked).toBe(true); + }, + ); + }); + + it('should throw error if vault unlocked has an unexpected shape', async () => { + await withController( + { cacheEncryptionKey: true }, + async ({ controller, initialState, encryptor }) => { + jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + { + foo: 'bar', + }, + ]); + + await expect( + controller.submitEncryptionKey( + MOCK_ENCRYPTION_KEY, + initialState.encryptionSalt as string, + ), + ).rejects.toThrow(KeyringControllerError.VaultDataError); + }, + ); + }); + + it('should throw error if encryptionSalt is different from the one in the vault', async () => { + await withController( + { cacheEncryptionKey: true }, + async ({ controller }) => { + await expect( + controller.submitEncryptionKey(MOCK_ENCRYPTION_KEY, '0x1234'), + ).rejects.toThrow(KeyringControllerError.ExpiredCredentials); + }, + ); + }); + + it('should throw error if encryptionKey is of an unexpected type', async () => { + await withController( + { cacheEncryptionKey: true }, + async ({ controller, initialState }) => { + await expect( + controller.submitEncryptionKey( + // @ts-expect-error we are testing the case of a user using + // the wrong encryptionKey type + 12341234, + initialState.encryptionSalt as string, + ), + ).rejects.toThrow(KeyringControllerError.WrongPasswordType); + }, + ); + }); }); describe('verifySeedPhrase', () => { @@ -1890,6 +2121,21 @@ describe('KeyringController', () => { }).not.toThrow(); }); }); + + it('should throw error if vault is missing', async () => { + const controller = new KeyringController({ + messenger: buildKeyringControllerMessenger(), + encryptor: new MockEncryptor(), + updateIdentities: jest.fn(), + setAccountLabel: jest.fn(), + syncIdentities: jest.fn(), + setSelectedAddress: jest.fn(), + }); + + await expect(controller.verifyPassword(password)).rejects.toThrow( + KeyringControllerError.VaultError, + ); + }); }); describe('when wrong password is provided', () => { @@ -1904,6 +2150,21 @@ describe('KeyringController', () => { ); }); }); + + it('should throw error if vault is missing', async () => { + const controller = new KeyringController({ + messenger: buildKeyringControllerMessenger(), + encryptor: new MockEncryptor(), + updateIdentities: jest.fn(), + setAccountLabel: jest.fn(), + syncIdentities: jest.fn(), + setSelectedAddress: jest.fn(), + }); + + await expect(controller.verifyPassword('123')).rejects.toThrow( + KeyringControllerError.VaultError, + ); + }); }); }); From e8cf940f0a551b4870e3d830aa9277eaaef5ba3d Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 25 Jan 2024 19:24:37 +0100 Subject: [PATCH 2/3] refactor: add `skipVaultCreation` to `withController` --- .../src/KeyringController.test.ts | 114 ++++++++---------- 1 file changed, 49 insertions(+), 65 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index db2a224e30..11153d4eb4 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -493,21 +493,17 @@ describe('KeyringController', () => { }); it('should throw error if password is of wrong type', async () => { - const controller = new KeyringController({ - messenger: buildKeyringControllerMessenger(), - encryptor: new MockEncryptor(), - updateIdentities: jest.fn(), - setAccountLabel: jest.fn(), - syncIdentities: jest.fn(), - setSelectedAddress: jest.fn(), - }); - - await expect( - controller.createNewVaultAndKeychain( - // @ts-expect-error invalid password - 123, - ), - ).rejects.toThrow(KeyringControllerError.WrongPasswordType); + await withController( + { skipVaultCreation: true }, + async ({ controller }) => { + await expect( + controller.createNewVaultAndKeychain( + // @ts-expect-error invalid password + 123, + ), + ).rejects.toThrow(KeyringControllerError.WrongPasswordType); + }, + ); }); }); @@ -836,21 +832,17 @@ describe('KeyringController', () => { }); it('should throw an error if there are no keyrings', async () => { - const controller = new KeyringController({ - messenger: buildKeyringControllerMessenger(), - encryptor: new MockEncryptor(), - updateIdentities: jest.fn(), - setAccountLabel: jest.fn(), - syncIdentities: jest.fn(), - setSelectedAddress: jest.fn(), - }); - - await expect( - controller.getKeyringForAccount( - '0x51253087e6f8358b5f10c0a94315d69db3357859', - ), - ).rejects.toThrow( - 'KeyringController - No keyring found. Error info: There are no keyrings', + await withController( + { skipVaultCreation: true }, + async ({ controller }) => { + await expect( + controller.getKeyringForAccount( + '0x51253087e6f8358b5f10c0a94315d69db3357859', + ), + ).rejects.toThrow( + 'KeyringController - No keyring found. Error info: There are no keyrings', + ); + }, ); }); }); @@ -1958,17 +1950,13 @@ describe('KeyringController', () => { }); it('should throw error if vault is missing', async () => { - const controller = new KeyringController({ - messenger: buildKeyringControllerMessenger(), - encryptor: new MockEncryptor(), - updateIdentities: jest.fn(), - setAccountLabel: jest.fn(), - syncIdentities: jest.fn(), - setSelectedAddress: jest.fn(), - }); - - await expect(controller.submitPassword(password)).rejects.toThrow( - KeyringControllerError.VaultError, + await withController( + { skipVaultCreation: true }, + async ({ controller }) => { + await expect(controller.submitPassword(password)).rejects.toThrow( + KeyringControllerError.VaultError, + ); + }, ); }); @@ -2123,17 +2111,13 @@ describe('KeyringController', () => { }); it('should throw error if vault is missing', async () => { - const controller = new KeyringController({ - messenger: buildKeyringControllerMessenger(), - encryptor: new MockEncryptor(), - updateIdentities: jest.fn(), - setAccountLabel: jest.fn(), - syncIdentities: jest.fn(), - setSelectedAddress: jest.fn(), - }); - - await expect(controller.verifyPassword(password)).rejects.toThrow( - KeyringControllerError.VaultError, + await withController( + { skipVaultCreation: true }, + async ({ controller }) => { + await expect(controller.verifyPassword(password)).rejects.toThrow( + KeyringControllerError.VaultError, + ); + }, ); }); }); @@ -2152,17 +2136,13 @@ describe('KeyringController', () => { }); it('should throw error if vault is missing', async () => { - const controller = new KeyringController({ - messenger: buildKeyringControllerMessenger(), - encryptor: new MockEncryptor(), - updateIdentities: jest.fn(), - setAccountLabel: jest.fn(), - syncIdentities: jest.fn(), - setSelectedAddress: jest.fn(), - }); - - await expect(controller.verifyPassword('123')).rejects.toThrow( - KeyringControllerError.VaultError, + await withController( + { skipVaultCreation: true }, + async ({ controller }) => { + await expect(controller.verifyPassword('123')).rejects.toThrow( + KeyringControllerError.VaultError, + ); + }, ); }); }); @@ -3093,7 +3073,9 @@ type WithControllerCallback = ({ messenger: KeyringControllerMessenger; }) => Promise | ReturnValue; -type WithControllerOptions = Partial; +type WithControllerOptions = Partial & { + skipVaultCreation?: boolean; +}; type WithControllerArgs = | [WithControllerCallback] @@ -3171,7 +3153,9 @@ async function withController( ...preferences, ...rest, }); - await controller.createNewVaultAndKeychain(password); + if (!rest.skipVaultCreation) { + await controller.createNewVaultAndKeychain(password); + } return await fn({ controller, preferences, From 26bf1dc0e5850b37629a3bd6515151cf4aa34620 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 25 Jan 2024 20:21:27 +0100 Subject: [PATCH 3/3] test: remove some istanbul ignore if --- .../src/KeyringController.test.ts | 33 +++++++++++++++++++ .../src/KeyringController.ts | 4 --- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 11153d4eb4..cacbc57121 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -189,6 +189,17 @@ describe('KeyringController', () => { }); }); }); + + it('should throw error with no HD keyring', async () => { + await withController( + { skipVaultCreation: true }, + async ({ controller }) => { + await expect(controller.addNewAccount()).rejects.toThrow( + 'No HD keyring found', + ); + }, + ); + }); }); describe('addNewAccountForKeyring', () => { @@ -342,6 +353,17 @@ describe('KeyringController', () => { }, ); }); + + it('should throw error with no HD keyring', async () => { + await withController( + { skipVaultCreation: true }, + async ({ controller }) => { + await expect(controller.addNewAccountWithoutUpdate()).rejects.toThrow( + 'No HD keyring found', + ); + }, + ); + }); }); describe('addNewKeyring', () => { @@ -2098,6 +2120,17 @@ describe('KeyringController', () => { ); }); }); + + it('should throw error with no HD keyring', async () => { + await withController( + { skipVaultCreation: true }, + async ({ controller }) => { + await expect(controller.verifySeedPhrase()).rejects.toThrow( + 'No HD keyring found', + ); + }, + ); + }); }); describe('verifyPassword', () => { diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 117ae1b93d..94cbee33ff 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -386,7 +386,6 @@ export class KeyringController extends BaseController< addedAccountAddress: string; }> { const primaryKeyring = this.#keyring.getKeyringsByType('HD Key Tree')[0]; - /* istanbul ignore if */ if (!primaryKeyring) { throw new Error('No HD keyring found'); } @@ -463,7 +462,6 @@ export class KeyringController extends BaseController< */ async addNewAccountWithoutUpdate(): Promise { const primaryKeyring = this.#keyring.getKeyringsByType('HD Key Tree')[0]; - /* istanbul ignore if */ if (!primaryKeyring) { throw new Error('No HD keyring found'); } @@ -700,7 +698,6 @@ export class KeyringController extends BaseController< throw new Error('Cannot import invalid private key.'); } - /* istanbul ignore if */ if ( !isValidPrivate(bufferedPrivateKey) || // ensures that the key is 64 bytes long @@ -934,7 +931,6 @@ export class KeyringController extends BaseController< */ async verifySeedPhrase(): Promise { const primaryKeyring = this.#keyring.getKeyringsByType(KeyringTypes.hd)[0]; - /* istanbul ignore if */ if (!primaryKeyring) { throw new Error('No HD keyring found.'); }