Subject: Re: [PATCH 5/8] credential: add WWW-Authenticate header to cred requests

From: Derrick Stolee <[email-masked]>
Date: Mon, 19 Sep 2022 12:33:33 -0400

On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <[email-masked]>

> In this case we send multiple `wwwauth[n]` properties where `n` is a
> zero-indexed number, reflecting the order the WWW-Authenticate headers
> appeared in the HTTP response.
> @@ -151,6 +151,15 @@ Git understands the following attributes:
>  	were read (e.g., `url=https://example.com` would behave as if
>  	`protocol=https` and `host=example.com` had been provided). This
>  	can help callers avoid parsing URLs themselves.
> +
> +`wwwauth[n]`::
> +
> +	When an HTTP response is received that includes one or more
> +	'WWW-Authenticate' authentication headers, these can be passed to Git
> +	(and subsequent credential helpers) with these attributes.
> +	Each 'WWW-Authenticate' header value should be passed as a separate
> +	attribute 'wwwauth[n]' where 'n' is the zero-indexed order the headers
> +	appear in the HTTP response.
>  +
>  Note that specifying a protocol is mandatory and if the URL
>  doesn't specify a hostname (e.g., "cert:///path/to/file") the

This "+" means that this paragraph should be connected to the previous
one, so it seems that you've inserted your new value in the middle of
the `url` key. You'll want to move yours to be after those two connected
paragraphs. Your diff hunk should look like this:

--- >8 ---

diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index f18673017f..127ae29be3 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -160,6 +160,15 @@ empty string.
 Components which are missing from the URL (e.g., there is no
 username in the example above) will be left unset.
 
+`wwwauth[n]`::
+
+	When an HTTP response is received that includes one or more
+	'WWW-Authenticate' authentication headers, these can be passed to Git
+	(and subsequent credential helpers) with these attributes.
+	Each 'WWW-Authenticate' header value should be passed as a separate
+	attribute 'wwwauth[n]' where 'n' is the zero-indexed order the headers
+	appear in the HTTP response.
+
 GIT
 ---
 Part of the linkgit:git[1] suite

--- >8 ---

> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
> index 497b9b9d927..fe118d76f98 100644
> --- a/t/lib-httpd/apache.conf
> +++ b/t/lib-httpd/apache.conf
> @@ -235,6 +235,19 @@ SSLEngine On
>  	Require valid-user
>  </LocationMatch>
>  
> +# Advertise two additional auth methods above "Basic".
> +# Neither of them actually work but serve test cases showing these
> +# additional auth headers are consumed correctly.
> +<Location /auth-wwwauth/>
> +	AuthType Basic
> +	AuthName "git-auth"
> +	AuthUserFile passwd
> +	Require valid-user
> +	SetEnvIf Authorization "^\S+" authz
> +	Header always add WWW-Authenticate "Bearer authority=https://login.example.com" env=!authz
> +	Header always add WWW-Authenticate "FooAuth foo=bar baz=1" env=!authz
> +</Location>
> +

This is cool that you've figured out how to make our Apache tests
add these headers! Maybe we won't need that extra test helper like
I thought (unless we want to confirm the second request sends the
right information).

> +test_expect_success 'http auth sends www-auth headers to credential helper' '
> +	write_script git-credential-tee <<-\EOF &&
> +		cmd=$1
> +		teefile=credential-$cmd
> +		if [ -f "$teefile" ]; then

I think we prefer using "test" over the braces (and linebreak
before then) like this:

		if test -n "$teefile"
		then

> +			rm $teefile
> +		fi

Alternatively, you could always run "rm -f $teefile" for
simplicity.

> +		(
> +			while read line;
> +			do
> +				if [ -z "$line" ]; then
> +					exit 0
> +				fi
> +				echo "$line" >> $teefile
> +				echo $line
> +			done
> +		) | git credential-store $cmd

Since I'm not sure, I'll ask the question: do we need the sub-shell
here, or could we pipe directly off of the "done"? Like this:

		while read line;
		do
			if [ -z "$line" ]; then
				exit 0
			fi
			echo "$line" >> $teefile
			echo $line
		done | git credential-store $cmd

> +	EOF

> +	cat >expected-get <<-EOF &&
> +	protocol=http
> +	host=127.0.0.1:5551
> +	wwwauth[0]=Bearer authority=https://login.example.com
> +	wwwauth[1]=FooAuth foo=bar baz=1
> +	wwwauth[2]=Basic realm="git-auth"
> +	EOF
> +
> +	cat >expected-store <<-EOF &&
> +	protocol=http
> +	host=127.0.0.1:5551
> +	username=user@host
> +	password=pass@host
> +	EOF
> +
> +	rm -f .git-credentials &&
> +	test_config credential.helper tee &&
> +	set_askpass user@host pass@host &&
> +	(
> +		PATH="$PWD:$PATH" &&
> +		git ls-remote "$HTTPD_URL/auth-wwwauth/smart/repo.git"
> +	) &&
> +	expect_askpass both user@host &&
> +	test_cmp expected-get credential-get &&
> +	test_cmp expected-store credential-store

Elegant check for both calls.

Thanks,
-Stolee