-
Notifications
You must be signed in to change notification settings - Fork 599
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
Add ConsolePasswordFinder to read from Console #338
Add ConsolePasswordFinder to read from Console #338
Conversation
* There was no example `PasswordFinder` to prompt a user for their password
// the request cannot be serviced | ||
return null; | ||
} | ||
return console.readPassword("Enter passphrase for %s:", resource.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make the String a parameter to the class, in that way users can customize the message the user sees.
Also let's add a max number of retries as a parameter.
public class TestConsolePasswordFinder { | ||
|
||
/* | ||
* Note that Mockito 1.9 cannot mock Console because it is a final class, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with upgrading to a newer Mockito version :)
@Test | ||
public void testReqPasswordNullConsole() { | ||
char[] password = new ConsolePasswordFinder(null) | ||
.reqPassword(Mockito.mock(Resource.class)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to mocking the Resource here? You don't verify it's not being called upon...
Nice improvement, few comments before I'll merge it. |
Thanks for the feedback! I should be able to make those changes some time today, and I'll work with upgrading Mockito to see if any other code breaks. |
Ok! I look forward to the updates :) |
* Upgraded Mockito to 2.8.47 (latest) * Added extension to allow mocking final classes * ConsolePasswordFinder allows custom message and number of retries * Added builder for ConsolePasswordFinder * Added more unit tests
This is ready for another look whenever you're free. |
|
||
public ConsolePasswordFinder(Console console, String promptFormat, int maxTries) { | ||
this.console = console; | ||
this.promptFormat = promptFormat; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're not checking the format of the prompt here, but the constructor is public.
I don't think we actually need the builder, it's just 3 parameters. I'm fine with using just a constructor.
In any case we need to do the check here also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! OK, I'll take out the builder (good to have less code to maintain) and put the check here
PasswordFinder
to prompt a user for their password,so this adds that implementation