cmd: Add passphrase confirmation when creating wallet from existing seed #3303
Conversation
There was a problem hiding this comment.
Thanks for the PR @carlaKC! The new logic looks good to me, but I think we should consider moving this into the branch where the seed is created (!hasMnemonic). When using an existing seed, that seed already has a chosen password so confirmation is not necessary, we are just trying to decrypt it. Currently when creating a seed if the passwords don't match on the first time, we'll exit. Adding this for loop there will make it easier if people mess up their first attempt.
cfromknecht
added
authentication
enhancement
|
Okay got it, I wasn't super clear on what needed to be done from the original issue, will update :) While I'm at it, I'm wondering if it's worth adding similar password retry to creating the wallet password? Not sure if endless password loops everywhere are ideal, but adding it there would make the whole flow consistent? |
carlaKC
force-pushed
the
ckc-I1507-lnclipasswordretry
branch
from
3c6bb39 to
f731c4d
Compare
|
@carlaKC sure that sounds great! should be able to reuse a good amount of code between the two :) |
carlaKC
force-pushed
the
ckc-I1507-lnclipasswordretry
branch
from
f731c4d to
3ffa38e
Compare
cmd/lncli/commands.go
Outdated
| @@ -1650,6 +1612,50 @@ mnemonicCheck: | |||
| return nil | |||
| } | |||
|
|
|||
| func capturePassword(instruction, noun string, optional bool, | |||
carlaKC
force-pushed
the
ckc-I1507-lnclipasswordretry
branch
from
3ffa38e to
21eefe6
Compare
There was a problem hiding this comment.
@carlaKC great refactor! 🏆
Code changes look solid, just have a few formatting comments and then should be good to go
cmd/lncli/commands.go
Outdated
| @@ -1650,6 +1612,56 @@ mnemonicCheck: | |||
| return nil | |||
| } | |||
|
|
|||
| // capturePassword returns a password value that has been | |||
There was a problem hiding this comment.
Excellent description! The line wrapping on the godoc looks like it breaks before 80 chars, but not a blocker
There was a problem hiding this comment.
Thanks!
I was wrapping at 72 because I remembered seeing it somewhere in the contributions guide. but that was for commit messages 🤦♀
carlaKC
force-pushed
the
ckc-I1507-lnclipasswordretry
branch
from
21eefe6 to
d5b662d
Compare
|
|
||
| // If the password provided is not valid, restart | ||
| // password capture process from the beginning. | ||
| if err := validate(password); err != nil { |
There was a problem hiding this comment.
Good call to do validation before asking to repeat! 😀
Require that users confirm their passphrase when providing their own seed with a passphrase.
Currently implemented as an infinite loop until passwords match, can be limited to a set number of retries if that's the UX wanted.
Fixes #1507