fix: clear encryption salt and key

[owencraston]

Explanation

• Currently, when a user locks the app and then resets their password, they are not able to log in wth their new password.
• This is because the encryption key was generated with the old password and not cleared.
• The solve this we will remove the encryption key from state when the user locks the wallet as well as when #createNewVaultWithKeyring is called.
• This allows for a new encryption key to be generated with the new password when the user logs in again and submitPassword is called.

References

• Fixes [Bug]: The new password is not recognized; Metamask only accepts the old password after wallet reset metamask-extension#25696
• Moves path from this PR into the KeyringController

Changelog

@metamask/keyring-controller

• FIXED: Reset encryption key when wallet is locked or when new vault is created to ensure that encryption key is always based on most recent password.

Checklist

• I've updated the test suite for new or updated code as appropriate
• I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
• I've highlighted breaking changes using the "BREAKING" category above as appropriate

[MajorLift]

How would you feel about replacing all of the toBeUndefined checks with not.toHaveProperty?

Checking for property would be the stricter test here, as the current checks would pass if the properties exist and are set to undefined at runtime.

// `controller.state`
{ ..., encryptionKey: undefined, encryptionSalt: undefined }

[owencraston]

@MajorLift good suggestion. I have made those changes in the latest commit.

[mikesposito]

This makes sense when the intention is to create a new vault with createNewVaultAndKeychain. But wouldn't the user be best served using the changePassword method? The purpose of this method is exactly changing only the password, as opposed to recreating the entire vault manually.

On the other hand, caching the encryption key allows us to make decryption faster as we skip derivation (the most time-consuming step), so clearing it out on lock would lose its purpose

[owencraston]

@mikesposito this is correct but in speaking with @Gudahtt he did not feel comfortable using that method in the extension yet. Perhaps this is the best long term solution though 🤔

[Gudahtt]

But wouldn't the user be best served using the changePassword method? The purpose of this method is exactly changing only the password, as opposed to recreating the entire vault manually.

changePassword is for when the wallet is unlocked. In the "forgot password" flow, the wallet is locked so we cannot use that method.

[Gudahtt]

LGTM! Just two nits about adding newlines