From 7d29aa3a48320d335db24c116635618ef5d527e8 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Fri, 25 Nov 2022 15:16:49 +0530 Subject: [PATCH 01/13] Fix: adding null check to avoid error if KeyringClass for a keyring type is not present --- index.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/index.js b/index.js index 07ef6471..3920afed 100644 --- a/index.js +++ b/index.js @@ -679,7 +679,9 @@ class KeyringController extends EventEmitter { */ async restoreKeyring(serialized) { const keyring = await this._restoreKeyring(serialized); - await this._updateMemStoreKeyrings(); + if (keyring) { + await this._updateMemStoreKeyrings(); + } return keyring; } @@ -696,6 +698,9 @@ class KeyringController extends EventEmitter { const { type, data } = serialized; const Keyring = this.getKeyringClassForType(type); + if (!Keyring) { + return + } const keyring = new Keyring(); await keyring.deserialize(data); // getAccounts also validates the accounts for some keyrings From 10106591ef2b4d40fdc511feb8bddb3376a700a9 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Sat, 26 Nov 2022 00:38:09 +0530 Subject: [PATCH 02/13] fix --- index.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/index.js b/index.js index 3920afed..10d38ea9 100644 --- a/index.js +++ b/index.js @@ -43,6 +43,7 @@ class KeyringController extends EventEmitter { isUnlocked: false, keyringTypes: this.keyringTypes.map((keyringType) => keyringType.type), keyrings: [], + unsupported_keyrings: [], encryptionKey: null, }); @@ -562,6 +563,8 @@ class KeyringController extends EventEmitter { }), ); + this.unsupported_keyrings.forEach(keyring => serializedKeyrings.push(keyring)); + let vault; let newEncryptionKey; @@ -699,7 +702,8 @@ class KeyringController extends EventEmitter { const Keyring = this.getKeyringClassForType(type); if (!Keyring) { - return + unsupported_keyrings.push(serialized); + return; } const keyring = new Keyring(); await keyring.deserialize(data); From fbea40fda78190afb92e0eba439bf2819857f619 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Mon, 28 Nov 2022 10:56:39 +0530 Subject: [PATCH 03/13] fix --- index.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/index.js b/index.js index 10d38ea9..e3964448 100644 --- a/index.js +++ b/index.js @@ -563,7 +563,9 @@ class KeyringController extends EventEmitter { }), ); - this.unsupported_keyrings.forEach(keyring => serializedKeyrings.push(keyring)); + this.unsupported_keyrings.forEach((keyring) => + serializedKeyrings.push(keyring), + ); let vault; let newEncryptionKey; @@ -702,8 +704,8 @@ class KeyringController extends EventEmitter { const Keyring = this.getKeyringClassForType(type); if (!Keyring) { - unsupported_keyrings.push(serialized); - return; + this.unsupported_keyrings.push(serialized); + return undefined; } const keyring = new Keyring(); await keyring.deserialize(data); From 3ac20f27b52cc447cef4b563dec293d40705230b Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Mon, 28 Nov 2022 15:37:14 +0530 Subject: [PATCH 04/13] Adding unit test coverage --- index.js | 2 +- test/index.js | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/index.js b/index.js index e3964448..83d8d7bd 100644 --- a/index.js +++ b/index.js @@ -43,12 +43,12 @@ class KeyringController extends EventEmitter { isUnlocked: false, keyringTypes: this.keyringTypes.map((keyringType) => keyringType.type), keyrings: [], - unsupported_keyrings: [], encryptionKey: null, }); this.encryptor = opts.encryptor || encryptor; this.keyrings = []; + this.unsupported_keyrings = []; // This option allows the controller to cache an exported key // for use in decrypting and encrypting data without password diff --git a/test/index.js b/test/index.js index 082f6631..01155674 100644 --- a/test/index.js +++ b/test/index.js @@ -94,6 +94,28 @@ describe('KeyringController', function () { }); }); + describe('persistAllKeyrings', function () { + it('should persist keyrings in unsupported_keyrings array', async function () { + const unsupportedKeyring = 'DUMMY_KEYRING'; + keyringController.unsupported_keyrings = [unsupportedKeyring]; + await keyringController.persistAllKeyrings(); + const { vault } = keyringController.store.getState(); + const keyrings = await mockEncryptor.decrypt(password, vault); + expect(keyrings.indexOf(unsupportedKeyring) > -1).toBeTruthy(); + expect(keyrings).toHaveLength(2); + }); + + it('emits "unlock" event', async function () { + await keyringController.setLocked(); + + const spy = sinon.spy(); + keyringController.on('unlock', spy); + + await keyringController.submitPassword(password); + expect(spy.calledOnce).toBe(true); + }); + }); + describe('createNewVaultAndKeychain', function () { it('should create a new vault', async function () { keyringController.store.updateState({ vault: null }); @@ -330,6 +352,15 @@ describe('KeyringController', function () { expect(keyring.wallets).toHaveLength(1); }); }); + it('add serialized keyring to unsupported_keyrings array if keyring type is not known', async function () { + mockEncryptor.encrypt(password, [ + { type: 'Ledger Keyring', data: 'DUMMY' }, + ]); + await keyringController.setLocked(); + const keyrings = await keyringController.unlockKeyrings(password); + expect(keyrings).toHaveLength(0); + expect(keyringController.unsupported_keyrings).toHaveLength(1); + }); }); describe('verifyPassword', function () { From 3e36a686d3adbd88d435fda617874f7fabb696d3 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Mon, 28 Nov 2022 15:52:05 +0530 Subject: [PATCH 05/13] fix --- test/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/index.js b/test/index.js index 01155674..0712309f 100644 --- a/test/index.js +++ b/test/index.js @@ -101,7 +101,7 @@ describe('KeyringController', function () { await keyringController.persistAllKeyrings(); const { vault } = keyringController.store.getState(); const keyrings = await mockEncryptor.decrypt(password, vault); - expect(keyrings.indexOf(unsupportedKeyring) > -1).toBeTruthy(); + expect(keyrings.indexOf(unsupportedKeyring) > -1).toBe(true); expect(keyrings).toHaveLength(2); }); From 3003ebca6abee7432764e57fe8adf22a32785691 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Wed, 30 Nov 2022 14:57:13 +0530 Subject: [PATCH 06/13] Update index.js Co-authored-by: Elliot Winkler --- index.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/index.js b/index.js index 83d8d7bd..caa42da9 100644 --- a/index.js +++ b/index.js @@ -563,9 +563,7 @@ class KeyringController extends EventEmitter { }), ); - this.unsupported_keyrings.forEach((keyring) => - serializedKeyrings.push(keyring), - ); + serializedKeyrings.push(...this.unsupported_keyrings); let vault; let newEncryptionKey; From 0bc0bc54c506244acf8594bd9374384fb1ed30cf Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Wed, 30 Nov 2022 15:01:21 +0530 Subject: [PATCH 07/13] Update test/index.js Co-authored-by: Elliot Winkler --- test/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/index.js b/test/index.js index 0712309f..9113ad7d 100644 --- a/test/index.js +++ b/test/index.js @@ -359,7 +359,7 @@ describe('KeyringController', function () { await keyringController.setLocked(); const keyrings = await keyringController.unlockKeyrings(password); expect(keyrings).toHaveLength(0); - expect(keyringController.unsupported_keyrings).toHaveLength(1); + expect(keyringController.unsupported_keyrings).toStrictEqual(keyrings); }); }); From 14416d9807a0bd510b39e58a292a1cc45fc8fdd3 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Wed, 30 Nov 2022 15:06:33 +0530 Subject: [PATCH 08/13] fix --- index.js | 8 ++++---- test/index.js | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/index.js b/index.js index caa42da9..149013ac 100644 --- a/index.js +++ b/index.js @@ -48,7 +48,7 @@ class KeyringController extends EventEmitter { this.encryptor = opts.encryptor || encryptor; this.keyrings = []; - this.unsupported_keyrings = []; + this.unsupportedKeyrings = []; // This option allows the controller to cache an exported key // for use in decrypting and encrypting data without password @@ -563,7 +563,7 @@ class KeyringController extends EventEmitter { }), ); - serializedKeyrings.push(...this.unsupported_keyrings); + serializedKeyrings.push(...this.unsupportedKeyrings); let vault; let newEncryptionKey; @@ -695,14 +695,14 @@ class KeyringController extends EventEmitter { * On success, returns the resulting keyring instance. * * @param {Object} serialized - The serialized keyring. - * @returns {Promise} The deserialized keyring. + * @returns {Promise} The deserialized keyring or undefined if the keyring type is unsupported. */ async _restoreKeyring(serialized) { const { type, data } = serialized; const Keyring = this.getKeyringClassForType(type); if (!Keyring) { - this.unsupported_keyrings.push(serialized); + this.unsupportedKeyrings.push(serialized); return undefined; } const keyring = new Keyring(); diff --git a/test/index.js b/test/index.js index 9113ad7d..8c73a114 100644 --- a/test/index.js +++ b/test/index.js @@ -95,9 +95,9 @@ describe('KeyringController', function () { }); describe('persistAllKeyrings', function () { - it('should persist keyrings in unsupported_keyrings array', async function () { + it('should persist keyrings in unsupportedKeyrings array', async function () { const unsupportedKeyring = 'DUMMY_KEYRING'; - keyringController.unsupported_keyrings = [unsupportedKeyring]; + keyringController.unsupportedKeyrings = [unsupportedKeyring]; await keyringController.persistAllKeyrings(); const { vault } = keyringController.store.getState(); const keyrings = await mockEncryptor.decrypt(password, vault); @@ -352,14 +352,14 @@ describe('KeyringController', function () { expect(keyring.wallets).toHaveLength(1); }); }); - it('add serialized keyring to unsupported_keyrings array if keyring type is not known', async function () { + it('add serialized keyring to unsupportedKeyrings array if keyring type is not known', async function () { mockEncryptor.encrypt(password, [ { type: 'Ledger Keyring', data: 'DUMMY' }, ]); await keyringController.setLocked(); const keyrings = await keyringController.unlockKeyrings(password); expect(keyrings).toHaveLength(0); - expect(keyringController.unsupported_keyrings).toStrictEqual(keyrings); + expect(keyringController.unsupportedKeyrings).toStrictEqual(keyrings); }); }); From e0b7b808f2bc1a80e108a5352ffbd72b0adf01eb Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Wed, 30 Nov 2022 15:10:37 +0530 Subject: [PATCH 09/13] fix --- test/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/index.js b/test/index.js index 8c73a114..378c24b3 100644 --- a/test/index.js +++ b/test/index.js @@ -359,7 +359,7 @@ describe('KeyringController', function () { await keyringController.setLocked(); const keyrings = await keyringController.unlockKeyrings(password); expect(keyrings).toHaveLength(0); - expect(keyringController.unsupportedKeyrings).toStrictEqual(keyrings); + expect(keyringController.unsupportedKeyrings).toHaveLength(1); }); }); From 4927bbcfcba7dad964a15ff709095b07da5a518c Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Wed, 30 Nov 2022 15:14:33 +0530 Subject: [PATCH 10/13] fix --- test/index.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/index.js b/test/index.js index 378c24b3..51fd5bf1 100644 --- a/test/index.js +++ b/test/index.js @@ -353,13 +353,14 @@ describe('KeyringController', function () { }); }); it('add serialized keyring to unsupportedKeyrings array if keyring type is not known', async function () { - mockEncryptor.encrypt(password, [ - { type: 'Ledger Keyring', data: 'DUMMY' }, - ]); + const unsupportedKeyrings = [{ type: 'Ledger Keyring', data: 'DUMMY' }]; + mockEncryptor.encrypt(password, unsupportedKeyrings); await keyringController.setLocked(); const keyrings = await keyringController.unlockKeyrings(password); expect(keyrings).toHaveLength(0); - expect(keyringController.unsupportedKeyrings).toHaveLength(1); + expect(keyringController.unsupportedKeyrings).toStrictEqual( + unsupportedKeyrings, + ); }); }); From 9b33800b6ff9483ce63a65e547ae68da1c983f43 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Wed, 30 Nov 2022 20:21:21 +0530 Subject: [PATCH 11/13] Update test/index.js Co-authored-by: Mark Stacey --- test/index.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/index.js b/test/index.js index 51fd5bf1..1065d125 100644 --- a/test/index.js +++ b/test/index.js @@ -98,7 +98,9 @@ describe('KeyringController', function () { it('should persist keyrings in unsupportedKeyrings array', async function () { const unsupportedKeyring = 'DUMMY_KEYRING'; keyringController.unsupportedKeyrings = [unsupportedKeyring]; + await keyringController.persistAllKeyrings(); + const { vault } = keyringController.store.getState(); const keyrings = await mockEncryptor.decrypt(password, vault); expect(keyrings.indexOf(unsupportedKeyring) > -1).toBe(true); From f1e7a625dc932dd20de6906504ffe3f74e185816 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Wed, 30 Nov 2022 20:31:44 +0530 Subject: [PATCH 12/13] fixes --- index.js | 6 +++--- test/index.js | 21 ++++++++++++++------- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/index.js b/index.js index 149013ac..f210ce20 100644 --- a/index.js +++ b/index.js @@ -48,7 +48,7 @@ class KeyringController extends EventEmitter { this.encryptor = opts.encryptor || encryptor; this.keyrings = []; - this.unsupportedKeyrings = []; + this._unsupportedKeyrings = []; // This option allows the controller to cache an exported key // for use in decrypting and encrypting data without password @@ -563,7 +563,7 @@ class KeyringController extends EventEmitter { }), ); - serializedKeyrings.push(...this.unsupportedKeyrings); + serializedKeyrings.push(...this._unsupportedKeyrings); let vault; let newEncryptionKey; @@ -702,7 +702,7 @@ class KeyringController extends EventEmitter { const Keyring = this.getKeyringClassForType(type); if (!Keyring) { - this.unsupportedKeyrings.push(serialized); + this._unsupportedKeyrings.push(serialized); return undefined; } const keyring = new Keyring(); diff --git a/test/index.js b/test/index.js index 51fd5bf1..f84430d0 100644 --- a/test/index.js +++ b/test/index.js @@ -95,9 +95,9 @@ describe('KeyringController', function () { }); describe('persistAllKeyrings', function () { - it('should persist keyrings in unsupportedKeyrings array', async function () { + it('should persist keyrings in _unsupportedKeyrings array', async function () { const unsupportedKeyring = 'DUMMY_KEYRING'; - keyringController.unsupportedKeyrings = [unsupportedKeyring]; + keyringController._unsupportedKeyrings = [unsupportedKeyring]; await keyringController.persistAllKeyrings(); const { vault } = keyringController.store.getState(); const keyrings = await mockEncryptor.decrypt(password, vault); @@ -267,6 +267,13 @@ describe('KeyringController', function () { const accounts = await keyring.getAccounts(); expect(accounts[0]).toBe(walletOneAddresses[0]); }); + it('should return undefined if keyring type is not supported.', async function () { + const unsupportedKeyring = { type: 'Ledger Keyring', data: 'DUMMY' }; + const keyring = await keyringController.restoreKeyring( + unsupportedKeyring, + ); + expect(keyring).toBeUndefined(); + }); }); describe('getAccounts', function () { @@ -352,14 +359,14 @@ describe('KeyringController', function () { expect(keyring.wallets).toHaveLength(1); }); }); - it('add serialized keyring to unsupportedKeyrings array if keyring type is not known', async function () { - const unsupportedKeyrings = [{ type: 'Ledger Keyring', data: 'DUMMY' }]; - mockEncryptor.encrypt(password, unsupportedKeyrings); + it('add serialized keyring to _unsupportedKeyrings array if keyring type is not known', async function () { + const _unsupportedKeyrings = [{ type: 'Ledger Keyring', data: 'DUMMY' }]; + mockEncryptor.encrypt(password, _unsupportedKeyrings); await keyringController.setLocked(); const keyrings = await keyringController.unlockKeyrings(password); expect(keyrings).toHaveLength(0); - expect(keyringController.unsupportedKeyrings).toStrictEqual( - unsupportedKeyrings, + expect(keyringController._unsupportedKeyrings).toStrictEqual( + _unsupportedKeyrings, ); }); }); From 9d0316de91d80d97b19fb17cdd2005b172d4454c Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Wed, 30 Nov 2022 20:33:19 +0530 Subject: [PATCH 13/13] Update test/index.js Co-authored-by: Ariella Vu <20778143+digiwand@users.noreply.github.com> --- test/index.js | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/test/index.js b/test/index.js index 1065d125..d3991d5f 100644 --- a/test/index.js +++ b/test/index.js @@ -106,16 +106,6 @@ describe('KeyringController', function () { expect(keyrings.indexOf(unsupportedKeyring) > -1).toBe(true); expect(keyrings).toHaveLength(2); }); - - it('emits "unlock" event', async function () { - await keyringController.setLocked(); - - const spy = sinon.spy(); - keyringController.on('unlock', spy); - - await keyringController.submitPassword(password); - expect(spy.calledOnce).toBe(true); - }); }); describe('createNewVaultAndKeychain', function () {