Skip to content

Commit

Permalink
Cherry-pick of 'Ensure user is prompted for password during reminder …
Browse files Browse the repository at this point in the history
…based backup (#22307)' into v11.7.0

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 <[email protected]>
  • Loading branch information
danjm and pedronfigueiredo committed Dec 18, 2023
1 parent 39d3fe6 commit bb181f4
Show file tree
Hide file tree
Showing 12 changed files with 167 additions and 35 deletions.
Original file line number Diff line number Diff line change
@@ -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
7 changes: 4 additions & 3 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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<number[]>} 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),
);
}

Expand Down
8 changes: 4 additions & 4 deletions app/scripts/metamask-controller.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.',
);
});

Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
13 changes: 12 additions & 1 deletion test/e2e/tests/incremental-security.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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: [
Expand Down Expand Up @@ -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();
Expand Down
1 change: 1 addition & 0 deletions ui/components/app/reveal-SRP-modal/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default } from './reveal-SRP-modal';
88 changes: 88 additions & 0 deletions ui/components/app/reveal-SRP-modal/reveal-SRP-modal.js
Original file line number Diff line number Diff line change
@@ -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 (
<Modal isOpen={isOpen} onClose={onClose}>
<ModalOverlay />
<ModalContent>
<ModalHeader onClose={onClose}>{t('revealSeedWords')}</ModalHeader>
<Box paddingLeft={4} paddingRight={4}>
<form
onSubmit={(e) => {
e.preventDefault();
onSubmit(password);
}}
>
<FormTextField
marginTop={6}
id="account-details-authenticate"
label={t('enterYourPassword')}
placeholder={t('password')}
onChange={(e) => setPassword(e.target.value)}
value={password}
variant={TextVariant.bodySm}
type="password"
labelProps={{ fontWeight: FontWeight.Medium }}
autoFocus
/>
</form>
<Box display={Display.Flex} marginTop={6} gap={2}>
<ButtonSecondary onClick={onClose} block>
{t('cancel')}
</ButtonSecondary>
<ButtonPrimary
onClick={() => onSubmit(password)}
disabled={password === ''}
block
>
{t('confirm')}
</ButtonPrimary>
</Box>
</Box>
</ModalContent>
</Modal>
);
}

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,
};
29 changes: 14 additions & 15 deletions ui/pages/onboarding-flow/onboarding-flow.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand All @@ -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),
Expand All @@ -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 (
<div className="onboarding-flow">
<RevealSRPModal
setSecretRecoveryPhrase={setSecretRecoveryPhrase}
onClose={() => history.push(DEFAULT_ROUTE)}
isOpen={showPasswordModalToAllowSRPReveal}
/>
<div className="onboarding-flow__wrapper">
<Switch>
<Route
Expand Down Expand Up @@ -203,7 +202,7 @@ export default function OnboardingFlow() {
<Route exact path="*" component={OnboardingFlowSwitch} />
</Switch>
</div>
{pathName === ONBOARDING_COMPLETION_ROUTE && (
{pathname === ONBOARDING_COMPLETION_ROUTE && (
<Button
className="onboarding-flow__twitter-button"
type="link"
Expand Down
1 change: 0 additions & 1 deletion ui/pages/onboarding-flow/onboarding-flow.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ jest.mock('../../store/actions', () => ({
createNewVaultAndGetSeedPhrase: jest.fn().mockResolvedValue(null),
unlockAndGetSeedPhrase: jest.fn().mockResolvedValue(null),
createNewVaultAndRestore: jest.fn(),
verifySeedPhrase: jest.fn(),
}));

describe('Onboarding Flow', () => {
Expand Down
6 changes: 3 additions & 3 deletions ui/store/actions.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -185,22 +185,22 @@ describe('Actions', () => {
const verifyPassword = background.verifyPassword.callsFake((_, cb) =>
cb(),
);
const verifySeedPhrase = background.verifySeedPhrase.callsFake((cb) =>
const getSeedPhrase = background.getSeedPhrase.callsFake((_, cb) =>
cb(null, Array.from(Buffer.from('test').values())),
);

setBackgroundConnection(background);

await store.dispatch(actions.requestRevealSeedWords());
expect(verifyPassword.callCount).toStrictEqual(1);
expect(verifySeedPhrase.callCount).toStrictEqual(1);
expect(getSeedPhrase.callCount).toStrictEqual(1);
});

it('displays warning error message then callback in background errors', async () => {
const store = mockStore();

background.verifyPassword.callsFake((_, cb) => cb());
background.verifySeedPhrase.callsFake((cb) => {
background.getSeedPhrase.callsFake((_, cb) => {
cb(new Error('error'));
});

Expand Down
11 changes: 6 additions & 5 deletions ui/store/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ export function createNewVaultAndGetSeedPhrase(

try {
await createNewVault(password);
const seedPhrase = await verifySeedPhrase();
const seedPhrase = await getSeedPhrase(password);
return seedPhrase;
} catch (error) {
dispatch(displayWarning(error));
Expand All @@ -243,7 +243,7 @@ export function unlockAndGetSeedPhrase(

try {
await submitPassword(password);
const seedPhrase = await verifySeedPhrase();
const seedPhrase = await getSeedPhrase(password);
await forceUpdateMetamaskState(dispatch);
return seedPhrase;
} catch (error) {
Expand Down Expand Up @@ -298,9 +298,10 @@ export function verifyPassword(password: string): Promise<boolean> {
});
}

export async function verifySeedPhrase() {
export async function getSeedPhrase(password: string) {
const encodedSeedPhrase = await submitRequestToBackground<string>(
'verifySeedPhrase',
'getSeedPhrase',
[password],
);
return Buffer.from(encodedSeedPhrase).toString('utf8');
}
Expand All @@ -314,7 +315,7 @@ export function requestRevealSeedWords(

try {
await verifyPassword(password);
const seedPhrase = await verifySeedPhrase();
const seedPhrase = await getSeedPhrase(password);
return seedPhrase;
} finally {
dispatch(hideLoadingIndication());
Expand Down
24 changes: 22 additions & 2 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -4468,7 +4468,7 @@ __metadata:
languageName: node
linkType: hard

"@metamask/keyring-controller@npm:^8.0.3":
"@metamask/keyring-controller@npm:8.0.3":
version: 8.0.3
resolution: "@metamask/keyring-controller@npm:8.0.3"
dependencies:
Expand All @@ -4488,6 +4488,26 @@ __metadata:
languageName: node
linkType: hard

"@metamask/keyring-controller@patch:@metamask/keyring-controller@npm%3A8.0.3#~/.yarn/patches/@metamask-keyring-controller-npm-8.0.3-63afac5958.patch":
version: 8.0.3
resolution: "@metamask/keyring-controller@patch:@metamask/keyring-controller@npm%3A8.0.3#~/.yarn/patches/@metamask-keyring-controller-npm-8.0.3-63afac5958.patch::version=8.0.3&hash=1ad908"
dependencies:
"@keystonehq/metamask-airgapped-keyring": "npm:^0.13.1"
"@metamask/base-controller": "npm:^3.2.3"
"@metamask/eth-keyring-controller": "npm:^13.0.1"
"@metamask/message-manager": "npm:^7.3.5"
"@metamask/preferences-controller": "npm:^4.4.3"
"@metamask/utils": "npm:^8.1.0"
async-mutex: "npm:^0.2.6"
ethereumjs-util: "npm:^7.0.10"
ethereumjs-wallet: "npm:^1.0.1"
immer: "npm:^9.0.6"
peerDependencies:
"@metamask/preferences-controller": ^4.4.3
checksum: f608bdf408323f233b16fc42b2013ad5de6c8cc8b63173ae1823872821e5c1935822f12209a326856af2a440878948d5e245cdbd557defe9ec4f55428d74cc5c
languageName: node
linkType: hard

"@metamask/logging-controller@npm:^1.0.1, @metamask/logging-controller@npm:^1.0.3":
version: 1.0.4
resolution: "@metamask/logging-controller@npm:1.0.4"
Expand Down Expand Up @@ -24330,7 +24350,7 @@ __metadata:
"@metamask/jazzicon": "npm:^2.0.0"
"@metamask/key-tree": "npm:^9.0.0"
"@metamask/keyring-api": "npm:^1.0.0"
"@metamask/keyring-controller": "npm:^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": "npm:^1.0.1"
"@metamask/logo": "npm:^3.1.2"
"@metamask/message-manager": "npm:^7.3.0"
Expand Down

0 comments on commit bb181f4

Please sign in to comment.