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

[Jeff King <peff@peff.net>]

On Thu, Feb 16, 2023 at 10:34:39PM +0000, Matthew John Cheetham via GitGitGadget wrote:

> Leverage a no-parsed headers (NPH) CGI script so that we can directly
> control the HTTP responses to simulate a multitude of good, bad and ugly
> remote server implementations around auth.

Hmm, today I learned about NPH scripts.

Obviously it works here, but I have to wonder: is there a reason we need
this? AFAICT the only thing we do is set the HTTP response code, which
could also be done with a Status: header.

I.e., this passes your test:

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index ccd5f3cf82..1eadfa4bbc 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -140,7 +140,7 @@ prepare_httpd() {
 	install_script error-smart-http.sh
 	install_script error.sh
 	install_script apply-one-time-perl.sh
-	install_script nph-custom-auth.sh
+	install_script custom-auth.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 2aac922376..0f9083dd6c 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -139,7 +139,7 @@ ScriptAlias /broken_smart/ broken-smart-http.sh/
 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/(.*) custom-auth.sh/$1
 <Directory ${GIT_EXEC_PATH}>
 	Options FollowSymlinks
 </Directory>
diff --git a/t/lib-httpd/nph-custom-auth.sh b/t/lib-httpd/custom-auth.sh
similarity index 94%
rename from t/lib-httpd/nph-custom-auth.sh
rename to t/lib-httpd/custom-auth.sh
index f5345e775e..8bf07e9398 100755
--- a/t/lib-httpd/nph-custom-auth.sh
+++ b/t/lib-httpd/custom-auth.sh
@@ -27,11 +27,10 @@ then
 	# status line.
 	# 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
 fi
 
-echo 'HTTP/1.1 401 Authorization Required'
+echo 'Status: 401'
 if test -f "$CHALLENGE_FILE"
 then
 	cat "$CHALLENGE_FILE"


The other, more invisible thing happening behind the scenes is that
Apache isn't adding any of its usual headers. But I don't know of any
that would interfere with our goal of doing auth here. Is there some
feature you're planning where it would?

I think you could argue that it's mostly a matter of personal preference
and doesn't matter much either way. But all things being equal, I'd
usually go with the thing that is simpler and closer to the rest of the
system (e.g., I think you kill the ability of http-backend to return a
non-200 status, though I doubt it matters much in practice).

So I dunno. We are on v10 and this is arguably a nit. Mostly I'm just
curious what led you in this direction in the first place.

> ---
>  t/lib-httpd.sh                 |  1 +
>  t/lib-httpd/apache.conf        |  6 +++
>  t/lib-httpd/nph-custom-auth.sh | 39 ++++++++++++++++
>  t/t5563-simple-http-auth.sh    | 82 ++++++++++++++++++++++++++++++++++
>  4 files changed, 128 insertions(+)
>  create mode 100755 t/lib-httpd/nph-custom-auth.sh

Most of the other scripts here don't have an execute bit. They get one
when they're copied by instal_script in lib-httpd.sh. The exception is
error.sh, but I don't think there's any good reason for it. So probably
not a big deal either way, but another nit. :)

The rest of it all looks quite nice to me.

-Peff