8366261: Provide utility methods for sun.security.util.Password #26973
Conversation
|
👋 Welcome back naoto! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
openjdk
bot
added
security
Webrevs
|
| private static final boolean istty = istty(); | ||
| static final int TTY_STDIN_MASK = 0x00000001; | ||
| static final int TTY_STDOUT_MASK = 0x00000002; | ||
| static final int TTY_STDERR_MASK = 0x00000004; |
There was a problem hiding this comment.
I recommend marking these fields as @Native.
| public static Optional<JdkConsoleImpl> passwordConsole() { | ||
| return INSTANCE.orElseSet(() -> { | ||
| // If there's already a proper console, throw an exception | ||
| if (System.console() != null) { |
There was a problem hiding this comment.
Since this initializes the console, this means the console can only be available when there is no provider and stdin is tty, but stdout is not. Is this intended? (In other words, should we make System.console() lazy, as it is initialized with java.io.Console as Console.cons?)
There was a problem hiding this comment.
Yes, this is intentional. This entry is dedicated solely to Password, and it is expected that System.console() will be called beforehand so that normal cases (without redirection) are handled by System.console(). The null check is a safeguard to prevent future Password refactoring from invoking this method without first calling System.console() in the standard case.
There was a problem hiding this comment.
It would be good to add comments that describes this precondition. We've all been discussing it so we "know" what it's doing and why. But other readers, like @liach, and future maintainers of the Password class, will know what the requirements are. (EDIT: I meant "will need to know" but I think you understood.)
There was a problem hiding this comment.
It would be good to add comments that describes this precondition.
Good point. Explained it in its comment.
Co-authored-by: Francesco Andreuzzi <andreuzzi.francesco@gmail.com>
| @@ -21,7 +21,7 @@ | |||
| * questions. | |||
| */ | |||
|
|
|||
| /** | |||
| /* | |||
| * @test | |||
| * @bug 8295803 8299689 8351435 8361613 | |||
There was a problem hiding this comment.
Should 8366261 be included?
| new JdkConsoleImpl( | ||
| Charset.forName(StaticProperty.stdinEncoding(), UTF_8.INSTANCE), | ||
| Charset.forName(StaticProperty.stdoutEncoding(), UTF_8.INSTANCE))) : | ||
| Optional.empty(); |
There was a problem hiding this comment.
(Finally getting back to this.)
The code returns an Optional containing a JdkConsoleImpl instance, or an empty Optional. However the comment above says it returns a JdkConsoleImpl instance or null. Which do we want it to be? Either is OK, but the comment should agree with the code.
I think @wangweij might want to weigh in here since his code will be calling this.
There was a problem hiding this comment.
Ah, yes. Comment does not align with the code. I am fine either way too.
There was a problem hiding this comment.
Either is OK. Optional sounds more modern and forces me to consider the empty case.
Providing a couple of utility methods using the "built-in"
Consoleimplementation to support tools that require password input, such askeytool, ensuring they work even when std0ut is redirected.Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26973/head:pull/26973$ git checkout pull/26973Update a local copy of the PR:
$ git checkout pull/26973$ git pull https://git.openjdk.org/jdk.git pull/26973/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26973View PR using the GUI difftool:
$ git pr show -t 26973Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26973.diff
Using Webrev
Link to Webrev Comment