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

Ask for wallet file password twice #61

Open
lrettig opened this issue Jul 11, 2023 · 8 comments · May be fixed by #62
Open

Ask for wallet file password twice #61

lrettig opened this issue Jul 11, 2023 · 8 comments · May be fixed by #62
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed UX user experience

Comments

@lrettig
Copy link
Member

lrettig commented Jul 11, 2023

And make sure both match

@lrettig lrettig added enhancement New feature or request UX user experience labels Jul 11, 2023
0xjac added a commit to 0xjac/smcli that referenced this issue Jul 12, 2023
@0xjac 0xjac linked a pull request Jul 12, 2023 that will close this issue
@lrettig lrettig added good first issue Good for newcomers help wanted Extra attention is needed labels Jul 12, 2023
0xjac added a commit to 0xjac/smcli that referenced this issue Jul 12, 2023
@monikasmolarek
Copy link

@lrettig would it be ok to add any kind of notice, that if we want to take the wallet created via smcli and open it in smapp, we should set a password? Otherwise, we better keep the mnemonics safe or there's no chance to open it in smapp without a password.

@lrettig
Copy link
Member Author

lrettig commented Nov 20, 2023

Does smapp require a password? It's best to harmonize the behavior of smapp and smcli as much as possible. IMHO a password should be recommended but not required (which is how smcli currently behaves).

@monikasmolarek
Copy link

@brusherru @maparr What do you think Guys?

@monikasmolarek
Copy link

I think smapp uses passwords too much to now remove this requirement. For example, we have to validate the modifications with password if we want to add additional accounts, rename accounts, add/edit/remove contact etc. Of course restoring/opening wallets from files also requires a password.
Of course it might be doable, but the point is that we are supposed to have Smapp 2 and not invest too much time and effort in the current version of the app...so I thought that adding 2 sentences of a warning would be enough for now. Especially taking into account that it might be quite an edge case. If someone uses cli, he will probably not want to switch to smapp, and even if so, he can use the mnemonics to restore the wallet.

@brusherru
Copy link
Member

IMHO a password should be recommended but not required

What is the point?

I'm afraid that then we'll have a lot of people who may lose their funds and when they discover that this is because they use the empty string as a password — they will be very angry at developers who "do not care enough" (at least from their point of view) about their safety. Especially since Smapp is focused on the common people, that may be not familiar with crypto, encryption, and basic security principles.

However, if smcli makes it possible to create a wallet with an "empty password" (but it still encrypts it) — it will be super easy to allow unlocking the wallet with an empty password (just get rid of "at least 1 char length" validation) :)

Due to security reasons, we also do not store a password in the memory and ask for it every time the User wants to change something in the wallet file.

@maparr
Copy link

maparr commented Nov 21, 2023

For smcli, the behavior is similar in that it prompts for a password, which may be empty, and also requests a password during the decryption of the wallet file.

I believe this approach is acceptable for smcli, considering it's intended for professional users. However, smapp is designed for a wider range of users, and it would be better to ensure their safety.

@lrettig
Copy link
Member Author

lrettig commented Nov 21, 2023

then we'll have a lot of people who may lose their funds

just to be clear, this can only happen if the wallet file itself is leaked.

for now let's not overthink it - I think we can leave the behavior of both smapp and smcli as-is. what happens if someone tries to open a passwordless smcli wallet in smapp right now?

I think the very best UX pattern here would be:

  • user tries to import passwordless wallet file from smcli into smapp
  • smapp detects this (it automatically tries to decrypt using the empty string)
  • smapp prompts the user to create a password ("this wallet was created without a password, but smapp requires a password to protect your wallet file"), then saves the same wallet in a new wallet file protected with a password

something like that.

@maparr
Copy link

maparr commented Nov 21, 2023

I think the very best UX pattern here would be:

We could consider enabling users to unlock without a password, although this will lead to a persistent password modal in the Settings tab. However, I am confident that we can effectively resolve this annoying issue in the new version of the app."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed UX user experience
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants