Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for KeyringController edge cases #3847

Merged
merged 3 commits into from
Jan 26, 2024
Merged

Add tests for KeyringController edge cases #3847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5bb5ea2
test: add test cases
mikesposito Jan 25, 2024
e8cf940
refactor: add `skipVaultCreation` to `withController`
mikesposito Jan 25, 2024
26bf1dc
test: remove some istanbul ignore if
mikesposito Jan 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
282 changes: 280 additions & 2 deletions packages/keyring-controller/src/KeyringController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -157,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', () => {
Expand Down Expand Up @@ -310,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', () => {
Expand Down Expand Up @@ -459,6 +513,20 @@ describe('KeyringController', () => {
expect(controller.state.vault).toBeDefined();
});
});

it('should throw error if password is of wrong type', async () => {
await withController(
{ skipVaultCreation: true },
async ({ controller }) => {
await expect(
controller.createNewVaultAndKeychain(
// @ts-expect-error invalid password
123,
),
).rejects.toThrow(KeyringControllerError.WrongPasswordType);
},
);
});
});

describe('when there is an existing vault', () => {
Expand Down Expand Up @@ -784,6 +852,21 @@ describe('KeyringController', () => {
);
});
});

it('should throw an error if there are no keyrings', async () => {
await withController(
{ skipVaultCreation: true },
async ({ controller }) => {
await expect(
controller.getKeyringForAccount(
'0x51253087e6f8358b5f10c0a94315d69db3357859',
),
).rejects.toThrow(
'KeyringController - No keyring found. Error info: There are no keyrings',
);
},
);
});
});
});

Expand Down Expand Up @@ -826,6 +909,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', () => {
Expand Down Expand Up @@ -936,6 +1029,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', () => {
Expand Down Expand Up @@ -1824,6 +1934,68 @@ 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 () => {
await withController(
{ skipVaultCreation: true },
async ({ controller }) => {
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 }) => {
Expand All @@ -1849,6 +2021,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', () => {
Expand Down Expand Up @@ -1879,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', () => {
Expand All @@ -1890,6 +2142,17 @@ describe('KeyringController', () => {
}).not.toThrow();
});
});

it('should throw error if vault is missing', async () => {
await withController(
{ skipVaultCreation: true },
async ({ controller }) => {
await expect(controller.verifyPassword(password)).rejects.toThrow(
KeyringControllerError.VaultError,
);
},
);
});
});

describe('when wrong password is provided', () => {
Expand All @@ -1904,6 +2167,17 @@ describe('KeyringController', () => {
);
});
});

it('should throw error if vault is missing', async () => {
await withController(
{ skipVaultCreation: true },
async ({ controller }) => {
await expect(controller.verifyPassword('123')).rejects.toThrow(
KeyringControllerError.VaultError,
);
},
);
});
});
});

Expand Down Expand Up @@ -2832,7 +3106,9 @@ type WithControllerCallback<ReturnValue> = ({
messenger: KeyringControllerMessenger;
}) => Promise<ReturnValue> | ReturnValue;

type WithControllerOptions = Partial<KeyringControllerOptions>;
type WithControllerOptions = Partial<KeyringControllerOptions> & {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Something I've started doing in other test files when it comes to withController (or other similar helpers, like setupController) is splitting off controller options from other kinds of options. So withController could take an options object, but the controller options could be bundled into an options object, and then any other setup configuration you want to make could be done in other options. Essentially that turns this type into:

type WithControllerOptions = {
  options: Partial<KeyringControllerOptions>;
  skipVaultCreation?: boolean;
}

I realize that this would require a change across the board, but something to think about perhaps for a future PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I do think it would improve readability! I will do that in another PR

skipVaultCreation?: boolean;
};

type WithControllerArgs<ReturnValue> =
| [WithControllerCallback<ReturnValue>]
Expand Down Expand Up @@ -2910,7 +3186,9 @@ async function withController<ReturnValue>(
...preferences,
...rest,
});
await controller.createNewVaultAndKeychain(password);
if (!rest.skipVaultCreation) {
await controller.createNewVaultAndKeychain(password);
}
return await fn({
controller,
preferences,
Expand Down
Loading
Loading