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

Move password verification to the final step #1142

Conversation

lontivero
Copy link
Collaborator

@lontivero lontivero commented Feb 4, 2019

This is a proposal for discussion regarding #1128. The hypothesis here is that users simply forget the passwords because they believe that it is not really important and that they can always recover the wallet with the seed.

The fact that it is clearly stated that password is super important is irrelevant because studies are clear: people DO NOT read. So, in this case, after a user back up the seed he/she is required to reenter the password and check the password match and also make sure the encrypted secret can be recovered with path password (this is an absolutely paranoid checking but anyways)

The PR is for discussion and not for merging.

@nopara73
Copy link
Contributor

nopara73 commented Feb 5, 2019

I tested it. I like it. Concept ACK.

@lontivero
Copy link
Collaborator Author

Okay then, what do you think I should do? What about adding the "See password" button?

@nopara73
Copy link
Contributor

nopara73 commented Feb 9, 2019

That's a good idea, but I don't think we should mix it with this PR.
@lontivero Did you not notice that this PR doesn't let you generate a wallet?

@@ -11,7 +11,14 @@
<TextBlock Text="You can recover your wallet on any computer with:" FontWeight="Bold" />
<TextBlock Text="- your mnemonic words AND" FontWeight="Bold" />
<TextBlock Text="- your password" FontWeight="Bold" />
<TextBlock Text="Unlike most other wallets if an attacker aquires your mnemonic words, it will not be able to hack your wallet without knowing your password. However, unlike other wallets, you will not be able to recover your wallet only with your mnemonic words if you lose your password." TextWrapping="Wrap" />
<TextBlock Text="Unlike most other wallets if an attacker aquires your mnemonic words, it will not be able to hack your wallet without knowing your password. However, unlike other wallets, you will not be able to recover your wallet only with your mnemonic words if you lose your password. For that reason, verify the password is correct and back it up." TextWrapping="Wrap" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I should write something like this:
Backup your password too! Unlike most other wallets if an attacker acquires your mnemonic words, it will not be able to hack your wallet without knowing your password. Only with the mnemonic words you will not be able to recover your wallet! For that reason, verify the password is correct and back it up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Too long, don't read. This is true for the original text, too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree. What do we do with that text? Should I rollback to the original text?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure at this point, I will look at the new UX workflow after the functionality is working.


try
{
encryptedSecret.GetSecret(Password + "trolo");
Copy link
Contributor

Choose a reason for hiding this comment

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

only for debug right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes. This PR was pushed for discussing the pros/cons of the idea with something real (with code) and to get feedback, i was never for merging. Anyway, if this is good then I will remove this failing trick.

@lontivero lontivero force-pushed the Features/Check-Password-Recovery branch from 4e56050 to 7a09bb2 Compare February 9, 2019 14:51
@nopara73
Copy link
Contributor

nopara73 commented Feb 9, 2019

Please make a mergable version, so I can review properly.

@nopara73 nopara73 mentioned this pull request Feb 11, 2019
2 tasks
@lontivero
Copy link
Collaborator Author

The #1152 is better that this so, I am closing it.

@lontivero lontivero closed this Feb 14, 2019
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.

None yet

3 participants