spacemeshos / smcli

Spacemesh command-line tool
https://spacemesh.io/
MIT License
8 stars 9 forks source link

Ask for wallet file password twice #61

Open lrettig opened 9 months ago

lrettig commented 9 months ago

And make sure both match

monikasmolarek commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

@brusherru @maparr What do you think Guys?

monikasmolarek commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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:

something like that.

maparr commented 5 months ago

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."