Add: password entry verification to prevent password mistype #662
Conversation
|
Thanks, approved. The final word is @jaraco's, he may have his own comments. |
|
@mitya57 diffcov failed in this case. What do you think i might need to change. |
|
Well, if you manage to write a test for this code, that would be awesome. If no, then I think we will have to ignore that warning. |
jaraco
force-pushed
the
passwordVerification
branch
from
8eda425
to
e277eb3
Compare
| f"password for '{self.username}' in '{self.service}': " | ||
| ) | ||
|
|
||
| reentered_password = self.input_password( |
There was a problem hiding this comment.
I've started work on tests and run into an issue. The input_password method is coded to accept the input from a pipe (if present). This change of expecting two passwords when it's piped in is unacceptable. Probably instead we'll want to hook this logic in at getpass.getpass instead of wrapping input_password.
There was a problem hiding this comment.
In #663, I'm working on adding some tests to capture at least the current expectation and hopefully serve as a foundation for testing this more sophisticated behavior. Interestingly, those tests didn't seem to capture the concern I mentioned above, but I expected them to, so I'll need to think about it a bit more before proceeding.
I've also worked on some refactorings in the feature/password-verification branch. I'd pushed them to this PR, but decided to leave them in their own branch for now while working out this issue.
There was a problem hiding this comment.
Sure. Please let me know if you need help with any other issues as well. I'll be more than happy to contribute.
There was a problem hiding this comment.
Maybe we should ask to re-enter password only if sys.stdin.isatty()?
There was a problem hiding this comment.
Yeah I think that works. I think it would be a good idea to skip verification for if not typed in explicitly. I can make this change if it's fine with @jaraco. I would imagine the piped password would already be visual to the user in some form so the verification would not make much sense in that case.
There was a problem hiding this comment.
Hi @jaraco. Any update on this?
There was a problem hiding this comment.
Sorry for the delay. Yes, please proceed with drafting an implementation that avoids the re-prompt for non-interactive cases.
Added a password retype mechanism that helps user verify the password in case the password is mistyped. The number of trials for incorrect passoword attempts is set to 3 for now.