{"thread":{"id":"23290","subject":"[PATCH] Prompt for a username when an HTTP request 401s","startedAt":"2010-04-01T20:29:04Z","lastAt":"2010-04-01T22:06:47Z","messageCount":3,"participants":["Scott Chacon","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"138358","messageId":"o2xd411cc4a1004011329wcbe34a45he7f3d7ce5d4eae59@mail.gmail.com","threadId":"23290","inReplyTo":null,"subject":"[PATCH] Prompt for a username when an HTTP request 401s","fromName":"Scott Chacon","fromEmail":"schacon@gmail.com","sentAt":"2010-04-01T20:29:04Z","receivedAt":"2010-04-01T20:29:04Z","isPatch":true,"sender":{"key":"schacon@gmail.com","avatar":"https://gravatar.com/avatar/9b13a8a078e1dcf8588c4eea9554445d51ebed6c41b51f56f4d96738130b05c6?d=mp&s=160"},"body":"When an HTTP request returns a 401, Git will currently fail with a\nconfusing message saying that it got a 401.  This changes\nhttp_request to prompt for the username and password, then return\nHTTP_REAUTH so http_get_strbuf can try again.  If it gets a 401 even\nwhen a user/pass is supplied, http_request will now return HTTP_NOAUTH\nwhich remote_curl can then use to display a more intelligent error\nmessage that is less confusing.\n\nSigned-off-by: Scott Chacon <schacon@gmail.com>\n---\n\nHere is the fourth version of this patch - now incorporating the\nGIT_ASKPASS stuff.\n\n http.c        |   20 ++++++++++++++++++--\n http.h        |    2 ++\n remote-curl.c |    2 ++\n 3 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 4814217..6027546 100644\n--- a/http.c\n+++ b/http.c\n@@ -815,7 +815,19 @@ static int http_request(const char *url, void\n*result, int target, int options)\n \t\t\tret = HTTP_OK;\n \t\telse if (missing_target(&results))\n \t\t\tret = HTTP_MISSING_TARGET;\n-\t\telse\n+\t\telse if (results.http_code == 401) {\n+\t\t\tif (user_name) {\n+\t\t\t\tret = HTTP_NOAUTH;\n+\t\t\t} else {\n+\t\t\t\t// git_getpass is needed here because its very likely stdin/stdout are\n+\t\t\t\t// pipes to our parent process.  So we instead need to use /dev/tty,\n+\t\t\t\t// but that is non-portable.  Using git_getpass() can at least be stubbed\n+\t\t\t\t// on other platforms with a different implementation if/when necessary.\n+\t\t\t\tuser_name = xstrdup(git_getpass(\"Username: \"));\n+\t\t\t\tinit_curl_http_auth(slot->curl);\n+\t\t\t\tret = HTTP_REAUTH;\n+\t\t\t}\n+\t\t} else\n \t\t\tret = HTTP_ERROR;\n \t} else {\n \t\terror(\"Unable to start HTTP request for %s\", url);\n@@ -831,7 +843,11 @@ static int http_request(const char *url, void\n*result, int target, int options)\n\n int http_get_strbuf(const char *url, struct strbuf *result, int options)\n {\n-\treturn http_request(url, result, HTTP_REQUEST_STRBUF, options);\n+\tint http_ret = http_request(url, result, HTTP_REQUEST_STRBUF, options);\n+\tif (http_ret == HTTP_REAUTH) {\n+\t\thttp_ret = http_request(url, result, HTTP_REQUEST_STRBUF, options);\n+\t}\n+\treturn http_ret;\n }\n\n /*\ndiff --git a/http.h b/http.h\nindex 5c9441c..2dd03e8 100644\n--- a/http.h\n+++ b/http.h\n@@ -126,6 +126,8 @@ extern char *get_remote_object_url(const char\n*url, const char *hex,\n #define HTTP_MISSING_TARGET\t1\n #define HTTP_ERROR\t\t2\n #define HTTP_START_FAILED\t3\n+#define HTTP_REAUTH\t4\n+#define HTTP_NOAUTH\t5\n\n /*\n  * Requests an url and stores the result in a strbuf.\ndiff --git a/remote-curl.c b/remote-curl.c\nindex b76bfcb..0782756 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -132,6 +132,8 @@ static struct discovery* discover_refs(const char *service)\n \tcase HTTP_MISSING_TARGET:\n \t\tdie(\"%s not found: did you run git update-server-info on the\"\n \t\t    \" server?\", refs_url);\n+\tcase HTTP_NOAUTH:\n+\t\tdie(\"Authentication failed\");\n \tdefault:\n \t\thttp_error(refs_url, http_ret);\n \t\tdie(\"HTTP request failed\");\n-- \n1.7.0.1\n"},{"id":"138363","messageId":"7v39zeoo70.fsf@alter.siamese.dyndns.org","threadId":"23290","inReplyTo":"o2xd411cc4a1004011329wcbe34a45he7f3d7ce5d4eae59@mail.gmail.com","subject":"Re: [PATCH] Prompt for a username when an HTTP request 401s","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-01T21:30:43Z","receivedAt":"2010-04-01T21:30:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Scott Chacon <schacon@gmail.com> writes:\n\n> When an HTTP request returns a 401, Git will currently fail with a\n> confusing message saying that it got a 401.  This changes\n> http_request to prompt for the username and password, then return\n> HTTP_REAUTH so http_get_strbuf can try again.  If it gets a 401 even\n> when a user/pass is supplied, http_request will now return HTTP_NOAUTH\n> which remote_curl can then use to display a more intelligent error\n> message that is less confusing.\n\nI'd like another sentence after \"that it got a 401.\", explaining why it is\nsometimes OK for us to get 401 and continue, in order to justify that it\nis a good idea to retry after asking for authentication credentials to the\nend user when it happens.  I am guessing that it might be something like\nthis:\n\n    The repository owner might have given out an HTTP URL as if it were a\n    public resource (e.g. \"http://github.com/myrepo.git/\"), and the end\n    user may find out that the URL is not valid and he needs to supply a\n    username (e.g. \"http://me@github.com/myrepo.git\") in the URL to\n    trigger authentication.  Retrying by asking for username and password\n    would help users in such a case.\n\nI said \"something like this\" because I do not think what I wrote above is\nthe whole story.  A natural question it begs is \"why didn't the repository\nowner give the right URL to begin with?\"\n\nAlso, earlier I said \"sometimes OK\", because I don't know if it always OK\nfor us to get 401 and continue.  If the end user got a 401 and then does\nnot have a good username or password (e.g. he realizes that the URL he\naccessed was incorrect), he used to see \"you are not allowed to access\nthis repository\" with a clean failure, but now he would have to get out of\n\"who are you?\" interaction (and how would he do that?).  Would that be a\nproblem?\n\nIf that is not a problem, then the patch looks like a good solution to the\nproblem, and an obvious enhancement that may want to happen would be to\nadd a boolean parameter to git_getpass() in order to control if we want to\nhide what the user types, as we would probably prefer the Username to be\nechoed.  But that is an independent issue to be addressed as a separate\nfollow-up patch.\n\n> Signed-off-by: Scott Chacon <schacon@gmail.com>\n> ---\n>\n> Here is the fourth version of this patch - now incorporating the\n> GIT_ASKPASS stuff.\n>\n>  http.c        |   20 ++++++++++++++++++--\n>  http.h        |    2 ++\n>  remote-curl.c |    2 ++\n>  3 files changed, 22 insertions(+), 2 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index 4814217..6027546 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -815,7 +815,19 @@ static int http_request(const char *url, void\n> *result, int target, int options)\n>  \t\t\tret = HTTP_OK;\n>  \t\telse if (missing_target(&results))\n>  \t\t\tret = HTTP_MISSING_TARGET;\n> -\t\telse\n> +\t\telse if (results.http_code == 401) {\n> +\t\t\tif (user_name) {\n> +\t\t\t\tret = HTTP_NOAUTH;\n> +\t\t\t} else {\n> +\t\t\t\t// git_getpass is needed here because its very likely stdin/stdout are\n> +\t\t\t\t// pipes to our parent process.  So we instead need to use /dev/tty,\n> +\t\t\t\t// but that is non-portable.  Using git_getpass() can at least be stubbed\n> +\t\t\t\t// on other platforms with a different implementation if/when necessary.\n> +\t\t\t\tuser_name = xstrdup(git_getpass(\"Username: \"));\n\nNo C99/C++ \"//\" comments.\n\n\t/*\n         * We format multi-line\n         * comments like\n         * this.\n         */\n\nThanks.  Tentatively I'll queue this version _without_ any touch-up to\n'pu'.\n"},{"id":"138366","messageId":"h2vd411cc4a1004011506p4d2db46bjafbe22f702b0f869@mail.gmail.com","threadId":"23290","inReplyTo":"7v39zeoo70.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Prompt for a username when an HTTP request 401s","fromName":"Scott Chacon","fromEmail":"schacon@gmail.com","sentAt":"2010-04-01T22:06:47Z","receivedAt":"2010-04-01T22:06:47Z","isPatch":true,"sender":{"key":"schacon@gmail.com","avatar":"https://gravatar.com/avatar/9b13a8a078e1dcf8588c4eea9554445d51ebed6c41b51f56f4d96738130b05c6?d=mp&s=160"},"body":"Hey,\n\nOn Thu, Apr 1, 2010 at 2:30 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Also, earlier I said \"sometimes OK\", because I don't know if it always OK\n> for us to get 401 and continue.  If the end user got a 401 and then does\n> not have a good username or password (e.g. he realizes that the URL he\n> accessed was incorrect), he used to see \"you are not allowed to access\n> this repository\" with a clean failure, but now he would have to get out of\n> \"who are you?\" interaction (and how would he do that?).  Would that be a\n> problem?\n\nThey did not get a clean failure, it looks like this:\n\n--\n$ git clone http://github.dev/defunkt/ambition.git\nInitialized empty Git repository in /private/tmp/svn/ambition/.git/\nerror: The requested URL returned error: 401 while accessing\nhttp://github.dev/defunkt/ambition.git/info/refs\n\nfatal: HTTP request failed\n--\n\n\"401 while accessing X, HTTP request failed\" is not really a clean\nfailure.  Most people don't know what a 401 is.  With this patch, if\nthey get prompted for a user/pass on 401 and it still fails, they see\nthis:\n\n--\n$  ~/bin/git clone http://github.dev/defunkt/ambition.git\nInitialized empty Git repository in /private/tmp/svn/ambition/.git/\nUsername:\nPassword:\nfatal: Authentication failed\n--\n\nI think \"Authentication failed\" is a bit cleaner.  I'm sending another\npatch with the comment format change and message update.\n\nScott\n"}]}