threads / patch / 23073

patchPrompt for a username when an HTTP request 401s

Subject: [PATCH] Prompt for a username when an HTTP request 401s

## tl;dr

8 messages between Mar 19, 2010 and Mar 19, 2010. Diffs are folded; open one to read it.

replies: 7people: 5as markdown or json

Scott Chacon· Mar 19, 2010, 03:41 UTC · lore

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 <schacon@gmail.com>
---

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(-)
Show changes to 3 files +21 −2

http.c, http.h, remote-curl.c

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
Tay Ray Chuan· Mar 19, 2010, 09:13 UTC · re: Scott Chacon · lore

Re: [PATCH] Prompt for a username when an HTTP request 401s

Hi,
On Fri, Mar 19, 2010 at 11:41 AM, Scott Chacon <schacon@gmail.com> wrote:
Show 7 quoted lines
> 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
Daniel Stenberg· Mar 19, 2010, 09:34 UTC · re: Tay Ray Chuan · lore

Re: [PATCH] Prompt for a username when an HTTP request 401s

On Fri, 19 Mar 2010, Tay Ray Chuan wrote:
Show 10 quoted lines
>> 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
Shawn O. Pearce· Mar 19, 2010, 14:16 UTC · re: Daniel Stenberg · lore

Re: [PATCH] Prompt for a username when an HTTP request 401s

Daniel Stenberg <daniel@haxx.se> wrote:
Show 11 quoted lines
> On Fri, 19 Mar 2010, Tay Ray Chuan wrote:
>>> 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.
Shawn O. Pearce· Mar 19, 2010, 14:32 UTC · re: Scott Chacon · lore

Re: [PATCH] Prompt for a username when an HTTP request 401s

Scott Chacon <schacon@gmail.com> wrote:
Show 14 quoted lines
> @@ -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.
Scott Chacon· Mar 19, 2010, 19:08 UTC · re: Shawn O. Pearce · lore

Re: [PATCH] Prompt for a username when an HTTP request 401s

Hey,
On Fri, Mar 19, 2010 at 7:32 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
Show 20 quoted lines
> Scott Chacon <schacon@gmail.com> wrote:
>> @@ -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
Shawn O. Pearce· Mar 19, 2010, 19:09 UTC · re: Scott Chacon · lore

Re: [PATCH] Prompt for a username when an HTTP request 401s

Scott Chacon <schacon@gmail.com> wrote:
Show 23 quoted lines
> On Fri, Mar 19, 2010 at 7:32 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
> > Scott Chacon <schacon@gmail.com> wrote:
> >> @@ -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.
Junio C Hamano· Mar 19, 2010, 19:27 UTC · re: Shawn O. Pearce · lore

Re: [PATCH] Prompt for a username when an HTTP request 401s

"Shawn O. Pearce" <spearce@spearce.org> writes:
Show 20 quoted lines
> Scott Chacon <schacon@gmail.com> wrote:
>> @@ -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.

In addition to the obligatory "no C++/C99 double-slash comments, please ", I think by the time this gets into an applicable shape, Frank's f206063 (git-core: Support retrieving passwords with GIT_ASKPASS, 2010-03-04) will have graduated to the 'master'. It would be a good idea to build this change on top of that one.

← back to recent threads