Skip to content

[keyring-controller]: Add changePassword method#4279

Merged
mikesposito merged 8 commits intomainfrom
feat/set-password
May 16, 2024
Merged

[keyring-controller]: Add changePassword method#4279
mikesposito merged 8 commits intomainfrom
feat/set-password

Conversation

@mikesposito
Copy link
Copy Markdown
Member

@mikesposito mikesposito commented May 14, 2024

Explanation

This PR adds changePassword as a new method for KeyringController, useful for changing the current password.

This addition aims to make it easier for clients to update the password, which is something that they currently need to implement themself

References

Changelog

@metamask/keyring-controller

  • ADDED: Added changePassword method
    • This method can be used to change the password used to encrypt the vault

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

@mikesposito mikesposito requested a review from a team May 14, 2024 10:59
@mikesposito mikesposito changed the title feat(keyring-controller): Add setPassword method [keyring-controller]: Add setPassword method May 14, 2024
// to force the controller to re-encrypt the vault using
// the new password.
if (this.#cacheEncryptionKey) {
this.update((state) => {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

#persistOrRollback will not be able to roll this back, so in case of errors during vault update clients will have to derive the key again

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From what I can tell, the impact of discarding these would be that the password would be used for vault persistence instead of the key, but that's all. And even then, that's only the case of cacheEncryptionKey is enabled, otherwise the password is always used. Does that sound right? This doesn't seem harmful if that's the case.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes I agree, in case of error consumers will just have to unlock with password again. Mine was a consideration more than a concern - I'm not 100% sure of MV3 implications but I'm guessing that the behaviour is the same, user having to enter the password again

@mikesposito mikesposito changed the title [keyring-controller]: Add setPassword method [keyring-controller]: Add changePassword method May 16, 2024
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants