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 <redacted>
---
I updated this patch to include comments on why I'm using getpass() -
we went through trying to get the info via stdin/stdout and it didn't
work very well because stdout/in are pipes, apparently. Shawn
suggested just using the getpass(). I've inlined it, though, as per
the other suggestion.
http.c | 19 +++++++++++++++++--
http.h | 2 ++
remote-curl.c | 2 ++
3 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/http.c b/http.c
index deab595..6370da4 100644
--- a/http.c
+++ b/http.c
@@ -815,7 +815,18 @@ 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 {
+ // it is neccesary to use getpass here because
+ // there appears to be no other clean way to
+ // read/write stdout/stdin
+ user_name = xstrdup(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 +842,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
Hi,
On Fri, Mar 19, 2010 at 11:41 AM, Scott Chacon [off-list ref] wrote:
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.
(added Daniel to the Cc list)
how are you getting 401s? Recently, git set the CURL_AUTH_ANY option,
so if the correct credentials are passed, curl should have "hid" the
401 from us.
--
Cheers,
Ray Chuan
On Fri, 19 Mar 2010, Tay Ray Chuan wrote:
quoted
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.
how are you getting 401s? Recently, git set the CURL_AUTH_ANY option, so if
the correct credentials are passed, curl should have "hid" the 401 from us.
That's correct. It should hide the 401 in the sense that it should try to
continue and do the correct authentication procedure and only if that fails it
should end up with an actual 401 end result.
--
/ daniel.haxx.se
Daniel Stenberg [off-list ref] wrote:
On Fri, 19 Mar 2010, Tay Ray Chuan wrote:
quoted
quoted
When an HTTP request returns a 401, Git will currently fail with a
confusing message saying that it got a 401.
how are you getting 401s? Recently, git set the CURL_AUTH_ANY option,
so if the correct credentials are passed, curl should have "hid" the
401 from us.
That's correct. It should hide the 401 in the sense that it should try to
continue and do the correct authentication procedure and only if that
fails it should end up with an actual 401 end result.
If the URL didn't contain a username, and the server returns a 401,
Git just aborts with an error.
What Scott is trying to do here is teach Git to request a
username/password if there was no username in the URL and
authentication is required by the server.
In the case of GitHub, this means they can advertise one http:// URL
for the repository. Anonymous fetch just works, and using that same
URL to push will ask for your username/password, and then complete.
--
Shawn.
Scott Chacon [off-list ref] wrote:
quoted hunk
@@ -815,7 +815,18 @@ 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 {
+ // it is neccesary to use getpass here because
+ // there appears to be no other clean way to
+ // read/write stdout/stdin
+ user_name = xstrdup(getpass("Username: "));
No, 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 getpass() can at least be stubbed
on other platforms with a different implementation if/when necessary.
--
Shawn.
Hey,
On Fri, Mar 19, 2010 at 7:32 AM, Shawn O. Pearce [off-list ref] wrote:
Scott Chacon [off-list ref] wrote:
quoted
@@ -815,7 +815,18 @@ 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 {
+ // it is neccesary to use getpass here because
+ // there appears to be no other clean way to
+ // read/write stdout/stdin
+ user_name = xstrdup(getpass("Username: "));
No, 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 getpass() can at least be stubbed
on other platforms with a different implementation if/when necessary.
Should I roll a new patch for this?
Scott
Scott Chacon [off-list ref] wrote:
On Fri, Mar 19, 2010 at 7:32 AM, Shawn O. Pearce [off-list ref] wrote:
quoted
Scott Chacon [off-list ref] wrote:
quoted
@@ -815,7 +815,18 @@ 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 {
+ ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? // it is neccesary to use getpass here because
+ ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? // there appears to be no other clean way to
+ ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? // read/write stdout/stdin
+ ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? user_name = xstrdup(getpass("Username: "));
No, 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 getpass() can at least be stubbed
on other platforms with a different implementation if/when necessary.
Should I roll a new patch for this?
Yea, you probably should since I think the comment could be improved.
--
Shawn.