Chore/optimize submit global pwd #6180
Conversation
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Show resolved
Hide resolved
|
|
||
| // restore the current keyring encryption key with the new global password | ||
| await this.storeKeyringEncryptionKey(keyringEncryptionKey); | ||
|
|
There was a problem hiding this comment.
Bug: Keyring Encryption Key Loading Error
The submitGlobalPassword method unconditionally attempts to load the keyring encryption key via #loadKeyringEncryptionKey. This method throws an error if encryptedKeyringEncryptionKey is not set in the state, which differs from the previous conditional handling and causes failures for users without a stored keyring encryption key.
There was a problem hiding this comment.
encryptedKeyringEncryptionKey is persisted in the controller state and it is required to unlock the Keyring Vault. Expected error will be thrown if it's not present in the state.
There was a problem hiding this comment.
that's not true. the controller is written in a way that it works also if encryptedKeyringEncryptionKey is not present.
| const [vaultKey, keyringEncryptionKey] = await Promise.all([ | ||
| this.#loadSeedlessEncryptionKey(currentDevicePwEncKey), | ||
| this.#loadKeyringEncryptionKey(currentDevicePwEncKey), | ||
| ]); |
There was a problem hiding this comment.
Bug: Keyring Encryption Key Loading Issue
The submitGlobalPasswordAndSync method unconditionally attempts to load the keyring encryption key via this.#loadKeyringEncryptionKey. This fails if encryptedKeyringEncryptionKey is not set in the state, as this.#loadKeyringEncryptionKey asserts its presence. This breaks backward compatibility, causing the method to fail for users without a stored keyring encryption key, unlike previous implementations (e.g., changePassword or the old submitGlobalPassword) which handled this conditionally or gracefully.
There was a problem hiding this comment.
this is a relevant comment.
note that unlock now fails if the keyring encryption key is not set.
the controller is designed in a way that it works without having a keyring key stored. unlock should work without it.
we should change this function to only attempt loading the keyring encryption key if it is stored.
There was a problem hiding this comment.
wait, i'm just realizing that we didn't even update the encrypted encryption key before. why do we do it now?
| }); | ||
|
|
||
| // restore the current keyring encryption key with the new global password | ||
| await this.storeKeyringEncryptionKey(keyringEncryptionKey); |
There was a problem hiding this comment.
Bug: Keyring Encryption Key Loading Issue
The submitGlobalPasswordAndSync method unconditionally attempts to load the keyring encryption key via this.#loadKeyringEncryptionKey within a Promise.all. This causes the method to fail with an EncryptedKeyringEncryptionKeyNotSet error if this.state.encryptedKeyringEncryptionKey is not set. This breaks backward compatibility, as previous implementations (e.g., submitGlobalPassword or changePassword) either did not load the key or did so conditionally, allowing the operation to succeed in such scenarios.
There was a problem hiding this comment.
only do this if we were able to load the keyring encryption key.
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| @@ -11,6 +11,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| - Added an optional parameter, `passwordOutdatedCacheTTL` to the constructor params and exported `SecretMetadata` class from the controller.([#6169](https://github.com/MetaMask/core/pull/6169)) | |||
|
|
|||
| ### Changed | |||
|
|
|||
| - Renamed public method `submitGlobalPassword` to `submitGlobalPasswordAndSync`. ([#6180](https://github.com/MetaMask/core/pull/6180)) | |||
There was a problem hiding this comment.
| - Renamed public method `submitGlobalPassword` to `submitGlobalPasswordAndSync`. ([#6180](https://github.com/MetaMask/core/pull/6180)) | |
| - **Breaking** Renamed public method `submitGlobalPassword` to `submitGlobalPasswordAndSync`. ([#6180](https://github.com/MetaMask/core/pull/6180)) |
|
|
||
| ### Removed | ||
|
|
||
| - Removed `syncLatestGlobalPassword` from the Controller. ([#6180](https://github.com/MetaMask/core/pull/6180)) |
There was a problem hiding this comment.
| - Removed `syncLatestGlobalPassword` from the Controller. ([#6180](https://github.com/MetaMask/core/pull/6180)) | |
| - **Breaking** Removed `syncLatestGlobalPassword` from the Controller. ([#6180](https://github.com/MetaMask/core/pull/6180)) |
| const [vaultKey, keyringEncryptionKey] = await Promise.all([ | ||
| this.#loadSeedlessEncryptionKey(currentDevicePwEncKey), | ||
| this.#loadKeyringEncryptionKey(currentDevicePwEncKey), | ||
| ]); |
There was a problem hiding this comment.
this is a relevant comment.
note that unlock now fails if the keyring encryption key is not set.
the controller is designed in a way that it works without having a keyring key stored. unlock should work without it.
we should change this function to only attempt loading the keyring encryption key if it is stored.
| const { revokeToken } = await this.#unlockVaultAndGetVaultData( | ||
| undefined, | ||
| vaultKey, | ||
| ); |
There was a problem hiding this comment.
note to myself: i should have written unlockVaultAndGetVaultData to accept an args object (args: {password?, encryptionKey?}) instead of (password, encryptionKey). calling it with (undefined, encryptionKey) just looks odd 🙈
| }); | ||
|
|
||
| // restore the current keyring encryption key with the new global password | ||
| await this.storeKeyringEncryptionKey(keyringEncryptionKey); |
There was a problem hiding this comment.
only do this if we were able to load the keyring encryption key.
|
|
||
| // restore the current keyring encryption key with the new global password | ||
| await this.storeKeyringEncryptionKey(keyringEncryptionKey); | ||
|
|
There was a problem hiding this comment.
that's not true. the controller is written in a way that it works also if encryptedKeyringEncryptionKey is not present.
Explanation
This PR optimizes the password sync operation at unlock.
Previously, we called two methods,
submitGlobalPasswordandsyncLatestGlobalPasswordin the controller.toprfClient.recoverEncKeyto retrieve the latest encKeys to recover the current device's pwEncKey and unlock the SeedlessOnboardingController with the recovered keytoprfClient.recoverEncKeyagain to recover the latest key again and do the state sync in the controller.This PR removes the later method,
syncLatestGlobalPasswordand do the state sync inside thesubmitGlobalPasswordhence, removing the one extra call toTOPRFEvalRequestReferences
Changelog
Checklist