{"thread":{"id":"23073","subject":"[PATCH] Prompt for a username when an HTTP request 401s","startedAt":"2010-03-19T03:41:10Z","lastAt":"2010-03-19T19:27:14Z","messageCount":8,"participants":["Scott Chacon","Tay Ray Chuan","Daniel Stenberg","Shawn O. Pearce","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"137180","messageId":"d411cc4a1003182041p67a93bf6i1528a8eaaab5c805@mail.gmail.com","threadId":"23073","inReplyTo":null,"subject":"[PATCH] Prompt for a username when an HTTP request 401s","fromName":"Scott Chacon","fromEmail":"schacon@gmail.com","sentAt":"2010-03-19T03:41:10Z","receivedAt":"2010-03-19T03:41:10Z","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\nI updated this patch to include comments on why I'm using getpass() -\nwe went through trying to get the info via stdin/stdout and it didn't\nwork very well because stdout/in are pipes, apparently.  Shawn\nsuggested just using the getpass().  I've inlined it, though, as per\nthe other suggestion.\n\n http.c        |   19 +++++++++++++++++--\n http.h        |    2 ++\n remote-curl.c |    2 ++\n 3 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex deab595..6370da4 100644\n--- a/http.c\n+++ b/http.c\n@@ -815,7 +815,18 @@ 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// it is neccesary to use getpass here because\n+\t\t\t\t// there appears to be no other clean way to\n+\t\t\t\t// read/write stdout/stdin\n+\t\t\t\tuser_name = xstrdup(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 +842,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":"137191","messageId":"be6fef0d1003190213m48b6c91dx5cbd489cf798dacc@mail.gmail.com","threadId":"23073","inReplyTo":"d411cc4a1003182041p67a93bf6i1528a8eaaab5c805@mail.gmail.com","subject":"Re: [PATCH] Prompt for a username when an HTTP request 401s","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2010-03-19T09:13:37Z","receivedAt":"2010-03-19T09:13:37Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Fri, Mar 19, 2010 at 11:41 AM, Scott Chacon <schacon@gmail.com> wrote:\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\n(added Daniel to the Cc list)\n\nhow are you getting 401s? Recently, git set the CURL_AUTH_ANY option,\nso if the correct credentials are passed, curl should have \"hid\" the\n401 from us.\n\n-- \nCheers,\nRay Chuan\n"},{"id":"137194","messageId":"alpine.DEB.2.00.1003191033022.15750@tvnag.unkk.fr","threadId":"23073","inReplyTo":"be6fef0d1003190213m48b6c91dx5cbd489cf798dacc@mail.gmail.com","subject":"Re: [PATCH] Prompt for a username when an HTTP request 401s","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2010-03-19T09:34:16Z","receivedAt":"2010-03-19T09:34:16Z","isPatch":true,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Fri, 19 Mar 2010, Tay Ray Chuan wrote:\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 http_request to \n>> prompt for the username and password, then return HTTP_REAUTH so \n>> http_get_strbuf can try again.  If it gets a 401 even when a user/pass is \n>> supplied, http_request will now return HTTP_NOAUTH which remote_curl can \n>> then use to display a more intelligent error message that is less \n>> confusing.\n>\n> how are you getting 401s? Recently, git set the CURL_AUTH_ANY option, so if \n> the correct credentials are passed, curl should have \"hid\" the 401 from us.\n\nThat's correct. It should hide the 401 in the sense that it should try to \ncontinue and do the correct authentication procedure and only if that fails it \nshould end up with an actual 401 end result.\n\n-- \n\n  / daniel.haxx.se"},{"id":"137224","messageId":"20100319141643.GA16211@spearce.org","threadId":"23073","inReplyTo":"alpine.DEB.2.00.1003191033022.15750@tvnag.unkk.fr","subject":"Re: [PATCH] Prompt for a username when an HTTP request 401s","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-03-19T14:16:43Z","receivedAt":"2010-03-19T14:16:43Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Daniel Stenberg <daniel@haxx.se> wrote:\n> On Fri, 19 Mar 2010, Tay Ray Chuan wrote:\n>>> When an HTTP request returns a 401, Git will currently fail with a  \n>>> confusing message saying that it got a 401.\n>>\n>> how are you getting 401s? Recently, git set the CURL_AUTH_ANY option, \n>> so if the correct credentials are passed, curl should have \"hid\" the \n>> 401 from us.\n>\n> That's correct. It should hide the 401 in the sense that it should try to \n> continue and do the correct authentication procedure and only if that \n> fails it should end up with an actual 401 end result.\n\nIf the URL didn't contain a username, and the server returns a 401,\nGit just aborts with an error.\n\nWhat Scott is trying to do here is teach Git to request a\nusername/password if there was no username in the URL and\nauthentication is required by the server.\n\nIn the case of GitHub, this means they can advertise one http:// URL\nfor the repository.  Anonymous fetch just works, and using that same\nURL to push will ask for your username/password, and then complete.\n\n-- \nShawn.\n"},{"id":"137225","messageId":"20100319143205.GB16211@spearce.org","threadId":"23073","inReplyTo":"d411cc4a1003182041p67a93bf6i1528a8eaaab5c805@mail.gmail.com","subject":"Re: [PATCH] Prompt for a username when an HTTP request 401s","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-03-19T14:32:05Z","receivedAt":"2010-03-19T14:32:05Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Scott Chacon <schacon@gmail.com> wrote:\n> @@ -815,7 +815,18 @@ 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// it is neccesary to use getpass here because\n> +\t\t\t\t// there appears to be no other clean way to\n> +\t\t\t\t// read/write stdout/stdin\n> +\t\t\t\tuser_name = xstrdup(getpass(\"Username: \"));\n\nNo, getpass is needed here because its very likely stdin/stdout are\npipes to our parent process.  So we instead need to use /dev/tty,\nbut that is non-portable.  Using getpass() can at least be stubbed\non other platforms with a different implementation if/when necessary.\n\n-- \nShawn.\n"},{"id":"137256","messageId":"d411cc4a1003191208m38b7baf2mbb8f1ea288f90819@mail.gmail.com","threadId":"23073","inReplyTo":"20100319143205.GB16211@spearce.org","subject":"Re: [PATCH] Prompt for a username when an HTTP request 401s","fromName":"Scott Chacon","fromEmail":"schacon@gmail.com","sentAt":"2010-03-19T19:08:18Z","receivedAt":"2010-03-19T19:08:18Z","isPatch":true,"sender":{"key":"schacon@gmail.com","avatar":"https://gravatar.com/avatar/9b13a8a078e1dcf8588c4eea9554445d51ebed6c41b51f56f4d96738130b05c6?d=mp&s=160"},"body":"Hey,\n\nOn Fri, Mar 19, 2010 at 7:32 AM, Shawn O. Pearce <spearce@spearce.org> wrote:\n> Scott Chacon <schacon@gmail.com> wrote:\n>> @@ -815,7 +815,18 @@ static int http_request(const char *url, void\n>> *result, int target, int options)\n>>                       ret = HTTP_OK;\n>>               else if (missing_target(&results))\n>>                       ret = HTTP_MISSING_TARGET;\n>> -             else\n>> +             else if (results.http_code == 401) {\n>> +                     if (user_name) {\n>> +                             ret = HTTP_NOAUTH;\n>> +                     } else {\n>> +                             // it is neccesary to use getpass here because\n>> +                             // there appears to be no other clean way to\n>> +                             // read/write stdout/stdin\n>> +                             user_name = xstrdup(getpass(\"Username: \"));\n>\n> No, getpass is needed here because its very likely stdin/stdout are\n> pipes to our parent process.  So we instead need to use /dev/tty,\n> but that is non-portable.  Using getpass() can at least be stubbed\n> on other platforms with a different implementation if/when necessary.\n\nShould I roll a new patch for this?\n\nScott\n"},{"id":"137257","messageId":"20100319190923.GA16758@spearce.org","threadId":"23073","inReplyTo":"d411cc4a1003191208m38b7baf2mbb8f1ea288f90819@mail.gmail.com","subject":"Re: [PATCH] Prompt for a username when an HTTP request 401s","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-03-19T19:09:23Z","receivedAt":"2010-03-19T19:09:23Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Scott Chacon <schacon@gmail.com> wrote:\n> On Fri, Mar 19, 2010 at 7:32 AM, Shawn O. Pearce <spearce@spearce.org> wrote:\n> > Scott Chacon <schacon@gmail.com> wrote:\n> >> @@ -815,7 +815,18 @@ static int http_request(const char *url, void\n> >> *result, int target, int options)\n> >> ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ret = HTTP_OK;\n> >> ?? ?? ?? ?? ?? ?? ?? else if (missing_target(&results))\n> >> ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ret = HTTP_MISSING_TARGET;\n> >> - ?? ?? ?? ?? ?? ?? else\n> >> + ?? ?? ?? ?? ?? ?? else if (results.http_code == 401) {\n> >> + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? if (user_name) {\n> >> + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ret = HTTP_NOAUTH;\n> >> + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? } else {\n> >> + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? // it is neccesary to use getpass here because\n> >> + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? // there appears to be no other clean way to\n> >> + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? // read/write stdout/stdin\n> >> + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? user_name = xstrdup(getpass(\"Username: \"));\n> >\n> > No, getpass is needed here because its very likely stdin/stdout are\n> > pipes to our parent process. ??So we instead need to use /dev/tty,\n> > but that is non-portable. ??Using getpass() can at least be stubbed\n> > on other platforms with a different implementation if/when necessary.\n> \n> Should I roll a new patch for this?\n\nYea, you probably should since I think the comment could be improved.\n\n-- \nShawn.\n"},{"id":"137261","messageId":"7vr5ngkt7x.fsf@alter.siamese.dyndns.org","threadId":"23073","inReplyTo":"20100319143205.GB16211@spearce.org","subject":"Re: [PATCH] Prompt for a username when an HTTP request 401s","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-19T19:27:14Z","receivedAt":"2010-03-19T19:27:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n> Scott Chacon <schacon@gmail.com> wrote:\n>> @@ -815,7 +815,18 @@ 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// it is neccesary to use getpass here because\n>> +\t\t\t\t// there appears to be no other clean way to\n>> +\t\t\t\t// read/write stdout/stdin\n>> +\t\t\t\tuser_name = xstrdup(getpass(\"Username: \"));\n>\n> No, getpass is needed here because its very likely stdin/stdout are\n> pipes to our parent process.  So we instead need to use /dev/tty,\n> but that is non-portable.  Using getpass() can at least be stubbed\n> on other platforms with a different implementation if/when necessary.\n\nIn addition to the obligatory \"no C++/C99 double-slash comments, please \",\nI think by the time this gets into an applicable shape, Frank's f206063\n(git-core: Support retrieving passwords with GIT_ASKPASS, 2010-03-04) will\nhave graduated to the 'master'.  It would be a good idea to build this\nchange on top of that one.\n"}]}