Re: [PATCH v8 1/3] t5563: add tests for basic and anoymous HTTP access

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Victoria Dye <vdye@github.com>
Cc: Matthew John Cheetham via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Derrick Stolee <derrickstolee@github.com>,
	Lessley Dennington <lessleydennington@gmail.com>,
	Matthew John Cheetham <mjcheetham@outlook.com>,
	M Hickford <mirth.hickford@gmail.com>,
	Jeff Hostetler <git@jeffhostetler.com>,
	Glen Choo <chooglen@google.com>, Jeff King <peff@peff.net>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH v8 1/3] t5563: add tests for basic and anoymous HTTP access
Date: Thu, 09 Feb 2023 12:19:03 +0100	[thread overview]
Message-ID: <230209.86v8kbvz51.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <87f79e79-1591-ca28-4975-3bca5b8f7266@github.com>

On Wed, Feb 08 2023, Victoria Dye wrote:

> Matthew John Cheetham via GitGitGadget wrote:
>>  ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1
>> +ScriptAliasMatch /custom_auth/(.*) nph-custom-auth.sh/$1
>
> This setup (redirecting '/custom_auth/' routes to the 'nph-custom-auth.sh'
> script) is nice and straightforward. 

*nod*

> [...]
>> +if test -f "$ANONYMOUS_FILE" || (test -f "$VALID_CREDS_FILE" && \
>> +	grep -qi "^${HTTP_AUTHORIZATION:-nopenopnope}$" "$VALID_CREDS_FILE")
> [...]
>     if test -f "$ANONYMOUS_FILE" || (test -n "$HTTP_AUTHORIZATION" && \
>     	grep -qsi "^${HTTP_AUTHORIZATION}\$" "$VALID_CREDS_FILE")
>
> Note the addition of '-s' to 'grep' - it seems cleaner than redirecting to
> '/dev/null' (as Ævar suggested [1]) while achieving the same result.
>
> [1] https://lore.kernel.org/git/230206.86fsbi5y63.gmgdl@evledraar.gmail.com/

I wondered if it's in POSIX, turns out it is!:
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/grep.html

But we don't have any existing use of it, even for things in POSIX it's
often a gamble what the exact semantics are on our long tail of *nix,
e.g. old AIX.

In general I'd think we could just avoid "-s" or piping to "/dev/null"
here, i.e. under "-x" or whatever it's informative to know it doesn't
exist from the stderr, but on second look I think both of us long track
of a larger issue here...
> [...]
>> +fi
>> +
>> +echo 'HTTP/1.1 401 Authorization Required'
>> +if test -f "$CHALLENGE_FILE"
>> +then
>> +	cat "$CHALLENGE_FILE"
>> +fi
>
> In contrast to Ævar's comments in the review linked earlier, I like having
> the explicit 'test -f' (to sort of "self-document" that the challenge is
> only issued if $CHALLENGE_FILE exists). I think you're fine keeping this
> as-is or changing it, depending on your preference.

Looking at this again I think we should just have it be unconditional
here. I.e. it looks like we both assumed that this needs to be a
conditional, but actually every /custom_auth/ test also sets up this
"$CHALLENGE_FILE".

So this "test -f" seems to only serve the purpose of burying an error
under the rug if things have already gone wrong.

But if we're making these requests why are we writing a script that
handles the combination of 3 parameters, and needs to second guess
things? We can just create N urls and N scripts instead. So I tried this
fix-up instead:

	diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
	index 7979605d344..c25e3000db0 100644
	--- a/t/lib-httpd.sh
	+++ b/t/lib-httpd.sh
	@@ -141,6 +141,7 @@ prepare_httpd() {
	 	install_script error.sh
	 	install_script apply-one-time-perl.sh
	 	install_script nph-custom-auth.sh
	+	install_script nph-custom-auth-anon.sh

	 	ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"

	diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
	index 2aac922376c..7a63a9169c3 100644
	--- a/t/lib-httpd/apache.conf
	+++ b/t/lib-httpd/apache.conf
	@@ -140,6 +140,7 @@ ScriptAlias /error_smart/ error-smart-http.sh/
	 ScriptAlias /error/ error.sh/
	 ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1
	 ScriptAliasMatch /custom_auth/(.*) nph-custom-auth.sh/$1
	+ScriptAliasMatch /custom_auth_anon/(.*) nph-custom-auth-anon.sh/$1
	 <Directory ${GIT_EXEC_PATH}>
	 	Options FollowSymlinks
	 </Directory>
	diff --git a/t/lib-httpd/nph-custom-auth-anon.sh b/t/lib-httpd/nph-custom-auth-anon.sh
	new file mode 100755
	index 00000000000..3c7a24fed6b
	--- /dev/null
	+++ b/t/lib-httpd/nph-custom-auth-anon.sh
	@@ -0,0 +1,4 @@
	+#!/bin/sh
	+
	+echo 'HTTP/1.1 200 OK'
	+exec "$GIT_EXEC_PATH"/git-http-backend
	diff --git a/t/lib-httpd/nph-custom-auth.sh b/t/lib-httpd/nph-custom-auth.sh
	index 8f851aebac4..e3ee61c8c9e 100755
	--- a/t/lib-httpd/nph-custom-auth.sh
	+++ b/t/lib-httpd/nph-custom-auth.sh
	@@ -1,28 +1,15 @@
	 #!/bin/sh

	+set -e
	+
	 VALID_CREDS_FILE=custom-auth.valid
	-CHALLENGE_FILE=custom-auth.challenge
	-ANONYMOUS_FILE=custom-auth.anonymous

	-#
	-# If $ANONYMOUS_FILE exists in $HTTPD_ROOT_PATH, allow anonymous access.
	-#
	 # If $VALID_CREDS_FILE exists in $HTTPD_ROOT_PATH, consider each line as a valid
	 # credential for the current request. Each line in the file is considered a
	 # valid HTTP Authorization header value. For example:
	 #
	 # Basic YWxpY2U6c2VjcmV0LXBhc3N3ZA==
	-#
	-# If $CHALLENGE_FILE exists in $HTTPD_ROOT_PATH, output the contents as headers
	-# in a 401 response if no valid authentication credentials were included in the
	-# request. For example:
	-#
	-# WWW-Authenticate: Bearer authorize_uri="id.example.com" p=1 q=0
	-# WWW-Authenticate: Basic realm="example.com"
	-#
	-
	-if test -f "$ANONYMOUS_FILE" || (test -f "$VALID_CREDS_FILE" && \
	-	grep -qi "^${HTTP_AUTHORIZATION:-nopenopnope}$" "$VALID_CREDS_FILE")
	+if grep -qi "^${HTTP_AUTHORIZATION:-nopenopnope}$" "$VALID_CREDS_FILE"
	 then
	 	# Note that although git-http-backend returns a status line, it
	 	# does so using a CGI 'Status' header. Because this script is an
	@@ -31,12 +18,15 @@ then
	 	# This is only a test script, so we don't bother to check for
	 	# the actual status from git-http-backend and always return 200.
	 	echo 'HTTP/1.1 200 OK'
	-	exec "$GIT_EXEC_PATH"/git-http-backend
	+	exit 1
	 fi

	+# Output of our challenge file as headers
	+# in a 401 response if no valid authentication credentials were included in the
	+# request. For example:
	+#
	+# WWW-Authenticate: Bearer authorize_uri="id.example.com" p=1 q=0
	+# WWW-Authenticate: Basic realm="example.com"
	 echo 'HTTP/1.1 401 Authorization Required'
	-if test -f "$CHALLENGE_FILE"
	-then
	-	cat "$CHALLENGE_FILE"
	-fi
	+cat custom-auth.challenge
	 echo
	diff --git a/t/t5563-simple-http-auth.sh b/t/t5563-simple-http-auth.sh
	index a7b1e5bd1af..feb8149de8f 100755
	--- a/t/t5563-simple-http-auth.sh
	+++ b/t/t5563-simple-http-auth.sh
	@@ -47,8 +47,7 @@ test_expect_success 'setup repository' '

	 test_expect_success 'access anonymous no challenge' '
	 	test_when_finished "per_test_cleanup" &&
	-	touch "$HTTPD_ROOT_PATH/custom-auth.anonymous" &&
	-	git ls-remote "$HTTPD_URL/custom_auth/repo.git"
	+	git ls-remote "$HTTPD_URL/custom_auth_anon/repo.git"
	 '

	 test_expect_success 'access using basic auth' '

I think that's much better, now we just have a 2-line script to handle
this "anon auth" case. Instead of creating a "custom-auth.anonymous"
file to communicate how the remote end should behave, let's just
communicate that by requesting a different URL, one that accepts
anonymous authentication.

I did insert a deliberate bug here, or:

	-	exec "$GIT_EXEC_PATH"/git-http-backend
	+	exit 1

So aside from your "exec" comment it seems both of us missed that this
"exec" does nothing useful, the test will fail if we emit different
headers, but it doesn't matter that we execute the git-http-backend.

Or maybe it does, but the tests aren't good enough to spot the
difference.

The above is a rough WIP, I'm leaving it here for Matthew to follow-up
on. I think it might benefit from being further split-up, i.e. we know
which URLs we expect to fail auth, so if we just had another URL for
"the auth response fails here" we'd have 3x trivial scripts with no
if/else; but maybe that sucks, I didn't try it.