From 1d83e1f92824ddc64a822902328210ad79800b55 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Mon, 18 Dec 2023 09:19:22 -0330 Subject: [PATCH] Cherry-pick of 'Ensure user is prompted for password during reminder based backup (#22307)' into v11.7.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit During the backup flow, we should prompt a user for a password. This provides extra friction during a security sensitive step. 1. Build, install and onboard 2. Make sure to back up your seed phrase during onboarding. Onboarding should work as normal 3. Once you get to the home screen, replace `home.html` in the url browsers url bar with `home.html#onboarding/secure-your-wallet/?isFromReminder=true` and press enter 4. Click "Secure my wallet" 5. You should then be prompted to enter your password. 6. After entering your password, you should be able to proceed as normal If you repeat those steps but click cancel when being prompted to enter your password, you should be taken to the home screen. If you repeat those steps, but on step 2 don't back up your seed phrase, the remaining steps should work as described above. https://github.com/MetaMask/metamask-extension/assets/7499938/ac8b5dfb-ca7b-4622-95b8-07db5405abfe - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've clearly explained what problem this PR is solving and how it is solved. - [ ] I've linked related issues - [ ] I've included manual testing steps - [ ] I've included screenshots/recordings if applicable - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [ ] I’ve properly set the pull request status: - [ ] In case it's not yet "ready for review", I've set it to "draft". - [ ] In case it's "ready for review", I've changed it from "draft" to "non-draft". - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: Pedro Figueiredo --- ...ring-controller-npm-8.0.3-63afac5958.patch | 12 +++ app/scripts/metamask-controller.js | 7 +- app/scripts/metamask-controller.test.js | 8 +- package.json | 2 +- test/e2e/tests/incremental-security.spec.js | 13 ++- ui/components/app/reveal-SRP-modal/index.js | 1 + .../app/reveal-SRP-modal/reveal-SRP-modal.js | 88 +++++++++++++++++++ ui/pages/onboarding-flow/onboarding-flow.js | 29 +++--- .../onboarding-flow/onboarding-flow.test.js | 1 - ui/store/actions.test.js | 6 +- ui/store/actions.ts | 11 +-- yarn.lock | 24 ++++- 12 files changed, 167 insertions(+), 35 deletions(-) create mode 100644 .yarn/patches/@metamask-keyring-controller-npm-8.0.3-63afac5958.patch create mode 100644 ui/components/app/reveal-SRP-modal/index.js create mode 100644 ui/components/app/reveal-SRP-modal/reveal-SRP-modal.js diff --git a/.yarn/patches/@metamask-keyring-controller-npm-8.0.3-63afac5958.patch b/.yarn/patches/@metamask-keyring-controller-npm-8.0.3-63afac5958.patch new file mode 100644 index 000000000000..4eb1771d1423 --- /dev/null +++ b/.yarn/patches/@metamask-keyring-controller-npm-8.0.3-63afac5958.patch @@ -0,0 +1,12 @@ +diff --git a/dist/KeyringController.js b/dist/KeyringController.js +index fc1c30d4e23badb803242eee5cac65ece8de172b..57d0067cbe551fc0cea986daedf95344dd41fa14 100644 +--- a/dist/KeyringController.js ++++ b/dist/KeyringController.js +@@ -645,7 +645,6 @@ class KeyringController extends base_controller_1.BaseControllerV2 { + throw new Error('Seed phrase imported different accounts.'); + } + }); +- return seedWords; + }); + } + // QR Hardware related methods diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index e663c4892d8f..077090dc9c56 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -2608,7 +2608,7 @@ export default class MetamaskController extends EventEmitter { // primary keyring management addNewAccount: this.addNewAccount.bind(this), - verifySeedPhrase: this.verifySeedPhrase.bind(this), + getSeedPhrase: this.getSeedPhrase.bind(this), resetAccount: this.resetAccount.bind(this), removeAccount: this.removeAccount.bind(this), importAccountWithStrategy: this.importAccountWithStrategy.bind(this), @@ -3798,12 +3798,13 @@ export default class MetamaskController extends EventEmitter { * * Called when the first account is created and on unlocking the vault. * + * @param password * @returns {Promise} The seed phrase to be confirmed by the user, * encoded as an array of UTF-8 bytes. */ - async verifySeedPhrase() { + async getSeedPhrase(password) { return this._convertEnglishWordlistIndicesToCodepoints( - await this.keyringController.verifySeedPhrase(), + await this.keyringController.exportSeedPhrase(password), ); } diff --git a/app/scripts/metamask-controller.test.js b/app/scripts/metamask-controller.test.js index b9e97c82a463..871857f99d67 100644 --- a/app/scripts/metamask-controller.test.js +++ b/app/scripts/metamask-controller.test.js @@ -925,10 +925,10 @@ describe('MetaMaskController', () => { }); }); - describe('#verifyseedPhrase', () => { - it('errors when no keying is provided', async () => { - await expect(metamaskController.verifySeedPhrase()).rejects.toThrow( - 'No HD keyring found', + describe('#getSeedPhrase', () => { + it('errors when no password is provided', async () => { + await expect(metamaskController.getSeedPhrase()).rejects.toThrow( + 'KeyringController - Cannot unlock without a previous vault.', ); }); diff --git a/package.json b/package.json index c203c6483d3f..5862028afd98 100644 --- a/package.json +++ b/package.json @@ -265,7 +265,7 @@ "@metamask/jazzicon": "^2.0.0", "@metamask/key-tree": "^9.0.0", "@metamask/keyring-api": "^1.0.0", - "@metamask/keyring-controller": "^8.0.3", + "@metamask/keyring-controller": "patch:@metamask/keyring-controller@npm%3A8.0.3#~/.yarn/patches/@metamask-keyring-controller-npm-8.0.3-63afac5958.patch", "@metamask/logging-controller": "^1.0.1", "@metamask/logo": "^3.1.2", "@metamask/message-manager": "^7.3.0", diff --git a/test/e2e/tests/incremental-security.spec.js b/test/e2e/tests/incremental-security.spec.js index 48ac135423b3..7c7346c52561 100644 --- a/test/e2e/tests/incremental-security.spec.js +++ b/test/e2e/tests/incremental-security.spec.js @@ -2,6 +2,8 @@ const { strict: assert } = require('assert'); const { convertToHexValue, withFixtures, openDapp } = require('../helpers'); const FixtureBuilder = require('../fixture-builder'); +const WALLET_PASSWORD = 'correct horse battery staple'; + describe('Incremental Security', function () { const ganacheOptions = { accounts: [ @@ -124,7 +126,16 @@ describe('Incremental Security', function () { // reveals the Secret Recovery Phrase await driver.clickElement('[data-testid="secure-wallet-recommended"]'); - await driver.clickElement('[data-testid="recovery-phrase-reveal"]'); + + await driver.fill('[placeholder="Password"]', WALLET_PASSWORD); + await driver.clickElement({ text: 'Confirm', tag: 'button' }); + await driver.waitForElementNotPresent('.mm-modal-overlay'); + + const recoveryPhraseRevealButton = await driver.findClickableElement( + '[data-testid="recovery-phrase-reveal"]', + ); + await recoveryPhraseRevealButton.click(); + const chipTwo = await ( await driver.findElement('[data-testid="recovery-phrase-chip-2"]') ).getText(); diff --git a/ui/components/app/reveal-SRP-modal/index.js b/ui/components/app/reveal-SRP-modal/index.js new file mode 100644 index 000000000000..0d5246dcfd0e --- /dev/null +++ b/ui/components/app/reveal-SRP-modal/index.js @@ -0,0 +1 @@ +export { default } from './reveal-SRP-modal'; diff --git a/ui/components/app/reveal-SRP-modal/reveal-SRP-modal.js b/ui/components/app/reveal-SRP-modal/reveal-SRP-modal.js new file mode 100644 index 000000000000..dabdf2e062d4 --- /dev/null +++ b/ui/components/app/reveal-SRP-modal/reveal-SRP-modal.js @@ -0,0 +1,88 @@ +import PropTypes from 'prop-types'; +import React, { useCallback, useState } from 'react'; +import { + Display, + TextVariant, + FontWeight, +} from '../../../helpers/constants/design-system'; +import { useI18nContext } from '../../../hooks/useI18nContext'; +import { getSeedPhrase } from '../../../store/actions'; +import { + Box, + Modal, + ModalContent, + ModalHeader, + ModalOverlay, + ButtonPrimary, + ButtonSecondary, + FormTextField, +} from '../../component-library'; + +export default function RevealSRPModal({ + setSecretRecoveryPhrase, + onClose, + isOpen, +}) { + const t = useI18nContext(); + + const [password, setPassword] = useState(''); + + const onSubmit = useCallback( + async (_password) => { + const seedPhrase = await getSeedPhrase(_password); + setSecretRecoveryPhrase(seedPhrase); + }, + [setSecretRecoveryPhrase], + ); + + return ( + + + + {t('revealSeedWords')} + +
{ + e.preventDefault(); + onSubmit(password); + }} + > + setPassword(e.target.value)} + value={password} + variant={TextVariant.bodySm} + type="password" + labelProps={{ fontWeight: FontWeight.Medium }} + autoFocus + /> + + + + {t('cancel')} + + onSubmit(password)} + disabled={password === ''} + block + > + {t('confirm')} + + +
+ + + ); +} + +RevealSRPModal.propTypes = { + /** + * A function to set a secret receovery phrase in the context that is rendering the RevealSRPModal + */ + setSecretRecoveryPhrase: PropTypes.func.isRequired, + onClose: PropTypes.func.isRequired, + isOpen: PropTypes.bool.isRequired, +}; diff --git a/ui/pages/onboarding-flow/onboarding-flow.js b/ui/pages/onboarding-flow/onboarding-flow.js index be6367277758..d6e7accfe7ea 100644 --- a/ui/pages/onboarding-flow/onboarding-flow.js +++ b/ui/pages/onboarding-flow/onboarding-flow.js @@ -27,11 +27,11 @@ import { createNewVaultAndGetSeedPhrase, unlockAndGetSeedPhrase, createNewVaultAndRestore, - verifySeedPhrase, } from '../../store/actions'; import { getFirstTimeFlowTypeRoute } from '../../selectors'; import { MetaMetricsContext } from '../../contexts/metametrics'; import Button from '../../components/ui/button'; +import RevealSRPModal from '../../components/app/reveal-SRP-modal'; import { useI18nContext } from '../../hooks/useI18nContext'; import { MetaMetricsEventCategory, @@ -60,7 +60,7 @@ const TWITTER_URL = 'https://twitter.com/MetaMask'; export default function OnboardingFlow() { const [secretRecoveryPhrase, setSecretRecoveryPhrase] = useState(''); const dispatch = useDispatch(); - const { pathName, search } = useLocation(); + const { pathname, search } = useLocation(); const history = useHistory(); const t = useI18nContext(); const completedOnboarding = useSelector(getCompletedOnboarding); @@ -74,18 +74,6 @@ export default function OnboardingFlow() { } }, [history, completedOnboarding, isFromReminder]); - useEffect(() => { - const verifyAndSetSeedPhrase = async () => { - if (completedOnboarding && !secretRecoveryPhrase) { - const verifiedSeedPhrase = await verifySeedPhrase(); - if (verifiedSeedPhrase) { - setSecretRecoveryPhrase(verifiedSeedPhrase); - } - } - }; - verifyAndSetSeedPhrase(); - }, [completedOnboarding, secretRecoveryPhrase]); - const handleCreateNewAccount = async (password) => { const newSecretRecoveryPhrase = await dispatch( createNewVaultAndGetSeedPhrase(password), @@ -105,8 +93,19 @@ export default function OnboardingFlow() { return await dispatch(createNewVaultAndRestore(password, srp)); }; + const showPasswordModalToAllowSRPReveal = + pathname === `${ONBOARDING_REVIEW_SRP_ROUTE}/` && + completedOnboarding && + !secretRecoveryPhrase && + isFromReminder; + return (
+ history.push(DEFAULT_ROUTE)} + isOpen={showPasswordModalToAllowSRPReveal} + />
- {pathName === ONBOARDING_COMPLETION_ROUTE && ( + {pathname === ONBOARDING_COMPLETION_ROUTE && (