Re: [PATCH v2 3/3] http: when proxy url has username but no password, ask for password
On 03/01/2012 10:58 PM, Jeff King wrote:
Show 21 quoted lines
> On Thu, Mar 01, 2012 at 09:49:16AM -0800, Sam Vilain wrote:
>
>>> if (curl_http_proxy) {
>>> - curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>>> + credential_from_url(&proxy_auth, curl_http_proxy);
>>> + if (proxy_auth.username != NULL&& proxy_auth.password == NULL) {
>>> + /* proxy string has username but no password, ask for password */
>>> + struct strbuf pbuf = STRBUF_INIT;
>>> + credential_fill(&proxy_auth);
>>
>> Wouldn't it be better to wait until the proxy returns a 403 before
>> assuming that the proxy setting is incorrect/missing a password?
>> What if the administrator expects the user to fill in both the
>> username and password? That is the behaviour of a web browser.
>>
>> Also, I think you should wait until that 403 to detect whether the
>> proxy setting came from the environment, and only load it explicitly
>> then.
>
> It's worth looking at what the http auth code does here.
> Show 27 quoted lines
> overhaul it.
>
> Complicating all of this is the fact that I think Nelson's original
> patch was based on an older, pre-986bbc0 version of git, which is why he
> followed the pre-prompt route, copying the style of regular http auth.
>
> So there's the history lesson. What should proxy auth do?
>
> 1. Definitely respond to HTTP 407 by prompting on the fly; this code
> should go along-side the HTTP 401 code in http.c.
>
> 2. Definitely do the pre-prompt thing when http_proactive_auth is set
> (which is used only by http-push). Unless somebody really feels
> like re-writing http-push to handle retries for authentication.
>
> 3. Consider doing the pre-prompt thing when http_proactive_auth is not
> set. This can save a round-trip, but we should not do it if there
> is a good reason not to. The two possible reasons I can think of
> are:
>
> a. Like http auth, if curl will read the proxy credentials from
> .netrc, then we should not do it for the same reasons
> mentioned in 986bbc0.
>
> b. If people realistically have proxy URLs with usernames but do
> _not_ want to ask for a password, then the prompt will be
> annoying. I'm not sure that anybody expects that.
So, trying to sum up, I will try to redo patch-set as follows:
- Ignore PATCH 2/3 , that is, we won't read any env var.
- Let cURL try to connect and if that fails with 407 , then do a credential_fill
and try to reconnect.
Is that ok? or do I need to do something more?