Matthew John Cheetham via GitGitGadget wrote:
> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
> index 608949ea80b..2c49569f675 100644
> --- a/t/lib-httpd.sh
> +++ b/t/lib-httpd.sh
> @@ -137,6 +137,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
>
> 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 0294739a77a..76335cdb24d 100644
> --- a/t/lib-httpd/apache.conf
> +++ b/t/lib-httpd/apache.conf
> @@ -135,6 +135,11 @@ Alias /auth/dumb/ www/auth/dumb/
> SetEnv GIT_HTTP_EXPORT_ALL
> SetEnv GIT_PROTOCOL
> </LocationMatch>
> +<LocationMatch /custom_auth/>
> + SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
> + SetEnv GIT_HTTP_EXPORT_ALL
> + CGIPassAuth on
> +</LocationMatch>
> ScriptAlias /smart/incomplete_length/git-upload-pack incomplete-length-upload-pack-v2-http.sh/
> ScriptAlias /smart/incomplete_body/git-upload-pack incomplete-body-upload-pack-v2-http.sh/
> ScriptAlias /smart/no_report/git-receive-pack error-no-report.sh/
> @@ -144,6 +149,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
This setup (redirecting '/custom_auth/' routes to the 'nph-custom-auth.sh'
script) is nice and straightforward.
> <Directory ${GIT_EXEC_PATH}>
> Options FollowSymlinks
> </Directory>
> diff --git a/t/lib-httpd/nph-custom-auth.sh b/t/lib-httpd/nph-custom-auth.sh
> new file mode 100755
> index 00000000000..8f851aebac4
> --- /dev/null
> +++ b/t/lib-httpd/nph-custom-auth.sh
> @@ -0,0 +1,42 @@
> +#!/bin/sh
> +
> +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")
So there are two cases where you want to return a '200 OK' response:
1. anonymous access is allowed (indicated by $ANONYMOUS_FILE existing)
2. anonymous access is *not* allowed, 'HTTP_AUTHORIZATION' is non-empty, and
it matches at least one line in $VALID_CREDS_FILE
Does the '$' at the end of "^${HTTP_AUTHORIZATION:-nopenopnope}$" need to be
escaped? I'm guessing it doesn't *need* to be based on the fact that the
tests are passing, but it might be safer to escape it anyway.
I see what you're going for with the "nopenopenope" substitution, but I
think you could be more explicit about requiring that 'HTTP_AUTHORIZATION'
is set without the need for a special invalid value fallback:
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@xxxxxxxxxxxxxxxxxxx/
> +then
> + # Note that although git-http-backend returns a status line, it
> + # does so using a CGI 'Status' header. Because this script is an
> + # No Parsed Headers (NPH) script, we must return a real HTTP
> + # 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
I'm not familiar with 'exec', but a cursory look at the documentation shows
that, because this replaces the current shell, it will exit with the code
from 'git-http-backend', so there's no risk of continuing on to print the
'401 Authorization Required' response & challenge handling.
> +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.
> +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"
> +'
> +
> +test_expect_success 'access using basic auth' '
> + test_when_finished "per_test_cleanup" &&
> +
> + set_credential_reply get <<-EOF &&
> + username=alice
> + password=secret-passwd
> + EOF
> +
> + cat >"$HTTPD_ROOT_PATH/custom-auth.valid" <<-EOF &&
> + Basic YWxpY2U6c2VjcmV0LXBhc3N3ZA==
> + EOF
> +
> + cat >"$HTTPD_ROOT_PATH/custom-auth.challenge" <<-EOF &&
> + WWW-Authenticate: Basic realm="example.com"
> + EOF
> +
> + test_config_global credential.helper test-helper &&
> + git ls-remote "$HTTPD_URL/custom_auth/repo.git" &&
> +
> + expect_credential_query get <<-EOF &&
> + protocol=http
> + host=$HTTPD_DEST
> + EOF
> +
> + expect_credential_query store <<-EOF
> + protocol=http
> + host=$HTTPD_DEST
> + username=alice
> + password=secret-passwd
> + EOF
> +'
> +
> +test_done
And these tests properly exercise the custom auth handling.
While I wasn't as opposed to the custom HTTP handler as others that have
commented, I do appreciate the relative simplicity of this new Apache setup
and like that it's still pretty easy to test. Nice work!
