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

[Matthew John Cheetham <mjcheetham@outlook.com>]

On 2023-02-09 03:19, Ævar Arnfjörð Bjarmason wrote:

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

Actually, we don't really need to test the anonymous auth case at all
because all other tests that try accessing a remote repository over HTTP
are already exercising this. See t5551-http-fetch-smart for example..
here we're performing various requests without auth.
Should we be erronously issuing credential helper challenges in these
scenarios then the tests would fail with an askpass prompt.

I will drop the anon auth test and script support.

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