Re: [RFC/PATCH] http-push: don't always prompt...

Re: [RFC/PATCH] http-push: don't always prompt for password

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:52:22

Stefan Näwe [off-list ref] writes:

quoted hunk

Am 01.11.2011 19:12, schrieb Junio C Hamano:

quoted

There are only handful of commits that even remotely touch http related
codepath between v1.7.7 and v1.7.8-rc0:

  * deba493 http_init: accept separate URL parameter

  This could change the URL string given to http_auth_init().
... 
Could you try reverting deba493 and retest, and then if the behaviour is
the same "need ENTER", further revert 070b4dd and retest?

I did a little more testing.
This WIP makes it work for me (i.e. "need ENTER" is gone, works with
and without .netrc, with 'https://host/repo.git' and 
'https://user@host...' URL). Needs testing, of course.

---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---

diff --git a/http.c b/http.c
index a4bc770..008ad72 100644
--- a/http.c
+++ b/http.c

@@ -279,8 +279,6 @@ static CURL *get_curl_handle(void)
        curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
 #endif

-       init_curl_http_auth(result);
-
        if (ssl_cert != NULL)
                curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
        if (has_cert_password())

@@ -846,7 +844,7 @@ static int http_request(const char *url, void *result, int target, int options)
                else if (missing_target(&results))
                        ret = HTTP_MISSING_TARGET;
                else if (results.http_code == 401) {
-                       if (user_name) {
+                       if (user_name && user_pass) {
                                ret = HTTP_NOAUTH;
                        } else {
                                /*

@@ -855,7 +853,8 @@ static int http_request(const char *url, void *result, int target, int options)
                                 * but that is non-portable.  Using git_getpass() can at least be stubbed
                                 * on other platforms with a different implementation if/when necessary.
                                 */
-                               user_name = xstrdup(git_getpass_with_description("Username", description));
+                               if (!user_name)
+                                       user_name = xstrdup(git_getpass_with_description("Username", description));
                                init_curl_http_auth(slot->curl);
                                ret = HTTP_REAUTH;
                        }
---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---

This defers the calls to git_getpass* until we get 401 from the server
side.

I am guessing the reason why in the current code get_curl_handle() has a
call to init_curl_http_auth() very early, but only when user_name is set,
is because it is likely for a site to require authentication when the user
already has "username@" in its URL, and doing it this way will avoid the
extra round-trip because by the time we make an HTTP request, we have both
name and pass. If we apply this patch, the check in init_curl_http_auth()
that asks for the password only when user_name is set becomes unnecessary.

I think the second hunk at l.846 sort of makes sense, but not quite.

"We got 401 even though we know we have supplied name and pass" is a valid
criterion to decide that the name/pass is an invalid combination. But it
makes me wonder if this code in its early days guaranteed whenever we have
user_name we always have made sure we have user_pass (otherwise by asking
for it with git_getpass) and that is the reason why it had to check only
for user_name, and if that is the case perhaps the real breakage is we are
not keeping that guarantee in the current code?

IOW, when in the current code do we make a new HTTP request while
user_name is set but no user_pass?  Perhaps if we fix that codepath we do
not have to have this extra 401 roundtrip this patch is introducing when
the username (but not password) is given?

Peff, what do you think?

Re: [RFC/PATCH] http-push: don't always prompt for password

From: Jeff King <hidden>
Date: 2016-06-15 22:52:22

On Wed, Nov 02, 2011 at 10:13:32AM -0700, Junio C Hamano wrote:

This defers the calls to git_getpass* until we get 401 from the server
side.

I am guessing the reason why in the current code get_curl_handle() has a
call to init_curl_http_auth() very early, but only when user_name is set,
is because it is likely for a site to require authentication when the user
already has "username@" in its URL, and doing it this way will avoid the
extra round-trip because by the time we make an HTTP request, we have both
name and pass. If we apply this patch, the check in init_curl_http_auth()
that asks for the password only when user_name is set becomes unnecessary.

Yeah, that was my reading, as well. I was tempted to do away with it, as
it makes the code much simpler, at the cost of doing that extra
round-trip. However, most browsers do that round-trip, and I don't think
it's that big a deal.

In the end I decided not to switch it just because I wanted to be as
minimally invasive as possible, just in case somebody did care about the
round trip.

I think the second hunk at l.846 sort of makes sense, but not quite.

"We got 401 even though we know we have supplied name and pass" is a valid
criterion to decide that the name/pass is an invalid combination. But it
makes me wonder if this code in its early days guaranteed whenever we have
user_name we always have made sure we have user_pass (otherwise by asking
for it with git_getpass) and that is the reason why it had to check only
for user_name, and if that is the case perhaps the real breakage is we are
not keeping that guarantee in the current code?

All of my patches attempted to keep that condition, as well (because
credential_fill tries to do so). But I didn't think about it during the
re-roll of the patches that are in master now, so maybe that wasn't
kept.

It seems to me that Stefan's patch actually causes that (because he
removes the early "set password if we have a username" logic). But I'll
take another look at what's in master.

-Peff

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help