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

Remove Chinese character masking from PasswordBox #2187

Merged
merged 28 commits into from Sep 9, 2019

Conversation

@molnard
Copy link
Collaborator

commented Aug 28, 2019

New times, new decisions. There was/is a mass public demand to remove the "confusing" Chinese masking from the PasswordBox, so now we become the leads @lontivero and me decided to let this happen.

There were no standard methods to create this kind of specially masked pwbox so it was a pimped Textbox. Now I take the chance to remove the "hack" and go with the standards, so completely refactor this control.

Closes: #2167
Closes: #2178
Fixes: #2158
Fixes: #1976

  • Daemon mode check.
  • Press space at the end and see warning message.
  • Long password.
  • Paste password.
  • Set visibility of password (New behavior there!).

@molnard molnard changed the title Remove Chinese character masking from NoparaPasswrodBox Remove Chinese character masking from PasswordBox Aug 28, 2019

WalletWasabi.Gui/Controls/LucasPasswordBox.cs Outdated Show resolved Hide resolved
WalletWasabi.Gui/Controls/LucasPasswordBox.cs Outdated Show resolved Hide resolved
WalletWasabi.Gui/Controls/LucasPasswordBox.cs Outdated Show resolved Hide resolved
WalletWasabi.Gui/Controls/LucasPasswordBox.cs Outdated Show resolved Hide resolved

@molnard molnard changed the title Remove Chinese character masking from PasswordBox [WIP] Remove Chinese character masking from PasswordBox Aug 28, 2019

@lontivero
Copy link
Collaborator

left a comment

Didn't test. This same PR should remove the NoparaPasswordBox, I think.

WalletWasabi.Gui/App.xaml Outdated Show resolved Hide resolved
molnard added 16 commits Aug 29, 2019

@molnard molnard changed the title [WIP] Remove Chinese character masking from PasswordBox Remove Chinese character masking from PasswordBox Aug 29, 2019

@molnard molnard requested review from danwalmsley and jmacato Aug 29, 2019

@molnard molnard added this to In Progress in v1.1.7 via automation Aug 29, 2019

@lontivero

This comment has been minimized.

Copy link
Collaborator

commented Sep 3, 2019

@molnard, I have to insist in my suggestion to avoid using the red color in the passwordbox because it is scary, what make the UX worse and it could potentially to raise the need of support. Red color should be used for error messages only.

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Sep 4, 2019

@molnard, I have to insist in my suggestion to avoid using the red color in the passwordbox because it is scary, what make the UX worse and it could potentially to raise the need of support. Red color should be used for error messages only.

Fully agree.

molnard and others added 6 commits Sep 4, 2019
Update WalletWasabi.Gui/Tabs/WalletManager/GenerateWalletViewModel.cs
Co-Authored-By: yahiheb <52379387+yahiheb@users.noreply.github.com>
Update WalletWasabi.Gui/Controls/WalletExplorer/CoinJoinTabView.xaml
Co-Authored-By: yahiheb <52379387+yahiheb@users.noreply.github.com>
Update WalletWasabi.Gui/Controls/TogglePasswordBox.xaml
Co-Authored-By: yahiheb <52379387+yahiheb@users.noreply.github.com>
Update WalletWasabi.Gui/Controls/TogglePasswordBox.xaml
Co-Authored-By: yahiheb <52379387+yahiheb@users.noreply.github.com>
Update WalletWasabi.Gui/Controls/TogglePasswordBox.xaml
Co-Authored-By: yahiheb <52379387+yahiheb@users.noreply.github.com>
@molnard

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 4, 2019

@molnard, I have to insist in my suggestion to avoid using the red color in the passwordbox because it is scary, what make the UX worse and it could potentially to raise the need of support. Red color should be used for error messages only.

Fully agree.

There is no way to add multiple validation results with different severity (error or warning). The different colorings are also missing. Currently, we have the simplest and the most basic implementation of validate-messages which is one string. I will consult with the UX team and try to figure out how to implement this.

@molnard

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 4, 2019

So we agreed on dev meeting to merge this without to have the yellow warning message which will be implemented in a separate PR before the next release. Are there any other issues other than this to solve?

@yahiheb

This comment has been minimized.

Copy link
Collaborator

commented Sep 4, 2019

I tried to generate a new wallet with a password that has a trailing space, the generate button will be disabled.
Then I tried to test a previous wallet that has a password with a trailing space. Load Wallet and Test Password buttons are not disabled. Shouldn't we disable them?
If you enter the password with a trailing space and click Test Password it says Correct Password. so there is trim here.

@yahiheb

This comment has been minimized.

Copy link
Collaborator

commented Sep 4, 2019

If you want to send bitcoin and you enter a correct password with a trailing space:

  1. Send Transaction button is not disabled. maybe it should be.
  2. if you click Send Transaction there is an exception
@molnard

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 4, 2019

If you want to send bitcoin and you enter a correct password with a trailing space:

  1. Send Transaction button is not disabled. maybe it should be.
  2. if you click Send Transaction there is an exception

It is intended. My conventions are:

When verifying a password (Send, CoinJoin, Test, Info)

  • Accept spaces - but let the user know it (warning)
  • Accept compatibility password (OSX paste problem) - but let the user know (error)

When generating a wallet

  • User cannot use a non-correctly formatted password.

Goals

Compatibility: the user can use their original passwords even if it was not correctly formatted. Wasabi will accept it but it will let them know about the issue.

Recoverability: if a trimmed or buggy password was used when the wallet was generated it won't be recoverable with the original password in another software, neither in wasabi in some cases. So I always let the user know that his password was modified in some way or another.

Update WalletWasabi.Gui/Tabs/WalletManager/GenerateWalletViewModel.cs
Co-Authored-By: yahiheb <52379387+yahiheb@users.noreply.github.com>

@nopara73 nopara73 merged commit c13ae81 into zkSNACKs:master Sep 9, 2019

3 of 4 checks passed

Wasabi.Linux #20190905.3 failed
Details
CodeFactor No issues found.
Details
Wasabi.Osx #20190904.21 succeeded
Details
Wasabi.Windows #20190904.21 succeeded
Details

v1.1.7 automation moved this from In Progress to Done Sep 9, 2019

@MaxHillebrand

This comment has been minimized.

Copy link
Collaborator

commented Sep 19, 2019

I'm afk for a couple blocks, and then you introduce a critical bug breaking the best feature of Wasabi, oh great... 😭

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
7 participants
You can’t perform that action at this time.