Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: seed recovery validation and unlock fix #467

Merged
merged 6 commits into from
Nov 28, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions specs/util/suri.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,22 @@ describe('suri', () => {
'SURI must contain a phrase.'
);
});

it('should parse string with extra spaces as brain wallet', () => {
const extraSpaces = ' great sparta ';
expect(parseSURI(extraSpaces)).toEqual({
derivePath: '',
password: '',
phrase: ' great sparta '
});

const extraSpacesWithPath = ' great sparta //hard/soft///password';
expect(parseSURI(extraSpacesWithPath)).toEqual({
derivePath: '//hard/soft',
password: 'password',
phrase: ' great sparta '
});
});
});

describe('constructing', () => {
Expand Down
11 changes: 6 additions & 5 deletions src/screens/AccountRecover.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,7 @@ import { emptyAccount, validateSeed } from '../util/account';
import { debounce } from '../util/debounce';
import { brainWalletAddress, substrateAddress } from '../util/native';
import { constructSURI } from '../util/suri';
import {
alertErrorWithMessage,
alertInvalidSeedRecovery
} from '../util/alertUtils';
import { alertErrorWithMessage, alertRisks } from '../util/alertUtils';

export default class AccountRecover extends React.PureComponent {
render() {
Expand Down Expand Up @@ -173,7 +170,11 @@ class AccountRecoverView extends React.PureComponent {

if (!validation.valid) {
if (validation.accountRecoveryAllowed) {
return alertInvalidSeedRecovery(`${validation.reason}`, navigation);
return alertRisks(`${validation.reason}`, () =>
navigation.navigate('AccountPin', {
isNew: true
})
);
} else {
return alertErrorWithMessage(`${validation.reason}`, 'Back');
}
Expand Down
38 changes: 34 additions & 4 deletions src/screens/IdentityNew.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,21 @@ import {
navigateToNewIdentityNetwork,
setPin
} from '../util/navigationHelpers';
import { alertIdentityCreationError } from '../util/alertUtils';
import {
alertErrorWithMessage,
alertIdentityCreationError,
alertRisks
} from '../util/alertUtils';
import testIDs from '../../e2e/testIDs';
import ScreenHeading from '../components/ScreenHeading';
import KeyboardScrollView from '../components/KeyboardScrollView';
import { brainWalletAddress } from '../util/native';
import { debounce } from '../util/debounce';

function IdentityNew({ accounts, navigation }) {
const isRecoverDefaultValue = navigation.getParam('isRecover', false);
const [isRecover, setIsRecover] = useState(isRecoverDefaultValue);
const [isSeedValid, setIsSeedValid] = useState(false);
const [seedPhrase, setSeedPhrase] = useState('');

useEffect(() => {
Expand All @@ -52,6 +59,18 @@ function IdentityNew({ accounts, navigation }) {
accounts.updateNewIdentity({ name });
};

const onSeedTextInput = inputSeedPhrase => {
setSeedPhrase(inputSeedPhrase);
const addressGeneration = () =>
brainWalletAddress(inputSeedPhrase)
.then(({ bip39 }) => {
setIsSeedValid(validateSeed(inputSeedPhrase, bip39));
})
.catch(() => setIsSeedValid(inputSeedPhrase, false));
const debouncedAddressGeneration = debounce(addressGeneration, 200);
Copy link
Contributor

Choose a reason for hiding this comment

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

This debounce isn't working. I remember struggling a lot with it in the past (in screens/AccountRecover.js).
To verify what I said, add a console.log in addressGeneration, you'll see that it's printed at every key stroke. I couldn't manage to make it work in a reasonable time in this component though :(.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, let me have a try.

Copy link
Contributor Author

@hanwencheng hanwencheng Nov 28, 2019

Choose a reason for hiding this comment

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

updated, in debounce.js we create new instance of timeout at each call of debounce, so that it is not cleared.

debouncedAddressGeneration();
};

const onRecoverIdentity = async () => {
const pin = await setPin(navigation);
try {
Expand All @@ -63,6 +82,17 @@ function IdentityNew({ accounts, navigation }) {
}
};

const onRecoverConfirm = () => {
if (!isSeedValid.valid) {
if (isSeedValid.accountRecoveryAllowed) {
return alertRisks(`${isSeedValid.reason}`, onRecoverIdentity);
} else {
return alertErrorWithMessage(`${isSeedValid.reason}`, 'Back');
}
}
return onRecoverIdentity();
};

const onCreateNewIdentity = () => {
setSeedPhrase('');
navigation.navigate('IdentityBackup', {
Expand All @@ -74,8 +104,8 @@ function IdentityNew({ accounts, navigation }) {
<>
<AccountSeed
testID={testIDs.IdentityNew.seedInput}
valid={validateSeed(seedPhrase, true).valid} //TODO: validation need to be improved.
onChangeText={setSeedPhrase}
valid={isSeedValid.valid}
onChangeText={onSeedTextInput}
value={seedPhrase}
/>
<View style={styles.btnBox}>
Expand All @@ -90,7 +120,7 @@ function IdentityNew({ accounts, navigation }) {
<Button
title="Recover Identity"
testID={testIDs.IdentityNew.recoverButton}
onPress={onRecoverIdentity}
onPress={onRecoverConfirm}
small={true}
/>
</View>
Expand Down
2 changes: 1 addition & 1 deletion src/util/account.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ export function validateSeed(seed, validBip39Seed) {
return {
accountRecoveryAllowed: true,
reason:
'This recovery phrase will be treated as a legacy Parity brain wallet.',
'This recovery phrase is not a valid BIP39 seed, will be treated as a legacy Parity brain wallet.',
valid: false
};
}
Expand Down
9 changes: 1 addition & 8 deletions src/util/alertUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ export const alertCopyBackupPhrase = seedPhrase =>
]
);

const alertRisks = (message, onPress) =>
export const alertRisks = (message, onPress) =>
Alert.alert('Warning', message, [
{
onPress,
Expand All @@ -102,13 +102,6 @@ const alertRisks = (message, onPress) =>
}
]);

export const alertInvalidSeedRecovery = (message, navigation) =>
alertRisks(message, () => {
navigation.navigate('AccountPin', {
isNew: true
});
});

export const alertMultipart = onNext =>
alertRisks(
'The payload of the transaction you are signing is too big to be decoded. Not seeing what you are signing is inherently unsafe. If possible, contact the developer of the application generating the transaction to ask for multipart support.',
Expand Down
4 changes: 2 additions & 2 deletions src/util/debounce.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@
*
* @return {any} the debounced function
*/
export function debounce(fn, time) {
let timeout;
let timeout;

export function debounce(fn, time) {
return function() {
const functionCall = () => fn.apply(this, arguments);

Expand Down
2 changes: 1 addition & 1 deletion src/util/suri.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
*/

export function parseSURI(suri) {
const RE_CAPTURE = /^(\w+(?: \w+)*)?(.*)$/;
const RE_CAPTURE = /^([\w ]+(?: +\w*)*)?(.*)$/;
const matches = suri.match(RE_CAPTURE);
let phrase,
parsedDerivationPath,
Expand Down