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

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

[Scott Chacon]

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(-)

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

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

[Tay Ray Chuan]

Hi,

On Fri, Mar 19, 2010 at 11:41 AM, Scott Chacon <schacon@gmail.com> 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

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

[Daniel Stenberg]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 884 bytes --]

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.  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

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

[Shawn O. Pearce]

Daniel Stenberg <daniel@haxx.se> wrote:
> 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.

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

[Shawn O. Pearce]

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.

-- 
Shawn.

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

[Scott Chacon]

Hey,

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?

Scott

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

[Shawn O. Pearce]

Scott Chacon <schacon@gmail.com> wrote:
> 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.

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

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> 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.

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

[Scott Chacon]

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

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

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

[Junio C Hamano]

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

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

[Scott Chacon]

Hey,

On Thu, Apr 1, 2010 at 11:39 PM, Junio C Hamano <gitster@pobox.com> 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

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

[Junio C Hamano]

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

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

[Scott Chacon]

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>
---

Here is the fourth version of this patch - now incorporating the
GIT_ASKPASS stuff.

 http.c        |   20 ++++++++++++++++++--
 http.h        |    2 ++
 remote-curl.c |    2 ++
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/http.c b/http.c
index 4814217..6027546 100644
--- a/http.c
+++ b/http.c
@@ -815,7 +815,19 @@ 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 +843,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

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

[Junio C Hamano]

Scott Chacon <schacon@gmail.com> writes:

> 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.

I'd like another sentence after "that it got a 401.", explaining why it is
sometimes OK for us to get 401 and continue, in order to justify that it
is a good idea to retry after asking for authentication credentials to the
end user when it happens.  I am guessing that it might be something like
this:

    The repository owner might have given out an HTTP URL as if it were a
    public resource (e.g. "http://github.com/myrepo.git/"), and the end
    user may find out that the URL is not valid and he needs to supply a
    username (e.g. "http://me@github.com/myrepo.git") in the URL to
    trigger authentication.  Retrying by asking for username and password
    would help users in such a case.

I said "something like this" because I do not think what I wrote above is
the whole story.  A natural question it begs is "why didn't the repository
owner give the right URL to begin with?"

Also, earlier I said "sometimes OK", because I don't know if it always OK
for us to get 401 and continue.  If the end user got a 401 and then does
not have a good username or password (e.g. he realizes that the URL he
accessed was incorrect), he used to see "you are not allowed to access
this repository" with a clean failure, but now he would have to get out of
"who are you?" interaction (and how would he do that?).  Would that be a
problem?

If that is not a problem, then the patch looks like a good solution to the
problem, and an obvious enhancement that may want to happen would be to
add a boolean parameter to git_getpass() in order to control if we want to
hide what the user types, as we would probably prefer the Username to be
echoed.  But that is an independent issue to be addressed as a separate
follow-up patch.

> Signed-off-by: Scott Chacon <schacon@gmail.com>
> ---
>
> Here is the fourth version of this patch - now incorporating the
> GIT_ASKPASS stuff.
>
>  http.c        |   20 ++++++++++++++++++--
>  http.h        |    2 ++
>  remote-curl.c |    2 ++
>  3 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/http.c b/http.c
> index 4814217..6027546 100644
> --- a/http.c
> +++ b/http.c
> @@ -815,7 +815,19 @@ 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: "));

No C99/C++ "//" comments.

	/*
         * We format multi-line
         * comments like
         * this.
         */

Thanks.  Tentatively I'll queue this version _without_ any touch-up to
'pu'.

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

[Scott Chacon]

Hey,

On Thu, Apr 1, 2010 at 2:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Also, earlier I said "sometimes OK", because I don't know if it always OK
> for us to get 401 and continue.  If the end user got a 401 and then does
> not have a good username or password (e.g. he realizes that the URL he
> accessed was incorrect), he used to see "you are not allowed to access
> this repository" with a clean failure, but now he would have to get out of
> "who are you?" interaction (and how would he do that?).  Would that be a
> problem?

They did not get a clean failure, it looks like this:

--
$ git clone http://github.dev/defunkt/ambition.git
Initialized empty Git repository in /private/tmp/svn/ambition/.git/
error: The requested URL returned error: 401 while accessing
http://github.dev/defunkt/ambition.git/info/refs

fatal: HTTP request failed
--

"401 while accessing X, HTTP request failed" is not really a clean
failure.  Most people don't know what a 401 is.  With this patch, if
they get prompted for a user/pass on 401 and it still fails, they see
this:

--
$  ~/bin/git clone http://github.dev/defunkt/ambition.git
Initialized empty Git repository in /private/tmp/svn/ambition/.git/
Username:
Password:
fatal: Authentication failed
--

I think "Authentication failed" is a bit cleaner.  I'm sending another
patch with the comment format change and message update.

Scott

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

[Scott Chacon]

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>
---

Third version of this with Shawn's comments on why getpass() is needed.

 http.c        |   20 ++++++++++++++++++--
 http.h        |    2 ++
 remote-curl.c |    2 ++
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/http.c b/http.c
index deab595..62db1f0 100644
--- a/http.c
+++ b/http.c
@@ -815,7 +815,19 @@ 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 {
+				// 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.
+				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 +843,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

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

[Scott Chacon]

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>
---
 http.c        |   21 +++++++++++++++++++--
 http.h        |    2 ++
 remote-curl.c |    2 ++
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/http.c b/http.c
index deab595..731783e 100644
--- a/http.c
+++ b/http.c
@@ -199,6 +199,11 @@ static int http_options(const char *var, const
char *value, void *cb)
 	return git_default_config(var, value, cb);
 }

+static void get_http_user_name()
+{
+	user_name = xstrdup(getpass("Username: "));
+}
+
 static void init_curl_http_auth(CURL *result)
 {
 	if (user_name) {
@@ -815,7 +820,15 @@ 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 {
+				get_http_user_name();
+				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 +844,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

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

[Shawn O. Pearce]

Scott Chacon <schacon@gmail.com> 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.
> 
> Signed-off-by: Scott Chacon <schacon@gmail.com>
> ---
>  http.c        |   21 +++++++++++++++++++--
>  http.h        |    2 ++
>  remote-curl.c |    2 ++
>  3 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/http.c b/http.c
> index deab595..731783e 100644
> --- a/http.c
> +++ b/http.c
> @@ -199,6 +199,11 @@ static int http_options(const char *var, const
> char *value, void *cb)
>  	return git_default_config(var, value, cb);
>  }
> 
> +static void get_http_user_name()
> +{
> +	user_name = xstrdup(getpass("Username: "));

Why are we getting the username via a password prompt where echo
has been disabled?  Traditionally a username field is obtained as
echoed input.

Also, this method shouldn't be named get_*() if its returning void.
Sounds far to funny.  init_http_user_name()?  prompt_for_user_name()?

-- 
Shawn.

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

[René Scharfe]

Am 18.03.2010 20:03, schrieb Shawn O. Pearce:
> Scott Chacon <schacon@gmail.com> wrote:
>> +static void get_http_user_name()
>> +{
>> +	user_name = xstrdup(getpass("Username: "));
> 
> Why are we getting the username via a password prompt where echo
> has been disabled?  Traditionally a username field is obtained as
> echoed input.
> 
> Also, this method shouldn't be named get_*() if its returning void.
> Sounds far to funny.  init_http_user_name()?  prompt_for_user_name()?

Or inline the one-liner at its single call-site?

René