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

feat: provide password feedback #5111

Merged

Conversation

AaronFeickert
Copy link
Collaborator

@AaronFeickert AaronFeickert commented Jan 12, 2023

Description

Adds wallet password complexity feedback. Allows empty passwords. Adds a warning indicating that password changing functionality is not yet implemented. Adds tests.

Closes issue 5101.

Motivation and Context

The only check on a wallet password is that it not be empty. This introduces two issues:

  • The user has no feedback on the practical strength of their password.
  • The user may specifically wish not to set a password for fail-available backups.

This PR uses the zxcvbn password complexity library to score a password and provide actionable feedback to the user. When the user enters a new password or changes their password, feedback is displayed if applicable. This is informational; the user is free to ignore the feedback if they wish.

Further, the user is now allowed to set an empty password, which may be desired for backups that must fail available. A warning is displayed if this happens.

Finally, a warning message is displayed during the password changing process to indicate that this functionality is incomplete.

How Has This Been Tested?

Existing CI passes. New tests pass. Tested manually for new wallets.

@AaronFeickert
Copy link
Collaborator Author

This presents a somewhat suboptimal user flow. If the user enters a weak password, sees the feedback, and then wishes to try a better password, they need to restart the password changing flow.

Another option is to provide feedback prior to the confirmation prompt and return to the initial prompt. If the user enters the same password, continue to the confirmation prompt without feedback. This adds no additional steps to the case where the password is strong (as there is no feedback), but adds an additional password entry step to the case where the password is weak (and feedback is presented). Not sure if this is preferable, or just annoying to the user.

@CjS77 CjS77 added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Jan 12, 2023
@AaronFeickert AaronFeickert force-pushed the password-feedback branch 2 times, most recently from 78b75b3 to 648f0f8 Compare January 18, 2023 02:47
@CjS77 CjS77 added the P-merge Process - Queued for merging label Jan 18, 2023
@CjS77 CjS77 removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Jan 18, 2023
@stringhandler stringhandler merged commit a568e04 into tari-project:development Jan 18, 2023
Copy link
Collaborator

@CjS77 CjS77 left a comment

Choose a reason for hiding this comment

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

+1

@AaronFeickert AaronFeickert deleted the password-feedback branch January 18, 2023 16:28
SWvheerden pushed a commit that referenced this pull request Mar 31, 2023
Description
---
Improves the flow for setting or changing a passphrase.

Closes [issue 5127](#5127).

Motivation and Context
---
When setting or
[changing](#5175) a wallet
passphrase, the console wallet provides
[feedback](#5111) on the
strength of the provided passphrase. In the case of a weak passphrase,
it does not prompt the user to choose a better one.

This PR implements a better flow for this process, as shown in [this
flowchart](#5127 (comment)).

How Has This Been Tested?
---
Tested manually.

What process can a PR reviewer use to test or verify this change?
---
Testing needs to be done manually to assert that the process represented
by the linked flowchart is implemented. Manual testing should cover the
entire flow for these two operations:
- Setting the passphrase for a new wallet
- Changing the passphrase for an existing wallet

Breaking Changes
---
None.
agubarev pushed a commit to agubarev/tari that referenced this pull request Mar 31, 2023
Description
---
Improves the flow for setting or changing a passphrase.

Closes [issue 5127](tari-project#5127).

Motivation and Context
---
When setting or
[changing](tari-project#5175) a wallet
passphrase, the console wallet provides
[feedback](tari-project#5111) on the
strength of the provided passphrase. In the case of a weak passphrase,
it does not prompt the user to choose a better one.

This PR implements a better flow for this process, as shown in [this
flowchart](tari-project#5127 (comment)).

How Has This Been Tested?
---
Tested manually.

What process can a PR reviewer use to test or verify this change?
---
Testing needs to be done manually to assert that the process represented
by the linked flowchart is implemented. Manual testing should cover the
entire flow for these two operations:
- Setting the passphrase for a new wallet
- Changing the passphrase for an existing wallet

Breaking Changes
---
None.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs P-merge Process - Queued for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add passphrase complexity feedback
3 participants