[PATCH 1/2] t5540: test DAV push with authentication

We don't currently test this case at all, and instead just
test the DAV mechanism over an unauthenticated push. That
isn't very realistic, as most people will want to
authenticate pushes.

Two of the tests expect_failure as they reveal bugs:

  1. Pushing without a username in the URL fails to ask for
     credentials when we get an HTTP 401. This has always
     been the case, but it would be nice if it worked like
     smart-http.

  2. Pushing with a username fails to ask for the password
     since 986bbc0 (http: don't always prompt for password,
     2011-11-04). This is a severe regression in v1.7.8, as
     authenticated push-over-DAV is now totally unusable
     unless you have credentials in your .netrc.

Signed-off-by: Jeff King <redacted>
---
Nobody has mentioned the regression on git@vger yet, but there are two
threads already on msysgit:

  http://thread.gmane.org/gmane.comp.version-control.msysgit/14138

  http://thread.gmane.org/gmane.comp.version-control.msysgit/14161

 t/lib-httpd/apache.conf |    3 +++
 t/t5540-http-push.sh    |   34 ++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 0a4cdfa..3c12b05 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf

@@ -92,6 +92,9 @@ SSLEngine On
 	<Location /dumb/>
 		Dav on
 	</Location>
+	<Location /auth/dumb>
+		Dav on
+	</Location>
 </IfDefine>
 
 <IfDefine SVN>

diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index 64767d8..3300227 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh

@@ -40,6 +40,22 @@ test_expect_success 'setup remote repository' '
 	mv test_repo.git "$HTTPD_DOCUMENT_ROOT_PATH"
 '
 
+test_expect_success 'create password-protected repository' '
+	mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb" &&
+	cp -Rf "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git" \
+	       "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git"
+'
+
+test_expect_success 'setup askpass helper' '
+	cat >askpass <<-\EOF &&
+	#!/bin/sh
+	echo user@host
+	EOF
+	chmod +x askpass &&
+	GIT_ASKPASS="$PWD/askpass" &&
+	export GIT_ASKPASS
+'
+
 test_expect_success 'clone remote repository' '
 	cd "$ROOT_PATH" &&
 	git clone $HTTPD_URL/dumb/test_repo.git test_repo_clone

@@ -144,6 +160,24 @@ test_expect_success 'PUT and MOVE sends object to URLs with SHA-1 hash suffix' '
 test_http_push_nonff "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git \
 	"$ROOT_PATH"/test_repo_clone master
 
+test_expect_failure 'push to password-protected repository (user in URL)' '
+	test_commit pw-user &&
+	git push "$HTTPD_URL_USER/auth/dumb/test_repo.git" HEAD &&
+	git rev-parse --verify HEAD >expect &&
+	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git" \
+		rev-parse --verify HEAD >actual &&
+	test_cmp expect actual
+'
+
+test_expect_failure 'push to password-protected repository (no user in URL)' '
+	test_commit pw-nouser &&
+	git push "$HTTPD_URL/auth/dumb/test_repo.git" HEAD &&
+	git rev-parse --verify HEAD >expect &&
+	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git" \
+		rev-parse --verify HEAD >actual &&
+	test_cmp expect actual
+'
+
 stop_httpd
 
 test_done

-- 
1.7.8.17.gfd3524

This reverts commit 986bbc0842334f0e07731fa37f2a55d2930a5b8c.

The rationale for that commit relied on the fact that asking
for the password up-front was merely an optimization,
because git will notice an HTTP 401 and prompt for the
password. However, that is only true for smart-http, and for
dumb fetching. Dumb push over DAV does not have this
feature; as a result, authenticated push-over-DAV does not
work at all, as it never prompts the user for a password.

Signed-off-by: Jeff King <redacted>
---
We need to deal with this regression for v1.7.8.1, I think.

There are basically three options for fixing it:

  1. Teach http-push the same retry-after-401 trick that the rest of the
     http code knows.

  2. Refactor the retry-after-401 logic from http.c into a common
     function that http-push can build on top of.

  3. Revert 986bbc08 and leave it alone; it only hurts .netrc users,
     there's a reasonable workaround (don't put the user in the URL) and
     hopefully those people will convert to using better storage via
     credential helper once it is available.

I looked at doing (1), but my first attempt[1] didn't quite work. So
it's not a huge amount of code, but it's annoyingly non-trivial. And as
a long-term solution, it's just making hack-y code hackier.

Doing (2) would be the best solution, but it's going to require some
pretty major surgery to http.c and http-push.c. I'll take a look, but if
it gets too complex, it may simply not be worth it (now that smart-http
is available, I would hope that push-over-DAV is slowly going away).

Doing (3) is obviously the easiest thing. And given the complexity of
the other two solutions, I think it makes sense to revert 986bbc08
(i.e., apply this patch), ship a working v1.7.8.1, and then look at
doing one of the other two solutions for v1.7.9.

[1] http://article.gmane.org/gmane.comp.version-control.msysgit/14153

 http.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/http.c b/http.c
index 008ad72..a4bc770 100644
--- a/http.c
+++ b/http.c

@@ -279,6 +279,8 @@ static CURL *get_curl_handle(void)
 	curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
 #endif
 
+	init_curl_http_auth(result);
+
 	if (ssl_cert != NULL)
 		curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
 	if (has_cert_password())

@@ -844,7 +846,7 @@ static int http_request(const char *url, void *result, int target, int options)
 		else if (missing_target(&results))
 			ret = HTTP_MISSING_TARGET;
 		else if (results.http_code == 401) {
-			if (user_name && user_pass) {
+			if (user_name) {
 				ret = HTTP_NOAUTH;
 			} else {
 				/*

@@ -853,8 +855,7 @@ static int http_request(const char *url, void *result, int target, int options)
 				 * but that is non-portable.  Using git_getpass() can at least be stubbed
 				 * on other platforms with a different implementation if/when necessary.
 				 */
-				if (!user_name)
-					user_name = xstrdup(git_getpass_with_description("Username", description));
+				user_name = xstrdup(git_getpass_with_description("Username", description));
 				init_curl_http_auth(slot->curl);
 				ret = HTTP_REAUTH;
 			}

-- 
1.7.8.17.gfd3524

On Tue, Dec 13, 2011 at 21:17, Jeff King [off-list ref] wrote:

We don't currently test this case at all, and instead just
test the DAV mechanism over an unauthenticated push. That
isn't very realistic, as most people will want to
authenticate pushes.

Thanks for adding this, Peff!

-- 
Sebastian Schuberth

On Tue, Dec 13, 2011 at 10:28:07PM +0100, Sebastian Schuberth wrote:

On Tue, Dec 13, 2011 at 21:17, Jeff King [off-list ref] wrote:

quoted

We don't currently test this case at all, and instead just
test the DAV mechanism over an unauthenticated push. That
isn't very realistic, as most people will want to
authenticate pushes.

Thanks for adding this, Peff!

You're welcome. Thank you for forwarding the bug report. I would never
have seen it on the msysgit list, and for some reason it seems that
msysgit people are more likely to use DAV.

Having looked a lot at the http code the past month or two, I knew it
was pretty flaky and I was nervous when we added Stefan's patch (and no,
I don't blame Stefan; his patch was completely reasonable, but just
happened to trigger a problem in a seldom-looked-at corner of the code).

But I hadn't looked at http-push at all until yesterday, and it didn't
even occur to me that there was another whole area of code relying in a
very obscure way on the http.c auth code. I'll take a look at some of
the refactoring I've done in http.c (both for the credentials topic as
well as the bundle topic) and see if we can't integrate http-push.c a
little more smoothly.

-Peff