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

fix: recovery passphrase flow #5877

Merged

Conversation

AaronFeickert
Copy link
Collaborator

@AaronFeickert AaronFeickert commented Oct 26, 2023

Description

Switches the handling of wallet recovery passphrase entry to require confirmation and provide complexity analysis.

Closes #5859.

Motivation and Context

When recovering a wallet from a seed, the user is prompted to enter a passphrase without confirming it, and the passphrase complexity is not checked. That is, the flow is the same for wallet recovery as for opening an existing wallet, which is incorrect.

This PR switches the flow to that of a new wallet. When the user enters a passphrase, they must confirm it, and its complexity is analyzed to ensure the user is informed of the safety of their chosen passphrase.

Note that each case (new wallet, wallet recovery, existing wallet) is now handled separately. This is mildly redundant, but allows for better logging and makes the intent more clear.

How Has This Been Tested?

Tested manually.

What process can a PR reviewer use to test or verify this change?

Assert that the passphrase handling logic is now correct. Manually test empty, weak, and strong passphrases in the interface, including the case where the user has a weak or empty passphrase and confirms this is their intent.

@ghpbot-tari-project ghpbot-tari-project 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 Oct 26, 2023
@AaronFeickert AaronFeickert marked this pull request as draft October 26, 2023 21:36
@github-actions
Copy link

Test Results (CI)

1 240 tests   1 240 ✔️  11m 23s ⏱️
     39 suites         0 💤
       1 files           0

Results for commit 9555364.

@github-actions
Copy link

github-actions bot commented Oct 26, 2023

Test Results (Integration tests)

34 tests  +34   34 ✔️ +34   13m 16s ⏱️ + 13m 16s
11 suites +11     0 💤 ±  0 
  2 files   +  2     0 ±  0 

Results for commit 9555364. ± Comparison against base commit 4947df5.

♻️ This comment has been updated with latest results.

@AaronFeickert AaronFeickert marked this pull request as ready for review October 26, 2023 22:09
@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Oct 27, 2023
@SWvheerden SWvheerden merged commit 4159b76 into tari-project:development Oct 27, 2023
15 checks passed
@AaronFeickert AaronFeickert deleted the recovery-passphrase branch October 27, 2023 15:18
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No warning when using weak or empty password on account recovery
3 participants