Separate SSH prompting from ENV logic #23737
Conversation
Code should be logically identical to the previous revision.
The credential code is very careful not to modify the saved credentials until the end of the callback. This was the only exception.
| end | ||
| if !isfile(privatekey) | ||
| response = Base.prompt("Private key location for '$prompt_url'", | ||
| default=privatekey) |
There was a problem hiding this comment.
Might as well fix the spacing to better fit with the existing base julia style (e.g., default=privatekey lined up with opening parenthesis).
There was a problem hiding this comment.
I ended up leaving the indentation for the default keyword as is as it matches the winprompt indentation.
| # typically this will just annoy users. | ||
| if !isfile(publickey) && isfile(privatekey) | ||
| response = Base.prompt("Public key location for '$prompt_url'", | ||
| default=publickey) |
| @@ -1825,7 +1825,6 @@ mktempdir() do dir | |||
| # User provides an empty private key which triggers a re-prompt | |||
| challenges = [ | |||
| "Private key location for '[email protected]':" => "\n", | |||
| "Public key location for '[email protected]' [.pub]:" => "\n", | |||
There was a problem hiding this comment.
I understand that this test doesn't make sense now that we're only checking for the public key when the private key is provided, but don't we still need to check the public key prompting when a valid private key is provided?
There was a problem hiding this comment.
There is one test I can add for public keys at this point. There's one round left of refactoring that will allow me to add a few more tests which currently cause the credential_loop to infinite loop.
base/libgit2/callbacks.jl
Outdated
| if Sys.iswindows() | ||
| response = Base.winprompt( | ||
| "Your SSH Key requires a password, please enter it now:", | ||
| "Passphrase required", privatekey; prompt_username = false) |
There was a problem hiding this comment.
Issue here is prompt_username = false?
There was a problem hiding this comment.
Yeah, and a few others. Looking at other places in base I'm guessing it should be.
response = Base.winprompt("Your SSH Key requires a password, ",
"please enter it now: Passphrase required",
privatekey; prompt_username=false)There was a problem hiding this comment.
One of the more extreme cases of this is in authenticate_userpass:
response = Base.winprompt("Please enter your credentials for '$prompt_url'", "Credentials required",
isempty(username) ? p.username : username; prompt_username = true)which would become the following in order to fit within the 92-limit:
response = Base.winprompt("Please enter your credentials for '$prompt_url'",
"Credentials required",
isempty(username) ? p.username : username;
prompt_username=true)I think this is also acceptable in Base (I've been allowed to do this before):
response = Base.winprompt(
"Please enter your credentials for '$prompt_url'", "Credentials required",
isempty(username) ? p.username : username; prompt_username=true)I'll note that the last example just slightly exceeds the 92-limit but I think is probably the most readable.
I'll try to strike a balance.
Note: currently one the tests cannot be run.
|
I'll merge once the CI passes. |
Separating some of the SSH callback logic so that
allow_promptonly enables/disables interactive user prompting. Other changes include:Part of #20725.