# [PATCH] Prompt for a username when an HTTP request 401s 3 messages from 2010-04-01 to 2010-04-01. Participants: Scott Chacon, Junio C Hamano. Thread: https://gitlist.dev/t/23290 ## Scott Chacon, 2010-04-01 20:29 Subject: [PATCH] Prompt for a username when an HTTP request 401s Message-ID: URL: https://gitlist.dev/e/o2xd411cc4a1004011329wcbe34a45he7f3d7ce5d4eae59%40mail.gmail.com ``` When an HTTP request returns a 401, Git will currently fail with a confusing message saying that it got a 401. This changes http_request to prompt for the username and password, then return HTTP_REAUTH so http_get_strbuf can try again. If it gets a 401 even when a user/pass is supplied, http_request will now return HTTP_NOAUTH which remote_curl can then use to display a more intelligent error message that is less confusing. Signed-off-by: Scott Chacon --- Here is the fourth version of this patch - now incorporating the GIT_ASKPASS stuff. http.c | 20 ++++++++++++++++++-- http.h | 2 ++ remote-curl.c | 2 ++ 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/http.c b/http.c index 4814217..6027546 100644 --- a/http.c +++ b/http.c @@ -815,7 +815,19 @@ static int http_request(const char *url, void *result, int target, int options) ret = HTTP_OK; else if (missing_target(&results)) ret = HTTP_MISSING_TARGET; - else + else if (results.http_code == 401) { + if (user_name) { + ret = HTTP_NOAUTH; + } else { + // git_getpass is needed here because its very likely stdin/stdout are + // pipes to our parent process. So we instead need to use /dev/tty, + // 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("Username: ")); + init_curl_http_auth(slot->curl); + ret = HTTP_REAUTH; + } + } else ret = HTTP_ERROR; } else { error("Unable to start HTTP request for %s", url); @@ -831,7 +843,11 @@ static int http_request(const char *url, void *result, int target, int options) int http_get_strbuf(const char *url, struct strbuf *result, int options) { - return http_request(url, result, HTTP_REQUEST_STRBUF, options); + int http_ret = http_request(url, result, HTTP_REQUEST_STRBUF, options); + if (http_ret == HTTP_REAUTH) { + http_ret = http_request(url, result, HTTP_REQUEST_STRBUF, options); + } + return http_ret; } /* diff --git a/http.h b/http.h index 5c9441c..2dd03e8 100644 --- a/http.h +++ b/http.h @@ -126,6 +126,8 @@ extern char *get_remote_object_url(const char *url, const char *hex, #define HTTP_MISSING_TARGET 1 #define HTTP_ERROR 2 #define HTTP_START_FAILED 3 +#define HTTP_REAUTH 4 +#define HTTP_NOAUTH 5 /* * Requests an url and stores the result in a strbuf. diff --git a/remote-curl.c b/remote-curl.c index b76bfcb..0782756 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -132,6 +132,8 @@ static struct discovery* discover_refs(const char *service) case HTTP_MISSING_TARGET: die("%s not found: did you run git update-server-info on the" " server?", refs_url); + case HTTP_NOAUTH: + die("Authentication failed"); default: http_error(refs_url, http_ret); die("HTTP request failed"); -- 1.7.0.1 ``` ## Junio C Hamano, 2010-04-01 21:30 Subject: Re: [PATCH] Prompt for a username when an HTTP request 401s Message-ID: <7v39zeoo70.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v39zeoo70.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Scott Chacon writes: > When an HTTP request returns a 401, Git will currently fail with a > confusing message saying that it got a 401. This changes > http_request to prompt for the username and password, then return > HTTP_REAUTH so http_get_strbuf can try again. If it gets a 401 even > when a user/pass is supplied, http_request will now return HTTP_NOAUTH > which remote_curl can then use to display a more intelligent error > message that is less confusing. I'd like another sentence after "that it got a 401.", explaining why it is sometimes OK for us to get 401 and continue, in order to justify that it is a good idea to retry after asking for authentication credentials to the end user when it happens. I am guessing that it might be something like this: The repository owner might have given out an HTTP URL as if it were a public resource (e.g. "http://github.com/myrepo.git/"), and the end user may find out that the URL is not valid and he needs to supply a username (e.g. "http://me@github.com/myrepo.git") in the URL to trigger authentication. Retrying by asking for username and password would help users in such a case. I said "something like this" because I do not think what I wrote above is the whole story. A natural question it begs is "why didn't the repository owner give the right URL to begin with?" Also, earlier I said "sometimes OK", because I don't know if it always OK for us to get 401 and continue. If the end user got a 401 and then does not have a good username or password (e.g. he realizes that the URL he accessed was incorrect), he used to see "you are not allowed to access this repository" with a clean failure, but now he would have to get out of "who are you?" interaction (and how would he do that?). Would that be a problem? If that is not a problem, then the patch looks like a good solution to the problem, and an obvious enhancement that may want to happen would be to add a boolean parameter to git_getpass() in order to control if we want to hide what the user types, as we would probably prefer the Username to be echoed. But that is an independent issue to be addressed as a separate follow-up patch. > Signed-off-by: Scott Chacon > --- > > Here is the fourth version of this patch - now incorporating the > GIT_ASKPASS stuff. > > http.c | 20 ++++++++++++++++++-- > http.h | 2 ++ > remote-curl.c | 2 ++ > 3 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/http.c b/http.c > index 4814217..6027546 100644 > --- a/http.c > +++ b/http.c > @@ -815,7 +815,19 @@ static int http_request(const char *url, void > *result, int target, int options) > ret = HTTP_OK; > else if (missing_target(&results)) > ret = HTTP_MISSING_TARGET; > - else > + else if (results.http_code == 401) { > + if (user_name) { > + ret = HTTP_NOAUTH; > + } else { > + // git_getpass is needed here because its very likely stdin/stdout are > + // pipes to our parent process. So we instead need to use /dev/tty, > + // 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("Username: ")); No C99/C++ "//" comments. /* * We format multi-line * comments like * this. */ Thanks. Tentatively I'll queue this version _without_ any touch-up to 'pu'. ``` ## Scott Chacon, 2010-04-01 22:06 Subject: Re: [PATCH] Prompt for a username when an HTTP request 401s Message-ID: URL: https://gitlist.dev/e/h2vd411cc4a1004011506p4d2db46bjafbe22f702b0f869%40mail.gmail.com In-Reply-To: <7v39zeoo70.fsf@alter.siamese.dyndns.org> ``` Hey, On Thu, Apr 1, 2010 at 2:30 PM, Junio C Hamano wrote: > > Also, earlier I said "sometimes OK", because I don't know if it always OK > for us to get 401 and continue.  If the end user got a 401 and then does > not have a good username or password (e.g. he realizes that the URL he > accessed was incorrect), he used to see "you are not allowed to access > this repository" with a clean failure, but now he would have to get out of > "who are you?" interaction (and how would he do that?).  Would that be a > problem? They did not get a clean failure, it looks like this: -- $ git clone http://github.dev/defunkt/ambition.git Initialized empty Git repository in /private/tmp/svn/ambition/.git/ error: The requested URL returned error: 401 while accessing http://github.dev/defunkt/ambition.git/info/refs fatal: HTTP request failed -- "401 while accessing X, HTTP request failed" is not really a clean failure. Most people don't know what a 401 is. With this patch, if they get prompted for a user/pass on 401 and it still fails, they see this: -- $ ~/bin/git clone http://github.dev/defunkt/ambition.git Initialized empty Git repository in /private/tmp/svn/ambition/.git/ Username: Password: fatal: Authentication failed -- I think "Authentication failed" is a bit cleaner. I'm sending another patch with the comment format change and message update. Scott ```