# [PATCH] Prompt for a username when an HTTP request 401s 4 messages from 2010-04-01 to 2010-04-02. Participants: Scott Chacon, Junio C Hamano. Thread: https://gitlist.dev/t/23292 ## Scott Chacon, 2010-04-01 22:14 Subject: [PATCH] Prompt for a username when an HTTP request 401s Message-ID: URL: https://gitlist.dev/e/m2wd411cc4a1004011514w6d350ac7l15ab6bb1a6be8d89%40mail.gmail.com ``` When an HTTP request returns a 401, Git will currently fail with a confusing message saying that it got a 401, which is not very descriptive. Currently if a user wants to use Git over HTTP, they have to use one URL with the username in the URL (e.g. "http://user@host.com/repo.git") for write access and another without the username for unauthenticated read access (unless they want to be prompted for the password each time). However, since the HTTP servers will return a 401 if an action requires authentication, we can prompt for username and password if we see this, allowing us to use a single URL for both purposes. This patch 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 --- Updated the comments style and the commit message for Junio. http.c | 22 ++++++++++++++++++++-- http.h | 2 ++ remote-curl.c | 2 ++ 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/http.c b/http.c index 4814217..51253e1 100644 --- a/http.c +++ b/http.c @@ -815,7 +815,21 @@ 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 +845,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-02 06:39 Subject: Re: [PATCH] Prompt for a username when an HTTP request 401s Message-ID: <7veiiymk75.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7veiiymk75.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, which is not very > descriptive. > > Currently if a user wants to use Git over HTTP, they have to use one > URL with the username in the URL (e.g. "http://user@host.com/repo.git") > for write access and another without the username for unauthenticated > read access (unless they want to be prompted for the password each > time). However, since the HTTP servers will return a 401 if an action > requires authentication, we can prompt for username and password if we > see this, allowing us to use a single URL for both purposes. Thanks; this illustrates the issue you are trying to solve much easier to see, don't you agree? An obvious enhancement could be to make "http://user@host.com/repo.git" ask for password lazily. Then such a URL can be used even for an access that does not need authentication and the user does not have to prompted for the password each time, which was what you wanted to really solve, no? Actually that could not just be an enhancement, but might be a better alternative solution to the problem, but I haven't thought things through. > Signed-off-by: Scott Chacon > --- > > Updated the comments style and the commit message for Junio. Heh, Message update is never _for_ me. It is to clarify the problem you are trying to solve, so that we can be certain that the proposed patch is the best approach to solve it. > diff --git a/http.c b/http.c > index 4814217..51253e1 100644 > --- a/http.c > +++ b/http.c > @@ -815,7 +815,21 @@ static int http_request(const char *url, void > *result, int target, int options) I fixed this up when I queued the previous version, and you have the same line wrapping problem in this version, which I have fixed, too, before replacing what was queued to 'pu'. I mention this not as a complaint (but I would appreciate if you try to be careful next time, especially if you plan to post more patches and to become a regular contributor), but primarily because it is curious that only the hunk headers are wrapped but not these long lines we see below: > ... > + } else { > + /* > + * git_getpass is needed here because its very likely stdin/stdout are > ... ``` ## Scott Chacon, 2010-04-02 15:43 Subject: Re: [PATCH] Prompt for a username when an HTTP request 401s Message-ID: URL: https://gitlist.dev/e/y2rd411cc4a1004020843we196537ak35ab6006ce28fefe%40mail.gmail.com In-Reply-To: <7veiiymk75.fsf@alter.siamese.dyndns.org> ``` Hey, On Thu, Apr 1, 2010 at 11:39 PM, Junio C Hamano wrote: > An obvious enhancement could be to make "http://user@host.com/repo.git" > ask for password lazily.  Then such a URL can be used even for an access > that does not need authentication and the user does not have to prompted > for the password each time, which was what you wanted to really solve, no? > > Actually that could not just be an enhancement, but might be a better > alternative solution to the problem, but I haven't thought things > through. Actually, what I want to do is be able to show a single URL that will work for everyone - both read-only and read-write users, so I much prefer the way I wrote the patch. This way, GitHub and kernel.org and whomever else can just publish the one url and it will prompt if they need auth for some reason, and just work if not. I do however agree that if someone _does_ put their username in the url that it should only prompt for the password if it 401s. That should probably be a separate patch, though. Scott ``` ## Junio C Hamano, 2010-04-02 16:11 Subject: Re: [PATCH] Prompt for a username when an HTTP request 401s Message-ID: <7viq89bzrx.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7viq89bzrx.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Scott Chacon writes: > I do however agree that if someone _does_ put their username in the > url that it should only prompt for the password if it 401s. That > should probably be a separate patch, though. Oh, absolutely. Thanks for a clear explanation. ```